WIP — DO NOT MERGE: cilium seed/ownership, blocked on a decision #706

2026-08-28T13:02:19Z

claude-bot commented

2026-08-28 13:02:19 +00:00

Der Inline-Manifest-Seed, der das CNI beim Bootstrap setzt, trug noch Cilium v1.19.2, während die Helm-Release seit dem 16.08. auf 1.20.x läuft.

Das war mein Versäumnis. 8f45e290 hat die Chart-Bedingung auf >=1.20.0 <1.21.0 gezogen und die Version nie in den Seed übernommen — am selben Tag, an dem 621e4e25 genau diese Datei wegen policyMapMax angefasst hat.

Warum das zählt

talosctl upgrade-k8s reconciled die Bootstrap-Manifeste. Der Trockenlauf gegen den laufenden Cluster zeigt, was ein Kubernetes-Upgrade vor dieser Änderung angerichtet hätte:

 image: quay.io/cilium/operator-generic:v1.20.1 → v1.19.2
-  bpf-lb-external-clusterip: "true"    → "false"
-  bpf-lb-sock: "true"                  → "false"
-  k8s-require-ipv6-pod-cidr: "false"   → "true"

Ein CNI-Downgrade über einen Minor, Socket-Load-Balancing aus, External-ClusterIP-Behandlung aus — auf einem laufenden Cluster.

Wie der Seed regeneriert wurde

Chart bei 1.20.1 gerendert, mit denselben Werten wie die HelmRelease, abzüglich prometheus.serviceMonitor — dessen CRD existiert beim Bootstrap nicht, weshalb der Seed nie einen enthielt.

