Skip to content

Commit 5486d4f

Browse files
authored
fix(networking): harden webhook policy assumptions (#4058)
## What this PR does This is follow-up for [#3997](#3997). It reads ingress-nginx admission port from effective subchart values and refuses empty value, and fixes `kubeProxyReplacement=false` status because cozystack has no supported Service translator in that state. Gateway test now requires one real ClusterIP and keeps same ready Pod UID, IP and port for whole probe, so stale EndpointSlice, replaced Pod or wrong Service can't report deny. Policy selectors and two-ended deny from [#3997](#3997) stay unchanged, and this does not claim root cause for [#3983](#3983). ### Screenshots Not applicable. ### 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: ### Release note ```release-note fix(networking): use effective ingress admission port, report unsupported Cilium service routing correctly, and reject stale backend evidence in gateway tests ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Admission webhook connectivity checks now validate the configured Service and its exact backend endpoint throughout probing, improving failure detection. - Admission webhook policies now fail closed when the webhook port is not explicitly configured. - **Documentation** - Clarified that supported service routing requires `kubeProxyReplacement: true`. - Updated guidance to explain that disabling this setting or webhook policies does not provide a supported fallback. - **Tests** - Added coverage for missing webhook port configuration and strengthened networking stability checks. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents ac4ab1c + fd0a0fa commit 5486d4f

13 files changed

Lines changed: 306 additions & 209 deletions

File tree

‎hack/e2e-chainsaw/gateway/chainsaw-test.yaml‎

Lines changed: 142 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2086,18 +2086,54 @@ spec:
20862086
20872087
svcs=$(kubectl -n "$ns" get svc -l "$sel" \
20882088
-o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}')
2089-
adm=$(printf '%s\n' "$svcs" | grep -- '-admission$' | head -1 || true)
2089+
adm_candidates=$(printf '%s\n' "$svcs" | grep -- '-admission$' || true)
20902090
# -metrics is excluded explicitly rather than relying on
2091-
# ingress-nginx-controller sorting before ingress-nginx-controller-metrics
2092-
# under head -1: the positive control has to be the Service that
2093-
# actually exposes the controller's https port.
2094-
ctl=$(printf '%s\n' "$svcs" | grep -v -- '-admission$' | grep -v -- '-metrics$' | grep . | head -1 || true)
2095-
[ -n "$adm" ] || { echo "SETUP FAILURE: no ingress-nginx admission Service in $ns" >&2; exit 1; }
2096-
[ -n "$ctl" ] || { echo "SETUP FAILURE: no ingress-nginx controller Service in $ns" >&2; exit 1; }
2091+
# ingress-nginx-controller sorting before a metrics Service: the
2092+
# positive control has to be the one unambiguous controller Service.
2093+
ctl_candidates=$(printf '%s\n' "$svcs" | grep -v -- '-admission$' | grep -v -- '-metrics$' | grep . || true)
2094+
adm_count=$(printf '%s\n' "$adm_candidates" | grep -c . || true)
2095+
ctl_count=$(printf '%s\n' "$ctl_candidates" | grep -c . || true)
2096+
[ "$adm_count" -eq 1 ] || {
2097+
echo "SETUP FAILURE: expected one ingress-nginx admission Service in $ns, found $adm_count" >&2
2098+
printf ' candidates: %s\n' "${adm_candidates:-<none>}" >&2
2099+
exit 1
2100+
}
2101+
[ "$ctl_count" -eq 1 ] || {
2102+
echo "SETUP FAILURE: expected one ingress-nginx controller Service in $ns, found $ctl_count" >&2
2103+
printf ' candidates: %s\n' "${ctl_candidates:-<none>}" >&2
2104+
exit 1
2105+
}
2106+
adm=$adm_candidates
2107+
ctl=$ctl_candidates
20972108
2098-
adm_port=$(kubectl -n "$ns" get svc "$adm" -o jsonpath='{.spec.ports[0].port}')
2099-
ctl_port=$(kubectl -n "$ns" get svc "$ctl" -o jsonpath='{.spec.ports[?(@.name=="https")].port}')
2100-
[ -n "$ctl_port" ] || { echo "SETUP FAILURE: $ctl exposes no https port" >&2; exit 1; }
2109+
service_port() {
2110+
kubectl -n "$ns" get svc "$1" -o json | jq -r --arg port_name "$2" '
2111+
[.spec.ports[]?
2112+
| select(.name == $port_name)
2113+
| select((.protocol // "TCP") == "TCP")
2114+
| .port
2115+
| select(type == "number")]
2116+
| if length == 1 then .[0] else empty end
2117+
'
2118+
}
2119+
adm_port=$(service_port "$adm" https-webhook)
2120+
ctl_port=$(service_port "$ctl" https)
2121+
adm_cluster_ip=$(kubectl -n "$ns" get svc "$adm" -o jsonpath='{.spec.clusterIP}')
2122+
[ -n "$adm_cluster_ip" ] && [ "$adm_cluster_ip" != None ] || {
2123+
echo "SETUP FAILURE: $adm must have a real ClusterIP, found ${adm_cluster_ip:-<empty>}" >&2
2124+
kubectl -n "$ns" get svc "$adm" -o yaml >&2 2>&1 || true
2125+
exit 1
2126+
}
2127+
[ -n "$adm_port" ] || {
2128+
echo "SETUP FAILURE: $adm must expose exactly one TCP https-webhook port" >&2
2129+
kubectl -n "$ns" get svc "$adm" -o yaml >&2 2>&1 || true
2130+
exit 1
2131+
}
2132+
[ -n "$ctl_port" ] || {
2133+
echo "SETUP FAILURE: $ctl must expose exactly one TCP https port" >&2
2134+
kubectl -n "$ns" get svc "$ctl" -o yaml >&2 2>&1 || true
2135+
exit 1
2136+
}
21012137
21022138
# Ready endpoint addresses behind a Service, from EndpointSlice rather
21032139
# than from pod readiness: this is the state the API server and the
@@ -2110,13 +2146,48 @@ spec:
21102146
echo "$n"
21112147
}
21122148
2113-
# Pod identity plus restart count, as one string. Compared for
2114-
# equality across the probe window, so a restart AND a replacement
2115-
# both show up -- a crash-looping controller can satisfy a readiness
2116-
# check at both ends of a window it spent dying in the middle of.
2149+
# Exact ready admission coordinates. The Pod identity, IP and backend
2150+
# port come from the same EndpointSlice as the ready condition;
2151+
# selecting them independently from .items[0] could probe an unready
2152+
# replica while a different replica kept aggregate readiness above
2153+
# zero.
2154+
ready_admission_backends() {
2155+
kubectl -n "$ns" get endpointslice -l "kubernetes.io/service-name=$adm" -o json | jq -r '
2156+
.items[]? as $slice
2157+
| $slice.ports[]? as $port
2158+
| select($port.name == "https-webhook")
2159+
| select(($port.protocol // "TCP") == "TCP")
2160+
| select(($port.port | type) == "number")
2161+
| $slice.endpoints[]?
2162+
| select(.conditions.ready == true)
2163+
| select(.targetRef.kind == "Pod")
2164+
| select((.targetRef.name // "") != "")
2165+
| select((.targetRef.uid // "") != "")
2166+
| .targetRef.name as $pod
2167+
| .targetRef.uid as $uid
2168+
| .addresses[]?
2169+
| select(type == "string" and length > 0)
2170+
| [$pod, $uid, ., ($port.port | tostring)]
2171+
| @tsv
2172+
' | sort -u
2173+
}
2174+
2175+
# Pod identity, IP and every container restart count, as one string.
2176+
# Compared for equality across the probe window, so a restart AND a
2177+
# replacement both show up -- a crash-looping controller can satisfy
2178+
# a readiness check at both ends of a window it spent dying in the
2179+
# middle of.
21172180
controller_generation() {
2118-
kubectl -n "$ns" get pod -l "$sel" \
2119-
-o jsonpath='{range .items[*]}{.metadata.name}={.status.containerStatuses[0].restartCount}{" "}{end}'
2181+
generation=$(kubectl -n "$ns" get pod -l "$sel" \
2182+
-o jsonpath='{range .items[*]}{.metadata.name}/{.metadata.uid}/{.status.podIP}={range .status.containerStatuses[*]}{.name}:{.restartCount},{end}{"\n"}{end}') || {
2183+
echo "SETUP FAILURE: unable to read ingress-nginx controller generation" >&2
2184+
return 1
2185+
}
2186+
[ -n "$generation" ] || {
2187+
echo "SETUP FAILURE: ingress-nginx controller generation is empty" >&2
2188+
return 1
2189+
}
2190+
printf '%s\n' "$generation" | sort
21202191
}
21212192
21222193
# Precondition. Both Services are backed by the same controller pods,
@@ -2125,7 +2196,7 @@ spec:
21252196
# backend to answer.
21262197
deadline=$(( $(date +%s) + 120 ))
21272198
while :; do
2128-
adm_ready=$(ready_backends "$adm")
2199+
adm_ready=$(ready_admission_backends | grep -c . || true)
21292200
ctl_ready=$(ready_backends "$ctl")
21302201
if [ "$adm_ready" -gt 0 ] && [ "$ctl_ready" -gt 0 ]; then break; fi
21312202
[ "$(date +%s)" -lt "$deadline" ] || {
@@ -2140,17 +2211,58 @@ spec:
21402211
sleep 5
21412212
done
21422213
2214+
backend=$(ready_admission_backends | head -1)
2215+
[ -n "$backend" ] || { echo "SETUP FAILURE: $adm has no complete ready Pod/UID/IP/port coordinate" >&2; exit 1; }
2216+
tab=$(printf '\t')
2217+
backend_fields=$(printf '%s\n' "$backend" | awk -F "$tab" '{ print NF }')
2218+
[ "$backend_fields" -eq 4 ] || { echo "SETUP FAILURE: malformed ready backend coordinate: $backend" >&2; exit 1; }
2219+
pod_name=$(printf '%s\n' "$backend" | cut -f1)
2220+
pod_uid=$(printf '%s\n' "$backend" | cut -f2)
2221+
pod_ip=$(printf '%s\n' "$backend" | cut -f3)
2222+
hook_port=$(printf '%s\n' "$backend" | cut -f4)
2223+
[ -n "$pod_name" ] && [ -n "$pod_uid" ] && [ -n "$pod_ip" ] && [ -n "$hook_port" ] || {
2224+
echo "SETUP FAILURE: incomplete ready backend coordinate: $backend" >&2
2225+
exit 1
2226+
}
2227+
2228+
selected_backend_ready() {
2229+
ready_admission_backends | awk -F "$tab" \
2230+
-v pod="$pod_name" -v uid="$pod_uid" -v ip="$pod_ip" -v port="$hook_port" '
2231+
$1 == pod && $2 == uid && $3 == ip && $4 == port { found = 1 }
2232+
END { exit !found }
2233+
'
2234+
}
2235+
2236+
require_selected_backend() {
2237+
selected_backend_ready && return 0
2238+
echo "SETUP FAILURE: selected admission backend $pod_name/$pod_uid ($pod_ip:$hook_port) was not ready $1" >&2
2239+
kubectl -n "$ns" get endpointslice -l "kubernetes.io/service-name=$adm" -o yaml >&2 2>&1 || true
2240+
kubectl -n "$ns" get pod "$pod_name" -o wide >&2 2>&1 || true
2241+
exit 1
2242+
}
2243+
2244+
require_selected_backend "before the stability window"
2245+
21432246
# Stability: the same evidence held across a window, not at one
21442247
# instant. Six samples over 30s, all of which must be live and show no
21452248
# restart or replacement.
21462249
gen_before=$(controller_generation)
2250+
printf '%s\n' "$gen_before" | awk \
2251+
-v prefix="${pod_name}/${pod_uid}/${pod_ip}=" '
2252+
index($0, prefix) == 1 { found = 1 }
2253+
END { exit !found }
2254+
' || {
2255+
echo "SETUP FAILURE: selected admission backend is not a current controller Pod: $backend" >&2
2256+
exit 1
2257+
}
21472258
i=0
21482259
while [ "$i" -lt 6 ]; do
21492260
sleep 5
21502261
i=$(( i + 1 ))
2151-
n=$(ready_backends "$adm")
2262+
require_selected_backend "during the stability window (sample $i)"
2263+
n=$(ready_backends "$ctl")
21522264
[ "$n" -gt 0 ] || {
2153-
echo "SETUP FAILURE: $adm lost its ready backend during the stability window (sample $i)" >&2
2265+
echo "SETUP FAILURE: $ctl lost its ready backend during the stability window (sample $i)" >&2
21542266
kubectl -n "$ns" get pod -l "$sel" -o wide >&2 2>&1 || true
21552267
exit 1
21562268
}
@@ -2164,14 +2276,7 @@ spec:
21642276
exit 1
21652277
}
21662278
2167-
pod_ip=$(kubectl -n "$ns" get pod -l "$sel" \
2168-
-o jsonpath='{.items[0].status.podIP}')
2169-
hook_port=$(kubectl -n "$ns" get pod -l "$sel" \
2170-
-o jsonpath='{.items[0].spec.containers[0].ports[?(@.name=="webhook")].containerPort}')
2171-
[ -n "$pod_ip" ] || { echo "SETUP FAILURE: no controller Pod IP in $ns" >&2; exit 1; }
2172-
[ -n "$hook_port" ] || { echo "SETUP FAILURE: controller declares no webhook port" >&2; exit 1; }
2173-
2174-
echo "probing from $ns: expect reachable ${ctl}:${ctl_port}, denied ${adm}:${adm_port}, denied ${pod_ip}:${hook_port}"
2279+
echo "probing from $ns: expect reachable ${ctl}:${ctl_port}, denied ${adm}@${adm_cluster_ip}:${adm_port}, denied ${pod_name}/${pod_uid}@${pod_ip}:${hook_port}"
21752280
21762281
kubectl -n "$ns" delete pod "$probe" --ignore-not-found --wait=true --timeout=60s
21772282
# busybox:musl is staged onto every node by hack/e2e-prepull-images.sh,
@@ -2205,7 +2310,7 @@ spec:
22052310
- name: CTL_PORT
22062311
value: "${ctl_port}"
22072312
- name: ADM_HOST
2208-
value: "${adm}.${ns}.svc"
2313+
value: "${adm_cluster_ip}"
22092314
- name: ADM_PORT
22102315
value: "${adm_port}"
22112316
- name: POD_IP
@@ -2241,9 +2346,10 @@ spec:
22412346
deadline=$(( $(date +%s) + 180 ))
22422347
phase=""
22432348
while [ "$(date +%s)" -lt "$deadline" ]; do
2244-
n=$(ready_backends "$adm")
2349+
require_selected_backend "while the probe was running"
2350+
n=$(ready_backends "$ctl")
22452351
[ "$n" -gt 0 ] || {
2246-
echo "SETUP FAILURE: $adm lost its ready backend while the probe was running, so an unreachable webhook proves nothing" >&2
2352+
echo "SETUP FAILURE: $ctl lost its ready backend while the probe was running, so an unreachable positive control proves nothing" >&2
22472353
kubectl -n "$ns" get pod -l "$sel" -o wide >&2 2>&1 || true
22482354
kubectl -n "$ns" logs "$probe" >&2 2>&1 || true
22492355
exit 1
@@ -2270,9 +2376,10 @@ spec:
22702376
kubectl -n "$ns" get pod -l "$sel" -o wide >&2 2>&1 || true
22712377
exit 1
22722378
}
2273-
n=$(ready_backends "$adm")
2379+
require_selected_backend "after the probe"
2380+
n=$(ready_backends "$ctl")
22742381
[ "$n" -gt 0 ] || {
2275-
echo "SETUP FAILURE: $adm has no ready backend after the probe, so its unreachability proves nothing" >&2
2382+
echo "SETUP FAILURE: $ctl has no ready backend after the probe, so its unreachability proves nothing" >&2
22762383
exit 1
22772384
}
22782385
@@ -2287,9 +2394,9 @@ spec:
22872394
}
22882395
done
22892396
2290-
# From here every branch is a real verdict: the destination had a live
2291-
# backend before, throughout and after, on pods that neither restarted
2292-
# nor were replaced.
2397+
# From here every branch is a real verdict: the exact direct
2398+
# destination stayed ready before, throughout and after, and the
2399+
# controller pods neither restarted nor were replaced.
22932400
rc=0
22942401
if [ "$ctl_seen" = reachable ]; then
22952402
echo "OK unrelated 443 egress works (${ctl}:${ctl_port})"

‎packages/apps/kubernetes/templates/helmreleases/cilium.yaml‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -88,21 +88,19 @@ spec:
8888

8989
{{- /*
9090
addons.cilium.valuesOverride is x-kubernetes-preserve-unknown-fields, so a
91-
tenant can set cilium.kubeProxyReplacement: false for its own cluster. That is
92-
the one path by which the value reaches a live cluster without a cozystack
93-
commit, and it is the path the guest cluster's own ingress-nginx admission deny
94-
depends on: that policy names the webhook backend port only, which covers a pod
95-
dialling the ClusterIP solely because Cilium performs the service DNAT ahead of
96-
its own egress hook. Override it and pods inside the guest cluster regain the
97-
ClusterIP path to the admission webhook while the backend deny keeps working --
98-
no failure, no log line, and the rendered policy unchanged.
91+
tenant can set cilium.kubeProxyReplacement: false for its own cluster. Cozystack
92+
does not support that value or supply an alternative service load balancer for
93+
it; guest ClusterIP connectivity can fail. The guest ingress-nginx policy's
94+
source-side deny also loses the pre-DNAT match, but its target-side ingressDeny
95+
continues to deny pods after any translation, so the override does not imply
96+
that the admission webhook becomes reachable.
9997

10098
The mismatch is detectable here rather than in the guest cluster, because this
10199
HelmRelease's values are assembled on the management cluster, so the beacon is
102100
emitted next to the release that carries the override. Same mechanism and same
103101
reasoning as the -awaiting-etcd beacon in ../cluster.yaml: a ConfigMap an
104-
operator and an alert can both see, not a fail() that would take the tenant's
105-
only CNI down over a weakened defence-in-depth control.
102+
operator and an alert can both see. It is a diagnostic for an unsupported
103+
override, not an alternative compatibility mode.
106104

107105
Absence is not a warning: the value is unset in both the override and
108106
cozystack.defaultCiliumValues on every supported path, and the cilium chart's
@@ -129,14 +127,15 @@ data:
129127
addons.cilium.valuesOverride sets cilium.kubeProxyReplacement to
130128
{{ toString $kpr | quote }} for cluster {{ .Release.Name }}.
131129
132-
Cilium in that guest cluster will not perform the ClusterIP DNAT ahead of
133-
its own egress hook, so the admission-webhook deny the guest cluster's
134-
ingress-nginx ships no longer covers traffic addressed to the webhook
135-
Service ClusterIP. Pods in the guest cluster can reach
136-
<webhook>.<namespace>.svc:443; the webhook's backend port stays denied.
130+
Cozystack supports guest cluster networking only when
131+
cilium.kubeProxyReplacement is "true". No supported alternative service load
132+
balancer is supplied for this override, so ClusterIP connectivity may fail.
137133
138-
This is a deliberate override, so it is reported rather than refused. Clear
139-
it to restore the control, or set the guest ingress-nginx addon's
140-
admissionWebhookPolicy.enabled explicitly so the posture is recorded.
134+
The guest ingress-nginx policy's source-side deny also loses its pre-DNAT
135+
match, but its target-side ingressDeny remains. This override does not imply
136+
that the admission webhook is reachable through its Service.
137+
138+
Clear the override or set cilium.kubeProxyReplacement back to true. Disabling
139+
the webhook policy does not make this networking configuration supported.
141140
{{- end }}
142141
{{- end }}

‎packages/apps/kubernetes/tests/cilium_kube_proxy_replacement_test.yaml‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
suite: tenant cilium kube-proxy-replacement override beacon
22
# addons.cilium.valuesOverride is x-kubernetes-preserve-unknown-fields, so a
33
# tenant can set cilium.kubeProxyReplacement: false for its own cluster. That is
4-
# the one path by which the value reaches a live cluster without a cozystack
5-
# commit, and it silently reopens the ClusterIP path to the guest cluster's own
6-
# ingress-nginx admission webhook -- the source-side deny there names the
7-
# backend port only, which covers the ClusterIP solely because Cilium performs
8-
# the service DNAT ahead of its own egress hook.
4+
# the one path by which the unsupported value reaches a live cluster without a
5+
# cozystack commit. It can break ClusterIP routing and removes the ingress-nginx
6+
# policy's source-side pre-DNAT match, but the target-side ingressDeny remains;
7+
# the override does not imply that the webhook becomes reachable.
98
#
109
# The override is assembled on the MANAGEMENT cluster, in this chart, which is
1110
# why the mismatch is detectable here at all. Same beacon mechanism as the
1211
# -awaiting-etcd ConfigMap in templates/cluster.yaml: something an operator and
13-
# an alert can both see, rather than a fail() that would take the tenant's only
14-
# CNI down over a weakened defence-in-depth control.
12+
# an alert can both see. It diagnoses an unsupported override rather than
13+
# defining an alternative compatibility mode.
1514
templates:
1615
- templates/helmreleases/cilium.yaml
1716
values:
@@ -61,7 +60,7 @@ tests:
6160
value: ConfigMap
6261
- matchRegex:
6362
path: data.message
64-
pattern: "will not perform the ClusterIP DNAT ahead of"
63+
pattern: "supports guest cluster networking only when"
6564
documentSelector:
6665
path: kind
6766
value: ConfigMap

0 commit comments

Comments
 (0)