fix(kubernetes): name TalosConfigTemplate by content hash - #3523

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant Helm
  participant MachineDeployment
  participant TalosReconcileJob
  participant TalosConfigTemplate
  Helm->>MachineDeployment: Set hashed bootstrap configRef
  Helm->>TalosReconcileJob: Provide TCT_NAME and TCT_SPEC
  TalosReconcileJob->>TalosConfigTemplate: Apply the hashed template

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Note for reviewers: overlapping files with a sibling PR

packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml and
packages/apps/kubernetes/tests/talos_templates_test.yaml are also touched by #3521, which raises that Job's
kubectl container memory limit from 256Mi to 512Mi and exposes it as talos.reconcileJobMemoryLimit.

This PR restructures that Job substantially (the machine config moves into a shared named template), so #3521 will
need a rebase if this one lands first — or this one will, in the other order. No functional interaction: the memory
limit is not part of the hashed spec, so it does not affect the content-hash name. Happy to rebase either way.

Thanks, but this one is a false positive, and I want to record why so it does not cost a reviewer time.

packages/apps/kubernetes-nodes/charts/cozy-lib is not an empty directory, it is a symlink. On this branch:

$ git ls-tree HEAD packages/apps/kubernetes-nodes/charts/
120000 blob e18135098...    packages/apps/kubernetes-nodes/charts/cozy-lib

Mode 120000 is a symlink, and the target is the real library, where the helper is defined:

$ git grep -n 'define "cozy-lib.resources.toFloat"'
packages/library/cozy-lib/templates/_resources.tpl:42:{{- define "cozy-lib.resources.toFloat" -}}

So Helm resolves it at render time and there is nothing to add. Two further points confirm it: the call sites are not introduced by this PR (resources.toFloat is already used in nodegroup.yaml on main, so a missing definition would have been breaking kubernetes-nodes long before this change), and helm unittest passes on this branch, which it could not if the function were undefined, since the tests render the very templates that call it.

The analysis chain above shows the cause: the shell probes ran fd/rg inside the symlinked path and got no matches, which they reported as "exists only as a directory". Tooling that does not follow symlinks will keep reaching this conclusion on any Cozystack package, because every package vendors cozy-lib this way.

Rebased, thanks for the precise pointer. I merged upstream/main rather than rebasing so the review history stays intact and no force-push is needed. mergeable is back to MERGEABLE.

Conflict resolution. Kept this branch's side of the group loop, as you suggested. I checked why before doing it rather than taking it on faith: since the merge base, the only upstream change to talos-reconcile-job.yaml is 4e9c5ce, 8 insertions and 1 deletion in a single hunk, switching .Values.nodeGroups to the kubernetes.nodeGroups helper. This branch already iterates the helper, so keeping our side carries #3535 rather than reverting it. The rest of what upstream still renders inline lives in _talosconfigtemplate.tpl here, $mgmtClusterDomain and the searchDomains/nameservers block included, and $mgmtClusterDomain has no consumer left in the Job outside that region, so nothing is dropped.

Verification matches your local run: 195 tests in packages/apps/kubernetes with main's talos_reconcile_nodegroups_test.yaml included, 9 in packages/apps/kubernetes-nodes, and render-parity.sh reports GOLDEN PARITY: all pool objects byte-identical across all cases across the resources, gpu and kubelet cases. make generate clean in both packages.

On your non-blocking point about the lookup-derived hash. You are right and it is worth naming the consequence explicitly, because it follows from my choice to hash the whole rendered spec: for a group sized by instanceType, editing the VirtualMachineClusterInstancetype itself rotates the hash and rolls every worker of every group referencing it. That is a real behaviour change, and unlike a values edit it can happen without anyone touching the cluster's Kubernetes resource.

Two ways out, and I do not have a strong preference:

• Exclude the kubelet-reservation block from the hash the way the runtime placeholders already are, on the grounds that it is derived state rather than a chart input. The cost is that a genuine reservation change then stops propagating, which is the bug this PR exists to fix, so it would need the reservations to move out of the immutable spec entirely.
• Keep it in the hash and accept that a reservation change rolls workers, which is arguably correct, and document it.

One data point in favour of the current shape: under helm template without cluster access the parent chart fails loudly rather than computing a divergent hash, cluster.yaml:532 raises nodeGroup <name>: specified instanceType "<type>" not found in cluster. So the dry-run divergence you were worried about surfaces as an error, not as a silent name mismatch. That does not cover the in-cluster instancetype-edit case above.

