fix(networking): prevent webhook admission outages during namespace churn - #3997

Exercise ingress and pod admission immediately after deleting a tenant
namespace, and route ingress-nginx changes to the gateway suite.

Assisted-By: GPT-5 <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>

Explain which callers the source-side Cilium policies select and how
the Service and backend ports are kept in agreement.

Assisted-By: GPT-5 <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>

Cilium resolves toServices only inside ALLOW rules: pkg/policy/k8s/service.go
walks spec.Egress in processRule, hasMatchingToServices and specHasToServices,
and never spec.EgressDeny. An egressDeny whose only destination is a
toServices therefore never gains a ToEndpoints/ToCIDRSet, and the
EgressDeny -> PolicyEntry conversion in pkg/policy/utils/parserules.go does not
pass ToServices to mergeEndpointSelectors at all. Its nil-guard only fires for
a non-nil empty slice, so with every other destination field absent it returns
make(types.Selectors, 0, 0) -- non-nil and empty, which
pkg/policy/types/policyentry.go documents as an implicit WILDCARD whenever the
rule carries toPorts.

The rule therefore did not deny the webhook Service on 443; it denied every
destination on TCP/443 for every endpoint its spec selected -- all pods outside
kube-system for ingress-nginx, and with endpointSelector {} every pod in the
cluster for kubeovn-webhook. Cilium deny beats allow, so the tenant
allow-external-communication policy could not offset it. Validation accepts
the construct (l3DependentL4Support marks ToServices supported), which is why
it failed silently.

Deny the backend port only. That already covers ClusterIP-addressed traffic,
because Cilium translates a service to its backends before enforcing egress
policy -- the same reason its own toServices support resolves to backend
selectors rather than the frontend. The kubeovn-webhook servicePort helper
existed only to feed the removed rule, so the Service goes back to a literal
443.

Also correct the four comments that documented the frontend deny as working,
record why the world deny is kept (cilium monitor showed the API server
arriving as host / kube-apiserver, never world), and state plainly that the
namespace-churn coupling is structural rather than a reproduced root cause for
issue 3983.

Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>

The suites asserted the toServices rule that produced the cluster-wide 443
deny, so they pinned the defect: every rendered-YAML assertion passed on the
broken policy, and removing the rule would have read as the regression.

Replace those cases with the invariant that was actually missing -- one deny
rule per spec, and its destination must be a selector Cilium can resolve
(toEndpoints present, toServices absent, spec count fixed). Verified by
mutation: reintroducing the toServices rule fails these assertions in both
packages.

Not used: matchRegexRaw / notMatchRegexRaw. helm-unittest 1.0.3 silently
ignores both -- a notMatchRegexRaw against a pattern guaranteed to be present
still reports pass -- so a document-wide regex guard would have asserted
nothing.

packages/system/kubeovn-webhook had no Makefile test target, and
hack/helm-unit-tests.sh discovers suites by probing `make -n test` per package
and skips any package without one. All three of its suites -- including the
port agreement guard, whose whole purpose is to catch a drift that fails open
-- had therefore never run in CI. Add the target; the sweep now reports
"Running tests in packages/system/kubeovn-webhook".

Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>

The churn case added alongside it proves the webhooks stay AVAILABLE. Nothing
proved they are still UNREACHABLE from pods, which is the property the
policies exist for, and no test anywhere checked that the deny is confined to
the webhook port -- which is why a rule that denied all TCP/443 egress
cluster-wide passed every existing assertion.

Probe from tenant-root, where the host cluster's ingress-nginx lives and where
allow-internal-communication permits pods to reach it, so the deny is the only
thing that can block the webhook port; from the isolated tenant-test the probe
would pass for the wrong reason. Three assertions against the same pods and
the same tenant egress allowances, differing only in port: the controller's
own HTTPS port must stay reachable, the admission Service ClusterIP must not
be, and the backend port must not be. The first is the one that separates a
scoped deny from a wildcard.

Uses busybox:musl, already staged on every node by hack/e2e-prepull-images.sh,
with a securityContext that satisfies the restricted PodSecurity profile.
Service names, ports and the backend port are discovered at runtime rather
than hardcoded, since the release name differs between installs.

Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>