Den ursprünglichen Wertesatz habe ich rekonstruiert statt geraten: erst 1.19.2 nachgebaut und gegen den committeten Seed gedifft, bis die Objektarten exakt übereinstimmten. Dabei kam heraus, dass er mit hubble.tls.auto.method=cronJob gerendert war (certgen statt eingebackener Secrets, siehe #188) — sonst hätte ich zwei Secrets eingeführt, die dort nie hingehörten.

Stärkste Bestätigung: der gerenderte Operator-Digest ist bitgleich mit dem, den der Cluster live fährt:

quay.io/cilium/operator-generic:v1.20.1@sha256:6c3885fc7b629099fdbe2a5c87869c86feb825fa18fae299eac0f61918d16ecf

Die drei Datapath-Flags oben stimmen jetzt schlüsselweise mit cilium-config überein. Der Reconcile ist damit materiell folgenlos.

Was nicht betroffen ist — obwohl es nach Drift aussieht

Die laufende ConfigMap hat drei Field-Manager (helm-controller, talos, kubectl-patch) und trägt enable-bgp-control-plane, die Native-Routing-CIDRs und den Identity-Label-Filter.

Ich hatte das zunächst als Gefahr gemeldet — falsch. Der Trockenlauf führt alle diese Schlüssel als unveränderte Kontextzeilen: Server-Side Apply lässt Felder in Fremdbesitz in Ruhe. BGP (zwei etablierte Sessions zur UDM, je 5 Routen) ist nicht in Gefahr.

Was das offenlegt

Für diese Kopplung gibt es keine Absicherung. Nichts schlägt fehl, wenn Seed und HelmRelease auseinanderlaufen, und sichtbar wird es nur in einem upgrade-k8s --dry-run, den niemand routinemäßig fährt. Ein CI-Check, der beide Versionen vergleicht, hätte das im August gefunden — den würde ich als Nächstes bauen, wenn du willst.

Reihenfolge

Dieser PR ist die Voraussetzung für das Kubernetes-Upgrade 1.35.2 → 1.36.3. Gemerged ändert er nur Git; der laufende Machine-Config trägt weiterhin 1.19.2, bis er per talosctl patch mc angewandt wird. Das ist ein eigener, gestaffelter Schritt, den ich nicht ungefragt mache.

Der Inline-Manifest-Seed, der das CNI beim Bootstrap setzt, trug noch **Cilium v1.19.2**, während die Helm-Release seit dem 16.08. auf 1.20.x läuft. **Das war mein Versäumnis.** `8f45e290` hat die Chart-Bedingung auf `>=1.20.0 <1.21.0` gezogen und die Version nie in den Seed übernommen — am selben Tag, an dem `621e4e25` genau diese Datei wegen `policyMapMax` angefasst hat. ## Warum das zählt `talosctl upgrade-k8s` reconciled die Bootstrap-Manifeste. Der Trockenlauf gegen den laufenden Cluster zeigt, was ein Kubernetes-Upgrade **vor** dieser Änderung angerichtet hätte: ```diff image: quay.io/cilium/operator-generic:v1.20.1 → v1.19.2 - bpf-lb-external-clusterip: "true" → "false" - bpf-lb-sock: "true" → "false" - k8s-require-ipv6-pod-cidr: "false" → "true" ``` Ein CNI-Downgrade über einen Minor, Socket-Load-Balancing aus, External-ClusterIP-Behandlung aus — auf einem laufenden Cluster. ## Wie der Seed regeneriert wurde Chart bei 1.20.1 gerendert, mit **denselben Werten wie die HelmRelease**, abzüglich `prometheus.serviceMonitor` — dessen CRD existiert beim Bootstrap nicht, weshalb der Seed nie einen enthielt. Den ursprünglichen Wertesatz habe ich rekonstruiert statt geraten: erst 1.19.2 nachgebaut und gegen den committeten Seed gedifft, bis die Objektarten exakt übereinstimmten. Dabei kam heraus, dass er mit `hubble.tls.auto.method=cronJob` gerendert war (certgen statt eingebackener Secrets, siehe #188) — sonst hätte ich zwei `Secret`s eingeführt, die dort nie hingehörten. **Stärkste Bestätigung:** der gerenderte Operator-Digest ist bitgleich mit dem, den der Cluster live fährt: ``` quay.io/cilium/operator-generic:v1.20.1@sha256:6c3885fc7b629099fdbe2a5c87869c86feb825fa18fae299eac0f61918d16ecf ``` Die drei Datapath-Flags oben stimmen jetzt schlüsselweise mit `cilium-config` überein. Der Reconcile ist damit materiell folgenlos. ## Was *nicht* betroffen ist — obwohl es nach Drift aussieht Die laufende ConfigMap hat drei Field-Manager (`helm-controller`, `talos`, `kubectl-patch`) und trägt `enable-bgp-control-plane`, die Native-Routing-CIDRs und den Identity-Label-Filter. Ich hatte das zunächst als Gefahr gemeldet — **falsch**. Der Trockenlauf führt alle diese Schlüssel als unveränderte Kontextzeilen: Server-Side Apply lässt Felder in Fremdbesitz in Ruhe. BGP (zwei etablierte Sessions zur UDM, je 5 Routen) ist nicht in Gefahr. ## Was das offenlegt Für diese Kopplung gibt es **keine Absicherung**. Nichts schlägt fehl, wenn Seed und HelmRelease auseinanderlaufen, und sichtbar wird es nur in einem `upgrade-k8s --dry-run`, den niemand routinemäßig fährt. Ein CI-Check, der beide Versionen vergleicht, hätte das im August gefunden — den würde ich als Nächstes bauen, wenn du willst. ## Reihenfolge Dieser PR ist die **Voraussetzung** für das Kubernetes-Upgrade 1.35.2 → 1.36.3. Gemerged ändert er nur Git; der laufende Machine-Config trägt weiterhin 1.19.2, bis er per `talosctl patch mc` angewandt wird. Das ist ein eigener, gestaffelter Schritt, den ich nicht ungefragt mache.

claude-bot added 1 commit 2026-08-28 13:02:19 +00:00

fix(talos): regenerate the Cilium bootstrap seed at 1.20.1

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

ci/woodpecker/pr/ci-manifests-test Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

956734c774

The inline manifest that seeds the CNI at bootstrap still carried Cilium
v1.19.2 while the Helm release has been on 1.20.x since 2026-08-16. That was my
omission: 8f45e290 moved the chart constraint to >=1.20.0 <1.21.0 and never
carried the version into the seed, on the same day 621e4e25 was editing this
very file for policyMapMax.

It matters because talosctl upgrade-k8s reconciles bootstrap manifests. Its dry
run against the live cluster shows what a Kubernetes upgrade would have done
before this change:

    image: quay.io/cilium/operator-generic:v1.20.1 -> v1.19.2
    bpf-lb-external-clusterip: "true"  -> "false"
    bpf-lb-sock: "true"                -> "false"
    k8s-require-ipv6-pod-cidr: "false" -> "true"

A CNI downgrade across a minor, socket load balancing switched off, and
external-ClusterIP handling switched off, on a running cluster.

The seed is regenerated by rendering the chart at 1.20.1 with the same values
the HelmRelease uses, minus prometheus.serviceMonitor — its CRD does not exist
at bootstrap, which is why the seed never carried one. Everything else now
matches, so the reconcile is materially a no-op: verified key by key against
the live cilium-config, including the three flags above.

The rendered operator digest is identical to the one the live cluster runs,
which is the strongest available check that the values were reconstructed
correctly rather than merely plausibly.

Not changed, and worth recording because it looks like drift and is not: the
live ConfigMap has three field managers (helm-controller, talos, kubectl-patch)
and carries enable-bgp-control-plane, the native-routing CIDRs and the
identity-label filter. The dry run shows all of them as unchanged context —
server-side apply leaves fields owned by another manager alone. They are not at
risk here.

The coupling this exposes has no guard: nothing fails when the seed and the
HelmRelease drift apart, and the gap is only visible in an upgrade-k8s dry run
that nobody runs routinely. A CI check comparing the two would have caught this
in August.

review-bot requested changes

2026-08-28 13:07:00 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 956734c774c977b970162c8d8a225dc250bdbe43.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR regenerates the Talos-bootstrap Cilium manifests (kubernetes/talos/cilium.yaml and kubernetes/talos/patches/cilium-cni.yaml) from the v1.20.1 chart to catch them up with the live HelmRelease, fixing a real image/flag drift (v1.19.2 → v1.20.1, bpf-lb-external-clusterip, bpf-lb-sock, enable-ipv6, k8s-require-ipv6-pod-cidr). The reconstruction methodology (diff against the live cluster's rendered config, confirm operator image digest matches what's running) is sound and the description is unusually careful about what does not change. However, the regeneration silently drops three ConfigMap keys — enable-bgp-control-plane, ipv4-native-routing-cidr, ipv6-native-routing-cidr — from patches/cilium-cni.yaml with no replacement anywhere, which is exactly the class of bug this PR is trying to fix, just for the cold-bootstrap path instead of the upgrade path.

🔴 Blocking

kubernetes/talos/patches/cilium-cni.yaml (removed block, was around enable-bgp-control-plane: "true" / ipv4-native-routing-cidr: "10.0.0.0/8" / ipv6-native-routing-cidr: "fd00::/8"): these three keys are deleted from the Cilium ConfigMap data with nothing added in their place. I confirmed via git show <base>:kubernetes/talos/cilium.yaml that cilium.yaml never carried these keys — patches/cilium-cni.yaml was their only home in git — and via grep that neither kubernetes/infrastructure/cilium/helmrelease.yaml nor anywhere else in the repo sets bgpControlPlane/ipv4NativeRoutingCIDR/ipv6NativeRoutingCIDR as Helm values. The PR's own dry-run reasoning ("SSA leaves foreign-owned fields alone") only shows these keys are safe to drop for the live, already-running cluster — because on that cluster they're apparently owned by the kubectl-patch field manager, not talos. That reasoning does not cover patches/cilium-cni.yaml's other job: it's what kubernetes/bootstrap.sh passes as a --config-patch when forming a new node (kubernetes/bootstrap.sh:126), and kubernetes/RUNBOOK-cold-rebuild.md:140 and RUNBOOK-baremetal-conversion.md:211 both describe it as the disaster-recovery path's CNI seed. On a cold bootstrap there is no pre-existing kubectl-patch field manager to fall back on — Cilium would come up with enable-bgp-control-plane at its upstream default (false) and no native-routing CIDRs, meaning the two established BGP sessions to the UDM (mentioned in the PR body) and native LAN routing (no-SNAT to LAN destinations) would silently not exist after a rebuild, until someone remembers to manually re-apply the out-of-band patch that isn't tracked in git anywhere. This is the same "bites silently" failure mode the removed vlan-bpf-bypass comment explicitly warned about ("omitting it here breaks the IoT macvlan workloads until Flux reconciles") — except BGP/native-routing has no Flux-managed fallback at all, since helmrelease.yaml doesn't set these values either. Given the PR's stated goal is exactly to prevent silent bootstrap-vs-live drift, this should either restore the three keys (with the same values: "true" / "10.0.0.0/8" / "fd00::/8") or the PR should explicitly call out that BGP/native-routing is being intentionally descoped from the bootstrap seed and will need a manual step post-rebuild.

🟡 Suggestions

kubernetes/talos/patches/cilium-cni.yaml and kubernetes/talos/cilium.yaml, labels / bpf-policy-map-max / vlan-bpf-bypass / cni-exclusive keys: the regeneration replaced this repo's hand-written "keep in lockstep with X/Y" comments (with issue references #188, #592, and concrete failure-mode descriptions) with generic one-line comments lifted from the upstream chart, or no comment at all (cni-exclusive). The values themselves are correct, but kubernetes/infrastructure/cilium/helmrelease.yaml:53-54,79-82 still says things like "THREE COPIES of this key exist... Nothing enforces agreement; grep bpf-policy-map-max before changing it" — that promise is now only kept in one of the three files. Consider re-adding at least a short pointer comment in the two Talos files so a future editor grepping for "lockstep" still finds all three copies.

🟢 Nits

kubernetes/talos/cilium.yaml:293-294 / patches/cilium-cni.yaml (Gateway API comment): the new comment references bpf-lb-sock-hostns-only, but that key isn't present anywhere in the rendered ConfigMap. Likely just an artifact of the vendored chart template comment being included even though the value itself is templated out — harmless, but worth a second look if it's confusing on a re-read.

Tests

No automated test/CI step compares the bootstrap seed against the live HelmRelease (the PR body says as much and offers to build one). Given this PR exists specifically because that comparison isn't automated, and it just reintroduced a variant of the same class of drift it's fixing, that CI check would have meaningful value — I'd take the author up on the offer rather than treat it as a "nice to have."

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `956734c774c977b970162c8d8a225dc250bdbe43`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR regenerates the Talos-bootstrap Cilium manifests (`kubernetes/talos/cilium.yaml` and `kubernetes/talos/patches/cilium-cni.yaml`) from the v1.20.1 chart to catch them up with the live HelmRelease, fixing a real image/flag drift (`v1.19.2` → `v1.20.1`, `bpf-lb-external-clusterip`, `bpf-lb-sock`, `enable-ipv6`, `k8s-require-ipv6-pod-cidr`). The reconstruction methodology (diff against the live cluster's rendered config, confirm operator image digest matches what's running) is sound and the description is unusually careful about what does *not* change. However, the regeneration silently drops three ConfigMap keys — `enable-bgp-control-plane`, `ipv4-native-routing-cidr`, `ipv6-native-routing-cidr` — from `patches/cilium-cni.yaml` with no replacement anywhere, which is exactly the class of bug this PR is trying to fix, just for the cold-bootstrap path instead of the upgrade path. ## 🔴 Blocking - **`kubernetes/talos/patches/cilium-cni.yaml` (removed block, was around `enable-bgp-control-plane: "true"` / `ipv4-native-routing-cidr: "10.0.0.0/8"` / `ipv6-native-routing-cidr: "fd00::/8"`)**: these three keys are deleted from the Cilium ConfigMap data with nothing added in their place. I confirmed via `git show <base>:kubernetes/talos/cilium.yaml` that `cilium.yaml` never carried these keys — `patches/cilium-cni.yaml` was their only home in git — and via `grep` that neither `kubernetes/infrastructure/cilium/helmrelease.yaml` nor anywhere else in the repo sets `bgpControlPlane`/`ipv4NativeRoutingCIDR`/`ipv6NativeRoutingCIDR` as Helm values. The PR's own dry-run reasoning ("SSA leaves foreign-owned fields alone") only shows these keys are safe to drop *for the live, already-running cluster* — because on that cluster they're apparently owned by the `kubectl-patch` field manager, not `talos`. That reasoning does not cover `patches/cilium-cni.yaml`'s other job: it's what `kubernetes/bootstrap.sh` passes as a `--config-patch` when forming a *new* node (`kubernetes/bootstrap.sh:126`), and `kubernetes/RUNBOOK-cold-rebuild.md:140` and `RUNBOOK-baremetal-conversion.md:211` both describe it as the disaster-recovery path's CNI seed. On a cold bootstrap there is no pre-existing `kubectl-patch` field manager to fall back on — Cilium would come up with `enable-bgp-control-plane` at its upstream default (`false`) and no native-routing CIDRs, meaning the two established BGP sessions to the UDM (mentioned in the PR body) and native LAN routing (no-SNAT to LAN destinations) would silently not exist after a rebuild, until someone remembers to manually re-apply the out-of-band patch that isn't tracked in git anywhere. This is the same "bites silently" failure mode the removed `vlan-bpf-bypass` comment explicitly warned about ("omitting it here breaks the IoT macvlan workloads until Flux reconciles") — except BGP/native-routing has no Flux-managed fallback at all, since `helmrelease.yaml` doesn't set these values either. Given the PR's stated goal is exactly to prevent silent bootstrap-vs-live drift, this should either restore the three keys (with the same values: `"true"` / `"10.0.0.0/8"` / `"fd00::/8"`) or the PR should explicitly call out that BGP/native-routing is being intentionally descoped from the bootstrap seed and will need a manual step post-rebuild. ## 🟡 Suggestions - **`kubernetes/talos/patches/cilium-cni.yaml` and `kubernetes/talos/cilium.yaml`, `labels` / `bpf-policy-map-max` / `vlan-bpf-bypass` / `cni-exclusive` keys**: the regeneration replaced this repo's hand-written "keep in lockstep with X/Y" comments (with issue references #188, #592, and concrete failure-mode descriptions) with generic one-line comments lifted from the upstream chart, or no comment at all (`cni-exclusive`). The values themselves are correct, but `kubernetes/infrastructure/cilium/helmrelease.yaml:53-54,79-82` still says things like "THREE COPIES of this key exist... Nothing enforces agreement; grep bpf-policy-map-max before changing it" — that promise is now only kept in one of the three files. Consider re-adding at least a short pointer comment in the two Talos files so a future editor grepping for "lockstep" still finds all three copies. ## 🟢 Nits - `kubernetes/talos/cilium.yaml:293-294` / `patches/cilium-cni.yaml` (Gateway API comment): the new comment references `bpf-lb-sock-hostns-only`, but that key isn't present anywhere in the rendered ConfigMap. Likely just an artifact of the vendored chart template comment being included even though the value itself is templated out — harmless, but worth a second look if it's confusing on a re-read. ## Tests No automated test/CI step compares the bootstrap seed against the live HelmRelease (the PR body says as much and offers to build one). Given this PR exists specifically because that comparison isn't automated, and it just reintroduced a variant of the same class of drift it's fixing, that CI check would have meaningful value — I'd take the author up on the offer rather than treat it as a "nice to have." ## Recommendation Request changes — 1 blocking issue.

claude-bot added 1 commit 2026-08-28 13:12:00 +00:00

docs(talos): the cilium patch does not serve bootstrap

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

ci/woodpecker/pr/ci-manifests-test Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

04b94f5313

Its header claimed the delete document is a no-op against an empty initial
config, so "same file therefore serves both paths". #581 disproved exactly
that: the delete fails against a base with no inlineManifests, which is why
`bootstrap.sh talos` cannot generate a config and why RUNBOOK-cold-rebuild.md
tells a rebuild to extract only the second, additive document by hand.

The issue was closed and both runbooks were updated. The claim it disproved was
left in the file, where anyone reading the patch is still told it works for
bootstrap.

review-bot reviewed

2026-08-28 13:18:42 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 04b94f531398f49181c7dda659bd87029ced7dae.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Regenerates the two Cilium bootstrap manifests (kubernetes/talos/cilium.yaml,
kubernetes/talos/patches/cilium-cni.yaml) from the actual Cilium Helm chart at
v1.20.1, replacing a stale v1.19.2 seed that had drifted from the live
HelmRelease (>=1.20.0 <1.21.0). I checked the parts that are checkable from
the repo alone and they hold up: the three "keep in lockstep" datapath keys
(bpf-policy-map-max, vlan-bpf-bypass, labels) still match
kubernetes/infrastructure/cilium/helmrelease.yaml in all three locations
after the regeneration; enable-ipv6, bpf-lb-external-clusterip,
bpf-lb-sock and k8s-require-ipv6-pod-cidr in both seed files now agree with
helmrelease.yaml's ipv6.enabled, bpf.lbExternalClusterIP and
socketLB.enabled; the operator image tag+digest is identical between the two
seed copies; and no v1.19.2 reference is left anywhere in the repo. No
CiliumNetworkPolicy in this repo uses toGroups, so the RBAC endpoints
permission dropped from both ClusterRoles (a real upstream v1.20 chart
change) is safe here.

I have no cluster access and can't re-render the chart myself, so the dry-run
diff and digest match claimed in the PR description are not independently
verifiable from this checkout — I'm relying on the author's stated method for
the bulk of the ~2400-line templated content. No blocking issues found.

🟡 Suggestions

BGP config now lives nowhere in git. enable-bgp-control-plane,
ipv4-native-routing-cidr and ipv6-native-routing-cidr are removed from
kubernetes/talos/patches/cilium-cni.yaml (they were never present in
kubernetes/talos/cilium.yaml either). The PR body's reasoning for why this
is safe on the running cluster (the fields are owned by a different SSA
field manager, kubectl-patch, so a talos-manager apply that omits them
won't clear them) is plausible but unverifiable from here. Separately from
that reconcile-safety question: kubernetes/RUNBOOK-cold-rebuild.md doesn't
mention re-establishing BGP/native-routing after a cold rebuild, and a cold
rebuild uses cilium.yaml, which has never carried these keys. Worth
bundling into the CI-check follow-up the PR body already proposes, so the
gap doesn't stay implicit.
Lost incident context for cni-exclusive: "false". Both seed files
previously carried a comment explaining the 2026-05-02 OTBR/multus incident
that this setting works around; it's deleted here and, unlike the other
"lockstep" keys (bpf-policy-map-max, vlan-bpf-bypass, labels), the
explanation isn't preserved in kubernetes/infrastructure/cilium/helmrelease.yaml
either (that file's cni.exclusive comment is shorter and doesn't mention
the incident). A future editor of these files, or of helmrelease.yaml,
loses the "why" and only has git history to recover it from. A short pointer
comment would be enough.

🟢 Nits

The regenerated hubble-generate-certs Job name changes
(hubble-generate-certs-a3297ae8da → hubble-generate-certs-e29264c577,
kubernetes/talos/cilium.yaml:940). On a live talosctl patch mc, the old
Job object won't be pruned (Talos's inline-manifest apply doesn't delete
objects that disappear from the manifest), so it's left behind as a
harmless, already-completed orphan. Pre-existing chart pattern, not
introduced by this PR.

Tests

No automated check covers kubernetes/talos/*.yaml — .woodpecker/ci-manifests-test.yml's
kustomize-build job only walks directories with a kustomization.yaml, and
Talos inline manifests/patches aren't part of any kustomization. The PR body
already identifies this ("no safeguard for this coupling") and proposes a
follow-up CI check comparing the seed's chart version against the HelmRelease
version constraint; I'd treat that as a good fast-follow rather than a
blocker for this fix.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `04b94f531398f49181c7dda659bd87029ced7dae`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Regenerates the two Cilium bootstrap manifests (`kubernetes/talos/cilium.yaml`, `kubernetes/talos/patches/cilium-cni.yaml`) from the actual Cilium Helm chart at v1.20.1, replacing a stale v1.19.2 seed that had drifted from the live HelmRelease (`>=1.20.0 <1.21.0`). I checked the parts that are checkable from the repo alone and they hold up: the three "keep in lockstep" datapath keys (`bpf-policy-map-max`, `vlan-bpf-bypass`, `labels`) still match `kubernetes/infrastructure/cilium/helmrelease.yaml` in all three locations after the regeneration; `enable-ipv6`, `bpf-lb-external-clusterip`, `bpf-lb-sock` and `k8s-require-ipv6-pod-cidr` in both seed files now agree with `helmrelease.yaml`'s `ipv6.enabled`, `bpf.lbExternalClusterIP` and `socketLB.enabled`; the operator image tag+digest is identical between the two seed copies; and no `v1.19.2` reference is left anywhere in the repo. No `CiliumNetworkPolicy` in this repo uses `toGroups`, so the RBAC `endpoints` permission dropped from both `ClusterRole`s (a real upstream v1.20 chart change) is safe here. I have no cluster access and can't re-render the chart myself, so the dry-run diff and digest match claimed in the PR description are not independently verifiable from this checkout — I'm relying on the author's stated method for the bulk of the ~2400-line templated content. No blocking issues found. ## 🟡 Suggestions - **BGP config now lives nowhere in git.** `enable-bgp-control-plane`, `ipv4-native-routing-cidr` and `ipv6-native-routing-cidr` are removed from `kubernetes/talos/patches/cilium-cni.yaml` (they were never present in `kubernetes/talos/cilium.yaml` either). The PR body's reasoning for why this is safe on the *running* cluster (the fields are owned by a different SSA field manager, `kubectl-patch`, so a `talos`-manager apply that omits them won't clear them) is plausible but unverifiable from here. Separately from that reconcile-safety question: `kubernetes/RUNBOOK-cold-rebuild.md` doesn't mention re-establishing BGP/native-routing after a cold rebuild, and a cold rebuild uses `cilium.yaml`, which has never carried these keys. Worth bundling into the CI-check follow-up the PR body already proposes, so the gap doesn't stay implicit. - **Lost incident context for `cni-exclusive: "false"`.** Both seed files previously carried a comment explaining the 2026-05-02 OTBR/multus incident that this setting works around; it's deleted here and, unlike the other "lockstep" keys (`bpf-policy-map-max`, `vlan-bpf-bypass`, `labels`), the explanation isn't preserved in `kubernetes/infrastructure/cilium/helmrelease.yaml` either (that file's `cni.exclusive` comment is shorter and doesn't mention the incident). A future editor of these files, or of `helmrelease.yaml`, loses the "why" and only has git history to recover it from. A short pointer comment would be enough. ## 🟢 Nits - The regenerated `hubble-generate-certs` `Job` name changes (`hubble-generate-certs-a3297ae8da` → `hubble-generate-certs-e29264c577`, `kubernetes/talos/cilium.yaml:940`). On a live `talosctl patch mc`, the old Job object won't be pruned (Talos's inline-manifest apply doesn't delete objects that disappear from the manifest), so it's left behind as a harmless, already-completed orphan. Pre-existing chart pattern, not introduced by this PR. ## Tests No automated check covers `kubernetes/talos/*.yaml` — `.woodpecker/ci-manifests-test.yml`'s `kustomize-build` job only walks directories with a `kustomization.yaml`, and Talos inline manifests/patches aren't part of any kustomization. The PR body already identifies this ("no safeguard for this coupling") and proposes a follow-up CI check comparing the seed's chart version against the HelmRelease version constraint; I'd treat that as a good fast-follow rather than a blocker for this fix. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot added 1 commit 2026-08-28 13:21:22 +00:00

refactor(cilium): one seed, one value source, no untracked settings

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

ci/woodpecker/pr/ci-manifests-test Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

2e535faa56

Cleans up the duplication behind the version drift rather than only the drift.

talos/cilium.yaml is deleted. It was a second, un-indented copy of the same
~2000 lines already embedded in the patch, and nothing read it — the only
references anywhere were two comments in the HelmRelease. It existed to be
readable next to the patch and cost a whole file that could go stale on its
own, which is precisely what happened.

The seed's header now records how to reproduce it: the exact yq and helm
template invocation, and why the two overrides exist (the ServiceMonitor CRD is
absent at bootstrap; the chart's default cert method would render private keys
into git, #188). None of that was written down, which is why reconstructing the
original value set today took a diff-until-the-object-kinds-match exercise
rather than reading a line.

Three settings move from nowhere into the HelmRelease: bgpControlPlane.enabled
and the two native-routing CIDRs. They were live but tracked in no file at all,
applied by hand and held by the kubectl-patch field manager on cilium-config.
Server-side apply left them alone so nothing ever broke — but a cold rebuild
installs only the seed, and would have brought the cluster up with the BGP
control plane off and no session to the UDM, while BGP looked configured
because the peering CRDs are in git.

That is not a free change of ownership. Enabling bgpControlPlane through the
chart emits five companion keys the hand-patch never set, so Cilium has been
running on its own internal defaults for router-id allocation and friends. The
comment says which ones and what to watch.

The per-key "THREE COPIES of this key exist" warnings are gone, replaced by one
note at the top of the values block: these values feed the seed, and nothing
enforces agreement. Two copies now, not three, and the surviving one says how
it is made.

Deliberately not done: a generator script and a CI check to diff the committed
seed against a fresh render. That answers duplication with more machinery and
still leaves a 2000-line generated artifact in the tree. If this should stop
being hand-maintained the real question is whether Talos should carry the CNI
at all, which is a rebuild-path decision, not a scripting one.

bootstrap.sh is untouched. Its talos phase remains dead.

claude-bot changed title from ~~fix(talos): regenerate the Cilium bootstrap seed at 1.20.1~~ to WIP — DO NOT MERGE: cilium seed/ownership, blocked on a decision

2026-08-28 13:29:33 +00:00

review-bot reviewed

2026-08-28 13:29:37 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 2e535faa56bc8641fa106c3426b5814a4acba41a.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR re-renders the Cilium bootstrap seed (kubernetes/talos/patches/cilium-cni.yaml) at chart 1.20.1 to match the live HelmRelease, deletes a redundant, fully-duplicate copy of the manifest (kubernetes/talos/cilium.yaml), and moves three previously hand-patched Cilium settings (bgpControlPlane, ipv4/ipv6NativeRoutingCIDR) into the tracked helmrelease.yaml so they flow into the seed too. I traced the resulting datapath flags, images, and RBAC through the diff against the chart-version bump and the checked-out files, and everything is internally consistent with the PR's stated intent — no blocking issues found. Two small things below are worth a look before merge.

🟡 Suggestions

kubernetes/talos/patches/cilium-cni.yaml:192-195 — this 4-line comment ("When Gateway API is enabled this is forced to true... Paired with bpf-lb-sock-hostns-only, which is forced true for the same reason") is a pure hand-typed addition: in the diff it appears as an unpaired + block (the line it replaced was blank), unlike every other added comment in this file, which is a # Source: ... marker that helm template emits automatically. Two things about it don't check out:
- bpf-lb-sock-hostns-only does not appear anywhere else in this ~2000-line rendered manifest, so the claim that it "is forced true" here is unverifiable from the file itself.
- It's indented 8 spaces, while every neighboring key/comment at this nesting level (e.g. lines 190, 196) is indented 10 spaces — a sign it wasn't copy-pasted from the same helm template output as the rest of the block.
The file's own header says to reproduce it exactly from helm template and paste verbatim, precisely because a manual touch is how this file drifted silently before (per the PR description). Worth double-checking whether bpf-lb-sock-hostns-only is actually supposed to be set here, and if not, dropping this comment (or fixing the indent/claim) to keep the seed strictly reproducible.

🟢 Nits

kubernetes/infrastructure/cilium/helmrelease.yaml:100 — the vlanBypass comment still reads "Same three-copy problem as policyMapMax above," but this PR removed the "three-copy" framing from the policyMapMax comment directly above it (and eliminated the third copy, kubernetes/talos/cilium.yaml, outright). The cross-reference is now stale and points at a problem statement that no longer exists in the file.

Tests

No automated test covers this file: kubernetes/talos has no kustomization.yaml, so .woodpecker/ci-manifests-test.yml's kustomize-build step (which walks kubernetes/**/kustomization.yaml) never touches talos/patches/cilium-cni.yaml, and none of the other three steps (woodpecker-lint, shellcheck, no-digest-pins) apply either. The PR description acknowledges this gap explicitly and offers a follow-up CI check comparing the seed against the HelmRelease version — that seems like the right fix and isn't a reason to block this PR, which is itself the manual reconciliation that check would have automated.