Live evidence for the "Upgrade impact" section, which I flagged as unverified.

We now have it, from a production fleet of 13 tenant clusters. Last night we exercised the same rollout path this PR triggers, by changing a value that rotates the KubevirtMachineTemplate hash on a GPU node group (4 to 5 replicas plus diskSize 175Gi to 200Gi). Three things came out of it that the section should probably mention:

1. maxSurge: maxReplicas is dangerous on a constrained resource. The fleet had 6 GPUs with 1 free. CAPI created all 5 new machines at once; 3 sat in Pending on Insufficient nvidia.com/GB202GL_RTX_PRO_6000_BLACKWELL_SERVER_EDITION, each having already pulled a 200 GiB image. It converges, but it is a lot of wasted import for nothing.

2. maxSurge: 0 with maxUnavailable: 1 deadlocks in that same situation. CAPI computes maxScaledDown = available - (desired - maxUnavailable), which was 4 - 4 = 0, so it removed no old machine, while the new machines were waiting for exactly those machines to release their GPUs. Nothing moved until we loosened maxUnavailable to 2. Worth a sentence, because maxSurge: 0 is the natural reflex when capacity is tight and it is the one setting that cannot make progress.

3. The MachineHealthCheck amplifies both. With maxUnhealthy: 100% and nodeStartupTimeout: 10m, the machines queuing for a GPU were declared unhealthy and remediated in a loop, three of them deleted and recreated while perfectly healthy, each iteration re-importing the disk. The per-node-group nodeStartupTimeout override fixes it, but the default is shorter than a plausible queue wait.

None of this changes the correctness of this PR, and I am not proposing to widen its scope. It does mean the one-time roll it ships is not free for clusters with a scarce-resource node group, so I would rather say so in the release note than have operators discover it.

Happy to add a paragraph to the upgrade-impact section covering the above if you want it in this PR, or to open it as a follow-up against the MachineDeployment strategy defaults.

Blocker addressed. Both commits are ported into _talosconfigtemplate.tpl rather than the conflict being resolved by picking a side.

In the helper, in the parent chart and copied verbatim into kubernetes-nodes so the two stay byte-identical: talosVersion, installerRepository and schematicID take the escape chain for backslash, dollar and backtick; the machine.registries.mirrors block comes across escaped the same way; and the INVARIANT comment moves above data: there. I reworded it for its new home rather than copying it verbatim, on two counts. The spec is rendered in the helper and nindented into the Job's heredoc by the caller, so the comment now says the invariant travels with the text rather than with the Job template. And because the file is shared between the two charts it names both render-time-validated sites, cluster.yaml in kubernetes and nodegroup.yaml in kubernetes-nodes, instead of one. The sink itself is untouched; what moved is where the escaping lives.

The execution-level evidence is that hack/talos-reconcile-heredoc_test.bats passes unmodified, all five cases, including the hostile registryMirrors endpoint and the hostile image coordinates, in both charts. That suite renders, extracts the heredoc and runs it through a real shell, so it is the check that a rendered-string regex could not do, and it did not need adjusting for the relocation.

You were right on both consequences.

talos.registryMirrors does land inside the hashed spec. Confirmed by rendering: the MachineDeployment configRef goes t-md0-35293f to t-md0-49ef3d on setting a mirror and to t-md0-ef9cc5 on changing it, so a mirror change now rolls the group rather than applying only to nodes created later. packages/apps/kubernetes/README.md claimed the opposite in the Phase 1 Talos rollover bullet and is corrected.

And the byte-identity claim needed rerunning, since it was measured against a main that no longer exists. It still holds: the machine config this branch renders is byte-identical to what current upstream/main renders for the same values on default values (92 lines), on a GPU node group (102) and with registryMirrors set (97). As you expected, the escapes are no-ops on ordinary values and the mirrors block is gated on a non-empty map.

Both "Upgrade impact" gaps are filled. The section now says the one roll is not the only one: an instancetype edit rotates the name later for a group sized by instanceType, which the built-in md0 is, and registryMirrors is a second trigger that did not exist before the merge, with the note that the KubevirtMachineTemplate hash has neither. And the scarce-resource rollout evidence moved out of my comment into the section, all three parts, with the surge and nodeStartupTimeout caveats also added to the release note.

Suites after the merge: 209 in packages/apps/kubernetes, 15 in packages/apps/kubernetes-nodes with your registry_mirrors_test.yaml and serial_console_log_test.yaml included, GOLDEN PARITY clean, make generate no drift, api/apps/v1alpha1 builds with every type covered by a deepcopy.