The rationale above the CiliumClusterwideNetworkPolicy was a `#` block, which
Helm copies verbatim into the manifest: 53 comment lines shipped with every
render. The sibling policy added alongside it already uses `{{/* */}}`. Convert
this one to match; the text is unchanged.

Also state in the values description what `enabled: true` actually promises.
The template renders only while cilium.io/v2/CiliumClusterwideNetworkPolicy is
discoverable, so on a cluster without it the default emits no policy at all and
leaves no Event or render-time message behind. `enabled: false` is what
distinguishes an opt-out from an absent API.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

…mment

hack/package.mk's .PHONY line does not carry `test`, so a path named `test`
next to the Makefile makes the target up to date and make skips the recipe.
Proven both ways: with the declaration the three suites run with such a file
present, without it make reports "'test' is up to date" and asserts nothing.
That is the same fail-open shape the port agreement guard exists for, and
hack/helm-unit-tests.sh only catches it after the fact.

The port comment named a ValidatingWebhookConfiguration. This package ships a
MutatingWebhookConfiguration, whose clientConfig.service omits `port` so the
API server defaults it to 443. The assertion was right; the sentence explaining
it was not, and it is the only place a reader learns why 443 is pinned.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

Both admission-webhook deny policies name only their webhook's backend port.
That covers a pod dialling the ClusterIP because Cilium translates a service to
its backends before it enforces egress policy -- in v1.19.5, bpf/bpf_lxc.c runs
__per_packet_lb_svc_xlate_4 and lb4_local() and only then tail-calls into
handle_ipv4_from_lxc where policy_can_egress4 evaluates the rule.

That ordering exists only while Cilium performs the DNAT, and the only reason
it does is cilium's kubeProxyReplacement: true, set against an upstream chart
default of "false". Set it false and the DNAT moves downstream of Cilium's
egress hook: the packet arrives with dst = ClusterIP, matches no endpoint
identity, and with default-deny off it is allowed. Both webhooks regain a pod
-reachable ClusterIP path while the backend deny keeps working, so nothing
fails and nothing is logged. Same shape as the port disagreement
namespace-annotation-webhook.port already exists to prevent, and it had no pin.

The suite asserts the rendered cilium-config key rather than .Values, so an
upstream bump that renames it or stops honouring the override is caught too,
and it spells out each of the five variant valuesFiles combinations from
sources/networking.yaml so a future variant file that overrides the value reds
here. Proven both ways: flipping values.yaml reds all six cases, and adding the
override to values-kilo.yaml alone reds exactly the cilium-kilo case.

The suite also records why a target-side ingressDeny is not available as a
belt-and-braces fallback. A target-side rule needs a peer selector covering
pods, and the peer selector is what carries namespace churn -- the failure
being removed here. pkg/policy/selectorcache.go UpdateIdentities notifies a
selector only when the churned identities were in its own cached selections,
and pkg/policy/l4.go IdentitySelectionUpdated then writes the policy map of
every endpoint the rule's endpointSelector selects, which for a target-side
rule is the fail-closed webhook pods. The one churn-free peer is the wildcard,
which Cilium collapses to a single ANY-identity entry and skips identity
updates for -- but ANY covers reserved:host, remote-node and kube-apiserver,
so it would deny the API server. There is no third shape.

Refs #3983.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

Both new cases could report a verdict they had not observed, and the first CI
run of them did exactly that against a crash-looping ingress-nginx controller.

admission-webhooks-survive-namespace-churn died in setup: the admission Service
had no ready backend, so the first apply -- which passes through that same
fail-closed webhook -- failed with "no route to host" before any churn ran, and
the failure read as the outage under test. It now asserts both webhooks are up
BEFORE the churn, resolving their coordinates from the webhook registrations
themselves rather than from a guessed release name, and says so in its own
words when they are not. Proven against the pre-change script with a stubbed
kubectl: with a dead admission Service the old script proceeded into the churn
(delete namespace, two applies, delete namespace, two dry-runs); the new one
stops and names the cause without touching the churn namespace.

