refactor(cilium): Flux is the only owner #709
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#709
Reference in New Issue
Block a user
Letzter Schritt aus #707. 3940 Zeilen weg. Talos trägt Cilium in keiner Form mehr, die HelmRelease ist der einzige Ort, an dem es konfiguriert ist.
Die Cluster-Seite ist bereits erledigt und belegt
upgrade-k8s --dry-runohne FlagDie letzte Zeile ist der Zweck der ganzen Übung: ein nackter
upgrade-k8shätte das CNI zerstört und tut ihm jetzt nichts.Der Ablauf: Inline-Manifest knotenweise entfernt (ohne Reboot, ohne Wirkung auf das laufende CNI), dann
upgrade-k8s --manifests-no-prune— derselbe Lauf, der Kubernetes hochgezogen hat. Cilium durchgehend Ready, BGP durchgehendestablishedmit je 5 Routen.Was ich dabei falsch geplant hatte
--manifests-no-pruneüberspringt nicht nur die Löschung, sondern auch die Fortschreibung des Inventars. Der Lauf, der die 22 Objekte gerettet hat, ließ Talos also weiterhin Anspruch darauf erheben — die Mine war vertagt, nicht entschärft. Mein Schritt 4 behauptete das Gegenteil.Talos bietet keinen Weg, einen veralteten Anspruch loszuwerden, außer dem Prune, der löscht. Also habe ich die 28 Einträge von Hand aus der ConfigMap entfernt.
Das ist Handarbeit an Cluster-Zustand, und ich benenne den Unterschied zu dem, was wir heute aufgeräumt haben: das Inventar ist abgeleitete Buchhaltung, keine Konfiguration, die ins Git gehört — und das Ergebnis ist in beide Richtungen prüfbar (28 → 0). Bei Konfiguration wäre das nicht akzeptabel gewesen.
Repo-Seite in diesem Commit
talos/patches/cilium-cni.yamlundtalos/cilium.yamlgelöschthelm installaus den HelmRelease-Werten, nach dem Bootstrap und vor Flux. Dass die Knoten bis dahinNotReadybleiben, steht jetzt ausdrücklich da — sonst sucht jemand den Fehler.RUNBOOK-baremetal-conversion.mdkorrigiert — es wies an, die Hälfte einer Datei zu extrahieren, die weg ist.bootstrap.shbleibt unangetastet. Seine Talos-Phase war tot und ist jetzt aus einem zweiten Grund tot; das Runbook sagt es.Der Preis, offen gesagt
Der Cluster formt sich nicht mehr allein aus dem Machine-Config. Ein Cold Rebuild braucht einen dokumentierten
helm install, den er vorher nicht brauchte. Das ist der Gegenwert dafür, dass Cilium an genau einer Stelle existiert.Final step of PROPOSAL-cilium-single-owner.md. Talos no longer carries Cilium in any form, and the HelmRelease is the only place it is configured. The cluster side is already done and verified: - the cilium inline manifest is gone from all three machine configs, applied node by node with no reboot and no effect on the running CNI - Kubernetes went 1.35.2 -> 1.36.3 in the same operation, with --manifests-no-prune so the 22 objects Talos claimed were not deleted - Talos's bootstrap-manifest inventory went from 41 entries to 13, and the 28 cilium/hubble entries are gone. A prune-enabled `upgrade-k8s --dry-run` reported 28 deletions before and reports 0 now That last number is the point of the whole exercise. A bare `upgrade-k8s` used to destroy the CNI and now does nothing to it. The inventory entries had to be removed by hand. --manifests-no-prune skips the prune AND the inventory rewrite, so the run that saved the objects also left Talos still claiming them — the mine was deferred, not cleared. Talos offers no way to drop a stale claim without the prune that deletes, so the ConfigMap was edited directly. It is derived bookkeeping, not configuration, and the result is verifiable in both directions, which is why this was acceptable where hand- editing configuration would not be. Repo side, this commit: - deletes talos/patches/cilium-cni.yaml and talos/cilium.yaml - adds Step 2b to RUNBOOK-cold-rebuild.md: install Cilium from the HelmRelease values with helm, after bootstrap and before Flux. The nodes stay NotReady until then, which the runbook now says out loud rather than leaving someone to wonder - drops the "keep in lockstep" and "THREE COPIES" comments, which described a duplication that no longer exists, and replaces them with one note saying where Cilium is configured - corrects RUNBOOK-baremetal-conversion.md, which told the reader to extract half of a file that is gone bootstrap.sh is left alone. Its talos phase was already dead and is now dead for a second reason; the runbook says so. The cost, stated plainly: the cluster no longer forms itself from the machine config. A cold rebuild needs one documented helm install that it did not need before. That is the price of Cilium existing in exactly one place.🤖 Automated review — model
CLI default(complexity: complex), commitbce155b8524f5c4a7aeeb0ada6b27f6c90eecbd8.Summary
This PR completes PROPOSAL-cilium-single-owner.md: it deletes the two Talos-side
copies of the Cilium manifest (
talos/cilium.yaml,talos/patches/cilium-cni.yaml,~3900 lines), updates the two runbooks to add a manual
helm installstep(Step 2b) before Flux takes over on a cold rebuild, and cleans up now-stale
"keep in lockstep" comments in
helmrelease.yaml. The intent and the live-clusterverification described in the PR body are sound, but the repo-side documentation
update has two concrete defects that would mislead an operator during an actual
disaster-recovery run.
🔴 Blocking
talos/patches/cluster.yamlnever gains thecni: none/proxy: disabledsettings the new docs claim it has. The deleted
talos/patches/cilium-cni.yamlwas the only place in the repo that set these two machine-config keys (see the
diff's second document in that file:
cluster.network.cni.name: none+cluster.proxy.disabled: true, alongside the inline manifest). This PR deletesthat file and drops it from the
talosctl gen configpatch list in bothrunbooks, but
kubernetes/talos/patches/cluster.yaml(checked in full) has nocniorproxykey, and a repo-wide grep forcni:/proxy:underkubernetes/talos/now returns nothing. Yet:kubernetes/RUNBOOK-cold-rebuild.md:168-170states "There is no CNI patchhere any more. Talos supplies
cni: noneandproxy: disabledfromcluster.yamland nothing else."kubernetes/RUNBOOK-baremetal-conversion.md:211-212makes the same claim("Talos supplies only
cni: noneandproxy: disabled").Neither claim is currently true. Following either runbook's
talosctl gen configcommand as written would leave Talos on its default CNI/kube-proxybehaviour (the previous file had to override the defaults, which is why it
existed), directly conflicting with Cilium's
kubeProxyReplacement: trueandthe "exactly one CNI" goal this PR exists to achieve — the opposite of the
intended outcome. The
cni: none/proxy: disabledkeys need to actually beadded to a git-tracked patch that both runbooks' commands still pass (most
naturally
talos/patches/cluster.yaml), not just asserted in prose.kubernetes/RUNBOOK-baremetal-conversion.md:201— thetalosctl gen configexample in the "Build the config" code block still contains--config-patch @<cilium additive half> \, a placeholder referring to adocument that used to be extracted from the now-deleted
talos/patches/cilium-cni.yaml. The parallel code block inRUNBOOK-cold-rebuild.md(Step 2) was correctly edited to drop this line, butthis one was missed. As written, the prose three lines below it ("There is no
CNI patch to pass any more") directly contradicts the command example an
operator would actually run.
🟡 Suggestions
cni: none/proxy: disabledare addedto
cluster.yaml, it would be worth a one-line note inPROPOSAL-cilium-single-owner.md's "Target state" section (currently onlyshows the intended YAML snippet in the abstract) pointing at the concrete file
it now lives in, so the proposal and the runbooks don't drift again the way
the three-copies problem did originally.
🟢 Nits
None beyond the above.
Tests
Not applicable — this is a documentation/manifest-deletion change with no
automated test coverage in this repo for runbook accuracy or Talos config
generation. The PR body describes manual verification of the live-cluster side
(machine configs, inventory ConfigMap,
upgrade-k8s --dry-run), which iscredible and appropriately called out as the risky, hand-applied part. The
repo-side gap above (missing
cni/proxysettings) is exactly the kind ofthing that verification didn't and couldn't catch, since the live cluster was
patched by hand rather than via a fresh
talosctl gen configfrom this repo'spatches — a cold-rebuild dry run (
talosctl gen config+talosctl validateusing the exact commands in the runbook) would have caught it.
Recommendation
Request changes — 2 blocking issues.
🤖 Automated review — model
CLI default(complexity: complex), commit1f18599b4c077202d05a5826cb388d41b685c974.Summary
This PR finishes making the Cilium HelmRelease the single source of truth for CNI configuration: it deletes the two Talos-side copies (
talos/cilium.yaml, the rendered inline manifest, andtalos/patches/cilium-cni.yaml, the additive/delete patch pair), moves the two settings Talos still needs (cluster.network.cni.name: none,cluster.proxy.disabled: true) intotalos/patches/cluster.yaml, and rewrites the cold-rebuild and baremetal-conversion runbooks to install Cilium via a documentedhelm installstep (new Step 2b) instead of relying on a Talos inline manifest. I traced every changed and deleted file against the rest of the repo (includingbootstrap.sh, CI configs, and other doc cross-references) and the change is internally consistent: the survivingcluster.yamlkeys use valid Talos v1alpha1 paths, no other kustomization/config still points at the deleted files, and the new runbook steps line up with the HelmRelease's namespace, chart version range, and values. No blocking issues found.🟡 Suggestions
kubernetes/bootstrap.sh:86,126still reference the now-deletedtalos/patches/cilium-cni.yaml. Running./bootstrap.sh talos(or the default./bootstrap.shwith no phase argument) will now hard-fail at the[[ -f ... ]] || fail ...check on line 86. This isn't a regression introduced by this PR — the talos phase was already broken per issue #581 and is already flagged dead inRUNBOOK-baremetal-conversion.md:417and the updatedRUNBOOK-cold-rebuild.md:139, and the PR body explicitly calls out leavingbootstrap.shuntouched as a deliberate choice. Still, since the reference is now to a file that no longer exists at all (not just unusable), it may be worth a follow-up to either delete the deadtalosphase from the script or have it fail immediately with a message pointing at the runbook, rather than leaving a stale path check.RUNBOOK-cold-rebuild.md:233pinshelm install ... --version 1.20.1while the HelmRelease (kubernetes/infrastructure/cilium/helmrelease.yaml:24) tracks>=1.20.0 <1.21.0. The runbook already warns "Pin--versionto whatever the HelmRelease range currently resolves to," so this is self-documented, but a hardcoded version in a runbook will silently drift out of the allowed range over time with no CI check to catch it.Tests
This is docs/config-only (no application code), so there's no automated test suite to extend. The PR body describes the verification the author actually performed against the live cluster (machine-config inventory counts,
upgrade-k8s --dry-runbefore/after, BGP/Cilium stayingReadythrough the migration), which is the appropriate form of validation for this kind of change and is more convincing than a static review can be. I could not independently verify those cluster-side claims (no access to the live cluster from this checkout) — the repo-side consistency is what I checked instead.Recommendation
Looks reasonable — no blocking issues; comments above are optional.
The install command in Step 2b hardcoded --version 1.20.1 and the chart repo URL. That is a copy of the HelmRelease in a text file nobody runs between rebuilds, so it would have gone stale unnoticed and the next rebuild would have installed something Flux immediately fought — the same failure the deleted bootstrap seed produced, in a new place. Everything now comes out of helmrelease.yaml at the moment the command runs: the repo URL from the HelmRepository, the values from .spec.values, and --version from .spec.chart.spec.version verbatim. Helm accepts the semver range and resolves it exactly as Flux does, so the two cannot pick different releases. Verified by running the block as written: --version ">=1.20.0 <1.21.0" -> cilium:v1.20.1, operator-generic:v1.20.1 enable-bgp-control-plane: "true" bpf-lb-sock: "true" bpf-policy-map-max: "65536" ipv4-native-routing-cidr: 10.0.0.0/8 ServiceMonitor objects: 0 which is the config the cluster is running. The only literal left is --set prometheus.serviceMonitor.enabled=false, and it is a bootstrap constraint rather than a value: the CRD arrives with the monitoring stack long afterwards. Because nothing else is overridden, helm-controller adopts this release instead of replacing it. This does not make the step unnecessary. Flux's controllers are pods and cannot reconcile a CNI they need in order to run; something has to seed it. What it does mean is that the step has nothing left in it to maintain.🤖 Automated review — model
CLI default(complexity: complex), commit17d4c55485931742e29ea42718060013018214c7.Summary
This PR finishes the Cilium single-owner migration (step 6 of
PROPOSAL-cilium-single-owner.md, follow-up to #707): it deletes the two Talos-side Cilium manifests (talos/cilium.yaml,talos/patches/cilium-cni.yaml, ~3900 lines of rendered/templated YAML), moves the two settings Talos genuinely needs (cluster.network.cni.name: none,cluster.proxy.disabled: true) intotalos/patches/cluster.yaml, removes now-stale "keep in lockstep" comments fromhelmrelease.yaml, and rewrites the cold-rebuild and baremetal-conversion runbooks to install Cilium viahelm installfrom the HelmRelease values as a new Step 2b. No blocking issues found — the schema placement, the deleted content, and the runbook's helm/yq commands all check out against what's actually in the repo.🟡 Suggestions
kubernetes/bootstrap.sh:86and:126still hard-referencepatches/cilium-cni.yaml, which this PR deletes. Running./bootstrap.sh talos(or./bootstrap.shwith no args) will now fail immediately at the[[ -f ... ]] || fail "cilium-cni.yaml patch not found"check instead of failing later duringtalosctl gen config(the pre-existing #581 failure the runbooks already documented). The PR description explicitly acknowledges this phase was already dead and stays dead "for a second reason," which the runbook now states — so this isn't a regression in practical terms, but the script itself gives no hint of why, and a future reader ofbootstrap.shalone (without also readingRUNBOOK-cold-rebuild.md) will hit a confusing "file not found" for a file that was deliberately removed. Worth either pointing thefailmessage at the runbook or stripping the deadtalosphase in a follow-up.Tests
No test suite applies here — this is documentation plus declarative Talos/Helm config. The safety net is operational (described in the PR body: inventory went 41→13 entries,
upgrade-k8s --dry-runwent from 28 deletions to 0, Cilium/BGP stayed up through the migration), which is appropriate for this kind of change and not something a unit test could cover.Recommendation
Looks reasonable — no blocking issues; comment above is optional.