I could not verify the referenced prior commits (8f45e290, 621e4e25) or the claimed live-cluster digest match, since the checkout is a shallow, single-commit clone with no network access — but the internal evidence (operator/agent image digests, datapath flags, and RBAC additions) is fully consistent with a straight helm template cilium/cilium --version 1.20.1 render using the values now declared in helmrelease.yaml, with prometheus.serviceMonitor and hubble.tls.auto.method overridden exactly as documented (no ServiceMonitor object or private key material appears anywhere in the seed).

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `2e535faa56bc8641fa106c3426b5814a4acba41a`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR re-renders the Cilium bootstrap seed (`kubernetes/talos/patches/cilium-cni.yaml`) at chart 1.20.1 to match the live HelmRelease, deletes a redundant, fully-duplicate copy of the manifest (`kubernetes/talos/cilium.yaml`), and moves three previously hand-patched Cilium settings (`bgpControlPlane`, `ipv4/ipv6NativeRoutingCIDR`) into the tracked `helmrelease.yaml` so they flow into the seed too. I traced the resulting datapath flags, images, and RBAC through the diff against the chart-version bump and the checked-out files, and everything is internally consistent with the PR's stated intent — no blocking issues found. Two small things below are worth a look before merge. ## 🟡 Suggestions - `kubernetes/talos/patches/cilium-cni.yaml:192-195` — this 4-line comment ("When Gateway API is enabled this is forced to `true`... Paired with `bpf-lb-sock-hostns-only`, which is forced `true` for the same reason") is a pure hand-typed addition: in the diff it appears as an unpaired `+` block (the line it replaced was blank), unlike every other added comment in this file, which is a `# Source: ...` marker that `helm template` emits automatically. Two things about it don't check out: - `bpf-lb-sock-hostns-only` does not appear anywhere else in this ~2000-line rendered manifest, so the claim that it "is forced true" here is unverifiable from the file itself. - It's indented 8 spaces, while every neighboring key/comment at this nesting level (e.g. lines 190, 196) is indented 10 spaces — a sign it wasn't copy-pasted from the same `helm template` output as the rest of the block. The file's own header says to reproduce it exactly from `helm template` and paste verbatim, precisely because a manual touch is how this file drifted silently before (per the PR description). Worth double-checking whether `bpf-lb-sock-hostns-only` is actually supposed to be set here, and if not, dropping this comment (or fixing the indent/claim) to keep the seed strictly reproducible. ## 🟢 Nits - `kubernetes/infrastructure/cilium/helmrelease.yaml:100` — the `vlanBypass` comment still reads "Same three-copy problem as policyMapMax above," but this PR removed the "three-copy" framing from the `policyMapMax` comment directly above it (and eliminated the third copy, `kubernetes/talos/cilium.yaml`, outright). The cross-reference is now stale and points at a problem statement that no longer exists in the file. ## Tests No automated test covers this file: `kubernetes/talos` has no `kustomization.yaml`, so `.woodpecker/ci-manifests-test.yml`'s `kustomize-build` step (which walks `kubernetes/**/kustomization.yaml`) never touches `talos/patches/cilium-cni.yaml`, and none of the other three steps (woodpecker-lint, shellcheck, no-digest-pins) apply either. The PR description acknowledges this gap explicitly and offers a follow-up CI check comparing the seed against the HelmRelease version — that seems like the right fix and isn't a reason to block this PR, which is itself the manual reconciliation that check would have automated. I could not verify the referenced prior commits (`8f45e290`, `621e4e25`) or the claimed live-cluster digest match, since the checkout is a shallow, single-commit clone with no network access — but the internal evidence (operator/agent image digests, datapath flags, and RBAC additions) is fully consistent with a straight `helm template cilium/cilium --version 1.20.1` render using the values now declared in `helmrelease.yaml`, with `prometheus.serviceMonitor` and `hubble.tls.auto.method` overridden exactly as documented (no `ServiceMonitor` object or private key material appears anywhere in the seed). ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot referenced this issue from a commit

