Skip to content

fix(networking): harden webhook policy assumptions - #4058

Open
myasnikovdaniil wants to merge 4 commits into
mainfrom
fix/3997-webhook-policy-followups
Open

fix(networking): harden webhook policy assumptions#4058
myasnikovdaniil wants to merge 4 commits into
mainfrom
fix/3997-webhook-policy-followups

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented Sep 3, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

What this PR does

This is follow-up for #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 stay unchanged, and this does not claim root cause for #3983.

Screenshots

Not applicable.

Downstream repositories

Release note

fix(networking): use effective ingress admission port, report unsupported Cilium service routing correctly, and reject stale backend evidence in gateway tests

Summary by CodeRabbit

  • Bug Fixes

    • Admission webhook connectivity checks now validate the exact Service and backend endpoint throughout probing, improving failure detection and diagnostics.
    • 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.

@myasnikovdaniil
fix(networking): correct webhook policy assumptions
454aea1 
Read the ingress admission port from the effective subchart values and fail if the port is absent. Describe kube-proxy replacement as a required service-routing invariant while retaining target-side enforcement.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>
@myasnikovdaniil
test(networking): harden webhook regression proof
4d7d144 
Probe one exact ready admission EndpointSlice backend throughout the measurement window and reject missing or ambiguous Service ports. Ensure the kubeovn-webhook Helm suites run through package discovery.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>
@myasnikovdaniil
docs(networking): qualify webhook translation fallback
e3fbb6c 
Limit the fail-open discussion to the actual counterexample: another component translating the Service after Cilium source-side egress policy. A disabled translator by itself leaves no working Service path.

Assisted-By: LLM
Signed-off-by: Myasnikov Daniil <[email protected]>
@myasnikovdaniil
test(networking): bind deny probe to live pod
69dd1a0 
Require a real admission ClusterIP and carry the EndpointSlice target UID into the direct-backend coordinate. Tie that coordinate to a non-empty current Pod snapshot so stale endpoints or masked list failures cannot produce a policy verdict.

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

coderabbitai Bot commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 991c7ad0-cce9-4b47-bf83-5eefd858d3b5

📥 Commits

Reviewing files that changed from the base of the PR and between e94fcf8 and 69dd1a0.

📒 Files selected for processing (13)
  • hack/e2e-chainsaw/gateway/chainsaw-test.yaml
  • packages/apps/kubernetes/templates/helmreleases/cilium.yaml
  • packages/apps/kubernetes/tests/cilium_kube_proxy_replacement_test.yaml
  • packages/system/cilium/templates/kube-proxy-replacement-beacon.yaml
  • packages/system/cilium/tests/kube_proxy_replacement_test.yaml
  • packages/system/cilium/values.yaml
  • packages/system/ingress-nginx/templates/admission-webhook-egress-policy.yaml
  • packages/system/ingress-nginx/tests/admission_webhook_egress_policy_test.yaml
  • packages/system/ingress-nginx/values.yaml
  • packages/system/kubeovn-webhook/Makefile
  • packages/system/kubeovn-webhook/templates/_helpers.tpl
  • packages/system/kubeovn-webhook/templates/networkpolicy.yaml
  • packages/system/kubeovn-webhook/values.yaml

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The changes define kubeProxyReplacement: true as required for supported ClusterIP routing, require an explicit admission webhook port, update related policy documentation and tests, and strengthen the admission probe’s Service and backend validation.

Changes

Webhook routing and validation

Layer / File(s) Summary
Cilium service-routing invariant
packages/system/cilium/..., packages/apps/kubernetes/...		 Beacon messages, HelmRelease text, values documentation, and tests describe kubeProxyReplacement: false as unsupported for ClusterIP routing.
Admission webhook policy contract
packages/system/ingress-nginx/...		 The policy reads the subchart webhook port and fails when it is unset. Documentation and tests reflect the supported service-translation behavior.
Kube-OVN webhook policy alignment
packages/system/kubeovn-webhook/...		 Policy comments describe the supported kube-ovn topology. The Makefile adds a local Helm unit-test target.
Exact admission backend probing
hack/e2e-chainsaw/gateway/chainsaw-test.yaml		 The test uniquely discovers Services, selects a complete ready backend, probes the admission ClusterIP, and revalidates the backend before, during, and after probing.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 69dd1

The webhook routing assumptions, policy configuration checks, and admission-backend probe validation are aligned without an identified merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant ChainsawTest
  participant AdmissionService
  participant EndpointSlices
  participant ControllerPod
  ChainsawTest->>AdmissionService: Discover ClusterIP and named ports
  ChainsawTest->>EndpointSlices: Select a ready backend
  ChainsawTest->>ControllerPod: Verify backend identity and restart state
  ChainsawTest->>AdmissionService: Probe the ClusterIP
  ChainsawTest->>EndpointSlices: Revalidate the selected backend
  ChainsawTest->>ControllerPod: Revalidate controller readiness
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: strengthening webhook policy assumptions and networking behavior. It is concise and specific enough for project history.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (13 skipped: 13 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/3997-webhook-policy-followups

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/bug Categorizes issue or PR as related to a bug labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lexfrei Aleksei Sviridkin (lexfrei) Awaiting requested review from lexfrei lexfrei is a code owner
@kvaps Andrei Kvapil (kvaps) Awaiting requested review from kvaps kvaps is a code owner
@lllamnyp Timofei Larkin (lllamnyp) Awaiting requested review from lllamnyp lllamnyp is a code owner
@IvanHunters IvanHunters Awaiting requested review from IvanHunters IvanHunters is a code owner
@sircthulhu Kirill Ilin (sircthulhu) Awaiting requested review from sircthulhu sircthulhu is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Milestone

No milestone

Development

See More

Successfully merging this pull request may close these issues.

1 participant

@myasnikovdaniil