fix(cilium): bump vendored chart to v1.19.6 - #3343
fix(cilium): bump vendored chart to v1.19.6#3343Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
|
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
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
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
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
|
Moving this to draft. The bump itself is clean, but v1.19.6 adds a listener conflict check that our default Gateway layout walks straight into, and that needs a decision before the version moves.
The check itself: for t in v1.19.5 v1.19.6; do
printf "%s: " "$t"
curl -sL "https://raw.githubusercontent.com/cilium/cilium/$t/operator/pkg/gateway-api/gateway_reconcile.go" |
grep -c samePortCrossProtocolConflictedListeners
done # v1.19.5: 0, v1.19.6: 2This is source-level only. I have not measured what it does on a live cluster, and the conditions on the rendered Gateway are worth confirming there before anyone commits to a shape of fix. Three ways out that I can see: drop the wildcard listener, move passthrough off 443, or hold at 1.19.5 and take it upstream. Only the last one leaves the public contract untouched. Parking this in draft until that call is made. |
Refresh the vendored upstream Cilium chart and agent image from v1.19.5 to v1.19.6 via `make update`. v1.19.5 rejects platform app HTTPRoutes on shared Gateways: CheckGatewayAllowedForNamespace early-returns false on the first namespace-selector miss instead of continuing to later listeners (cilium#42159). v1.19.6 carries the fix (cilium#45693, 1.19 backport cilium#46826). The IPv6-passthrough BPF patch (pass IPv6 to the kernel stack when the IPv6 datapath is disabled under host firewall) still applies cleanly to v1.19.6 bpf/bpf_host.c with no offset or fuzz; the update target's dry-run confirms it, so the patch is carried unchanged. Keep the upstream base image pinned by digest alongside the tag. The update target rewrites ARG VERSION from the chart version and cannot carry a digest, so a plain bump would leave the one image built here resolving a mutable tag. The digest is the manifest digest of quay.io/cilium/cilium:v1.19.6 and matches image.digest in the vendored chart values. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The tls-passthrough listener comment claimed the cosmetic ResolvedRefs=False/InvalidRouteKinds condition was removable once cilium#45693 shipped, so bumping to v1.19.6 would satisfy the stated trigger and invite a reader to drop the port443Kinds workaround. Two things were wrong about that framing, and both cut the same way. The condition is not an argument for keeping port443Kinds, it is a consequence of it: gateway_reconcile.go compares len(supportedKinds) against len(AllowedRoutes.Kinds), and the mismatch exists only because port443Kinds puts two kinds on a listener whose protocol supports one. That check is byte-identical in v1.19.5 and v1.19.6, so the noise does not clear by itself. And cilium#45693 is not unrelated to the workaround: it fixes the multi-listener namespace and kind restrictions, it is backported to 1.19, and in v1.19.6 CheckGatewayRouteKindAllowed filters listeners by sectionName and port rather than scanning them all last-one-wins, which is the collapse port443Kinds exists to defeat. Reword to state what the source shows: where the cosmetic condition comes from, that removing the workaround is now an open question rather than a settled no, and that answering it needs its own change plus a cluster check because the two-kind set is pinned by a test. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The healer pinned the host-scope IPAM restore-window race to "no released fix as of v1.19.5". The v1.19.6 bump was not verified to fix that race, so the watermark would silently no longer apply to what the platform runs. Reword to be version-agnostic rather than claim a fix that has not been established. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
05b4cc4 to
2b80d63
Compare
What this PR does
Refreshes the vendored upstream Cilium chart and the agent image build from v1.19.5 to v1.19.6 via
make update.The motivating fix is cilium#42159: on v1.19.5,
CheckGatewayAllowedForNamespaceearly-returnsfalseon the first namespace-selector miss instead of continuing to later listeners, so platform app HTTPRoutes (Keycloak, dashboard, and others) attaching to a shared tenant Gateway are wrongly rejected withNotAllowedByListeners. v1.19.6 carries the upstream fix (cilium#45693, 1.19 backport cilium#46826).The bump also picks up the upstream host-firewall config fix (cilium#44748):
enable-host-firewallis now emitted unconditionally, so togglinghostFirewall.enabledfrom true to false no longer leaves a stale value in the live ConfigMap. Rendering the package both ways puts a number on what that changes here: the default variant emits"true"before and after, and the only rendered difference across both variants is the kilo one, where the key goes from absent to"false". That is Cilium's own default, so the behaviour is unchanged and only the explicitness is new.The IPv6-passthrough BPF patch (pass IPv6 to the kernel stack when the IPv6 datapath is disabled under host firewall) applies cleanly to v1.19.6
bpf/bpf_host.cwith no offset or fuzz and is carried unchanged.One thing worth knowing before reading this diff for completeness: the agent image and the operator image move on separate clocks.
packages/system/cilium/values.yamlpins the first-party agent image by digest, and a chart bump does not repin it (the previous bump to v1.19.5 left it alone as well), so the agent keeps running its last build until that image is rebuilt. The fix this PR exists for lives in the operator, inoperator/pkg/gateway-api/gateway_reconcile.go, and the operator image is pinned by the vendored chart. Rendering the package on both sides showsoperator-genericgoing v1.19.5 to v1.19.6 while the agent image stays put, so the motivating fix ships with this change regardless of when the agent is next rebuilt.Areas worth exercising on a running cluster: Gateway API multi-listener cross-namespace HTTPRoute attachment; host-firewall with the IPv6 datapath disabled (node IPv6 / ICMPv6 ND); and the
hostFirewall.enabledtrue→false ConfigMap behavior on the kilo variant.Release note
Summary by CodeRabbit