-
Notifications
You must be signed in to change notification settings - Fork 465
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
Add LoadBalancer to getExternalIPs #995
Conversation
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.
Overall this looks fine to me. You'll need to update the unit tests to make it pass now that external IPs are being added for LoadBalancer service types. I think it's failing at multiple places:
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller_test.go#L293
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller_test.go#L633
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller_test.go#L897
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/ecmp_vip_test.go#L117
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/ecmp_vip_test.go#L207
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/ecmp_vip_test.go#L281
- https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/ecmp_vip_test.go#L292
Running make
should allow you to test the unit tests locally on any host that has a working docker daemon.
Have you had a chance to test your changes in a cluster just to double-check that this does what you want?
@aauren -- I can confirm the updates enable BGP advertisement of External-IPs on svc type LoadBalancer. |
I can also confirm the changes work, I'll try to update the unit tests. Thanks! |
@aauren Tests updated, I also remove the comments about |
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.
LGTM. Thanks for the PR!
LoadBalancer was not advertising externalIPs in services, this fixes it.
Fixes: #994