2026-08-28 13:31:27 +00:00

docs(cilium): decide the ownership question before changing anything

claude-bot referenced this pull request

2026-08-28 13:31:51 +00:00

docs(cilium): decide the ownership question before changing anything #707

claude-bot closed this pull request

2026-08-28 13:34:09 +00:00

claude-bot commented

2026-08-28 13:34:22 +00:00

Geschlossen — hinfällig durch #707.

Der Seed wird in Schritt 6 des Übergangs gelöscht, also war seine Regeneration Arbeit für eine Datei, die verschwindet. Auch dazwischen hätte sie nichts gerettet: der einzige Lauf, der Bootstrap-Manifeste anfasst, ist upgrade-k8s — und genau der ist Schritt 4 des Übergangs. Ein nackter upgrade-k8s vorher findet nicht statt.

Was aus diesem PR überlebt, gehört in den Übergang, nicht hierher:

Inhalt	Status
Seed auf 1.20.1 regeneriert	hinfällig — Datei wird gelöscht
`talos/cilium.yaml` gelöscht (kein Konsument)	überlebt, unabhängig von der Entscheidung
`bgpControlPlane` + Native-Routing-CIDRs getrackt	überlebt — Schritt 1 des Übergangs
Kommentar-Korrekturen in der HelmRelease	überlebt

