feat(ingress-nginx): bump to 1.11.5 and restrict the admission webhook - #3798

No actionable comments were generated in the recent review. 🎉

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 @coderabbitai help to get the list of available commands.}

Applied exactly as you proposed — the deny is now two selectors that OR together: everything outside kube-system with no label condition at all, and inside kube-system the existing NotIn on k8s-app. Only the agent in its own namespace is exempt.

You're right that this was the whole access decision rather than a hardening detail. With enableDefaultDeny.ingress: false and no ingress section, falling out of the deny is being allowed, so a label carve-out spanning every namespace meant any pod anywhere could reach :8443 by labelling itself.

I re-checked the three things the fix rests on rather than taking them on faith. Nothing in the platform constrains pod labels by admission, so the label is freely settable. Cilium keeps k8s-app identity-relevant — pkg/labelsfilter/filter.go ships no default whitelist, so a forged label genuinely changes the endpoint's identity rather than being dropped. And Kamaji at the pinned 26.3.6-edge sets nothing but k8s-app: konnectivity-agent on the agent pod template (internal/resources/konnectivity/agent.go), so the label stays the only discriminator available — which is why confining it by namespace is the fix rather than picking a more specific label.

Tests cover both halves, since the interesting property here is an absence: the outside-kube-system selector must carry no label condition (asserted by expression count plus an explicit notContains), and the kube-system selector must keep exactly the namespace/label pair. The previous test asserted operator: Exists on the namespace key, which the split makes wrong, so it was rewritten.

CodeRabbit flagged the same thing independently on this file, which is a fair signal it was findable rather than exotic. Good catch before it shipped.

One thing from your last line stays open: the chart refresh really is unreviewed. The vendored charts/ingress-nginx/ now carries upstream's own test suites, which the package's test: target skips via --with-subchart=false because they assert against upstream defaults rather than ours — so they are inert here, but that also means nothing in CI reads them. If you have appetite for a second pass, that diff and the two files in patches/ are where the remaining risk sits.

Confirmed, all of it, and the diagnosis goes one layer deeper than "fix the luarocks step" — that layer changes what the fix has to be.

Verified independently first: controller:v1.11.5 does resolve to the pinned sha256:b78ae118, its config reports Created: 2025-03-26T08:46:21Z — your date to the day, and before the template cleanup — the fork's only CI run (31722204918) failed on the controller step while the exporter step passed, and protobuf-exporter:v1.11.5 now resolves to sha256:1bd53e41 against the sha256:1e60d533 pinned here. So the image was never rebuilt and the tag has been pointing at a March build all along.

Where it gets more interesting is why the luarocks step fails. I reproduced it in the exact base image and walked the obvious fixes; each one is wrong:

• The rock was not yanked — lua-protobuf 0.4.1-1 is still published.
• Bumping luarocks does not help. 3.11.1 fails identically to 3.0.0.
• Nor does the busybox-vs-GNU wget difference, though it is real: luarocks invokes wget --no-cache --tries=1 --timestamping, which busybox does not support. Fixing that only changes the error.

With a new enough luarocks and GNU wget, the manifest downloads and then fails to load:

Failed loading manifest for https://luarocks.org/...:
main function has more than 65536 constants

That is LuaJIT's hard per-function constant limit. The luarocks.org index has simply grown past what LuaJIT can compile, so every query returns "No results matching query were found" no matter which client asks. Nothing in the Dockerfile rotted — the ecosystem moved out from under it, which is also why a build that worked in March started failing on its own with no commit to blame.

The fix that actually works, verified end to end in registry.k8s.io/ingress-nginx/controller:v1.11.5, is three things together:

1. Install the rocks by direct rockspec URL, bypassing the index entirely: luarocks install https://luarocks.org/manifests/xavier-wang/lua-protobuf-0.4.1-1.rockspec.
2. Add git to the apk build deps — the rockspec fetches its sources over git, and without it the install dies on 'git' program not found.
3. Move lua-iconv from 7-3 to 7.1.0-1. The 7-3 rockspec points at https://github.com/downloads/ittner/lua-iconv/lua-iconv-7.tar.gz, and GitHub removed the /downloads/ scheme years ago, so that version is unfetchable regardless. 7.1.0-1 builds from a live git tag.

