cluster-api-provider-gcp
cluster-api-provider-gcp copied to clipboard
kubernetes-sigs
Add support for creating an internal load balancer with predefined IP address
What type of PR is this?
/kind feature
What this PR does / why we need it: When deploying a cluster, it is useful to know the static IP of the API server before the internal passthrough load balancer is created, especially for integration with certain third-party tools. This setup is similar to how private IPs are handled in other CAPI providers (such as CAPZ), where users can predefine the private IP of the load balancer.
Which issue(s) this PR fixes: Fixes #1474
Release note:
Add an option to create a predefined static IP address for an internal passthrough load balancer.
Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
| Name | Link |
|---|---|
| Latest commit | 3121cf8ae7f546fc8d6c6d5e00a682b74d9856cd |
| Latest deploy log | https://app.netlify.com/projects/kubernetes-sigs-cluster-api-gcp/deploys/68467f7e5a513c0009705e0c |
| Deploy Preview | https://deploy-preview-1475--kubernetes-sigs-cluster-api-gcp.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify project configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Welcome @RnkeZ!
It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-gcp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/cluster-api-provider-gcp has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @RnkeZ. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
@damdo: GitHub didn't allow me to assign the following users: barbacbd.
Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide
In response to this:
/assign @barbacbd
for a review
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
@RnkeZ Thank you for all of the work you have put into the PR so far. I am interested in using this feature.
Since the IPAddress is a new field, the tests need to be updated to ensure this change works and doesn't get disturbed by a future change.
cloud/services/compute/loadbalancers/reconcile_test.go
I submitted a PR to your fork with the tests.
@RnkeZ Looks like I added a blank line where the linter doesn't want one. All that needs to be done is to delete that blank line at 616.
Error: cloud/services/compute/loadbalancers/reconcile_test.go:616:1: File is not properly formatted (gofmt)
It also looks like test static_address_set_for_internal_load_balancer is not properly configured or the code isn't properly setting the static address.
From what I can tell, s.internaladdresses = tt.mockAddress doesn't like the fact that a static address is configured via s.GCPCluster.Spec.LoadBalancer.InternalLoadBalancer.IPAddress = ptr.To("1.2.3.4")
I wonder if the static address needs to be set in Objects: map[meta.Key]*cloud.MockAddressesObj{},
I will work on fixing the tests and submit a new PR to your branch @RnkeZ
Okay, I'm away from my PC atm, however, if it means anything, we've had this feature running in production for the last few weeks, and it works as expected. We had to build our own image as it was pretty urgent to get it up and running.
The failures are due to the tests. I will get them fixed and send you a new PR.
@barbacbd @damdo Is there anything else that needs to be done for approval?
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: cpanato, RnkeZ
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [cpanato]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@damdo I hope your are doing well. Do you have some time to spare to review this PR?
I am looking to use this by August.
Hey @swarren83 I am away on PTO until next week. Maybe @barbacbd and @salasberryfin or @richardcase can take a look
Is this feat going to be scheduled in a Milestone for release? If so when will it get released?
Unfortunately, it looks like we missed the release window since we didn't get a second approval.
Dam asked me to take a look at this. From a high level, I don't see anything wrong with this request or this approach.
/lgtm
