fix(webhook): set EXTERNAL-IP when a service is changed to LoadBalancer; add --load-balancer-ip [KS-75] - #178
Conversation
…er; add --load-balancer-ip [KS-75] The mutating webhook was registered for CREATE only, and nothing else watches Services, so admission was the sole trigger for populating status.loadBalancer.ingress. Flipping an existing Service from ClusterIP to LoadBalancer via kubectl apply or helm upgrade is an UPDATE, so it never received an EXTERNAL-IP and the only workaround was to delete and recreate the Service. Split the webhook rule in two rather than adding UPDATE globally. Pods, PVCs and jobs stay CREATE-only because pod spec.nodeName and job spec.template are immutable after creation, so returning those patches on an update would make the apiserver reject the request. Only services match CREATE and UPDATE. services/status is not matched, so the status patch this triggers does not re-enter the webhook
|
Note PR image published: |
There was a problem hiding this comment.
🟡 Not ready to approve
The webhook Service struct uses sync.WaitGroup but the code calls wg.Go(...), which will not compile without adjusting the waitgroup implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
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 gap in the mutating webhook’s LoadBalancer handling so that Services flipped from ClusterIP to LoadBalancer on UPDATE (e.g., kubectl apply / helm upgrade) still get status.loadBalancer.ingress populated, and adds a --load-balancer-ip override for the published EXTERNAL-IP.
Changes:
- Scope the webhook rules so only
servicesmatchCREATE+UPDATE, while pods/PVCs/jobs remainCREATE-only. - Add
--load-balancer-ip(and runtime resolution) to control the EXTERNAL-IP independently of the detected node IP. - Add unit + e2e coverage for the UPDATE path, plus dry-run safety for out-of-band status patching.
File summaries
| File | Description |
|---|---|
| types/types.go | Adds Embedded.LoadBalancerIP to carry the chosen EXTERNAL-IP through runtime wiring. |
| internal/config/flags/flags.go | Introduces --load-balancer-ip and updates LoadBalancer flag description to include UPDATE behavior. |
| internal/runtime/network/ip.go | Adds ResolveLoadBalancerIP() to validate/choose the published EXTERNAL-IP. |
| internal/runtime/network/ip_test.go | Unit tests for ResolveLoadBalancerIP() behavior and fallbacks. |
| cmd/kubesolo/main.go | Computes loadBalancerIP and wires it into embedded runtime configuration. |
| pkg/kubernetes/apiserver/service.go | Passes Embedded.LoadBalancerIP into the webhook constructor. |
| pkg/kubernetes/webhook/service.go | Renames stored IP from nodeIP to loadBalancerIP and embeds it into the status patch payload. |
| pkg/kubernetes/webhook/webhooks.go | Skips out-of-band status mutation on dry-run requests; uses loadBalancerIP for logging. |
| pkg/kubernetes/webhook/loadbalancer.go | Retries when a stale read returns the pre-update Service type; patches status when committed. |
| pkg/kubernetes/webhook/loadbalancer_test.go | New unit tests validating retry behavior and status patching for CREATE vs UPDATE paths. |
| pkg/kubernetes/webhook/config.go | Splits webhook rules so services include UPDATE while others remain CREATE-only. |
| test/e2e/manifests/06-lb-update/lb-update.yaml | New e2e tier manifest covering ClusterIP→LoadBalancer flip, plus guard Job. |
| test/e2e/manifests/06-lb-update/flip-to-lb.yaml | Update overlay manifest applied via kubectl apply to exercise UPDATE admission. |
| test/e2e/manifests.sh | Adds tier6 e2e flow, shared polling helper, and webhook-rule assertions. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…assertion Addresses Copilot review on #178. The Service path patches status out of band, so None was inaccurate; the dry-run guard makes NoneOnDryRun the correct declaration. The e2e assertion used a wildcard that would still pass if PVCs or jobs were dropped from the CREATE-only rule
The mutating webhook was registered for CREATE only, and nothing else watches Services, so admission was the sole trigger for populating status.loadBalancer.ingress. Flipping an existing Service from ClusterIP to LoadBalancer via kubectl apply or helm upgrade is an UPDATE, so it never received an EXTERNAL-IP and the only workaround was to delete and recreate the Service.
Split the webhook rule in two rather than adding UPDATE globally. Pods, PVCs and jobs stay CREATE-only because pod spec.nodeName and job spec.template are immutable after creation, so returning those patches on an update would make the apiserver reject the request. Only services match CREATE and UPDATE. services/status is not matched, so the status patch this triggers does not re-enter the webhook