With those, both rocks install cleanly.

That lands in cozystack/ingress-nginx-with-protobuf-exporter, not here, and republishing the controller image is a decision rather than a mechanical follow-up, so I am not pushing it unilaterally — it needs an owner to sign off on the rebuild.

On the exporter digest: agreed, and I would rather refresh both in one commit once the controller image exists than re-pin the exporter alone now. A half-refreshed pair would make the PR look updated while the controller is still the March build.

And your last point stands — the "with policy / Ingress create works" row cannot have come from this digest, since a crash-looping controller serves no webhook. The policy was exercised against a working controller and the digest bump against a broken one, so the two halves have indeed never run together. Both get re-run against the republished image before this merges.

Review: LGTM with non-blocking notes

TL;DR: verified end-to-end on a real cluster — the policy works and blocks nothing legitimate. Deployed the actual v1.11.5 controller (09ff83ca, no more buildHTTP3Listener crashloop) together with the policy on a live Kamaji-hosted tenant, and admission behaves exactly as intended. No blockers; the notes below are hardening for invariants a future change could silently break.

Reviewed at HEAD 091d8ce1. The version bump and the deny-only CiliumNetworkPolicy are correct, and the split-deny genuinely closes the forged-label hole. I ran the policy on a live Kamaji-hosted tenant cluster (Cilium 1.19.5, konnectivity-agent at 10.243.x in the pod CIDR) and confirmed:

• admission works through the konnectivity carve-out (Ingress create succeeds under the policy);
• a plain pod is denied on :8443 (timeout);
• a pod in default that forges k8s-app: konnectivity-agent stays denied — the split closes Daniil's hole;
• removing the konnectivity carve-out makes every Ingress create fail with context deadline exceeded, so the exemption is load-bearing, exactly as the design claims.

No blockers. The notes below are all non-blocking, but the first two are worth acting on because they encode invariants that a future change can silently violate.

[MINOR] The host-network comment names the wrong mechanism, and the real one is version-dependent

templates/admission-webhook-networkpolicy.yaml:21-23 says host-network callers are safe because "fromEndpoints never matches non-endpoints". That is not what protects them. Verified against Cilium source:

• On Cilium >= 1.18, addClusterFilterByDefault (pkg/k8s/apis/cilium.io/utils/utils.go) injects an implicit k8s:io.cilium.k8s.policy.cluster=<name> equality into every fromEndpoints selector. Pods carry that label; the reserved host/remote-node identities do not, so they fall out of deny selector A. That injection is absent in v1.17.6 and earlier (grep-confirmed against the tagged source).
• Without it, selector A is just namespace NotIn [kube-system], and a NotIn requirement is satisfied when the key is absent. Reserved identities carry no io.kubernetes.pod.namespace label, so selector A matches host/remote-node on its own terms. On a management cluster the host-network kube-apiserver then lands in the deny, and with failurePolicy: Fail every Ingress create/update times out cluster-wide.
• The filter also needs a cluster name: addClusterFilterByDefault no-ops when the name is PolicyAnyCluster (""). Cozystack ships cluster-name: default, so it fires — but that is a second implicit precondition.

This repo pins Cilium 1.19.5 with cluster-name: default, so as merged the policy behaves correctly (my live test was on 1.19.5). The problem is the comment states a false invariant. Someone porting this pattern to a CiliumClusterwideNetworkPolicy, to a cluster on 1.17, or to a setup with an empty cluster name would reintroduce the exact hole. Suggest: correct the comment to name the real mechanism (implicit cluster-label filter, Cilium >= 1.18, non-empty cluster name), and change the values.yaml note from "Requires Cilium" to "Requires Cilium >= 1.18".

[MINOR] ~50 lines of # comments render verbatim into every emitted object

