diff --git a/pkg/clouds/pulumi/destructive_replace.go b/pkg/clouds/pulumi/destructive_replace.go index 5004313a..22f1691c 100644 --- a/pkg/clouds/pulumi/destructive_replace.go +++ b/pkg/clouds/pulumi/destructive_replace.go @@ -8,6 +8,8 @@ import ( "os" "sort" "strings" + + "github.com/pulumi/pulumi/sdk/v3/go/common/apitype" ) // The set of safe-to-replace types lives in `pkg/api` (see @@ -21,6 +23,80 @@ import ( // are additive replaces. See // forge/docs/roadmap/items/sc-safe-to-replace-via-plugin-schema.md. +// remoteCommandType is pulumi-command's SSH command resource — the +// thing every vm / k3s provision is built out of. +const remoteCommandType = "command:remote:Command" + +// commandDeleteInput is the pulumi-command input holding the script +// run when the resource goes away. Its presence is the ONLY thing +// that makes replacing a remote command destructive. +const commandDeleteInput = "delete" + +// isStatelessRemoteCommand reports whether this replace is of a +// remote command that will destroy nothing when it is replaced. +// +// Why this is not a `safeToReplaceTypes` entry. That allowlist is +// per-TYPE, and `command:remote:Command` is not uniformly safe: a +// command carrying a `delete` script runs that script against a live +// machine when it is replaced (pulumi-command replaces as +// create-then-delete), which is a real teardown and exactly what the +// guard is for. SC's k3s uninstall +// (pkg/clouds/pulumi/k3s/cluster.go's uninstallOnDestroy) and the vm +// ingress teardown (pkg/clouds/pulumi/vm/caddy.go) are both that +// shape. So the question is per-RESOURCE, not per-type, and the +// answer is in the inputs. +// +// Why the exemption is needed at all. SC nonces its commands: every +// install / readiness / read command gets the run timestamp in its +// `triggers` via pulumiVm.TriggersForGuardedChange or +// TriggersForRead, deliberately, so a host whose k3s was stopped out +// of band is repaired rather than reported as "no changes" (the +// scripts are idempotent and carry their own args-marker guard — +// k3s's installer answers an unchanged config with "already running +// as k3s with the requested configuration"). A replace on every run +// is therefore the NORMAL shape of a vm or k3s provision, not a rare +// event to wave through. Live-caught 2026-11-08 on a 3-node k3s +// stand: the second `sc provision` refused outright, naming nine +// resources — six k3s commands and three per-host readiness gates — +// every one of them a nonce-only trigger diff with `keys=[]`, so the +// guard's own explanation ("a changed REPLACE-forcing input") did not +// even apply. Nothing in the fleet could re-provision a vm stack. +// +// The failure direction when inputs cannot be read. Engine step +// events populate Inputs, but if that ever stops being true (or a +// delete script arrives as a secret placeholder rather than a +// string) a remote command becomes exempt rather than guarded. That +// is the chosen direction: a remote command holds no cloud object at +// the other end of its id — the id is a random suffix on the +// resource name — so the guard was never protecting state here, only +// the delete script. A non-string value is treated as present +// precisely because it is not provably empty. +func isStatelessRemoteCommand(meta apitype.StepEventMetadata) bool { + if meta.Type != remoteCommandType { + return false + } + // Both sides matter: the old state is what gets deleted, and the new + // state is what a subsequent destroy would run. + return !hasCommandDelete(meta.Old) && !hasCommandDelete(meta.New) +} + +// hasCommandDelete reports whether a step's resource state declares a +// non-empty `delete` script. +func hasCommandDelete(state *apitype.StepEventStateMetadata) bool { + if state == nil { + return false + } + value, ok := state.Inputs[commandDeleteInput] + if !ok { + return false + } + script, ok := value.(string) + if !ok { + return true + } + return strings.TrimSpace(script) != "" +} + // envAllowDestructiveReplace is the operator-level escape hatch for // S4 of `cloudflare-plugin-wrong-zone`. Setting it to a truthy value // (`1` / `true` / `yes`, case-insensitive) in the deploy environment @@ -74,6 +150,18 @@ func (e *destructiveReplaceError) Error() string { "Review each resource below; if the replace is intentional, set %s=1 and re-run.\n", len(replaces), envAllowDestructiveReplace) for _, r := range replaces { + // An empty key list used to render as `keys=[]`, which reads as + // "no inputs changed" directly under a sentence blaming a changed + // input — and that combination cost an hour on 2026-11-08. The + // engine leaves Keys (and DetailedDiff) empty for a provider that + // forces replacement without naming a property, pulumi-command's + // `triggers` among them, so say that instead of printing a hole. + // The message PREFIX above is the stable part for CI grepping; this + // line is diagnostics. + if len(r.Keys) == 0 { + fmt.Fprintf(&b, " - %s (%s): the provider named no replace-forcing key — check the resource's `triggers`\n", r.URN, r.Type) + continue + } fmt.Fprintf(&b, " - %s (%s): keys=[%s]\n", r.URN, r.Type, strings.Join(r.Keys, ", ")) } return strings.TrimRight(b.String(), "\n") diff --git a/pkg/clouds/pulumi/destructive_replace_test.go b/pkg/clouds/pulumi/destructive_replace_test.go index e12ee643..46339268 100644 --- a/pkg/clouds/pulumi/destructive_replace_test.go +++ b/pkg/clouds/pulumi/destructive_replace_test.go @@ -105,6 +105,49 @@ func TestIsDestructiveReplace_OpCoverage(t *testing.T) { }, want: false, }, + { + // 2026-11-08, a 3-node k3s stand: the SECOND `sc provision` + // refused, naming all nine of the stack's remote commands. SC + // nonces them on purpose (TriggersForGuardedChange / + // TriggersForRead) so a host that drifted out of band is + // repaired, so this shape is every vm / k3s provision, and the + // engine reports it with Keys empty. + name: "nonce-triggered remote command with no delete → safe (replacing it runs nothing)", + meta: apitype.StepEventMetadata{ + Op: apitype.OpReplace, + URN: "urn:pulumi:fleet::sck3s::command:remote:Command::smoke-cluster-install--staging", + Type: "command:remote:Command", + Old: &apitype.StepEventStateMetadata{ + ID: "smoke-cluster-install--staging1a697627", + Inputs: map[string]any{"create": "curl -sfL https://get.k3s.io | sh -", "triggers": []any{"2026-11-08T13:14:27Z"}}, + }, + New: &apitype.StepEventStateMetadata{ + Inputs: map[string]any{"create": "curl -sfL https://get.k3s.io | sh -", "triggers": []any{"2026-11-08T13:43:39Z"}}, + }, + }, + want: false, + }, + { + // The k3s uninstall and the vm ingress teardown are this shape. + // Both are registered WITHOUT triggers precisely so nothing but + // a destroy runs them — so a replace here means a real input + // moved (a connection address, the script itself), and + // pulumi-command would run that delete against a live machine. + name: "remote command carrying a delete script → destructive (the delete runs on replace)", + meta: apitype.StepEventMetadata{ + Op: apitype.OpReplace, + URN: "urn:pulumi:fleet::sck3s::command:remote:Command::smoke-vm-host-0-k3s-uninstall--staging", + Type: "command:remote:Command", + Old: &apitype.StepEventStateMetadata{ + ID: "smoke-vm-host-0-k3s-uninstall--staging0c1d2e3f", + Inputs: map[string]any{"create": "true", "delete": "/usr/local/bin/k3s-uninstall.sh"}, + }, + New: &apitype.StepEventStateMetadata{ + Inputs: map[string]any{"create": "true", "delete": "/usr/local/bin/k3s-uninstall.sh"}, + }, + }, + want: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -269,3 +312,101 @@ func TestRecordDestructiveReplace_AppendsOnMatch(t *testing.T) { t.Fatalf("expected tracker to be cleared after drain, got %d", len(got2)) } } + +// isStatelessRemoteCommand asks a per-RESOURCE question (does this +// command have a delete script?) rather than the per-TYPE one the +// safe-to-replace allowlist can express, so its edge cases are worth +// pinning directly: the whole fix rests on reading an input. +func TestIsStatelessRemoteCommand(t *testing.T) { + tests := []struct { + name string + meta apitype.StepEventMetadata + want bool + }{ + { + name: "not a remote command → never exempt, whatever its inputs say", + meta: apitype.StepEventMetadata{ + Type: "cloudflare:index/record:Record", + Old: &apitype.StepEventStateMetadata{Inputs: map[string]any{}}, + }, + want: false, + }, + { + name: "no delete input at all → exempt", + meta: apitype.StepEventMetadata{ + Type: remoteCommandType, + Old: &apitype.StepEventStateMetadata{Inputs: map[string]any{"create": "true"}}, + New: &apitype.StepEventStateMetadata{Inputs: map[string]any{"create": "true"}}, + }, + want: true, + }, + { + name: "delete present but empty → exempt (pulumi-command skips the step either way)", + meta: apitype.StepEventMetadata{ + Type: remoteCommandType, + Old: &apitype.StepEventStateMetadata{Inputs: map[string]any{"delete": " "}}, + New: &apitype.StepEventStateMetadata{Inputs: map[string]any{"delete": ""}}, + }, + want: true, + }, + { + // A delete that arrives only on the NEW side still means a + // subsequent destroy would run a script, and the replace is the + // moment to look at it. + name: "delete on the new side only → guarded", + meta: apitype.StepEventMetadata{ + Type: remoteCommandType, + Old: &apitype.StepEventStateMetadata{Inputs: map[string]any{"create": "true"}}, + New: &apitype.StepEventStateMetadata{Inputs: map[string]any{"delete": "rm -rf /srv/app"}}, + }, + want: false, + }, + { + // Not provably empty, so not waved through — a secret + // placeholder or an unresolved output lands here. + name: "non-string delete value → treated as present, guarded", + meta: apitype.StepEventMetadata{ + Type: remoteCommandType, + Old: &apitype.StepEventStateMetadata{Inputs: map[string]any{"delete": map[string]any{"4dabf18193072939515e22adb298388d": "secret"}}}, + }, + want: false, + }, + { + // The documented failure direction: unreadable inputs exempt a + // remote command rather than wedging every provision, because + // its id names no cloud object. + name: "nil states → exempt", + meta: apitype.StepEventMetadata{Type: remoteCommandType}, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isStatelessRemoteCommand(tt.meta); got != tt.want { + t.Fatalf("isStatelessRemoteCommand() = %v, want %v", got, tt.want) + } + }) + } +} + +// The engine leaves Keys empty when a provider forces replacement +// without naming a property. `keys=[]` under a sentence blaming a +// changed input reads as a contradiction; the message must point at +// `triggers` instead. +func TestDestructiveReplaceError_NamesTriggersWhenNoKeys(t *testing.T) { + err := &destructiveReplaceError{Replaces: []destructiveReplace{{ + URN: "urn:pulumi:fleet::sck3s::command:remote:Command::smoke-cluster-install--staging", + Type: remoteCommandType, + }}} + msg := err.Error() + if strings.Contains(msg, "keys=[]") { + t.Fatalf("message still renders an empty key list:\n%s", msg) + } + if !strings.Contains(msg, "`triggers`") { + t.Fatalf("message does not point at triggers:\n%s", msg) + } + // The grep-stable prefix must survive. + if !strings.HasPrefix(msg, "input-driven destructive replace:") { + t.Fatalf("message prefix changed:\n%s", msg) + } +} diff --git a/pkg/clouds/pulumi/events.go b/pkg/clouds/pulumi/events.go index df15573c..573b716b 100644 --- a/pkg/clouds/pulumi/events.go +++ b/pkg/clouds/pulumi/events.go @@ -148,6 +148,13 @@ func isDestructiveReplace(meta apitype.StepEventMetadata) bool { if api.IsSafeToReplace(meta.Type) { return false } + // A remote command that carries no delete script destroys nothing when + // it is replaced, and SC replaces those on EVERY provision by design. + // See isStatelessRemoteCommand for why the type cannot simply go on the + // safe-to-replace allowlist. + if isStatelessRemoteCommand(meta) { + return false + } // A resource SC itself marked for retention is still physically // present post-delete, but the state entry still gets rewritten and // a replace still re-points the state at a NEW object — which is