diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 644ebc40..38dc5598 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -1511,7 +1511,7 @@ func runInfraApply(args []string) error { } } } - return syncInfraOutputSecrets(ctx, secretsCfg, secretsProvider, states, wfCfg, envName, runHydrated) + return syncInfraOutputSecrets(ctx, secretsCfg, secretsProvider, states, wfCfg, envName, runHydrated, refreshOutputsFlag) } func runInfraStatus(args []string) error { diff --git a/cmd/wfctl/infra_apply_refresh_flag_test.go b/cmd/wfctl/infra_apply_refresh_flag_test.go index 7965c0e1..3f04d3f8 100644 --- a/cmd/wfctl/infra_apply_refresh_flag_test.go +++ b/cmd/wfctl/infra_apply_refresh_flag_test.go @@ -443,3 +443,155 @@ func TestApply_RefreshOutputsFlag_EnvVarAndFlagBothSet_RunsOnce(t *testing.T) { t.Errorf("refresh should have populated 'id'; got outputs=%v", got.Outputs) } } + +// ── End-to-end: --refresh-outputs updates existing infra_output secrets ──────── + +// writeRefreshFlagTestConfigWithSecrets writes an infra YAML config with: +// - auto_bootstrap disabled +// - filesystem state backend +// - iac.provider + infra.vpc modules +// - secrets block (provider: env) with one infra_output generator whose +// source is "vpc-resource.runner_url" +// +// The generated secret key is RUNNER_URL (no prefix, env provider key == +// env var name). Returns the config path. +func writeRefreshFlagTestConfigWithSecrets(t *testing.T, dir string) string { + t.Helper() + stateDir := filepath.Join(dir, "state") + if err := os.MkdirAll(stateDir, 0o755); err != nil { + t.Fatal(err) + } + cfgPath := filepath.Join(dir, "infra.yaml") + cfg := `infra: + auto_bootstrap: false +secrets: + provider: env + generate: + - key: RUNNER_URL + type: infra_output + source: vpc-resource.runner_url +modules: + - name: cloud-provider + type: iac.provider + config: + provider: fake-provider + - name: state-store + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` + - name: vpc-resource + type: infra.vpc + config: + provider: cloud-provider +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + return cfgPath +} + +// seedVPCStateWithRunnerURL seeds a vpc-resource state entry whose Outputs +// include a runner_url field, simulating a provider whose URL changed. +func seedVPCStateWithRunnerURL(t *testing.T, cfgPath, url string) { + t.Helper() + store, err := resolveStateStore(cfgPath, "") + if err != nil { + t.Fatalf("resolveStateStore: %v", err) + } + entry := interfaces.ResourceState{ + ID: "vpc-resource", + Name: "vpc-resource", + Type: "infra.vpc", + Provider: "fake-provider", + ProviderID: "pid-runner", + Outputs: map[string]any{"runner_url": url}, + } + if err := store.SaveResource(context.Background(), entry); err != nil { + t.Fatalf("seed state: %v", err) + } +} + +// TestApply_RefreshOutputsFlag_UpdatesExistingInfraOutputSecret is the +// end-to-end regression test for the original bug: +// +// Given: +// - RUNNER_URL env-secret already exists with a stale value +// - State contains vpc-resource.runner_url with the new (live) value +// +// When: +// - wfctl infra apply --refresh-outputs --auto-approve is run +// +// Then: +// - RUNNER_URL is updated to the new value (not skipped as "already exists") +// +// This exercises the infra.go:1514 call site — if refreshOutputsFlag were not +// passed through, the secret would be skipped and the assertion would fail. +func TestApply_RefreshOutputsFlag_UpdatesExistingInfraOutputSecret(t *testing.T) { + t.Setenv("WFCTL_REFRESH_OUTPUTS", "") // ensure env-var pre-step does not fire + os.Unsetenv("WFCTL_REFRESH_OUTPUTS") + + dir := t.TempDir() + cfgPath := writeRefreshFlagTestConfigWithSecrets(t, dir) + + // Seed state with the new URL already in place (simulates provider URL change + // having been captured by a previous refresh or initial apply). + const newURL = "https://runner-new.example.com" + const staleURL = "https://runner-old.example.com" + seedVPCStateWithRunnerURL(t, cfgPath, newURL) + + // Pre-populate the secret with the stale value — this is what would happen + // after the initial apply before the provider was re-created. + t.Setenv("RUNNER_URL", staleURL) + + // The fake provider's plan/apply is a no-op; state remains as seeded above. + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "pid-runner": {"runner_url": newURL}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "--refresh-outputs", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply --refresh-outputs: %v", err) + } + + // RUNNER_URL must have been updated to the new value. + if got := os.Getenv("RUNNER_URL"); got != newURL { + t.Errorf("RUNNER_URL: got %q, want %q — secret was not updated by --refresh-outputs", got, newURL) + } +} + +// TestApply_NoRefreshOutputsFlag_DoesNotOverwriteExistingSecret verifies that +// without --refresh-outputs, a pre-existing infra_output secret is NOT +// overwritten by a normal apply (the "already exists — skipped" invariant). +func TestApply_NoRefreshOutputsFlag_DoesNotOverwriteExistingSecret(t *testing.T) { + t.Setenv("WFCTL_REFRESH_OUTPUTS", "") + os.Unsetenv("WFCTL_REFRESH_OUTPUTS") + + dir := t.TempDir() + cfgPath := writeRefreshFlagTestConfigWithSecrets(t, dir) + + const newURL = "https://runner-new.example.com" + const userManagedURL = "https://user-managed.example.com" + seedVPCStateWithRunnerURL(t, cfgPath, newURL) + + // User-managed value that must survive the apply. + t.Setenv("RUNNER_URL", userManagedURL) + + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "pid-runner": {"runner_url": newURL}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply: %v", err) + } + + // Without --refresh-outputs, the existing secret must not be overwritten. + if got := os.Getenv("RUNNER_URL"); got != userManagedURL { + t.Errorf("RUNNER_URL: got %q, want %q — normal apply must not overwrite existing secret", got, userManagedURL) + } +} diff --git a/cmd/wfctl/infra_output_secrets.go b/cmd/wfctl/infra_output_secrets.go index 8e1ab65f..fc1c26b1 100644 --- a/cmd/wfctl/infra_output_secrets.go +++ b/cmd/wfctl/infra_output_secrets.go @@ -112,8 +112,13 @@ func stateKeys(m map[string]map[string]any) []string { } // syncInfraOutputSecrets writes infra_output-typed secrets after a successful -// apply. It skips secrets that already exist in the provider so idempotent -// re-runs never overwrite live values. +// apply. In normal mode (refreshOutputs=false) it skips secrets that already +// exist in the provider so idempotent re-runs never overwrite live values. +// When refreshOutputs=true (operator-opt-in via --refresh-outputs), existing +// secrets are reconciled: updated if the value changed, logged as "unchanged" +// if the value is the same (readable providers only), or always overwritten for +// write-only providers (e.g. GitHub Actions) where comparison is impossible. +// // wfCfg and envName are used to resolve source module names through per-env // overrides so that "bmw-database.uri" finds "bmw-staging-db" in state when // --env staging renames the module. @@ -125,7 +130,7 @@ func stateKeys(m map[string]map[string]any) []string { // without going through provider.Get (which is unsupported on write-only // providers like GitHub Actions). May be nil for callers that don't // have a same-process apply hand-off (e.g., wfctl infra outputs CLI). -func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, provider secrets.Provider, states []interfaces.ResourceState, wfCfg *config.WorkflowConfig, envName string, hydrated map[string]string) error { +func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, provider secrets.Provider, states []interfaces.ResourceState, wfCfg *config.WorkflowConfig, envName string, hydrated map[string]string, refreshOutputs bool) error { if secretsCfg == nil { return nil } @@ -162,40 +167,61 @@ func syncInfraOutputSecrets(ctx context.Context, secretsCfg *SecretsConfig, prov _, ok := listSet[key] return ok, nil } - secretExists := func(key string) (bool, error) { - _, err := provider.Get(ctx, key) - switch { - case err == nil: - return true, nil - case errors.Is(err, secrets.ErrNotFound): - return false, nil - case errors.Is(err, secrets.ErrUnsupported): - return lookupViaList(key) - default: - return false, fmt.Errorf("check secret %q: %w", key, err) - } - } stateOutputs := buildStateOutputsMap(states) for _, gen := range gens { - exists, err := secretExists(gen.Key) - if err != nil { - return err + // Attempt to read the current value. This serves two purposes: + // 1. Existence check for readable providers. + // 2. Value comparison in refresh mode to avoid spurious updates. + currentVal, getErr := provider.Get(ctx, gen.Key) + + var exists bool + var isReadable bool + switch { + case getErr == nil: + exists = true + isReadable = true + case errors.Is(getErr, secrets.ErrNotFound): + exists = false + case errors.Is(getErr, secrets.ErrUnsupported): + // Write-only provider (e.g. GitHub Actions): fall back to List. + var listLookupErr error + exists, listLookupErr = lookupViaList(gen.Key) + if listLookupErr != nil { + return listLookupErr + } + default: + return fmt.Errorf("check secret %q: %w", gen.Key, getErr) } - if exists { + + if exists && !refreshOutputs { fmt.Printf(" secret %q: already exists — skipped\n", gen.Key) continue } - value, err := resolveInfraOutput(wfCfg, gen.Source, envName, stateOutputs, hydrated) - if err != nil { - return fmt.Errorf("generate infra_output secret %q: %w", gen.Key, err) + newValue, resolveErr := resolveInfraOutput(wfCfg, gen.Source, envName, stateOutputs, hydrated) + if resolveErr != nil { + return fmt.Errorf("generate infra_output secret %q: %w", gen.Key, resolveErr) } - if err := provider.Set(ctx, gen.Key, value); err != nil { - return fmt.Errorf("store secret %q: %w", gen.Key, err) + + if exists { + // refreshOutputs is true here (guarded by the continue above). + // For readable providers skip the Set when the value is unchanged. + if isReadable && currentVal == newValue { + fmt.Printf(" secret %q: unchanged\n", gen.Key) + continue + } + if err := provider.Set(ctx, gen.Key, newValue); err != nil { + return fmt.Errorf("store secret %q: %w", gen.Key, err) + } + fmt.Printf(" secret %q: updated from infra output\n", gen.Key) + } else { + if err := provider.Set(ctx, gen.Key, newValue); err != nil { + return fmt.Errorf("store secret %q: %w", gen.Key, err) + } + fmt.Printf(" secret %q: created from infra output\n", gen.Key) } - fmt.Printf(" secret %q: created from infra output\n", gen.Key) } return nil } diff --git a/cmd/wfctl/infra_output_secrets_test.go b/cmd/wfctl/infra_output_secrets_test.go index f6124c91..e255f85f 100644 --- a/cmd/wfctl/infra_output_secrets_test.go +++ b/cmd/wfctl/infra_output_secrets_test.go @@ -16,6 +16,19 @@ type simpleProvider struct { data map[string]string } +// setCountProvider wraps simpleProvider and records every Set call. Use it +// when a test needs to assert that Set was or was not called a specific +// number of times (e.g. the "unchanged" optimisation path). +type setCountProvider struct { + simpleProvider + setCalls int +} + +func (p *setCountProvider) Set(ctx context.Context, key, val string) error { + p.setCalls++ + return p.simpleProvider.Set(ctx, key, val) +} + func newSimpleProvider() *simpleProvider { return &simpleProvider{data: map[string]string{}} } @@ -109,7 +122,7 @@ func TestBuildStateOutputsMap_Empty(t *testing.T) { func TestSyncInfraOutputSecrets_NilConfig(t *testing.T) { p := newSimpleProvider() - err := syncInfraOutputSecrets(context.Background(), nil, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), nil, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("nil config should be no-op: %v", err) } @@ -125,7 +138,7 @@ func TestSyncInfraOutputSecrets_NoInfraOutputGens(t *testing.T) { {Key: "JWT_SECRET", Type: "random_hex", Length: 32}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("no infra_output generators should be no-op: %v", err) } @@ -141,7 +154,7 @@ func TestSyncInfraOutputSecrets_WritesSecret(t *testing.T) { {Key: "STAGING_DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("syncInfraOutputSecrets: %v", err) } @@ -158,7 +171,7 @@ func TestSyncInfraOutputSecrets_WritesMultiple(t *testing.T) { {Key: "REDIS_URL", Type: "infra_output", Source: "bmw-cache.url"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("syncInfraOutputSecrets: %v", err) } @@ -178,7 +191,7 @@ func TestSyncInfraOutputSecrets_SkipsExisting(t *testing.T) { {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("syncInfraOutputSecrets: %v", err) } @@ -198,7 +211,7 @@ func TestSyncInfraOutputSecrets_WriteOnlyProviderSkips(t *testing.T) { {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("syncInfraOutputSecrets: %v", err) } @@ -217,7 +230,7 @@ func TestSyncInfraOutputSecrets_WriteOnlyProviderWrites(t *testing.T) { {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err != nil { t.Fatalf("syncInfraOutputSecrets: %v", err) } @@ -233,7 +246,7 @@ func TestSyncInfraOutputSecrets_MissingModule(t *testing.T) { {Key: "X", Type: "infra_output", Source: "nonexistent.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) if err == nil { t.Fatal("expected error for missing module in state") } @@ -246,12 +259,123 @@ func TestSyncInfraOutputSecrets_EmptyStates(t *testing.T) { {Key: "X", Type: "infra_output", Source: "bmw-database.uri"}, }, } - err := syncInfraOutputSecrets(context.Background(), cfg, p, nil, nil, "", nil) + err := syncInfraOutputSecrets(context.Background(), cfg, p, nil, nil, "", nil, false) if err == nil { t.Fatal("expected error when state has no matching module") } } +// ── syncInfraOutputSecrets --refresh-outputs mode ───────────────────────────── + +// TestSyncInfraOutputSecrets_RefreshOutputs_UpdatesStaleSecret verifies that +// when refreshOutputs=true and the existing secret value differs from the +// current infra output, the secret is updated (not skipped). +func TestSyncInfraOutputSecrets_RefreshOutputs_UpdatesStaleSecret(t *testing.T) { + p := newSimpleProvider() + p.data["DATABASE_URL"] = "postgres://old-host:5432/app" // stale value + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, + }, + } + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, true) + if err != nil { + t.Fatalf("syncInfraOutputSecrets: %v", err) + } + // Must overwrite stale value with the current infra output. + if p.data["DATABASE_URL"] != "postgres://user:pass@db.example.com:5432/app" { + t.Errorf("DATABASE_URL: got %q, want current infra output", p.data["DATABASE_URL"]) + } +} + +// TestSyncInfraOutputSecrets_RefreshOutputs_UnchangedSkipsSet verifies that +// when refreshOutputs=true and the existing secret already matches the infra +// output, provider.Set is NOT called — the "unchanged" optimisation must not +// be masked by a write of the same value. +func TestSyncInfraOutputSecrets_RefreshOutputs_UnchangedSkipsSet(t *testing.T) { + current := "postgres://user:pass@db.example.com:5432/app" + // setCountProvider records how many times Set is called. If the + // "unchanged" path fires, the count must be 0. + p := &setCountProvider{simpleProvider: simpleProvider{data: map[string]string{"DATABASE_URL": current}}} + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, + }, + } + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, true) + if err != nil { + t.Fatalf("syncInfraOutputSecrets: %v", err) + } + if p.setCalls != 0 { + t.Errorf("Set must not be called when value is unchanged; called %d time(s)", p.setCalls) + } + if p.data["DATABASE_URL"] != current { + t.Errorf("value must remain unchanged: got %q", p.data["DATABASE_URL"]) + } +} + +// TestSyncInfraOutputSecrets_RefreshOutputs_CreatesNew verifies that when +// refreshOutputs=true and the secret does not yet exist, it is created (same +// as normal mode). +func TestSyncInfraOutputSecrets_RefreshOutputs_CreatesNew(t *testing.T) { + p := newSimpleProvider() + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, + }, + } + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, true) + if err != nil { + t.Fatalf("syncInfraOutputSecrets: %v", err) + } + if p.data["DATABASE_URL"] != "postgres://user:pass@db.example.com:5432/app" { + t.Errorf("DATABASE_URL: got %q", p.data["DATABASE_URL"]) + } +} + +// TestSyncInfraOutputSecrets_RefreshOutputs_WriteOnlyProviderUpdates verifies +// that for write-only providers (GitHub Actions style), --refresh-outputs +// always overwrites an existing secret because comparison is not possible. +func TestSyncInfraOutputSecrets_RefreshOutputs_WriteOnlyProviderUpdates(t *testing.T) { + p := &writeOnlyProvider{ + existing: []string{"DATABASE_URL"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, + }, + } + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, true) + if err != nil { + t.Fatalf("syncInfraOutputSecrets: %v", err) + } + // Write-only provider cannot compare, so Set must have been called. + if p.stored["DATABASE_URL"] != "postgres://user:pass@db.example.com:5432/app" { + t.Errorf("DATABASE_URL: got %q, want infra output value", p.stored["DATABASE_URL"]) + } +} + +// TestSyncInfraOutputSecrets_NormalMode_DoesNotOverwrite verifies the +// invariant that normal apply (refreshOutputs=false) never overwrites a +// user-managed secret, even when the infra output value has changed. +func TestSyncInfraOutputSecrets_NormalMode_DoesNotOverwrite(t *testing.T) { + p := newSimpleProvider() + p.data["DATABASE_URL"] = "user-managed-value" + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "DATABASE_URL", Type: "infra_output", Source: "bmw-database.uri"}, + }, + } + err := syncInfraOutputSecrets(context.Background(), cfg, p, sampleStates(), nil, "", nil, false) + if err != nil { + t.Fatalf("syncInfraOutputSecrets: %v", err) + } + if p.data["DATABASE_URL"] != "user-managed-value" { + t.Errorf("normal apply must not overwrite existing secret: got %q", p.data["DATABASE_URL"]) + } +} + // ── bootstrapSecrets skips infra_output ─────────────────────────────────────── func TestBootstrapSecrets_SkipsInfraOutputGens(t *testing.T) {