diff --git a/docs/manual-test-plan.md b/docs/manual-test-plan.md index 524fa60..d5a3017 100644 --- a/docs/manual-test-plan.md +++ b/docs/manual-test-plan.md @@ -1166,7 +1166,7 @@ go vet ./... ### Known limitations / follow-ups - **Talos-mutated-field allowlist**: Phase 2B reports cert hashes / timestamps as divergence today; the verify is off by default until an allowlist lands. -- **`talm upgrade` has no pre-upgrade gates** (Phase 2C runs *after*, not before): the upgrade flow wraps `talosctl upgrade` and doesn't route through `buildApplyClosure` / `applyOneFileDirectPatchMode`, so Phase 1 / Phase 2A do not run. Phase 2C (post-upgrade version verify) was added precisely to catch the silent-rollback class without that refactor. Full pre-upgrade gates would require reproducing the gate calls in `upgrade_handler.go` or refactoring the apply flow. +- **`talm upgrade` does not run the apply-path gates**: the upgrade flow wraps `talosctl upgrade` and doesn't route through `buildApplyClosure` / `applyOneFileDirectPatchMode`, so Phase 1 / Phase 2A do not run. Two gates of its own cover the classes that matter most: the pre-upgrade path check refuses a move Talos will not make, and Phase 2C catches the silent rollback afterwards. Bringing the apply gates over would still require reproducing the gate calls in `upgrade_handler.go` or refactoring the apply flow. - **Phase 1/2 on `--insecure`**: the safety gates can't run before the chart renders, and the chart's `lookup` calls need an authenticated COSI connection. Insecure path = effectively no gates today. ## D. Apply (insecure / maintenance path) @@ -1320,6 +1320,69 @@ Expected: stderr emits the "Synced machine.install.image" line immediately after Regression anchor: contract tests `TestContract_WriteBackInstallImage_*` pin the on-disk shape of the patch (scalar swap, idempotency, silent-skip on orphan files, structural errors, file-list fan-out). A regression that fires the write-back BEFORE verify (or instead of verify) would silently pin the body to an image the node never actually ran — the failure-path scenario above would catch it. Cross-reference: `pkg/commands/upgrade_image_writeback.go`. + +### E5. Node body image divergence is reported + +The upgrade target comes from `values.yaml`, so a node body naming something else is ignored and then rewritten. Both directions are reported, but only one of them is a problem. + +```bash +# Point a node body at a NEWER Talos than values.yaml names. +sed -i.bak 's|image: ghcr.io/cozystack/cozystack/talos:.*|image: ghcr.io/cozystack/cozystack/talos:|' nodes/node0.yaml +talm upgrade -f nodes/node0.yaml 2>upgrade.log; head -5 upgrade.log +``` + +Do not pipe this into `head`. With `2>&1 |` talm's stderr is the pipe, `head` closes it after five lines, and a Go process takes SIGPIPE on fd 2 rather than an error — talm dies after the RPC has already fired, skipping the post-upgrade verify and the `install.image` write-back that E4 covers. The pipeline still exits 0, so nothing says it happened. + +Expected: `Using image from values.yaml: ` followed by a `warning:` line naming `nodes/node0.yaml`, the body's image, and the resolved target, telling you to put that image in values.yaml if the file is the one you meant. The warning fires because the body names a newer Talos than the target: the upgrade is going somewhere the file does not. + +`` only has to be newer than what `values.yaml` names — the comparison is ordered down to the patch, so `v1.13.1` against a `v1.13.0` target warns just like a newer minor does. A node moving backwards by a patch is the same incident one granularity down, and the pre-upgrade path check lets it through because Talos allows patch moves inside a minor. + +Now the ordinary direction. Restore the body, then bump `values.yaml` forward a minor and leave the bodies as rendered, which is what an operator raising the cluster's version actually does: + +```bash +mv nodes/node0.yaml.bak nodes/node0.yaml +cp values.yaml values.yaml.orig +sed -i '' 's|^image: .*|image: "ghcr.io/cozystack/cozystack/talos:"|' values.yaml +talm upgrade -f nodes/node0.yaml 2>upgrade.log; head -5 upgrade.log +``` + +Expected: the divergence reports as a plain line with no `warning:` label and no advice. That is the canonical flow, where every body trails the target until the write-back resyncs it, so advice to bump values.yaml would be advice to repeat what you just did. + +Watch for: + +- A `warning:` label on the trailing-body case, which is every node after a values.yaml bump. +- Silence on a body that names another registry at the same version: that one cannot be shown to trail, so it is a divergence. +- The report firing when the body already matches the target, or on a side-patch that declares no `install.image`. + +### E6. Pre-upgrade path check + +The refusal itself stops before the RPC, so that half needs no node that reboots. The escape-hatch flag is a different matter — see the warning below it. + +Pick a target Talos actually refuses: two minors back, or several minors forward. One minor back is a supported rollback and must NOT be blocked. + +```bash +sed -i '' 's|^image: .*|image: "ghcr.io/cozystack/cozystack/talos:v1.10.5"|' values.yaml +talm upgrade -f nodes/node0.yaml +``` + +Expected: the upgrade is refused with `refusing this upgrade: host version is too new to downgrade to Talos 1.10.5`, and a hint offering the two ways out. Nothing is sent to the node. + +Do not run `--skip-upgrade-path-check` on this fixture to see the escape hatch work. It does work, and that is the problem: the RPC goes out and the node attempts a cross-minor downgrade, which is what the hint two lines above says ends in an A/B rollback or a node that will not boot. Exercise the flag only against a target the node can actually take, or on a stand you are willing to rebuild. + +Watch for: + +- A refusal on one minor back, which Talos supports: the guard must follow the matrix, not a version comparison. +- A node that is unreachable blocking the upgrade: that case prints `warning: upgrade-path check skipped for ` and carries on. +- A missing hint on the refusal: `errors.Join` drops the hints of the errors it joins, so the way out has to be attached to the joined error. + +Restore `values.yaml` afterwards, from the copy E5 took before it edited anything: + +```bash +mv values.yaml.orig values.yaml +``` + +E5 and E6 both edit `values.yaml`, so they must not share one backup: a second `sed -i.bak` overwrites the first backup with already-edited content, and the restore then puts back E5's value rather than the original. The sections that follow read `values.yaml::image`. + ## F. CA rotation ### F1. Rotate CA dry-run diff --git a/docs/operations/safety-gates.md b/docs/operations/safety-gates.md index 8eeb68b..9a4f38c 100644 --- a/docs/operations/safety-gates.md +++ b/docs/operations/safety-gates.md @@ -8,6 +8,7 @@ | Pre-apply drift preview | `--skip-drift-preview` | on | | Post-apply state verification | `--skip-post-apply-verify` | **off** | | Post-upgrade version verify | `--skip-post-upgrade-verify` | on | +| Pre-upgrade path check | `--skip-upgrade-path-check` | on | ## 1. Declared-resource existence @@ -49,6 +50,14 @@ After `ApplyConfiguration` returns success, re-reads the on-node MachineConfig a After `talm upgrade` reports success, waits the configured reconcile window (default 90s; tune via `--post-upgrade-reconcile-window` for slow hardware / large image pulls) for the node to finish booting, then reads `runtime.Version` COSI and compares the running version's `(Major, Minor)` contract against the contract parsed from the target image tag. Point releases share a minor contract; cross-minor mismatch surfaces as a hint-bearing blocker. Catches the silent A/B rollback case where the upgrade RPC acks success but Talos rolled back to the previous partition (cross-vendor image, missing extensions, failed boot readiness check, slow boot exceeding the configured window). Best-effort surrender on digest-pinned images and unparseable tags. +## 5. Pre-upgrade path check + +Before `talm upgrade` fires the RPC, reads `runtime.Version` COSI from every target node and asks Talos's own compatibility matrix (`pkg/machinery/compatibility`) whether the node can be moved to the target. That is the same matrix the Talos installer runs as its own pre-flight, so this gate answers with the authority rather than a version comparison of its own. + +Two shapes are refused. A target too far forward — Talos accepts upgrades only from a bounded range of older versions — would otherwise fail inside the installer after the image has been pulled. A downgrade past what Talos allows (it supports at most one minor back) is invisible to gate 4, because once the downgrade takes, running equals target and the verify passes; the post-upgrade write-back then pins the node body to the older image. + +The usual way in is a `values.yaml` nobody bumped, so the refusal carries Talos's own verdict and offers bumping it or passing `--image`. To try anyway, `--skip-upgrade-path-check`. Best-effort surrender on digest-pinned images, unparseable tags, and a target newer than any minor this talm build's matrix knows — that last one means the binary is too old to have an opinion, not that the path is wrong. A node whose version cannot be read is reported and skipped rather than blocked: an unreachable node is a worse reason to stop an upgrade than the move this prevents. + ## Skip flags and the maintenance path The skip flags don't suppress each other — pass them independently. On the `--insecure` (maintenance) path the gates are functionally unreachable for charts that drive discovery via `lookup` — those COSI lookups require an authenticated connection and the render itself errors before any gate runs. Charts that render fully offline (no `lookup` calls) reach the gates on `--insecure` as well, with the Phase 2 hooks degrading gracefully because the `MachineConfig` resource is Sensitive. diff --git a/docs/reference/upgrade.md b/docs/reference/upgrade.md index 2cb690a..840e2f2 100644 --- a/docs/reference/upgrade.md +++ b/docs/reference/upgrade.md @@ -16,8 +16,15 @@ Image resolution (when -f is provided): The first -f file anchors the project root (Chart.yaml + secrets.yaml); its modeline supplies the nodes / endpoints. The -node body's machine.install.image is no longer consulted by the -upgrade flow. +node body's machine.install.image no longer selects the target: +it is read only to report it back, and rewritten afterwards. + + - A body naming an older Talos than the target is the normal + shape after a values.yaml bump, since refreshing node files + first is not required. It is reported as a plain line. + - A body naming a newer Talos, or an image from somewhere else, + means the upgrade is not going where that file says. That one + is a warning, and it names the way out. Post-upgrade sync (when the upgrade succeeds): - talm point-patches machine.install.image in every -f node body @@ -33,6 +40,19 @@ Post-upgrade sync (when the upgrade succeeds): intentionally leaves the body untouched, so it still reflects what the node actually runs. +Pre-upgrade guard: + + - Before the RPC, every target node's running version is checked + against the target through Talos's own compatibility matrix, the + same one the installer runs as its pre-flight. An unsupported + move is refused: too far forward fails inside the installer after + the image is pulled, and a downgrade past what Talos allows is + invisible to the post-upgrade verify, which sees running == + target once it took. --skip-upgrade-path-check opts out. + - A node whose version cannot be read is reported and skipped + rather than blocked, and so is a target newer than any minor this + binary's matrix knows. + ``` talm upgrade [flags] ``` @@ -45,7 +65,7 @@ talm upgrade [flags] --drain-timeout duration timeout for draining the Kubernetes node (default 5m0s) -f, --file strings specify config files or patches in a YAML file (can specify multiple) -h, --help help for upgrade - -i, --image string the container image to use for performing the install (default "factory.talos.dev/metal-installer/376567988ad370138ad8b2698212367b8edcb69b5fd68c80be1f2ec7d603b4ba:v1.14.0") + -i, --image string the container image to use for performing the install; with -f and no explicit --image the target is values.yaml::image at the project root, and the default shown here does not apply (default "factory.talos.dev/metal-installer/376567988ad370138ad8b2698212367b8edcb69b5fd68c80be1f2ec7d603b4ba:v1.14.0") --legacy force use of legacy upgrade method --namespace string namespace to use: "system" (etcd and kubelet images), "cri" for all Kubernetes workloads, "inmem" for in-memory containerd instance (default "system") --no-reboot do not reboot the node after upgrade (skip reboot and drain) @@ -53,6 +73,7 @@ talm upgrade [flags] --progress string output mode for upgrade progress. Values: [auto plain] (default "auto") -m, --reboot-mode string select the reboot mode during upgrade. Mode "powercycle" bypasses kexec. Values: [default force powercycle] (default "default") --skip-post-upgrade-verify skip the post-upgrade check that compares running Talos version against the target image's tag (detects silent A/B rollback after the RPC acks success) + --skip-upgrade-path-check skip the pre-upgrade check that refuses a target the node cannot be moved to --timeout duration time to wait for the operation is complete if --debug or --wait is set (default 30m0s) --wait wait for the operation to complete, tracking its progress. always set to true when --debug is set (default true) ``` diff --git a/go.mod b/go.mod index 8fa6255..9f075c0 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( filippo.io/age v1.3.1 github.com/BurntSushi/toml v1.6.0 github.com/Masterminds/sprig/v3 v3.3.0 + github.com/blang/semver/v4 v4.0.0 github.com/cockroachdb/errors v1.14.0 github.com/cosi-project/runtime v1.16.3 github.com/dustin/go-humanize v1.0.1 @@ -70,7 +71,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/sts v1.45.4 // indirect github.com/aws/smithy-go v1.27.7 // indirect github.com/beorn7/perks v1.0.1 // indirect - github.com/blang/semver/v4 v4.0.0 // indirect github.com/brianvoe/gofakeit/v7 v7.14.1 // indirect github.com/cenkalti/backoff/v4 v4.3.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect diff --git a/pkg/commands/contract_upgrade_image_divergence_test.go b/pkg/commands/contract_upgrade_image_divergence_test.go new file mode 100644 index 0000000..86cb4ab --- /dev/null +++ b/pkg/commands/contract_upgrade_image_divergence_test.go @@ -0,0 +1,249 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// After a values.yaml bump every node body still names the previous image: +// the command's own help calls that the canonical way to raise the cluster's +// Talos version and says re-templating first is not required. So a body older +// than the target is the normal shape, and telling that operator to "bump +// values.yaml" is advice to repeat what they just did. +// +// The incident worth prescribing for is the other direction: values.yaml left +// on an old preset default while the node file was edited to something +// current. There the upgrade goes somewhere older than the file says, and the +// file is then rewritten to match it. +func TestContract_UpgradeDivergenceMessageMatchesTheDirection(t *testing.T) { + const target = "ghcr.io/cozystack/cozystack/talos:v1.12.6" + + write := func(t *testing.T, image string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "node0.yaml") + body := "machine:\n type: controlplane\n install:\n disk: /dev/sda\n image: " + image + "\n" + + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + return path + } + + t.Run("body behind the target is reported without prescribing", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "ghcr.io/cozystack/cozystack/talos:v1.11.0")}, target) + + out := buf.String() + if !strings.Contains(out, "v1.11.0") || !strings.Contains(out, "v1.12.6") { + t.Errorf("the stale body was not reported at all:\n%s", out) + } + + if strings.Contains(out, "Bump values.yaml") { + t.Errorf("told the operator to bump values.yaml, which is what put the body behind:\n%s", out) + } + + if strings.Contains(out, "warning:") { + t.Errorf("labelled the canonical flow as a warning:\n%s", out) + } + }) + + t.Run("body ahead of the target warns and prescribes", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "ghcr.io/cozystack/cozystack/talos:v1.13.0")}, target) + + out := buf.String() + if !strings.Contains(out, "warning:") { + t.Errorf("a body the upgrade will not honour was not flagged:\n%s", out) + } + + if !strings.Contains(out, "values.yaml") { + t.Errorf("the way out was not named:\n%s", out) + } + }) + + // A patch ahead is the same incident one granularity down: the node moves + // backwards, and the pre-upgrade guard lets it through because Talos allows + // patch moves inside a minor. Ordering below the minor is what catches it. + t.Run("body a patch ahead of the target warns", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "ghcr.io/cozystack/cozystack/talos:v1.12.9")}, target) + + if out := buf.String(); !strings.Contains(out, "warning:") { + t.Errorf("a body a patch ahead was reported as the canonical flow:\n%s", out) + } + }) + + t.Run("body a patch behind the target is the canonical flow", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "ghcr.io/cozystack/cozystack/talos:v1.12.1")}, target) + + if out := buf.String(); strings.Contains(out, "warning:") { + t.Errorf("a trailing patch was flagged as going somewhere else:\n%s", out) + } + }) + + t.Run("a body already on the target says nothing", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, target)}, target) + + if out := buf.String(); out != "" { + t.Errorf("reported a body that is already on the target, which is every node on an ordinary upgrade:\n%s", out) + } + }) + + t.Run("an empty image declares nothing", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, `""`)}, target) + + if out := buf.String(); out != "" { + t.Errorf("reported an empty image, which would print a message with a hole in it:\n%s", out) + } + }) + + t.Run("a body with no tag warns", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "ghcr.io/cozystack/cozystack/talos")}, target) + + if out := buf.String(); !strings.Contains(out, "warning:") { + t.Errorf("an untagged body cannot be shown to trail the target, so it is a divergence:\n%s", out) + } + }) + + t.Run("a body naming another image warns", func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{write(t, "registry.example.com/custom/talos:v1.12.6")}, target) + + if out := buf.String(); !strings.Contains(out, "warning:") { + t.Errorf("an image the upgrade will not go to was not flagged:\n%s", out) + } + }) +} + +// The godoc promises silence on a file the walker cannot make sense of. Each +// of these reaches a different surrender: an unreadable path, a stream that +// does not parse, and a structurally wrong machine.install.image. +func TestContract_UpgradeDivergenceWarningSurrendersQuietly(t *testing.T) { + dir := t.TempDir() + + badYAML := filepath.Join(dir, "broken.yaml") + if err := os.WriteFile(badYAML, []byte("machine:\n\tinstall: [\n"), 0o600); err != nil { + t.Fatal(err) + } + + wrongShape := filepath.Join(dir, "wrong-shape.yaml") + if err := os.WriteFile(wrongShape, []byte("machine:\n install: \"oops\"\n"), 0o600); err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + name string + path string + }{ + {name: "unreadable file", path: filepath.Join(dir, "does-not-exist.yaml")}, + {name: "unparseable stream", path: badYAML}, + {name: "structurally wrong install", path: wrongShape}, + } { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + + warnNodeBodyImageDivergence(&buf, []string{tc.path}, "ghcr.io/cozystack/cozystack/talos:v1.12.6") + + if out := buf.String(); out != "" { + t.Errorf("warned instead of surrendering:\n%s", out) + } + }) + } +} + +// The report only exists if the handler still calls it. Replacing the call +// with `_ = filesToProcess` leaves every test above green, because they all +// drive the function directly — so this one goes through cmd.RunE. +// +// The downgrade guard has the same pin for the same reason; without this the +// two halves of the branch are covered unevenly. +func TestContract_UpgradeDivergenceReportReachesTheOperator(t *testing.T) { + root := t.TempDir() + + // DetectAndSetRootFromFiles anchors on these two, so the -f file has to sit + // inside something shaped like a talm project. + for name, content := range map[string]string{ + "Chart.yaml": "apiVersion: v2\nname: t\nversion: 0.1.0\n", + "secrets.yaml": "cluster:\n id: x\n", + } { + if err := os.WriteFile(filepath.Join(root, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + + if err := os.WriteFile(filepath.Join(root, "values.yaml"), + []byte("image: ghcr.io/cozystack/cozystack/talos:v1.12.6\n"), 0o600); err != nil { + t.Fatal(err) + } + + nodeFile := filepath.Join(root, "node0.yaml") + body := "# talm: nodes=[\"192.0.2.10\"]\nmachine:\n type: controlplane\n install:\n image: ghcr.io/cozystack/cozystack/talos:v1.13.0\n" + + if err := os.WriteFile(nodeFile, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + savedRoot, savedVerify, savedNodes := Config.RootDir, upgradeCmdFlags.skipPostUpgradeVerify, GlobalArgs.Nodes + + t.Cleanup(func() { + Config.RootDir = savedRoot + upgradeCmdFlags.skipPostUpgradeVerify = savedVerify + GlobalArgs.Nodes = savedNodes + }) + + Config.RootDir = root + + cmd := &cobra.Command{Use: upgradeCmdName} + cmd.Flags().StringP("image", "i", "", "") + cmd.Flags().Bool("stage", false, "") + cmd.Flags().StringSliceP("file", "f", nil, "") + wrapUpgradeCommand(cmd, func(*cobra.Command, []string) error { return nil }) + + if err := cmd.Flags().Set("file", nodeFile); err != nil { + t.Fatal(err) + } + + upgradeCmdFlags.skipPostUpgradeVerify = true + + // The RunE error is deliberately ignored: the report is written during + // image resolution, before anything that needs a cluster, so whatever + // happens later does not affect what the operator already saw. + out := captureStderr(t, func() { _ = cmd.RunE(cmd, nil) }) + + if !strings.Contains(out, "node0.yaml") || !strings.Contains(out, "v1.13.0") { + t.Errorf("the divergence never reached the operator through the real flow:\n%s", out) + } +} diff --git a/pkg/commands/contract_upgrade_image_help_test.go b/pkg/commands/contract_upgrade_image_help_test.go new file mode 100644 index 0000000..3dda7ed --- /dev/null +++ b/pkg/commands/contract_upgrade_image_help_test.go @@ -0,0 +1,39 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// The --image flag comes from upstream, whose help names a factory image as +// the default. talm resolves the target from values.yaml whenever -f is given +// without an explicit --image, so the inherited text describes a default that +// the documented flow never reaches. +func TestContract_UpgradeImageFlagHelpNamesTheResolution(t *testing.T) { + cmd := &cobra.Command{Use: "upgrade"} + cmd.Flags().StringP("image", "i", "factory.talos.dev/metal-installer/abc:v1.14.0", + "the container image to use for performing the install") + + wrapUpgradeCommand(cmd, nil) + + usage := cmd.Flags().Lookup("image").Usage + if !strings.Contains(usage, "values.yaml") { + t.Errorf("--image help does not mention where the target actually comes from:\n%s", usage) + } +} diff --git a/pkg/commands/contract_upgrade_path_guard_test.go b/pkg/commands/contract_upgrade_path_guard_test.go new file mode 100644 index 0000000..324fd91 --- /dev/null +++ b/pkg/commands/contract_upgrade_path_guard_test.go @@ -0,0 +1,269 @@ +// Copyright Cozystack Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "bytes" + "context" + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/cockroachdb/errors" + "github.com/siderolabs/talos/pkg/machinery/api/machine" + "github.com/siderolabs/talos/pkg/machinery/compatibility" + "github.com/spf13/cobra" +) + +// talosRef builds the installer reference shape the guard actually receives; +// the guard lifts the version out of the tag, it is not handed one. +func talosRef(tag string) string { + return "ghcr.io/cozystack/cozystack/talos:" + tag +} + +// Talos ships the authoritative upgrade matrix in pkg/machinery/compatibility +// and its own installer runs it as a pre-flight. Hand-rolling the comparison +// gets it wrong in both directions: one minor back is supported and would be +// refused, while a jump too far forward is not and would be waved through to +// fail inside the installer after the image is already pulled. +func TestContract_UnsupportedUpgradePathIsRefusedBeforeTheRPC(t *testing.T) { + for _, tc := range []struct { + name string + running string + target string + wantBlock bool + }{ + {name: "one minor forward", running: "v1.12.6", target: talosRef("v1.13.0"), wantBlock: false}, + {name: "same minor, newer patch", running: "v1.12.5", target: talosRef("v1.12.6"), wantBlock: false}, + {name: "same version", running: "v1.12.6", target: talosRef("v1.12.6"), wantBlock: false}, + // One minor back is a supported rollback, and the most common reason to + // move a node backwards at all. + {name: "one minor back", running: "v1.14.0", target: talosRef("v1.13.7"), wantBlock: false}, + {name: "two minors back", running: "v1.14.0", target: talosRef("v1.12.6"), wantBlock: true}, + // Too far forward fails inside the installer, after the image is pulled. + {name: "three minors forward", running: "v1.11.0", target: talosRef("v1.14.0"), wantBlock: true}, + {name: "running unreadable", running: "", target: talosRef("v1.12.6"), wantBlock: false}, + {name: "target unparseable", running: "v1.13.0", target: talosRef("latest"), wantBlock: false}, + {name: "digest-pinned target", running: "v1.13.0", target: "ghcr.io/cozystack/cozystack/talos@sha256:abc123", wantBlock: false}, + // A target newer than anything this talm build knows means the matrix + // cannot judge it, not that the path is wrong. + {name: "target beyond this build's matrix", running: "v1.14.0", target: talosRef("v1.15.0"), wantBlock: false}, + } { + t.Run(tc.name, func(t *testing.T) { + err := checkUpgradePathSupported(tc.running, tc.target) + + if tc.wantBlock && err == nil { + t.Errorf("running=%s target=%s: expected a block, got none", tc.running, tc.target) + } + + if !tc.wantBlock && err != nil { + t.Errorf("running=%s target=%s: unexpected block: %v", tc.running, tc.target, err) + } + + // Talos normalises the version it prints, dropping the leading "v". + if tc.wantBlock && err != nil && !strings.Contains(err.Error(), strings.TrimPrefix(tc.running, "v")) { + t.Errorf("block does not name the running version: %v", err) + } + }) + } +} + +// A node that cannot be read must not block the upgrade: an unreachable node +// is a worse reason to refuse than the downgrade the guard is there to stop. +func TestContract_UpgradePathCheckSkipsUnreadableNodes(t *testing.T) { + var buf bytes.Buffer + + unreadable := func(context.Context) (string, bool, error) { + return "", false, errors.New("connection refused") + } + + if err := checkNodesUpgradePath(context.Background(), []string{"node0"}, talosRef("v1.12.6"), unreadable, &buf); err != nil { + t.Errorf("an unreadable node blocked the upgrade: %v", err) + } + + if !strings.Contains(buf.String(), "node0") { + t.Errorf("the skip was not reported:\n%s", buf.String()) + } +} + +// Every node is consulted, so a set upgrade surfaces all refusals at once. +func TestContract_UpgradePathCheckReportsEveryNode(t *testing.T) { + var buf bytes.Buffer + + running := func(context.Context) (string, bool, error) { + return "v1.14.0", true, nil + } + + err := checkNodesUpgradePath(context.Background(), []string{"node0", "node1"}, talosRef("v1.12.6"), running, &buf) + if err == nil { + t.Fatal("expected the upgrade to be refused") + } + + for _, node := range []string{"node0", "node1"} { + if !strings.Contains(err.Error(), node) { + t.Errorf("refusal does not name %s: %v", node, err) + } + } + + // The refusal is the only thing standing between the operator and a + // deliberate downgrade, so it has to carry the way out. errors.Join drops + // hints from the errors it joins, including a single one, so the hint has + // to sit on the joined error rather than on each refusal. + hints := strings.Join(errors.GetAllHints(err), "\n") + if !strings.Contains(hints, "--skip-upgrade-path-check") { + t.Errorf("refusal does not tell the operator how to proceed anyway:\n%s", hints) + } +} + +// The guard only helps if it runs before the RPC, and --skip-upgrade-path-check +// only helps if it actually suppresses it. Neither is visible in the pure +// functions: moving the call below originalRunE, or dropping the flag check, +// leaves every other test in this file green. +// +// The skip half runs offline — with the flag set the guard never builds a +// client — so the sentinel reaching originalRunE is the whole assertion. +func TestContract_UpgradePathCheckIsSkippableAndRunsBeforeTheRPC(t *testing.T) { + savedSkip := upgradeCmdFlags.skipUpgradePathCheck + savedWindow := upgradeCmdFlags.postUpgradeReconcileWindow + savedVerify := upgradeCmdFlags.skipPostUpgradeVerify + + t.Cleanup(func() { + upgradeCmdFlags.skipUpgradePathCheck = savedSkip + upgradeCmdFlags.postUpgradeReconcileWindow = savedWindow + upgradeCmdFlags.skipPostUpgradeVerify = savedVerify + }) + + reached := false + + cmd := &cobra.Command{Use: upgradeCmdName} + cmd.Flags().StringP("image", "i", "", "") + cmd.Flags().Bool("stage", false, "") + wrapUpgradeCommand(cmd, func(*cobra.Command, []string) error { + reached = true + + return nil + }) + + upgradeCmdFlags.skipPostUpgradeVerify = true + + // Both halves need a target the guard would actually judge and a + // talosconfig it cannot use. Hoisted above the subtests so the only thing + // that differs between them is the flag: with the setup inside the second + // subtest, the first one left the image empty and the guard returned before + // ever reading the flag, which made it pass with the flag check deleted. + if err := cmd.Flags().Set("image", talosRef("v1.12.6")); err != nil { + t.Fatal(err) + } + + savedTalosconfig := GlobalArgs.Talosconfig + t.Cleanup(func() { GlobalArgs.Talosconfig = savedTalosconfig }) + + GlobalArgs.Talosconfig = filepath.Join(t.TempDir(), "absent.yaml") + + t.Run("the flag suppresses the guard", func(t *testing.T) { + reached = false + upgradeCmdFlags.skipUpgradePathCheck = true + + if err := cmd.RunE(cmd, nil); err != nil { + t.Fatalf("skipped guard still failed the upgrade: %v", err) + } + + if !reached { + t.Error("--skip-upgrade-path-check did not suppress the guard: the RPC was never reached") + } + }) + + t.Run("a guard that cannot run blocks before the RPC", func(t *testing.T) { + reached = false + upgradeCmdFlags.skipUpgradePathCheck = false + + // No talosconfig means the guard cannot build a client and returns an + // error. What that pins is the ordering: the sentinel must not have + // fired, so the guard sits above the RPC rather than after it. + if err := cmd.RunE(cmd, nil); err == nil { + t.Fatal("expected the guard to fail without a talosconfig") + } + + if reached { + t.Error("the RPC fired before the guard: moving the guard below originalRunE would not be caught") + } + }) +} + +// An empty node list must say so rather than pass silently, matching the +// post-upgrade verify's contract for the same shape. +func TestContract_UpgradePathCheckAnnouncesNoTargetNodes(t *testing.T) { + var buf bytes.Buffer + + never := func(context.Context) (string, bool, error) { + t.Error("the reader was called with no target nodes") + + return "", false, nil + } + + if err := checkNodesUpgradePath(context.Background(), nil, talosRef("v1.12.6"), never, &buf); err != nil { + t.Fatalf("an empty node list failed the upgrade: %v", err) + } + + if !strings.Contains(buf.String(), "no target nodes") { + t.Errorf("the skip was not announced:\n%s", buf.String()) + } +} + +// A reader may surrender without an error — the "not applicable on this path" +// shape the post-upgrade verify also allows for. Reporting that as +// "could not read: " would name a cause that does not exist. +func TestContract_UpgradePathCheckReportsASilentSurrender(t *testing.T) { + var buf bytes.Buffer + + silent := func(context.Context) (string, bool, error) { + return "", false, nil + } + + if err := checkNodesUpgradePath(context.Background(), []string{"node0"}, talosRef("v1.12.6"), silent, &buf); err != nil { + t.Fatalf("a silent surrender blocked the upgrade: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "unavailable") { + t.Errorf("the surrender was not reported in its own shape:\n%s", out) + } + + if strings.Contains(out, "") { + t.Errorf("named a cause that does not exist:\n%s", out) + } +} + +// The guard tells "this minor is absent from the matrix" from a real verdict by +// probing the target against itself, which only holds while a same-version pair +// can fail for no other reason. That is a property of Talos's tables, not a +// contract they owe us: the day an upstream bump lists a version of the same +// minor in DeniedHostUpgradeVersions, the probe would read it as "unknown +// minor" and switch the whole guard off silently. +func TestContract_UpgradePathSelfProbeStillSeparatesUnknownMinors(t *testing.T) { + for minor := 2; minor <= 14; minor++ { + tag := fmt.Sprintf("v1.%d.0", minor) + + version, err := compatibility.ParseTalosVersion(&machine.VersionInfo{Tag: tag}) + if err != nil { + t.Fatalf("%s: %v", tag, err) + } + + if err := version.UpgradeableFrom(version); err != nil { + t.Errorf("%s fails against itself, so the guard reads it as an unknown minor and turns itself off: %v", tag, err) + } + } +} diff --git a/pkg/commands/preflight_upgrade_verify.go b/pkg/commands/preflight_upgrade_verify.go index dd4d355..a9e76c9 100644 --- a/pkg/commands/preflight_upgrade_verify.go +++ b/pkg/commands/preflight_upgrade_verify.go @@ -22,6 +22,8 @@ import ( "time" "github.com/cockroachdb/errors" + "github.com/siderolabs/talos/pkg/machinery/api/machine" + "github.com/siderolabs/talos/pkg/machinery/compatibility" machineryconfig "github.com/siderolabs/talos/pkg/machinery/config" ) @@ -161,3 +163,59 @@ func parseTargetVersion(image string) string { return tail[idx+1:] } + +// upgradePathHint is attached to a refused upgrade path. Both ways out are +// real: the usual cause is a values.yaml nobody bumped, and the rare +// deliberate case needs the flag. +const upgradePathHint = "Talos supports upgrading from a limited range of versions and downgrading at most one minor. " + + "Point the upgrade at a version the node can take: bump values.yaml::image in the project, or pass --image. " + + "If you mean to try it anyway, re-run with --skip-upgrade-path-check." + +// checkUpgradePathSupported refuses a target the node cannot be moved to. +// +// The matrix is Talos's own (pkg/machinery/compatibility), the same one the +// installer runs as its pre-flight, so this asks the authority rather than +// guessing. Guessing gets it wrong in both directions: a downgrade of one +// minor IS supported, and an upgrade from too far back is not, which a plain +// version comparison has backwards. +// +// The post-upgrade verify cannot cover either case. A downgrade that took +// leaves running equal to target, so the gate passes and the write-back then +// pins the node file to the older image; an upgrade the installer rejects +// fails only after the image has been pulled. +// +// Anything unreadable on either side surrenders silently, and so does a target +// newer than any minor this talm build's matrix knows: that means the binary +// is too old to have an opinion, not that the path is wrong. +func checkUpgradePathSupported(running, targetImage string) error { + targetTag := parseTargetVersion(targetImage) + if targetTag == "" || running == "" { + return nil + } + + target, err := compatibility.ParseTalosVersion(&machine.VersionInfo{Tag: targetTag}) + if err != nil { + return nil //nolint:nilerr // surrender on an unparseable tag; see the post-upgrade verify. + } + + host, err := compatibility.ParseTalosVersion(&machine.VersionInfo{Tag: running}) + if err != nil { + return nil //nolint:nilerr // same surrender for an unreadable running version. + } + + // Probing the target against itself separates "this minor is absent from + // the matrix" from a real verdict: the matrix's default arm is the only + // thing that can fail a same-version pair. The error is the answer here, + // not a failure — a matrix that cannot place the target has no opinion to + // act on. + if knownToMatrix := target.UpgradeableFrom(target) == nil; !knownToMatrix { + return nil + } + + if err := target.UpgradeableFrom(host); err != nil { + //nolint:wrapcheck // cockroachdb/errors.WithHint at boundary. + return errors.WithHint(errors.Wrap(err, "refusing this upgrade"), upgradePathHint) + } + + return nil +} diff --git a/pkg/commands/upgrade_handler.go b/pkg/commands/upgrade_handler.go index b7d4436..bc87047 100644 --- a/pkg/commands/upgrade_handler.go +++ b/pkg/commands/upgrade_handler.go @@ -47,6 +47,7 @@ const ( //nolint:gochecknoglobals // command-scoped flag struct, mirrors applyCmdFlags pattern. var upgradeCmdFlags struct { skipPostUpgradeVerify bool + skipUpgradePathCheck bool postUpgradeReconcileWindow time.Duration } @@ -90,8 +91,15 @@ Image resolution (when -f is provided): The first -f file anchors the project root (Chart.yaml + secrets.yaml); its modeline supplies the nodes / endpoints. The -node body's machine.install.image is no longer consulted by the -upgrade flow. +node body's machine.install.image no longer selects the target: +it is read only to report it back, and rewritten afterwards. + + - A body naming an older Talos than the target is the normal + shape after a values.yaml bump, since refreshing node files + first is not required. It is reported as a plain line. + - A body naming a newer Talos, or an image from somewhere else, + means the upgrade is not going where that file says. That one + is a warning, and it names the way out. Post-upgrade sync (when the upgrade succeeds): - talm point-patches machine.install.image in every -f node body @@ -105,11 +113,38 @@ Post-upgrade sync (when the upgrade succeeds): - The patch fires after post-upgrade verify confirms the running version matches the target. A failed verify (auto-rollback) intentionally leaves the body untouched, so it still reflects - what the node actually runs.` + what the node actually runs. + +Pre-upgrade guard: + + - Before the RPC, every target node's running version is checked + against the target through Talos's own compatibility matrix, the + same one the installer runs as its pre-flight. An unsupported + move is refused: too far forward fails inside the installer after + the image is pulled, and a downgrade past what Talos allows is + invisible to the post-upgrade verify, which sees running == + target once it took. --skip-upgrade-path-check opts out. + - A node whose version cannot be read is reported and skipped + rather than blocked, and so is a target newer than any minor this + binary's matrix knows.` + + // Upstream's --image help advertises a factory image as the default, which + // only applies to a bare `talm upgrade`. With -f and no explicit --image the + // target comes from values.yaml, so the inherited text describes a default + // the documented flow never reaches. Rewrite it rather than let the flag + // list contradict the synopsis above. + if imageFlag := wrappedCmd.Flags().Lookup("image"); imageFlag != nil { + imageFlag.Usage = "the container image to use for performing the install; " + + "with -f and no explicit --image the target is values.yaml::image at the project root, " + + "and the default shown here does not apply" + } wrappedCmd.Flags().BoolVar(&upgradeCmdFlags.skipPostUpgradeVerify, "skip-post-upgrade-verify", false, "skip the post-upgrade check that compares running Talos version against the target image's tag (detects silent A/B rollback after the RPC acks success)") + wrappedCmd.Flags().BoolVar(&upgradeCmdFlags.skipUpgradePathCheck, "skip-upgrade-path-check", false, + "skip the pre-upgrade check that refuses a target the node cannot be moved to") + wrappedCmd.Flags().DurationVar(&upgradeCmdFlags.postUpgradeReconcileWindow, "post-upgrade-reconcile-window", defaultPostUpgradeReconcileWindow, "how long to wait after upgrade returns before re-reading the running version; widen for slow hardware / large image pulls") @@ -188,6 +223,7 @@ Post-upgrade sync (when the upgrade succeeds): fmt.Fprintf(os.Stderr, "Warning: failed to set --image flag: %v\n", err) } else { fmt.Fprintf(os.Stderr, "Using image from values.yaml: %s\n", image) + warnNodeBodyImageDivergence(os.Stderr, filesToProcess, image) } } @@ -203,6 +239,12 @@ Post-upgrade sync (when the upgrade succeeds): targetImage, _ := cmd.Flags().GetString("image") staged, _ := cmd.Flags().GetBool("stage") + if !upgradeCmdFlags.skipUpgradePathCheck { + if err := runPreUpgradePathCheck(targetImage); err != nil { + return err + } + } + // Execute original command var execErr error @@ -319,6 +361,75 @@ func runPostUpgradeVersionVerify(parentCtx context.Context, image string) error }) } +// runPreUpgradePathCheck reads the running version off every target node and +// refuses moves Talos's compatibility matrix rejects, in either direction. +// +// Read failures are not fatal here, unlike in the post-upgrade verify. There a +// node that will not answer IS the signal the gate exists to catch; here it +// only means the guard cannot form an opinion, and blocking an upgrade because +// one node was briefly unreachable would be worse than the downgrade this is +// meant to prevent. +func runPreUpgradePathCheck(image string) error { + if parseTargetVersion(image) == "" { + return nil + } + + return WithClient(func(ctx context.Context, c *client.Client) error { + ctxNodes := []string(nil) + if cfg := c.GetConfigContext(); cfg != nil { + ctxNodes = cfg.Nodes + } + + return checkNodesUpgradePath(ctx, resolveUpgradeTargetNodes(GlobalArgs.Nodes, ctxNodes), image, cosiVersionReader(c), os.Stderr) + }) +} + +// checkNodesUpgradePath is the testable body: every node is consulted and the +// verdicts are joined, so an operator upgrading a set sees each refusal at +// once instead of one per re-run. +func checkNodesUpgradePath(ctx context.Context, nodes []string, image string, read versionReader, w io.Writer) error { + if len(nodes) == 0 { + _, _ = fmt.Fprintln(w, "upgrade-path check: skipped, no target nodes resolved from --nodes or talosconfig context") + + return nil + } + + var refusals []error + + for _, node := range nodes { + running, ok, err := read(client.WithNode(ctx, node)) + if !ok { + // Split the two surrender shapes the way verifyPostUpgradeVersion + // does: a reader that signals "not applicable" without an error + // would otherwise be reported as "could not read: ". + if err != nil { + _, _ = fmt.Fprintf(w, "warning: upgrade-path check skipped for %s, could not read its running version: %v\n", node, err) + } else { + _, _ = fmt.Fprintf(w, "warning: upgrade-path check skipped for %s, its running version is unavailable\n", node) + } + + continue + } + + if err := checkUpgradePathSupported(running, image); err != nil { + refusals = append(refusals, errors.Wrapf(err, "node %s", node)) + } + } + + joined := errors.Join(refusals...) + if joined == nil { + return nil + } + + // errors.Join drops the hints of the errors it joins, a single one + // included, so the way out has to be attached to the joined error. Without + // this the operator gets a refusal naming no escape and the skip flag is + // discoverable only from --help. + // + //nolint:wrapcheck // cockroachdb/errors.WithHint at boundary. + return errors.WithHint(joined, upgradePathHint) +} + // resolveUpgradeTargetNodes picks the per-node target list for the // post-upgrade verify. CLI `--nodes` wins outright when non-empty; // otherwise the talosconfig context's pre-configured node list is diff --git a/pkg/commands/upgrade_image_source.go b/pkg/commands/upgrade_image_source.go index 3b53c91..a8de8f1 100644 --- a/pkg/commands/upgrade_image_source.go +++ b/pkg/commands/upgrade_image_source.go @@ -15,9 +15,13 @@ package commands import ( + "fmt" + "io" "os" "path/filepath" + "strings" + "github.com/blang/semver/v4" "github.com/cockroachdb/errors" "gopkg.in/yaml.v3" ) @@ -61,3 +65,117 @@ func resolveUpgradeImageFromValues(rootDir string) (string, error) { return values.Image, nil } + +// warnNodeBodyImageDivergence reports every -f body whose +// machine.install.image is not the resolved target. +// +// Two things happen silently to such a file: the upgrade ignores its image +// (the target is values.yaml::image) and the post-upgrade write-back then +// rewrites it. Both are worth stating, but only one direction is a problem. +// +// A body naming an OLDER Talos is the canonical shape: the command's help +// calls bumping values.yaml the way to raise the cluster's version and says +// re-templating first is not required, so every body trails the target until +// the write-back resyncs it. Reported as a plain fact, with no advice. +// +// A body naming a NEWER Talos, or an image whose version cannot be compared, +// is the incident: the upgrade goes somewhere the file does not name, and the +// file is then overwritten to match. That one carries the warning label and +// the way out. +// +// Only reached when the target was resolved from values.yaml. With an explicit +// --image the body is still ignored and still rewritten, but the operator named +// the target themselves, so the half of the surprise worth reporting is gone. +// +// Best-effort by design: a file that does not parse, or carries no install +// image at all, is skipped rather than reported. Side-patches and orphans in +// the -f list legitimately have neither, and a parse error surfaces from the +// write-back with a better message than a pre-flight warning could give. +func warnNodeBodyImageDivergence(w io.Writer, files []string, target string) { + if target == "" { + return + } + + for _, path := range files { + data, err := os.ReadFile(path) + if err != nil { + continue + } + + docs, err := decodeAllYAMLDocs(data) + if err != nil { + continue + } + + node, ok, err := locateInstallImageNode(docs) + if err != nil || !ok { + continue + } + + // An empty value declares nothing to diverge from, and reporting it + // would print a message with a hole where the image should be. + if node.Value == "" || node.Value == target { + continue + } + + if bodyTrailsTarget(node.Value, target) { + fmt.Fprintf(w, + "%s declares machine.install.image %s; the upgrade installs %s and rewrites the file to match "+ + "after it succeeds.\n", + path, node.Value, target) + + continue + } + + fmt.Fprintf(w, + "warning: %s declares machine.install.image %s, but the upgrade target is %s. "+ + "The file's image is not used; it will be rewritten to the target after a successful upgrade. "+ + "If the file is the one you meant, put that image in values.yaml or pass --image.\n", + path, node.Value, target) + } +} + +// bodyTrailsTarget reports whether the body's image names an older Talos than +// the target, which is what a values.yaml bump leaves behind. +// +// Ordered down to the patch. Comparing at major.minor would call a body on +// v1.12.9 against a v1.12.6 target "trailing", but that is the incident this +// report exists for, one granularity down: the node moves backwards and the +// pre-upgrade guard lets it through, because Talos allows patch moves inside a +// minor. Anything that cannot be compared — a different repository, a digest +// pin, an unparseable tag — is deliberately not treated as trailing, because +// then the upgrade really is going somewhere the file does not name. +func bodyTrailsTarget(bodyImage, target string) bool { + bodyVersion, targetVersion := parseTargetVersion(bodyImage), parseTargetVersion(target) + if bodyVersion == "" || targetVersion == "" { + return false + } + + if repositoryOf(bodyImage) != repositoryOf(target) { + return false + } + + body, err := semver.ParseTolerant(bodyVersion) + if err != nil { + return false + } + + want, err := semver.ParseTolerant(targetVersion) + if err != nil { + return false + } + + return body.LT(want) +} + +// repositoryOf strips the tag from an image reference, leaving the part that +// says where the image comes from. A body pointing at another repository is a +// divergence even when the versions line up. +func repositoryOf(image string) string { + tag := parseTargetVersion(image) + if tag == "" { + return image + } + + return strings.TrimSuffix(image, ":"+tag) +}