feat(cilium): track the BGP switch and native-routing CIDRs #708

Merged

k.renner merged 1 commits from cilium-track-untracked-settings into master 2026-08-28 13:47:55 +00:00

Conversation 1 Commits 1 Files Changed 1

+25

claude-bot commented 2026-08-28 13:39:50 +00:00

Member

Copy Link

Copy Source

Schritt 1 des Übergangs aus #707. Am Besitz ändert sich hier noch nichts — es holt nur drei laufende Einstellungen aus dem Nichts ins Git.

Was fehlte

enable-bgp-control-plane und beide Native-Routing-CIDRs existierten in keiner Datei. Von Hand appliziert, gehalten vom kubectl-patch-Field-Manager auf cilium-config. Server-Side Apply hat sie in Ruhe gelassen, also ist nie etwas kaputtgegangen und nie etwas aufgefallen.

Warum die Lücke unsichtbar war: die Peering-CRDs in post-crds/cilium-bgp.yaml lagen immer im Git. BGP sah also konfiguriert aus — versioniert war aber nur die Policy, nicht der Schalter, der den Control-Plane startet. Ein Cold Rebuild installiert nur, was Git beschreibt: zwei sauber definierte Peers und kein BGP.

Warum die CIDRs mitmüssen

Sie sind dieselbe Entscheidung, keine zweite. Es ist eine Masquerading-Ausnahme, keine Routing-Aussage: Pod-Verkehr nach 10.0.0.0/8 und fd00::/8 geht unmaskiert raus, damit LAN-Dienste echte Pod-IPs sehen. Das funktioniert nur, weil BGP die Pod-CIDRs announced und der UDM damit eine Rückroute gibt. Eines ohne das andere zu tracken hinterließe eine halbe Entscheidung im Git.

Warum das ein eigener Schritt ist

Weil es kein reiner Eigentumswechsel ist. bgpControlPlane über das Chart zu aktivieren emittiert fünf Begleitschlüssel, die der Handpatch nie gesetzt hat:

bgp-secrets-namespace
bgp-router-id-allocation-mode
bgp-router-id-allocation-ip-pool
enable-bgp-control-plane-status-report
enable-bgp-legacy-origin-attribute

Cilium läuft dafür bisher auf seinen internen Defaults. Die sollten mit denen des Charts übereinstimmen — Router-ID-Allocation ist die, auf die zu achten ist: weichen sie ab, bauen sich die Sessions neu auf.

Nach dem Merge

Ich prüfe die BGP-Peers, bevor Schritt 2 angefasst wird — beide Sessions established, Routen weiterhin announced. Wenn die Sessions durchlaufen, war es ein reiner Ownership-Wechsel; wenn sie neu aufbauen, war es die Router-ID und das gehört dann dokumentiert.

Geprüft: Dry-Run mit Flux' Field-Manager sauber, Werte parsen als true / 10.0.0.0/8 / fd00::/8.

**Schritt 1** des Übergangs aus #707. Am Besitz ändert sich hier noch nichts — es holt nur drei laufende Einstellungen aus dem Nichts ins Git. ## Was fehlte `enable-bgp-control-plane` und beide Native-Routing-CIDRs existierten in **keiner Datei**. Von Hand appliziert, gehalten vom `kubectl-patch`-Field-Manager auf `cilium-config`. Server-Side Apply hat sie in Ruhe gelassen, also ist nie etwas kaputtgegangen und nie etwas aufgefallen. **Warum die Lücke unsichtbar war:** die Peering-CRDs in `post-crds/cilium-bgp.yaml` lagen immer im Git. BGP *sah* also konfiguriert aus — versioniert war aber nur die Policy, nicht der Schalter, der den Control-Plane startet. Ein Cold Rebuild installiert nur, was Git beschreibt: zwei sauber definierte Peers und kein BGP. ## Warum die CIDRs mitmüssen Sie sind dieselbe Entscheidung, keine zweite. Es ist eine **Masquerading-Ausnahme, keine Routing-Aussage**: Pod-Verkehr nach `10.0.0.0/8` und `fd00::/8` geht unmaskiert raus, damit LAN-Dienste echte Pod-IPs sehen. Das funktioniert **nur**, weil BGP die Pod-CIDRs announced und der UDM damit eine Rückroute gibt. Eines ohne das andere zu tracken hinterließe eine halbe Entscheidung im Git. ## Warum das ein eigener Schritt ist Weil es **kein reiner Eigentumswechsel** ist. `bgpControlPlane` über das Chart zu aktivieren emittiert fünf Begleitschlüssel, die der Handpatch nie gesetzt hat: ``` bgp-secrets-namespace bgp-router-id-allocation-mode bgp-router-id-allocation-ip-pool enable-bgp-control-plane-status-report enable-bgp-legacy-origin-attribute ``` Cilium läuft dafür bisher auf seinen internen Defaults. Die sollten mit denen des Charts übereinstimmen — **Router-ID-Allocation ist die, auf die zu achten ist**: weichen sie ab, bauen sich die Sessions neu auf. ## Nach dem Merge Ich prüfe die BGP-Peers, **bevor** Schritt 2 angefasst wird — beide Sessions `established`, Routen weiterhin announced. Wenn die Sessions durchlaufen, war es ein reiner Ownership-Wechsel; wenn sie neu aufbauen, war es die Router-ID und das gehört dann dokumentiert. Geprüft: Dry-Run mit Flux' Field-Manager sauber, Werte parsen als `true / 10.0.0.0/8 / fd00::/8`.

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

