diff --git a/CHANGELOG.md b/CHANGELOG.md index e01b8e5..5f59171 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 age survives a tag-read failure and only falls back to the agent's own start time when IMDS is also unavailable. The fallback source is now labelled inline when it isn't the authoritative tag. +- **Two lifecycle tags asserted things about an instance that weren't + true** (#515). `spawn:version` was hard-coded to the literal `"0.1.0"` in + `buildTags`, so every instance spawn has ever launched carried that value + regardless of the actual running spawn — worse than an absent tag, since + it looks like an answer to "was this instance launched by a spawn that + predates fix X?" and always gives the wrong one. It now reflects the + actual launching spawn's resolved version (`LaunchConfig.SpawnVersion`, + pushed in once from `cmd.version()`/`pkg/buildinfo` via a new + `aws.CallerVersion` seam — `pkg/aws` can't import `cmd` directly) and is + omitted, not written as a false placeholder, when a caller doesn't supply + one. Separately, `spawn:completion-file` was written whenever + `--completion-file` was non-empty, but that flag has a non-empty default + (`/tmp/SPAWN_COMPLETE`) — so a launch that never passed `--on-complete` + still got a completion-file tag describing a watch with no action attached. + The two tags are now written atomically: `spawn:completion-file` only + alongside `spawn:on-complete` (spored's own config load already defaults + the file path when `on-complete` is set but no file was tagged, so nothing + regresses for a caller who *did* ask for completion handling). ## [0.100.2] - 2026-08-18 diff --git a/cmd/root.go b/cmd/root.go index 87efe35..51f249b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/cobra" "github.com/spore-host/libs/i18n" "github.com/spore-host/libs/update" + "github.com/spore-host/spawn/pkg/aws" "github.com/spore-host/spawn/pkg/buildinfo" spawnconfig "github.com/spore-host/spawn/pkg/config" ) @@ -60,6 +61,12 @@ func Execute() { _ = rootCmd.ParseFlags(os.Args[1:]) ensureI18nInitialized() + // So every launched instance's spawn:version tag reflects the ACTUAL + // running spawn, not a restated literal (spawn#515) — pkg/aws can't + // import cmd to resolve this itself, so it's pushed in here once at + // startup. + aws.CallerVersion = version() + // Start async update check (non-blocking, respects SPORE_NO_UPDATE_CHECK). updateCh := startUpdateCheck(version()) diff --git a/pkg/aws/client.go b/pkg/aws/client.go index f30364d..9b78638 100644 --- a/pkg/aws/client.go +++ b/pkg/aws/client.go @@ -39,6 +39,18 @@ type Client struct { cfg aws.Config } +// CallerVersion is the resolved version of the launching spawn binary, +// written to every launched instance's spawn:version tag unless a caller +// sets LaunchConfig.SpawnVersion explicitly. The cmd package sets this once +// at startup (cmd.version(), which itself resolves via pkg/buildinfo) — +// pkg/aws can't import cmd to call that directly without a cycle, so this is +// the seam. Left empty by an SDK caller that never sets it, buildTags omits +// the tag rather than writing a hardcoded placeholder (spawn#515 — the +// literal "0.1.0" this replaces was worse than no tag at all, since it +// looked like an answer to "was this launched by a spawn that predates fix +// X?" and always gave the wrong one). +var CallerVersion = "" + // NewClient creates a Client using the default AWS credential chain. // Use [NewClientFromConfig] in tests to inject a pre-configured aws.Config. func NewClient(ctx context.Context) (*Client, error) { @@ -271,6 +283,17 @@ type LaunchConfig struct { Name string Tags map[string]string + // SpawnVersion is the resolved version of the launching spawn binary + // (cmd.version(), i.e. buildinfo.Version(cmd.Version)) — written to + // spawn:version so a launched instance's tags reflect the ACTUAL spawn + // that created it, not a restated literal (spawn#515: buildTags used to + // hard-code "0.1.0" here regardless of the real version, on every + // instance spawn has ever launched). Empty is tolerated (older/SDK + // callers that don't set it): buildTags simply omits the tag rather than + // writing a false value, since pkg/aws cannot import cmd to resolve it + // itself. + SpawnVersion string + // TargetOS is the operating system of the instance ("windows" or "linux"; // "" = treated as linux). Set at launch from --os or AMI auto-detection // (IsWindowsAMI) and written as the spawn:os tag so connect and the @@ -340,6 +363,10 @@ func (c *Client) Launch(ctx context.Context, launchConfig LaunchConfig) (*Launch // Update config for region ec2Client := c.regionalEC2(launchConfig.Region) + if launchConfig.SpawnVersion == "" { + launchConfig.SpawnVersion = CallerVersion + } + // Get caller identity for per-user isolation tagging accountID, userARN, err := c.GetCallerIdentityInfo(ctx) if err != nil { diff --git a/pkg/aws/tags.go b/pkg/aws/tags.go index c2d2ae6..e618c32 100644 --- a/pkg/aws/tags.go +++ b/pkg/aws/tags.go @@ -47,12 +47,23 @@ func buildTags(config LaunchConfig, accountID, userARN, accountNameSlug string) {Key: aws.String("spawn:managed"), Value: aws.String("true")}, {Key: aws.String("spawn:root"), Value: aws.String("true")}, {Key: aws.String("spawn:created-by"), Value: aws.String("spawn")}, - {Key: aws.String("spawn:version"), Value: aws.String("0.1.0")}, {Key: aws.String("spawn:account-id"), Value: aws.String(accountID)}, {Key: aws.String("spawn:account-base36"), Value: aws.String(accountBase36)}, {Key: aws.String("spawn:iam-user"), Value: aws.String(userARN)}, // Per-user isolation } + // spawn:version records the ACTUAL launching spawn's version (config.SpawnVersion, + // set by the CLI from cmd.version()/buildinfo — pkg/aws can't import cmd to + // resolve it itself). Previously hard-coded to the literal "0.1.0" regardless + // of the real version, on every instance spawn has ever launched (spawn#515) — + // worse than an absent tag, since it looks like an answer to "was this + // instance launched by a spawn that predates fix X?" and gives the wrong one. + // Omitted (not written as a false placeholder) when the caller doesn't supply + // it, e.g. an older SDK integration. + if config.SpawnVersion != "" { + tags = append(tags, types.Tag{Key: aws.String("spawn:version"), Value: aws.String(config.SpawnVersion)}) + } + // Friendly account-name DNS segment, when the account has one and it // slugifies to a valid DNS label (#121). base36 stays canonical (it's always // valid and unique); the name is an alias the DNS updater can prefer for a @@ -175,13 +186,19 @@ func buildTags(config LaunchConfig, accountID, userARN, accountNameSlug string) tags = append(tags, types.Tag{Key: aws.String("spawn:hibernate-on-idle"), Value: aws.String("true")}) } - // Completion signal settings + // Completion signal settings. spawn:completion-file is gated on OnComplete + // being set too, not just CompletionFile being non-empty: --completion-file + // has a non-empty flag default ("/tmp/SPAWN_COMPLETE", cmd/launch_flags.go), + // so a launch that never asked for --on-complete previously still got a + // spawn:completion-file tag — a watch with no action attached. That reads, + // to an operator or anything summarizing tags, as "this instance has a + // completion path" when touching the file does nothing (spawn#515). The + // pair is atomic: a completion file is meaningless without an action. if config.OnComplete != "" { tags = append(tags, types.Tag{Key: aws.String("spawn:on-complete"), Value: aws.String(config.OnComplete)}) - } - - if config.CompletionFile != "" { - tags = append(tags, types.Tag{Key: aws.String("spawn:completion-file"), Value: aws.String(config.CompletionFile)}) + if config.CompletionFile != "" { + tags = append(tags, types.Tag{Key: aws.String("spawn:completion-file"), Value: aws.String(config.CompletionFile)}) + } } if config.CompletionDelay != "" { diff --git a/pkg/aws/tags_test.go b/pkg/aws/tags_test.go index becb6b6..c86cd22 100644 --- a/pkg/aws/tags_test.go +++ b/pkg/aws/tags_test.go @@ -311,3 +311,87 @@ func TestSlugifyDNSLabel_MaxLength(t *testing.T) { t.Errorf("len = %d, want 63 (DNS label max)", len(got)) } } + +// TestBuildTags_VersionIsNotHardcoded is the spawn#515 regression test for +// defect 1: spawn:version was a hardcoded literal ("0.1.0") regardless of the +// actual running spawn's version, on every instance spawn has ever launched. +// It must now reflect config.SpawnVersion (set by the CLI from +// cmd.version()/buildinfo, pkg/aws can't resolve that itself) rather than a +// restated constant, and be OMITTED (not written as a false placeholder) +// when the caller doesn't supply one. +func TestBuildTags_VersionIsNotHardcoded(t *testing.T) { + t.Run("caller-supplied version is written verbatim", func(t *testing.T) { + tags := buildTags(LaunchConfig{Name: "t", SpawnVersion: "0.100.2"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + if got := findTagValue(tags, "spawn:version"); got != "0.100.2" { + t.Errorf("spawn:version = %q, want 0.100.2 (must reflect the actual caller, not a hardcoded literal)", got) + } + }) + + t.Run("no caller-supplied version => tag omitted, not a false placeholder", func(t *testing.T) { + tags := buildTags(LaunchConfig{Name: "t"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + found := false + for _, tg := range tags { + if tg.Key != nil && *tg.Key == "spawn:version" { + found = true + } + } + if found { + t.Errorf("spawn:version should be omitted when SpawnVersion is unset, got %q", findTagValue(tags, "spawn:version")) + } + }) + + t.Run("never the old hardcoded literal", func(t *testing.T) { + tags := buildTags(LaunchConfig{Name: "t", SpawnVersion: "0.100.2"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + if got := findTagValue(tags, "spawn:version"); got == "0.1.0" { + t.Error("spawn:version must never be the old hardcoded literal 0.1.0") + } + }) +} + +// TestBuildTags_CompletionFileRequiresOnComplete is the spawn#515 regression +// test for defect 2: --completion-file has a non-empty flag default +// ("/tmp/SPAWN_COMPLETE"), so a launch that never asked for --on-complete +// still got a spawn:completion-file tag under the old unconditional `if +// config.CompletionFile != ""` — a watch with no action attached, which reads +// as "this instance has a completion path" when touching the file does +// nothing. The two tags must be written atomically: completion-file only +// alongside on-complete. +func TestBuildTags_CompletionFileRequiresOnComplete(t *testing.T) { + t.Run("CompletionFile without OnComplete => neither tag written", func(t *testing.T) { + // Models the real-world shape: --completion-file's flag default is + // always non-empty even when --on-complete was never passed. + tags := buildTags(LaunchConfig{Name: "t", CompletionFile: "/tmp/SPAWN_COMPLETE"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + if got := findTagValue(tags, "spawn:completion-file"); got != "" { + t.Errorf("spawn:completion-file = %q, want empty (no spawn:on-complete to act on it)", got) + } + if got := findTagValue(tags, "spawn:on-complete"); got != "" { + t.Errorf("spawn:on-complete = %q, want empty", got) + } + }) + + t.Run("OnComplete + CompletionFile => both tags written together", func(t *testing.T) { + tags := buildTags(LaunchConfig{Name: "t", OnComplete: "terminate", CompletionFile: "/tmp/SPAWN_COMPLETE"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + if got := findTagValue(tags, "spawn:completion-file"); got != "/tmp/SPAWN_COMPLETE" { + t.Errorf("spawn:completion-file = %q, want /tmp/SPAWN_COMPLETE", got) + } + if got := findTagValue(tags, "spawn:on-complete"); got != "terminate" { + t.Errorf("spawn:on-complete = %q, want terminate", got) + } + }) + + t.Run("OnComplete without CompletionFile => only on-complete written (spored defaults the file itself)", func(t *testing.T) { + tags := buildTags(LaunchConfig{Name: "t", OnComplete: "terminate"}, + "123456789012", "arn:aws:iam::123456789012:user/test", "") + if got := findTagValue(tags, "spawn:on-complete"); got != "terminate" { + t.Errorf("spawn:on-complete = %q, want terminate", got) + } + if got := findTagValue(tags, "spawn:completion-file"); got != "" { + t.Errorf("spawn:completion-file = %q, want empty (spored's own config load defaults it when on-complete is set, pkg/provider/ec2.go)", got) + } + }) +}