diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 3244deb5b..26d85fc8e 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -14,6 +14,9 @@ permissions: env: GOPRIVATE: github.com/GoCodeAlone/* GONOSUMCHECK: github.com/GoCodeAlone/* + # See ci.yml for rationale. Force in-memory diffcache in CI per the + # T3.5 rev3 lifecycle constraint. + WFCTL_DIFFCACHE: ":memory:" jobs: benchmark: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dcb275b51..6e3aa8831 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,12 @@ permissions: env: GOPRIVATE: github.com/GoCodeAlone/* GONOSUMCHECK: github.com/GoCodeAlone/* + # CI runners are ephemeral; the iac/diffcache filesystem cache would + # cold-miss every job and waste a few ms on disk I/O. Force in-memory + # mode per the T3.5 rev3 lifecycle constraint — no filesystem writes + # in containerized runners. Operators running wfctl locally see the + # default filesystem cache at ~/.cache/wfctl/diff/. + WFCTL_DIFFCACHE: ":memory:" jobs: test: diff --git a/.github/workflows/dependency-update.yml b/.github/workflows/dependency-update.yml index 73c3d9385..407a0f1d3 100644 --- a/.github/workflows/dependency-update.yml +++ b/.github/workflows/dependency-update.yml @@ -10,6 +10,12 @@ on: - cron: '0 9 * * 1' workflow_dispatch: +env: + # See ci.yml for rationale. Force in-memory diffcache in CI per the + # T3.5 rev3 lifecycle constraint — no filesystem writes from this + # workflow's `go test ./...` step. + WFCTL_DIFFCACHE: ":memory:" + jobs: update-dependencies: name: Update Go Dependencies diff --git a/.github/workflows/pre-release.yml b/.github/workflows/pre-release.yml index 0f0b41ffb..cb913cb5e 100644 --- a/.github/workflows/pre-release.yml +++ b/.github/workflows/pre-release.yml @@ -15,6 +15,9 @@ permissions: env: GOPRIVATE: github.com/GoCodeAlone/* GONOSUMCHECK: github.com/GoCodeAlone/* + # See ci.yml for rationale. Force in-memory diffcache in CI per the + # T3.5 rev3 lifecycle constraint. + WFCTL_DIFFCACHE: ":memory:" jobs: test: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 66b2d5ab9..db345899f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -22,6 +22,9 @@ env: GOPRIVATE: github.com/GoCodeAlone/* GONOSUMCHECK: github.com/GoCodeAlone/* TAG_NAME: ${{ inputs.tag_name || github.ref_name }} + # See ci.yml for rationale. Force in-memory diffcache in CI per the + # T3.5 rev3 lifecycle constraint. + WFCTL_DIFFCACHE: ":memory:" jobs: test: diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 7582baba6..45129b262 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -46,6 +46,8 @@ func runInfra(args []string) error { return runInfraBootstrap(args[1:]) case "outputs": return runInfraOutputs(args[1:]) + case "refresh-outputs": + return runInfraRefreshOutputs(args[1:]) case "align": return runInfraAlign(args[1:]) case "security-check": @@ -69,6 +71,7 @@ Actions: import Import an existing cloud resource into state state Manage IaC state (list, export, import) outputs Print captured resource outputs from state + refresh-outputs Read live outputs and reconcile state (no cloud writes) align Validate IaC config + plan alignment (8 rule families) security-check Scan plan.json for security policy violations @@ -972,6 +975,8 @@ func runInfraApply(args []string) error { fs.BoolVar(&refreshFlag, "refresh", false, "Detect drift and prune ghost-in-state entries before applying") var allowProtectedPruneFlag bool fs.BoolVar(&allowProtectedPruneFlag, "allow-protected-prune", false, "Allow pruning state entries for resources marked protected: true (requires --refresh)") + var skipRefreshFlag bool + fs.BoolVar(&skipRefreshFlag, "skip-refresh", false, "Skip the WFCTL_REFRESH_OUTPUTS pre-step refresh even if the env var is set") autoApprove := &autoApproveVal showSensitive := showSensitiveVal if err := fs.Parse(args); err != nil { @@ -1074,6 +1079,18 @@ func runInfraApply(args []string) error { } } + // WFCTL_REFRESH_OUTPUTS pre-step (T2.3): when opted in, read live + // Outputs from each provider and persist any field-level changes + // before computing the plan, so apply doesn't make decisions on + // stale state. Default off; --skip-refresh always wins. Only + // applicable for infra.* configs (legacy platform.* path doesn't + // flow through iac/refreshoutputs). + if applyPreStepRefreshEnabled(skipRefreshFlag) && hasInfraModules(cfgFile) { + if err := applyPreStepRefreshOutputs(ctx, cfgFile, envName, os.Stdout); err != nil { + return fmt.Errorf("apply pre-step refresh-outputs: %w", err) + } + } + fmt.Printf("Applying infrastructure from %s...\n", cfgFile) // --plan: dispatch actions from a pre-emitted plan file, skipping ComputePlan. diff --git a/cmd/wfctl/infra_apply.go b/cmd/wfctl/infra_apply.go index e1d7dd74c..94a32b93e 100644 --- a/cmd/wfctl/infra_apply.go +++ b/cmd/wfctl/infra_apply.go @@ -14,10 +14,32 @@ import ( "time" "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" "github.com/GoCodeAlone/workflow/interfaces" "github.com/GoCodeAlone/workflow/platform" ) +// printDriftReportIfAny writes the canonical FormatStaleError output to w +// when result.InputDriftReport is non-empty. Safe to call when result is +// nil or InputDriftReport is empty/nil — both yield a no-op so callers +// don't need to defensively check the field before calling. +// +// Wired in by W-3a/T3.1.5 as a standalone helper; the actual call site in +// applyWithProviderAndStore (or its successor) lands when W-3b/T3.7 +// switches the in-process apply path through wfctlhelpers.ApplyPlan for +// v2 plugins. Until then this helper is exercised solely by the +// in-process drift test, NOT yet by any production caller — preserving +// the W-3a "zero runtime change for v1 plugins" invariant. +func printDriftReportIfAny(w io.Writer, result *interfaces.ApplyResult) { + if result == nil || len(result.InputDriftReport) == 0 { + return + } + // FormatStaleError emits the multi-line "plan stale: N input(s) changed" + // header + per-key fingerprint diff + trailing hint. Println adds the + // terminating newline so the next stderr line is not glued to the hint. + fmt.Fprintln(w, inputsnapshot.FormatStaleError(result.InputDriftReport)) +} + // infraApplyTroubleshootTimeout is the budget for a Troubleshoot call when // infra apply fails. Kept separate so tests can override it. var infraApplyTroubleshootTimeout = 30 * time.Second diff --git a/cmd/wfctl/infra_apply_in_process_test.go b/cmd/wfctl/infra_apply_in_process_test.go new file mode 100644 index 000000000..d45e95fd3 --- /dev/null +++ b/cmd/wfctl/infra_apply_in_process_test.go @@ -0,0 +1,127 @@ +package main + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// fingerprintForTest is provided by infra_apply_plan_test.go in this +// package — it delegates to inputsnapshot.Compute so the production +// fingerprint algorithm is always used. Reused here to keep the +// in-process test consistent with the persisted-plan test. + +// inProcessFakeProvider satisfies interfaces.IaCProvider with no-op +// methods — TestApply_InProcess_PlanStaleDiagnostic_NamesChangedKeys +// exercises wfctlhelpers.ApplyPlan's drift postcondition independently +// of any per-action driver behavior. +type inProcessFakeProvider struct{} + +func (inProcessFakeProvider) Name() string { return "in-process-fake" } +func (inProcessFakeProvider) Version() string { return "0.0.0" } +func (inProcessFakeProvider) Initialize(_ context.Context, _ map[string]any) error { return nil } +func (inProcessFakeProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } +func (inProcessFakeProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return &interfaces.IaCPlan{}, nil +} +func (inProcessFakeProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return &interfaces.ApplyResult{}, nil +} +func (inProcessFakeProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (inProcessFakeProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (inProcessFakeProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (inProcessFakeProvider) Import(_ context.Context, _ string, _ string) (*interfaces.ResourceState, error) { + return nil, nil +} +func (inProcessFakeProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (inProcessFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return nil, nil +} +func (inProcessFakeProvider) SupportedCanonicalKeys() []string { return nil } +func (inProcessFakeProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (inProcessFakeProvider) Close() error { return nil } + +// TestApply_InProcess_PlanStaleDiagnostic_NamesChangedKeys validates the +// full T3.1.5 chain: plan-time env value → wfctlhelpers.ApplyPlan +// captures InitialInputSnapshot at entry, then the deferred postcondition +// computes drift against the apply-time snapshot, populating +// result.InputDriftReport. cmd/wfctl/printDriftReportIfAny then formats +// the report onto stderr in the canonical FormatStaleError shape. +// +// This is the in-process counterpart to the persisted-`--plan` path +// covered by cmd/wfctl/infra.go (W-1/T1.5). Once W-3b/T3.7 wires +// wfctlhelpers.ApplyPlan into the in-process apply call site, this test +// exercises exactly the path operators run during `wfctl infra apply` +// against a v2-conformant IaCProvider plugin. +func TestApply_InProcess_PlanStaleDiagnostic_NamesChangedKeys(t *testing.T) { + const varName = "WFCTL_T315_INPROC_PASSWORD" + planFP := fingerprintForTest("old-value") + t.Setenv(varName, "new-value") // post-plan env value differs from plan-time fingerprint + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{varName: planFP}, + } + result, err := wfctlhelpers.ApplyPlan(context.Background(), inProcessFakeProvider{}, plan) + if err != nil { + t.Fatalf("ApplyPlan returned top-level error: %v", err) + } + // Postcondition assertion: exactly one entry naming the changed key. + if len(result.InputDriftReport) != 1 { + t.Fatalf("expected 1 drift entry; got %d (%+v)", + len(result.InputDriftReport), result.InputDriftReport) + } + if got := result.InputDriftReport[0].Name; got != varName { + t.Errorf("expected drift on %q; got %q", varName, got) + } + + // cmd/wfctl wiring assertion: printDriftReportIfAny formats the + // report onto the supplied writer in the canonical shape. + var buf bytes.Buffer + printDriftReportIfAny(&buf, result) + out := buf.String() + if !strings.Contains(out, "plan stale:") { + t.Errorf("expected canonical stale-error header in output; got %q", out) + } + if !strings.Contains(out, varName) { + t.Errorf("expected drift output to name %q; got %q", varName, out) + } + if !strings.Contains(out, "hint: ensure all env vars referenced") { + t.Errorf("expected canonical hint line in output; got %q", out) + } +} + +// TestPrintDriftReportIfAny_NoOpOnEmpty locks the no-op contract so +// callers don't need to guard the call: nil result, nil report, and +// empty-but-non-nil report all produce zero output. +func TestPrintDriftReportIfAny_NoOpOnEmpty(t *testing.T) { + cases := []struct { + name string + result *interfaces.ApplyResult + }{ + {"nil-result", nil}, + {"nil-report", &interfaces.ApplyResult{}}, + {"empty-report", &interfaces.ApplyResult{InputDriftReport: []interfaces.DriftEntry{}}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var buf bytes.Buffer + printDriftReportIfAny(&buf, c.result) + if buf.Len() != 0 { + t.Errorf("expected no output for %s; got %q", c.name, buf.String()) + } + }) + } +} diff --git a/cmd/wfctl/infra_apply_refresh_pre.go b/cmd/wfctl/infra_apply_refresh_pre.go new file mode 100644 index 000000000..b5c63d466 --- /dev/null +++ b/cmd/wfctl/infra_apply_refresh_pre.go @@ -0,0 +1,99 @@ +package main + +import ( + "context" + "fmt" + "io" + "os" + "strconv" +) + +// applyPreStepRefreshOutputsEnvVar is the environment variable that opts in +// to running iac/refreshoutputs.Refresh as a pre-step before apply computes +// its plan. The default is OFF so apply behaves identically to pre-W-2 for +// every operator who doesn't set it. +// +// The value is parsed with strconv.ParseBool: "1", "t", "T", "TRUE", +// "true", "True" enable; "0", "f", "F", "FALSE", "false", "False" disable; +// unset, empty, or unrecognised values disable. Operators who use the +// "0" / "false" convention to turn features off therefore get the +// expected behaviour rather than the off-by-one foot-gun a presence-only +// toggle would produce. +const applyPreStepRefreshOutputsEnvVar = "WFCTL_REFRESH_OUTPUTS" + +// applyPreStepRefreshOutputs runs the read-only output refresh against +// every iac.provider declared in cfgFile and persists any state entries +// whose Outputs changed. It is the apply-time counterpart to wfctl infra +// refresh-outputs (T2.2): same helpers, same error semantics. On any Read +// or driver-resolution failure, it returns the wrapped error so apply +// aborts before computing a plan against stale outputs. +// +// This function exists as its own file so reverting commit +// "feat(iac): apply-time refresh-outputs pre-step ..." removes the entire +// code path in one operation. The caller in runInfraApply gates this on +// three conditions, all of which must be true for the pre-step to fire: +// +// - applyPreStepRefreshEnabled returns true (WFCTL_REFRESH_OUTPUTS +// parses to true and --skip-refresh was not passed). +// - hasInfraModules(cfgFile) is true (legacy platform.* configs are +// skipped — they don't flow through iac/refreshoutputs). +// +// The helper itself is purely additive on top of those gates: it never +// short-circuits on values it would otherwise accept, so a caller that +// has already decided to refresh can rely on the helper to do exactly +// that. +func applyPreStepRefreshOutputs(ctx context.Context, cfgFile, envName string, stdout io.Writer) error { + providerDefs, err := discoverIaCProvidersForRefresh(cfgFile, envName) + if err != nil { + return err + } + if len(providerDefs) == 0 { + // No provider for this env — log and return nil rather than + // aborting apply: the downstream applyInfraModules call will + // produce a more actionable error if a provider was actually + // expected. The log line makes the no-op visible to operators + // who explicitly opted in. + fmt.Fprintln(stdout, "Refresh pre-step: no iac.provider modules in config; skipping.") + return nil + } + + states, err := loadCurrentState(cfgFile, envName) + if err != nil { + return fmt.Errorf("load current state: %w", err) + } + if len(states) == 0 { + return nil + } + + store, err := resolveStateStore(cfgFile, envName) + if err != nil { + return fmt.Errorf("open state store: %w", err) + } + + fmt.Fprintln(stdout, "Refreshing outputs from cloud (read-only)...") + return refreshOutputsAcrossProviders(ctx, providerDefs, states, store, 0 /* default concurrency */, stdout) +} + +// applyPreStepRefreshEnabled reports whether the opt-in env var parses +// to true AND --skip-refresh was not passed. The flag always wins so +// operators can disable the pre-step in environments where the env var +// is forced on globally (e.g. CI that exports it for every job). +// +// Empty/unset and unrecognised values both disable: the env var is +// strictly opt-in, never opt-out. "0" / "false" therefore disable +// rather than (mis-)enabling, which is the convention every operator +// expects. +func applyPreStepRefreshEnabled(skipRefreshFlag bool) bool { + if skipRefreshFlag { + return false + } + v := os.Getenv(applyPreStepRefreshOutputsEnvVar) + if v == "" { + return false + } + enabled, err := strconv.ParseBool(v) + if err != nil { + return false + } + return enabled +} diff --git a/cmd/wfctl/infra_apply_refresh_pre_test.go b/cmd/wfctl/infra_apply_refresh_pre_test.go new file mode 100644 index 000000000..4a2406d49 --- /dev/null +++ b/cmd/wfctl/infra_apply_refresh_pre_test.go @@ -0,0 +1,210 @@ +package main + +import ( + "context" + "io" + "os" + "path/filepath" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// writeApplyPreRefreshConfig writes an infra YAML at /infra.yaml with +// auto_bootstrap disabled (so the apply path doesn't try to run bootstrap), +// a filesystem state backend pointed at /state, a single iac.provider +// module, and one infra.vpc resource keyed to it. Returned path is the +// absolute config path. +func writeApplyPreRefreshConfig(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 +modules: + - name: cloud-provider + type: iac.provider + config: + provider: fake-provider + - name: state-store + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` + - name: coredump-staging-vpc + type: infra.vpc + config: + provider: cloud-provider +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + return cfgPath +} + +// seedStaleVPCState writes a single VPC ResourceState to the configured +// filesystem state backend with no "id" output, ready for refresh to add it. +func seedStaleVPCState(t *testing.T, cfgPath string) { + t.Helper() + store, err := resolveStateStore(cfgPath, "") + if err != nil { + t.Fatalf("resolveStateStore: %v", err) + } + stale := interfaces.ResourceState{ + ID: "coredump-staging-vpc", + Name: "coredump-staging-vpc", + Type: "infra.vpc", + Provider: "fake-provider", + ProviderRef: "cloud-provider", + ProviderID: "uuid-1", + Outputs: map[string]any{"ip_range": "10.0.0.0/16"}, + } + if err := store.SaveResource(context.Background(), stale); err != nil { + t.Fatalf("seed state: %v", err) + } +} + +// loadVPCStateOutputs returns the Outputs map for coredump-staging-vpc as +// persisted on disk after the apply call returns. +func loadVPCStateOutputs(t *testing.T, cfgPath string) map[string]any { + t.Helper() + states, err := loadCurrentState(cfgPath, "") + if err != nil { + t.Fatalf("loadCurrentState: %v", err) + } + for _, s := range states { + if s.Name == "coredump-staging-vpc" { + return s.Outputs + } + } + t.Fatalf("coredump-staging-vpc not in state after apply; got %d entries", len(states)) + return nil +} + +// installFakeRefreshProvider swaps the global resolveIaCProvider for the +// duration of the test with a stub that handles both Read (via the embedded +// driver) and Apply (returning an empty ApplyResult so the apply path is a +// no-op). Returns the cleanup function. +func installFakeRefreshProvider(t *testing.T, liveOutputs map[string]map[string]any) func() { + t.Helper() + orig := resolveIaCProvider + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return &refreshOutputsCmdFakeProvider{readOutputs: liveOutputs}, nil, nil + } + return func() { resolveIaCProvider = orig } +} + +// TestApply_PreStepRefresh_OptInViaEnvVar verifies that setting +// WFCTL_REFRESH_OUTPUTS=1 causes runInfraApply to read live Outputs for +// every state entry and persist new fields BEFORE the plan+apply phase. +// The end-to-end signal is the "id" field present in state on disk after +// apply returns. +func TestApply_PreStepRefresh_OptInViaEnvVar(t *testing.T) { + t.Setenv("WFCTL_REFRESH_OUTPUTS", "1") + dir := t.TempDir() + cfgPath := writeApplyPreRefreshConfig(t, dir) + seedStaleVPCState(t, cfgPath) + + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply: %v", err) + } + + got := loadVPCStateOutputs(t, cfgPath) + if id, _ := got["id"].(string); id != "uuid-1" { + t.Errorf("apply pre-refresh should populate 'id'; got %v", got) + } +} + +// TestApply_PreStepRefresh_DisabledByDefault verifies the opt-in semantics: +// without WFCTL_REFRESH_OUTPUTS set, the pre-step is skipped and stale +// state remains stale. +func TestApply_PreStepRefresh_DisabledByDefault(t *testing.T) { + // Clear any inherited value to make the test resilient under -count=N. + t.Setenv("WFCTL_REFRESH_OUTPUTS", "") + os.Unsetenv("WFCTL_REFRESH_OUTPUTS") + dir := t.TempDir() + cfgPath := writeApplyPreRefreshConfig(t, dir) + seedStaleVPCState(t, cfgPath) + + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply: %v", err) + } + + got := loadVPCStateOutputs(t, cfgPath) + if _, ok := got["id"]; ok { + t.Errorf("default-off pre-refresh should not populate 'id'; got %v", got) + } +} + +// TestApply_PreStepRefresh_EnvVarFalseyValueDisables pins the +// strconv.ParseBool semantic: "0" / "false" / "off" don't enable the +// pre-step. Operators routinely set env vars to "0" to disable a +// feature; without ParseBool that would be a foot-gun (any non-empty +// value enables, including "0"). One representative falsey value is +// enough — strconv.ParseBool's accept set is well-tested upstream. +func TestApply_PreStepRefresh_EnvVarFalseyValueDisables(t *testing.T) { + t.Setenv("WFCTL_REFRESH_OUTPUTS", "0") + dir := t.TempDir() + cfgPath := writeApplyPreRefreshConfig(t, dir) + seedStaleVPCState(t, cfgPath) + + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply: %v", err) + } + + got := loadVPCStateOutputs(t, cfgPath) + if _, ok := got["id"]; ok { + t.Errorf("WFCTL_REFRESH_OUTPUTS=0 must disable pre-refresh; got %v", got) + } +} + +// TestApply_PreStepRefresh_SkipFlagOverridesEnvVar verifies that +// --skip-refresh trumps WFCTL_REFRESH_OUTPUTS. Operators need a way to +// disable the pre-step in the same invocation that has the env var set +// (for example, when a CI environment forces it on globally). +func TestApply_PreStepRefresh_SkipFlagOverridesEnvVar(t *testing.T) { + t.Setenv("WFCTL_REFRESH_OUTPUTS", "1") + dir := t.TempDir() + cfgPath := writeApplyPreRefreshConfig(t, dir) + seedStaleVPCState(t, cfgPath) + + cleanup := installFakeRefreshProvider(t, map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, + }) + defer cleanup() + + if _, err := captureStdout(t, func() error { + return runInfraApply([]string{"--auto-approve", "--skip-refresh", "-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraApply: %v", err) + } + + got := loadVPCStateOutputs(t, cfgPath) + if _, ok := got["id"]; ok { + t.Errorf("--skip-refresh should suppress refresh even with env var set; got %v", got) + } +} diff --git a/cmd/wfctl/infra_refresh_outputs.go b/cmd/wfctl/infra_refresh_outputs.go new file mode 100644 index 000000000..fdd889138 --- /dev/null +++ b/cmd/wfctl/infra_refresh_outputs.go @@ -0,0 +1,244 @@ +package main + +import ( + "context" + "flag" + "fmt" + "io" + "os" + "reflect" + + "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/iac/refreshoutputs" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// runInfraRefreshOutputs reads live Outputs from each provider for the +// resources already in state and persists any field-level changes back to +// the state backend. The contract is strictly read-only at the cloud level — +// no Update or Replace is ever invoked. See iac/refreshoutputs/refresh.go +// for the helper this command wraps. +// +// When the resolved config has no usable iac.provider module for the +// requested env, the literal error +// +// refresh-outputs: provider not configured for env "" +// +// is returned so that operators can distinguish a misconfigured workflow +// from a transient cloud-side failure. T2.7 asserts against this exact line. +func runInfraRefreshOutputs(args []string) error { + fs := flag.NewFlagSet("infra refresh-outputs", flag.ContinueOnError) + // Direct help/usage to stdout so `--help` is pipeable and the CI + // runtime-launch validator (T2.7) can capture it via captureStdout. + fs.SetOutput(os.Stdout) + var configFile, envName string + var concurrency int + fs.StringVar(&configFile, "config", "", "Config file") + fs.StringVar(&configFile, "c", "", "Config file (short for --config)") + fs.StringVar(&envName, "env", "", "Environment name (resolves per-module overrides)") + fs.StringVar(&envName, "e", "", "Environment name (short for --env)") + fs.IntVar(&concurrency, "concurrency", 0, "Maximum concurrent Read calls (default 8)") + if err := fs.Parse(args); err != nil { + return err + } + + cfgFile, err := resolveInfraConfig(fs, configFile) + if err != nil { + return err + } + + ctx := context.Background() + + providerDefs, err := discoverIaCProvidersForRefresh(cfgFile, envName) + if err != nil { + return err + } + if len(providerDefs) == 0 { + return fmt.Errorf("refresh-outputs: provider not configured for env %q", envName) + } + + states, err := loadCurrentState(cfgFile, envName) + if err != nil { + return fmt.Errorf("load current state: %w", err) + } + if len(states) == 0 { + fmt.Println("Refresh: no state to refresh.") + return nil + } + + store, err := resolveStateStore(cfgFile, envName) + if err != nil { + return fmt.Errorf("open state store: %w", err) + } + + return refreshOutputsAcrossProviders(ctx, providerDefs, states, store, concurrency, os.Stdout) +} + +// refreshOutputsProviderDef captures everything refresh-outputs needs to +// load and call a single iac.provider module: its module name, the provider +// type, and the env-resolved config. +type refreshOutputsProviderDef struct { + moduleName string + provType string + provCfg map[string]any +} + +// discoverIaCProvidersForRefresh walks cfgFile's modules and returns one +// providerDef per iac.provider module that resolves successfully for envName +// (modules disabled with `environments: { : ~ }` are skipped). When +// envName is "", the top-level module config is used as-is. The returned +// slice preserves declaration order. +func discoverIaCProvidersForRefresh(cfgFile, envName string) ([]refreshOutputsProviderDef, error) { + cfg, err := config.LoadFromFile(cfgFile) + if err != nil { + return nil, fmt.Errorf("load config: %w", err) + } + var defs []refreshOutputsProviderDef + for i := range cfg.Modules { + m := &cfg.Modules[i] + if m.Type != "iac.provider" { + continue + } + var modCfg map[string]any + if envName != "" { + resolved, ok := m.ResolveForEnv(envName) + if !ok { + // Disabled for this env via null override; not "no provider" + // for the env unless every iac.provider is disabled. + continue + } + modCfg = config.ExpandEnvInMap(resolved.Config) + } else { + modCfg = config.ExpandEnvInMap(m.Config) + } + pt, _ := modCfg["provider"].(string) + if pt == "" { + fmt.Fprintf(os.Stderr, "warning: iac.provider %q has no 'provider' field; skipping\n", m.Name) + continue + } + defs = append(defs, refreshOutputsProviderDef{ + moduleName: m.Name, + provType: pt, + provCfg: modCfg, + }) + } + return defs, nil +} + +// refreshOutputsAcrossProviders groups state entries by which iac.provider +// module owns them, calls refreshoutputs.Refresh for each group, and +// persists any state entries whose Outputs changed. It loads each provider +// at most once. +// +// State entries with a non-empty ProviderRef are matched to the iac.provider +// module of the same name. State entries without a ProviderRef fall back to +// the iac.provider module whose provider type matches state.Provider, but +// only when exactly one such module exists; otherwise the fallback is +// ambiguous and the entry is skipped with a warning rather than refreshed +// against the wrong provider. +func refreshOutputsAcrossProviders( + ctx context.Context, + providerDefs []refreshOutputsProviderDef, + states []interfaces.ResourceState, + store infraStateStore, + concurrency int, + stdout io.Writer, +) error { + defByName := make(map[string]refreshOutputsProviderDef, len(providerDefs)) + defsByType := make(map[string][]string) + for _, d := range providerDefs { + defByName[d.moduleName] = d + defsByType[d.provType] = append(defsByType[d.provType], d.moduleName) + } + + groups := make(map[string][]int) // moduleName → indices into states + var groupOrder []string + for i := range states { + s := &states[i] + moduleName := s.ProviderRef + if moduleName == "" && s.Provider != "" { + candidates := defsByType[s.Provider] + if len(candidates) == 1 { + moduleName = candidates[0] + } + } + if moduleName == "" { + fmt.Fprintf(stdout, "Refresh: skipping %q — cannot resolve owning provider (provider_ref=%q, provider=%q)\n", + s.Name, s.ProviderRef, s.Provider) + continue + } + if _, ok := defByName[moduleName]; !ok { + fmt.Fprintf(stdout, "Refresh: skipping %q — provider module %q not declared in config\n", s.Name, moduleName) + continue + } + if _, exists := groups[moduleName]; !exists { + groupOrder = append(groupOrder, moduleName) + } + groups[moduleName] = append(groups[moduleName], i) + } + + updated := 0 + for _, moduleName := range groupOrder { + def := defByName[moduleName] + idxs := groups[moduleName] + groupStates := make([]interfaces.ResourceState, len(idxs)) + for j, idx := range idxs { + groupStates[j] = states[idx] + } + fmt.Fprintf(stdout, "Refresh: reading %d resource(s) via provider %q (%s)...\n", + len(groupStates), moduleName, def.provType) + if err := refreshOneProviderGroup(ctx, def, idxs, groupStates, states, store, concurrency, &updated, stdout); err != nil { + return err + } + } + fmt.Fprintf(stdout, "Refresh: complete — %d resource(s) updated.\n", updated) + return nil +} + +// refreshOneProviderGroup loads a single provider, refreshes its state +// subset, and persists any entries whose Outputs changed. Extracted so the +// closer is `defer`-closed for panic safety and to keep +// refreshOutputsAcrossProviders readable. +func refreshOneProviderGroup( + ctx context.Context, + def refreshOutputsProviderDef, + idxs []int, + groupStates []interfaces.ResourceState, + states []interfaces.ResourceState, + store infraStateStore, + concurrency int, + updated *int, + stdout io.Writer, +) error { + provider, closer, err := resolveIaCProvider(ctx, def.provType, def.provCfg) + if err != nil { + return fmt.Errorf("provider %q (%s): load provider: %w", def.moduleName, def.provType, err) + } + if closer != nil { + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", def.provType, cerr) + } + }() + } + refreshed, err := refreshoutputs.Refresh(ctx, provider, groupStates, refreshoutputs.Options{Concurrency: concurrency}) + if err != nil { + return fmt.Errorf("provider %q: %w", def.moduleName, err) + } + for j, idx := range idxs { + fresh := refreshed[j] + // reflect.DeepEqual handles non-comparable nested values + // (slices, maps, structs) that the Outputs maps can carry from + // real cloud APIs. A `==` compare on `any` panics for those. + if reflect.DeepEqual(states[idx].Outputs, fresh.Outputs) { + continue + } + states[idx] = fresh + if err := store.SaveResource(ctx, fresh); err != nil { + return fmt.Errorf("provider %q: persist refreshed %q: %w", def.moduleName, fresh.Name, err) + } + *updated++ + fmt.Fprintf(stdout, "Refresh: updated %s\n", fresh.Name) + } + return nil +} diff --git a/cmd/wfctl/infra_refresh_outputs_test.go b/cmd/wfctl/infra_refresh_outputs_test.go new file mode 100644 index 000000000..65e1acf1a --- /dev/null +++ b/cmd/wfctl/infra_refresh_outputs_test.go @@ -0,0 +1,337 @@ +package main + +import ( + "context" + "errors" + "flag" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// refreshOutputsCmdFakeProvider is the IaCProvider stub used by the +// refresh-outputs subcommand tests. It only needs to answer ResourceDriver → +// fakeResourceDriver.Read with canned outputs; everything else can be a +// safe no-op because the subcommand never calls them on the read-only path. +type refreshOutputsCmdFakeProvider struct { + readOutputs map[string]map[string]any +} + +func (f *refreshOutputsCmdFakeProvider) Name() string { return "fake-refresh-outputs" } +func (f *refreshOutputsCmdFakeProvider) Version() string { return "0.0.0" } +func (f *refreshOutputsCmdFakeProvider) Initialize(_ context.Context, _ map[string]any) error { + return nil +} +func (f *refreshOutputsCmdFakeProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { + return nil +} +func (f *refreshOutputsCmdFakeProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return &interfaces.IaCPlan{}, nil +} +func (f *refreshOutputsCmdFakeProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return &interfaces.ApplyResult{}, nil +} +func (f *refreshOutputsCmdFakeProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) Import(_ context.Context, _ string, _ string) (*interfaces.ResourceState, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return &refreshOutputsCmdFakeDriver{outputs: f.readOutputs}, nil +} +func (f *refreshOutputsCmdFakeProvider) SupportedCanonicalKeys() []string { return nil } +func (f *refreshOutputsCmdFakeProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (f *refreshOutputsCmdFakeProvider) Close() error { return nil } + +// refreshOutputsCmdFakeDriver answers Read with canned outputs keyed by +// ProviderID. All other ResourceDriver methods panic to make accidental +// non-Read use loud during testing. +type refreshOutputsCmdFakeDriver struct { + outputs map[string]map[string]any +} + +func (d *refreshOutputsCmdFakeDriver) Create(context.Context, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + return &interfaces.ResourceOutput{ + Name: ref.Name, + Type: ref.Type, + ProviderID: ref.ProviderID, + Outputs: d.outputs[ref.ProviderID], + }, nil +} +func (d *refreshOutputsCmdFakeDriver) Update(context.Context, interfaces.ResourceRef, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) Delete(context.Context, interfaces.ResourceRef) error { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) Diff(context.Context, interfaces.ResourceSpec, *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) HealthCheck(context.Context, interfaces.ResourceRef) (*interfaces.HealthResult, error) { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) Scale(context.Context, interfaces.ResourceRef, int) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *refreshOutputsCmdFakeDriver) SensitiveKeys() []string { return nil } + +// TestRefreshOutputs_CommandHelp verifies that --help prints the standard +// FlagSet usage banner naming the subcommand. The banner is what T2.7's +// runtime-launch validation will exercise via the built binary. +func TestRefreshOutputs_CommandHelp(t *testing.T) { + out, err := captureStdout(t, func() error { + return runInfraRefreshOutputs([]string{"--help"}) + }) + if !errors.Is(err, flag.ErrHelp) { + t.Fatalf("expected flag.ErrHelp, got %v", err) + } + if !strings.Contains(out, "Usage of infra refresh-outputs") { + t.Errorf("help output missing 'Usage of infra refresh-outputs'; got: %s", out) + } +} + +// TestRefreshOutputs_PersistsNewFieldsToState exercises the end-to-end +// persists-new-output path: pre-populate a filesystem state backend with a +// stale VPC entry, swap the IaCProvider loader for a stub that returns an +// extra "id" field on Read, run the subcommand, and verify the field has +// been written back to state. +func TestRefreshOutputs_PersistsNewFieldsToState(t *testing.T) { + dir := t.TempDir() + stateDir := filepath.Join(dir, "state") + if err := os.MkdirAll(stateDir, 0o755); err != nil { + t.Fatal(err) + } + + cfgPath := filepath.Join(dir, "infra.yaml") + cfg := `modules: + - name: cloud-provider + type: iac.provider + config: + provider: fake-provider + - name: state-store + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` + - name: coredump-staging-vpc + type: infra.vpc + config: + provider: cloud-provider +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + + // Pre-populate state with a stale VPC entry (no "id" output yet). + store, err := resolveStateStore(cfgPath, "") + if err != nil { + t.Fatalf("resolveStateStore: %v", err) + } + stale := interfaces.ResourceState{ + ID: "coredump-staging-vpc", + Name: "coredump-staging-vpc", + Type: "infra.vpc", + Provider: "fake-provider", + ProviderRef: "cloud-provider", + ProviderID: "uuid-1", + Outputs: map[string]any{"ip_range": "10.0.0.0/16"}, + } + if err := store.SaveResource(context.Background(), stale); err != nil { + t.Fatalf("seed state: %v", err) + } + + // Swap the provider loader so the test never touches a real cloud. + orig := resolveIaCProvider + defer func() { resolveIaCProvider = orig }() + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return &refreshOutputsCmdFakeProvider{ + readOutputs: map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, + }, + }, nil, nil + } + + if _, err := captureStdout(t, func() error { + return runInfraRefreshOutputs([]string{"-c", cfgPath, "--env", "staging"}) + }); err != nil { + t.Fatalf("runInfraRefreshOutputs: %v", err) + } + + refreshed, err := loadCurrentState(cfgPath, "") + if err != nil { + t.Fatalf("loadCurrentState: %v", err) + } + byName := make(map[string]interfaces.ResourceState, len(refreshed)) + for _, s := range refreshed { + byName[s.Name] = s + } + got := byName["coredump-staging-vpc"] + if id, _ := got.Outputs["id"].(string); id != "uuid-1" { + t.Errorf("expected 'id' field after refresh, got %v", got.Outputs) + } +} + +// TestRefreshOutputs_NonComparableOutputs_DoesNotPanic regression-tests +// the Outputs-changed check against real-world Outputs that contain slices +// and nested maps. A naive `lv != v` on `any` values panics with +// "comparing uncomparable type" the moment a slice or map is involved — +// reflect.DeepEqual is the correct tool. Covers both the unchanged path +// (live == persisted) and the changed path (live grows a new field) so +// neither code direction can regress to the panic. +func TestRefreshOutputs_NonComparableOutputs_DoesNotPanic(t *testing.T) { + for _, tc := range []struct { + name string + liveOutputs map[string]any + expectUpdate bool + }{ + { + name: "unchanged-with-slice-and-nested-map", + liveOutputs: map[string]any{ + "subnet_ids": []string{"a", "b"}, + "tags": map[string]any{"env": "staging"}, + }, + expectUpdate: false, + }, + { + name: "added-field-with-slice-and-nested-map", + liveOutputs: map[string]any{ + "subnet_ids": []string{"a", "b"}, + "tags": map[string]any{"env": "staging"}, + "id": "uuid-1", + }, + expectUpdate: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + stateDir := filepath.Join(dir, "state") + if err := os.MkdirAll(stateDir, 0o755); err != nil { + t.Fatal(err) + } + cfgPath := filepath.Join(dir, "infra.yaml") + cfg := `modules: + - name: cloud-provider + type: iac.provider + config: + provider: fake-provider + - name: state-store + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` + - name: vpc-1 + type: infra.vpc + config: + provider: cloud-provider +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + + store, err := resolveStateStore(cfgPath, "") + if err != nil { + t.Fatalf("resolveStateStore: %v", err) + } + persisted := interfaces.ResourceState{ + ID: "vpc-1", + Name: "vpc-1", + Type: "infra.vpc", + Provider: "fake-provider", + ProviderRef: "cloud-provider", + ProviderID: "uuid-1", + Outputs: map[string]any{ + "subnet_ids": []string{"a", "b"}, + "tags": map[string]any{"env": "staging"}, + }, + } + if err := store.SaveResource(context.Background(), persisted); err != nil { + t.Fatalf("seed state: %v", err) + } + + orig := resolveIaCProvider + defer func() { resolveIaCProvider = orig }() + liveOutputs := tc.liveOutputs + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return &refreshOutputsCmdFakeProvider{ + readOutputs: map[string]map[string]any{"uuid-1": liveOutputs}, + }, nil, nil + } + + // The critical assertion: this call must NOT panic with + // "comparing uncomparable type". + if _, err := captureStdout(t, func() error { + return runInfraRefreshOutputs([]string{"-c", cfgPath}) + }); err != nil { + t.Fatalf("runInfraRefreshOutputs panicked or errored: %v", err) + } + + refreshed, err := loadCurrentState(cfgPath, "") + if err != nil { + t.Fatalf("loadCurrentState: %v", err) + } + var got interfaces.ResourceState + for _, s := range refreshed { + if s.Name == "vpc-1" { + got = s + break + } + } + if tc.expectUpdate { + if _, ok := got.Outputs["id"]; !ok { + t.Errorf("expected new 'id' field after refresh; got Outputs=%v", got.Outputs) + } + } + // Whether updated or not, post-state must still carry the + // nested values intact. + if _, ok := got.Outputs["subnet_ids"]; !ok { + t.Errorf("expected subnet_ids preserved; got Outputs=%v", got.Outputs) + } + }) + } +} + +// TestRefreshOutputs_NoProviderConfigured_ReturnsLiteralError pins the +// exact stderr line T2.7 asserts against. The wording is load-bearing — +// changing it breaks the runtime-launch-validation gate. +func TestRefreshOutputs_NoProviderConfigured_ReturnsLiteralError(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + // Config has no iac.provider modules. + cfg := `modules: + - name: state-store + type: iac.state + config: + backend: filesystem + directory: ` + dir + ` +` + if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } + err := runInfraRefreshOutputs([]string{"-c", cfgPath, "--env", "staging"}) + if err == nil { + t.Fatal("expected error when no iac.provider configured") + } + want := `refresh-outputs: provider not configured for env "staging"` + if err.Error() != want { + t.Errorf("error mismatch:\n got: %q\nwant: %q", err.Error(), want) + } +} diff --git a/docs/WFCTL.md b/docs/WFCTL.md index bc5f908d9..ffb432bca 100644 --- a/docs/WFCTL.md +++ b/docs/WFCTL.md @@ -96,6 +96,7 @@ graph TD infra --> infra-bootstrap["bootstrap"] infra --> infra-state["state"] infra --> infra-outputs["outputs"] + infra --> infra-refresh-outputs["refresh-outputs"] infra-state --> infra-state-list["list"] infra-state --> infra-state-export["export"] @@ -1171,6 +1172,7 @@ wfctl infra [options] [config.yaml] | `bootstrap` | Generate secrets and initialise state backend before first apply | | `state` | Manage state storage (list/export/import) | | `outputs` | Print resource outputs from state (yaml/json/env formats) | +| `refresh-outputs` | Read live outputs from each provider and reconcile state (no cloud writes) | | Flag | Default | Description | |------|---------|-------------| @@ -1216,6 +1218,63 @@ wfctl infra bootstrap -c infra.yaml --env staging --force-rotate NATS_AUTH_TOKEN wfctl infra bootstrap -c infra.yaml --force-rotate FOO --force-rotate BAR ``` +#### `infra refresh-outputs` + +Read live outputs from each `iac.provider` for resources already in state and persist any field-level changes back to the state backend. The contract is strictly read-only at the cloud level — `refresh-outputs` never invokes Update or Replace. + +``` +wfctl infra refresh-outputs [-c CONFIG] [--env ENV] [--concurrency N] +``` + +| Flag | Default | Description | +|------|---------|-------------| +| `-c`, `--config` | _(auto-detected)_ | Config file (searches `infra.yaml`, `config/infra.yaml`) | +| `-e`, `--env` | `` | Environment name (resolves per-module overrides; iac.provider modules disabled for the env are skipped) | +| `--concurrency` | `8` | Maximum concurrent Read calls. Values < 1 fall back to the default. | + +**Behavior:** + +- Discovers `iac.provider` modules with per-env resolution. +- Loads current state from the configured `iac.state` backend. +- Groups state entries by their owning provider module (`provider_ref` first, falling back to provider type when exactly one module of that type is declared). +- Calls each provider's `ResourceDriver.Read` once per resource via the bounded-concurrency `iac/refreshoutputs.Refresh` helper. +- Persists any state entry whose `outputs` map changed — entries whose live outputs equal the persisted outputs are left alone. + +**Errors:** + +When the resolved config has no usable `iac.provider` module for the requested env, `wfctl` exits 1 with the literal stderr line: + +``` +error: refresh-outputs: provider not configured for env "" +``` + +This wording is load-bearing — CI gates and runtime-launch validation pin the exact form. On any provider Read or driver-resolution failure, the command returns the wrapped error from the `iac/refreshoutputs` helper without persisting partial progress. + +**Apply-time pre-step (opt-in):** + +`wfctl infra apply` can run the same refresh as a pre-plan step, ensuring the planner doesn't make decisions against stale outputs. + +| Variable / Flag | Effect | +|------|---------| +| `WFCTL_REFRESH_OUTPUTS=1` (or any `strconv.ParseBool` truthy value) | Enable the apply pre-step. | +| `WFCTL_REFRESH_OUTPUTS=0` (or any falsey value, empty, or unrecognised) | Disable the apply pre-step (default). | +| `wfctl infra apply --skip-refresh` | Suppress the apply pre-step regardless of the env var (CI escape hatch). | + +The pre-step only fires for `infra.*` configs; legacy `platform.*` configs are silently skipped. + +**Examples:** + +```bash +# One-off explicit refresh against the staging env. +wfctl infra refresh-outputs -c infra.yaml --env staging + +# Apply with pre-plan refresh enabled. +WFCTL_REFRESH_OUTPUTS=1 wfctl infra apply --auto-approve -c infra.yaml --env staging + +# Apply with pre-step suppressed even though CI exports the env var. +WFCTL_REFRESH_OUTPUTS=1 wfctl infra apply --auto-approve --skip-refresh -c infra.yaml +``` + --- ### `docs generate` @@ -2162,3 +2221,51 @@ deploy: runtime: minikube # minikube | kind | docker-desktop | k3d | remote registry: ghcr.io/myorg ``` + +--- + +## Diff Cache + +`wfctl infra plan` consults a per-resource diff cache so a Plan re-run against unchanged inputs can skip the (sometimes network-expensive) provider-side `Diff` call. The cache lives at the package `iac/diffcache/` and is keyed by `(plugin-version, resource-type, provider-id, sha256(config), sha256(outputs))`. + +**The cache is purely an amortization optimization, NOT a correctness mechanism.** Apply paths remain correct on a 100 % miss rate, which is exactly what CI sees on every fresh runner. Workflows MUST NOT depend on a cache hit for correctness or reproducibility. + +### Backend selection (`WFCTL_DIFFCACHE`) + +The env var `WFCTL_DIFFCACHE` selects the cache backend at process start: + +| Value | Backend | +|---------------|------------------------------------------------------------------------------------------------| +| `disabled` | No-op cache: every `Get` misses; `Put` is a no-op. Use this for fully-deterministic timing or to bypass any disk state. | +| `:memory:` | In-memory cache that lives only for the current process. **CI workflows in this repo set this explicitly** so containerized runners never write cache data to disk. | +| _(unset)_ or anything else | Filesystem cache rooted at `~/.cache/wfctl/diff/.json` (honors `XDG_CACHE_HOME` on Linux via `os.UserCacheDir`). The default for operators running `wfctl` locally. | + +If the user cache directory cannot be resolved, the helper falls back to the in-memory cache automatically — so the caller always gets a working `Cache`. + +### Eviction (filesystem backend) + +The filesystem cache enforces an LRU eviction policy on each `Put`: + +- **By count:** at most 1024 entries. +- **By total size:** at most 64 MiB on disk. + +Whichever cap is exceeded first triggers the eviction pass. The pass evicts the oldest 10 % of entries (sorted by mtime; secondary sort by path for determinism) so amortized cost stays bounded. + +### Corruption recovery + +If a cache file fails to parse on `Get` (truncated, partial-write, JSON syntax error, or a `schemaVersion` mismatch from a wfctl downgrade), the cache silently deletes the corrupt file and returns a miss. No error is surfaced to the caller; a single info-level log line fires the first time corruption is observed in a process so operators have a breadcrumb without log spam. + +### Plugin downgrade + +`PluginVersion` is part of the cache key, so a plugin downgrade naturally invalidates entries (cache key miss). Old entries persist on disk until the LRU eviction reclaims them; the size cap above bounds the disk waste. + +### CI configuration + +Per the T3.5 rev3 lifecycle constraint, **all CI workflows in this repository set `WFCTL_DIFFCACHE=:memory:` at the workflow level** so containerized runners never write to disk: + +```yaml +env: + WFCTL_DIFFCACHE: ":memory:" +``` + +Files: `.github/workflows/ci.yml`, `benchmark.yml`, `pre-release.yml`, `release.yml`, `dependency-update.yml`. New workflow files that invoke `go test` or `wfctl` should add the same env var. diff --git a/docs/adr/006-wfctl-refresh-outputs-env-var-parsebool.md b/docs/adr/006-wfctl-refresh-outputs-env-var-parsebool.md new file mode 100644 index 000000000..09e8859eb --- /dev/null +++ b/docs/adr/006-wfctl-refresh-outputs-env-var-parsebool.md @@ -0,0 +1,51 @@ +# ADR 006: WFCTL_REFRESH_OUTPUTS uses strconv.ParseBool, not bare presence + +## Status + +Accepted + +## Context + +W-2 Task T2.3 (`docs/plans/2026-05-03-iac-conformance-and-replace.md`, line 1061) specified the apply-time pre-step opt-in gate as a bare presence check: + +```go +if os.Getenv("WFCTL_REFRESH_OUTPUTS") != "" { + // run pre-step +} +``` + +Under that contract, any non-empty value enables the pre-step — including values an operator would universally read as disabling: `0`, `false`, `no`, `off`. T2.3's first quality review (code-reviewer) flagged this as a foot-gun: operators reach for the `=0` / `=false` convention to disable a feature, and the bare-presence check turns that convention into the opposite of its intent. The original commit body also leaned into the wrong mental model by listing `"yes"` as an enabling example, reinforcing the misread that the value was being parsed. + +`--skip-refresh` exists as a robust off-switch override, so an operator who falls into the foot-gun has an escape hatch — but discovering it requires reading the docs after the surprise hit, by which point a planner has already burned a `Read` round-trip on every state entry. + +## Decision + +Implement the gate using `strconv.ParseBool` instead of bare presence. The accept set is well-known to Go developers and matches every common operator convention: + +- Truthy enables: `1`, `t`, `T`, `TRUE`, `true`, `True` +- Falsey explicitly disables: `0`, `f`, `F`, `FALSE`, `false`, `False` +- Empty / unset / unrecognised: disabled (default; never an exception) + +`--skip-refresh` continues to override the env var unconditionally, preserving the CI escape-hatch contract for environments where the env var is forced on globally. + +## Consequences + +**Positive:** + +- Operator surprise eliminated: `WFCTL_REFRESH_OUTPUTS=0` disables, as expected. +- Self-documenting contract: `ParseBool` is a familiar Go idiom; the godoc on `applyPreStepRefreshEnabled` enumerates the accept set. +- Future-proofed against the same bug class for other refresh-outputs related env vars added in W-3+. + +**Negative:** + +- Plan-deviation: the implementation diverges from `docs/plans/2026-05-03-iac-conformance-and-replace.md` line 1061's literal contract. Recorded in this ADR so future contributors see the rejected alternative and the reasoning, rather than discovering it via `git blame` archaeology. A future plan revision should reflect the actual implementation; out of scope for this ADR. +- Operators who set the env var to an unrecognised string (e.g. `WFCTL_REFRESH_OUTPUTS=enabled`) silently get the disabled default. The accept set is intentionally narrow; the alternative — accepting any non-empty as truthy — is exactly the foot-gun this ADR rejects. Mitigated by the doc table in `docs/WFCTL.md` enumerating the truthy/falsey values explicitly. + +**Provenance:** decided by Claude (autonomous-pipeline team-lead) after code-review/spec-review consensus on 2026-05-04. code-reviewer surfaced the foot-gun during T2.3 quality review; implementer prepared the ParseBool change; spec-reviewer requested this ADR post-hoc; team-lead approved option-1 (approve-as-is + follow-up ADR) over plan revert. + +## References + +- Plan task spec: `docs/plans/2026-05-03-iac-conformance-and-replace.md` §T2.3 (line 1061) +- Implementation: `cmd/wfctl/infra_apply_refresh_pre.go::applyPreStepRefreshEnabled` (commit `bfd1bbe`) +- Test pinning the falsey-disables semantic: `cmd/wfctl/infra_apply_refresh_pre_test.go::TestApply_PreStepRefresh_EnvVarFalseyValueDisables` +- Operator-facing docs: `docs/WFCTL.md` → `infra refresh-outputs` → "Apply-time pre-step (opt-in)" diff --git a/iac/diffcache/cache.go b/iac/diffcache/cache.go new file mode 100644 index 000000000..7d164548a --- /dev/null +++ b/iac/diffcache/cache.go @@ -0,0 +1,185 @@ +// Package diffcache caches per-resource Diff results so a wfctl invocation +// that re-runs a Plan against unchanged inputs can skip the (sometimes +// network-expensive) provider-side Diff call. The cache is purely an +// amortization optimization, NOT a correctness mechanism — apply paths +// remain correct on a 100% miss rate (which is exactly what CI sees on +// every fresh runner). +// +// # Storage backends +// +// Cache selection is driven by the WFCTL_DIFFCACHE env var, resolved by +// [New]: +// +// - `disabled` → noop cache (every Get misses; Put is a no-op). Use +// this when an operator wants fully-deterministic Plan/Apply timing +// with no shared state across invocations. +// - `:memory:` → in-memory cache that lives only for the current +// process. CI workflows in this repo set WFCTL_DIFFCACHE=:memory: +// explicitly so containerized runners never write to disk. +// - any other value (or unset) → filesystem cache rooted at +// `~/.cache/wfctl/diff/`. +// +// # CI ephemerality (load-bearing) +// +// CI runners are ephemeral — each job starts with an empty cache. +// Workflow correctness MUST NOT depend on a cache hit. The diff cache +// is an operator-local performance optimization for repeated +// `wfctl infra plan` invocations against the same checkout. +// +// # Cache key +// +// The cache key is a [Key] tuple of (PluginVersion, Type, ProviderID, +// SHAConfig, SHAOutputs). Plugin downgrades naturally invalidate +// entries since PluginVersion is part of the key — old entries persist +// on disk until the LRU eviction reclaims them; the size cap (1024 +// entries / 64 MiB) bounds the disk waste. +// +// # Schema versioning +// +// Each cache file embeds [cacheSchemaVersion] in a JSON envelope. On +// Get, a mismatched version is treated identically to file corruption: +// the entry is silently evicted and the caller re-Diffs. +// +// # T3.5 / W-3a status +// +// This package ships in W-3a. The consumer that wires it into +// platform.ComputePlan lands in W-3b/T3.6f. Until then, the cache is +// callable but never invoked from production code paths. +package diffcache + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// cacheSchemaVersion is the on-disk envelope version. Bump on any +// breaking change to the JSON shape; old files will be silently +// evicted on the next Get (same code path as corruption recovery). +const cacheSchemaVersion = 1 + +// defaultMaxEntries is the LRU eviction trigger by count. +const defaultMaxEntries = 1024 + +// defaultMaxBytes is the LRU eviction trigger by total on-disk size +// (64 MiB). Whichever cap (entries or bytes) is exceeded first triggers +// the eviction pass. +const defaultMaxBytes = 64 * 1024 * 1024 + +// evictionFraction is the fraction of cache entries to evict in one +// over-cap pass. 0.10 = oldest 10%; matches rev2 lifecycle constraint. +const evictionFraction = 0.10 + +// Key tuples the inputs to a single Diff. Two Keys are equal iff every +// field matches; the canonical sha256 fingerprint of the key +// determines the on-disk filename. +type Key struct { + // PluginVersion is the plugin's name@version string, e.g., + // "do@v0.10.0". Plugin downgrades naturally invalidate cache + // entries via this field. + PluginVersion string `json:"plugin_version"` + // Type is the canonical resource type, e.g., "infra.vpc". + Type string `json:"type"` + // ProviderID is the resource's cloud-side identifier; empty for + // net-new resources. + ProviderID string `json:"provider_id"` + // SHAConfig is the sha256-hex of canonical-marshal(spec.Config). + SHAConfig string `json:"sha_config"` + // SHAOutputs is the sha256-hex of canonical-marshal(currentState.Outputs); + // empty for net-new resources. + SHAOutputs string `json:"sha_outputs"` +} + +// Cache is the diff-result cache. Implementations are safe for +// concurrent use (the backing fs cache uses os-level atomic file ops; +// the in-memory cache locks internally). +type Cache interface { + // Get returns the cached DiffResult for key. The boolean is true + // iff the entry was found and successfully decoded; corruption, + // schema-version mismatch, and missing entries all yield + // (zero-value, false). + Get(key Key) (interfaces.DiffResult, bool) + // Put stores result under key. Errors during Put (e.g., disk + // full, serialization failure) are silently swallowed because + // cache misses are correct — apply behavior must not depend on + // Put success. + Put(key Key, result interfaces.DiffResult) +} + +// New returns a Cache configured by the WFCTL_DIFFCACHE env var. +// +// - "disabled" → [NewNoop] +// - ":memory:" → [NewMemory] +// - default → [NewFilesystem] rooted at the user cache directory +// (typically `~/.cache/wfctl/diff/`). +// +// When the user cache directory cannot be resolved, falls back to the +// in-memory cache so the caller still gets a working Cache (the +// filesystem path being unavailable is the operator's hint that disk +// caching is off). +func New() Cache { + switch strings.TrimSpace(os.Getenv("WFCTL_DIFFCACHE")) { + case "disabled": + return NewNoop() + case ":memory:": + return NewMemory() + } + dir, err := userCacheDir() + if err != nil { + return NewMemory() + } + return NewFilesystem(dir) +} + +// NewNoop returns a Cache that always misses. Put is a no-op. +func NewNoop() Cache { return &noopCache{} } + +// noopCache is the disabled-cache implementation. +type noopCache struct{} + +func (noopCache) Get(_ Key) (interfaces.DiffResult, bool) { return interfaces.DiffResult{}, false } +func (noopCache) Put(_ Key, _ interfaces.DiffResult) { /* no-op */ } + +// userCacheDir returns the diff-cache root under the user cache +// directory, e.g., `~/.cache/wfctl/diff/` on Linux. Honors XDG_CACHE_HOME. +func userCacheDir() (string, error) { + base, err := os.UserCacheDir() + if err != nil { + return "", fmt.Errorf("resolve user cache dir: %w", err) + } + return filepath.Join(base, "wfctl", "diff"), nil +} + +// keyFingerprint returns the sha256-hex fingerprint of the canonical +// key serialization. Exported as a package-level function so tests can +// assert key-stability without coupling to the filesystem cache. +func keyFingerprint(k Key) string { + // Concatenate fields with a NUL separator so per-field collisions + // are impossible (any field containing NUL would have to be + // deliberately injected; cloud IDs and resource types don't). + var b strings.Builder + b.WriteString(k.PluginVersion) + b.WriteByte(0) + b.WriteString(k.Type) + b.WriteByte(0) + b.WriteString(k.ProviderID) + b.WriteByte(0) + b.WriteString(k.SHAConfig) + b.WriteByte(0) + b.WriteString(k.SHAOutputs) + sum := sha256.Sum256([]byte(b.String())) + return hex.EncodeToString(sum[:]) +} + +// envelope is the JSON shape persisted to disk and used by the +// in-memory cache to validate on Get. The schemaVersion field gates +// silent eviction on a future schema bump. +type envelope struct { + SchemaVersion int `json:"schemaVersion"` + Result interfaces.DiffResult `json:"result"` +} diff --git a/iac/diffcache/cache_corruption_test.go b/iac/diffcache/cache_corruption_test.go new file mode 100644 index 000000000..98009aa75 --- /dev/null +++ b/iac/diffcache/cache_corruption_test.go @@ -0,0 +1,107 @@ +package diffcache + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestCache_CorruptionRecovery_TruncatedFile verifies that a partially- +// written or truncated cache file does not crash Get and is silently +// removed so the next Put succeeds. The contract: Get returns +// (_, false) on parse failure; the file is deleted from disk. +func TestCache_CorruptionRecovery_TruncatedFile(t *testing.T) { + dir := t.TempDir() + c := NewFilesystem(dir).(*filesystemCache) + key := Key{Type: "infra.vpc", ProviderID: "vpc-abc"} + c.Put(key, interfaces.DiffResult{NeedsUpdate: true}) + + // Truncate the cache file to simulate a partial write or crash mid-Put. + path := c.pathFor(key) + if err := os.WriteFile(path, []byte("{not val"), 0o644); err != nil { + t.Fatalf("failed to truncate cache file: %v", err) + } + + if _, hit := c.Get(key); hit { + t.Errorf("expected miss on truncated file; got hit") + } + // The corrupt file should be removed so a fresh Put can succeed. + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("corrupt cache file should be deleted; stat err=%v", err) + } + + // Re-Put + Get round-trip must work after corruption recovery. + c.Put(key, interfaces.DiffResult{NeedsReplace: true}) + got, hit := c.Get(key) + if !hit { + t.Fatal("expected hit after re-Put following corruption") + } + if !got.NeedsReplace { + t.Errorf("post-recovery Get returned wrong value: %+v", got) + } +} + +// TestCache_CorruptionRecovery_SchemaVersionMismatch verifies that +// cache files written under a different schema version are silently +// evicted on Get — same code path as JSON corruption since we treat +// both as "cache file we cannot use." Lock the contract so a future +// schema bump (cacheSchemaVersion = 2) keeps the silent-eviction +// behavior. +func TestCache_CorruptionRecovery_SchemaVersionMismatch(t *testing.T) { + dir := t.TempDir() + c := NewFilesystem(dir).(*filesystemCache) + key := Key{Type: "infra.vpc", ProviderID: "vpc-abc"} + + // Write a valid-JSON file with a schemaVersion that doesn't match + // the current cacheSchemaVersion constant. Use a value >> current + // so it can't accidentally match a future bump. + path := c.pathFor(key) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + envelope := map[string]any{ + "schemaVersion": 9999, + "result": map[string]any{"needs_update": true}, + } + data, _ := json.Marshal(envelope) + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatal(err) + } + + if _, hit := c.Get(key); hit { + t.Errorf("expected miss on schema-version mismatch; got hit") + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("schema-mismatched cache file should be deleted; stat err=%v", err) + } +} + +// TestCache_CorruptionRecovery_GetReturnsNoErrorOnParseFailure proves +// the no-error contract: callers Get a (zero, false) tuple, never a +// surfaced error. Cache failures must NEVER abort apply. +func TestCache_CorruptionRecovery_GetReturnsNoErrorOnParseFailure(t *testing.T) { + dir := t.TempDir() + c := NewFilesystem(dir).(*filesystemCache) + key := Key{Type: "x"} + path := c.pathFor(key) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("\x00\x00not-json\x00\x00"), 0o644); err != nil { + t.Fatal(err) + } + // We can't directly assert "no error returned" via the Get + // signature, but we can lock that hit is false and the file is + // removed (the canonical recovery path). A panic would also fail + // this test, which is the implicit contract this case anchors. + got, hit := c.Get(key) + if hit { + t.Error("expected miss on garbage bytes") + } + if got.NeedsUpdate || got.NeedsReplace || len(got.Changes) > 0 { + t.Errorf("zero-value DiffResult on miss; got %+v", got) + } +} diff --git a/iac/diffcache/cache_eviction_test.go b/iac/diffcache/cache_eviction_test.go new file mode 100644 index 000000000..cb025154c --- /dev/null +++ b/iac/diffcache/cache_eviction_test.go @@ -0,0 +1,122 @@ +package diffcache + +import ( + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestCache_LRUEvictionByCount verifies the LRU eviction trigger when +// the entry count exceeds maxEntries. Per the rev2 lifecycle constraint +// (10% per-overflow batch), the eviction is amortized — for the test +// we drive it with a small cap (10 entries; 10% = 1 evicted per +// over-cap Put) to keep the test fast. +func TestCache_LRUEvictionByCount(t *testing.T) { + dir := t.TempDir() + c := &filesystemCache{ + dir: dir, + maxEntries: 10, + maxBytes: defaultMaxBytes, // not the trigger here + } + // Fill to the cap. Each Put with a different key creates a new file. + // We sleep 1ms between Puts so mtimes are distinguishable; the OS + // mtime resolution is 1ns on most filesystems but coarse on some, + // so 1ms is a safe lower bound. + for i := range 10 { + c.Put(Key{Type: fmt.Sprintf("k%d", i)}, interfaces.DiffResult{}) + time.Sleep(time.Millisecond) + } + if got := countCacheFiles(t, dir); got != 10 { + t.Fatalf("expected 10 entries pre-overflow; got %d", got) + } + // Trigger overflow: one more Put → cap exceeded → evict 1 (10% of 10). + c.Put(Key{Type: "k_overflow"}, interfaces.DiffResult{}) + got := countCacheFiles(t, dir) + // After eviction (10% of 10 = 1) + new entry: 10 - 1 + 1 = 10. + if got != 10 { + t.Errorf("post-overflow count: got %d entries, want 10 (cap holds)", got) + } + // Oldest key (k0) should have been evicted. + if _, hit := c.Get(Key{Type: "k0"}); hit { + t.Errorf("oldest key (k0) should be evicted") + } + // Newest key (k_overflow) should still be present. + if _, hit := c.Get(Key{Type: "k_overflow"}); !hit { + t.Errorf("newest key (k_overflow) should remain") + } +} + +// TestCache_LRUEvictionBatchOf10Percent verifies that when the cap is +// large enough to make 10% a multi-entry batch, multiple oldest +// entries are evicted in one pass. This locks the "amortized cost" / +// "evict oldest 10% in one pass" rev2 constraint. +func TestCache_LRUEvictionBatchOf10Percent(t *testing.T) { + dir := t.TempDir() + c := &filesystemCache{ + dir: dir, + maxEntries: 100, + maxBytes: defaultMaxBytes, + } + for i := range 100 { + c.Put(Key{Type: fmt.Sprintf("k%03d", i)}, interfaces.DiffResult{}) + time.Sleep(time.Millisecond) + } + // Trigger overflow: one extra Put → evict 10 oldest (10% of 100) + + // add 1 → 100 - 10 + 1 = 91. + c.Put(Key{Type: "k_overflow"}, interfaces.DiffResult{}) + got := countCacheFiles(t, dir) + if got < 88 || got > 92 { + // Allow ±2 for filesystems with mtime resolution coarser than + // the 1ms sleep; the central tendency must be ~91. + t.Errorf("post-overflow count: got %d, want ~91 (allow ±2 for mtime precision)", got) + } + // First 5 oldest keys should be evicted. + for i := range 5 { + k := Key{Type: fmt.Sprintf("k%03d", i)} + if _, hit := c.Get(k); hit { + t.Errorf("k%03d should be in the evicted oldest 10%%", i) + } + } +} + +// countCacheFiles returns the number of *.json files under dir. Used +// to assert eviction behavior without depending on cache internals. +func countCacheFiles(t *testing.T, dir string) int { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, "*.json")) + if err != nil { + t.Fatalf("glob: %v", err) + } + return len(matches) +} + +// TestCache_EvictionTouchesNothingWhenUnderCap verifies the no-op +// case: Put when count < maxEntries leaves all existing files alone. +func TestCache_EvictionTouchesNothingWhenUnderCap(t *testing.T) { + dir := t.TempDir() + c := &filesystemCache{ + dir: dir, + maxEntries: 100, + maxBytes: defaultMaxBytes, + } + c.Put(Key{Type: "a"}, interfaces.DiffResult{}) + c.Put(Key{Type: "b"}, interfaces.DiffResult{}) + c.Put(Key{Type: "c"}, interfaces.DiffResult{}) + if got := countCacheFiles(t, dir); got != 3 { + t.Errorf("under-cap puts should not evict; got %d entries", got) + } + // All three should be retrievable. + for _, k := range []string{"a", "b", "c"} { + if _, hit := c.Get(Key{Type: k}); !hit { + t.Errorf("under-cap key %q should be retained", k) + } + } + // Sanity: verify no errant background eviction took place. + if _, err := os.Stat(filepath.Join(dir, "*.json")); err == nil { + t.Error("glob path should not exist as a literal file (sanity check)") + } +} diff --git a/iac/diffcache/cache_filesystem.go b/iac/diffcache/cache_filesystem.go new file mode 100644 index 000000000..c822e336b --- /dev/null +++ b/iac/diffcache/cache_filesystem.go @@ -0,0 +1,162 @@ +package diffcache + +import ( + "encoding/json" + "errors" + "log" + "os" + "path/filepath" + "sort" + "sync" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// filesystemCache is the file-backed Cache implementation. Cache files +// live under cache.dir, named by the sha256 fingerprint of the Key +// with a .json extension, e.g., +// `/abcdef0123456789....json`. The schema-version envelope +// gates silent eviction on a future schema bump. +// +// Concurrency: Get is serialized through the read filesystem; Put +// uses an os.WriteFile that is atomic on POSIX-compliant filesystems +// for sizes <= PIPE_BUF, and for larger writes the worst case is a +// truncated file that the next Get treats as corruption (silent +// eviction). The eviction-on-overflow scan is guarded by evictMu so +// concurrent Puts don't multiply-evict. +// +// Logging: a single info-level log fires the first time corruption is +// observed in this process so an operator has a breadcrumb without +// log spam. +type filesystemCache struct { + dir string + maxEntries int + maxBytes int64 + evictMu sync.Mutex + corruptOnce sync.Once +} + +// NewFilesystem returns a Cache whose entries are persisted under dir. +// The directory is created on first Put if absent. Callers may pass +// any directory; the [New] factory uses `~/.cache/wfctl/diff/`. +func NewFilesystem(dir string) Cache { + return &filesystemCache{ + dir: dir, + maxEntries: defaultMaxEntries, + maxBytes: defaultMaxBytes, + } +} + +// pathFor returns the on-disk path for key. Exported (within-package) +// so tests can mutate the file directly to exercise the corruption +// recovery path. +func (c *filesystemCache) pathFor(k Key) string { + return filepath.Join(c.dir, keyFingerprint(k)+".json") +} + +func (c *filesystemCache) Get(k Key) (interfaces.DiffResult, bool) { + path := c.pathFor(k) + data, err := os.ReadFile(path) + if err != nil { + // Either file-not-found (cache miss, normal) or some other read + // error (permissions, transient I/O). Both are treated as miss + // — apply correctness must not hinge on Get success. + return interfaces.DiffResult{}, false + } + var env envelope + if err := json.Unmarshal(data, &env); err != nil { + c.handleCorruption(path, err) + return interfaces.DiffResult{}, false + } + if env.SchemaVersion != cacheSchemaVersion { + c.handleCorruption(path, errors.New("schema-version mismatch")) + return interfaces.DiffResult{}, false + } + return env.Result, true +} + +func (c *filesystemCache) Put(k Key, result interfaces.DiffResult) { + if err := os.MkdirAll(c.dir, 0o755); err != nil { + // Disk-side errors during Put are intentionally silent: the + // next Get will miss (correct), and the operator already has + // "stuff isn't working" signal from elsewhere. + return + } + env := envelope{SchemaVersion: cacheSchemaVersion, Result: result} + data, err := json.Marshal(env) + if err != nil { + return + } + path := c.pathFor(k) + _ = os.WriteFile(path, data, 0o644) + c.maybeEvict() +} + +// handleCorruption silently deletes the corrupt file and emits a +// once-per-process info log. The single log gives operators a +// breadcrumb without spamming on every Get when an attacker or a +// rogue tool has filled the cache directory with garbage files. +func (c *filesystemCache) handleCorruption(path string, cause error) { + _ = os.Remove(path) + c.corruptOnce.Do(func() { + log.Printf("info: diffcache: detected corrupted cache file %q (%v); silently re-Diffing. This message logs once per process.", path, cause) + }) +} + +// maybeEvict scans the cache directory and, if either cap is exceeded, +// evicts the oldest evictionFraction (10%) of entries by mtime. The +// scan + sort cost is amortized: a single Put after N entries +// triggers one full scan; the next eviction-fraction Puts after that +// see no scan because the cap is back below threshold. +func (c *filesystemCache) maybeEvict() { + c.evictMu.Lock() + defer c.evictMu.Unlock() + entries, err := os.ReadDir(c.dir) + if err != nil { + return + } + // Filter to *.json files (defensive in case of stray files). + type fileInfo struct { + path string + mtime int64 + size int64 + } + files := make([]fileInfo, 0, len(entries)) + var totalBytes int64 + for _, e := range entries { + if e.IsDir() || filepath.Ext(e.Name()) != ".json" { + continue + } + info, err := e.Info() + if err != nil { + continue + } + files = append(files, fileInfo{ + path: filepath.Join(c.dir, e.Name()), + mtime: info.ModTime().UnixNano(), + size: info.Size(), + }) + totalBytes += info.Size() + } + overCount := len(files) > c.maxEntries + overBytes := totalBytes > c.maxBytes + if !overCount && !overBytes { + return + } + // Sort oldest-first by mtime. Files written in close succession + // may share an mtime under some filesystems; the secondary sort + // by path makes the order deterministic. + sort.Slice(files, func(i, j int) bool { + if files[i].mtime != files[j].mtime { + return files[i].mtime < files[j].mtime + } + return files[i].path < files[j].path + }) + // Evict the oldest fraction in one pass. Always at least 1 so + // tiny caps still make progress. + evictCount := max(int(float64(len(files))*evictionFraction), 1) + evictCount = min(evictCount, len(files)) + for i := range evictCount { + _ = os.Remove(files[i].path) + } +} diff --git a/iac/diffcache/cache_inmemory.go b/iac/diffcache/cache_inmemory.go new file mode 100644 index 000000000..671b20895 --- /dev/null +++ b/iac/diffcache/cache_inmemory.go @@ -0,0 +1,44 @@ +package diffcache + +import ( + "sync" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// memoryCache is the in-memory Cache implementation. Entries live for +// the current process; CI workflows in this repo set +// WFCTL_DIFFCACHE=:memory: explicitly so containerized runners never +// write cache data to disk. +// +// Concurrency: a single mutex guards the map. Diff results are small +// and operations are infrequent (one per resource per Plan), so a +// finer-grained scheme isn't justified. +type memoryCache struct { + mu sync.Mutex + entries map[string]interfaces.DiffResult +} + +// NewMemory returns an in-memory Cache. The returned Cache does not +// enforce a size cap — process lifetime is the eviction horizon. +// This matches the rev2 lifecycle constraint that the in-memory mode +// is the CI default and is not relied on for correctness. +func NewMemory() Cache { + return &memoryCache{entries: map[string]interfaces.DiffResult{}} +} + +func (c *memoryCache) Get(k Key) (interfaces.DiffResult, bool) { + c.mu.Lock() + defer c.mu.Unlock() + r, ok := c.entries[keyFingerprint(k)] + if !ok { + return interfaces.DiffResult{}, false + } + return r, true +} + +func (c *memoryCache) Put(k Key, result interfaces.DiffResult) { + c.mu.Lock() + defer c.mu.Unlock() + c.entries[keyFingerprint(k)] = result +} diff --git a/iac/diffcache/cache_test.go b/iac/diffcache/cache_test.go new file mode 100644 index 000000000..9aa038245 --- /dev/null +++ b/iac/diffcache/cache_test.go @@ -0,0 +1,120 @@ +package diffcache + +import ( + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestCache_FilesystemRoundtrip verifies the basic Get/Put roundtrip on +// a filesystem-backed cache: Put a DiffResult under a Key, then Get the +// same key returns the same DiffResult with hit=true. +func TestCache_FilesystemRoundtrip(t *testing.T) { + dir := t.TempDir() + c := NewFilesystem(dir) + key := Key{ + PluginVersion: "do@v0.10.0", + Type: "infra.vpc", + ProviderID: "vpc-abc", + SHAConfig: "deadbeef", + SHAOutputs: "cafebabe", + } + want := interfaces.DiffResult{ + NeedsUpdate: true, + Changes: []interfaces.FieldChange{ + {Path: "size", Old: "s-1vcpu-1gb", New: "s-2vcpu-2gb"}, + }, + } + c.Put(key, want) + got, hit := c.Get(key) + if !hit { + t.Fatal("expected cache hit after Put") + } + if got.NeedsUpdate != want.NeedsUpdate { + t.Errorf("NeedsUpdate: got %v want %v", got.NeedsUpdate, want.NeedsUpdate) + } + if len(got.Changes) != 1 || got.Changes[0].Path != "size" { + t.Errorf("Changes: got %+v want one entry with Path=size", got.Changes) + } +} + +// TestCache_FilesystemMissOnDifferentKey verifies key isolation: a Put +// under one key does not service a Get under any of the 5 fields' +// distinct values. Each field must contribute to the key independently. +func TestCache_FilesystemMissOnDifferentKey(t *testing.T) { + dir := t.TempDir() + c := NewFilesystem(dir) + base := Key{PluginVersion: "v1", Type: "T", ProviderID: "P", SHAConfig: "C", SHAOutputs: "O"} + c.Put(base, interfaces.DiffResult{NeedsUpdate: true}) + + cases := map[string]Key{ + "diff-pluginversion": {PluginVersion: "v2", Type: "T", ProviderID: "P", SHAConfig: "C", SHAOutputs: "O"}, + "diff-type": {PluginVersion: "v1", Type: "U", ProviderID: "P", SHAConfig: "C", SHAOutputs: "O"}, + "diff-providerid": {PluginVersion: "v1", Type: "T", ProviderID: "Q", SHAConfig: "C", SHAOutputs: "O"}, + "diff-shaconfig": {PluginVersion: "v1", Type: "T", ProviderID: "P", SHAConfig: "D", SHAOutputs: "O"}, + "diff-shaoutputs": {PluginVersion: "v1", Type: "T", ProviderID: "P", SHAConfig: "C", SHAOutputs: "X"}, + } + for name, k := range cases { + t.Run(name, func(t *testing.T) { + if _, hit := c.Get(k); hit { + t.Errorf("expected miss for distinct key field; got hit on %+v", k) + } + }) + } +} + +// TestCache_MemoryRoundtrip is the in-memory cache's basic test. +// Same contract as the filesystem cache. +func TestCache_MemoryRoundtrip(t *testing.T) { + c := NewMemory() + key := Key{Type: "infra.vpc", ProviderID: "vpc-abc"} + want := interfaces.DiffResult{NeedsReplace: true} + c.Put(key, want) + got, hit := c.Get(key) + if !hit { + t.Fatal("expected cache hit after Put") + } + if got.NeedsReplace != want.NeedsReplace { + t.Errorf("NeedsReplace: got %v want %v", got.NeedsReplace, want.NeedsReplace) + } +} + +// TestCache_NoopAlwaysMisses verifies the disabled cache: Put is a +// no-op, every Get returns hit=false. +func TestCache_NoopAlwaysMisses(t *testing.T) { + c := NewNoop() + key := Key{Type: "x"} + c.Put(key, interfaces.DiffResult{NeedsUpdate: true}) + if _, hit := c.Get(key); hit { + t.Error("noop cache should always miss") + } +} + +// TestCache_EnvDispatch verifies the New() factory's env-var driven +// dispatch: WFCTL_DIFFCACHE=disabled → noop; =:memory: → memory; +// default → filesystem (we just verify it's not noop/memory). +func TestCache_EnvDispatch(t *testing.T) { + t.Run("disabled", func(t *testing.T) { + t.Setenv("WFCTL_DIFFCACHE", "disabled") + c := New() + if _, ok := c.(*noopCache); !ok { + t.Errorf("WFCTL_DIFFCACHE=disabled should yield *noopCache; got %T", c) + } + }) + t.Run("memory", func(t *testing.T) { + t.Setenv("WFCTL_DIFFCACHE", ":memory:") + c := New() + if _, ok := c.(*memoryCache); !ok { + t.Errorf("WFCTL_DIFFCACHE=:memory: should yield *memoryCache; got %T", c) + } + }) + t.Run("default", func(t *testing.T) { + // Set HOME to a tempdir so we don't pollute the real cache dir. + t.Setenv("HOME", t.TempDir()) + t.Setenv("WFCTL_DIFFCACHE", "") + c := New() + if _, ok := c.(*filesystemCache); !ok { + t.Errorf("default should yield *filesystemCache; got %T", c) + } + }) +} diff --git a/iac/refreshoutputs/refresh.go b/iac/refreshoutputs/refresh.go new file mode 100644 index 000000000..b2087b5e1 --- /dev/null +++ b/iac/refreshoutputs/refresh.go @@ -0,0 +1,105 @@ +// Package refreshoutputs implements read-only state refresh — it reads +// current Outputs from providers and updates the persisted state when fields +// differ. It never invokes Update or Replace at the cloud level; the contract +// is strictly "Read and reconcile in-memory state". +// +// Refresh is the foundation for two consumers in W-2: +// +// - wfctl infra refresh-outputs (T2.2): explicit operator-driven refresh. +// - wfctl infra apply pre-step (T2.3): opt-in via WFCTL_REFRESH_OUTPUTS to +// keep stale outputs from poisoning the planner. +package refreshoutputs + +import ( + "context" + "fmt" + "maps" + "reflect" + "sync" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// defaultConcurrency is the worker count used when Options.Concurrency is +// non-positive. +const defaultConcurrency = 8 + +// Options tunes Refresh behaviour. The zero value is valid and uses +// defaultConcurrency. +type Options struct { + // Concurrency is the maximum number of concurrent Read calls. Values < 1 + // fall back to defaultConcurrency (8). + Concurrency int +} + +// Refresh issues a bounded-concurrency Read against p for each entry in +// states and returns a copy with Outputs reconciled to the live values. +// Resources whose live Outputs are deeply equal to the persisted Outputs are +// returned unchanged (callers can rely on this to skip writes). +// +// Refresh never mutates the input slice. The returned slice is a fresh copy +// of states with possibly-updated Outputs maps; on any Read or +// ResourceDriver failure, Refresh returns (nil, err) and discards partial +// progress so callers don't half-persist a refresh. +// +// Aliasing: for entries whose live Outputs match the persisted Outputs, +// out[i].Outputs is the same map as states[i].Outputs (unchanged maps are +// not cloned). Callers must not mutate Outputs maps in the returned slice +// in place. +func Refresh(ctx context.Context, p interfaces.IaCProvider, states []interfaces.ResourceState, opts Options) ([]interfaces.ResourceState, error) { + if opts.Concurrency < 1 { + opts.Concurrency = defaultConcurrency + } + out := make([]interfaces.ResourceState, len(states)) + copy(out, states) + + sem := make(chan struct{}, opts.Concurrency) + errs := make([]error, len(states)) + var wg sync.WaitGroup + + for i := range states { + sem <- struct{}{} + wg.Go(func() { + defer func() { <-sem }() + errs[i] = refreshOne(ctx, p, &out[i], states[i]) + }) + } + wg.Wait() + + for _, e := range errs { + if e != nil { + return nil, e + } + } + return out, nil +} + +// refreshOne performs a single resource Read and writes the live Outputs +// into dst when they differ from src.Outputs. It returns nil on success or +// the error otherwise. +func refreshOne(ctx context.Context, p interfaces.IaCProvider, dst *interfaces.ResourceState, src interfaces.ResourceState) error { + d, err := p.ResourceDriver(src.Type) + if err != nil { + return err + } + ref := interfaces.ResourceRef{Name: src.Name, Type: src.Type, ProviderID: src.ProviderID} + live, err := d.Read(ctx, ref) + if err != nil { + return fmt.Errorf("could not refresh %q: %w", src.Name, err) + } + if !reflect.DeepEqual(live.Outputs, src.Outputs) { + dst.Outputs = cloneMap(live.Outputs) + } + return nil +} + +// cloneMap returns an independent shallow copy of m. Callers receive a map +// they can mutate without aliasing the live driver output. +func cloneMap(m map[string]any) map[string]any { + if m == nil { + return nil + } + c := make(map[string]any, len(m)) + maps.Copy(c, m) + return c +} diff --git a/iac/refreshoutputs/refresh_concurrency_test.go b/iac/refreshoutputs/refresh_concurrency_test.go new file mode 100644 index 000000000..da747cb65 --- /dev/null +++ b/iac/refreshoutputs/refresh_concurrency_test.go @@ -0,0 +1,209 @@ +package refreshoutputs + +import ( + "context" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// countingProvider is a stress-test IaCProvider that hands out a single +// shared countingDriver. Every Read call increments the per-ProviderID +// counter so the test can assert "exactly once per resource" after +// Refresh returns. +type countingProvider struct { + driver *countingDriver +} + +func (p *countingProvider) Name() string { panic("not used") } +func (p *countingProvider) Version() string { panic("not used") } +func (p *countingProvider) Initialize(context.Context, map[string]any) error { + panic("not used") +} +func (p *countingProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { + panic("not used") +} +func (p *countingProvider) Plan(context.Context, []interfaces.ResourceSpec, []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + panic("not used") +} +func (p *countingProvider) Apply(context.Context, *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + panic("not used") +} +func (p *countingProvider) Destroy(context.Context, []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + panic("not used") +} +func (p *countingProvider) Status(context.Context, []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + panic("not used") +} +func (p *countingProvider) DetectDrift(context.Context, []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + panic("not used") +} +func (p *countingProvider) Import(context.Context, string, string) (*interfaces.ResourceState, error) { + panic("not used") +} +func (p *countingProvider) ResolveSizing(string, interfaces.Size, *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + panic("not used") +} +func (p *countingProvider) SupportedCanonicalKeys() []string { panic("not used") } +func (p *countingProvider) BootstrapStateBackend(context.Context, map[string]any) (*interfaces.BootstrapResult, error) { + panic("not used") +} +func (p *countingProvider) Close() error { return nil } + +func (p *countingProvider) ResourceDriver(string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} + +// countingDriver atomically tracks how many times Read was called for +// each ProviderID and how many goroutines are inside Read at once. +// concurrentPeak gives the test a way to assert that the bounded +// semaphore actually enforced its cap. +type countingDriver struct { + mu sync.Mutex + callsByID map[string]int + inFlight atomic.Int32 + concurrentPeak atomic.Int32 +} + +func (d *countingDriver) Create(context.Context, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *countingDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + now := d.inFlight.Add(1) + defer d.inFlight.Add(-1) + for { + peak := d.concurrentPeak.Load() + if now <= peak || d.concurrentPeak.CompareAndSwap(peak, now) { + break + } + } + + // Hold each call long enough that, with N=100 and Concurrency=8, the + // bounded pool will queue up: every dispatched call MUST overlap with + // at least one other or the test's concurrency-cap check is vacuous. + // 5ms × 100 / 8 ≈ 63ms total wall time at the cap, well under the + // watchdog's 10s budget. + time.Sleep(5 * time.Millisecond) + + d.mu.Lock() + d.callsByID[ref.ProviderID]++ + d.mu.Unlock() + return &interfaces.ResourceOutput{ + Name: ref.Name, + Type: ref.Type, + ProviderID: ref.ProviderID, + // Add a "id" field so Refresh sees a diff and copies the new + // Outputs into out[i].Outputs — gives the test a positive + // per-resource signal that every refresh propagated. + Outputs: map[string]any{"id": ref.ProviderID, "fresh": true}, + }, nil +} +func (d *countingDriver) Update(context.Context, interfaces.ResourceRef, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *countingDriver) Delete(context.Context, interfaces.ResourceRef) error { + panic("not used") +} +func (d *countingDriver) Diff(context.Context, interfaces.ResourceSpec, *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + panic("not used") +} +func (d *countingDriver) HealthCheck(context.Context, interfaces.ResourceRef) (*interfaces.HealthResult, error) { + panic("not used") +} +func (d *countingDriver) Scale(context.Context, interfaces.ResourceRef, int) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *countingDriver) SensitiveKeys() []string { return nil } + +// TestRefresh_ConcurrencyStress_NoDeadlock_AllRefreshed_OnceEach exercises +// Refresh against 100 resources with Concurrency=8 and asserts: +// +// 1. No deadlock — Refresh returns within a generous watchdog budget. +// Caught by `done` channel + 10s timeout `select`. +// 2. Read was called exactly once per resource — no double-dispatch +// hidden bugs in the semaphore acquire/release pairing. +// 3. The resulting state slice carries the refreshed Outputs for every +// entry — no goroutine wrote into the wrong out[i] under concurrency. +// 4. Concurrent peak in flight is between 2 and the requested +// concurrency cap. ≥2 confirms the bounded pool actually parallelised +// work; ≤cap confirms the semaphore enforced its limit. +func TestRefresh_ConcurrencyStress_NoDeadlock_AllRefreshed_OnceEach(t *testing.T) { + const ( + nResources = 100 + concurrency = 8 + watchdog = 10 * time.Second + ) + + states := make([]interfaces.ResourceState, nResources) + for i := range states { + id := fmt.Sprintf("uuid-%03d", i) + states[i] = interfaces.ResourceState{ + Name: fmt.Sprintf("vpc-%03d", i), + Type: "infra.vpc", + ProviderID: id, + Outputs: map[string]any{"ip_range": "10.0.0.0/16"}, + } + } + + driver := &countingDriver{callsByID: make(map[string]int, nResources)} + provider := &countingProvider{driver: driver} + + done := make(chan struct{}) + var refreshed []interfaces.ResourceState + var refreshErr error + go func() { + defer close(done) + refreshed, refreshErr = Refresh(context.Background(), provider, states, Options{Concurrency: concurrency}) + }() + select { + case <-done: + // Refresh returned — proceed with assertions. + case <-time.After(watchdog): + t.Fatalf("Refresh did not return within %s — possible deadlock", watchdog) + } + + if refreshErr != nil { + t.Fatalf("Refresh: %v", refreshErr) + } + if len(refreshed) != nResources { + t.Fatalf("expected %d refreshed states, got %d", nResources, len(refreshed)) + } + + // Each ProviderID should have been Read exactly once. + driver.mu.Lock() + defer driver.mu.Unlock() + if len(driver.callsByID) != nResources { + t.Errorf("expected reads for %d distinct ProviderIDs, got %d", nResources, len(driver.callsByID)) + } + for id, n := range driver.callsByID { + if n != 1 { + t.Errorf("ProviderID %q: Read called %d times, want exactly 1", id, n) + } + } + + // Every state in the result must carry the refreshed Outputs map + // (driver returns "id" + "fresh": true on every Read). + for i, s := range refreshed { + if got, _ := s.Outputs["id"].(string); got != fmt.Sprintf("uuid-%03d", i) { + t.Errorf("refreshed[%d]: Outputs[\"id\"]=%q, want uuid-%03d", i, got, i) + } + if got, _ := s.Outputs["fresh"].(bool); !got { + t.Errorf("refreshed[%d]: Outputs[\"fresh\"]=%v, want true", i, got) + } + } + + // Concurrency-cap invariants: peak inflight must have been >1 to + // prove parallelism happened, and ≤concurrency to prove the + // semaphore enforced its limit. + peak := int(driver.concurrentPeak.Load()) + if peak < 2 { + t.Errorf("concurrent peak in flight = %d; expected >=2 (parallelism not exercised)", peak) + } + if peak > concurrency { + t.Errorf("concurrent peak in flight = %d; expected <=%d (semaphore cap exceeded)", peak, concurrency) + } +} diff --git a/iac/refreshoutputs/refresh_test.go b/iac/refreshoutputs/refresh_test.go new file mode 100644 index 000000000..7e5d66e98 --- /dev/null +++ b/iac/refreshoutputs/refresh_test.go @@ -0,0 +1,148 @@ +package refreshoutputs + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// fakeIaCProvider is a minimal IaCProvider stub that returns canned +// ResourceOutput values for Read via fakeResourceDriver. It only implements +// the methods that Refresh exercises (ResourceDriver); the rest panic to +// make accidental use during testing obvious. +type fakeIaCProvider struct { + // readOutputs maps ProviderID → fake driver output Outputs map. nil + // entries produce a ResourceOutput with empty Outputs. + readOutputs map[string]map[string]any + // readErr, when non-nil, causes the driver Read to return the error + // regardless of the resource ref. + readErr error +} + +func (f *fakeIaCProvider) Name() string { panic("not used") } +func (f *fakeIaCProvider) Version() string { panic("not used") } +func (f *fakeIaCProvider) Initialize(context.Context, map[string]any) error { + panic("not used") +} +func (f *fakeIaCProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { + panic("not used") +} +func (f *fakeIaCProvider) Plan(context.Context, []interfaces.ResourceSpec, []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + panic("not used") +} +func (f *fakeIaCProvider) Apply(context.Context, *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + panic("not used") +} +func (f *fakeIaCProvider) Destroy(context.Context, []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + panic("not used") +} +func (f *fakeIaCProvider) Status(context.Context, []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + panic("not used") +} +func (f *fakeIaCProvider) DetectDrift(context.Context, []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + panic("not used") +} +func (f *fakeIaCProvider) Import(context.Context, string, string) (*interfaces.ResourceState, error) { + panic("not used") +} +func (f *fakeIaCProvider) ResolveSizing(string, interfaces.Size, *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + panic("not used") +} +func (f *fakeIaCProvider) SupportedCanonicalKeys() []string { panic("not used") } +func (f *fakeIaCProvider) BootstrapStateBackend(context.Context, map[string]any) (*interfaces.BootstrapResult, error) { + panic("not used") +} +func (f *fakeIaCProvider) Close() error { return nil } + +func (f *fakeIaCProvider) ResourceDriver(string) (interfaces.ResourceDriver, error) { + return &fakeResourceDriver{provider: f}, nil +} + +// fakeResourceDriver answers Read from the parent fakeIaCProvider's +// readOutputs map. All other methods panic to make misuse loud. +type fakeResourceDriver struct { + provider *fakeIaCProvider +} + +func (d *fakeResourceDriver) Create(context.Context, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *fakeResourceDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + if d.provider.readErr != nil { + return nil, d.provider.readErr + } + out := d.provider.readOutputs[ref.ProviderID] + return &interfaces.ResourceOutput{ + Name: ref.Name, + Type: ref.Type, + ProviderID: ref.ProviderID, + Outputs: out, + }, nil +} +func (d *fakeResourceDriver) Update(context.Context, interfaces.ResourceRef, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *fakeResourceDriver) Delete(context.Context, interfaces.ResourceRef) error { + panic("not used") +} +func (d *fakeResourceDriver) Diff(context.Context, interfaces.ResourceSpec, *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + panic("not used") +} +func (d *fakeResourceDriver) HealthCheck(context.Context, interfaces.ResourceRef) (*interfaces.HealthResult, error) { + panic("not used") +} +func (d *fakeResourceDriver) Scale(context.Context, interfaces.ResourceRef, int) (*interfaces.ResourceOutput, error) { + panic("not used") +} +func (d *fakeResourceDriver) SensitiveKeys() []string { return nil } + +func mapsEqual(a, b map[string]any) bool { + if len(a) != len(b) { + return false + } + for k, v := range a { + bv, ok := b[k] + if !ok || bv != v { + return false + } + } + return true +} + +func TestRefreshOutputs_ReadsEachResource_PersistsChangedOnly(t *testing.T) { + states := []interfaces.ResourceState{ + {Name: "vpc-1", Type: "infra.vpc", ProviderID: "uuid-1", Outputs: map[string]any{"ip_range": "10.0.0.0/16"}}, + {Name: "vpc-2", Type: "infra.vpc", ProviderID: "uuid-2", Outputs: map[string]any{"ip_range": "10.1.0.0/16"}}, + } + fakeProvider := &fakeIaCProvider{readOutputs: map[string]map[string]any{ + "uuid-1": {"ip_range": "10.0.0.0/16", "id": "uuid-1"}, // new "id" field + "uuid-2": {"ip_range": "10.1.0.0/16"}, // unchanged + }} + refreshed, err := Refresh(context.Background(), fakeProvider, states, Options{Concurrency: 2}) + if err != nil { + t.Fatal(err) + } + if got := refreshed[0].Outputs["id"]; got != "uuid-1" { + t.Errorf("vpc-1 should have new 'id' output: %v", refreshed[0].Outputs) + } + if !mapsEqual(refreshed[1].Outputs, states[1].Outputs) { + t.Errorf("vpc-2 should be unchanged: %v vs %v", refreshed[1].Outputs, states[1].Outputs) + } +} + +func TestRefreshOutputs_PartialFailure_ReturnsError(t *testing.T) { + states := []interfaces.ResourceState{ + {Name: "vpc-1", Type: "infra.vpc", ProviderID: "uuid-1"}, + } + fakeProvider := &fakeIaCProvider{readErr: errors.New("network failure")} + _, err := Refresh(context.Background(), fakeProvider, states, Options{Concurrency: 1}) + if err == nil { + t.Fatalf("expected error on Read failure") + } + if !strings.Contains(err.Error(), "could not refresh") { + t.Errorf("error should mention 'could not refresh'; got: %v", err) + } +} diff --git a/iac/wfctlhelpers/apply.go b/iac/wfctlhelpers/apply.go new file mode 100644 index 000000000..871ce431b --- /dev/null +++ b/iac/wfctlhelpers/apply.go @@ -0,0 +1,369 @@ +// Package wfctlhelpers hosts the wfctl-side dispatch helper for v2 IaC +// plugins. wfctl calls [ApplyPlan] when a plugin manifest declares +// iacProvider.computePlanVersion: v2 (see plugin/sdk.IaCProvider). The +// helper iterates plan.Actions, fetches the matching ResourceDriver from +// the provider, and dispatches each action to a per-action sub-function +// (doCreate, doUpdate, doReplace, doDelete). +// +// Lifecycle inside W-3a: +// +// - T3.1 (this file's ApplyPlan + dispatch + skeleton sub-functions) +// - T3.1.5 — wraps ApplyPlan with the input-drift postcondition +// - T3.2 — fills doCreate with UpsertSupporter recovery +// - T3.3 — fills doUpdate + doDelete (the latent doDelete bug fix) +// - T3.4 — fills doReplace and populates ApplyResult.ReplaceIDMap +// +// Until W-3b lands the cmd/wfctl dispatch wiring, [ApplyPlan] has no +// in-tree caller — the helper ships in W-3a as foundation only and is +// exercised solely by this package's tests. +// +// # Per-action error-prefix policy +// +// Sub-functions follow a "decompose-then-prefix" rule for the strings +// recorded in [interfaces.ApplyResult].Errors[].Error: +// +// - doCreate, doUpdate, doDelete pass driver errors through +// unchanged. The ActionError struct already carries Resource + +// Action context fields, so a per-kind prefix would be redundant. +// - doCreate's upsert recovery path prefixes "upsert: " (e.g., +// "upsert: read after conflict: ...") because the failure is +// specifically about the recovery flow, not the original Create. +// - doReplace prefixes "replace: delete: " or "replace: create: " +// because a Replace decomposes into two driver calls — without +// the prefix, an operator reading result.Errors couldn't tell +// which sub-step failed. +// +// Tests in apply_update_delete_test.go and apply_replace_test.go +// lock this contract via exact-string assertions; future refactors +// that drop or rename a prefix fail loudly. +package wfctlhelpers + +import ( + "context" + "errors" + "fmt" + "log" + + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// ApplyPlan dispatches each plan action to the matching ResourceDriver on +// the provider. Per-action errors are recorded on result.Errors and do NOT +// abort the loop — apply best-effort across actions, surface every failure +// for the operator to triage. Context cancellation between actions IS +// respected: when ctx is canceled or its deadline expires, the loop stops +// at the next iteration boundary and returns ctx.Err() as the top-level +// error so a long apply terminates promptly on Ctrl-C / SIGTERM. +// +// At entry ApplyPlan captures result.InitialInputSnapshot by fingerprinting +// every name listed in plan.InputSnapshot through the OS env. After the +// dispatch loop completes — successfully or not — a deferred postcondition +// computes result.InputDriftReport against an apply-time snapshot taken +// through inputsnapshot.NewTolerantEnvProvider (sub-action env unsets are +// preserved, not flagged as drift). The postcondition is wrapped in +// recover() so a buggy env-provider closure cannot corrupt apply results; +// on panic, InputDriftReport is reset to nil and a warning is logged. +// +// The function is concurrency-safe with respect to its inputs: result is +// owned by ApplyPlan for the duration of the call and is not shared with +// the provider or driver implementations. +// +// T3.1 ships the dispatch skeleton; T3.1.5 added the postcondition above; +// T3.2/T3.3/T3.4 fill the per-action sub-functions with their full bodies. +func ApplyPlan(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return applyPlanWithEnvProvider(ctx, p, plan, nil) +} + +// applyPlanWithEnvProvider is the same-package test seam used by +// apply_postcondition_test.go to inject a custom apply-time env provider +// into the deferred drift postcondition (e.g., a panicky closure that +// stresses the recover() guard). When applyTimeEnv is nil, the function +// uses inputsnapshot.NewTolerantEnvProvider(plan.InputSnapshot) — the +// production behavior. The seam stays unexported because the only +// sanctioned external entry point is ApplyPlan. +func applyPlanWithEnvProvider( + ctx context.Context, + p interfaces.IaCProvider, + plan *interfaces.IaCPlan, + applyTimeEnv func(string) (string, bool), +) (*interfaces.ApplyResult, error) { + inputNames := snapshotKeys(plan.InputSnapshot) + result := &interfaces.ApplyResult{ + PlanID: plan.ID, + InitialInputSnapshot: inputsnapshot.Snapshot(inputNames, inputsnapshot.OSEnvProvider), + } + + // Deferred drift postcondition — runs unconditionally (success OR + // per-action failure), wrapped in recover() so a buggy env-provider + // closure (e.g., one freed mid-flight) cannot corrupt the apply + // result. On panic, drop the report rather than ship a partial one. + defer func() { + defer func() { + if r := recover(); r != nil { + result.InputDriftReport = nil + log.Printf("warning: input-drift postcondition panicked: %v", r) + } + }() + // Resolve the apply-time env provider lazily so the production + // path's NewTolerantEnvProvider closure isn't constructed when a + // test override is in play. + env := applyTimeEnv + if env == nil { + // CRITICAL: factory MUST be invoked here, NOT passed by + // reference (NewTolerantEnvProvider returns the closure; + // passing the function value would call the factory itself + // every Snapshot call and short-circuit the planSnapshot + // closure-capture). The cycle-4 reviewer caught this exact + // bug-class in the rev3 pseudo-code. + env = inputsnapshot.NewTolerantEnvProvider(plan.InputSnapshot) + } + applyTimeSnap := inputsnapshot.Snapshot(inputNames, env) + result.InputDriftReport = inputsnapshot.ComputeDrift(plan.InputSnapshot, applyTimeSnap) + }() + + for _, action := range plan.Actions { + // Honor cancellation at the loop boundary. Drivers should also + // check ctx internally for in-flight work, but the loop check + // guarantees apply stops between actions even if a driver + // happens to ignore ctx. The deferred postcondition still runs + // on early return so InputDriftReport is populated even on a + // canceled apply. + if err := ctx.Err(); err != nil { + return result, err + } + d, err := p.ResourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, interfaces.ActionError{ + Resource: action.Resource.Name, + Action: action.Action, + Error: fmt.Sprintf("resolve driver: %v", err), + }) + continue + } + if err := dispatchAction(ctx, d, action, result); err != nil { + result.Errors = append(result.Errors, interfaces.ActionError{ + Resource: action.Resource.Name, + Action: action.Action, + Error: err.Error(), + }) + } + } + return result, nil +} + +// snapshotKeys returns the keys of m as an unordered slice. ComputeDrift +// sorts its output, and Snapshot iterates in any order, so no key sort +// is needed at this stage. Inlined helper to keep the dependency +// surface minimal and avoid pulling in slices/maps at the postcondition +// call site. +func snapshotKeys(m map[string]string) []string { + if len(m) == 0 { + return nil + } + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + +// dispatchAction routes a single PlanAction to the per-kind sub-function. +// An unknown action kind returns an error which ApplyPlan records on +// result.Errors so an operator running a malformed plan sees a per-action +// diagnostic rather than a silent skip. +func dispatchAction(ctx context.Context, d interfaces.ResourceDriver, action interfaces.PlanAction, result *interfaces.ApplyResult) error { + switch action.Action { + case "create": + return doCreate(ctx, d, action, result) + case "update": + return doUpdate(ctx, d, action, result) + case "replace": + return doReplace(ctx, d, action, result) + case "delete": + return doDelete(ctx, d, action) + default: + return fmt.Errorf("unknown action %q", action.Action) + } +} + +// doCreate invokes Create and, on ErrResourceAlreadyExists, attempts an +// idempotent upsert recovery for drivers that opt in via the +// UpsertSupporter interface. The recovery path is: +// +// 1. Probe the driver for UpsertSupporter — if absent (interface +// assertion fails) or SupportsUpsert()==false, the original +// conflict surfaces unchanged. +// 2. Read the existing resource by Name + Type (no ProviderID — the +// driver's Read is responsible for locating by name when ProviderID +// is empty; SupportsUpsert()==true is the contract that this works). +// 3. Defensive: if Read returns an empty ProviderID, fail with a named +// diagnostic — Update with an empty ProviderID would route to a +// create-by-spec path on most drivers, defeating the upsert. +// 4. Update the existing resource with the desired spec, threading +// the ProviderID found in step 2. +// +// Recovery is single-pass: if the recovery Update itself returns +// ErrResourceAlreadyExists (a driver bug — Update with a known +// ProviderID should not conflict), the second conflict surfaces +// unchanged rather than retriggering the recovery loop. +// +// Error wrapping (in-package contract): +// +// - Read-after-conflict failures wrap both the original Create error +// and the Read error via errors.Join, so callers in this package +// can match either via errors.Is. +// - The doCreate return value preserves the wrap chain. ApplyPlan's +// dispatch loop, however, flattens errors to a string in +// result.Errors[].Error (see [ApplyPlan]) — external callers +// reading [interfaces.ApplyResult].Errors lose errors.Is matching +// and must inspect the canonical "upsert: read after conflict:" +// prefix instead. This boundary is deliberate: ActionError carries +// the per-resource action context fields the wrap chain otherwise +// duplicates. +func doCreate(ctx context.Context, d interfaces.ResourceDriver, action interfaces.PlanAction, result *interfaces.ApplyResult) error { + out, err := d.Create(ctx, action.Resource) + if errors.Is(err, interfaces.ErrResourceAlreadyExists) { + us, ok := d.(interfaces.UpsertSupporter) + if !ok || !us.SupportsUpsert() { + return err // no recovery available; surface the conflict + } + ref := interfaces.ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + existing, readErr := d.Read(ctx, ref) + if readErr != nil { + return fmt.Errorf("upsert: read after conflict: %w", errors.Join(err, readErr)) + } + if existing == nil || existing.ProviderID == "" { + return fmt.Errorf("upsert: resource %q found by name but ProviderID is empty: %w", ref.Name, err) + } + ref.ProviderID = existing.ProviderID + out, err = d.Update(ctx, ref, action.Resource) + } + if err == nil && out != nil { + result.Resources = append(result.Resources, *out) + } + return err +} + +// doUpdate invokes Update with a ResourceRef carrying action.Current's +// ProviderID (when action.Current is non-nil), appending the driver's +// returned ResourceOutput to result.Resources on success. Driver errors +// pass through unchanged so the caller's per-action error wrapper +// (ApplyPlan's loop body) records them with the canonical action + +// resource fields. +// +// Defensive contract: doUpdate does NOT synthesize a precondition error +// when action.Current is nil — the driver is the authority on what an +// empty ProviderID means. ComputePlan upstream is responsible for never +// emitting an Update without action.Current; if it does, the driver's +// own typed validation surfaces the bug. +func doUpdate(ctx context.Context, d interfaces.ResourceDriver, action interfaces.PlanAction, result *interfaces.ApplyResult) error { + ref := refFromAction(action) + out, err := d.Update(ctx, ref, action.Resource) + if err != nil { + return err + } + if out != nil { + result.Resources = append(result.Resources, *out) + } + return nil +} + +// doReplace decomposes a Replace action into Delete-then-Create on the +// driver and propagates the new ProviderID through +// result.ReplaceIDMap[action.Resource.Name] so JIT substitution in W-5 +// can patch dependent resources whose configs reference the replaced +// resource by name. +// +// Failure semantics: +// - Delete fails → return wrapped "replace: delete: "; Create +// does NOT run; ReplaceIDMap is NOT populated for this resource. +// - Delete succeeds, ctx canceled before Create → return wrapped +// "replace: canceled after delete: "; Create does NOT run; +// ReplaceIDMap is NOT populated. The half-replaced state is the +// operator's recovery surface (same as the Create-fails case). +// - Delete succeeds, Create fails → return wrapped +// "replace: create: "; ReplaceIDMap stays empty for this +// resource. Operators inspect the apply log + the empty-for-this- +// name slot in ReplaceIDMap to know which resources are in a +// half-replaced state and need manual cloud restoration. +// - Both succeed → result.Resources gets the new output appended, +// result.ReplaceIDMap[action.Resource.Name] = new ProviderID. Map +// is lazily-initialized on first successful Replace so plans with +// no Replace actions don't carry an empty map through serialisation. +// +// The "replace: ..." prefix is essential because Replace decomposes +// into two driver calls — without it, an operator reading +// result.Errors couldn't tell whether the Delete or the Create failed. +// Other sub-functions (doCreate non-recovery path, doUpdate, doDelete) +// pass driver errors through unchanged. +func doReplace(ctx context.Context, d interfaces.ResourceDriver, action interfaces.PlanAction, result *interfaces.ApplyResult) error { + if err := d.Delete(ctx, refFromAction(action)); err != nil { + return fmt.Errorf("replace: delete: %w", err) + } + // Honor cancellation between Delete and Create. Without this guard + // a Ctrl-C / SIGTERM that arrives mid-Replace would still trigger + // the Create call, leaving the operator without the cleanest + // possible interruption point. The half-replaced state is still + // recoverable (Delete already happened; Create did not, so + // ReplaceIDMap stays empty) but cancellation propagates fast. + if err := ctx.Err(); err != nil { + return fmt.Errorf("replace: canceled after delete: %w", err) + } + out, err := d.Create(ctx, action.Resource) + if err != nil { + return fmt.Errorf("replace: create: %w", err) + } + if out != nil { + result.Resources = append(result.Resources, *out) + // Lazy-init: only allocate the map when there's an actual + // entry to record. ApplyResult.ReplaceIDMap stays nil for + // plans with no Replace actions, which the omitempty JSON tag + // then drops from the encoded form (covered by + // TestApplyResult_OmitEmptyContract in interfaces/iac_state_test.go). + if result.ReplaceIDMap == nil { + result.ReplaceIDMap = make(map[string]string) + } + result.ReplaceIDMap[action.Resource.Name] = out.ProviderID + } + return nil +} + +// doDelete invokes Delete with a ResourceRef carrying action.Current's +// ProviderID. This closes the latent gap documented in the design +// (DOProvider.Apply has no "case delete" arm today, so wfctl's +// state-prune action silently skipped cloud-resource deletion through +// the v1 dispatch path); under v2 dispatch wfctlhelpers.ApplyPlan +// always invokes the driver's Delete, ensuring state-prune is paired +// with a real cloud-side mutation. +// +// Driver errors pass through unchanged for the caller's per-action +// error wrapping. doDelete does not append to result.Resources — a +// successful delete has no resource to record. +func doDelete(ctx context.Context, d interfaces.ResourceDriver, action interfaces.PlanAction) error { + return d.Delete(ctx, refFromAction(action)) +} + +// refFromAction builds a ResourceRef from the action's resource identity, +// threading the ProviderID from action.Current when the plan was built +// from existing state. For net-new actions (action.Current == nil) the +// returned ref has an empty ProviderID, matching the contract that +// drivers locate by Name when ProviderID is absent. +// +// Invariant: callers MUST ensure action.Current's Name/Type match +// action.Resource — Replace plans assume same-name same-type. If a future +// plan generator emits a Replace where Current.Name != Resource.Name +// (e.g., a rename across replace), the Delete would target the new name +// with the old ProviderID — likely a "not found" or wrong-resource bug. +// This function does not enforce the invariant; the contract is upstream +// in ComputePlan. +func refFromAction(action interfaces.PlanAction) interfaces.ResourceRef { + ref := interfaces.ResourceRef{ + Name: action.Resource.Name, + Type: action.Resource.Type, + } + if action.Current != nil { + ref.ProviderID = action.Current.ProviderID + } + return ref +} diff --git a/iac/wfctlhelpers/apply_create_test.go b/iac/wfctlhelpers/apply_create_test.go new file mode 100644 index 000000000..cbb440429 --- /dev/null +++ b/iac/wfctlhelpers/apply_create_test.go @@ -0,0 +1,267 @@ +package wfctlhelpers + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// fakeDriverWithUpsert is a ResourceDriver test double that: +// - returns createErr from Create (typically ErrResourceAlreadyExists) +// - returns readResult / readErr from Read +// - records whether Update was called via updateCalled +// - implements UpsertSupporter when supportsUpsert is true +// +// Used to exercise doCreate's upsert-recovery path. Embedded fakeDriver +// supplies the no-op other methods so the recorder is minimal. +type fakeDriverWithUpsert struct { + *fakeDriver + createErr error + readResult *interfaces.ResourceOutput + readErr error + updateCalled bool + updateOut *interfaces.ResourceOutput + supportsUpsert bool +} + +func (d *fakeDriverWithUpsert) Create(_ context.Context, _ interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + if d.createErr != nil { + return nil, d.createErr + } + return &interfaces.ResourceOutput{}, nil +} + +func (d *fakeDriverWithUpsert) Read(_ context.Context, _ interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + if d.readErr != nil { + return nil, d.readErr + } + return d.readResult, nil +} + +func (d *fakeDriverWithUpsert) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.updateCalled = true + d.fakeDriver.updateCount++ + if d.updateOut != nil { + return d.updateOut, nil + } + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil +} + +func (d *fakeDriverWithUpsert) SupportsUpsert() bool { return d.supportsUpsert } + +// upsertFakeProvider returns the fakeDriverWithUpsert from ResourceDriver +// regardless of resource type. +type upsertFakeProvider struct { + *fakeProvider + upsert *fakeDriverWithUpsert +} + +func newUpsertFakeProvider(d *fakeDriverWithUpsert) *upsertFakeProvider { + if d.fakeDriver == nil { + d.fakeDriver = &fakeDriver{} + } + return &upsertFakeProvider{fakeProvider: newFakeProvider(), upsert: d} +} + +func (p *upsertFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.upsert, nil +} + +// TestApplyPlan_Create_UpsertOnAlreadyExists is the canonical T3.2 test: +// a Create that returns ErrResourceAlreadyExists must trigger +// Read + Update on a driver that opts in via UpsertSupporter. The +// recovery must clear the per-action error so result.Errors is empty. +func TestApplyPlan_Create_UpsertOnAlreadyExists(t *testing.T) { + d := &fakeDriverWithUpsert{ + createErr: interfaces.ErrResourceAlreadyExists, + readResult: &interfaces.ResourceOutput{ProviderID: "found-uuid"}, + supportsUpsert: true, + } + fp := newUpsertFakeProvider(d) + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) > 0 { + t.Errorf("upsert should recover; got errors: %v", result.Errors) + } + if !d.updateCalled { + t.Errorf("upsert path should call Update after Read; updateCalled=%v", d.updateCalled) + } +} + +// TestApplyPlan_Create_AlreadyExists_NoUpsertSupport verifies the +// fall-through: if the driver does NOT implement UpsertSupporter (or +// returns SupportsUpsert()==false), the original ErrResourceAlreadyExists +// surfaces unchanged via result.Errors. No Read or Update happens. +func TestApplyPlan_Create_AlreadyExists_NoUpsertSupport(t *testing.T) { + d := &fakeDriverWithUpsert{ + createErr: interfaces.ErrResourceAlreadyExists, + supportsUpsert: false, + } + fp := newUpsertFakeProvider(d) + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }} + result, _ := ApplyPlan(context.Background(), fp, plan) + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if !strings.Contains(result.Errors[0].Error, "already exists") { + t.Errorf("expected ErrResourceAlreadyExists in error; got %q", result.Errors[0].Error) + } + if d.updateCalled { + t.Errorf("Update must not be called when SupportsUpsert returns false") + } +} + +// alreadyExistsBareDriver is a ResourceDriver that does NOT implement +// UpsertSupporter at all (no SupportsUpsert method). Used to exercise +// doCreate's `!ok` interface-assertion fall-through — distinct from +// the `SupportsUpsert()==false` path covered above. Behavioral +// outcome is identical (conflict surfaces unchanged, no Read/Update), +// but the code path through the type assertion is different. +type alreadyExistsBareDriver struct { + *fakeDriver +} + +func (d *alreadyExistsBareDriver) Create(_ context.Context, _ interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + return nil, interfaces.ErrResourceAlreadyExists +} + +// TestApplyPlan_Create_AlreadyExists_DriverDoesNotImplementUpsertSupporter +// covers the `!ok` arm of doCreate's `us, ok := d.(interfaces.UpsertSupporter)` +// type assertion: a driver that does not implement UpsertSupporter at +// all. Distinct from TestApplyPlan_Create_AlreadyExists_NoUpsertSupport, +// which covers the `ok && !us.SupportsUpsert()` arm. +func TestApplyPlan_Create_AlreadyExists_DriverDoesNotImplementUpsertSupporter(t *testing.T) { + bare := &alreadyExistsBareDriver{fakeDriver: &fakeDriver{}} + // Sanity-check the test premise: bare must NOT satisfy UpsertSupporter. + // If a future refactor lifts SupportsUpsert onto the embedded fakeDriver, + // this test would silently switch to the "ok && !SupportsUpsert" branch + // and stop covering the `!ok` arm. The compile-time assertion locks + // the premise. + var _ interfaces.ResourceDriver = bare + if _, ok := any(bare).(interfaces.UpsertSupporter); ok { + t.Fatal("test premise broken: alreadyExistsBareDriver must not implement UpsertSupporter") + } + // Inject the bare driver via a one-off provider that returns it for + // any resource type. + fp := &bareDriverProvider{fakeProvider: newFakeProvider(), driver: bare} + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }} + result, _ := ApplyPlan(context.Background(), fp, plan) + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if !strings.Contains(result.Errors[0].Error, "already exists") { + t.Errorf("expected ErrResourceAlreadyExists in error; got %q", result.Errors[0].Error) + } +} + +// bareDriverProvider returns the alreadyExistsBareDriver for any +// resource type so the test stays focused on the type-assertion +// fall-through. +type bareDriverProvider struct { + *fakeProvider + driver *alreadyExistsBareDriver +} + +func (p *bareDriverProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} + +// TestApplyPlan_Create_UpsertReadFailureWraps verifies the diagnostic +// when Read fails after an ErrResourceAlreadyExists conflict: the error +// wraps both the original create error and the read error so callers +// can match either via errors.Is. +func TestApplyPlan_Create_UpsertReadFailureWraps(t *testing.T) { + readErr := errors.New("read failed: 503") + d := &fakeDriverWithUpsert{ + createErr: interfaces.ErrResourceAlreadyExists, + readErr: readErr, + supportsUpsert: true, + } + fp := newUpsertFakeProvider(d) + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }} + result, _ := ApplyPlan(context.Background(), fp, plan) + if len(result.Errors) != 1 { + t.Fatalf("expected 1 error; got %d (%v)", len(result.Errors), result.Errors) + } + got := result.Errors[0].Error + if !strings.Contains(got, "upsert: read after conflict") { + t.Errorf("expected canonical 'upsert: read after conflict' prefix; got %q", got) + } + if !strings.Contains(got, "503") { + t.Errorf("expected wrapped read error in message; got %q", got) + } + if d.updateCalled { + t.Errorf("Update must not be called when Read fails") + } +} + +// TestApplyPlan_Create_UpsertEmptyProviderIDFails verifies a defensive +// check: if Read finds a resource by name but its ProviderID is empty, +// the upsert path must NOT call Update with an empty ProviderID (which +// would route to a Create-by-spec semantics). Instead, fail with a +// diagnostic that names the resource. +func TestApplyPlan_Create_UpsertEmptyProviderIDFails(t *testing.T) { + d := &fakeDriverWithUpsert{ + createErr: interfaces.ErrResourceAlreadyExists, + readResult: &interfaces.ResourceOutput{ProviderID: ""}, // defensive: empty ID + supportsUpsert: true, + } + fp := newUpsertFakeProvider(d) + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("vpc-1", "infra.vpc")}, + }} + result, _ := ApplyPlan(context.Background(), fp, plan) + if len(result.Errors) != 1 { + t.Fatalf("expected 1 error; got %d (%v)", len(result.Errors), result.Errors) + } + got := result.Errors[0].Error + if !strings.Contains(got, "ProviderID is empty") { + t.Errorf("expected 'ProviderID is empty' in diagnostic; got %q", got) + } + if !strings.Contains(got, "vpc-1") { + t.Errorf("diagnostic should name the resource; got %q", got) + } + if d.updateCalled { + t.Errorf("Update must not be called with empty ProviderID") + } +} + +// TestApplyPlan_Create_UpsertAppendsResources verifies happy-path +// plumbing: a successful upsert recovery must also append the Update's +// output to result.Resources so downstream state-write paths see the +// recovered resource exactly as if Create had succeeded directly. +func TestApplyPlan_Create_UpsertAppendsResources(t *testing.T) { + d := &fakeDriverWithUpsert{ + createErr: interfaces.ErrResourceAlreadyExists, + readResult: &interfaces.ResourceOutput{ProviderID: "found-uuid"}, + updateOut: &interfaces.ResourceOutput{Name: "vpc-1", Type: "infra.vpc", ProviderID: "found-uuid", Status: "active"}, + supportsUpsert: true, + } + fp := newUpsertFakeProvider(d) + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("vpc-1", "infra.vpc")}, + }} + result, _ := ApplyPlan(context.Background(), fp, plan) + if len(result.Resources) != 1 { + t.Fatalf("expected 1 resource appended; got %d (%+v)", len(result.Resources), result.Resources) + } + if got := result.Resources[0].ProviderID; got != "found-uuid" { + t.Errorf("ProviderID: got %q want %q", got, "found-uuid") + } +} diff --git a/iac/wfctlhelpers/apply_postcondition_test.go b/iac/wfctlhelpers/apply_postcondition_test.go new file mode 100644 index 000000000..b7ef765b6 --- /dev/null +++ b/iac/wfctlhelpers/apply_postcondition_test.go @@ -0,0 +1,237 @@ +package wfctlhelpers + +import ( + "context" + "errors" + "os" + "testing" + + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// fingerprint delegates to inputsnapshot.Compute so this test file's +// fixtures always use the production fingerprint algorithm. If +// Compute's prefix length or hash algorithm ever changes, every +// expected-fingerprint constant flowing through here re-aligns +// automatically — keeping the cross-package guarantee that +// `apply_postcondition_test.go::fingerprint` and +// `cmd/wfctl/infra_apply_plan_test.go::fingerprintForTest` produce +// identical outputs. Mirrors that helper deliberately. +func fingerprint(value string) string { + snap := inputsnapshot.Compute([]string{"k"}, func(string) (string, bool) { return value, true }) + return snap["k"] +} + +// TestApplyPlan_InitialInputSnapshot_CapturedAtEntry verifies that +// ApplyPlan populates ApplyResult.InitialInputSnapshot at apply entry by +// fingerprinting every name listed in plan.InputSnapshot through the +// real OS env. T3.1.5 lands this capture; T3.1's skeleton did not. +func TestApplyPlan_InitialInputSnapshot_CapturedAtEntry(t *testing.T) { + t.Setenv("WFCTL_T315_FOO", "value-foo") + t.Setenv("WFCTL_T315_BAR", "value-bar") + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{ + "WFCTL_T315_FOO": fingerprint("value-foo"), + "WFCTL_T315_BAR": fingerprint("value-bar"), + }, + } + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if got, want := result.InitialInputSnapshot["WFCTL_T315_FOO"], fingerprint("value-foo"); got != want { + t.Errorf("InitialInputSnapshot[WFCTL_T315_FOO]: got %q want %q", got, want) + } + if got, want := result.InitialInputSnapshot["WFCTL_T315_BAR"], fingerprint("value-bar"); got != want { + t.Errorf("InitialInputSnapshot[WFCTL_T315_BAR]: got %q want %q", got, want) + } +} + +// TestApply_Postcondition_PanicDoesNotCorruptResult verifies that a +// panic inside the deferred postcondition (e.g., a buggy env-provider +// closure) does not crash apply or surface as the top-level error. +// Recovers and clears result.InputDriftReport so an operator sees an +// empty drift report rather than a partially-populated one. Uses the +// internal seam applyPlanWithEnvProvider to inject the panicky provider. +func TestApply_Postcondition_PanicDoesNotCorruptResult(t *testing.T) { + panickyEnv := func(_ string) (string, bool) { panic("env-provider closure freed") } + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{"FOO": fingerprint("v")}, + } + fp := newFakeProvider() + result, err := applyPlanWithEnvProvider(context.Background(), fp, plan, panickyEnv) + if err != nil { + t.Fatalf("apply should not surface postcondition panic: %v", err) + } + if result.InputDriftReport != nil { + t.Errorf("on postcondition panic, drift report should be nil; got %+v", result.InputDriftReport) + } +} + +// TestApply_Postcondition_FingerprintAfterEnvUnset_NoFalsePositive +// covers the cycle-3 sub-action cleanup case: an apply action unsets a +// credential env var (e.g., the post-apply security hardening pattern). +// The postcondition must NOT flag this as drift — the operator's mental +// model says the value at plan time is what matters; an unset post-apply +// is not "the env changed mid-flight" in the user-facing sense. The +// production NewTolerantEnvProvider preserves fingerprints for +// plan-time-set apply-time-unset vars via the in-package sentinel; this +// test exercises that path end-to-end through ApplyPlan. +func TestApply_Postcondition_FingerprintAfterEnvUnset_NoFalsePositive(t *testing.T) { + const varName = "WFCTL_T315_PG_PASSWORD" + t.Setenv(varName, "value") + planFP := fingerprint("value") + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{varName: planFP}, + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }, + } + // envUnsetingFakeProvider unsets the env var inside Create, simulating + // a sub-action cleanup that happens between snapshot capture and + // postcondition execution. + fp := &envUnsetingFakeProvider{ + fakeProvider: newFakeProvider(), + varToUnset: varName, + } + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.InputDriftReport) != 0 { + t.Errorf("post-apply env-unset must not trigger drift false-positive; got: %+v", result.InputDriftReport) + } + // Sanity: the sub-action did unset the var. + if _, set := os.LookupEnv(varName); set { + t.Errorf("test setup: sub-action did not unset %s", varName) + } +} + +// envUnsetingFakeProvider returns a fake driver whose Create method +// unsets a configured env var as a side-effect, simulating the +// sub-action credential-cleanup pattern that the tolerant env provider +// must accommodate without false-positive drift. +type envUnsetingFakeProvider struct { + *fakeProvider + varToUnset string +} + +func (p *envUnsetingFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return &envUnsetingFakeDriver{fakeDriver: p.driver, varToUnset: p.varToUnset}, nil +} + +type envUnsetingFakeDriver struct { + *fakeDriver + varToUnset string +} + +func (d *envUnsetingFakeDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + out, err := d.fakeDriver.Create(ctx, spec) + if err != nil { + return out, err + } + _ = os.Unsetenv(d.varToUnset) + return out, nil +} + +// TestApplyPlan_PlanStaleDiagnostic_NamesChangedKeys is the canonical +// drift-detection test: an env var captured at plan time has a +// different value at apply time. The postcondition must record exactly +// one drift entry naming the changed key, with both fingerprints +// distinct. +func TestApplyPlan_PlanStaleDiagnostic_NamesChangedKeys(t *testing.T) { + const varName = "WFCTL_T315_STAGING_PG_PASSWORD" + planFP := fingerprint("old-value") + t.Setenv(varName, "new-value") // post-plan env value differs from plan-time fingerprint + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{varName: planFP}, + } + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.InputDriftReport) != 1 { + t.Fatalf("expected 1 drift entry, got %d (%+v)", len(result.InputDriftReport), result.InputDriftReport) + } + got := result.InputDriftReport[0] + if got.Name != varName { + t.Errorf("Name: got %q want %q", got.Name, varName) + } + if got.PlanFingerprint != planFP { + t.Errorf("PlanFingerprint: got %q want %q", got.PlanFingerprint, planFP) + } + if got.ApplyFingerprint == planFP || got.ApplyFingerprint == "" { + t.Errorf("ApplyFingerprint should be a distinct, non-empty value; got %q", got.ApplyFingerprint) + } +} + +// TestApplyPlan_NoDriftWhenInputSnapshotEmpty verifies the no-op case: +// a plan with no InputSnapshot must produce no drift report. Avoids +// the postcondition incorrectly synthesizing entries for vars that +// were never tracked. +func TestApplyPlan_NoDriftWhenInputSnapshotEmpty(t *testing.T) { + plan := &interfaces.IaCPlan{} + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.InputDriftReport) != 0 { + t.Errorf("empty InputSnapshot must yield empty drift; got %+v", result.InputDriftReport) + } +} + +// errFromDispatch is a sentinel for the "apply errored AND drift +// detected" interleave test. Confirms the deferred postcondition runs +// regardless of dispatch outcome. +var errFromDispatch = errors.New("dispatch failure (test)") + +// TestApplyPlan_PostconditionRunsEvenIfDispatchErrored verifies the +// "regardless of apply success/error path" contract on the +// InputDriftReport godoc. A driver that returns an error must still +// allow drift detection to populate the report — the postcondition is +// strictly best-effort but unconditional. +func TestApplyPlan_PostconditionRunsEvenIfDispatchErrored(t *testing.T) { + const varName = "WFCTL_T315_DRIFT_DURING_FAIL" + planFP := fingerprint("plan-time") + t.Setenv(varName, "apply-time") + plan := &interfaces.IaCPlan{ + InputSnapshot: map[string]string{varName: planFP}, + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + }, + } + fp := &erroringFakeProvider{fakeProvider: newFakeProvider()} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Errorf("expected per-action error from erroring driver; got %d (%v)", len(result.Errors), result.Errors) + } + if len(result.InputDriftReport) != 1 { + t.Errorf("postcondition must populate drift report even when dispatch errors; got %+v", result.InputDriftReport) + } +} + +// erroringFakeProvider returns a driver whose Create returns +// errFromDispatch so apply produces a per-action error. +type erroringFakeProvider struct { + *fakeProvider +} + +func (p *erroringFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return &erroringFakeDriver{fakeDriver: p.driver}, nil +} + +type erroringFakeDriver struct { + *fakeDriver +} + +func (d *erroringFakeDriver) Create(_ context.Context, _ interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + return nil, errFromDispatch +} diff --git a/iac/wfctlhelpers/apply_replace_test.go b/iac/wfctlhelpers/apply_replace_test.go new file mode 100644 index 000000000..5e215bb84 --- /dev/null +++ b/iac/wfctlhelpers/apply_replace_test.go @@ -0,0 +1,316 @@ +package wfctlhelpers + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// orderRecordingDriver records the order of Delete vs. Create +// invocations so doReplace's "delete THEN create" contract can be +// asserted exactly. createReturn is the canned Create output (carries +// the NEW ProviderID); deleteFn / createFn are optional hooks that +// override the default success behavior. +type orderRecordingDriver struct { + *fakeDriver + deleteAt int // sequence number when Delete was called (0 if not called) + createAt int // sequence number when Create was called (0 if not called) + step int + createReturn *interfaces.ResourceOutput + deleteErr error + createErr error +} + +func (d *orderRecordingDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { + d.fakeDriver.deleteCount++ + d.step++ + d.deleteAt = d.step + return d.deleteErr +} + +func (d *orderRecordingDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + d.step++ + d.createAt = d.step + if d.createErr != nil { + return nil, d.createErr + } + if d.createReturn != nil { + return d.createReturn, nil + } + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: "fake-id-" + spec.Name}, nil +} + +// orderRecordingProvider returns the orderRecordingDriver for any +// resource type. +type orderRecordingProvider struct { + *fakeProvider + driver *orderRecordingDriver +} + +func newOrderRecordingProvider() *orderRecordingProvider { + base := newFakeProvider() + return &orderRecordingProvider{ + fakeProvider: base, + driver: &orderRecordingDriver{fakeDriver: base.driver}, + } +} + +func (p *orderRecordingProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} + +// stateWithID is a helper that builds a ResourceState with a known +// ProviderID, used to seed the "old" side of a Replace action. +func stateWithID(name, providerID string) *interfaces.ResourceState { + return &interfaces.ResourceState{Name: name, ProviderID: providerID} +} + +// TestApplyPlan_Replace_DeletesThenCreates_PropagatesNewID is the +// canonical T3.4 test: Replace must (1) call Delete first, (2) call +// Create after the Delete, (3) thread the NEW ProviderID from Create +// into result.Resources. +func TestApplyPlan_Replace_DeletesThenCreates_PropagatesNewID(t *testing.T) { + fp := newOrderRecordingProvider() + fp.driver.createReturn = &interfaces.ResourceOutput{ + Name: "vpc", Type: "infra.vpc", ProviderID: "new-uuid", + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-uuid")}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if fp.driver.deleteAt == 0 || fp.driver.createAt == 0 { + t.Errorf("Replace should call both Delete and Create; deleteAt=%d createAt=%d", + fp.driver.deleteAt, fp.driver.createAt) + } + if fp.driver.createAt < fp.driver.deleteAt { + t.Errorf("Create must run AFTER Delete; deleteAt=%d createAt=%d", + fp.driver.deleteAt, fp.driver.createAt) + } + if len(result.Resources) != 1 || result.Resources[0].ProviderID != "new-uuid" { + t.Errorf("expected new ProviderID in result.Resources, got %+v", result.Resources) + } +} + +// TestApplyPlan_Replace_PopulatesReplaceIDMap is the new-in-T3.4 +// contract: result.ReplaceIDMap[action.Resource.Name] must equal the +// new ProviderID returned by Create. Keyed by the *replaced* resource's +// Name (per T3.0.4 godoc — fixed during T3.0.4 review). Lazy-init on +// first Replace. +func TestApplyPlan_Replace_PopulatesReplaceIDMap(t *testing.T) { + fp := newOrderRecordingProvider() + fp.driver.createReturn = &interfaces.ResourceOutput{ + Name: "vpc", Type: "infra.vpc", ProviderID: "new-uuid", + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-uuid")}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if got := result.ReplaceIDMap["vpc"]; got != "new-uuid" { + t.Errorf("ReplaceIDMap[vpc]: got %q want %q (full map: %+v)", got, "new-uuid", result.ReplaceIDMap) + } +} + +// TestApplyPlan_Replace_MultipleActionsAllPopulate verifies the map +// accumulates across actions: two Replace actions in one plan must +// produce two entries in result.ReplaceIDMap, each keyed by its +// replaced resource's name. +func TestApplyPlan_Replace_MultipleActionsAllPopulate(t *testing.T) { + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-vpc")}, + {Action: "replace", Resource: spec("db", "infra.database"), Current: stateWithID("db", "old-db")}, + }} + // Use a per-call provider that returns a fresh ID per resource so + // the map entries are distinguishable. + fp := &perResourceReplaceProvider{ + fakeProvider: newFakeProvider(), + newIDs: map[string]string{"vpc": "new-vpc-id", "db": "new-db-id"}, + } + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if got := result.ReplaceIDMap["vpc"]; got != "new-vpc-id" { + t.Errorf("ReplaceIDMap[vpc]: got %q want %q", got, "new-vpc-id") + } + if got := result.ReplaceIDMap["db"]; got != "new-db-id" { + t.Errorf("ReplaceIDMap[db]: got %q want %q", got, "new-db-id") + } +} + +// perResourceReplaceProvider mints a fresh new-ID per resource Name so +// MultipleActionsAllPopulate can distinguish the two map entries. Each +// driver is a one-shot recorder owned by the provider. +type perResourceReplaceProvider struct { + *fakeProvider + newIDs map[string]string + driver *perResourceReplaceDriver +} + +func (p *perResourceReplaceProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + if p.driver == nil { + p.driver = &perResourceReplaceDriver{fakeDriver: p.fakeProvider.driver, newIDs: p.newIDs} + } + return p.driver, nil +} + +type perResourceReplaceDriver struct { + *fakeDriver + newIDs map[string]string +} + +func (d *perResourceReplaceDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { + d.fakeDriver.deleteCount++ + return nil +} + +func (d *perResourceReplaceDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + id := d.newIDs[spec.Name] + if id == "" { + id = "fallback-id" + } + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: id}, nil +} + +// TestApplyPlan_Replace_DeleteFailsDoesNotCreate verifies that when +// the Delete sub-step of a Replace fails, Create is NOT called and +// result.ReplaceIDMap is NOT populated for that resource. The +// per-action error must surface with the canonical "replace: delete:" +// prefix that doReplace decorates (not bare driver error — Replace +// decomposes, so the prefix tells the operator which sub-step failed). +func TestApplyPlan_Replace_DeleteFailsDoesNotCreate(t *testing.T) { + fp := newOrderRecordingProvider() + fp.driver.deleteErr = errors.New("delete failed: 503") + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-uuid")}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if fp.driver.createAt != 0 { + t.Errorf("Create must not run when Delete fails; createAt=%d", fp.driver.createAt) + } + if _, present := result.ReplaceIDMap["vpc"]; present { + t.Errorf("ReplaceIDMap must not contain vpc when Delete failed; got %+v", result.ReplaceIDMap) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if !strings.HasPrefix(result.Errors[0].Error, "replace: delete:") { + t.Errorf("expected canonical 'replace: delete:' prefix; got %q", result.Errors[0].Error) + } +} + +// TestApplyPlan_Replace_CtxCancelAfterDelete_SkipsCreate verifies the +// post-Delete cancellation guard: if ctx is canceled between Delete +// and Create, doReplace must NOT call Create — instead returning a +// wrapped error with the canonical "replace: canceled after delete:" +// prefix. The half-replaced state is the operator's recovery surface +// (Delete happened, Create did not, ReplaceIDMap stays empty for the +// resource). +func TestApplyPlan_Replace_CtxCancelAfterDelete_SkipsCreate(t *testing.T) { + // cancelOnDeleteDriver cancels the context inside Delete after + // recording the call but before returning. This simulates + // SIGTERM/SIGINT arriving exactly between the Delete and Create + // driver calls. + ctx, cancel := context.WithCancel(context.Background()) + fp := &cancelOnDeleteFakeProvider{ + fakeProvider: newFakeProvider(), + cancel: cancel, + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-uuid")}, + }} + result, err := ApplyPlan(ctx, fp, plan) + if err != nil { + // ApplyPlan's loop check catches the cancellation at the next + // iteration, but the per-action ctx check inside doReplace + // fires first. Either path yields a per-action error rather + // than a top-level error from this single-action plan. + t.Fatalf("ApplyPlan should not surface top-level error: %v", err) + } + if fp.driver.deleteCount != 1 { + t.Errorf("Delete should have run before cancellation; deleteCount=%d", fp.driver.deleteCount) + } + if fp.driver.createCount != 0 { + t.Errorf("Create must NOT run after ctx cancellation; createCount=%d", fp.driver.createCount) + } + if _, present := result.ReplaceIDMap["vpc"]; present { + t.Errorf("ReplaceIDMap must not contain vpc when Create skipped; got %+v", result.ReplaceIDMap) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if !strings.HasPrefix(result.Errors[0].Error, "replace: canceled after delete:") { + t.Errorf("expected canonical 'replace: canceled after delete:' prefix; got %q", result.Errors[0].Error) + } +} + +// cancelOnDeleteFakeProvider returns a driver whose Delete cancels a +// supplied context.CancelFunc as a side-effect, simulating exact +// post-Delete pre-Create cancellation. +type cancelOnDeleteFakeProvider struct { + *fakeProvider + cancel context.CancelFunc + driver *cancelOnDeleteDriver +} + +func (p *cancelOnDeleteFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + if p.driver == nil { + p.driver = &cancelOnDeleteDriver{fakeDriver: p.fakeProvider.driver, cancel: p.cancel} + } + return p.driver, nil +} + +type cancelOnDeleteDriver struct { + *fakeDriver + cancel context.CancelFunc +} + +func (d *cancelOnDeleteDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { + d.fakeDriver.deleteCount++ + d.cancel() // ctx is now canceled; Create must not run. + return nil +} + +// TestApplyPlan_Replace_CreateFailsLeavesMapEmpty verifies the +// post-Delete pre-Create failure window: Delete succeeded but Create +// failed → ReplaceIDMap stays empty for this resource (no spurious +// new-ID entry). The plan rollback note says operators inspect this +// state checkpoint to know which resources are in a half-replaced +// state; "absent from ReplaceIDMap + Resource was Replace target" is +// the canonical signal. +func TestApplyPlan_Replace_CreateFailsLeavesMapEmpty(t *testing.T) { + fp := newOrderRecordingProvider() + fp.driver.createErr = errors.New("create failed: 422") + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("vpc", "infra.vpc"), Current: stateWithID("vpc", "old-uuid")}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if fp.driver.deleteAt == 0 { + t.Errorf("Delete should still have run (failure was in Create, not Delete)") + } + if _, present := result.ReplaceIDMap["vpc"]; present { + t.Errorf("ReplaceIDMap must not contain vpc when Create failed; got %+v", result.ReplaceIDMap) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if !strings.HasPrefix(result.Errors[0].Error, "replace: create:") { + t.Errorf("expected canonical 'replace: create:' prefix; got %q", result.Errors[0].Error) + } +} diff --git a/iac/wfctlhelpers/apply_test.go b/iac/wfctlhelpers/apply_test.go new file mode 100644 index 000000000..5432740c7 --- /dev/null +++ b/iac/wfctlhelpers/apply_test.go @@ -0,0 +1,339 @@ +package wfctlhelpers + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// fakeDriver counts CRUD-method invocations so dispatch tests can prove +// each per-action sub-function ran the right number of times. Counters +// are integers (not booleans) because the Replace action decomposes into +// Delete-then-Create on the driver — a boolean recorder can't tell +// "Replace dispatched correctly" apart from "explicit Create + explicit +// Delete also ran in the same plan." Each test gets a fresh instance via +// newFakeProvider(). +type fakeDriver struct { + createCount int + updateCount int + deleteCount int +} + +func (d *fakeDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.createCount++ + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: "fake-id-" + spec.Name}, nil +} + +func (d *fakeDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + return &interfaces.ResourceOutput{Name: ref.Name, Type: ref.Type, ProviderID: ref.ProviderID}, nil +} + +func (d *fakeDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.updateCount++ + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil +} + +func (d *fakeDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { + d.deleteCount++ + return nil +} + +func (d *fakeDriver) Diff(_ context.Context, _ interfaces.ResourceSpec, _ *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + return &interfaces.DiffResult{}, nil +} + +func (d *fakeDriver) HealthCheck(_ context.Context, _ interfaces.ResourceRef) (*interfaces.HealthResult, error) { + return &interfaces.HealthResult{Healthy: true}, nil +} + +func (d *fakeDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ int) (*interfaces.ResourceOutput, error) { + return nil, nil +} + +func (d *fakeDriver) SensitiveKeys() []string { return nil } + +// fakeProvider returns the same fakeDriver for every resource type so a +// single instance can record all dispatch invocations within one +// ApplyPlan call. The driverErr field, when non-nil, is returned from +// ResourceDriver instead of the driver — used by tests that need to +// exercise the resolve-driver-error path. +type fakeProvider struct { + driver *fakeDriver + driverErr error +} + +func newFakeProvider() *fakeProvider { return &fakeProvider{driver: &fakeDriver{}} } + +func (p *fakeProvider) Name() string { return "fake" } +func (p *fakeProvider) Version() string { return "0.0.0" } +func (p *fakeProvider) Initialize(_ context.Context, _ map[string]any) error { return nil } +func (p *fakeProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } +func (p *fakeProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return &interfaces.IaCPlan{}, nil +} +func (p *fakeProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return &interfaces.ApplyResult{}, nil +} +func (p *fakeProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (p *fakeProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (p *fakeProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (p *fakeProvider) Import(_ context.Context, _ string, _ string) (*interfaces.ResourceState, error) { + return nil, nil +} +func (p *fakeProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (p *fakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + if p.driverErr != nil { + return nil, p.driverErr + } + return p.driver, nil +} +func (p *fakeProvider) SupportedCanonicalKeys() []string { return nil } +func (p *fakeProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (p *fakeProvider) Close() error { return nil } + +// spec is a minimal ResourceSpec helper for tests. +func spec(name, typ string) interfaces.ResourceSpec { + return interfaces.ResourceSpec{Name: name, Type: typ} +} + +// state is a minimal ResourceState helper for tests; sets a deterministic +// ProviderID so doUpdate / doDelete / doReplace have something to thread. +func state(name string) *interfaces.ResourceState { + return &interfaces.ResourceState{Name: name, ProviderID: "old-id-" + name} +} + +// TestApplyPlan_HandlesAllFourActions is the T3.1 dispatch smoke test: +// one PlanAction of each kind (create/update/replace/delete) must reach +// the ResourceDriver. Asserts via integer call counts so a missing +// "case replace" arm in dispatchAction is detectable: a 4-action plan +// [create, update, replace, delete] must produce exactly 2 Create, 1 +// Update, 2 Delete. If Replace dispatch were silently dropped, those +// counts would shift to 1/1/1 and the assertion would fail. +func TestApplyPlan_HandlesAllFourActions(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + {Action: "update", Resource: spec("b", "infra.vpc"), Current: state("b")}, + {Action: "replace", Resource: spec("c", "infra.vpc"), Current: state("c")}, + {Action: "delete", Resource: spec("d", "infra.vpc"), Current: state("d")}, + }, + } + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Errorf("expected no errors, got %v", result.Errors) + } + // Replace = Delete + Create on the driver. + // Plan = [create, update, replace, delete] → + // Create: 1 (from create) + 1 (from replace) = 2 + // Update: 1 (from update) + // Delete: 1 (from delete) + 1 (from replace) = 2 + if got, want := fp.driver.createCount, 2; got != want { + t.Errorf("Create call count: got %d, want %d", got, want) + } + if got, want := fp.driver.updateCount, 1; got != want { + t.Errorf("Update call count: got %d, want %d", got, want) + } + if got, want := fp.driver.deleteCount, 2; got != want { + t.Errorf("Delete call count: got %d, want %d", got, want) + } +} + +// TestApplyPlan_ReplaceDispatchesViaDeleteThenCreate isolates Replace +// dispatch from explicit create/delete actions: a plan containing ONLY a +// single Replace action must produce exactly 1 Delete + 1 Create. If +// Replace were routed to default (unknown action) or to a different arm, +// these counts would not both be 1. +func TestApplyPlan_ReplaceDispatchesViaDeleteThenCreate(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "replace", Resource: spec("c", "infra.vpc"), Current: state("c")}, + }, + } + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Errorf("expected no errors, got %v", result.Errors) + } + if got, want := fp.driver.deleteCount, 1; got != want { + t.Errorf("Replace.deleteCount: got %d, want %d", got, want) + } + if got, want := fp.driver.createCount, 1; got != want { + t.Errorf("Replace.createCount: got %d, want %d", got, want) + } + if got, want := fp.driver.updateCount, 0; got != want { + t.Errorf("Replace.updateCount: got %d, want %d", got, want) + } +} + +// TestApplyPlan_UnknownActionRecordsError verifies the dispatch's +// default-case behavior: an action with an unrecognised kind should not +// crash but must surface a per-action error in result.Errors so an +// operator running a malformed plan sees the diagnostic instead of a +// silently dropped action. +func TestApplyPlan_UnknownActionRecordsError(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "frobnicate", Resource: spec("x", "infra.vpc")}, + }, + } + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatalf("ApplyPlan should not return a top-level error for a per-action issue; got %v", err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error, got %d (%v)", len(result.Errors), result.Errors) + } + if result.Errors[0].Resource != "x" || result.Errors[0].Action != "frobnicate" { + t.Errorf("unexpected error shape: %+v", result.Errors[0]) + } +} + +// TestApplyPlan_PreservesPlanID checks the ApplyResult.PlanID echo. Future +// log-correlation paths rely on this so an operator can match an apply +// invocation back to the persisted plan. +func TestApplyPlan_PreservesPlanID(t *testing.T) { + plan := &interfaces.IaCPlan{ID: "plan-12345"} + fp := newFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if result.PlanID != "plan-12345" { + t.Errorf("PlanID: got %q want %q", result.PlanID, "plan-12345") + } +} + +// errResolveDriver is a sentinel for the ResourceDriver-error test path. +var errResolveDriver = errors.New("driver lookup failed") + +// TestApplyPlan_ResolveDriverErrorRecordsActionError covers the branch +// where p.ResourceDriver returns an error: the per-action error must be +// recorded with the canonical "resolve driver:" prefix and the loop must +// continue to the next action so a single bad type doesn't abort apply. +func TestApplyPlan_ResolveDriverErrorRecordsActionError(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("bad", "infra.unknown")}, + {Action: "create", Resource: spec("good", "infra.vpc")}, + }, + } + fp := newFakeProvider() + fp.driverErr = errResolveDriver + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatalf("top-level error not expected for per-action driver-resolve failure; got %v", err) + } + // fp.driverErr applies to BOTH actions in this test (driver lookup + // hits the same fake), so both record the resolve-driver error. The + // important contracts are: the loop continued past action[0] (so + // len(Errors)==2 not 1), and the prefix is canonical. + if len(result.Errors) != 2 { + t.Fatalf("expected 2 per-action errors (loop must continue past action[0]); got %d (%v)", + len(result.Errors), result.Errors) + } + for i, e := range result.Errors { + if !strings.HasPrefix(e.Error, "resolve driver:") { + t.Errorf("Errors[%d].Error: want prefix %q; got %q", i, "resolve driver:", e.Error) + } + if e.Action != "create" { + t.Errorf("Errors[%d].Action: got %q want %q", i, e.Action, "create") + } + } + if result.Errors[0].Resource != "bad" || result.Errors[1].Resource != "good" { + t.Errorf("expected Resource ordering [bad, good]; got [%s, %s]", + result.Errors[0].Resource, result.Errors[1].Resource) + } +} + +// TestApplyPlan_LoopContinuesAfterPerActionFailure verifies the +// best-effort contract: action[0]'s driver lookup fails, action[1]'s +// resource type uses a different driver path that succeeds. Both effects +// must be observable in the result. +func TestApplyPlan_LoopContinuesAfterPerActionFailure(t *testing.T) { + // Use a per-call fakeProvider variant whose ResourceDriver returns + // an error for one type and the real driver for the other. + fp := &selectiveFakeProvider{ + fakeProvider: newFakeProvider(), + errorType: "infra.unknown", + } + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("bad", "infra.unknown")}, + {Action: "create", Resource: spec("good", "infra.vpc")}, + }, + } + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 error from action[0]; got %d (%v)", len(result.Errors), result.Errors) + } + if fp.driver.createCount != 1 { + t.Errorf("action[1] should have reached the driver; createCount=%d want 1", fp.driver.createCount) + } + // Successful action's output should be in result.Resources. + if len(result.Resources) != 1 || result.Resources[0].Name != "good" { + t.Errorf("expected one Resources entry for action[1]; got %+v", result.Resources) + } +} + +// selectiveFakeProvider returns errResolveDriver for one resource type +// and the wrapped fakeProvider's driver for any other type. Used by +// TestApplyPlan_LoopContinuesAfterPerActionFailure to exercise mixed +// success/failure ordering across a multi-action plan. +type selectiveFakeProvider struct { + *fakeProvider + errorType string +} + +func (p *selectiveFakeProvider) ResourceDriver(typ string) (interfaces.ResourceDriver, error) { + if typ == p.errorType { + return nil, errResolveDriver + } + return p.driver, nil +} + +// TestApplyPlan_CtxCancellationStopsLoop verifies the loop respects +// context cancellation between actions. Drivers may honor ctx individually, +// but the loop itself must check at the iteration boundary so a +// long-running multi-action apply terminates promptly on Ctrl-C / deadline. +func TestApplyPlan_CtxCancellationStopsLoop(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before invocation so the very first iteration aborts. + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: spec("a", "infra.vpc")}, + {Action: "create", Resource: spec("b", "infra.vpc")}, + }, + } + fp := newFakeProvider() + _, err := ApplyPlan(ctx, fp, plan) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled top-level error; got %v", err) + } + if fp.driver.createCount != 0 { + t.Errorf("no driver calls should run after ctx cancellation; createCount=%d", fp.driver.createCount) + } +} diff --git a/iac/wfctlhelpers/apply_update_delete_test.go b/iac/wfctlhelpers/apply_update_delete_test.go new file mode 100644 index 000000000..44614653d --- /dev/null +++ b/iac/wfctlhelpers/apply_update_delete_test.go @@ -0,0 +1,259 @@ +package wfctlhelpers + +import ( + "context" + "errors" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// providerIDCapturingDriver records the ResourceRef passed to Update / +// Delete so tests can assert ProviderID propagation from action.Current +// to the driver call. +type providerIDCapturingDriver struct { + *fakeDriver + updateRef interfaces.ResourceRef + updateSpec interfaces.ResourceSpec + deleteRef interfaces.ResourceRef + deleteErr error + updateErr error +} + +func (d *providerIDCapturingDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.updateCount++ + d.updateRef = ref + d.updateSpec = spec + if d.updateErr != nil { + return nil, d.updateErr + } + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil +} + +func (d *providerIDCapturingDriver) Delete(_ context.Context, ref interfaces.ResourceRef) error { + d.fakeDriver.deleteCount++ + d.deleteRef = ref + return d.deleteErr +} + +// captureFakeProvider returns the providerIDCapturingDriver for any +// resource type so the caller can assert what ApplyPlan passed through. +type captureFakeProvider struct { + *fakeProvider + driver *providerIDCapturingDriver +} + +func newCaptureFakeProvider() *captureFakeProvider { + base := newFakeProvider() + return &captureFakeProvider{ + fakeProvider: base, + driver: &providerIDCapturingDriver{fakeDriver: base.driver}, + } +} + +func (p *captureFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} + +// TestApplyPlan_Update_PassesProviderID is the canonical T3.3 update +// test: the plan's update action carries action.Current with a known +// ProviderID; doUpdate must thread that ProviderID into the +// ResourceRef passed to driver.Update so the driver knows which +// resource to mutate. Threading it via Name alone would lose the +// upstream ID and force the driver to re-resolve. +func TestApplyPlan_Update_PassesProviderID(t *testing.T) { + const knownID = "do-vpc-abc123" + cur := &interfaces.ResourceState{ + Name: "vpc-1", + Type: "infra.vpc", + ProviderID: knownID, + } + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "update", Resource: spec("vpc-1", "infra.vpc"), Current: cur}, + }, + } + fp := newCaptureFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected errors: %v", result.Errors) + } + if got := fp.driver.updateRef.ProviderID; got != knownID { + t.Errorf("Update ResourceRef.ProviderID: got %q want %q", got, knownID) + } + if got := fp.driver.updateRef.Name; got != "vpc-1" { + t.Errorf("Update ResourceRef.Name: got %q want %q", got, "vpc-1") + } + if got := fp.driver.updateRef.Type; got != "infra.vpc" { + t.Errorf("Update ResourceRef.Type: got %q want %q", got, "infra.vpc") + } + // Driver returns a populated ResourceOutput; ApplyPlan must thread + // it through to result.Resources. + if len(result.Resources) != 1 || result.Resources[0].ProviderID != knownID { + t.Errorf("Resources: expected one entry with ProviderID %q; got %+v", knownID, result.Resources) + } +} + +// TestApplyPlan_Update_NilCurrentIsHandledDefensively verifies the +// edge case where a malformed plan's update action lacks +// action.Current. ComputePlan upstream should never emit such a plan, +// but doUpdate must not panic — it should call Update with an empty +// ProviderID (matching the skeleton's behavior) so the driver can +// surface its own typed validation error. +func TestApplyPlan_Update_NilCurrentIsHandledDefensively(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "update", Resource: spec("orphan", "infra.vpc"), Current: nil}, + }, + } + fp := newCaptureFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if got := fp.driver.updateRef.ProviderID; got != "" { + t.Errorf("nil Current must yield empty ProviderID; got %q", got) + } + // Per-action result behavior: driver returned non-nil success → + // no errors, output appended. The contract is that doUpdate + // doesn't synthesize a precondition error; the driver is the + // authority on what an empty ProviderID means. + if len(result.Errors) != 0 { + t.Errorf("unexpected per-action errors for nil Current: %v", result.Errors) + } + // Lock the resource-append contract on the success path so a + // regression that made doUpdate skip the append on nil Current + // would fail this test loudly. + if len(result.Resources) != 1 { + t.Errorf("expected 1 Resources entry on driver-success path; got %d", len(result.Resources)) + } +} + +// TestApplyPlan_Delete_NilCurrentIsHandledDefensively mirrors the +// Update nil-Current contract for doDelete: a delete action without +// action.Current must not panic; the empty ProviderID flows to the +// driver, which is the authority on what to do (most drivers will +// surface a typed validation error). doDelete itself does not +// synthesize a precondition error — same defensive shape as doUpdate. +func TestApplyPlan_Delete_NilCurrentIsHandledDefensively(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "delete", Resource: spec("orphan", "infra.vpc"), Current: nil}, + }, + } + fp := newCaptureFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if got := fp.driver.deleteRef.ProviderID; got != "" { + t.Errorf("nil Current must yield empty ProviderID on Delete; got %q", got) + } + if got := fp.driver.deleteRef.Name; got != "orphan" { + t.Errorf("Delete ResourceRef.Name: got %q want %q", got, "orphan") + } + if len(result.Errors) != 0 { + t.Errorf("unexpected per-action errors for nil Current Delete: %v", result.Errors) + } + // Sanity: driver was called exactly once (latent bug-fix contract + // from T3.3 — Delete must not be silently skipped). + if fp.driver.deleteCount != 1 { + t.Errorf("driver.Delete must be called exactly once; got %d", fp.driver.deleteCount) + } +} + +// TestApplyPlan_Delete_InvokesDriverDelete is the latent-bug-fix test +// from the design notes: today's DOProvider.Apply has no +// `case "delete":` arm, so wfctl's state-prune action silently skips +// cloud-resource deletion. doDelete CLOSES that gap by always calling +// driver.Delete. This test fails on the design's pre-T3.3 codepath. +func TestApplyPlan_Delete_InvokesDriverDelete(t *testing.T) { + const knownID = "do-vpc-xyz789" + cur := &interfaces.ResourceState{ + Name: "old-vpc", + Type: "infra.vpc", + ProviderID: knownID, + } + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "delete", Resource: spec("old-vpc", "infra.vpc"), Current: cur}, + }, + } + fp := newCaptureFakeProvider() + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected errors: %v", result.Errors) + } + if fp.driver.deleteCount != 1 { + t.Errorf("driver.Delete must be called exactly once for delete action; got %d", fp.driver.deleteCount) + } + if got := fp.driver.deleteRef.ProviderID; got != knownID { + t.Errorf("Delete ResourceRef.ProviderID: got %q want %q", got, knownID) + } +} + +// TestApplyPlan_Delete_DriverErrorRecorded verifies that a driver-level +// delete failure is recorded in result.Errors with the action + +// resource captured, matching the per-action error contract used by +// the rest of the dispatch loop. +func TestApplyPlan_Delete_DriverErrorRecorded(t *testing.T) { + deleteErr := errors.New("delete failed: 503 service unavailable") + cur := &interfaces.ResourceState{Name: "old-vpc", Type: "infra.vpc", ProviderID: "do-id"} + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "delete", Resource: spec("old-vpc", "infra.vpc"), Current: cur}, + }, + } + fp := newCaptureFakeProvider() + fp.driver.deleteErr = deleteErr + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if result.Errors[0].Action != "delete" { + t.Errorf("Action: got %q want %q", result.Errors[0].Action, "delete") + } + if result.Errors[0].Resource != "old-vpc" { + t.Errorf("Resource: got %q want %q", result.Errors[0].Resource, "old-vpc") + } + if got := result.Errors[0].Error; got == "" || got != deleteErr.Error() { + t.Errorf("Error: got %q want bare driver error %q", got, deleteErr.Error()) + } +} + +// TestApplyPlan_Update_DriverErrorRecorded mirrors the Delete error +// recording test — driver Update failures must surface in +// result.Errors with the canonical fields populated. +func TestApplyPlan_Update_DriverErrorRecorded(t *testing.T) { + updateErr := errors.New("update failed: 422 invalid spec") + cur := &interfaces.ResourceState{Name: "vpc-1", Type: "infra.vpc", ProviderID: "do-id"} + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{ + {Action: "update", Resource: spec("vpc-1", "infra.vpc"), Current: cur}, + }, + } + fp := newCaptureFakeProvider() + fp.driver.updateErr = updateErr + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error; got %d (%v)", len(result.Errors), result.Errors) + } + if result.Errors[0].Action != "update" { + t.Errorf("Action: got %q want %q", result.Errors[0].Action, "update") + } + if got := result.Errors[0].Error; got != updateErr.Error() { + t.Errorf("Error: got %q want bare driver error %q", got, updateErr.Error()) + } +} diff --git a/interfaces/iac_resource_driver.go b/interfaces/iac_resource_driver.go index 0efd2a369..058ceab0f 100644 --- a/interfaces/iac_resource_driver.go +++ b/interfaces/iac_resource_driver.go @@ -40,6 +40,26 @@ type ResourceAdoptionLocator interface { AdoptionRef(spec ResourceSpec) (ResourceRef, bool, error) } +// UpsertSupporter is an optional interface implemented by ResourceDrivers +// that support locating an existing resource by name alone (empty +// ProviderID) in their Read method. wfctlhelpers.ApplyPlan uses this hook +// to recover from Create that returns ErrResourceAlreadyExists: when the +// driver opts in via SupportsUpsert()==true, ApplyPlan calls Read with a +// name-only ResourceRef to obtain the existing ProviderID, then calls +// Update to bring the resource to the desired state — net effect of an +// idempotent upsert without requiring drivers to implement upsert +// natively. +// +// Drivers that do not implement this interface (or return false) yield +// the original ErrResourceAlreadyExists unchanged — ApplyPlan does NOT +// silently swallow the conflict. Implementations should return true only +// when their Read can locate a resource by Name + Type without a +// ProviderID; returning true while requiring a non-empty ProviderID in +// Read defeats the recovery path. +type UpsertSupporter interface { + SupportsUpsert() bool +} + // ResourceOutput is the concrete output of a provisioned or read resource. type ResourceOutput struct { Name string `json:"name"` diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 668b834b1..c0dd3caf5 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -107,6 +107,37 @@ type ApplyResult struct { PlanID string `json:"plan_id"` Resources []ResourceOutput `json:"resources"` Errors []ActionError `json:"errors,omitempty"` + + // InitialInputSnapshot captures env-var fingerprints at the start of apply. + // Populated by wfctlhelpers.ApplyPlan (T3.1) at apply entry by snapshotting + // every name listed in plan.InputSnapshot through inputsnapshot.OSEnvProvider. + // Used by the deferred postcondition (T3.1.5) to compute the drift report + // against the apply-time snapshot. + InitialInputSnapshot map[string]string `json:"initial_input_snapshot,omitempty"` + + // InputDriftReport names env-vars whose fingerprint changed between plan + // and apply. Populated by the deferred postcondition in + // wfctlhelpers.ApplyPlan (T3.1.5) regardless of whether the apply + // itself succeeded or errored. Empty (or nil) means no drift detected. + // Entries are sorted by Name for deterministic comparison and stable + // diagnostic output; the sort guarantee is enforced by + // inputsnapshot.ComputeDrift (covered by + // TestComputeDrift_ResultIsSortedByName in + // iac/inputsnapshot/compute_drift_test.go). + InputDriftReport []DriftEntry `json:"input_drift_report,omitempty"` + + // ReplaceIDMap propagates new ProviderIDs from Replace actions to + // dependent resources whose Apply runs later in the same plan. + // Populated by doReplace (T3.4); consumed by JIT substitution in W-5 + // (T5.2/T5.3). + // + // Keyed by the *replaced* resource's Name (i.e., action.Resource.Name + // from the Replace PlanAction — the resource whose Delete-then-Create + // just produced a fresh ProviderID). The value is the new ProviderID + // returned from the post-Delete Create. Dependent resources reference + // the replaced resource by name in their config, so JIT substitution + // in W-5 translates "name → new ProviderID" via this map. + ReplaceIDMap map[string]string `json:"replace_id_map,omitempty"` } // DestroyResult summarises the outcome of a destroy operation. diff --git a/interfaces/iac_state_test.go b/interfaces/iac_state_test.go index 494bde584..90f4e0349 100644 --- a/interfaces/iac_state_test.go +++ b/interfaces/iac_state_test.go @@ -53,3 +53,103 @@ func TestPlanAction_ResolvedConfigHashField(t *testing.T) { t.Errorf("ResolvedConfigHash: got %q want %q", got.ResolvedConfigHash, realisticHash) } } + +// TestApplyResult_InputDriftReport_RoundTrip verifies the InputDriftReport +// field declared in T3.0.4. Field is populated by the deferred postcondition +// in wfctlhelpers.ApplyPlan (T3.1.5). +func TestApplyResult_InputDriftReport_RoundTrip(t *testing.T) { + r := ApplyResult{InputDriftReport: []DriftEntry{ + {Name: "STAGING_PG_PASSWORD", PlanFingerprint: "abc", ApplyFingerprint: "def"}, + }} + data, err := json.Marshal(r) + if err != nil { + t.Fatal(err) + } + var got ApplyResult + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if len(got.InputDriftReport) != 1 || got.InputDriftReport[0].Name != "STAGING_PG_PASSWORD" { + t.Errorf("InputDriftReport roundtrip failed: %+v", got) + } +} + +// TestApplyResult_InitialInputSnapshot_RoundTrip verifies the +// InitialInputSnapshot field declared in T3.0.4. Field is populated at apply +// entry by wfctlhelpers.ApplyPlan (T3.1) and consumed by the deferred +// postcondition (T3.1.5) when computing drift. +func TestApplyResult_InitialInputSnapshot_RoundTrip(t *testing.T) { + r := ApplyResult{InitialInputSnapshot: map[string]string{"FOO": "fp1234"}} + data, err := json.Marshal(r) + if err != nil { + t.Fatal(err) + } + var got ApplyResult + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.InitialInputSnapshot["FOO"] != "fp1234" { + t.Errorf("InitialInputSnapshot roundtrip failed: %+v", got) + } +} + +// TestApplyResult_ReplaceIDMap_RoundTrip verifies the ReplaceIDMap field +// declared in T3.0.4. Field is populated by doReplace (T3.4) and consumed +// by JIT substitution in W-5 (T5.2/T5.3). +func TestApplyResult_ReplaceIDMap_RoundTrip(t *testing.T) { + r := ApplyResult{ReplaceIDMap: map[string]string{"vpc": "new-uuid"}} + data, err := json.Marshal(r) + if err != nil { + t.Fatal(err) + } + var got ApplyResult + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.ReplaceIDMap["vpc"] != "new-uuid" { + t.Errorf("ReplaceIDMap roundtrip failed: %+v", got) + } +} + +// TestApplyResult_OmitEmptyContract locks in the omitempty JSON tag +// behavior on the three T3.0.4 fields. Both nil and empty-but-non-nil +// values must drop from the encoded form so plan/result transcripts stay +// lean and downstream consumers can treat "absent key" and "empty value" +// identically — matching the behavior already documented for +// IaCPlan.InputSnapshot and PlanAction.ResolvedConfigHash. +func TestApplyResult_OmitEmptyContract(t *testing.T) { + cases := map[string]ApplyResult{ + "nil-fields": {}, + "empty-non-nil-fields": { + InitialInputSnapshot: map[string]string{}, + InputDriftReport: []DriftEntry{}, + ReplaceIDMap: map[string]string{}, + }, + } + for name, r := range cases { + t.Run(name, func(t *testing.T) { + data, err := json.Marshal(r) + if err != nil { + t.Fatal(err) + } + s := string(data) + for _, key := range []string{"initial_input_snapshot", "input_drift_report", "replace_id_map"} { + if containsString(s, key) { + t.Errorf("expected %q to be omitted from %s; got %s", key, name, s) + } + } + }) + } +} + +// containsString is a tiny, dependency-free substring helper local to this +// test file so the omitempty test does not pull in strings just for one +// check (the file's other tests use only encoding/json + testing). +func containsString(s, substr string) bool { + for i := 0; i+len(substr) <= len(s); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/plugin/sdk/manifest.go b/plugin/sdk/manifest.go new file mode 100644 index 000000000..d9f22bfda --- /dev/null +++ b/plugin/sdk/manifest.go @@ -0,0 +1,136 @@ +// Package sdk hosts the plugin SDK manifest schema and helpers used by +// wfctl to discover plugin capabilities (currently: IaC dispatch version). +// +// The SDK manifest is intentionally additive over [plugin.PluginManifest]; +// it captures only the fields that wfctl reads at apply-time to choose +// between the v1 (legacy in-provider Apply) and v2 (wfctlhelpers.ApplyPlan) +// dispatch paths. +package sdk + +import ( + "bytes" + _ "embed" + "encoding/json" + "fmt" + "sync" + + "github.com/santhosh-tekuri/jsonschema/v6" +) + +// manifestSchemaJSON is the JSON Schema validating the SDK manifest. It is +// embedded so wfctl always validates against the schema version compiled +// into the binary, not whatever happens to be on disk. +// +//go:embed manifest_schema.json +var manifestSchemaJSON []byte + +// ManifestSchemaJSON returns the raw JSON Schema bytes used to validate +// SDK manifests. Exported for plugin authors and external tooling that +// want to validate plugin.json without depending on this package's +// ParseManifest entry point. +// +// Returns a copy so callers cannot mutate the embedded schema; the +// underlying slice from //go:embed is technically writable. +func ManifestSchemaJSON() []byte { + return bytes.Clone(manifestSchemaJSON) +} + +// Manifest captures the SDK-level fields wfctl reads from plugin.json. +// It is a strict subset of the full plugin.PluginManifest — only fields +// that gate apply-time dispatch live here. +type Manifest struct { + // Name is the plugin name. Carried for diagnostics; the SDK schema + // does not enforce shape (lowercase/hyphen rules live in plugin.PluginManifest). + Name string `json:"name"` + + // IaCProvider holds IaC-provider-specific manifest fields. Empty + // (zero-valued) when the plugin does not implement IaCProvider. + IaCProvider IaCProvider `json:"iacProvider"` +} + +// IaCProvider describes IaC-provider-specific manifest fields. +type IaCProvider struct { + // ComputePlanVersion selects the apply-time dispatch path: + // "" (default, treated as "v1"): legacy in-provider Apply switch. + // "v1": explicit legacy dispatch. + // "v2": route through wfctlhelpers.ApplyPlan + // (Replace + input-drift postcondition). + // Schema-validated against the enum ["v1","v2"]; "" passes validation + // because the field is optional. + ComputePlanVersion string `json:"computePlanVersion,omitempty"` +} + +// EffectiveComputePlanVersion returns the dispatch version, defaulting to +// "v1" when the manifest omits the field. Callers should always go through +// this accessor rather than reading ComputePlanVersion directly so the +// default-v1 contract stays in one place. +func (p IaCProvider) EffectiveComputePlanVersion() string { + if p.ComputePlanVersion == "" { + return "v1" + } + return p.ComputePlanVersion +} + +// compiledSchema is the parsed manifest schema. It is compiled lazily on +// first ParseManifest call and cached for the process lifetime; the schema +// is embedded and immutable, so a single compilation is correct. +// +// The compilation is guarded by sync.Once so concurrent callers cannot race +// on the cache pointer or on the jsonschema compiler's internal state. Both +// the success result and the error are captured so subsequent calls return +// the same outcome without re-compiling. +var ( + compiledSchema *jsonschema.Schema + compiledSchemaErr error + compiledSchemaOnce sync.Once +) + +// loadSchema compiles manifestSchemaJSON exactly once per process. Returns +// the compiled schema or an error wrapping the underlying compile failure +// (so failures surface with a clear "schema bug" diagnostic rather than as +// a generic "ParseManifest failed"). +func loadSchema() (*jsonschema.Schema, error) { + compiledSchemaOnce.Do(func() { + doc, err := jsonschema.UnmarshalJSON(bytes.NewReader(manifestSchemaJSON)) + if err != nil { + compiledSchemaErr = fmt.Errorf("sdk manifest schema: unmarshal: %w", err) + return + } + c := jsonschema.NewCompiler() + if err := c.AddResource("manifest.json", doc); err != nil { + compiledSchemaErr = fmt.Errorf("sdk manifest schema: add resource: %w", err) + return + } + s, err := c.Compile("manifest.json") + if err != nil { + compiledSchemaErr = fmt.Errorf("sdk manifest schema: compile: %w", err) + return + } + compiledSchema = s + }) + return compiledSchema, compiledSchemaErr +} + +// ParseManifest validates raw plugin.json bytes against the SDK schema and +// decodes them into a Manifest. Returns an error if the JSON is malformed +// or violates the schema (e.g., iacProvider.computePlanVersion not in +// {"v1","v2"}). Pure-additive: existing plugin.json files without an +// iacProvider key parse cleanly with a zero-valued IaCProvider. +func ParseManifest(data []byte) (*Manifest, error) { + s, err := loadSchema() + if err != nil { + return nil, err + } + doc, err := jsonschema.UnmarshalJSON(bytes.NewReader(data)) + if err != nil { + return nil, fmt.Errorf("manifest: invalid JSON: %w", err) + } + if err := s.Validate(doc); err != nil { + return nil, fmt.Errorf("manifest: schema validation failed: %w", err) + } + var m Manifest + if err := json.Unmarshal(data, &m); err != nil { + return nil, fmt.Errorf("manifest: decode: %w", err) + } + return &m, nil +} diff --git a/plugin/sdk/manifest_schema.json b/plugin/sdk/manifest_schema.json new file mode 100644 index 000000000..aec5a9767 --- /dev/null +++ b/plugin/sdk/manifest_schema.json @@ -0,0 +1,24 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/GoCodeAlone/workflow/plugin/sdk/manifest_schema.json", + "title": "Plugin SDK manifest", + "description": "Schema for the SDK-level plugin manifest (plugin.json) consumed by wfctl. Pure-additive; existing manifests without iacProvider remain valid. The root object permits additional properties so existing plugin.json files (with name/version/author/etc.) parse cleanly. The iacProvider sub-object is strict so typos like 'computeplanversion' or unknown keys are rejected at parse time, not silently downgraded to v1 dispatch.", + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "iacProvider": { + "type": "object", + "description": "IaC-provider-specific manifest fields. Only present for plugins that implement the IaCProvider interface.", + "additionalProperties": false, + "properties": { + "computePlanVersion": { + "type": "string", + "description": "Selects the apply-time dispatch path. v1 (default when omitted) routes through the legacy in-provider Apply switch. v2 routes through wfctlhelpers.ApplyPlan with full Replace + drift-postcondition support.", + "enum": ["v1", "v2"] + } + } + } + } +} diff --git a/plugin/sdk/manifest_test.go b/plugin/sdk/manifest_test.go new file mode 100644 index 000000000..4112e7bd9 --- /dev/null +++ b/plugin/sdk/manifest_test.go @@ -0,0 +1,137 @@ +package sdk + +import ( + "strings" + "sync" + "testing" +) + +// TestManifest_IaCProvider_ComputePlanVersion exercises the new +// iacProvider.computePlanVersion field. Cases: +// - default-v1: field omitted → EffectiveComputePlanVersion() == "v1" +// - explicit-v1: "v1" → "v1" +// - explicit-v2: "v2" → "v2" +// - rejected: "v3" → ParseManifest returns an error (schema-rejected) +func TestManifest_IaCProvider_ComputePlanVersion(t *testing.T) { + cases := map[string]struct { + in string + want string + wantErr bool + }{ + "default-v1": {`{"name":"x","iacProvider":{}}`, "v1", false}, + "explicit-v1": {`{"name":"x","iacProvider":{"computePlanVersion":"v1"}}`, "v1", false}, + "explicit-v2": {`{"name":"x","iacProvider":{"computePlanVersion":"v2"}}`, "v2", false}, + "rejected": {`{"name":"x","iacProvider":{"computePlanVersion":"v3"}}`, "", true}, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + m, err := ParseManifest([]byte(c.in)) + if (err != nil) != c.wantErr { + t.Fatalf("err=%v wantErr=%v", err, c.wantErr) + } + if !c.wantErr && m.IaCProvider.EffectiveComputePlanVersion() != c.want { + t.Errorf("got %q want %q", m.IaCProvider.EffectiveComputePlanVersion(), c.want) + } + }) + } +} + +// TestManifest_IaCProvider_ComputePlanVersion_ZeroValue verifies that an +// IaCProvider with the zero value (empty string) reports v1, matching the +// "default-v1" case but exercising the accessor on a Go-zero-valued struct +// (no JSON involved). +func TestManifest_IaCProvider_ComputePlanVersion_ZeroValue(t *testing.T) { + var p IaCProvider + if got := p.EffectiveComputePlanVersion(); got != "v1" { + t.Errorf("zero IaCProvider.EffectiveComputePlanVersion() = %q, want %q", got, "v1") + } +} + +// TestManifest_IaCProvider_RejectsTypoKey verifies that a typo inside +// iacProvider (e.g., the lowercase "computeplanversion") is rejected by +// the schema rather than silently parsing to a zero-valued IaCProvider — +// which would produce a silent v1 dispatch downgrade. The root object +// stays permissive so plugin.json files with version/author/etc. still +// parse, but iacProvider is strict by design. +func TestManifest_IaCProvider_RejectsTypoKey(t *testing.T) { + cases := []string{ + `{"name":"x","iacProvider":{"computeplanversion":"v2"}}`, // lowercase typo + `{"name":"x","iacProvider":{"foo":"bar"}}`, // unknown key + } + for _, in := range cases { + t.Run(in, func(t *testing.T) { + _, err := ParseManifest([]byte(in)) + if err == nil { + t.Errorf("expected schema rejection for %q; got nil", in) + } + }) + } +} + +// TestManifest_RootPermitsAdditionalProperties verifies that the root +// object accepts unknown top-level keys so existing plugin.json files +// (which carry version/author/dependencies/etc.) parse cleanly through +// the SDK manifest. Pure-additive contract. +func TestManifest_RootPermitsAdditionalProperties(t *testing.T) { + in := `{"name":"x","version":"1.2.3","author":"jane","description":"hi","iacProvider":{"computePlanVersion":"v2"}}` + m, err := ParseManifest([]byte(in)) + if err != nil { + t.Fatalf("expected pass; got %v", err) + } + if m.IaCProvider.EffectiveComputePlanVersion() != "v2" { + t.Errorf("got %q want %q", m.IaCProvider.EffectiveComputePlanVersion(), "v2") + } +} + +// TestManifestSchemaJSON_ReturnsCopy verifies that mutating the slice +// returned from ManifestSchemaJSON cannot affect the embedded schema +// observed by subsequent callers. +func TestManifestSchemaJSON_ReturnsCopy(t *testing.T) { + a := ManifestSchemaJSON() + if len(a) == 0 { + t.Fatal("ManifestSchemaJSON() returned empty bytes") + } + a[0] = 0xFF // attempt to corrupt + b := ManifestSchemaJSON() + if b[0] == 0xFF { + t.Error("ManifestSchemaJSON returned a shared slice; embedded schema is mutable from callers") + } + if !strings.Contains(string(b), "computePlanVersion") { + t.Error("ManifestSchemaJSON copy lost the schema body") + } +} + +// TestParseManifest_ConcurrentRaceFree exercises the race detector against +// the lazy schema cache: 32 goroutines call ParseManifest simultaneously +// before any sequential call has populated the cache. Run under -race; a +// failure here means the sync.Once guard around compiledSchema regressed. +// +// Note: this test relies on the package-init ordering — Go's testing +// framework gives each test a fresh goroutine but the package globals +// persist across tests in the same binary, so by the time this test runs +// other tests may already have warmed the cache. The test is still useful +// because it stresses concurrent reads of the cached pointer and any +// internal state inside the jsonschema.Schema.Validate path. +func TestParseManifest_ConcurrentRaceFree(t *testing.T) { + const goroutines = 32 + const inputs = 4 + manifests := []string{ + `{"name":"a","iacProvider":{}}`, + `{"name":"b","iacProvider":{"computePlanVersion":"v1"}}`, + `{"name":"c","iacProvider":{"computePlanVersion":"v2"}}`, + `{"name":"d","iacProvider":{"computePlanVersion":"v3"}}`, // expected to error + } + var wg sync.WaitGroup + start := make(chan struct{}) + wg.Add(goroutines) + for i := range goroutines { + go func(idx int) { + defer wg.Done() + <-start // align goroutines for maximum concurrent pressure on loadSchema + in := manifests[idx%inputs] + _, _ = ParseManifest([]byte(in)) + }(i) + } + close(start) + wg.Wait() +}