Pruning and the two hand-maintained literals in talos_config_template_name_test.yaml:37,56 are untouched, still follow-ups as agreed. Merge order noted: #3571 is merged with main and out of draft, so it can go first.

NOT LGTM. The #3513 port is done and I verified it properly this time, so that blocker is closed. What is left is that the new invariant this PR creates, two templates computing the same name, is guarded in the parent chart and not guarded in kubernetes-nodes, and a mutation walks straight through the suite.

Blocker: the hash input is assembled twice in kubernetes-nodes

The description says rendering the spec in one place makes a name divergence structurally impossible. That holds for packages/apps/kubernetes, where cluster.yaml:723 and talos-reconcile-job.yaml:385 both pass the same $group straight out of the kubernetes.nodeGroups helper. It does not hold for kubernetes-nodes. There nodegroup.yaml:34 builds a ten key $group and talos-reconcile-job.yaml:273 builds a separate four key one, and both feed kubernetes.talosConfigTemplateHash. The rendering is shared, the input assembly is not, so the two sites agree by coincidence of the four keys the helper happens to read.

I dropped "gpus" .Values.gpus from the Job's dict, one line, and nothing caught it. helm unittest . in packages/apps/kubernetes-nodes stayed at 15 of 15 passing and bash tests/render-parity.sh stayed green, which is expected once you read its header: it compares KubevirtMachineTemplate, MachineDeployment, MachineHealthCheck and WorkloadMonitor, and says so at line 19, never the Job. Rendering kubernetes-nodes-myk8s-md0 with gpus: [{name: nvidia.com/GH200}] on the mutated tree gives TCT_NAME=kubernetes-myk8s-md0-ffdbe1 on the Job and configRef.name=kubernetes-myk8s-md0-0bbd2a on the MachineDeployment. On the clean tree both are 0bbd2a. That divergence is the templates do not exist deadlock this PR exists to make impossible, and no worker is ever created.

The suite is not broken, it just does not cover this. Reverting nodegroup.yaml's configRef to the old fixed name does turn nodegroup_test.yaml red, and perturbing the printf format in the Job's $tctName does turn talos_reconcile_job_test.yaml red, so both suites bite. The gap is the fixture shape: the only pair of literals in kubernetes-nodes is rendered with instanceType: "", gpus: [] and kubelet: {}, and every one of those is falsy, so a divergence in how the Job assembles them is invisible. The parent chart does have the GPU shaped pair, and it works: I injected the same drop into cluster.yaml's hash input and talos_config_template_name_test.yaml went red on a machine-config change rotates the name the MachineDeployment references. So the coverage sits on the chart that cannot drift and is missing from the chart that can.

Two ways to close it and I would take the first. Derive the Job's $group from the same expression nodegroup.yaml uses, which removes the seam instead of watching it. Failing that, add a GPU shaped pair to talos_reconcile_job_test.yaml and nodegroup_test.yaml, which at least makes the next divergence fail.

While you are in that file, talos-reconcile-job.yaml:217 still says the computed locals are read as .group, .groupName, and this PR removed the set $jobContext "group" line. Nothing reads .group any more.

#3571

Merging #3571 into this head conflicts in four files, so it will not slip through unnoticed, but the obvious resolution is wrong in a way worth writing down before either of you does it. Keeping this branch's side of talos-reconcile-job.yaml, which is what you would reach for since this branch deleted that region, drops #3571's machine.kernel.modules out of the machine config entirely. I built that tree: with kernelModules: [nvidia, nvidia_uvm] the #3571 head renders kernel: modules: into the config, the merged tree renders no kernel block at all while the config itself renders fine. The per pool schematic goes half way as well: with schematicID set the merged tree emits the pool schematic on the boot disk from nodegroup.yaml, which auto merged, and the global default on the installer inside the machine config, which came from _talosconfigtemplate.tpl. That is precisely the mismatch #3571's own comment warns about, an in-place upgrade swapping the node's extension set out from under it.

The saving grace is that it is loud: helm unittest on that merged tree is 12 failed of 32, so anyone who runs the suite after resolving finds it. Same shape as the #3513 port, port the fields into _talosconfigtemplate.tpl rather than pick a side. This does not change the merge order, #3571 first or both together still looks right.

What I checked and did not find

