-
Notifications
You must be signed in to change notification settings - Fork 616
feat: set loadbalancerIP from Gateway.spec.addresses #13070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for setting the LoadBalancer service IP from Gateway.spec.addresses in the Gateway API. When a Gateway specifies an IP address in its spec, and the service type is LoadBalancer, the implementation extracts this IP and sets it as the loadBalancerIP field in the generated Kubernetes Service resource.
Key Changes
- Gateway address extraction: New
extractLoadBalancerIPfunction validates and extracts the first valid IP address fromGateway.spec.addresseswhere the type isIPAddressType(or nil, which defaults toIPAddressTypeper Gateway API spec) - Service generation: Modified
GetServiceValuesto accept an optionalloadBalancerIPparameter and added the field to theHelmServicestruct - Helm template updates: Updated both envoy and agentgateway service templates to conditionally render the
loadBalancerIPfield
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/kgateway/deployer/gateway_parameters.go |
Added extractLoadBalancerIP function with IP validation using netip.ParseAddr, and integrated it into the values generation flow for LoadBalancer-type services |
pkg/deployer/values_helpers.go |
Updated GetServiceValues signature to accept loadBalancerIP parameter and refactored to safely handle nil svcConfig |
pkg/deployer/values.go |
Added LoadBalancerIP field to HelmService struct with json omitempty tag |
pkg/kgateway/helm/envoy/templates/service.yaml |
Added conditional rendering of loadBalancerIP field using Helm with directive |
pkg/kgateway/helm/agentgateway/templates/service.yaml |
Added conditional rendering of loadBalancerIP field using Helm with directive |
test/deployer/testdata/loadbalancer-static-ip.yaml |
New test input with Gateway specifying a static IP address (203.0.113.10) in spec.addresses |
test/deployer/testdata/loadbalancer-static-ip-out.yaml |
Expected Helm output showing the loadBalancerIP field properly set in the generated Service manifest |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: omar <[email protected]>
timflannagan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also enable the conformance test that backs this type of functionality?
Signed-off-by: omar <[email protected]>
|
@timflannagan see "Additional Notes" |
Signed-off-by: omar <[email protected]>
|
lgtm |
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
Signed-off-by: omar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // SetLoadBalancerIPFromGateway extracts the IP address from Gateway.spec.addresses | ||
| // and sets it on the HelmService if the service type is LoadBalancer. | ||
| // Only sets the IP if exactly one valid IP address is found in Gateway.spec.addresses. | ||
| // Returns an error if more than one address is specified or no valid IP address is found. |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function documentation is incomplete. It states that an error is returned "if more than one address is specified or no valid IP address is found", but it doesn't clarify what happens when a single address of a non-IP type (e.g., HostnameAddressType) is provided. The current implementation returns ErrNoValidIPAddress in this case. Consider updating the documentation to explicitly state: "Returns an error if more than one address is specified, if the single address is not of type IPAddress, or if the IP address value is invalid."
| // Returns an error if more than one address is specified or no valid IP address is found. | |
| // Returns an error if more than one address is specified, if the single address is not of type IPAddress, | |
| // or if the IP address value is invalid. |
sheidkamp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes, definitely cleaner.
Signed-off-by: omar <[email protected]>
Description
Motivation:
Users want to be able to set IP of the LoadBalancer Service for a gateway. Gateway API provides Gateway.spec.addresses for this.
Changes:
Use the first Gateway.spec.addresses of type IP to set the
loadbalancerIPfield of the Service.Note that although this field is deprecated it is not going to be removed. See here for a good write-up.
Change Type
/kind feature
Changelog
Additional Notes
Did not add conformance testing as it depends on setting status which we currently don't have a good way to do from the deployer.