feat(cilium): track the BGP switch and native-routing CIDRs ... 96e20146a7

Step 1 of the ownership transition in PROPOSAL-cilium-single-owner.md. Nothing
about ownership changes yet — this only moves three live settings out of nowhere
and into git.

enable-bgp-control-plane and both native-routing CIDRs existed in no file at
all. They were applied by hand and are held by the `kubectl-patch` field manager
on cilium-config; server-side apply left them alone, so nothing broke and
nothing reported them. The peering itself was always in git as CRDs in
post-crds/cilium-bgp.yaml, which is what made the gap invisible: BGP looked
configured because its policy was version-controlled, while the switch that
starts the control plane was not. A cold rebuild installs only what git
describes, so it would have come up with two established-looking peers
configured and no BGP at all.

The CIDRs go with it because they are the same decision. They are a masquerading
exception, not a routing statement: pod traffic to 10.0.0.0/8 and fd00::/8
leaves unmasqueraded so LAN services see real pod IPs, and that only works
because BGP advertises the pod CIDRs and gives the UDM a route back. Tracking
one without the other would leave a half-decision in git.

This is not purely a change of ownership, which is why it is its own step and
not folded into a later one. 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 among others. They should agree
with the chart's; if they do not, the sessions re-establish. Verify the peers
after Flux reconciles before moving to step 2.

review-bot requested changes 2026-08-28 13:45:12 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 96e20146a74ce6c4f6e8081da7a0ebeda97df590.

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

---

Summary

Adds bgpControlPlane.enabled: true, ipv4NativeRoutingCIDR: 10.0.0.0/8 and ipv6NativeRoutingCIDR: fd00::/8 to the Cilium HelmRelease values (kubernetes/infrastructure/cilium/helmrelease.yaml:143-167), bringing three settings that were previously only hand-applied on the live cilium-config ConfigMap under Flux/Helm's control. The change itself is mechanically simple and the values match what the chart's own key names expect. However, the PR's central justification — both in the description and in the new in-file comment — is contradicted by a file already in this repo, which matters a lot given how documentation-driven this codebase is.

🔴 Blocking

• helmrelease.yaml:9-14 (new comment) and the PR body both claim these settings "existed in no file" and that "a cold rebuild would have come up with BGP off." That's not accurate: kubernetes/talos/patches/cilium-cni.yaml:407-426 already sets enable-bgp-control-plane: "true", ipv4-native-routing-cidr: "10.0.0.0/8" and ipv6-native-routing-cidr: "fd00::/8", with values identical to what this PR adds. Per this repo's own established convention — the comment on bpf.policyMapMax a few lines above the diff (helmrelease.yaml:79-82) explicitly calls cilium-cni.yaml and talos/cilium.yaml "the latter two [that] are what a cold rebuild installs from" — and per RUNBOOK-cold-rebuild.md Step 2, cilium-cni.yaml is passed as --config-patch to talosctl gen config, i.e. it is applied at cluster genesis, before Flux ever runs.

  So a cold rebuild before this PR would already come up with BGP enabled and the native-routing CIDRs set (from the Talos-applied inline manifest), and Flux's subsequent server-side-apply reconcile of the pre-PR HelmRelease — which rendered no bgp-*/*-native-routing-cidr keys at all — would leave those fields alone for the same SSA per-key-ownership reason the PR body gives for the kubectl-patch case. The real gap this PR closes is narrower than described: it's that the Flux/Helm-rendered steady-state config didn't express these settings (so future chart-driven changes, drift detection, or Renovate-style version bumps had no way to know about or control them) — not that a cold rebuild would produce BGP-off. As written, the comment will mislead a future reader debugging a cold rebuild, and the author's own stated post-merge check ("I'll verify whether the peer sessions stayed established, to tell an ownership-only change from a router-id-triggered reset") is framed on the wrong premise for why the values could differ at bootstrap.

  Please correct the comment (and description) to say what's actually true: the CRDs and the raw Talos bootstrap config already carried this decision; what was missing was tracking it in the Helm values that govern ongoing Flux reconciliation.