The #3513 port is complete. I dedented main's inline machine-config block and diffed it against the helper body after normalising the variable renames the move forces, .talosVersion to $talosVersion and $.Values to $root.Values, plus the nindent depth. Four lines differ and all four are the two Helm comments, reworded to describe the new location. Every escaped interpolation is carried over: talosVersion in the quoted field, installerRepository, schematicID, talosVersion in the image tag, and toYaml . for the mirrors. The INVARIANT comment now names both charts, which is the right call given the file is copied.

Execution level agrees. hack/cozytest.sh hack/talos-reconcile-heredoc_test.bats exits 0 with five cases OK, and the hostile mirror endpoint comes back out of a real shell literally, in both charts.

The byte-identity claim holds against the main that exists now. I rendered the Job on origin/main and on this head with tests/values/common.yaml, pulled the heredoc out, parsed it as YAML and hashed the machine config object: identical, 469f3f1d. Positive control on the same instrument, the GPU render differs at 2a85738e and the diff is the nodeLabels block, so it does discriminate.

The README correction is accurate. Setting talos.registryMirrors rotates the name from 35293f to d53117, so the group does roll.

Reservations are in the hash: resources.memory from 4Gi to 16Gi rotates 35293f to a235e1. That is the mechanism behind the instancetype roll trigger you now describe in the upgrade note.

Nothing dangles after the move. Every variable the diff removed from $jobContext, kubeletVersion, talosVersion, podCIDR, serviceCIDR, mgmtClusterDomain and the six reservation locals, has zero remaining uses in either Job template; the one .group hit is the stale comment above. No consumer of the old fixed template name anywhere else in the tree either, the Go references are comments and the <cluster>-<group> literals in nodegroup.yaml and dashboard-resourcemap.yaml are MachineDeployment, MachineHealthCheck and WorkloadMonitor names.

The kubernetes-nodes Makefile change is safe: update is declared phony at line 3, so the added cp runs, and there is no file in the package directory that shadows a target.

A failed apply is loud rather than silent, which also covers the truncated hash. The container runs sh -ec and kubectl apply is last in its pipeline, so a webhook rejection aborts the Job instead of logging applied and exiting 0.

Base state on the branch: 209 tests in packages/apps/kubernetes, 15 in packages/apps/kubernetes-nodes, render-parity.sh reports GOLDEN PARITY.

Still open from earlier rounds

Pruning superseded templates and the hand maintained literal pairs are both unchanged and both were follow-ups. There are four such pairs now across the two charts. The bot's suggestion to add the notMatchRegex guard to kubernetes-nodes/tests/talos_reconcile_job_test.yaml, which the parent suite has, is still worth taking.

Not verified

No cluster, so everything in the upgrade-impact section about CAPI's rollout, the MachineHealthCheck interaction and your GPU fleet numbers is taken as reported. The instancetype roll trigger I confirmed only through the reservation path, not through an actual instancetype edit.

Blocker closed in 38b13f9, by the first of your two routes: the seam is gone rather than watched. I took the second one as well, for the reason below.

The assembly is now single

_helpers.tpl gains kubernetes-nodes.group, the one place the dict is built, and both nodegroup.yaml and talos-reconcile-job.yaml read it back with include "kubernetes-nodes.group" . | fromYaml — the same shape the parent chart already uses for include "kubernetes.nodeGroups" $ | fromYaml. A Helm template cannot return a dict and a Helm variable cannot cross a file boundary, so the YAML round-trip is what lets one assembly serve two files; it is also what the parent chart's groups already pass through, so if anything it converges the two charts' type handling rather than diverging it.

Your numbers reproduce exactly. On the clean tree the GPU-shaped pool gives TCT_NAME=kubernetes-myk8s-md0-0bbd2a and configRef.name=kubernetes-myk8s-md0-0bbd2a, the 0bbd2a you measured. And I re-ran your mutation against the new tree rather than reasoning about it: reintroducing the four-key dict in the Job without gpus, which previously left helm unittest . at 15 of 15 and render-parity.sh green, now fails — Tests: 1 failed, 16 passed, 17 total, in talos_reconcile_job_test.yaml.

The coverage is added anyway

The seam is removed, but it can be reintroduced by the next person who needs one more key in one of the two files, so your second route is worth having on top of the first. Your diagnosis of the fixture gap is the whole of it: the only literal pair in this chart rendered with instanceType: "", gpus: [] and kubelet: {}, every one falsy, so a dict carrying those keys and a dict omitting them render identically and the pair cannot tell them apart. So talos_reconcile_job_test.yaml and nodegroup_test.yaml each gain a GPU-shaped test, pinning kubernetes-myk8s-md0-0bbd2a as a shared literal the way the parent's suite does — 17 tests in the chart now, and the mutation above is what they catch.

