Repository navigation
fix(cilium): bump vendored chart to v1.20.2 - #3343
Conversation
github-actions
Bot
added
area/networking
|
Important Review skippedThe saved review history does not include the base for the last reviewed commit. This saved history cannot establish the base for an incremental review. Comment You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCilium chart metadata, image tags and digests, and the Docker build source are updated for the 1.19.6 release. Host firewall configuration rendering and related Cilium behavior comments are also updated. ChangesCilium release update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the vendored Cilium chart and agent image to v1.19.6 to incorporate critical upstream fixes. The update resolves specific regressions related to Gateway API HTTPRoute attachment and host-firewall configuration management, ensuring more robust network policy and gateway behavior. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
the
area/platform
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Cilium image version in the Dockerfile from v1.19.5 to v1.19.6. The reviewer noted that removing the SHA256 digest pin reduces build reproducibility and security, and recommended appending the specific digest for the new version to maintain a secure build.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -1,4 +1,4 @@ | |||
| ARG VERSION=v1.19.5@sha256:20fbbc14ac20b55a292c0dcda5571bf31cde30a7dbc68c29db3e709390ab0732 | |||
| ARG VERSION=v1.19.6 | |||
There was a problem hiding this comment.
The previous version pinned the base image using a SHA256 digest (@sha256:...). Removing the digest pin reduces build reproducibility and security, as the tag v1.19.6 could theoretically be updated or compromised. Consider appending the specific SHA256 digest for v1.19.6 to maintain a secure and reproducible build.
There was a problem hiding this comment.
This is intentional. The package's make update rewrites ARG VERSION=v${version} without a digest by design (Makefile line 25), and .github/renovate.json sets pinDigests: true for the dockerfile manager, so Renovate re-pins the base image to an immutable digest right after the bump — hand-adding it here would just be overwritten on the next make update. The deployed agent image is digest-pinned via values.yaml (image.digest) regardless, so the supply-chain pin that actually ships is already immutable.
There was a problem hiding this comment.
Reversed on this one: the digest is back, in aae4bf396.
My earlier reasoning was about the mechanism, not the outcome. make update producing a bare tag explains how the pin disappears; it does not argue that main should carry an unpinned base for the one image built here until Renovate's next weekly batch picks it up.
The value is the manifest digest of quay.io/cilium/cilium:v1.19.6 and matches image.digest in the vendored chart values, so it is checkable from inside this diff. The structural part of my earlier answer still holds: the next make update strips it again, and that conflict is not this PR's to solve.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed with a hermetic (no-cluster) pass. LGTM with non-blocking notes.
Verified:
- The configmap guard removal is upstream (cilium#44748): v1.19.5 gated
enable-host-firewallbehindhostFirewall.enabled, v1.19.6 emits it unconditionally. Reproduced viahelm pull, so the diff is faithfulmake updateoutput, not a hand-edit. - Render safety:
hostFirewall.enableddefaults tofalseandhostFirewallis always defined, so the now-unconditional key has no nil-pointer risk; both true/false render. - Upgrade: patch-level bump within 1.19, no CRD/workload/Service rename, images stay digest-pinned. Expected CNI agent rollout only.
Non-blocking:
DockerfileARG VERSION drops the@sha256digest, but that is exactly whatmake updatewrites and renovate re-pins it afterwards; the deployed image is digest-pinned via values.yaml regardless.
228eceb to
0b9d473
Compare
|
IvanHunters thanks for the review — your findings match what I verified independently: the ConfigMap guard removal is upstream cilium#44748, and the deployed image stays digest-pinned via values.yaml regardless of the ARG. Your comment reads as an approval, but it was submitted as a review comment rather than an Approve, so the PR is still blocked on REVIEW_REQUIRED. Could you convert it? The branch was rebased onto main since your pass to pick up the serviceexposure suite removal, and the diff is unchanged. |
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
NOT LGTM — the bump itself is clean and I could not fault it, but merging it invalidates an in-tree comment in a way that sets a trap for the next reader. It is a one-line documentation fix; no functional change is being asked for.
Business context: Cilium v1.19.5 wrongly rejects platform-app HTTPRoutes (Keycloak, dashboard) attaching to a shared tenant Gateway with multiple listeners; this bumps the vendored chart and agent image to v1.19.6, which carries the upstream fix.
What I verified
Every load-bearing claim checks out at source level, not just by release-note title:
- The premise. The v1.19.5
CheckGatewayAllowedForNamespaceearly-return on the first listener selector miss is gone in v1.19.6 — theif !allowed { … return false }block is deleted and the loop now falls through to the next listener. - The fix is in this tag specifically. Backport commit
54aa8fca5carries[ upstream commit f334eb443a3a… ], byte-identical to cilium#45693's merge SHA, andcompare/f21b3a57…v1.19.6reportsbehind_by: 0. - Vendoring is faithful. I reproduced
make updatefrom scratch (helm pull cilium/cilium --version 1.19plus the Makefile's sed/perl transforms) and diffed against the committed tree: identical, including the file set. No hand edits, no missed files, no stale1.19.5left anywhere. - The BPF IPv6-passthrough patch still applies to v1.19.6 (
patch --dry-run --fuzz=0clean, no offset), and all six image digests match upstreamvalues.yamland the registry.
The 5-file / 64-line diff really is all a patch bump touches.
Blocker
reconciler.go:868-872 becomes actively misleading the moment this merges
cozystack/internal/controller/tenantgateway/reconciler.go
Lines 868 to 872 in 2642965
// NOTE: on Cilium 1.19.x each tls-<svc> listener will surface
// ResolvedRefs=False/InvalidRouteKinds on the raw Gateway object
// (cosmetic — …); removable once Cilium 1.20 /
// cilium#45693 ships.Evidence: the condition that emits InvalidRouteKinds lives in operator/pkg/gateway-api/gateway_reconcile.go and is unchanged between the two tags — if len(supportedKinds) != len(l.AllowedRoutes.Kinds) { conds = merge(conds, gatewayListenerInvalidRouteKinds(...)) } at v1.19.5:580-581 and at v1.19.6:653-654 (the line shift is only because the file grew). So cilium#45693 does not touch that condition at all.
This PR is precisely the event that makes the comment dangerous: it ships #45693, thereby satisfying the comment's stated trigger while the underlying reason still stands. A maintainer reading it post-merge would reasonably conclude the port443Kinds anti-collapse workaround is now removable — and removing it re-triggers cilium#45559, which silently drops the HTTPRoutes accepted by the HTTPS-terminate listeners. That is the same outage class this PR exists to fix.
TestReconcile_Port443ListenersShareKinds (reconciler_test.go:3672) would fail loudly if anyone acted on it, which is the only reason this is a comment fix rather than a functional one. Suggested rewording: drop the cilium#45693 trigger and state that the condition originates in gateway_reconcile.go's supportedKinds length check, which is still present as of v1.19.6.
Non-blocking
1. hack/e2e-cilium-endpoint-leak-healer.sh:206-207 — the healer's version watermark stops covering what we ship.
cozystack/hack/e2e-cilium-endpoint-leak-healer.sh
Lines 206 to 207 in 2642965
# cilium host-scope IPAM + Gateway API restore-window race; no released fix as
# of v1.19.5.)Being explicit about the evidence level, since this is weaker than the blocker above: I did not establish that v1.19.6 fixes this race, so the claim becomes unverified rather than proven false. But after this merge the platform runs v1.19.6 while the healer's rationale stays indexed to v1.19.5, so whoever next evaluates whether the healer is still needed inherits a watermark that quietly no longer applies. Either re-validate against v1.19.6 and bump the reference, or reword it to be version-agnostic.
(Related, but do not bank on it: 9f7d1eb7f endpoint: Fix race when reading endpoint properties (#46991) does land in v1.19.6 — nothing in the notes claims it addresses IPAM address leakage.)
2. The release note omits a behavioural change worth naming — allowedRoutes.kinds goes from broken-open to enforced.
In v1.19.5 CheckGatewayRouteKindAllowed ended its listener loop with // return true, nil commented out and fell through to return true, nil: it set an Accepted=False/NotAllowedByListeners condition but still returned true. v1.19.6 adds hasKindRestriction and genuinely returns false.
This cuts both ways for us. reconciler_test.go:3649-3654 states that GRPCRoute/TCPRoute/UDPRoute "must NEVER appear in any port-443 listener's kinds set… admitting them would let a tenant serve arbitrary traffic under the apex cert without admission control" — on v1.19.5 that control was declarative only, and v1.19.6 is what actually enforces it. Conversely, any tenant GRPCRoute/TCPRoute/UDPRoute that attached under the old broken-open behaviour will stop attaching. Nothing first-party regresses (no such routes ship in-tree, and port443Kinds is [HTTPRoute, TLSRoute]), but it belongs in the release-note block.
3. Heads-up, not a defect here. The v1.19.6 notes advertise "Fix a regression that could cause established connections to a Pod to be briefly dropped during Cilium agent restart/upgrade" (#46927), but the same notes also list "Revert restore-path policy map changes (#46927, #47062, #47075)" (#47213). Both the three commits and their three reverts sit inside the v1.19.5...v1.19.6 range, so that fix is not effectively present — worth not expecting agent-restart drops to improve.
Dismissed during review
Noting these so the dismissals can be checked rather than silently dropped:
- Unconditional
enable-host-firewallchurning every cluster's ConfigMap — rendered both variants: the default (hostFirewall.enabled: true) emits"true"on main and on this branch, unchanged. Only the kilo variant changes (key absent →"false"), andfalseis Cilium's own default, so it is a semantic no-op. One variant, not every cluster. - Removing the
and .Values.hostFirewallnil-guard risking a nil-pointer render — the subchart defaultshostFirewall.enabled: false, no values file sets it null, both variants render. - Pinning the Dockerfile base image by digest — the package
Makefile'supdate:target writesARG VERSION=v${version}without a digest by design, and.github/renovate.jsonsetspinDigests: truefor the dockerfile manager. Git history shows the cycle resolving itself (bump unpinned → renovate re-pins), and the deployed agent image is digest-pinned viavalues.yamlregardless. - Issue #42159's framing — the PR body cites it for false rejections, but the issue's headline case is the opposite direction (an over-permissive
from: Samelistener wrongly accepting a cross-namespace route) and it reports>= v1.18.2, < v1.19.0. The operative conclusion still holds since the code path demonstrably persisted into v1.19.5, so this is not worth changing.
0b9d473 to
7329176
Compare
github-actions
Bot
added
size/S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/cilium/images/cilium/Dockerfile`:
- Around line 1-3: Update the upstream build stage declaration associated with
VERSION to retain the v1.19.6 version while also pinning quay.io/cilium/cilium
to its required immutable digest. Preserve the existing version argument and use
the matching digest for that image tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c88c6c81-adb8-4775-a0d2-427bdb0467fc
📒 Files selected for processing (7)
hack/e2e-cilium-endpoint-leak-healer.shinternal/controller/tenantgateway/reconciler.gopackages/system/cilium/charts/cilium/Chart.yamlpackages/system/cilium/charts/cilium/README.mdpackages/system/cilium/charts/cilium/templates/cilium-configmap.yamlpackages/system/cilium/charts/cilium/values.yamlpackages/system/cilium/images/cilium/Dockerfile
💤 Files with no reviewable changes (1)
- packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/system/cilium/charts/cilium/Chart.yaml
- packages/system/cilium/charts/cilium/values.yaml
- packages/system/cilium/charts/cilium/README.md
|
myasnikovdaniil thanks for the source-level review — the make-update reproduction and digest checks especially. Addressed:
|
|
myasnikovdaniil the blocker and both actionable non-blocking notes were addressed in e8038de and 6ca2c68, but the review is still at CHANGES_REQUESTED so the PR stays blocked. On the blocker: the Nothing else changed beyond those two comment commits. Could you re-check? |
|
myasnikovdaniil the blocker and both actionable notes landed in The for t in v1.19.5 v1.19.6; do
curl -sL "https://raw.githubusercontent.com/cilium/cilium/$t/operator/pkg/gateway-api/gateway_reconcile.go" -o "gw-$t.go"
done
diff <(sed -n '578,586p' gw-v1.19.5.go) <(sed -n '651,659p' gw-v1.19.6.go) # no outputcilium#45693 is in v1.19.6 and not in v1.19.5, so an identical block across the two tags settles that it does not touch this condition — which is exactly what the note in |
6ca2c68 to
8a06f60
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
8a06f60 to
05b4cc4
Compare
235c9ee to
3e0c6e6
Compare
v1.19.5 rejects platform HTTPRoutes on a shared tenant Gateway with NotAllowedByListeners (cilium#42159). The 1.19 line that fixes it also marks a same-port HTTPS-terminate and TLS-passthrough pair ProtocolConflict while still programming both, which leaves the TenantGateway Ready=False on a layout that serves. 1.20 drops such a pair from Envoy outright, so the listener layout is fixed against 1.20 rather than against a 1.19 status quirk, and the bump goes straight there. The update target now follows the 1.20 line. v1.20 moved the ARP case of do_netdev under ENABLE_IPV4, which broke the anchor of the IPv6 passthrough patch; the patch's ETH_P_IPV6 case now sits right before `default:`, outside the ENABLE_IPV4 block. The other two cases apply unchanged. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The note said the restore-window race had no released fix as of v1.19.5, which reads as current on every later pin whether or not anything changed upstream. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The admission-webhook deny policies in ingress-nginx and kubeovn-webhook cite Cilium source and the vendored values by line number. The bump moved endpointLockdownOnMapOverflow and the policy-map ordering block in pkg/endpoint/bpf.go, so the cited lines pointed elsewhere. Name the key and the function instead, which survive the next bump, and move the getEndpointSelector citation to v1.20.2, where the cited lines are unchanged. The kubeProxyReplacement pin explained the service translation in terms of v1.19.5. With Gateway API on, v1.20.2 keeps socket LB in the host namespace, so pod-namespace connections are translated per packet in bpf_lxc before egress policy runs, which is what the pin relies on. The same holds for the e2e image mirror, whose egress-policy comment in hack/e2e-talos-image-cache.yaml described socket LB. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
Since v1.20.0 the bpf_host SNAT conflict check fires for any packet carrying the host mark on every device bpf_host runs on, and the SNAT path replaces that mark with SNAT_DONE. With kube-ovn chaining a host connection to a pod on the same node leaves through ovn0, so the pod's to-container program sees SNAT_DONE and classifies the source as world instead of host. The implicit localhost allow then stops matching: kubelet probes fail under any default-deny ingress policy that does not admit world, and a world deny drops the API server calling a webhook on its own node. Carry a verbatim backport of cilium/cilium#49232, which skips the conflict check when the destination is a local endpoint: such traffic never leaves the node and cannot collide with pod egress SNAT. Keeping the hunks identical to upstream lets the patch be dropped as soon as a shipped cilium release contains the fix. Without chaining, host traffic to a local pod does not pass to-netdev and nothing changes. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
fe0c1d8 to
b079990
Compare
|
IvanHunters, answers to the review of 9205f93.
|
|
myasnikovdaniil, |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The round-1 regression is closed. The two new cilium-operator patches let an HTTPS termination listener and a TLS passthrough listener share port 443, and the routing is correct: a catch-all terminate chain sits beside explicit, narrower passthrough chains, Envoy's most-specific match sends passthrough SNI to the passthrough chain and everything else to terminate, and a gate miss fails closed. What remains is non-blocking. This PR is designed to merge together with #4638, so the notes below that mention #4638 are context, not asks against this branch.
Findings
- [NIT]
packages/system/cilium/images/cilium-operator/patches/conflict-https-and-tls-passthrough-unless-passthrough-is-narrower.diff:50, Conflict status message still says 'overlapping hostnames' even when overlap is now allowed
Round-1 reconciliation
- The standalone-regression blocker is closed. The operator patches apply to v1.20.2 at
--fuzz=0, the new cilium-operator image build is wired intomake imageand the releasemake build, andgo test ./operator/pkg/{gateway-api,model/translation}is green on the patched tree. RevertingstrictlyNarrowerHostnamereddens the accept-case tests, so the behaviour is covered. - The SNAT patch header no longer cites the orphaned commit: it cites cilium/cilium#49232 without a SHA and states the drop is manual, with the redundancy risk spelled out.
- The dead
externalIPs/nodePortvalues are pre-existing (byte-identical at the merge base) and tracked in #4816.
Caveats
- The committed
operator.image.overrideis the unpatched upstream operator by design:make imagestamps the cozystack-built patched digest into values.yaml, whichmake buildand the release pipeline run before packaging, and the comment at values.yaml:108 documents it. A tree rendered straight from source without a build pulls the unpatched operator, so dev-side verification of the pair needs the stamped image. Shipped artifacts carry the patched digest. - The http01 equal-hostname case (an HTTPRoute claiming a passthrough name renders an equal-hostname terminate listener, which still conflicts and drops both on 1.20) is handled in #4638, not here. Gateway integration is off by default.
- The tenantgateway controller comments that still name the 1.19.5 pin (renderers.go and a few siblings) are rewritten in #4638 (1943501), which is stacked on this branch. On this branch alone they read stale.
E2E (in-tree)was still in progress at review time; the other 14 checks are green, 3 skipped.- Live-cluster paths a static review cannot run: the rolling 1.19 to 1.20 xDS window (reported down to ~15s with xdsMode:split), the SNAT datapath change, and SSA on the running agent and envoy DaemonSets. Replay with
cozystack-pr-testonce the stamped operator image is in the registry.
Checked and correct
- Routing is tenant-safe: conflict detection is per Gateway, translation is one CEC per Gateway with its own LB Service, and child-tenant TLSRoutes are confined to
<child>.<apex>by the hostname policy, so they do not attach to the root'stls-<svc>listener. - The patched branches are a no-op for HTTPS-only or passthrough-only Gateways, so unrelated cilium Gateway-API users are unaffected.
helm unittest packages/system/cilium: 25/25 on the tree merged with current main (includes the new envoy-xds-mode split test).
| + } | ||
| + h := m.normalizedListenerHostname(https) | ||
| + p := m.normalizedListenerHostname(passthrough) | ||
| + if helpers.SNIHostnamesIntersect(h, p) && !strictlyNarrowerHostname(p, h) { |
There was a problem hiding this comment.
[NIT] Conflict status message still says 'overlapping hostnames' even when overlap is now allowed
The rule became intersect AND NOT strictlyNarrower, but the status message in listenerConflictMessage (status_listener.go, untouched by the patch) still reads same port 443 has overlapping HTTPS and TLS passthrough hostnames. An operator who pairs HTTPS api.example.test with passthrough *.example.test gets that message, while the reverse (HTTPS *.example.test + passthrough api.example.test) is accepted, with no hint that the direction of narrowness is what decides it. Purely cosmetic: both listeners are still correctly dropped before translation in the conflict case. Consider making the message name the narrower-passthrough rule.
There was a problem hiding this comment.
Changed in the existing patch, the message for that pair now ends with "and the TLS passthrough hostname is not strictly narrower than the HTTPS one." Your exact pair (HTTPS api.example.test, passthrough *.example.test) has a unit case, and the goldens carry the new text.
Cilium 1.19.6 and later mark an HTTPS-terminate listener and a TLS passthrough listener on the same port Conflicted whenever their hostnames intersect, and 1.20 drops both from Envoy. That rejects *.<apex> HTTPS next to api.<apex> passthrough on 443, which a tenant Gateway with one address needs. Envoy selects the most specific server_names match before the transport protocol, so the pair maps every SNI to exactly one filter chain. The operator is rebuilt from the upstream tag with a patch that accepts the pair when the passthrough hostname is strictly narrower than the HTTPS one, and keeps every other overlap conflicted. The asymmetry is needed because passthrough chains take their server_names from the attached TLSRoutes: under a passthrough wildcard a route may claim the HTTPS listener's exact name, and two chains with one match make Envoy refuse the whole listener. The binary replaces the one in the upstream operator-generic image, and the builder runs the patched packages' tests, so a tag bump that breaks the patch fails the build. make update re-pins both upstream images by the digests the vendored chart carries, the builder by upstream's own Go toolchain, and the source archive by its checksum. The image ref sits under operator.image.override: committed as the upstream operator the rebuild starts from, so a tree rendered without a build still pulls an image that exists, and stamped by make image. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
When a Gateway needs per-port Envoy listeners, the operator emits one listener per HTTPS port and, with two or more passthrough ports, one per passthrough port, all named listener-<port>. A port carrying both gets two listeners with one name, and the agent skips the whole CiliumEnvoyConfig, so every listener of the Gateway stops serving while its status stays green. With one passthrough port the port is instead mapped to two listeners in the CEC services. A tenant Gateway with HTTPS and passthrough on 443 plus a passthrough listener on a native port reaches the first case. The patch puts the passthrough filter chains of such a port on its HTTPS listener and leaves the port out of the passthrough listeners and service mappings. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The 1.20 chart renders envoy-xds-mode "ads" and an adsConfig bootstrap for cilium-envoy unless xdsMode or upgradeCompatibility says otherwise. A 1.19 agent never registers the aggregated discovery service, and a 1.20 agent in ADS mode registers nothing else. The agent and envoy DaemonSets roll independently, so on every node where their versions differ envoy gets no configuration until both pods are replaced, and Gateway traffic stops on all nodes at once during the upgrade. Pin xdsMode to split, the per-type xDS both versions serve and the agent's own default when the key is absent. Moving to ADS is a separate step once every node runs 1.20. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The operator rebuild compiles one Cilium release with the Go toolchain upstream's builder image pins at that tag, and make update re-pins the builder line from there, along with the operator-generic base, its digest and the source archive checksum. A Renovate bump of that line, by tag or by digest, would build the release with a toolchain upstream never shipped it with, and the next make update would revert it without notice. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The patch header cited a commit of cilium/cilium#49232, an open PR whose branch is force-pushed, so the commit is reachable from no ref. Cite the PR alone. It also read as if the build would flag the patch once upstream carries the fix. It does not: a fix that lands in another place leaves this patch applying cleanly as a second early return, so the drop is a manual step. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
3238d79 to
e1319b5
Compare
|
IvanHunters, the message NIT is fixed in e1319b5, details in the thread. |
xdsMode: split is in 8658d5d with a unittest. The in-place upgrade rerun from 1.19.5 had 15s of Gateway downtime instead of ~80s, details in the reply above.
## What this PR does This raises the cilium-agent memory limit from 4Gi to 24Gi. The 4Gi limit from #3495 has no room for the agent's BPF maps. On nodes with roughly a terabyte of RAM or more, the agent is OOM-killed on every start after a reboot, and the node loses pod networking. The maps grow with node RAM. Cilium sizes its CT, NAT, neighbor and socket rev-NAT maps as a share of total RAM (`bpf-map-dynamic-size-ratio`, 0.0025 by default), and caps each map at 16Mi entries. These are LRU hash maps, so the kernel preallocates them in full. Since Linux 5.11 that memory is charged to the memcg of the process that creates the maps, which is the agent container. This is also why the upgrade itself works. A map reused from bpffs stays charged to the cgroup that created it. The failure shows up on the first reboot after the upgrade, when the maps are recreated inside the limited cgroup. Policy maps don't belong on that list. The issue and the upstream chart's configmap comment both say the ratio sizes them, but at this version they are LPM tries sized by `bpf.policyMapMax`, and they allocate per entry. One DaemonSet serves every node, so the limit can't follow node RAM. The map sizes are capped, though, so a fixed limit can cover any node. With every map at 16Mi entries for both IPv4 and IPv6, the maps take just under 20Gi of kernel memory. Add the 4Gi the agent already had and you get 24Gi. The request stays at 256Mi. The limit doesn't reserve memory, and the pod cgroup still gets `memory.max`, which is what #3495 was after. What it costs is the leak backstop. The agent is now stopped at 24Gi minus its own map memory, so on most nodes a leak in its heap is no longer caught before the node feels it. If someone prefers a tighter backstop over IPv6 headroom, they can lower `cilium.resources.limits.memory` in the networking component values. About 13Gi still covers the IPv4-only map ceiling. I didn't pin the map sizes (`bpf.ctTcpMax`, `bpf.natMax` and so on). No single value works for every node. A value small enough for a 16Gi node cuts the conntrack capacity of a 1.5TiB node many times over. A pinned size also differs from what almost every node above ~14Gi of RAM runs today, so the agent would recreate those maps on upgrade and drop their conntrack and NAT state. A lower ratio has the same problem and doesn't lower the ceiling. The new test keeps the ratio at its default and the sizes unpinned, so changing either has to be deliberate. The figures are computed, not measured. I read the sizing code at cilium 1.19.5 (`pkg/option/config.go`), and 1.20.2, which #3343 moves to, has the same code. Memory comes from that formula plus the kernel hash map layout: a 48-byte element header, the key and value rounded up to 8 bytes, and a 16-byte bucket per power of two of `max_entries`. By that math the IPv4 maps alone pass 4Gi at roughly a terabyte of RAM. Bucket rounding moves the exact point, so I wouldn't read more precision into it. For the 1.5TiB node in the report, the issue's 3.9GiB is the agent's own budget (0.25% of RAM), and the kernel's actual charge comes out higher. Follow-ups, not in this PR: - Operators who want smaller maps on large nodes can size them per node with a `CiliumNodeConfig` selected by node label. This needs a documentation note. - The reporter's `bpftool` measurement of the real map memory is still pending. That is the measured check for the numbers above. The cilium helm-unittest suite passes on this branch, and also with this change applied over the 1.20.2 chart from #3343. Each new assertion fails when its value changes: the limit, the CT, NAT and neighbor sizes, the socket rev-NAT size set through `extraConfig`, and the ratio. Closes #4800 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The diff changes only `packages/system/cilium/values.yaml` and its test. None of the trigger-map entries apply: no app, schema, platform value, variant, release asset or tooling changes, and no website page documents the agent limit. ### Release note ```release-note fix(cilium): raise the cilium-agent memory limit from 4Gi to 24Gi so it covers the agent's BPF maps, which cilium sizes from node RAM. On nodes with around a terabyte of RAM or more, the 4Gi limit got the agent OOM-killed on every start after a reboot, leaving the node without pod networking. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Increased the Cilium agent memory limit to 24 GiB. * Updated validation for BPF map sizing: checks the dynamic size ratio and confirms that specific map-size settings are absent. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
What this PR does
This moves Cilium from v1.19.5 to v1.20.2, both the vendored chart and the agent image build.
The bump is for #3721. On v1.19.5, platform HTTPRoutes attached to a shared tenant Gateway get
NotAllowedByListeners(cilium/cilium#42159). The fix is cilium/cilium#45693, first released in v1.19.6. I skip v1.19.6 on purpose. Starting there, Cilium marks two listeners on one port, one HTTPS terminate and one TLS passthrough, asProtocolConflictwhen their hostnames intersect (#3912). On 1.19.x only the status breaks: traffic still flows, but the TenantGateway goesReady=Falseand tenant releases time out. On 1.20 both listeners are dropped from Envoy. The listener layout has to work on 1.20 anyway, so a stop at 1.19.x would only need its own workaround. This PR patches the operator for it, see below.This PR also rebuilds cilium-operator from the v1.20.2 source with two patches. With the first one, an HTTPS-terminate listener and a TLS-passthrough listener on one port are accepted when the passthrough hostname is strictly narrower than the HTTPS one, and every other overlap stays conflicted. Envoy matches
server_namesbefore the transport protocol and prefers the most specific name, so*.<apex>next toapi.<apex>sends every SNI to exactly one filter chain. The rule is one-sided because a passthrough chain takes its names from the attached TLSRoutes: under a wildcard passthrough listener a route can claim the HTTPS listener's exact name, and two chains with one match make Envoy refuse the whole listener. With the patch a tenant Gateway keeps*.<apex>and the passthrough listeners on 443 in every certificate mode, as it does on 1.19.5. Gateway API lets an implementation refuse this pair, and the rule is still open in kubernetes-sigs/gateway-api#4271. Upstream Cilium waits for it, so this patch stays downstream.The second patch fixes cilium/cilium#48269. When a Gateway has passthrough listeners on two ports and one of those ports also carries HTTPS, the operator emits two Envoy listeners named
listener-<port>, and the agent skips the whole CiliumEnvoyConfig while the Gateway status stays green. A tenant with atlsPassthroughListenersentry next to the passthrough services on 443 hits it. It is upstream as cilium/cilium#49248.The patched binary goes into the upstream
operator-genericimage in place of the original. The build runs the tests of the patched packages, andmake updatechecks that the patches still apply to a new tag. The newcilium-operatorrepository in the e2e registry has to be made public after the first CI build.This PR still must not reach main without #4638. The two form stack #4717, and one case is left for it. Under
http01, an HTTPRoute that claims a passthrough hostname such asapi.<apex>makes the controller render a terminate listener with that same hostname next totls-api. Equal hostnames still conflict, so on 1.20 both are dropped. The Gateway is off by default.The IPv6 passthrough BPF patch had to be regenerated. v1.20 moved the ARP case of
do_netdevunderENABLE_IPV4, which broke the anchor of the first hunk. The patch's ETH_P_IPV6 case now sits right beforedefault:, outside theENABLE_IPV4block. The other two hunks apply unchanged with--fuzz=0. TheDockerfilepinsquay.io/cilium/cilium:v1.20.2by digest. I left the agent digest invalues.yamlalone, since it is release-managed and CI rebuilds the image for this PR.I rendered all five networking variants before and after. Things worth knowing from that diff:
operator-genericv1.20.2 and Envoy is v1.37.6.envoy.xdsMode: split, the per-type xDS both versions serve and the agent's own default. Moving to ADS is a separate step once every node runs 1.20.bpf-lb-sock-hostns-onlyandbpf-lb-algorithm-annotationtotrue. The first one changes the datapath. The renderedbpf-lb-sock: "false"does not turn socket LB off, because the agent enables it wheneverkube-proxy-replacementistrue, and it istruein every variant. So on 1.19.5 pod connections to Services were translated atconnect(). On 1.20.2 socket LB works in the host namespace only, and pod traffic to ClusterIP and NodePort Services is translated per packet at the client pod. The second key only matters for a Service that carries the annotation.ciliumGateway whoseTerminatelistener has anything but exactly onecertificateRef. The TenantGateway controller always renders exactly one.TLSRouteandBackendTLSPolicyatv1and turns Gateway API off without them. The shipped v1.6.1 CRD bundle serves both.This bump also carries a patch to the Cilium image. Upstream commit 990f79be17 (cilium/cilium#43955), new in v1.20.0, makes bpf_host treat any host-marked packet as a NodePort SNAT conflict on every interface it is attached to, and that path overwrites the host mark. With kube-ovn chaining, host traffic to a pod on the same node leaves through ovn0, so the pod saw it as
worldinstead ofhost. That broke admission webhooks behind aworlddeny (the API server timed out calling the replica on its own node), kubelet probes under ingress policies that do not allowworld, the talos-log-collector push path, and any user default-deny policy that relied on the implicit host allow.The patch is taken verbatim from the open upstream PR, so it can simply be dropped later. It comes from cilium/cilium#49232, which fixes cilium/cilium#49208. It skips the conflict check when the destination is a local endpoint, so that traffic behaves like on 1.19 again. On a three-node kube-ovn test cluster with the original
worlddeny in place, webhook calls from the API server's own node went from 0/30 to 30/30. A readiness probe under acluster-only policy passed, and traffic to pods on other nodes did not change. The patch goes away once a Cilium release we ship includes the fix. Refs #4781Two things stay for #4638. The TenantGateway controller and the gateway README describe v1.19.5 behaviour in several places, and the port-443
allowedRoutes.kindsworkaround from #3087 is still in place. Both depend on the listener layout that PR changes, so I rewrite them there against v1.20.2. One related change does come with this bump. Since v1.19.6 Cilium enforcesallowedRoutes.kinds, where before it only reported the violation and attached the route anyway. Nothing first-party depends on the old behaviour, but a tenant GRPCRoute, TCPRoute or UDPRoute that attached only because of it stops attaching.I tested the patched operator on a three-node kube-ovn cluster, with the agent from this branch. On upstream 1.20.2,
*.exampleon HTTPS andapi.exampleon passthrough, both on 443, came upConflicted. With the patch all listeners were accepted and every case passed 30/30 from each node. Wildcard names terminated at the Gateway,apireached the backend with its own certificate, and a passthrough listener on a second port worked next to them. The same layout with disjoint names reproduced cilium/cilium#48269 on upstream 1.20.2: the agent loggedduplicate Listener name, and the second port served nothing.Checked locally: every variant renders,
helm unittestpasses, the BPF patches dry-run against v1.20.2 with--fuzz=0, andmake updatereproducescharts/byte for byte. CI covers fresh installs only. I ran an in-place upgrade from 1.19.5 together with #4638 on a live cluster. Pod and service traffic had no loss, but the Gateway on 443 was down for about 80 seconds while cilium-envoy rolled. That run predates thexdsMode: splitpin, which addresses the cause found in the source; the upgrade has not been re-measured with it yet. The Cilium upgrade notes recommend the latest patch of the current minor first.Downstream repositories
Release note