• The newly-chart-tracked keys aren't cross-referenced with their existing copy in cilium-cni.yaml, breaking this repo's own "keep multiple copies in lockstep" convention. labels (helmrelease.yaml:53-54) and bpf.policyMapMax/bpf.vlanBypass (helmrelease.yaml:79-82, 98-100) both carry an explicit "keep in lockstep with kubernetes/talos/patches/cilium-cni.yaml and kubernetes/talos/cilium.yaml" note, precisely because nothing enforces agreement between the copies. The three keys this PR adds are exactly that same class of duplicated setting (confirmed identical values in cilium-cni.yaml:411,425-426), but the new comment doesn't mention the other copies at all, so the convention silently doesn't cover them. Worth noting too: kubernetes/talos/cilium.yaml (the "third copy" per that same convention) has none of these three keys — it only has routing-mode: "tunnel" — so if bootstrap.sh talos's config generation is ever revived (currently dead per issue #581, per RUNBOOK-cold-rebuild.md), that path would still produce a BGP-off cluster. That's arguably today's real version of the risk the PR is trying to describe, and it isn't mentioned.

🟡 Suggestions

• The PR body itself flags bgp-router-id-allocation-mode/-ip-pool as "the one to watch" because enabling bgpControlPlane via the chart emits companion keys the hand-patch (and cilium-cni.yaml) never set, and Cilium is currently running on its internal defaults for them. Since the risk is already identified and the values needed to pin it are known, consider setting bgp-router-id-allocation-mode/-ip-pool explicitly in this same change rather than deferring the check to a post-merge peer-session inspection — that avoids the possible session flap entirely instead of just being ready to diagnose it.

Tests