The notMatchRegex guard against the old ${RELEASE}-${GROUP_NAME} name is taken too, so the assertion set on the Job matches the parent suite's.

The stale comment

It is in the parent chart, packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml:217, not in kubernetes-nodes — that chart's copy of the comment never named .group. Reworded to name the locals that are actually set (.groupName, .dnsDomain, .tctName, .tctSpec, .kubectlImage) and to say why the group is not among them: the machine config it feeds is rendered outside that template, into .tctSpec. The comment sits outside the define, so it is not in the Job's content hash and no golden name moves.

Verification

helm unittest 209 in packages/apps/kubernetes and 17 in packages/apps/kubernetes-nodes, render-parity.sh reports GOLDEN PARITY: all pool objects byte-identical across all cases, hack/talos-reconcile-heredoc_test.bats still 5 of 5 with the file unmodified, make generate clean in both packages.

Nothing rendered changed. The golden hashes are untouched — ffdbe1 on the kubernetes-nodes pair, 35293f on the parent's — which is the check that matters for the fromYaml round-trip: had it altered a type in a way the spec renders differently, the hash would have moved and this commit would have added a second worker roll on top of the one the PR already costs. It did not.

#3571

Agreed on both the diagnosis and the resolution, and worth having it written down before either of us reaches for the obvious merge. Port machine.kernel.modules and the per-pool schematicID into _talosconfigtemplate.tpl rather than keeping a side, same shape as the #3513 port. The half-applied schematic is the worse of the two failures: the pool value on the boot disk and the global default on the installer inside the machine config is exactly the in-place extension-set swap that PR's own comment warns about. I have not touched the merge order — #3571 first, or both together, still looks right, and I will do the port rather than resolve by picking.

Still open

Pruning superseded templates and the four hand-maintained literal pairs are unchanged and still follow-ups, not part of this PR.

Rebased onto main at a1b32f2e5, with your blocker resolved the way you asked and the three non-blocking points addressed. The body is rewritten rather than refreshed, because the ground moved under this PR: worker node pools left the parent chart for kubernetes-nodes while it sat, so everything it used to add under packages/apps/kubernetes/templates/talos/ is dropped and kubernetes-nodes is now the only home for the fix.

The blocker. Ported rather than resolved by picking, as you said it had to be. _talosconfigtemplate.tpl now carries the #3513 escaping on talosVersion, installerRepository and schematicID, the escaped machine.registries.mirrors block, and the INVARIANT comment above data:. hack/talos-reconcile-heredoc_test.bats passes — two cases now rather than five, since the parent chart no longer renders worker pools — which is the execution-level evidence that the escaping survived the move. make update no longer copies the helper from the parent chart: there is nothing to copy from any more, so that target was broken as written.

The name-agreement guard. Your point that the pair is only as good as its failure output was right, and it is the one I most wanted to fix. hack/check-talosconfigtemplate-name-agreement.bats renders the chart once and compares the Job's TCT_NAME against the MachineDeployment's configRef.name, so the failure names the invariant instead of a number. Dropping gpus at one call site now fails with talos-reconcile-job.yaml creates: …-0bbd2a against nodegroup.yaml references: …-ffdbe1, and a line saying to fix the shared input rather than re-pin either literal. It asserts on a GPU-shaped pool as well as the default one, since the default renders gpus [], kubelet {} and instanceType "" as falsy and a subset-reading consumer is indistinguishable there; a third case pins that the two shapes hash differently so the first two cannot pass vacuously.

The prune. Filed as #4355, pointing at the MachineSet enumeration in nodegroup.yaml rather than the cluster.yaml line you cited, which moved with the split. I noted two things worth settling before anyone implements it: whether a draining MachineSet's Machines can outlive it, and whether a suspended pool release can render with an empty lookup and prune everything at once.

The instancetype roll trigger is now in the upgrade note in the commit message as well as the body, alongside the registryMirrors one. The field's own documentation now says that changing a mirror rolls the pool; it said nothing before.

One thing the rebase turned up that is worth your attention: kubernetes-nodes.group was missing podCpuLimit and podCpuRequest, two keys main added after this branch opened. That is a live instance of exactly the failure this PR exists to prevent — the two consumers would have hashed different groups — and it is what the new guard would have caught. Both keys are carried now.

84 tests in kubernetes-nodes, 150 in kubernetes, golden hashes unchanged, make generate a no-op in both.