Repository navigation
fix(networking): harden webhook policy assumptions - #4058
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe changes describe ChangesWebhook routing and validation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix 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: Check backend identity and restart state
ChainsawTest->>AdmissionService: Probe the ClusterIP
ChainsawTest->>EndpointSlices: Recheck the selected backend
ChainsawTest->>ControllerPod: Recheck controller readiness
Merge Risk: ⚪ Minimal · up to This change clarifies the supported Cilium routing requirement, requires an explicit admission webhook port, and tightens the gateway admission test. No actionable merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
size/XL
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
myasnikovdaniil LGTM. The corrected kubeProxyReplacement=false story matches the tree, and the tighter gateway probe closes the stale-evidence gaps it targets.
I checked the new claim against the topology. kubeovn/values.yaml sets ENABLE_LB: false, the documented installs disable kube-proxy, and the Kamaji control plane in apps/kubernetes ships only the konnectivity addon. So with the value off there is nothing left to translate a ClusterIP, and the old "webhook reopens" wording described a path that doesn't exist. The probe now reads the admission port by name, and the vendored chart does call it https-webhook on both the Service and its EndpointSlice. E2E Tests passed on this head on 2026-09-03 (attempt 1).
On a trial merge with current main, the helm suites for cilium, ingress-nginx, kubeovn-webhook and apps/kubernetes all pass. Dropping the new required and going back to dig "port" 8443 turns the new ingress-nginx case red.
One small thing about the description, not blocking. Reading the port from the parent's .Values["ingress-nginx"] again, with required kept, still passes all 24 tests. That's because Helm already merges subchart defaults into the parent's view of that key, so main was reading the effective port too, and the .Subcharts switch is a cleanup. The real fix is the fail-closed required. The PR body and the first commit say the port source changed, so you may want to reword that before merge, since the body becomes the merge commit message.
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]>
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]>
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]>
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]>
69dd1a0 to
fd0a0fa
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
myasnikovdaniil I rebased this onto current main.
github-actions
Bot
added
the
area/networking
2 participants
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=falsestatus 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
Summary by CodeRabbit
Bug Fixes
Documentation
kubeProxyReplacement: true.Tests