admission-webhook-deny-still-enforced measured a corpse. Its `kubectl wait` did
observe both pods Ready, but the controller was flapping on a ~100s cycle and
the probe ran against a dead backend 19 seconds later. "Denied by policy" and
"no backend to route to" are the same observation from inside the probe, so the
two deny assertions would have reported success against a cluster with no
policy installed at all, and the port-scoping assertion reported a policy bug
that did not exist. The deny verdicts are now gated on the admission Service
having ready backends before the probe, on every poll while it runs, and after
it finishes, and on the controller pods neither restarting nor being replaced
across that window -- EndpointSlice readiness rather than pod readiness,
because a `kubectl wait` is what a flapping pod satisfies transiently. The
probe reports reachable/unreachable per destination and always exits 0; this
script holds the endpoint evidence and is the only place that can attribute a
refused connect, so anything undecidable now fails as a setup failure instead
of as a verdict.

Ten scenarios driven through both scripts under dash with a stubbed kubectl,
all behaving as specified: a dead backend, a backend dying mid-probe, a
controller restarting mid-probe, a probe that never completed, an unregistered
webhook, the deny lost, the deny widened to every 443 destination, and the
healthy path. The restart-mid-probe case is the one that used to produce a
confident false pass.

Two more things the same rewrite fixes. The step budget was 6m against inner
budgets summing to 420s, so a slow-but-healthy cluster was killed with no
verdict and no describe output; both steps now carry a timeout above their own
sum, per the rule in hack/e2e-chainsaw/.chainsaw.yaml, and the `kubectl wait`
whose 180s dominated that sum is gone. The churn namespace now carries a
running pod, because a Cilium identity is a set of endpoint labels including
k8s:io.kubernetes.pod.namespace: an empty namespace allocates no identity and
its deletion drives none of the UpdateIdentities path the old target-side
policies coupled the webhook endpoint to. And both server-dry-run Pods carry a
restricted-compliant securityContext -- never scheduled, but a Pod that
PodSecurity would reject aborts the check under set -eu and reads as the
webhook being down.

Refs #3983.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

…thing

The pin added earlier guards a cozystack developer changing the default. It
cannot observe an operator override in the field, and neither can the e2e,
which only ever runs the default configuration -- so the path Ivan's review
actually names, addons.cilium.valuesOverride on a tenant cluster and the
cozystack.networking Package's cilium component on the host, produced no signal
at all. Both webhook deny policies name their webhook's backend port only,
which covers a pod dialling the ClusterIP solely because Cilium performs the
service DNAT ahead of its own egress hook, so an override degrades that half
silently.

A status-beacon ConfigMap rather than a fail(), following the -awaiting-etcd
beacon in packages/apps/kubernetes/templates/cluster.yaml. Whether Cilium
without kube-proxy replacement is a supported cozystack configuration is a
platform decision, and refusing to render the CNI over a weakened
defence-in-depth control is not a call these charts should make. The host
beacon fires on a bool false, on the string "false", and on the umbrella key
being deleted, because the subchart's own "false" default coalesces into the
parent. The tenant beacon fires on an explicit non-true override only, since
absence means the chart default applies, and it renders on the management
cluster next to the HelmRelease carrying the override -- which is why the
mismatch is detectable there at all rather than inside the guest cluster.

Also withdraws the argument the suite header used to carry, that a target-side
ingressDeny could not be re-added as a fallback. It was wrong, and the next
commit adds that rule. The header now records what the two halves each buy.

Makefile gets .PHONY: test for the reason this branch already argued for
kubeovn-webhook, which applies with more force here now that the suite is
load-bearing for two other packages: hack/package.mk's .PHONY list deliberately
carries only the targets that file defines, so a path named `test` would
otherwise make the target up to date and skip the recipe. Proven both ways.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

…ules

Restores the deny that does not care who performs the service DNAT, which is
what the source-side rules on their own gave up. Both policies now carry it on
their webhook endpoint for the backend port, alongside the retained world deny,
so losing cilium.kubeProxyReplacement degrades enforcement rather than removing
it.

I argued against this and was wrong. The argument was that a target-side rule
needs a peer covering pods, that such a peer either reattaches namespace churn
to a fail-closed endpoint or, as a wildcard, denies the API server. The second
half does not hold: a CRD-sourced fromEndpoints is never a true wildcard,
because for a clusterwide policy Cilium rewrites every selector to also require
k8s:io.kubernetes.pod.namespace to Exist plus the cluster label (Cilium v1.19.5,
pkg/k8s/apis/cilium.io/utils/utils.go:120-154 getEndpointSelector, whose own
comment at :141 says a wildcard must not become a truly empty allow-all
selector), and no reserved identity carries either label. So this rule denies
pods and leaves host, remote-node and kube-apiserver allowed. `fromEntities:
[all]` and an omitted peer are the shapes that would reach them.

