fix: allow updating a MultiClusterIngress that already carries an invalid load balancer IP - #7812
Conversation
…alid load balancer IP The validating webhook rejects any update to a MultiClusterIngress whose status already holds an address that fails IP validation, so such an object can never be changed again, not even to fix it. The status is not written by Karmada. aggregateIngressStatus collects it from member clusters, where the ingress controller writes whatever it writes, and nothing validates it on the way in. Once an address that our parser rejects lands there, the object is stuck. Upstream hit the same problem and fixed it in kubernetes@ad22c0d4, "Fix IP/CIDR validation to allow updates to existing invalid objects", by passing the old status in and tolerating addresses that are already present. The lifted copy predates that commit. This applies the same approach: ValidateIngressLoadBalancerStatus now takes oldStatus, and an address carried over from it is left alone. A fresh invalid address is still rejected. The regression test fails without the change and passes with it. doc.go regenerated with hack/update-lifted.sh, which flipped the entry to changed=Y. Signed-off-by: Anton Karpov <karpovantonme@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @karpovantonme! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/M
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped, aligns with upstream behavior, updates all call sites, and includes targeted unit coverage for the regression scenario.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a validating-webhook edge case where a MultiClusterIngress update could be permanently blocked if its existing status.loadBalancer already contained an IP string that fails validation (e.g., written by a member-cluster ingress controller and aggregated into Karmada).
Changes:
- Update lifted
ValidateIngressLoadBalancerStatusto acceptoldStatusand skip IP validation for IPs already present in the old status. - Wire the webhook update path to pass the old load balancer status into the validator.
- Add a unit test covering tolerance of an existing invalid IP and update lifted metadata to reflect the divergence.
File summaries
| File | Description |
|---|---|
| pkg/webhook/multiclusteringress/validating.go | Passes old load balancer status to the lifted validator during update admission. |
| pkg/util/lifted/validatingmci.go | Extends the lifted validator to tolerate IPs already present in oldStatus. |
| pkg/util/lifted/validatingmci_test.go | Adds/adjusts tests for the new validator signature and the “tolerate existing invalid IP” behavior. |
| pkg/util/lifted/doc.go | Marks the lifted function as changed (Y) to match the updated lifted annotations. |
Review details
Files not reviewed (1)
- pkg/util/lifted/doc.go: Generated file
- Files reviewed: 3/4 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7812 +/- ##
=======================================
Coverage 42.10% 42.10%
=======================================
Files 879 879
Lines 54857 54862 +5
=======================================
+ Hits 23095 23099 +4
- Misses 30019 30020 +1
Partials 1743 1743
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/ok-to-test |
What type of PR is this?
/kind bug
What this PR does / why we need it:
A
MultiClusterIngresswhose status already holds an address that fails IP validation cannot be updated any more. The validating webhook rejects every update, including one that would fix the object. It is stuck.The status is not written by Karmada.
aggregateIngressStatuscollects it from member clusters:Whatever the ingress controller in a member cluster wrote ends up here, and nothing validates it on the way in. So an address our parser rejects can land in the status without anyone doing something wrong.
Then
validateMCIUpdatehas the old object in hand but does not pass it down:so the same address is re-validated on every update and the update is denied.
Upstream ran into this and fixed it in kubernetes@ad22c0d4, "Fix IP/CIDR validation to allow updates to existing invalid objects", by taking
oldStatusand tolerating addresses already present in it. The lifted copy here comes fromrelease-1.27and predates that commit, so the fix never reached us.hack/verify-lifted.shdoes not catch this. It only checks thatdoc.gomatches the annotations; it does not compare a lifted copy against upstream.Which issue(s) this PR fixes:
None filed. Happy to open one first if you would rather discuss it there.
Special notes for your reviewer:
The change follows upstream rather than inventing anything:
ValidateIngressLoadBalancerStatusnow takesoldStatus, and an address carried over from it is left alone. A freshly introduced invalid address is still rejected, so this does not weaken validation for new objects.Verification:
TestValidateIngressLoadBalancerStatusToleratesExistingIPfails without the change and passes with itgo test ./pkg/util/lifted/ ./pkg/webhook/multiclusteringress/- okhack/verify-lifted.sh- up to date,doc.goregenerated withhack/update-lifted.sh, which flipped the entry tochanged=Yhack/verify-gofmt.sh- cleanTwo more lifted copies look like they also missed upstream fixes. I did not touch them, since each needs its own reading of how Karmada uses the code, and one of the two turned out not to apply once I read the caller:
SetFeatureGateDuringTestinvisitpod_test.goreturns a restore function fordefer, upstream moved totb.Cleanupin 9fcf279e because a parallel subtest can outlive the parent test. Test helper, lower stakes.GetDeletableResourcesindiscovery.goswallows the discovery error, and upstream made it return one in c9a084d5. I checked the caller inpkg/detector/detector.goand it does not need the error: it only adds informers for resources it has not seen yet, so a partial list is picked up on the next tick. Mentioning it so nobody re-derives that.Say the word if you want the first one done too.