No automated coverage exists for this class of change and none is expected: .woodpecker/ci-manifests-test.yml runs kubectl kustomize over every kustomization (would catch structural YAML/kustomize errors) and woodpecker-cli lint, but nothing renders the Helm chart against its values schema, so an invalid/misspelled Helm key wouldn't be caught by CI — bgpControlPlane, ipv4NativeRoutingCIDR and ipv6NativeRoutingCIDR do match the Cilium chart's real value names, so this isn't a defect, just a gap the PR inherits rather than introduces. The author's stated verification (dry-run against Flux's field manager, plus a post-merge check that both BGP peer sessions stay established) is the right kind of check for an infra-as-code repo without a live test cluster in CI — make sure it actually happens given the corrected understanding above of what was and wasn't already tracked.

Recommendation

Request changes — 1 blocking issue (the comment/PR narrative about what a cold rebuild currently produces is factually wrong and should be corrected before merge; the second bullet is a lockstep/consistency gap that should be closed in the same pass).

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `96e20146a74ce6c4f6e8081da7a0ebeda97df590`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Adds `bgpControlPlane.enabled: true`, `ipv4NativeRoutingCIDR: 10.0.0.0/8` and `ipv6NativeRoutingCIDR: fd00::/8` to the Cilium `HelmRelease` values (`kubernetes/infrastructure/cilium/helmrelease.yaml:143-167`), bringing three settings that were previously only hand-applied on the live `cilium-config` ConfigMap under Flux/Helm's control. The change itself is mechanically simple and the values match what the chart's own key names expect. However, the PR's central justification — both in the description and in the new in-file comment — is contradicted by a file already in this repo, which matters a lot given how documentation-driven this codebase is. ## 🔴 Blocking - **`helmrelease.yaml:9-14` (new comment) and the PR body both claim these settings "existed in no file" and that "a cold rebuild would have come up with BGP off."** That's not accurate: `kubernetes/talos/patches/cilium-cni.yaml:407-426` already sets `enable-bgp-control-plane: "true"`, `ipv4-native-routing-cidr: "10.0.0.0/8"` and `ipv6-native-routing-cidr: "fd00::/8"`, with values identical to what this PR adds. Per this repo's own established convention — the comment on `bpf.policyMapMax` a few lines above the diff (`helmrelease.yaml:79-82`) explicitly calls `cilium-cni.yaml` and `talos/cilium.yaml` "the latter two [that] are what a cold rebuild installs from" — and per `RUNBOOK-cold-rebuild.md` Step 2, `cilium-cni.yaml` is passed as `--config-patch` to `talosctl gen config`, i.e. it *is* applied at cluster genesis, before Flux ever runs. So a cold rebuild before this PR would already come up with BGP enabled and the native-routing CIDRs set (from the Talos-applied inline manifest), and Flux's subsequent server-side-apply reconcile of the pre-PR `HelmRelease` — which rendered no `bgp-*`/`*-native-routing-cidr` keys at all — would leave those fields alone for the same SSA per-key-ownership reason the PR body gives for the `kubectl-patch` case. The real gap this PR closes is narrower than described: it's that the **Flux/Helm-rendered steady-state config** didn't express these settings (so future chart-driven changes, drift detection, or Renovate-style version bumps had no way to know about or control them) — not that a cold rebuild would produce BGP-off. As written, the comment will mislead a future reader debugging a cold rebuild, and the author's own stated post-merge check ("I'll verify whether the peer sessions stayed established, to tell an ownership-only change from a router-id-triggered reset") is framed on the wrong premise for *why* the values could differ at bootstrap. Please correct the comment (and description) to say what's actually true: the CRDs *and* the raw Talos bootstrap config already carried this decision; what was missing was tracking it in the Helm values that govern ongoing Flux reconciliation. - **The newly-chart-tracked keys aren't cross-referenced with their existing copy in `cilium-cni.yaml`, breaking this repo's own "keep multiple copies in lockstep" convention.** `labels` (`helmrelease.yaml:53-54`) and `bpf.policyMapMax`/`bpf.vlanBypass` (`helmrelease.yaml:79-82`, `98-100`) both carry an explicit "keep in lockstep with `kubernetes/talos/patches/cilium-cni.yaml` and `kubernetes/talos/cilium.yaml`" note, precisely because nothing enforces agreement between the copies. The three keys this PR adds are exactly that same class of duplicated setting (confirmed identical values in `cilium-cni.yaml:411,425-426`), but the new comment doesn't mention the other copies at all, so the convention silently doesn't cover them. Worth noting too: `kubernetes/talos/cilium.yaml` (the "third copy" per that same convention) has **none** of these three keys — it only has `routing-mode: "tunnel"` — so if `bootstrap.sh talos`'s config generation is ever revived (currently dead per issue #581, per `RUNBOOK-cold-rebuild.md`), that path would still produce a BGP-off cluster. That's arguably today's real version of the risk the PR is trying to describe, and it isn't mentioned. ## 🟡 Suggestions - The PR body itself flags `bgp-router-id-allocation-mode`/`-ip-pool` as "the one to watch" because enabling `bgpControlPlane` via the chart emits companion keys the hand-patch (and `cilium-cni.yaml`) never set, and Cilium is currently running on its internal defaults for them. Since the risk is already identified and the values needed to pin it are known, consider setting `bgp-router-id-allocation-mode`/`-ip-pool` explicitly in this same change rather than deferring the check to a post-merge peer-session inspection — that avoids the possible session flap entirely instead of just being ready to diagnose it. ## Tests No automated coverage exists for this class of change and none is expected: `.woodpecker/ci-manifests-test.yml` runs `kubectl kustomize` over every kustomization (would catch structural YAML/kustomize errors) and `woodpecker-cli lint`, but nothing renders the Helm chart against its values schema, so an invalid/misspelled Helm key wouldn't be caught by CI — `bgpControlPlane`, `ipv4NativeRoutingCIDR` and `ipv6NativeRoutingCIDR` do match the Cilium chart's real value names, so this isn't a defect, just a gap the PR inherits rather than introduces. The author's stated verification (dry-run against Flux's field manager, plus a post-merge check that both BGP peer sessions stay `established`) is the right kind of check for an infra-as-code repo without a live test cluster in CI — make sure it actually happens given the corrected understanding above of what was and wasn't already tracked. ## Recommendation Request changes — 1 blocking issue (the comment/PR narrative about what a cold rebuild currently produces is factually wrong and should be corrected before merge; the second bullet is a lockstep/consistency gap that should be closed in the same pass). <!-- pr-review:sha=96e20146a74ce6c4f6e8081da7a0ebeda97df590 -->

k.renner merged commit 6d543194af into master 2026-08-28 13:47:55 +00:00

k.renner referenced this issue from a commit 2026-08-28 13:47:56 +00:00

Merge pull request 'feat(cilium): track the BGP switch and native-routing CIDRs' (#708) from cilium-track-untracked-settings into master

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

Labels

Clear labels

apollo

Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant

atlas

Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring

clio

Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites

daedalus

Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes

hestia

Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#708