The first half was a cost dressed up as a correctness problem. A cluster-
spanning peer is enumerated per identity, so the rule does add policy-map
entries on the webhook endpoints in proportion to pod-identity count and a map
write per namespace event. It does not add a drop window:
pkg/endpoint/bpf.go:1168-1177 adds entries before deleting them expressly to
avoid transient drops, and inverts that order only when the addition would
overflow the map, warning when it does. The residual risk is therefore
cardinality, not churn -- an endpoint near MaxEntries gets the inverted order,
and cozystack leaves endpointLockdownOnMapOverflow false
(charts/cilium/values.yaml:1250) so an overflow degrades rather than locking the
endpoint down. Both templates now say that where the rule is, rather than
leaving the cost unstated.

The peer is the load-bearing part and both ways of losing it render as a
plausible ingressDeny, so each suite pins it: rule count, the exact peer
selectors, no fromEntities on the pod rule, no fromEndpoints on the world rule.
Mutation-proved by making exactly the two substitutions that would deny the API
server -- fromEntities: [all], and dropping the peer -- in both packages; all
four render cleanly and all four go red.

The ingress-nginx rule carries the konnectivity-agent ServiceAccount exemption
for the same reason the source-side rules do: a Kamaji-hosted tenant cluster
reaches this webhook through that pod, so without it guest-cluster Ingress
admission breaks.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

…Service

Three narrowing fixes to the two webhook cases.

The availability precondition resolved only the FIRST registration of each
webhook name (awk ... exit) while every matching registration fires on the
applies below, so an unrelated flaky ingress-nginx release would have aborted
an apply and been reported as a churn-induced outage. It now waits on all of
them and says which one it is waiting for. Split into require_service_up plus a
loop, indexed with sed rather than piped into `while read`, because a
pipeline's body runs in a subshell under dash and a `return` from it would not
reach the caller.

That split immediately produced its own bug, caught by the scenario harness:
POSIX functions share one scope, so the callee's `label="$1"` overwrote the
caller's copy and the second registration reported itself as "(1/2) (2/2)".
The callee now uses distinct names.

The positive control's Service was picked with `grep -v -- '-admission$' |
head -1`, which relies on ingress-nginx-controller sorting before
ingress-nginx-controller-metrics. Deterministic but undocumented, and the
control has to be the Service that actually exposes the https port, so
-metrics is now excluded explicitly.

Also drops a stale sentence: the churn pod's comment claimed the old
target-side policies coupled the webhook endpoint to identity churn, which
overstates a map write as an outage and is moot now that the target-side rule
is back. The pod is there because an empty namespace allocates no identity, so
without it the case would exercise nothing it names.

Eleven scenarios through both scripts under dash with a stubbed kubectl, all as
specified, the new multi-registration case included.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>

Restoring the target-side pod deny put a cluster-spanning peer selector back on
the webhook endpoint, which is the exact thing the source-side rules were added
to take off it. The comments still said the coupling was "structural and
removed", and the release note still said deleting a namespace no longer
rewrites that endpoint's policy map. Both were true of the previous revision
and are not true of this one.

What ships now is two denies that fail in different directions: the source-side
one keeps enforcing while the webhook endpoint's policy is being recomputed or
has overflowed its map, the target-side one keeps enforcing where Cilium is not
the service load balancer, and neither subsumes the other. So the coupling is
not eliminated, it is made non-load-bearing -- whichever end is degraded the
other still denies, and the map writes are writes rather than an outage.
Eliminating it outright would mean dropping the target-side rule and accepting
a silent fail-open, which is the worse trade.

The chainsaw case's header claimed the deletion "briefly made fail-closed
admission webhooks unreachable". That is the unconfirmed hypothesis, and the
errno in the report contradicts it: EHOSTUNREACH cannot come from a policy
denial, which drops after connect() has returned. It now says what the case
actually asserts -- both webhooks still admit across a namespace deletion --
rather than claiming to reproduce a cause.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>