Der dritte Punkt hängt allerdings am Ausgang von #707: bei Flux-Besitz gehören diese Werte in die HelmRelease, bei Talos-Besitz in den Seed. Das Ziel ist also erst nach der Entscheidung bestimmbar — deshalb mache ich dafür jetzt keinen neuen PR auf.

Der Branch fix-cilium-bootstrap-seed bleibt stehen, damit die Rekonstruktion des Wertesatzes nicht verloren geht — sie war nicht trivial und wird für Schritt 1 zumindest teilweise wieder gebraucht.

Geschlossen — hinfällig durch #707. Der Seed wird in Schritt 6 des Übergangs gelöscht, also war seine Regeneration Arbeit für eine Datei, die verschwindet. Auch dazwischen hätte sie nichts gerettet: der einzige Lauf, der Bootstrap-Manifeste anfasst, ist `upgrade-k8s` — und genau der *ist* Schritt 4 des Übergangs. Ein nackter `upgrade-k8s` vorher findet nicht statt. **Was aus diesem PR überlebt, gehört in den Übergang, nicht hierher:** | Inhalt | Status | |---|---| | Seed auf 1.20.1 regeneriert | hinfällig — Datei wird gelöscht | | `talos/cilium.yaml` gelöscht (kein Konsument) | überlebt, unabhängig von der Entscheidung | | `bgpControlPlane` + Native-Routing-CIDRs getrackt | überlebt — **Schritt 1** des Übergangs | | Kommentar-Korrekturen in der HelmRelease | überlebt | Der dritte Punkt hängt allerdings am Ausgang von #707: bei Flux-Besitz gehören diese Werte in die HelmRelease, bei Talos-Besitz in den Seed. Das Ziel ist also erst nach der Entscheidung bestimmbar — deshalb mache ich dafür jetzt keinen neuen PR auf. Der Branch `fix-cilium-bootstrap-seed` bleibt stehen, damit die Rekonstruktion des Wertesatzes nicht verloren geht — sie war nicht trivial und wird für Schritt 1 zumindest teilweise wieder gebraucht.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#706