Same file, the # blocks at lines 5-37 and 52-69 are YAML comments, not {{/* */}}, so they are not stripped at render. helm template copies all of them into the live CiliumNetworkPolicy in every cluster and every tenant-ingress namespace, and they drift stale the moment the selectors change (this is how the wrong-mechanism claim above ships into every object). Keep the one to two lines that explain the load-bearing split; move the CVE background and the Kamaji source-file internals to the docs site with a one-line pointer.

[MINOR] The policy renders even where the webhook is disabled

The template gates only on admissionWebhookPolicy.enabled (default true). In extra/ingress for non-root tenants, controller.admissionWebhooks.enabled: false is set, so nothing listens on 8443, yet a full deny policy is still emitted per tenant-ingress namespace. It is inert, but it is dead config shipped per tenant. Consider gating on dig "controller" "admissionWebhooks" "enabled" true $inc so the policy tracks the webhook it protects.

[MINOR] The webhook port is templated but never tested, and its failure mode is silent

dig "controller" "admissionWebhooks" "port" 8443 falls back to 8443, which is also the value every test asserts. If the dig path ever goes stale (upstream key rename, wrapper restructure), rendering silently degrades to the hardcoded 8443 and all eight tests stay green — while a webhook moved to another port would be left open to every pod. Add one test that overrides controller.admissionWebhooks.port and asserts both deny rules follow it.

What actually breaks admission (for the release decision)

The static analysis and the live test both say the policy is correct on this platform. The residual risk is not in the code, it is in the invariants it depends on:

1. The whole tenant carve-out rests on k8s-app: konnectivity-agent in kube-system, which Kamaji owns, not cozystack. A Kamaji upgrade that renames that label, adds a second one, or moves the agent's namespace makes the exemption miss and every Ingress create/update in every tenant fail at once. This is the one to watch on the pinned 26.3.6-edge bump. Worth a comment in the policy pointing at the exact Kamaji version whose pod template was verified, so the coupling is visible at the next bump.
2. The management-cluster path depends on the Cilium >= 1.18 + non-empty-cluster-name invariant above, which is currently satisfied but undocumented.
3. A webhook-port change (finding 4) or a controller-label change degrades to fail-open silently — the webhook goes back to being reachable by every pod with no test going red.

None of these block the merge. They are the surfaces to keep an eye on, and (1) is the one that would take down every tenant if Kamaji shifts under it.

All three points are addressed. The first two landed yesterday; the third is the one that needed a live cluster, and I ran it just now.

Image and digests. The luarocks failure was not the version — the luarocks.org manifest has outgrown LuaJIT's 65536-constant limit, so no client can load the index at all. Fixed by installing the rockspecs by direct URL, bypassing the index: cozystack/ingress-nginx-with-protobuf-exporter#6, merged, v1.11.5 re-pointed, build green. Both digests re-pinned in 091d8ce14 — controller sha256:09ff83ca, exporter sha256:cd4a5b7f. You were right that the exporter pin was a build behind; it is refreshed together with the controller as you suggested.

Both halves together. Ran on a Kamaji-hosted tenant cluster with the addon enabled and valuesOverride pinning exactly the digests this PR pins.

The controller that used to crash-loop now runs:

controller         ghcr.io/.../controller@sha256:09ff83ca8916...          restarts: 0
protobuf-exporter  ghcr.io/.../protobuf-exporter@sha256:cd4a5b7f35af...   restarts: 0

Then, in order: an Ingress created before the policy, the policy applied (Policy validation succeeded), and an Ingress created under the policy on that controller. So admission through the konnectivity tunnel survives the deny, on the rebuilt image — the row you flagged in the verification table is now backed by a run where both halves were actually present.

The deny direction, with a control. Same pod, same forged k8s-app: konnectivity-agent label, same namespace outside kube-system, same target:

400 is the webhook answering a GET on an AdmissionReview endpoint, i.e. the connection completed. Without the second row the first would prove nothing.

The cluster is back to its recorded baseline: addon disabled, valuesOverride cleared, test namespace, Ingresses, probe pod and policy removed.