diff --git a/README.md b/README.md index 8ca77bf1..0bf6641c 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,17 @@ of silently embedding a placeholder. For cozystack VIP setups set `endpoint` and `floatingIP` together (same IP, single shared VIP); for single-node clusters use that node's routable IP and leave `floatingIP` blank; for multi-node with an external load balancer -use the LB URL and leave `floatingIP` blank. Subnet-selector fields +use the LB URL and leave `floatingIP` blank. When the VIP must sit +on a link that does not yet exist on the live system at first apply +(typically a VLAN sub-interface), set `vipLink` to that link name — +the chart pins `Layer2VIPConfig.link` to it instead of the default- +gateway link that discovery would otherwise pick, and emits the +document even on a totally fresh node where no default-gateway link +has been discovered yet. The chart does not auto-emit a `LinkConfig` +or `VLANConfig` for the override link; the operator is responsible +for ensuring the link comes up, typically by adding a `LinkConfig` +or `VLANConfig` for that link to the per-node body overlay alongside +`vipLink`. Subnet-selector fields (`kubelet.validSubnets`, `etcd.advertisedSubnets`) are derived automatically from the node's default-gateway-bearing link, so no override is needed unless you have a multi-homed node that requires diff --git a/charts/cozystack/templates/_helpers.tpl b/charts/cozystack/templates/_helpers.tpl index 7d8bcf41..a0bb0a88 100644 --- a/charts/cozystack/templates/_helpers.tpl +++ b/charts/cozystack/templates/_helpers.tpl @@ -182,6 +182,19 @@ nameservers: {{- else }} [] {{- end }} +{{- /* Operator-declared vipLink override: emit Layer2VIPConfig + regardless of discovery state. Useful when the target link + does not yet exist on the live system at first apply (typical + case: a VLAN sub-interface this template is about to bring up). + The discovery-derived block below skips its own Layer2VIPConfig + when this branch fires, so we never emit duplicates. */}} +{{- if and .Values.floatingIP .Values.vipLink (eq .MachineType "controlplane") }} +--- +apiVersion: v1alpha1 +kind: Layer2VIPConfig +name: {{ .Values.floatingIP | quote }} +link: {{ .Values.vipLink }} +{{- end }} {{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }} {{- if $defaultLinkName }} {{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }} @@ -258,11 +271,15 @@ addresses: routes: - gateway: {{ include "talm.discovered.default_gateway" . }} {{- end }} +{{- /* Discovery-derived Layer2VIPConfig: skipped when the operator + has set .Values.vipLink, since the override-path block above + has already emitted the document with the operator's chosen + link. */}} +{{- if and .Values.floatingIP (not .Values.vipLink) (eq .MachineType "controlplane") }} {{- $vipLinkName := $interfaceName }} {{- if $isVlan }} {{- $vipLinkName = $defaultLinkName }} {{- end }} -{{- if and .Values.floatingIP (eq .MachineType "controlplane") }} --- apiVersion: v1alpha1 kind: Layer2VIPConfig @@ -280,11 +297,21 @@ link: {{ $vipLinkName }} {{- (include "talm.discovered.physical_links_info" .) | nindent 4 }} {{- $existingInterfacesConfiguration := include "talm.discovered.existing_interfaces_configuration" . }} {{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }} - {{- if or $existingInterfacesConfiguration $defaultLinkName }} + {{- /* vipLink override on the legacy schema: legacy Talos has no + Layer2VIPConfig document, so the override is expressed as a + top-level interfaces[] entry that carries only the vip block. + When vipLink == $defaultLinkName the inline vip below already + lands on the right link, so no override entry is needed. */}} + {{- $vipOverride := and .Values.floatingIP .Values.vipLink (eq .MachineType "controlplane") (ne .Values.vipLink $defaultLinkName) }} + {{- /* Suppress the inline (discovery-derived) vip when the operator + has redirected it to a different link; otherwise the VIP would + be pinned twice on different interfaces. */}} + {{- $suppressInlineVip := and .Values.vipLink (ne .Values.vipLink $defaultLinkName) }} + {{- if or $existingInterfacesConfiguration $defaultLinkName $vipOverride }} interfaces: {{- if $existingInterfacesConfiguration }} {{- $existingInterfacesConfiguration | nindent 4 }} - {{- else }} + {{- else if $defaultLinkName }} {{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }} {{- $parentLinkName := "" }} {{- if $isVlan }} @@ -306,7 +333,7 @@ link: {{ $vipLinkName }} routes: - network: 0.0.0.0/0 gateway: {{ include "talm.discovered.default_gateway" . }} - {{- if and .Values.floatingIP (eq .MachineType "controlplane") }} + {{- if and .Values.floatingIP (eq .MachineType "controlplane") (not $suppressInlineVip) }} vip: ip: {{ .Values.floatingIP }} {{- end }} @@ -315,12 +342,17 @@ link: {{ $vipLinkName }} routes: - network: 0.0.0.0/0 gateway: {{ include "talm.discovered.default_gateway" . }} - {{- if and .Values.floatingIP (eq .MachineType "controlplane") }} + {{- if and .Values.floatingIP (eq .MachineType "controlplane") (not $suppressInlineVip) }} vip: ip: {{ .Values.floatingIP }} {{- end }} {{- end }} {{- end }} + {{- if $vipOverride }} + - interface: {{ .Values.vipLink }} + vip: + ip: {{ .Values.floatingIP }} + {{- end }} {{- end }} {{- end }} diff --git a/charts/cozystack/values.yaml b/charts/cozystack/values.yaml index baecf331..d76f105f 100644 --- a/charts/cozystack/values.yaml +++ b/charts/cozystack/values.yaml @@ -29,6 +29,26 @@ clusterDomain: cozy.local # Single-node clusters and external-LB topologies leave it blank. # Example: floatingIP: 192.168.0.1 floatingIP: "" + +# Optional override for the link Layer2VIPConfig is pinned to. When +# left empty the chart picks the default-gateway-bearing link the +# node already has (the VLAN sub-interface if one carries the +# default route, otherwise the physical NIC). Set this when the +# target link does not yet exist on the live system at first apply +# -- typically a VLAN sub-interface that the same template is about +# to bring up. Without an override the chart would derive the link +# from discovery on the bare-metal NIC, pin the VIP there, and the +# VIP would land on the wrong link once the VLAN comes up. +# When set, Layer2VIPConfig is emitted unconditionally, even on a +# totally fresh node where discovery has not yet resolved a default- +# gateway link. The chart does NOT auto-emit a LinkConfig/VLANConfig +# for the override link; the operator is responsible for ensuring +# the link comes up — typically by adding a LinkConfig or VLANConfig +# document for that link to the per-node body overlay. If no such +# document brings vipLink up, Layer2VIPConfig will dangle on a +# non-existent link and the cluster endpoint will be unreachable. +# Example: vipLink: eth0.4000 +vipLink: "" image: "ghcr.io/cozystack/cozystack/talos:v1.12.6" podSubnets: - 10.244.0.0/16 diff --git a/charts/generic/templates/_helpers.tpl b/charts/generic/templates/_helpers.tpl index dc080ed1..f4d7ab2c 100644 --- a/charts/generic/templates/_helpers.tpl +++ b/charts/generic/templates/_helpers.tpl @@ -105,6 +105,19 @@ nameservers: {{- else }} [] {{- end }} +{{- /* Operator-declared vipLink override: emit Layer2VIPConfig + regardless of discovery state. Useful when the target link + does not yet exist on the live system at first apply (typical + case: a VLAN sub-interface this template is about to bring up). + The discovery-derived block below skips its own Layer2VIPConfig + when this branch fires, so we never emit duplicates. */}} +{{- if and .Values.floatingIP .Values.vipLink (eq .MachineType "controlplane") }} +--- +apiVersion: v1alpha1 +kind: Layer2VIPConfig +name: {{ .Values.floatingIP | quote }} +link: {{ .Values.vipLink }} +{{- end }} {{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }} {{- if $defaultLinkName }} {{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }} @@ -181,11 +194,15 @@ addresses: routes: - gateway: {{ include "talm.discovered.default_gateway" . }} {{- end }} +{{- /* Discovery-derived Layer2VIPConfig: skipped when the operator + has set .Values.vipLink, since the override-path block above + has already emitted the document with the operator's chosen + link. */}} +{{- if and .Values.floatingIP (not .Values.vipLink) (eq .MachineType "controlplane") }} {{- $vipLinkName := $interfaceName }} {{- if $isVlan }} {{- $vipLinkName = $defaultLinkName }} {{- end }} -{{- if and .Values.floatingIP (eq .MachineType "controlplane") }} --- apiVersion: v1alpha1 kind: Layer2VIPConfig @@ -203,11 +220,21 @@ link: {{ $vipLinkName }} {{- (include "talm.discovered.physical_links_info" .) | nindent 4 }} {{- $existingInterfacesConfiguration := include "talm.discovered.existing_interfaces_configuration" . }} {{- $defaultLinkName := include "talm.discovered.default_link_name_by_gateway" . }} - {{- if or $existingInterfacesConfiguration $defaultLinkName }} + {{- /* vipLink override on the legacy schema: legacy Talos has no + Layer2VIPConfig document, so the override is expressed as a + top-level interfaces[] entry that carries only the vip block. + When vipLink == $defaultLinkName the inline vip below already + lands on the right link, so no override entry is needed. */}} + {{- $vipOverride := and .Values.floatingIP .Values.vipLink (eq .MachineType "controlplane") (ne .Values.vipLink $defaultLinkName) }} + {{- /* Suppress the inline (discovery-derived) vip when the operator + has redirected it to a different link; otherwise the VIP would + be pinned twice on different interfaces. */}} + {{- $suppressInlineVip := and .Values.vipLink (ne .Values.vipLink $defaultLinkName) }} + {{- if or $existingInterfacesConfiguration $defaultLinkName $vipOverride }} interfaces: {{- if $existingInterfacesConfiguration }} {{- $existingInterfacesConfiguration | nindent 4 }} - {{- else }} + {{- else if $defaultLinkName }} {{- $isVlan := include "talm.discovered.is_vlan" $defaultLinkName }} {{- $parentLinkName := "" }} {{- if $isVlan }} @@ -229,7 +256,7 @@ link: {{ $vipLinkName }} routes: - network: 0.0.0.0/0 gateway: {{ include "talm.discovered.default_gateway" . }} - {{- if and .Values.floatingIP (eq .MachineType "controlplane") }} + {{- if and .Values.floatingIP (eq .MachineType "controlplane") (not $suppressInlineVip) }} vip: ip: {{ .Values.floatingIP }} {{- end }} @@ -238,12 +265,17 @@ link: {{ $vipLinkName }} routes: - network: 0.0.0.0/0 gateway: {{ include "talm.discovered.default_gateway" . }} - {{- if and .Values.floatingIP (eq .MachineType "controlplane") }} + {{- if and .Values.floatingIP (eq .MachineType "controlplane") (not $suppressInlineVip) }} vip: ip: {{ .Values.floatingIP }} {{- end }} {{- end }} {{- end }} + {{- if $vipOverride }} + - interface: {{ .Values.vipLink }} + vip: + ip: {{ .Values.floatingIP }} + {{- end }} {{- end }} {{- end }} diff --git a/charts/generic/values.yaml b/charts/generic/values.yaml index cd959a00..b90cd12f 100644 --- a/charts/generic/values.yaml +++ b/charts/generic/values.yaml @@ -8,6 +8,36 @@ # Example: endpoint: "https://192.168.0.1:6443" endpoint: "" +# Layer-2 VIP for multi-node setups. When set, the chart emits a +# Layer2VIPConfig document pinning this IP as a floating address on +# the node's primary link. MUST equal the host portion of `endpoint` +# above, otherwise the cluster dials an IP that no node actually +# claims. Blank by default so the shipped value never silently embeds +# a wrong VIP — fill in only if you want a VIP. Single-node clusters +# and external-LB topologies leave it blank. +# Example: floatingIP: 192.168.0.1 +floatingIP: "" + +# Optional override for the link Layer2VIPConfig is pinned to. When +# left empty the chart picks the default-gateway-bearing link the +# node already has (the VLAN sub-interface if one carries the +# default route, otherwise the physical NIC). Set this when the +# target link does not yet exist on the live system at first apply +# — typically a VLAN sub-interface that the same template is about +# to bring up. Without an override the chart would derive the link +# from discovery on the bare-metal NIC, pin the VIP there, and the +# VIP would land on the wrong link once the VLAN comes up. +# When set, Layer2VIPConfig is emitted unconditionally, even on a +# totally fresh node where discovery has not yet resolved a default- +# gateway link. The chart does NOT auto-emit a LinkConfig/VLANConfig +# for the override link; the operator is responsible for ensuring +# the link comes up — typically by adding a LinkConfig or VLANConfig +# document for that link to the per-node body overlay. If no such +# document brings vipLink up, Layer2VIPConfig will dangle on a +# non-existent link and the cluster endpoint will be unreachable. +# Example: vipLink: eth0.4000 +vipLink: "" + podSubnets: - 10.244.0.0/16 serviceSubnets: diff --git a/charts/talm/templates/_helpers.tpl b/charts/talm/templates/_helpers.tpl index b6f4152d..cb742e4d 100644 --- a/charts/talm/templates/_helpers.tpl +++ b/charts/talm/templates/_helpers.tpl @@ -18,10 +18,39 @@ {{- (lookup "machinetype" "" "machine-type").spec }} {{- end }} +{{- /* + talm.discovered.hostname returns the live node's hostname when it looks + user-set, and falls back to a synthetic talos-XXXXX placeholder otherwise. + + Boot-to-talos and Talos's own pre-config state can leave a node with a + transient placeholder hostname — `rescue` (Hetzner rescue OS), `talos` + (Talos default before any config), `localhost` and `localhost.localdomain` + (kernel and initramfs defaults). + Propagating those names into the rendered template would let + `talm template -I` write them back into the node body, where they read + as "applied state" to the user; the next apply replays the placeholder, + discovery returns it, and the loop never resolves to the user's intended + per-node hostname. + + Filtering well-known transient names and falling through to the + address-derived placeholder gives the user a visibly synthetic + `hostname: "talos-abcde"` in the autogenerated body, signalling + "replace me with a real per-node value". + + The match is case-insensitive: Sprig's `has` is exact-match, but some + PXE/DHCP servers hand out title-case forms (`Localhost`) or all-caps + (`TALOS`, `RESCUE`), and the trap-loop is identical for those. Lower- + casing $name before the comparison covers them with no behavior change + for the canonical lowercase forms. +*/ -}} {{- define "talm.discovered.hostname" }} {{- $hostname := lookup "hostname" "" "hostname" }} +{{- $name := "" }} {{- if $hostname }} -{{- $hostname.spec.hostname }} +{{- $name = $hostname.spec.hostname }} +{{- end }} +{{- if and $name (not (has (lower $name) (list "rescue" "talos" "localhost" "localhost.localdomain"))) }} +{{- $name }} {{- else }} {{- printf "talos-%s" (include "talm.discovered.default_addresses_by_gateway" . | sha256sum | trunc 5) }} {{- end }} diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index ba7258d7..c72b6043 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -22,8 +22,10 @@ import ( "strings" "time" + "github.com/cockroachdb/errors" "github.com/cozystack/talm/pkg/engine" "github.com/spf13/cobra" + "google.golang.org/grpc/metadata" "google.golang.org/protobuf/types/known/durationpb" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" @@ -122,9 +124,30 @@ func apply(args []string) error { fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, nodes, GlobalArgs.Endpoints) applyClosure := func(ctx context.Context, c *client.Client, data []byte) error { - // applyTemplatesPerNode rotates ctx via client.WithNode per node, - // so ctx here is already single-target and safe for COSI reads. - preflightCheckTalosVersion(ctx, cosiVersionReader(c), applyCmdFlags.talosVersion, os.Stderr) + // ctx is shaped for ApplyConfiguration on every apply path: + // the auth branch sets `nodes` (plural, one element) via + // openClientPerNodeAuth so apid resolves a single backend + // and helpers.ForEachResource can read the plural key from + // inside template lookups; the insecure branch carries no + // node metadata at all and the maintenance client dials a + // single endpoint per call. + // + // The COSI preflight needs a different context shape: + // Talos's apid director rejects every COSI method whose + // ctx carries the plural "nodes" key, regardless of slice + // length (its COSI guard is unconditional). cosiVersionReader + // swallows errors and returns ok=false on rejection, so the + // preflight would silently no-op on the auth path — defeating + // the whole point of the version-mismatch warning that + // preflightCheckTalosVersion exists to surface. + // cosiPreflightContext rebuilds ctx with the singular "node" + // key so the COSI router accepts the call; ApplyConfiguration + // keeps the original ctx unchanged. + cosiCtx, err := cosiPreflightContext(ctx) + if err != nil { + return err + } + preflightCheckTalosVersion(cosiCtx, cosiVersionReader(c), applyCmdFlags.talosVersion, os.Stderr) resp, err := c.ApplyConfiguration(ctx, &machineapi.ApplyConfigurationRequest{ Data: data, @@ -133,7 +156,7 @@ func apply(args []string) error { TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout), }) if err != nil { - return fmt.Errorf("error applying new configuration: %w", annotateApplyConfigError(err)) + return errors.Wrap(annotateApplyConfigError(err), "applying new configuration") } helpers.PrintApplyResults(resp) return nil @@ -159,12 +182,18 @@ func apply(args []string) error { patches := []string{"@" + configFile} configBundle, machineType, err := engine.FullConfigProcess(ctx, opts, patches) if err != nil { - return fmt.Errorf("full config processing error: %w", err) + return errors.WithHint( + errors.Wrap(err, "full config processing"), + "the chart did not render or could not be combined with the supplied patches; check that the chart in scope and the patches reference fields that exist", + ) } result, err := engine.SerializeConfiguration(configBundle, machineType) if err != nil { - return fmt.Errorf("error serializing configuration: %w", err) + return errors.WithHint( + errors.Wrap(err, "serializing configuration"), + "the merged config bundle could not be encoded back to YAML; this is internal — file an issue if reproducible", + ) } if err := withApplyClient(func(ctx context.Context, c *client.Client) error { @@ -180,9 +209,26 @@ func apply(args []string) error { } fmt.Printf("- talm: file=%s, nodes=%s, endpoints=%s\n", configFile, targetNodes, GlobalArgs.Endpoints) - // COSI does not support multi-node proxying - // (see rotate_ca_handler.go:317). Run preflight per node with - // a single-target context. + // COSI does not support multi-node proxying — apid's + // director rejects every /cosi.* method whose ctx + // carries the plural "nodes" key, regardless of slice + // length. The rule lives in + // internal/app/apid/pkg/director/director.go (search + // for the "one-2-many proxying is not supported" + // guard). Run preflight per node with a single-target + // context. + // + // client.WithNode (singular) here is intentional and unrelated + // to the auth template-rendering apply path's switch from + // WithNode to WithNodes (openClientPerNodeAuth) — preflight + // performs a direct COSI Get against one resource, not a + // helpers.ForEachResource walk that reads the plural "nodes" + // metadata key. apid's COSI router accepts the singular + // "node" key for single-target addressing (and rejects the + // plural "nodes" key for any COSI method, regardless of + // slice length — see cosiPreflightContext for the auth + // path's workaround that has to scope ctx back to "node" + // before calling the same COSI preflight). read := cosiVersionReader(c) for _, node := range targetNodes { preflightCheckTalosVersion(client.WithNode(ctx, node), read, applyCmdFlags.talosVersion, os.Stderr) @@ -195,7 +241,7 @@ func apply(args []string) error { TryModeTimeout: durationpb.New(applyCmdFlags.configTryTimeout), }) if err != nil { - return fmt.Errorf("error applying new configuration: %w", annotateApplyConfigError(err)) + return errors.Wrap(annotateApplyConfigError(err), "applying new configuration") } helpers.PrintApplyResults(resp) @@ -253,10 +299,13 @@ type applyFunc func(ctx context.Context, c *client.Client, data []byte) error // openClientFunc opens a Talos client suitable for a single node and runs // action with it. Authenticated mode reuses one parent client and rotates -// the node via single-target gRPC metadata (client.WithNode); insecure -// (maintenance) mode opens a fresh single-endpoint client per node because -// Talos's maintenance client ignores node metadata in the context and -// round-robins between its configured endpoints. +// the node via single-element-slice gRPC metadata (client.WithNodes with +// one entry — the plural key is what helpers.ForEachResource and apid both +// read, while FailIfMultiNodes still treats len("nodes") == 1 as +// single-target). Insecure (maintenance) mode opens a fresh +// single-endpoint client per node because Talos's maintenance client +// ignores node metadata in the context and round-robins between its +// configured endpoints. type openClientFunc func(node string, action func(ctx context.Context, c *client.Client) error) error // applyTemplatesPerNode runs render → MergeFileAsPatch → apply once per @@ -281,7 +330,10 @@ func applyTemplatesPerNode( apply applyFunc, ) error { if len(nodes) == 0 { - return fmt.Errorf("nodes are not set for the command: please use '--nodes' flag, the node file modeline, or talosconfig context to set the nodes to run the command against") + return errors.WithHint( + errors.New("nodes are not set for the command"), + "set the targets via --nodes, a `# talm: nodes=[...]` modeline at the top of the node file, or the talosconfig context", + ) } // A node-file body (hostname, address, VIP, etc.) is a per-node // pin. Replaying it across multiple targets would stamp the same @@ -294,9 +346,10 @@ func applyTemplatesPerNode( return err } if hasOverlay { - return fmt.Errorf( - "node file %s targets %d nodes (%v) but carries a non-empty per-node body; the same body would be stamped onto every node. Split it into one file per node, or remove the per-node fields if you want the rendered template alone applied to each", - configFile, len(nodes), nodes, + return errors.WithHintf( + errors.Newf("node file %q targets %d nodes (%v) but carries a non-empty per-node body", configFile, len(nodes), nodes), + "split %q into one file per node, or remove the per-node fields if you want the rendered template alone applied to each", + configFile, ) } } @@ -304,7 +357,7 @@ func applyTemplatesPerNode( if err := openClient(node, func(ctx context.Context, c *client.Client) error { return renderMergeAndApply(ctx, c, opts, configFile, render, apply) }); err != nil { - return fmt.Errorf("node %s: %w", node, err) + return errors.Wrapf(err, "node %s", node) } } return nil @@ -346,13 +399,65 @@ func openClientPerNodeMaintenance(fingerprints []string, mkClient maintenanceCli // openClientPerNodeAuth returns an openClientFunc that reuses one // authenticated client (the one withApplyClientBare opened above this -// callback) and rotates the addressed node via client.WithNode on the -// per-iteration context. WithNode (rather than WithNodes) sets the -// "node" metadata key for single-target proxying, which engine.Render's -// FailIfMultiNodes guard treats as one node. +// callback) and rotates the addressed node via client.WithNodes on the +// per-iteration context, passing a single-element slice. The plural key +// is what Talos's helpers.ForEachResource reads inside template lookups +// (cmd/talosctl/pkg/talos/helpers/resources.go) — a singular "node" key +// is invisible to it and the helper falls back to []string{""}, which +// surfaces as `rpc error: code = Internal desc = invalid target ""` +// from inside template `lookup` calls. helpers.FailIfMultiNodes accepts +// len("nodes") <= 1, so a single-element slice still satisfies the +// multi-node guard while making lookups work. func openClientPerNodeAuth(parentCtx context.Context, c *client.Client) openClientFunc { return func(node string, action func(ctx context.Context, c *client.Client) error) error { - return action(client.WithNode(parentCtx, node), c) + return action(client.WithNodes(parentCtx, node), c) + } +} + +// cosiPreflightContext returns a context suitable for a COSI call +// against the same single target the caller's ctx addresses on the +// machine API. Talos's apid director rejects every COSI method whose +// outgoing context carries the plural "nodes" metadata key, regardless +// of how many entries the slice has — the COSI router insists on the +// singular "node" key. The director rule lives in +// internal/app/apid/pkg/director/director.go (search for the +// "/cosi." method-prefix branch); a future maintainer who suspects +// the rule has changed should re-check that file before relaxing this +// helper. +// +// The auth template-rendering apply path uses client.WithNodes +// (plural, single-element slice) so that helpers.ForEachResource and +// the apid backend resolver can both read the plural key from template +// lookups; that ctx is therefore unsuitable for COSI reads as is. +// +// client.WithNode (machinery client/context.go) copies the existing +// outgoing metadata, deletes "nodes", and sets "node" — so calling it +// with the single target is enough; we do not have to mutate metadata +// ourselves. ctx is unchanged for the insecure (maintenance) path +// that carries no node metadata at all. +// +// A multi-element plural slice is a programmer error at this layer: +// applyTemplatesPerNode iterates one node at a time, so an outgoing +// "nodes" of length > 1 means a future caller broke the per-node +// invariant. Surface it as an error instead of silently passing the +// ctx through to a COSI call that apid will reject — the latter is +// the exact silent no-op this helper exists to prevent. +func cosiPreflightContext(ctx context.Context) (context.Context, error) { + md, ok := metadata.FromOutgoingContext(ctx) + if !ok { + return ctx, nil + } + nodes := md.Get("nodes") + switch len(nodes) { + case 0: + return ctx, nil + case 1: + return client.WithNode(ctx, nodes[0]), nil + default: + return nil, errors.WithHint( + errors.Newf("cosiPreflightContext: refusing to scope ctx with %d nodes; expected exactly one", len(nodes)), + "applyTemplatesPerNode iterates one node at a time, so a multi-element plural slice at this point indicates a broken caller", + ) } } @@ -382,11 +487,14 @@ func resolveAuthTemplateNodes(cliNodes []string, c *client.Client) []string { func renderMergeAndApply(ctx context.Context, c *client.Client, opts engine.Options, configFile string, render renderFunc, apply applyFunc) error { rendered, err := render(ctx, c, opts) if err != nil { - return fmt.Errorf("template rendering: %w", err) + return errors.WithHint( + errors.Wrap(err, "template rendering"), + "the chart did not render against the current node's discovery state; verify the templates referenced in the modeline exist and the node is reachable", + ) } merged, err := engine.MergeFileAsPatch(rendered, configFile) if err != nil { - return fmt.Errorf("merging node file as patch: %w", err) + return errors.Wrapf(err, "merging node file %q as patch", configFile) } return apply(ctx, c, merged) } @@ -429,11 +537,17 @@ func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) fu nodes := append([]string(nil), GlobalArgs.Nodes...) if len(nodes) < 1 { if c == nil { - return fmt.Errorf("failed to resolve config context: no client available") + return errors.WithHint( + errors.New("resolving config context: no client available"), + "this code path requires a Talos client; if you reached it from a flow that did not open one, check the call site", + ) } configContext := c.GetConfigContext() if configContext == nil { - return fmt.Errorf("failed to resolve config context") + return errors.WithHint( + errors.New("resolving config context"), + "the talosconfig has no active context; pick one with `talosctl config context ` or pass --talosconfig", + ) } nodes = configContext.Nodes } diff --git a/pkg/commands/apply_test.go b/pkg/commands/apply_test.go index 8006848a..a3bbce18 100644 --- a/pkg/commands/apply_test.go +++ b/pkg/commands/apply_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/cockroachdb/errors" "github.com/cozystack/talm/pkg/engine" "github.com/siderolabs/talos/pkg/machinery/client" "google.golang.org/grpc/metadata" @@ -288,30 +289,43 @@ func TestWrapWithNodeContext_NoNodesNoClient(t *testing.T) { wrapped := wrapWithNodeContext(inner) err := wrapped(context.Background(), nil) if err == nil { - t.Error("expected error when no nodes and no client config context, got nil") + t.Fatal("expected error when no nodes and no client config context, got nil") + } + // The hint chain must keep an operator-actionable explanation, not + // just the bare "no client available" wrap. A future migration that + // drops the hint without replacing it would silently degrade the + // diagnostic. + hints := errors.GetAllHints(err) + if len(hints) == 0 { + t.Errorf("expected at least one hint guiding the operator, got bare error: %v", err) } } // nodesFromOutgoingCtx pulls per-iteration node identity out of gRPC -// outgoing metadata. The Talos client SDK writes single-target metadata to -// the "node" key (client.WithNode) and multi-target metadata to "nodes" -// (client.WithNodes) — checking both keys lets the per-node loop tests -// assert iteration shape regardless of which writer the loop chose. +// outgoing metadata. Production rotates nodes via client.WithNodes with a +// single-element slice (the plural "nodes" key is what +// helpers.ForEachResource and apid both read); the singular "node" key is +// also checked here to keep the helper resilient against fakes or future +// callers that use client.WithNode directly. Tests that pin the metadata +// contract assert against md.Get directly rather than going through this +// helper. func nodesFromOutgoingCtx(t *testing.T, ctx context.Context) []string { t.Helper() md, ok := metadata.FromOutgoingContext(ctx) if !ok { return nil } - if vs := md.Get("node"); len(vs) > 0 { + if vs := md.Get("nodes"); len(vs) > 0 { return vs } - return md.Get("nodes") + return md.Get("node") } // fakeAuthOpenClient mimics openClientPerNodeAuth for tests: shares one -// (nil) parent client across iterations and rotates the node via WithNode -// on a fresh per-iteration context. +// (nil) parent client across iterations and rotates the node via WithNodes +// (single-element plural slice) on a fresh per-iteration context. Mirroring +// production keeps the loop-semantics tests honest about what metadata key +// downstream lookups will actually see. // // The action receives a nil *client.Client. Callers are responsible for // not dereferencing it; tests that exercise client method calls must @@ -321,7 +335,7 @@ func nodesFromOutgoingCtx(t *testing.T, ctx context.Context) []string { // silent test coverage of an untouched code path. func fakeAuthOpenClient(parentCtx context.Context) openClientFunc { return func(node string, action func(ctx context.Context, c *client.Client) error) error { - return action(client.WithNode(parentCtx, node), nil) + return action(client.WithNodes(parentCtx, node), nil) } } @@ -511,13 +525,23 @@ func TestApplyTemplatesPerNode_NoNodesIsAnError(t *testing.T) { if err == nil { t.Fatal("expected an error for empty nodes list, got nil") } - // The error must point the user at the concrete ways to set nodes - // so the message survives a cosmetic reword but catches a regression - // that drops the guidance entirely. + // The error itself names the missing inputs; the cockroachdb/errors + // hint chain points the user at the concrete ways to set them. Both + // must survive a reword — the message catches a future regression + // that drops the topic entirely; the hint catches one that drops + // the actionable guidance. msg := err.Error() - for _, want := range []string{"nodes", "--nodes"} { - if !strings.Contains(msg, want) { - t.Errorf("error message %q does not mention %q", msg, want) + if !strings.Contains(msg, "nodes") { + t.Errorf("error message %q must mention %q (the missing input)", msg, "nodes") + } + hints := errors.GetAllHints(err) + if len(hints) == 0 { + t.Fatalf("expected at least one hint guiding the operator to set nodes, got %v", err) + } + combined := strings.Join(hints, "\n") + for _, want := range []string{"--nodes", "modeline", "talosconfig"} { + if !strings.Contains(combined, want) { + t.Errorf("hint chain %q does not mention %q (operator-actionable resolution path)", combined, want) } } } @@ -652,15 +676,18 @@ func TestOpenClientPerNodeMaintenance_RestoresGlobalNodesOnError(t *testing.T) { } } -// TestApplyTemplatesPerNode_AuthModeUsesSingleNodeMetadataKey pins the -// gRPC metadata key the auth-mode opener writes. WithNode sets "node" -// (single-target proxy); WithNodes sets "nodes" (apid aggregation). -// engine.Render's FailIfMultiNodes guard treats len("nodes") > 1 as the -// multi-node case, so single-target metadata under "node" passes -// trivially. A future refactor that swaps WithNode back to WithNodes -// would slip past nodesFromOutgoingCtx (which reads either key) — this -// assertion catches that regression directly. -func TestApplyTemplatesPerNode_AuthModeUsesSingleNodeMetadataKey(t *testing.T) { +// TestApplyTemplatesPerNode_AuthModeUsesPluralNodesMetadataKey pins the +// gRPC metadata key the auth-mode opener writes. The auth template-rendering +// path drives lookups inside engine.Render through Talos's +// helpers.ForEachResource, which reads only the plural "nodes" metadata key +// (cmd/talosctl/pkg/talos/helpers/resources.go) — when that key is empty the +// helper falls back to []string{""} and issues an RPC with an empty target, +// surfacing as "rpc error: code = Internal desc = invalid target". +// helpers.FailIfMultiNodes accepts len("nodes") <= 1, so a single-element +// plural slice keeps the multi-node guard happy while making lookups work. +// The singular "node" key, in contrast, is invisible to ForEachResource and +// must never be used on the auth path. +func TestApplyTemplatesPerNode_AuthModeUsesPluralNodesMetadataKey(t *testing.T) { dir := t.TempDir() configFile := filepath.Join(dir, "node.yaml") if err := os.WriteFile(configFile, []byte("# talm: nodes=[\"a\"]\n"), 0o644); err != nil { @@ -673,11 +700,11 @@ func TestApplyTemplatesPerNode_AuthModeUsesSingleNodeMetadataKey(t *testing.T) { if !ok { t.Fatal("expected outgoing metadata on per-iteration ctx") } - if got := md.Get("node"); !slices.Equal(got, []string{node}) { - t.Errorf(`metadata key "node" = %v, want [%q]`, got, node) + if got := md.Get("nodes"); !slices.Equal(got, []string{node}) { + t.Errorf(`metadata key "nodes" = %v, want [%q] (single-element plural slice — what helpers.ForEachResource reads)`, got, node) } - if got := md.Get("nodes"); len(got) != 0 { - t.Errorf(`metadata key "nodes" must be unset for single-target apply, got %v`, got) + if got := md.Get("node"); len(got) != 0 { + t.Errorf(`metadata key "node" must be unset on auth apply, got %v`, got) } return []byte("version: v1alpha1\nmachine:\n type: worker\n"), nil } @@ -689,6 +716,75 @@ func TestApplyTemplatesPerNode_AuthModeUsesSingleNodeMetadataKey(t *testing.T) { } } +// TestCosiPreflightContext_StripsPluralAndAttachesSingular pins the +// COSI preflight ctx contract: the auth template-rendering apply path +// puts the target node under the plural "nodes" metadata key (so +// helpers.ForEachResource and apid's machine-API backend resolver can +// read it), but Talos's apid director rejects every COSI method whose +// outgoing context carries the plural key, regardless of slice +// length. cosiPreflightContext rebuilds ctx with the singular "node" +// key so the COSI router accepts the call. Without this, the version +// preflight silently no-ops on the auth path: cosiVersionReader +// swallows errors and returns ok=false on rejection, so the user +// never sees the mismatch warning the preflight exists to surface. +func TestCosiPreflightContext_StripsPluralAndAttachesSingular(t *testing.T) { + const node = "10.0.0.1" + in := client.WithNodes(context.Background(), node) + + out, err := cosiPreflightContext(in) + if err != nil { + t.Fatalf("cosiPreflightContext: %v", err) + } + + md, ok := metadata.FromOutgoingContext(out) + if !ok { + t.Fatal("expected outgoing metadata on preflight ctx") + } + if got := md.Get("nodes"); len(got) != 0 { + t.Errorf(`metadata key "nodes" must be unset on COSI preflight ctx, got %v (apid's COSI router rejects every call carrying it)`, got) + } + if got := md.Get("node"); !slices.Equal(got, []string{node}) { + t.Errorf(`metadata key "node" = %v, want [%q] (apid's COSI router routes by the singular key)`, got, node) + } +} + +// TestCosiPreflightContext_LeavesNoMetadataAlone pins the noop case +// for the insecure (maintenance) apply path, whose ctx carries no +// outgoing metadata at all — the maintenance client dials a single +// endpoint per call and routing-by-key is irrelevant. The helper +// must return ctx unchanged, not synthesize an empty "node" key +// that apid would route to the wrong target. +func TestCosiPreflightContext_LeavesNoMetadataAlone(t *testing.T) { + in := context.Background() + out, err := cosiPreflightContext(in) + if err != nil { + t.Fatalf("cosiPreflightContext: %v", err) + } + + if md, ok := metadata.FromOutgoingContext(out); ok && len(md) > 0 { + t.Errorf("expected no outgoing metadata on preflight ctx for maintenance path, got %v", md) + } +} + +// TestCosiPreflightContext_RejectsMultiNodeCtx pins that a multi- +// element plural slice surfaces as an explicit error rather than a +// silent passthrough. applyTemplatesPerNode iterates one node at a +// time, so a multi-element ctx at this point indicates a broken +// caller; passing it through to the COSI router would silently +// no-op the preflight (apid rejects, cosiVersionReader swallows the +// rejection, version mismatch never surfaces) — the exact symptom +// this helper exists to prevent on the single-node case. +func TestCosiPreflightContext_RejectsMultiNodeCtx(t *testing.T) { + in := client.WithNodes(context.Background(), "a", "b") + _, err := cosiPreflightContext(in) + if err == nil { + t.Fatal("expected error for multi-node outgoing ctx, got nil") + } + if !strings.Contains(err.Error(), "expected exactly one") { + t.Errorf("expected error to mention single-node invariant, got: %v", err) + } +} + // TestTemplateAndApplyDiverge_NodeBodyOverlayLimitation pins a known // trade-off: `talm apply -f node.yaml` overlays the node file body on the // rendered template before sending the result to ApplyConfiguration, but diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index a0580d1b..b329670d 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "maps" "os" "path" @@ -14,6 +15,7 @@ import ( "strings" "unsafe" + "github.com/cockroachdb/errors" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "gopkg.in/yaml.v3" @@ -225,26 +227,632 @@ func SerializeConfiguration(configBundle *bundle.Bundle, machineType machine.Typ func MergeFileAsPatch(rendered []byte, patchFile string) ([]byte, error) { patchBytes, err := os.ReadFile(patchFile) if err != nil { - return nil, fmt.Errorf("reading patch %s: %w", patchFile, err) + return nil, errors.WithHint( + errors.Wrapf(err, "reading patch %q", patchFile), + "verify the path is correct and the file is readable by the user running talm", + ) } if isEffectivelyEmptyYAML(patchBytes) { return rendered, nil } - patch, err := configpatcher.LoadPatch(patchBytes) + cleanedRendered, renderedDirectivePaths, err := stripAllPatchDeleteDirectives(rendered) if err != nil { - return nil, fmt.Errorf("loading patch from %s: %w", patchFile, err) + return nil, errors.WithHint( + errors.Wrap(err, "stripping $patch:delete directives from rendered"), + "the rendered template did not parse as YAML; this points at a chart-helper bug, not a user input issue", + ) } - out, err := configpatcher.Apply(configpatcher.WithBytes(rendered), []configpatcher.Patch{patch}) + cleanedPatch, err := stripPatchDeleteDirectivesAtPaths(patchBytes, renderedDirectivePaths) if err != nil { - return nil, fmt.Errorf("applying patch from %s: %w", patchFile, err) + return nil, errors.WithHintf( + errors.Wrapf(err, "stripping redundant $patch:delete directives from %q", patchFile), + "the node body did not parse as YAML; verify %q is well-formed", + patchFile, + ) + } + prunedBytes, allPruned, err := pruneBodyIdentitiesAgainstRendered(cleanedPatch, cleanedRendered) + if err != nil { + return nil, errors.WithHintf( + errors.Wrapf(err, "pruning identity overlap in %q", patchFile), + "the prune walk failed; the input is likely malformed YAML or has an unexpected document shape; inspect %q", + patchFile, + ) + } + if allPruned { + return cleanedRendered, nil + } + patch, err := configpatcher.LoadPatch(prunedBytes) + if err != nil { + return nil, errors.WithHint( + errors.Wrapf(err, "loading patch from %q", patchFile), + "the node body must be a Talos config (full or partial), a JSON Patch list, or a YAML patch list — see https://www.talos.dev/latest/talos-guides/configuration/patching/", + ) + } + out, err := configpatcher.Apply(configpatcher.WithBytes(cleanedRendered), []configpatcher.Patch{patch}) + if err != nil { + return nil, errors.WithHintf( + errors.Wrapf(err, "applying patch from %q", patchFile), + "the patch references a path the rendered template does not contain; check the output of: talm template -f %q", + patchFile, + ) } merged, err := out.Bytes() if err != nil { - return nil, fmt.Errorf("encoding merged config from %s: %w", patchFile, err) + return nil, errors.WithHintf( + errors.Wrapf(err, "encoding merged config from %q", patchFile), + "configpatcher.Apply succeeded but the result could not be serialised back to YAML; this is internal — file an issue if reproducible", + ) } return merged, nil } +// stripAllPatchDeleteDirectives walks every YAML document in `data` and +// removes every `: {$patch: delete}` pair from mapping nodes, +// returning the cleaned bytes and the identity-prefixed paths of every +// removed pair. +// +// configpatcher.Apply loads the merge target via configloader.NewFromBytes +// WITHOUT WithAllowPatchDelete (apply.go: configOrBytes.Config), so the +// directive-aware decoding pass that would normally extract these pairs +// (configloader/internal/decoder/delete.go AppendDeletesTo) is never +// invoked for the target tree. A directive nested in the target therefore +// reaches the strict v1alpha1.Config decoder unprocessed: when the parent +// field's declared type is a scalar map (e.g. `machine.nodeLabels` is +// map[string]string), the directive's `{$patch: delete}` map-shaped value +// trips the decoder with `cannot construct !!map into string`. Talos's +// ApplyConfiguration RPC has the same constraint on the receiving side, +// so we cannot just forward the directive untouched either. +// +// Stripping the (key, directive) pair from the target preserves its +// observable effect — the named key is absent from the merged config that +// talm sends to Talos — without inventing new merge semantics. +// +// The function returns the cleaned bytes and the identity-prefixed +// paths of every removed pair. The caller uses those paths via +// stripPatchDeleteDirectivesAtPaths to scrub matching entries from the +// patch body, leaving any user-intent directive at a path the chart did +// not own. +// +// Multi-document inputs are handled per-document; the document identity +// tuple (apiVersion+kind+name, or the legacy-root sentinel) is embedded +// in each path so a body that re-orders typed documents relative to +// rendered still pairs the directives by content rather than by +// positional accident. +func stripAllPatchDeleteDirectives(data []byte) ([]byte, []string, error) { + docs, err := decodeAllYAMLDocuments(data) + if err != nil { + return nil, nil, err + } + if len(docs) == 0 { + return data, nil, nil + } + var stripped []string + for _, doc := range docs { + stripped = append(stripped, removePatchDeleteFromNode(doc, "/"+documentIdentityFromNode(doc), nil)...) + } + if len(stripped) == 0 { + return data, nil, nil + } + out, err := encodeAllYAMLDocuments(docs) + if err != nil { + return nil, nil, err + } + return out, stripped, nil +} + +// stripPatchDeleteDirectivesAtPaths walks every YAML document in `data` +// and removes only those `: {$patch: delete}` pairs whose +// identity-prefixed path is present in `paths`. Directives at any other +// path are left intact so configpatcher.LoadPatch can honour them as +// user-intent (load.go: NewFromBytes with allowPatchDelete=true → +// AppendDeletesTo extracts them and applies the deletion as a Selector +// during the merge). +// +// `paths` is the slice returned by stripAllPatchDeleteDirectives on the +// rendered side — i.e. the addresses of every chart-emitted directive, +// each prefixed by its document's identity tuple. Pairing by identity +// rather than by document index lets a body that re-orders typed +// documents relative to rendered still strip the chart-side directives +// from the matching body documents (and leave user-intent directives at +// the same nominal path on a different doc untouched). +// +// When `paths` is empty (rendered carried no directives), nothing is +// stripped from the patch body. +func stripPatchDeleteDirectivesAtPaths(data []byte, paths []string) ([]byte, error) { + if len(paths) == 0 { + return data, nil + } + docs, err := decodeAllYAMLDocuments(data) + if err != nil { + return nil, err + } + if len(docs) == 0 { + return data, nil + } + pruneSet := make(map[string]struct{}, len(paths)) + for _, p := range paths { + pruneSet[p] = struct{}{} + } + stripped := 0 + for _, doc := range docs { + stripped += len(removePatchDeleteFromNode(doc, "/"+documentIdentityFromNode(doc), pruneSet)) + } + if stripped == 0 { + return data, nil + } + return encodeAllYAMLDocuments(docs) +} + +func decodeAllYAMLDocuments(data []byte) ([]*yaml.Node, error) { + dec := yaml.NewDecoder(bytes.NewReader(data)) + var docs []*yaml.Node + for { + var doc yaml.Node + err := dec.Decode(&doc) + if err != nil { + if errors.Is(err, io.EOF) { + break + } + return nil, errors.WithHint( + errors.Wrap(err, "decoding YAML before stripping $patch:delete directives"), + "the input is malformed YAML; check for unbalanced quotes or stray indentation in the rendered template or node body", + ) + } + docs = append(docs, &doc) + } + return docs, nil +} + +func encodeAllYAMLDocuments(docs []*yaml.Node) ([]byte, error) { + var buf bytes.Buffer + enc := yaml.NewEncoder(&buf) + enc.SetIndent(2) + for _, doc := range docs { + if err := enc.Encode(doc); err != nil { + return nil, errors.WithHint( + errors.Wrap(err, "re-encoding YAML after stripping $patch:delete directives"), + "the YAML.v3 encoder rejected the post-strip tree; file an issue with the rendered+body that triggered it", + ) + } + } + if err := enc.Close(); err != nil { + return nil, errors.WithHint( + errors.Wrap(err, "closing YAML encoder after stripping $patch:delete directives"), + "the YAML.v3 encoder failed to flush; file an issue with the rendered+body that triggered it", + ) + } + return buf.Bytes(), nil +} + +// removePatchDeleteFromNode recursively walks `node` and removes every +// (key, value) pair where value is the directive `{$patch: delete}`. +// When `prunePaths` is nil every directive is removed (rendered-side +// pass). When non-nil only directives at JSON-Pointer paths in the set +// are removed; others survive for downstream configpatcher.LoadPatch. +// +// Returns the JSON-Pointer paths of every removed pair. +func removePatchDeleteFromNode(node *yaml.Node, parentPath string, prunePaths map[string]struct{}) []string { + if node == nil { + return nil + } + var removed []string + switch node.Kind { + case yaml.DocumentNode: + for _, child := range node.Content { + removed = append(removed, removePatchDeleteFromNode(child, parentPath, prunePaths)...) + } + case yaml.MappingNode: + kept := make([]*yaml.Node, 0, len(node.Content)) + for i := 0; i+1 < len(node.Content); i += 2 { + keyNode := node.Content[i] + valueNode := node.Content[i+1] + childPath := parentPath + "/" + jsonPointerEscape(keyNode.Value) + if isPatchDeleteDirective(valueNode) { + if prunePaths == nil { + removed = append(removed, childPath) + continue + } + if _, prune := prunePaths[childPath]; prune { + removed = append(removed, childPath) + continue + } + } + removed = append(removed, removePatchDeleteFromNode(valueNode, childPath, prunePaths)...) + kept = append(kept, keyNode, valueNode) + } + node.Content = kept + case yaml.SequenceNode: + for i, child := range node.Content { + removed = append(removed, removePatchDeleteFromNode(child, fmt.Sprintf("%s/%d", parentPath, i), prunePaths)...) + } + } + return removed +} + +// jsonPointerEscape encodes a YAML mapping key as a JSON Pointer segment +// per RFC 6901 (~ → ~0, / → ~1). The encoded form is what JSON Patch +// implementations expect, but here we use it only to give every directive +// a unique, comparable identity across the rendered- and body-side +// strips. +func jsonPointerEscape(s string) string { + s = strings.ReplaceAll(s, "~", "~0") + s = strings.ReplaceAll(s, "/", "~1") + return s +} + +// isPatchDeleteDirective reports whether `n` is exactly the YAML mapping +// `{$patch: delete}` — a single key/value pair with scalar key "$patch" +// and scalar value "delete". +func isPatchDeleteDirective(n *yaml.Node) bool { + if n == nil || n.Kind != yaml.MappingNode { + return false + } + if len(n.Content) != 2 { + return false + } + k, v := n.Content[0], n.Content[1] + return k.Kind == yaml.ScalarNode && k.Value == "$patch" && + v.Kind == yaml.ScalarNode && v.Value == "delete" +} + +// pruneBodyIdentitiesAgainstRendered removes from body every key whose value +// is deep-equal to the same key in rendered. Talos's strategic-merge appends +// to primitive arrays rather than treating them as a set, so a body that +// re-states an unchanged primitive list (the dominant case after +// `talm template -I` writes the rendered template back into the node file as +// the body) would otherwise duplicate every entry on each apply round-trip: +// every certSAN, every nameserver, every podSubnet doubles per round-trip. +// +// Returns (prunedBytes, allPruned, err). When allPruned is true the body +// carried no semantic change beyond the rendered template and the caller +// should short-circuit to rendered. +// +// Multi-document inputs (Talos v1.12+ output format) are pruned per-document: +// each body document is matched against a rendered document by its identity +// tuple (apiVersion + kind + name for typed documents; the empty tuple for +// the legacy v1alpha1 root config), then pruneIdenticalKeys runs on the +// pair. Body documents with no matching rendered document survive untouched +// — they are user additions that the merge needs to see. +// +// Re-encoding goes through a fresh yaml.Encoder per kept document. That +// loses the original key order and any comments (including the +// modeline) — configpatcher.LoadPatch reads structure, not comments, so +// this is fine for the apply path; do not feed the output back into a +// human-facing rendering surface. +func pruneBodyIdentitiesAgainstRendered(body, rendered []byte) ([]byte, bool, error) { + bodyDocs, bodyAllMaps, err := decodeAsMaps(body) + if err != nil { + return nil, false, errors.WithHint( + errors.Wrap(err, "parsing body"), + "the node body did not parse as YAML; check the file referenced by the modeline for unbalanced quotes or stray indentation", + ) + } + if !bodyAllMaps { + // JSON Patch / YAML patch-list bodies: top-level is a sequence, + // not a mapping, so the identity-prune step has no map keys to + // compare. Pass through untouched and let configpatcher.LoadPatch + // route it through the JSON Patch path (load.go: jsonpatch.DecodePatch). + return body, false, nil + } + renderedDocs, _, err := decodeAsMaps(rendered) + if err != nil { + // Rendered should always parse — engine.Render produced it from + // chart templates this binary owns. Surface the parse error + // directly: continuing on to LoadPatch with the original body + // would mask the real failure as a downstream configpatcher + // error against malformed bytes. + return nil, false, errors.WithHint( + errors.Wrap(err, "parsing rendered template for identity prune"), + "the rendered template did not parse as YAML; this points at a chart-helper bug, not a user input issue", + ) + } + if len(bodyDocs) == 0 { + return nil, true, nil + } + + renderedByID := make(map[string]map[string]any, len(renderedDocs)) + for _, doc := range renderedDocs { + renderedByID[documentIdentity(doc)] = doc + } + + keptDocs := make([]map[string]any, 0, len(bodyDocs)) + for _, bdoc := range bodyDocs { + id := documentIdentity(bdoc) + if rdoc, ok := renderedByID[id]; ok { + pruneIdenticalKeys(bdoc, rdoc) + // Typed multi-doc bodies use apiVersion/kind/name as the + // identity tuple configpatcher.LoadPatch routes on. Those + // keys are byte-equal between body and rendered when the + // user does a partial edit, so the prune deletes them and + // the surviving body looks like a bare {field: value} map + // that LoadPatch rejects with "missing kind". Re-attach + // the identity tuple from rendered when the body kept any + // override fields. The legacy v1alpha1 root carries no + // apiVersion/kind/name (its top-level identity is the + // version field, which is at the same nesting level as the + // machine/cluster blocks), so this only fires for the typed + // multi-doc shape. + if id != legacyRootIdentity && len(bdoc) > 0 { + reattachIdentityKeys(bdoc, rdoc) + } + } + if len(bdoc) > 0 { + keptDocs = append(keptDocs, bdoc) + } + } + if len(keptDocs) == 0 { + return nil, true, nil + } + + var buf bytes.Buffer + enc := yaml.NewEncoder(&buf) + enc.SetIndent(2) + for _, doc := range keptDocs { + if err := enc.Encode(doc); err != nil { + return nil, false, errors.WithHint( + errors.Wrap(err, "re-encoding pruned body"), + "the YAML.v3 encoder rejected the post-prune body; file an issue with the rendered+body that triggered it", + ) + } + } + if err := enc.Close(); err != nil { + return nil, false, errors.WithHint( + errors.Wrap(err, "closing encoder for pruned body"), + "the YAML.v3 encoder failed to flush; file an issue with the rendered+body that triggered it", + ) + } + return buf.Bytes(), false, nil +} + +// pruneIdenticalKeys recursively deletes every body[k] that deep-equals +// rendered[k] (mutating `body` in place — the caller still holds the +// reference). When a body sub-map becomes empty after pruning, the whole +// entry is removed so the encoded output stays minimal. For primitive +// arrays the function additionally subtracts every element already +// present in rendered's array, replacing body's slice with the user-add +// difference (when the diff is empty the entry is deleted) — this +// neutralises Talos's strategic-merge primitive-array append behaviour +// for both byte-identical and partial-edit cases — without it, every +// `talm template -I` round-trip would double every certSAN, nameserver, +// and podSubnet entry on the next apply. +// +// Object arrays (arrays whose elements are maps) are intentionally left +// alone: configpatcher's StrategicMerge handles them via patchMergeKey +// semantics that primitive subtraction would silently corrupt. +func pruneIdenticalKeys(body, rendered map[string]any) { + for k, bodyV := range body { + renderedV, exists := rendered[k] + if !exists { + continue + } + if reflect.DeepEqual(bodyV, renderedV) { + delete(body, k) + continue + } + if bodySub, ok := bodyV.(map[string]any); ok { + if renderedSub, ok2 := renderedV.(map[string]any); ok2 { + // Only delete when the recursive prune actually + // removed every child entry. If bodySub was already + // empty before the recursion, leave it: a user-intent + // empty map (e.g. `key: {}` to clear a section) must + // reach the merge as-is, not get silently dropped so + // rendered's populated value wins. + before := len(bodySub) + pruneIdenticalKeys(bodySub, renderedSub) + if before > 0 && len(bodySub) == 0 { + delete(body, k) + } + continue + } + } + if bodySlice, ok := bodyV.([]any); ok { + if renderedSlice, ok2 := renderedV.([]any); ok2 && isPrimitiveSlice(bodySlice) && isPrimitiveSlice(renderedSlice) { + diff := primitiveSliceDifference(bodySlice, renderedSlice) + if len(diff) == 0 { + delete(body, k) + } else { + body[k] = diff + } + } + } + } +} + +// isPrimitiveSlice reports whether every element of `s` is a YAML scalar +// (string, number, bool, nil) — i.e. a value Talos's strategic-merge +// would append rather than merge by key. Object arrays return false and +// are left to the configpatcher's patchMergeKey handling. Narrow integer +// widths are listed defensively: yaml.v3 returns `int` and `float64` for +// numbers in practice, but if a future caller hands us a body decoded +// by a different unmarshaller, an `[]int8` (or similar) would otherwise +// fall through to the default branch and skip the dedup pass. +func isPrimitiveSlice(s []any) bool { + for _, e := range s { + switch e.(type) { + case nil, string, bool, + int, int8, int16, int32, int64, + uint, uint8, uint16, uint32, uint64, + float32, float64: + continue + default: + return false + } + } + return true +} + +// primitiveSliceDifference returns body \ rendered — every element of +// body whose deep-equal counterpart is not in rendered. Order from +// body is preserved on the elements that survive. Used to strip out +// rendered-side prefix entries from a partial-edit body so the +// strategic-merge append step does not duplicate them. +// +// Trade-off: this loses any user-side reordering of primitive arrays. +// If body is `[b, a]` and rendered is `[a, b]`, both elements match +// and the difference is `[]`, so the caller deletes the body's value +// and rendered's `[a, b]` order survives untouched. Strategic-merge's +// own primitive-array semantics already cannot replace, only append, +// so a body cannot impose a new order on a rendered list anyway — +// even without this prune, the merge result would have been +// `[a, b, b, a]` (rendered prepended, body appended in body order). +// The dedup makes the silent-undo more visible because it now reaches +// the partial-edit case, but the underlying constraint is upstream. +// Callers that need ordered overrides have to model the field as a +// non-primitive merge target (e.g. patchMergeKey on an object array) +// or reach for a JSON Patch body, which the engine forwards through +// LoadPatch unchanged. +func primitiveSliceDifference(body, rendered []any) []any { + out := make([]any, 0, len(body)) + for _, b := range body { + found := false + for _, r := range rendered { + if reflect.DeepEqual(b, r) { + found = true + break + } + } + if !found { + out = append(out, b) + } + } + return out +} + +// decodeAsMaps parses every YAML document in `data` into a generic map. +// Returns the decoded documents, a flag indicating whether every +// document unmarshalled into map[string]any, and any error. +// +// allMaps == false signals that at least one document had a non-mapping +// top level — typically a JSON Patch list or YAML patch-list body, both +// of which are sequence-shaped at the root. The caller must NOT consume +// `docs` in that case; the right thing to do is bypass identity-keyed +// pruning entirely and forward the original bytes to configpatcher.LoadPatch. +// +// Returns (nil, true, nil) for empty input — vacuously "all maps". +func decodeAsMaps(data []byte) ([]map[string]any, bool, error) { + if len(data) == 0 { + return nil, true, nil + } + dec := yaml.NewDecoder(bytes.NewReader(data)) + var docs []map[string]any + allMaps := true + for { + var doc any + if err := dec.Decode(&doc); err != nil { + if errors.Is(err, io.EOF) { + break + } + return nil, false, err + } + if doc == nil { + continue + } + asMap, ok := doc.(map[string]any) + if !ok { + allMaps = false + continue + } + docs = append(docs, asMap) + } + return docs, allMaps, nil +} + +// legacyRootIdentity is the sentinel documentIdentity returns for the +// legacy v1alpha1 root config (no apiVersion/kind/name fields). The +// per-document identity prune skips identity-key reattachment for this +// shape because the legacy root carries no identity tuple to begin +// with — its only top-level identifier is `version`, which is at the +// same nesting level as the machine/cluster blocks rather than peer +// to a routable apiVersion/kind/name. +const legacyRootIdentity = "__legacy_root__" + +// documentIdentity returns a stable string identifying a Talos config +// document. The legacy v1alpha1 root config (a single document with +// `version: v1alpha1` at the top and no apiVersion/kind/name fields) +// collapses to a fixed sentinel so that legacy bodies match legacy +// renders. Typed documents (HostnameConfig, LinkConfig, RegistryMirrorConfig, +// Layer2VIPConfig, …) identify by `apiVersion/kind` plus `/name` when a +// name is present. +// +// The shape mirrors configpatcher's StrategicMerge documentID +// (machinery/config/configpatcher/strategic.go: documentID), with one +// deliberate difference: upstream omits the trailing `/name` segment +// when the document does not implement NamedDocument; this function +// follows the same rule via the empty-name shortcut so a typed doc +// without a `name` field collides with itself across body and rendered +// streams instead of with every other unnamed doc of the same kind. +func documentIdentity(doc map[string]any) string { + apiVersion, _ := doc["apiVersion"].(string) + kind, _ := doc["kind"].(string) + if apiVersion == "" && kind == "" { + return legacyRootIdentity + } + id := apiVersion + "/" + kind + if name, _ := doc["name"].(string); name != "" { + id += "/" + name + } + return id +} + +// documentIdentityFromNode returns the same identity tuple as +// documentIdentity, but operates on a *yaml.Node instead of a decoded +// map[string]any. The strip/prune-by-path passes work on yaml.Node +// trees (so they can preserve comments and key order on round-trip), +// so they need an identity helper that does not require a parallel +// map decode. The output is byte-for-byte equal to documentIdentity's +// output for the same logical document. +func documentIdentityFromNode(doc *yaml.Node) string { + root := doc + if root != nil && root.Kind == yaml.DocumentNode && len(root.Content) > 0 { + root = root.Content[0] + } + if root == nil || root.Kind != yaml.MappingNode { + return legacyRootIdentity + } + var apiVersion, kind, name string + for i := 0; i+1 < len(root.Content); i += 2 { + k := root.Content[i] + v := root.Content[i+1] + if k.Kind != yaml.ScalarNode || v.Kind != yaml.ScalarNode { + continue + } + switch k.Value { + case "apiVersion": + apiVersion = v.Value + case "kind": + kind = v.Value + case "name": + name = v.Value + } + } + if apiVersion == "" && kind == "" { + return legacyRootIdentity + } + id := apiVersion + "/" + kind + if name != "" { + id += "/" + name + } + return id +} + +// reattachIdentityKeys copies apiVersion / kind / name from rendered +// onto body when body's prune dropped them. Only intended for typed +// multi-doc bodies — the legacy v1alpha1 root carries no identity +// tuple to reattach. Each key is reattached only when missing, so a +// body that explicitly overrides one (rare, but possible — e.g. an +// operator pinning a different name on a Layer2VIPConfig) keeps its +// override. +func reattachIdentityKeys(body, rendered map[string]any) { + for _, k := range []string{"apiVersion", "kind", "name"} { + if _, has := body[k]; has { + continue + } + if v, ok := rendered[k]; ok { + body[k] = v + } + } +} + // NodeFileHasOverlay reports whether a node file carries a non-empty // per-node body below its modeline. The apply path uses this to reject // multi-node node files that would otherwise stamp the same pinned diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index caa9016f..82a09ae1 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -14,7 +14,16 @@ package engine -import "testing" +import ( + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + + "github.com/cockroachdb/errors" +) func TestIsTalosConfigPatch(t *testing.T) { tests := []struct { @@ -234,3 +243,460 @@ func TestNormalizeTemplatePath(t *testing.T) { }) } } + +// TestIsPrimitiveSlice_NarrowIntegerWidths pins that all Go integer +// widths plus floats/bools/strings/nil count as primitive. yaml.v3 +// returns `int` and `float64` in practice, but the dedup pass is +// reused by callers that may decode bodies with other unmarshallers, +// so a narrow-width slice (e.g. []int8 from a manually built map) +// must not silently fall through to the default branch and skip the +// dedup. +func TestIsPrimitiveSlice_NarrowIntegerWidths(t *testing.T) { + tests := []struct { + name string + input []any + want bool + }{ + {"int8", []any{int8(1), int8(2)}, true}, + {"int16", []any{int16(1), int16(2)}, true}, + {"int32", []any{int32(1), int32(2)}, true}, + {"int64", []any{int64(1), int64(2)}, true}, + {"uint8", []any{uint8(1), uint8(2)}, true}, + {"uint16", []any{uint16(1), uint16(2)}, true}, + {"uint32", []any{uint32(1), uint32(2)}, true}, + {"uint64", []any{uint64(1), uint64(2)}, true}, + {"float32", []any{float32(1.0), float32(2.0)}, true}, + {"float64", []any{1.0, 2.0}, true}, + {"int default", []any{1, 2}, true}, + {"strings", []any{"a", "b"}, true}, + {"bools", []any{true, false}, true}, + {"nils", []any{nil, nil}, true}, + {"mixed scalar", []any{int8(1), "two", true, nil}, true}, + {"map element", []any{map[string]any{"k": "v"}}, false}, + {"slice element", []any{[]any{1, 2}}, false}, + {"struct element", []any{struct{ A int }{A: 1}}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isPrimitiveSlice(tt.input); got != tt.want { + t.Errorf("isPrimitiveSlice(%v) = %v, want %v", tt.input, got, tt.want) + } + }) + } +} + +// TestPruneIdenticalKeys_DedupsNarrowIntSlice pins that the dedup +// pass that callers rely on actually fires for a narrow-width +// primitive slice — not just that isPrimitiveSlice returns true in +// isolation. +func TestPruneIdenticalKeys_DedupsNarrowIntSlice(t *testing.T) { + body := map[string]any{ + "k": []any{int8(1), int8(2)}, + } + rendered := map[string]any{ + "k": []any{int8(1), int8(2)}, + } + + pruneIdenticalKeys(body, rendered) + + if _, ok := body["k"]; ok { + t.Errorf("expected key %q to be deleted from body after identical narrow-int dedup, got %v", "k", body) + } +} + +// TestPruneIdenticalKeys_PreservesUserIntentEmptyMap pins that a +// body that explicitly sets a section to an empty map (typically to +// clear it) is not silently dropped by the dedup pass. Without this +// guard the recursive prune sees an already-empty bodySub, finds +// nothing to remove, then evaluates len(bodySub) == 0 and deletes +// the parent key — letting rendered's populated value survive +// untouched. The user's clear-this-section intent never reaches +// configpatcher.LoadPatch. +func TestPruneIdenticalKeys_PreservesUserIntentEmptyMap(t *testing.T) { + body := map[string]any{ + "a": map[string]any{ + "b": map[string]any{}, + }, + } + rendered := map[string]any{ + "a": map[string]any{ + "b": map[string]any{ + "c": "value", + }, + }, + } + + pruneIdenticalKeys(body, rendered) + + a, ok := body["a"].(map[string]any) + if !ok { + t.Fatalf("expected body[a] to be retained as a map, got %#v", body["a"]) + } + bv, ok := a["b"].(map[string]any) + if !ok { + t.Fatalf(`expected body["a"]["b"] to be retained as a map (user-intent empty override), got %#v`, a["b"]) + } + if len(bv) != 0 { + t.Errorf(`expected body["a"]["b"] to be empty (user-intent), got %#v`, bv) + } +} + +// TestPruneIdenticalKeys_RemovesIdenticalNestedMap pins the +// complementary case to TestPruneIdenticalKeys_PreservesUserIntentEmptyMap: +// when the body's nested map is fully covered by rendered (every +// child entry is deep-equal to rendered's counterpart), the dedup +// pass MUST collapse it. Without this, byte-identical bodies would +// leave behind a wrapping map that strategic-merge then re-stamps, +// re-introducing the duplicate-primitive-array-entries-per-round-trip +// regression this prune is the entire reason for existing. +func TestPruneIdenticalKeys_RemovesIdenticalNestedMap(t *testing.T) { + body := map[string]any{ + "a": map[string]any{ + "b": "value", + }, + } + rendered := map[string]any{ + "a": map[string]any{ + "b": "value", + }, + } + + pruneIdenticalKeys(body, rendered) + + if _, ok := body["a"]; ok { + t.Errorf(`expected body["a"] to be removed (every child deep-equal to rendered), got %#v`, body["a"]) + } +} + +// TestDocumentIdentityHelpersAgree pins that documentIdentity (which +// works on map[string]any) and documentIdentityFromNode (which works +// on *yaml.Node) produce byte-equal output for the same logical +// document. The strip-by-path-then-prune pipeline depends on this: +// the rendered scan emits paths prefixed by documentIdentityFromNode, +// the body strip routes by the same prefix, and the multi-doc prune +// keys rendered docs by documentIdentity. A drift between the two +// helpers would silently break strip-pairing on identity-tuple- +// matched docs, leaving directives behind in production while every +// integration test still passed (the bypass is a no-op on bodies +// the helpers happen to disagree on, not a hard failure). +func TestDocumentIdentityHelpersAgree(t *testing.T) { + tests := []struct { + name string + yaml string + }{ + { + name: "legacy v1alpha1 root", + yaml: "version: v1alpha1\nmachine:\n type: worker\n", + }, + { + name: "typed doc with name", + yaml: "apiVersion: v1alpha1\nkind: Layer2VIPConfig\nname: 192.168.0.1\nlink: eth0\n", + }, + { + name: "typed doc without name", + yaml: "apiVersion: v1alpha1\nkind: HostnameConfig\nhostname: cozy-01\n", + }, + { + name: "anonymous map", + yaml: "machine:\n type: worker\n", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + docs, err := decodeAllYAMLDocuments([]byte(tc.yaml)) + if err != nil { + t.Fatalf("decodeAllYAMLDocuments: %v", err) + } + if len(docs) != 1 { + t.Fatalf("expected 1 document, got %d", len(docs)) + } + fromNode := documentIdentityFromNode(docs[0]) + + mapDocs, _, err := decodeAsMaps([]byte(tc.yaml)) + if err != nil { + t.Fatalf("decodeAsMaps: %v", err) + } + if len(mapDocs) != 1 { + t.Fatalf("expected 1 map document, got %d", len(mapDocs)) + } + fromMap := documentIdentity(mapDocs[0]) + + if fromNode != fromMap { + t.Errorf("identity helpers disagree:\n documentIdentityFromNode = %q\n documentIdentity = %q\n yaml: %s", + fromNode, fromMap, tc.yaml) + } + }) + } +} + +// TestNoWorkflowLeakageInRepoSource walks every committed text file +// in the module and fails on phrases that describe the iteration +// process that produced the change rather than the change itself. +// Committed content must read as if the change was right the first +// time, with no "this PR adds…", "address review pass N", +// "branch-review caught…" provenance: a reader six months from now +// has no access to the chat session, the PR thread, or the planning +// doc, and process-leaking comments age into noise. +// +// Scope is the whole repo, not just this package. A leak in +// pkg/commands/, charts/, README.md, or anywhere else under the +// module root is just as much of a problem as one inside pkg/engine. +// The walk skips this test file (which has to spell the banlist +// out), the .git directory, and vendored/build artefacts. +func TestNoWorkflowLeakageInRepoSource(t *testing.T) { + moduleRoot, err := filepath.Abs(filepath.Join("..", "..")) + if err != nil { + t.Fatalf("resolve module root: %v", err) + } + selfPath, err := filepath.Abs("engine_test.go") + if err != nil { + t.Fatalf("resolve self path: %v", err) + } + + banned := []string{ + "branch-review", + "branch review", + "this PR", + "this branch fixes", + "this branch adds", + "this branch introduces", + "review pass", + "review fix", + "review-fix", + "address review", + "in-flight rebase", + "rebase notes", + } + scanExt := map[string]bool{ + ".go": true, + ".tpl": true, + ".yaml": true, + ".yml": true, + ".md": true, + } + skipDirs := map[string]bool{ + ".git": true, + "vendor": true, + "node_modules": true, + ".claude": true, // worktrees, plans, memory — not committed source + } + if err := filepath.WalkDir(moduleRoot, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if skipDirs[d.Name()] { + return filepath.SkipDir + } + return nil + } + if path == selfPath { + return nil + } + if !scanExt[filepath.Ext(path)] { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + src := string(data) + rel, _ := filepath.Rel(moduleRoot, path) + for _, phrase := range banned { + if strings.Contains(src, phrase) { + t.Errorf("workflow-leaky phrase %q found in %s; committed content must read as self-contained, with no references to the iteration process that produced it", phrase, rel) + } + } + return nil + }); err != nil { + t.Fatalf("walk module root: %v", err) + } +} + +// TestNoDanglingSubtestReferencesInSource walks every test file in +// the package and checks that every parent-test plus slash plus +// subtest-slug citation in source comments resolves to a real +// subtest. Citations that lose their referent during refactoring +// turn into actively misleading documentation: a future maintainer +// chasing the citation reads the test file looking for an upstream +// guardrail that does not exist. This guard catches that class of +// drift on the next change. +// +// Slug matching mirrors Go's testing rewrite: spaces become +// underscores, other special characters pass through. We accept +// either an exact match or a prefix match (subtest names commonly +// get truncated when cited in prose). +func TestNoDanglingSubtestReferencesInSource(t *testing.T) { + // Restricted to *_test.go: prose citations of the parent/subtest + // shape are a test-file convention. Production code may legitimately + // embed substrings like `Foo/bar` that match the reference regex + // but have nothing to do with subtests (HTTP routes, file paths, + // log entries), and we do not want to false-positive on them. + files, err := filepath.Glob("*_test.go") + if err != nil { + t.Fatalf("glob: %v", err) + } + + // Collect every slugified subtest name seen in any t.Run literal. + subtestRe := regexp.MustCompile(`t\.Run\("([^"\\]+)"`) + subtests := map[string]struct{}{} + for _, f := range files { + data, err := os.ReadFile(f) + if err != nil { + t.Fatalf("read %s: %v", f, err) + } + for _, m := range subtestRe.FindAllSubmatch(data, -1) { + slug := strings.ReplaceAll(string(m[1]), " ", "_") + subtests[slug] = struct{}{} + } + } + + // Find every parent-plus-subtest-slug citation in any test file. + // Conservative pattern: Test prefix followed by an identifier, + // then slash, then a slug of non-whitespace/non-quote chars. + refRe := regexp.MustCompile(`Test[A-Z][A-Za-z0-9_]+/[A-Za-z0-9_$.:\-]+`) + for _, f := range files { + data, err := os.ReadFile(f) + if err != nil { + t.Fatalf("read %s: %v", f, err) + } + for _, m := range refRe.FindAllSubmatch(data, -1) { + ref := string(m[0]) + parts := strings.SplitN(ref, "/", 2) + if len(parts) != 2 { + continue + } + slug := parts[1] + if _, ok := subtests[slug]; ok { + continue + } + matched := false + for known := range subtests { + if strings.HasPrefix(known, slug) { + matched = true + break + } + } + if !matched { + t.Errorf("dangling subtest reference in %s: %q has no matching t.Run subtest in this package", f, ref) + } + } + } +} + +// TestMergeFileAsPatch_QuotesPatchFileInHints pins that user- +// controlled file paths cannot inject shell metacharacters into the +// suggested-command hint. A `talm template -f $(rm -rf /).yaml` +// path that landed in a copy-pastable hint without quoting would +// turn the diagnostic into a footgun. The fix is `%q` quoting on +// every path interpolation; this test catches the next time a hint +// drops back to bare `%s` or string concatenation. +func TestMergeFileAsPatch_QuotesPatchFileInHints(t *testing.T) { + rendered := []byte("version: v1alpha1\nmachine:\n type: worker\n") + dir := t.TempDir() + // File path containing shell-meaningful characters: parentheses, + // $, spaces. All three need quoting to be safe to copy-paste from + // a hint into a terminal. + patchFile := filepath.Join(dir, "$(rm -rf -).yaml") + // JSON Patch body with a `test` op whose path is not present in + // rendered: configpatcher.Apply runs the patch, the test fails, + // and the resulting error reaches the patchFile-quoting hint + // branch we want to exercise. + body := []byte(`- op: test + path: /machine/network/hostname + value: never-rendered +`) + if err := os.WriteFile(patchFile, body, 0o644); err != nil { + t.Fatalf("write patch file: %v", err) + } + + _, err := MergeFileAsPatch(rendered, patchFile) + if err == nil { + t.Fatal("expected MergeFileAsPatch to fail on a JSON Patch test op against a missing path") + } + hints := errors.GetAllHints(err) + if len(hints) == 0 { + t.Fatalf("expected at least one hint on the error, got %v", err) + } + // The patchFile must appear quoted somewhere in the hint chain; + // the bare path embedded in plain prose (or worse, inside a + // suggested shell command) would be a copy-paste hazard. The + // production code emits the path via `%q`, which on Windows + // escapes path separators (`C:\foo` becomes `"C:\\foo"`); compare + // the same way so the test is not platform-sensitive. + wantQuoted := strconv.Quote(patchFile) + if !strings.Contains(strings.Join(hints, "\n"), wantQuoted) { + t.Errorf("expected at least one hint to contain the path quoted as %s, got hints:\n%s", wantQuoted, strings.Join(hints, "\n")) + } +} + +// TestPruneBodyIdentitiesAgainstRendered_PropagatesRenderedParseError +// pins the contract that a malformed rendered template surfaces the +// real parse error to the caller, not a downstream configpatcher +// failure on the same malformed bytes. engine.Render produces the +// rendered input from chart helpers this binary owns, so a parse +// failure here points at a chart-helper bug; masking it as a +// LoadPatch error against malformed bytes wastes an entire diagnostic +// session. +func TestPruneBodyIdentitiesAgainstRendered_PropagatesRenderedParseError(t *testing.T) { + body := []byte("version: v1alpha1\nmachine:\n type: worker\n") + rendered := []byte(": : :\n") // malformed YAML + + _, _, err := pruneBodyIdentitiesAgainstRendered(body, rendered) + if err == nil { + t.Fatal("expected parse error for malformed rendered template, got nil") + } + if !strings.Contains(err.Error(), "rendered") { + t.Errorf("expected error to mention rendered template, got: %v", err) + } +} + +// TestPruneIdenticalKeys_DropsBodyEmptySliceAgainstPopulatedRendered +// pins the slice-path asymmetry vs the map-path empty-override guard. +// A body that explicitly sets `key: []` against rendered's populated +// `key: [a, b]` produces an empty primitiveSliceDifference, which the +// caller deletes — rendered's `[a, b]` survives the merge untouched. +// The map path preserves a user-intent empty map; the slice path +// does not. This is consistent with upstream strategic-merge primitive +// array semantics (append-only, cannot replace), so the user could +// not actually clear a primitive list via strategic merge regardless +// — clearing happens via $patch:delete on the parent or via JSON +// Patch. Pin the current behavior so a future maintainer who tries +// to add user-intent empty-slice handling has to also reckon with +// the upstream merge constraint. +func TestPruneIdenticalKeys_DropsBodyEmptySliceAgainstPopulatedRendered(t *testing.T) { + body := map[string]any{ + "key": []any{}, + } + rendered := map[string]any{ + "key": []any{"a", "b"}, + } + + pruneIdenticalKeys(body, rendered) + + if _, ok := body["key"]; ok { + t.Errorf(`expected body["key"] to be deleted (consistent with strategic-merge's append-only primitive-array semantics; clearing must happen via $patch:delete on the parent or via JSON Patch), got %#v`, body["key"]) + } +} + +// TestPrimitiveSliceDifference_ReorderCollapsesToEmpty pins the +// known trade-off documented on primitiveSliceDifference: a body +// that reorders rendered's primitive elements ([b, a] over [a, b]) +// has the same element set as rendered, so the difference is empty, +// the caller deletes the key, and rendered's order survives. The +// test exists so a maintainer who later tries to "fix" the dedup to +// preserve body order has a fail surface that explains why the +// behavior is intentional (strategic-merge's own primitive-array +// semantics cannot replace, only append, so body-side order cannot +// be imposed regardless — the prune just makes the no-op visible). +func TestPrimitiveSliceDifference_ReorderCollapsesToEmpty(t *testing.T) { + body := []any{"b", "a"} + rendered := []any{"a", "b"} + + got := primitiveSliceDifference(body, rendered) + + if len(got) != 0 { + t.Errorf("expected empty diff for reordered-but-identical-set slices, got %v", got) + } +} diff --git a/pkg/engine/render_test.go b/pkg/engine/render_test.go index 2b5aa63f..3fa641b9 100644 --- a/pkg/engine/render_test.go +++ b/pkg/engine/render_test.go @@ -1723,6 +1723,539 @@ machine: t.Errorf("comments-only patch must round-trip rendered byte-for-byte") } }) + + t.Run("round-trips an autogenerated controlplane body", func(t *testing.T) { + // `talm init` writes the rendered template back into nodes/.yaml + // as the body. The next `talm apply` then re-enters + // MergeFileAsPatch with that same body. Pin that the chart's actual + // rendered output round-trips cleanly across both legacy and + // multi-doc formats — a chart regression that re-introduces a + // directive incompatible with configpatcher's strict-decode path + // is caught here at the integration level. The upstream + // constraint lives in configloader/internal/decoder/delete.go + // AppendDeletesTo: it extracts $patch:delete only at document + // and top-level mapping scopes, so a directive nested under a + // typed map field reaches the strict v1alpha1 decoder and trips + // it. MergeFileAsPatch strips chart-side directives before the + // merge target loads to dodge this. + for _, version := range []string{"v1.11", "v1.12"} { + t.Run(version, func(t *testing.T) { + rendered := renderChartTemplate(t, "../../charts/cozystack", "templates/controlplane.yaml", version) + + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + body := "# talm: nodes=[\"10.0.0.1\"], templates=[\"templates/controlplane.yaml\"]\n" + rendered + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + if _, err := MergeFileAsPatch([]byte(rendered), nodeFile); err != nil { + t.Fatalf("MergeFileAsPatch on autogenerated %s body: %v", version, err) + } + }) + } + }) + + t.Run("real cozystack chart round-trip does not duplicate primitive arrays", func(t *testing.T) { + // End-to-end guard against the headline regression: render the + // chart, write the rendered output as the node body (the exact + // `talm init` / `talm template -I` flow), re-render the same + // template, run MergeFileAsPatch on (rendered, body) and assert + // every primitive list entry the chart emits appears exactly + // once in the merged output. Without identity-prune + per-doc + // reattach this fails because Talos's strategic-merge appends + // every primitive list per document and certSANs/nameservers/ + // validSubnets/endpoints all double on each apply round-trip. + // This is a stronger contract than the inline-fixture sibling + // subtest above: it walks the actual chart, so any future chart + // change that adds a new primitive list also gets covered. + for _, version := range []string{"v1.11", "v1.12"} { + t.Run(version, func(t *testing.T) { + rendered := renderChartTemplate(t, "../../charts/cozystack", "templates/controlplane.yaml", version) + + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + body := "# talm: nodes=[\"10.0.0.1\"], templates=[\"templates/controlplane.yaml\"]\n" + rendered + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(rendered), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch on autogenerated %s body: %v", version, err) + } + + // Tokens that appear in the chart's rendered primitive + // lists. Every one must appear at most as many times in + // merged as it appeared in rendered (no duplication). + // Pulled from the actual chart output rather than + // hardcoded so a chart change does not invalidate this + // guard silently. + probes := []string{ + "127.0.0.1", + "https://mirror.gcr.io", + } + for _, probe := range probes { + rcount := strings.Count(rendered, probe) + if rcount == 0 { + continue + } + mcount := strings.Count(string(merged), probe) + if mcount > rcount { + t.Errorf("primitive list entry %q duplicated by round-trip: rendered=%d, merged=%d (%s)\n%s", + probe, rcount, mcount, version, string(merged)) + } + } + }) + } + }) + + t.Run("body identical to rendered does not duplicate primitive arrays", func(t *testing.T) { + // `talm template -I` writes the rendered template back as the + // body. The next apply re-enters MergeFileAsPatch with body == + // rendered. Talos's strategic-merge appends to primitive arrays + // rather than treating them as a set, so without identity-pruning + // every certSANs/nameservers/validSubnets/endpoints entry doubles + // on every apply round-trip — every certSAN, every nameserver, + // every podSubnet appears twice on the second apply, four times + // on the third, and so on. + // + // Pin the post-fix contract: when the body's keys match the + // rendered's keys exactly (deep-equal), MergeFileAsPatch must + // return a result whose primitive arrays are not duplicated. + const renderedTemplate = `version: v1alpha1 +machine: + type: controlplane + certSANs: + - 127.0.0.1 + network: + nameservers: + - 1.1.1.1 + - 8.8.8.8 + install: + disk: /dev/sda +cluster: + controlPlane: + endpoint: https://10.0.0.10:6443 + apiServer: + certSANs: + - 127.0.0.1 +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + body := "# talm: nodes=[\"10.0.0.1\"]\n" + renderedTemplate + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + + out := string(merged) + // certSANs appears in two places (machine and apiServer); each + // should still hold exactly one 127.0.0.1. + if got := strings.Count(out, "127.0.0.1"); got != 2 { + t.Errorf("expected 127.0.0.1 to appear twice (once under each certSANs), got %d:\n%s", got, out) + } + if got := strings.Count(out, "1.1.1.1"); got != 1 { + t.Errorf("nameservers entry 1.1.1.1 duplicated (count=%d):\n%s", got, out) + } + if got := strings.Count(out, "8.8.8.8"); got != 1 { + t.Errorf("nameservers entry 8.8.8.8 duplicated (count=%d):\n%s", got, out) + } + }) + + t.Run("body adding a primitive-array entry must not duplicate rendered entries", func(t *testing.T) { + // The "autogenerated body re-states unchanged values" prune covers + // the byte-identical case; this test pins the partial-edit case + // where the user adds one new entry to an array the chart already + // populated. Talos's strategic-merge appends primitive arrays + // rather than treating them as a set, so without per-element diff + // the rendered entries appear twice in the merged config — the + // same duplicate-primitive-array-entries-per-round-trip symptom + // as the byte-identical case, just one entry deeper. + const renderedTemplate = `version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda + certSANs: + - 127.0.0.1 + - 10.0.0.10 +` + const userBody = `# talm: nodes=["10.0.0.1"] +machine: + certSANs: + - 127.0.0.1 + - 10.0.0.10 + - 10.0.0.11 +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + if err := os.WriteFile(nodeFile, []byte(userBody), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + out := string(merged) + if got := strings.Count(out, "127.0.0.1"); got != 1 { + t.Errorf("rendered entry 127.0.0.1 duplicated after partial-array edit (count=%d):\n%s", got, out) + } + if got := strings.Count(out, "10.0.0.10"); got != 1 { + t.Errorf("rendered entry 10.0.0.10 duplicated after partial-array edit (count=%d):\n%s", got, out) + } + if !strings.Contains(out, "10.0.0.11") { + t.Errorf("user-added entry 10.0.0.11 missing from merged output:\n%s", out) + } + }) + + t.Run("JSON Patch body is forwarded to LoadPatch unchanged", func(t *testing.T) { + // MergeFileAsPatch's documented contract (and the existing + // LoadPatch error hint) advertises support for JSON Patch and + // YAML patch-list shapes. Those bodies decode as a YAML + // sequence at the top level, not a mapping; the identity-prune + // step cannot operate on them and must pass them through to + // configpatcher.LoadPatch unchanged. A regression here + // silently neutralises the patch. + const renderedTemplate = `version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda + network: + hostname: rendered-host + nameservers: + - 1.1.1.1 +` + const jsonPatchBody = `# talm: nodes=["10.0.0.1"] +- op: replace + path: /machine/network/hostname + value: cozy-01 +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + if err := os.WriteFile(nodeFile, []byte(jsonPatchBody), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + out := string(merged) + if !strings.Contains(out, "hostname: cozy-01") { + t.Errorf("JSON Patch replace op silently dropped (hostname not overridden):\n%s", out) + } + if strings.Contains(out, "rendered-host") { + t.Errorf("rendered hostname still present despite JSON Patch replace op:\n%s", out) + } + }) + + t.Run("multi-doc body identical to rendered does not duplicate primitive arrays", func(t *testing.T) { + // Talos v1.12+ output is multi-document. The single-doc identity + // prune cannot help here unless it understands document boundaries: + // `talm template -I` writes each rendered document back as a body + // document, and Talos's strategic-merge appends to primitive arrays + // per-document. Without per-doc identity matching, the prune + // short-circuits the multi-doc input and the duplicate-primitive- + // array-entries-per-round-trip symptom reappears at the v1.12+ + // default — a `127.0.0.1` certSAN entry doubles on every apply. + // + // Pin the post-fix contract: a body that re-states an unchanged + // multi-doc rendered template must merge to a config whose + // primitive-array entry counts are unchanged. + rendered := renderChartTemplate(t, "../../charts/cozystack", "templates/controlplane.yaml", "v1.12") + + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + body := "# talm: nodes=[\"10.0.0.1\"], templates=[\"templates/controlplane.yaml\"]\n" + rendered + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(rendered), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + + renderedCount := strings.Count(string(rendered), "127.0.0.1") + mergedCount := strings.Count(string(merged), "127.0.0.1") + if renderedCount == 0 { + t.Fatalf("test fixture broken: rendered output has no 127.0.0.1 to count duplicates against") + } + if mergedCount != renderedCount { + t.Errorf("primitive array entry duplicated across multi-doc round-trip: rendered had %d occurrences of 127.0.0.1, merged has %d", renderedCount, mergedCount) + } + }) + + t.Run("body with override and identical arrays merges only the override", func(t *testing.T) { + // User pattern: keep the auto-generated body almost intact, change + // just one field (e.g. hostname). The unchanged keys must be + // pruned before merge so they don't replay back as appends. + const renderedTemplate = `version: v1alpha1 +machine: + type: controlplane + network: + hostname: rescue + nameservers: + - 1.1.1.1 + - 8.8.8.8 +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + const body = `# talm: nodes=["10.0.0.1"] +machine: + type: controlplane + network: + hostname: cozy-01 + nameservers: + - 1.1.1.1 + - 8.8.8.8 +` + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + + out := string(merged) + if !strings.Contains(out, "hostname: cozy-01") { + t.Errorf("override not applied: %s", out) + } + if strings.Contains(out, "hostname: rescue") { + t.Errorf("rendered hostname survived merge: %s", out) + } + if got := strings.Count(out, "1.1.1.1"); got != 1 { + t.Errorf("nameservers 1.1.1.1 duplicated despite identity-pruning (count=%d):\n%s", got, out) + } + }) + + t.Run("preserves chart directive effect: rendered with $patch:delete merges into config without the deleted key", func(t *testing.T) { + // MergeFileAsPatch is the only consumer of the rendered template's + // $patch:delete directives in the apply pipeline — the bytes it + // returns are sent verbatim to Talos's ApplyConfiguration RPC, + // whose server-side configloader.NewFromBytes does NOT enable + // WithAllowPatchDelete (see Talos's internal/app/machined/internal/server/v1alpha1/v1alpha1_server.go + // ApplyConfiguration). A directive surviving the merge would be + // rejected on the wire. + // + // Pin the contract: when the rendered template carries a nested + // `$patch: delete` directive (the cozystack chart pattern that + // removes the exclude-from-external-load-balancers label on + // controlplane), MergeFileAsPatch must + // 1. apply the directive's effect locally so the deleted key is + // absent from the merged output, and + // 2. return bytes that contain no `$patch: delete` literal so + // Talos's strict decoder accepts the payload. + // + // The fixture mimics the rendered template that contains the + // directive plus a non-empty user body to exercise the full + // LoadPatch + Apply path (modeline-only would short-circuit). + const renderedWithDirective = `version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda + nodeLabels: + node.kubernetes.io/exclude-from-external-load-balancers: + $patch: delete +` + const userBody = `# talm: nodes=["10.0.0.1"] +machine: + network: + hostname: cozy-01 +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + if err := os.WriteFile(nodeFile, []byte(userBody), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedWithDirective), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + + out := string(merged) + if strings.Contains(out, "$patch: delete") { + t.Errorf("merged output still carries the directive literal:\n%s", out) + } + if strings.Contains(out, "exclude-from-external-load-balancers") { + t.Errorf("merged output still carries the deleted key (directive's effect not applied):\n%s", out) + } + if !strings.Contains(out, "hostname: cozy-01") { + t.Errorf("body's hostname override missing from merged output:\n%s", out) + } + }) + +} + +// TestMergeFileAsPatch_PreservesUserIntentPatchDelete pins the contract +// that a user-supplied `$patch: delete` in the per-node body — at a path +// the chart-rendered template did NOT itself mark for deletion — must +// survive MergeFileAsPatch and remove the named key from the merged +// output. configpatcher.LoadPatch already routes such bodies through +// configloader.NewFromBytes(WithAllowPatchDelete()) (load.go:24), so a +// genuine user-intent directive should land in the merge as a Selector +// and delete the key from rendered. +// +// The chart pattern (rendered AND body both carry the directive at the +// same path because `talm template -I` writes rendered back as body) +// is handled by stripping the redundant entry; this test exercises the +// orthogonal case where the directive is user intent, not chart noise. +func TestMergeFileAsPatch_PreservesUserIntentPatchDelete(t *testing.T) { + const renderedTemplate = `version: v1alpha1 +machine: + type: controlplane + install: + disk: /dev/sda + network: + hostname: rendered-hostname + nameservers: + - 1.1.1.1 +` + const userBodyDeleteHostname = `# talm: nodes=["10.0.0.1"] +machine: + network: + hostname: + $patch: delete +` + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + if err := os.WriteFile(nodeFile, []byte(userBodyDeleteHostname), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(renderedTemplate), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch: %v", err) + } + + out := string(merged) + if strings.Contains(out, "rendered-hostname") { + t.Errorf("user-intent $patch:delete on machine.network.hostname did not remove the rendered value:\n%s", out) + } + if strings.Contains(out, "$patch: delete") { + t.Errorf("merged output still carries the directive literal — Talos will reject it on ApplyConfiguration:\n%s", out) + } + if !strings.Contains(out, "1.1.1.1") { + t.Errorf("nameservers (an untouched sibling field) lost from merge:\n%s", out) + } +} + +// TestTalmDiscoveredHostnameFiltersTransientNames pins the filter contract +// for `talm.discovered.hostname`. Boot-to-talos and Talos's own pre-config +// state can leave a node with a placeholder hostname (`rescue`, `talos`, +// `localhost`, `localhost.localdomain`). When the helper propagates such a +// name, `talm template -I` writes it back into the node body — and because +// the body now matches what the live node already has, the user has no diff +// to alert them that the hostname is transient. The next apply replays the +// placeholder, discovery keeps returning it, and the loop never resolves to +// the user's intended per-node hostname: a freshly imaged node sits with +// `hostname: rescue` indefinitely until the operator notices and edits the +// node body by hand. +// +// The fix: skip the discovery hit for a small set of well-known transient +// names and fall through to the address-derived `talos-XXXXX` placeholder, +// the same form a node with no discoverable hostname produces. The +// placeholder is visibly synthetic, signalling "this needs a real +// per-node value" to anyone reviewing the autogenerated body. +func TestTalmDiscoveredHostnameFiltersTransientNames(t *testing.T) { + // Includes case-variant entries to pin the lower-case fold the + // helper applies before its `has` membership check. Some PXE/DHCP + // servers hand out `Localhost`/`TALOS` and the trap-loop is + // identical for those; the filter must catch them too. + transient := []string{ + "rescue", "talos", "localhost", "localhost.localdomain", + "Localhost", "TALOS", "RESCUE", "Localhost.LocalDomain", + } + for _, name := range transient { + t.Run(name, func(t *testing.T) { + output := renderChartTemplateWithLookup( + t, + "../../charts/cozystack", + "templates/controlplane.yaml", + hostnameLookupOverride(simpleNicLookup(), name), + "v1.11", + ) + if strings.Contains(output, `hostname: "`+name+`"`) { + t.Errorf("transient hostname %q leaked into rendered output:\n%s", name, output) + } + if !strings.Contains(output, `hostname: "talos-`) { + t.Errorf(`expected fallback hostname "talos-XXXXX", got:\n%s`, output) + } + }) + } + + t.Run("real_hostname_passes_through", func(t *testing.T) { + output := renderChartTemplateWithLookup( + t, + "../../charts/cozystack", + "templates/controlplane.yaml", + hostnameLookupOverride(simpleNicLookup(), "cozy-01"), + "v1.11", + ) + if !strings.Contains(output, `hostname: "cozy-01"`) { + t.Errorf("real hostname did not propagate, output:\n%s", output) + } + }) +} + +// hostnameLookupOverride wraps a base LookupFunc to return a fixed hostname +// for the `hostname//hostname` query that talm.discovered.hostname +// issues. Every other lookup falls through to base. Used by the +// hostname-filter regression tests to exercise the helper without standing +// up a real Talos node. +func hostnameLookupOverride(base func(string, string, string) (map[string]any, error), hostname string) func(string, string, string) (map[string]any, error) { + return func(kind, namespace, id string) (map[string]any, error) { + if kind == "hostname" { + return map[string]any{ + "metadata": map[string]any{"id": "hostname"}, + "spec": map[string]any{"hostname": hostname}, + }, nil + } + return base(kind, namespace, id) + } +} + +// TestRenderedControlplaneEmitsExcludeLabelDeleteDirective pins the +// cozystack-side intent of commit abf48543 ("Remove +// node.kubernetes.io/exclude-from-external-load-balancers label for +// Cozystack"): the rendered controlplane output MUST emit +// +// machine.nodeLabels.node.kubernetes.io/exclude-from-external-load-balancers: +// $patch: delete +// +// so that controlplane nodes participate in external load balancer target +// pools. The directive is a strategic-merge SMP signal; it must reach the +// merged config and be applied before the bytes are sent to Talos's +// ApplyConfiguration RPC, which strict-decodes the payload without +// WithAllowPatchDelete and would reject a directive that survived merge. +// The upstream guardrail lives in +// pkg/machinery/config/configloader/internal/decoder/delete.go +// AppendDeletesTo: it only extracts $patch:delete at document and top- +// level mapping scopes, so a nested directive that survived merge +// reaches the strict v1alpha1 decoder and trips +// `cannot construct !!map into string`. talm's MergeFileAsPatch +// resolves the directive locally — see the +// "preserves chart directive effect" subtest in TestMergeFileAsPatch +// for the resolved-config contract. +func TestRenderedControlplaneEmitsExcludeLabelDeleteDirective(t *testing.T) { + for _, version := range []string{"v1.11", "v1.12"} { + t.Run(version, func(t *testing.T) { + output := renderChartTemplate(t, "../../charts/cozystack", "templates/controlplane.yaml", version) + assertContains(t, output, "node.kubernetes.io/exclude-from-external-load-balancers:") + assertContains(t, output, "$patch: delete") + }) + } } // TestNodeFileHasOverlay pins the classifier used by the apply path to @@ -1959,6 +2492,27 @@ func simpleNicLookup() func(string, string, string) (map[string]any, error) { } } +// freshNicLookup returns a lookup fixture for a node in first-boot +// state: no routes, no addresses, no usable links. Discovery cannot +// resolve a default-gateway-bearing link; every helper that depends +// on it returns empty. Used to exercise code paths that must work +// when the chart is generating the very network configuration that +// will populate discovery on the next reconciliation. +func freshNicLookup() func(string, string, string) (map[string]any, error) { + emptyList := map[string]any{ + "apiVersion": "v1", + "kind": "List", + "items": []any{}, + } + return func(resource, namespace, id string) (map[string]any, error) { + switch resource { + case "routes", "links", "addresses": + return emptyList, nil + } + return map[string]any{}, nil + } +} + // renderCozystackWith renders the cozystack controlplane template // against the supplied LookupFunc and values overrides, returning the // final template output or failing the test. Mirrors the pattern used @@ -2139,6 +2693,23 @@ func TestMultiDocGeneric_ValidSubnetsFallsBackToDiscovery(t *testing.T) { } } +// TestMultiDocGeneric_VIPLinkOverride mirrors the cozystack VIP-link +// override test for the generic preset. The two charts share helper +// shape, so the override must apply symmetrically — without this the +// generic-preset apply pipeline would still pin the VIP onto the +// physical NIC discovered at first apply when the operator wanted +// the VIP on a not-yet-existing VLAN sub-interface. +func TestMultiDocGeneric_VIPLinkOverride(t *testing.T) { + result := renderGenericWith(t, simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + }) + + assertContains(t, result, "kind: Layer2VIPConfig") + assertContains(t, result, "link: eth0.4000") + assertNotContains(t, result, "link: eth0\n") +} + // TestMultiDocCozystack_ShippedDefaultsFailFresh asserts that a fresh // `talm init -p cozystack` user who keeps values.yaml defaults gets a // loud `required` error — not a silently-embedded placeholder @@ -2194,6 +2765,255 @@ func TestMultiDocCozystack_NoVIPOnFreshDefaults(t *testing.T) { assertNotContains(t, result, "kind: Layer2VIPConfig") } +// TestMultiDocCozystack_VIPLinkOverride pins the chicken-and-egg fix +// for nodes that need the VIP on a link that does not yet exist on +// the live system at first apply (typical case: a VLAN sub-interface +// that the same template is about to bring up). Without an override +// the chart would derive vipLink from discovery, which on a fresh +// install sees only the physical NIC and pins the VIP there — after +// apply the VLAN comes up and the VIP is on the wrong link. Setting +// .Values.vipLink lets the operator declare the target link up front +// so the rendered Layer2VIPConfig matches the post-apply network. +func TestMultiDocCozystack_VIPLinkOverride(t *testing.T) { + result := renderCozystackWith(t, simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + }) + + assertContains(t, result, "kind: Layer2VIPConfig") + assertContains(t, result, "link: eth0.4000") + assertNotContains(t, result, "link: eth0\n") + + // Override-path Layer2VIPConfig must emit exactly once. The + // discovery-derived block is gated on `not .Values.vipLink`, so + // no second document with link: eth0 should appear alongside + // the override. + if c := strings.Count(result, "kind: Layer2VIPConfig"); c != 1 { + t.Errorf("expected exactly one Layer2VIPConfig document, got %d:\n%s", c, result) + } +} + +// TestMultiDocCozystack_VIPLinkOverrideOnFreshNode pins the +// fresh-node case for the vipLink override: a node with no +// discovered default-gateway link (totally fresh: no addresses, no +// routes — first-boot state before the chart's own LinkConfig has +// run) must still emit a Layer2VIPConfig when the operator has set +// .Values.vipLink. Without this the override silently no-ops on the +// exact case it was added for: the operator wants the VIP on a VLAN +// sub-interface this same template is about to bring up, but the +// chart hides the VIP doc behind a discovery-resolved-link gate the +// fresh node has not met. +func TestMultiDocCozystack_VIPLinkOverrideOnFreshNode(t *testing.T) { + // On a fresh node, discovery cannot derive advertisedSubnets, so + // the operator must set it explicitly — same path the chart's + // `required` guard documents in values.yaml. Set it here so the + // render reaches the VIP block we want to exercise instead of + // erroring out earlier. + result := renderCozystackWith(t, freshNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + "advertisedSubnets": []any{"192.168.201.0/24"}, + }) + + assertContains(t, result, "kind: Layer2VIPConfig") + assertContains(t, result, "link: eth0.4000") + if c := strings.Count(result, "kind: Layer2VIPConfig"); c != 1 { + t.Errorf("expected exactly one Layer2VIPConfig on fresh-node override, got %d:\n%s", c, result) + } +} + +// renderLegacyChart renders the controlplane template of the supplied +// chart against a "legacy" Talos config (TalosVersion=""), routing +// through talos.config.legacy. Mirrors the multidoc render helpers +// above but exercises the legacy code path that pre-1.12 Talos still +// uses by default. Returns the rendered controlplane document. +func renderLegacyChart(t *testing.T, chartDir, templateName string, lookup func(string, string, string) (map[string]any, error), overrides map[string]any) string { + t.Helper() + origLookup := helmEngine.LookupFunc + t.Cleanup(func() { helmEngine.LookupFunc = origLookup }) + helmEngine.LookupFunc = lookup + + chrt, err := loader.LoadDir(chartDir) + if err != nil { + t.Fatalf("load chart: %v", err) + } + values := cloneValues(chrt.Values) + if v, _ := values["endpoint"].(string); v == "" { + values["endpoint"] = testEndpoint + } + maps.Copy(values, overrides) + + eng := helmEngine.Engine{} + out, err := eng.Render(chrt, chartutil.Values{ + "Values": values, + "TalosVersion": "", + }) + if err != nil { + t.Fatalf("render: %v", err) + } + return out[templateName] +} + +// TestLegacyCozystack_VIPLinkOverride pins the legacy-schema mirror +// of TestMultiDocCozystack_VIPLinkOverride. The legacy Talos config +// shape has no Layer2VIPConfig document — VIPs live at +// machine.network.interfaces[].vip — so the override is expressed as +// a separate vip-only top-level interfaces[] entry. Without this +// fix, fresh `talm init -p cozystack` users on the default +// `talosVersion: ""` chart setting silently lose the override. +func TestLegacyCozystack_VIPLinkOverride(t *testing.T) { + result := renderLegacyChart(t, "../../charts/cozystack", "cozystack/templates/controlplane.yaml", simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + }) + + // Override entry: a top-level interfaces[] entry with the + // operator's link name and only the vip block. + assertContains(t, result, "- interface: eth0.4000") + assertContains(t, result, "ip: 192.168.201.5") + // Inline (discovery-derived) vip on the bare NIC must be + // suppressed when vipLink redirects the VIP. + assertNotContains(t, result, "interface: eth0\n addresses: [\"192.168.201.10/24\"]\n routes:\n - network: 0.0.0.0/0\n gateway: 192.168.201.1\n vip:") + // Legacy schema has no Layer2VIPConfig kind. + assertNotContains(t, result, "kind: Layer2VIPConfig") +} + +// TestLegacyGeneric_VIPLinkOverride mirrors the cozystack-side legacy +// override test for the generic preset. The generic chart ships +// `talosVersion: ""` by default, so the legacy branch is the path a +// fresh `talm init -p generic` user actually takes. +func TestLegacyGeneric_VIPLinkOverride(t *testing.T) { + result := renderLegacyChart(t, "../../charts/generic", "generic/templates/controlplane.yaml", simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + }) + + assertContains(t, result, "- interface: eth0.4000") + assertContains(t, result, "ip: 192.168.201.5") + assertNotContains(t, result, "interface: eth0\n addresses: [\"192.168.201.10/24\"]\n routes:\n - network: 0.0.0.0/0\n gateway: 192.168.201.1\n vip:") + assertNotContains(t, result, "kind: Layer2VIPConfig") +} + +// TestLegacyCozystack_VIPLinkOverrideOnFreshNode pins the +// chicken-and-egg case for legacy: a node where discovery returns no +// default-gateway link must still emit the override entry. Without +// this the override would silently no-op on the exact case it was +// added for — the operator wants the VIP on a VLAN sub-interface +// this same template is about to bring up. +func TestLegacyCozystack_VIPLinkOverrideOnFreshNode(t *testing.T) { + result := renderLegacyChart(t, "../../charts/cozystack", "cozystack/templates/controlplane.yaml", freshNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + "advertisedSubnets": []any{"192.168.201.0/24"}, + }) + + assertContains(t, result, "interfaces:") + assertContains(t, result, "- interface: eth0.4000") + assertContains(t, result, "ip: 192.168.201.5") +} + +// TestLegacyCozystack_VIPLinkMatchesDiscovery pins the no-op case: +// when vipLink names the same link discovery already picked, the +// chart must NOT emit a duplicate interfaces[] entry — Talos legacy +// validation rejects duplicate interface names. The inline vip block +// on the discovered interface must remain, since it already pins the +// VIP on the right link. +func TestLegacyCozystack_VIPLinkMatchesDiscovery(t *testing.T) { + result := renderLegacyChart(t, "../../charts/cozystack", "cozystack/templates/controlplane.yaml", simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0", + }) + + // Exactly one interface entry for eth0 — not a duplicate. + if c := strings.Count(result, "- interface: eth0"); c != 1 { + t.Errorf("expected exactly one - interface: eth0 entry, got %d:\n%s", c, result) + } + // Inline vip is preserved on the discovered entry. + assertContains(t, result, "vip:") + assertContains(t, result, "ip: 192.168.201.5") +} + +// TestMergeFileAsPatch_TypedDocPartialEditPreservesIdentityKeys pins +// the regression that the multi-doc identity prune introduced: a +// typed multi-doc body where the user changes one field but keeps +// the rest identical to rendered (the dominant `talm template -I` +// follow-up edit pattern) had its apiVersion / kind / name pruned +// because every identity key is byte-equal to rendered's. The body +// then reached configpatcher.LoadPatch as a bare key/value map and +// LoadPatch rejected it with `missing kind`. +// +// Pin the post-fix contract: a partial-edit typed-doc body must +// preserve enough identity for LoadPatch to route the patch to the +// correct rendered document, and the override field must reach the +// merged config. +func TestMergeFileAsPatch_TypedDocPartialEditPreservesIdentityKeys(t *testing.T) { + rendered := renderChartTemplate(t, "../../charts/cozystack", "templates/controlplane.yaml", "v1.12") + + // Operator copies rendered output into the body, then edits the + // hostname. apiVersion/kind/name remain byte-identical with rendered. + body := strings.Replace(rendered, + `hostname: "talos-`, + `hostname: "operator-edited-`, + 1, + ) + if body == rendered { + t.Fatalf("test fixture broken: rendered output did not contain expected hostname pattern:\n%s", rendered) + } + body = "# talm: nodes=[\"10.0.0.1\"], templates=[\"templates/controlplane.yaml\"]\n" + body + + dir := t.TempDir() + nodeFile := filepath.Join(dir, "node0.yaml") + if err := os.WriteFile(nodeFile, []byte(body), 0o644); err != nil { + t.Fatalf("write node file: %v", err) + } + + merged, err := MergeFileAsPatch([]byte(rendered), nodeFile) + if err != nil { + t.Fatalf("MergeFileAsPatch on typed-doc partial edit: %v", err) + } + + out := string(merged) + if !strings.Contains(out, "operator-edited-") { + t.Errorf("hostname override did not reach merged config:\n%s", out) + } +} + +// TestMultiDocCozystack_VIPLinkOverrideDoesNotAutoEmitLinkConfig +// pins the doc-vs-reality contract for the vipLink override: +// values.yaml comments and the README state explicitly that the +// chart does NOT auto-emit a LinkConfig or VLANConfig for the +// override link. The operator is responsible for bringing the link +// up via their own per-node body overlay. Without this guard, a +// future "make vipLink autoconfigure the link" patch would leave +// the documented contract stale and the existing operator workflow +// (override + body LinkConfig) would suddenly produce duplicate +// LinkConfig documents. +func TestMultiDocCozystack_VIPLinkOverrideDoesNotAutoEmitLinkConfig(t *testing.T) { + result := renderCozystackWith(t, freshNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + "vipLink": "eth0.4000", + "advertisedSubnets": []any{"192.168.201.0/24"}, + }) + + if strings.Contains(result, "name: eth0.4000") { + t.Errorf("rendered output unexpectedly references the override link by name (LinkConfig/VLANConfig auto-emit?); the chart docs state this is the operator's responsibility:\n%s", result) + } +} + +// TestMultiDocCozystack_VIPLinkDefaultsToDiscovery asserts that the +// override is opt-in: when .Values.vipLink is left blank the chart +// keeps the existing discovery-derived behavior (link from the +// default-gateway-bearing interface), unchanged from prior releases. +func TestMultiDocCozystack_VIPLinkDefaultsToDiscovery(t *testing.T) { + result := renderCozystackWith(t, simpleNicLookup(), map[string]any{ + "floatingIP": "192.168.201.5", + }) + + assertContains(t, result, "kind: Layer2VIPConfig") + assertContains(t, result, "link: eth0") + assertNotContains(t, result, "link: eth0.") +} + // TestMultiDocCozystack_DedupesDuplicateSubnetsFromMultipleAddresses // pins that a link with multiple addresses in the same subnet emits // a single entry in validSubnets / advertisedSubnets, not one entry