Create an Internal Load Balancer if configured #1222

bfournie · 2024-05-01T15:42:06Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Provide the ability to configure the types of Load Balancers to be created (Internal and/or External). By default, an External Proxy Load Balancer will be created per the current implementation. If set for an Internal Load Balancer, an Internal Passthrough Load Balancer will be created using resources in the specified region.

Which issue(s) this PR fixes :
Fixes #1221

Special notes for your reviewer:

TODOs:

[x ] squashed commits
[x ] includes documentation
[ x] adds unit tests

Release note:
-->

Added a new `LoadBalancerTypes` field to `LoadBalancerSpec` that defines the type of Load Balancers (External or Internal) which should be created. By default a Global External Proxy Load Balancer will be created as is currently implemented. 
Also added an `InternalLoadBalancer` field to optionally configure the name and subnet of the Internal Load Balancer.

k8s-ci-robot · 2024-05-01T15:42:15Z

Hi @bfournie. 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/test-infra repository.

netlify · 2024-05-01T15:42:24Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

Name	Link
🔨 Latest commit	`1a8192b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-gcp/deploys/665728d33ad9e600085b37ff
😎 Deploy Preview	https://deploy-preview-1222--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 site configuration.

cpanato

/ok-to-test

barbacbd · 2024-05-08T15:21:36Z

api/v1beta1/types.go

+ // APIInternalAddress is the IPV4 regional address assigned to the
+ // internal Load Balancer.
+ // +optional
+ APIInternalAddress *string `json:"apiInternalIpAddress,omitempty"`


I understand that this is a breaking change, but should we consider a structure to hold these values? The added values are the same as those for the api server, so a lot of duplicated values appear.

The main reason I didn't do that was because it is a breaking change, I tried to keep the same API for consistency and just add the additional fields.

If the consensus is that its OK to change this API I can make it an array.

make sense as we are adding this to add as a struct

wdyt @damdo

OK, how would we handle the API in this case though? Would the existing fields - APIServerAddress, APIServerHealthCheck etc need to be marked as deprecated but still handled for a few releases until the deprecation is complete? It seems like it will add a lot of complexity to this API.

I tend to agree with @bfournie here.
I'd probably stick with a non-breaking change, and then schedule a full API re-review for a new v1beta2 where we can improve the situation and plan the breaking changes we want to make to the API to clean it up.

api/v1beta1/types.go

cloud/scope/cluster.go

damdo · 2024-05-09T09:05:47Z

cc. @nrb

bfournie · 2024-05-14T12:40:40Z

/hold

cloud/services/compute/loadbalancers/reconcile.go

Remove the configuration of the GCP Internal Load Balancer from the installer as its now being done in the CAPG provider. This allows only the Internal LB to be created to support Private Clusters. This depends on kubernetes-sigs/cluster-api-provider-gcp#1222 which is still under review.

bfournie · 2024-05-16T09:39:15Z

/unhold

bfournie · 2024-05-29T16:22:49Z

i am always happy :)

just a question; this will not break any existing clusters that eventually upgrade to this version, correct?

Correct, it will not break existing clusters on upgrade which will not have LoadBalancerType set so only an External LB will created which is the current behavior, see https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1222/files#diff-a7274316ed5d60f25cc61e1bcb771a612c3566832a59ecc3a13b279a849c95ebR61

cpanato · 2024-05-29T18:03:58Z

/approve
/lgtm

thanks!

cpanato · 2024-05-29T18:04:08Z

will wait for the e2e tests :)

k8s-ci-robot · 2024-05-29T18:04:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bfournie, cpanato

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cpanato]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bfournie · 2024-05-29T18:07:34Z

will wait for the e2e tests :)

Thanks @cpanato! I created a separate PR for the e2e tests with some questions #1245

Can we merge this first and then focus on the e2e test in #1245 ?

cpanato · 2024-05-29T18:10:11Z

/unhold

bfournie · 2024-05-29T18:25:09Z

/unhold

Thank you

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

jwmay2012 · 2024-06-04T20:04:35Z

I'm having an issue with the Internal only loadbalancer type. When cluster-api brings everything up, the LB never passes the healthcheck.
I can get the LB to be healthy by:

deleting the "frontend/Forwardingrule" and allowing CAPG to recreate it. immediately healthy when it is recreated.
OR manually creating a second internal passthrough LB, identically configured (or slightly different, but 'correct') to the one created by CAPG. Both LB's become healthy. After deleting the new LB, the original stays healthy.

Seems super weird. Creating a new identical frontend/forwardingrule or recreating the old one causes the healthcheck to start passing and the LB to start accepting requests.

This is for a 1 node cluster.

If there's a better place to post this, I can do that.

bfournie · 2024-06-05T01:00:51Z

I'm having an issue with the Internal only loadbalancer type. When cluster-api brings everything up, the LB never passes the healthcheck. I can get the LB to be healthy by:

deleting the "frontend/Forwardingrule" and allowing CAPG to recreate it. immediately healthy when it is recreated.

OR manually creating a second internal passthrough LB, identically configured (or slightly different, but 'correct') to the one created by CAPG. Both LB's become healthy. After deleting the new LB, the original stays healthy.

Seems super weird. Creating a new identical frontend/forwardingrule or recreating the old one causes the healthcheck to start passing and the LB to start accepting requests.

This is for a 1 node cluster.

If there's a better place to post this, I can do that.

So this was with just the Internal LB or both External and Internal LB?
We have a test now for both External and Internal. I haven't seen that issue with the
healthcheck not passing. For Internal only most of my testing has been using this as a provider of Openshift, I haven't seen this in particular but will investigate it.

Probably best to create a Issue under https://github.com/kubernetes-sigs/cluster-api-provider-gcp/issues so we can track it.

jwmay2012 · 2024-06-05T17:05:34Z

Internal Only LB. No external. I'll work to create an issue soon.

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 1, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 1, 2024

k8s-ci-robot requested review from cpanato and dims May 1, 2024 15:42

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 1, 2024

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 1, 2024

cpanato reviewed May 2, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 2, 2024

bfournie force-pushed the internal-load-balancer branch 3 times, most recently from 2d101b7 to e312d9c Compare May 2, 2024 20:19

barbacbd reviewed May 8, 2024

View reviewed changes

bfournie force-pushed the internal-load-balancer branch from e312d9c to 173c005 Compare May 9, 2024 13:33

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2024

nrb reviewed May 15, 2024

View reviewed changes

cloud/services/compute/loadbalancers/reconcile.go Show resolved Hide resolved

bfournie force-pushed the internal-load-balancer branch 4 times, most recently from 6db2752 to f5670bf Compare May 15, 2024 19:26

bfournie mentioned this pull request May 15, 2024

WIP CORS-3296: CAPG - configure Internal LoadBalancer in the provider openshift/installer#8421

Open

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2024

k8s-ci-robot merged commit e5efa79 into kubernetes-sigs:main May 29, 2024
17 of 18 checks passed

k8s-ci-robot added this to the v1.1.0 milestone May 29, 2024

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

b5b0e30

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

60789f5

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

9c54206

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

165cf6a

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

b0be1ab

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

f1d24e1

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

307a5ec

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request May 31, 2024

WIP: Add e2e test for internal load balancer

b6ed46c

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request Jun 2, 2024

WIP: Add e2e test for internal load balancer

25eb857

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request Jun 3, 2024

WIP: Add e2e test for internal load balancer

9595691

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request Jun 3, 2024

WIP: Add e2e test for internal load balancer

91abab8

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request Jun 3, 2024

WIP: Add e2e test for internal load balancer

f7f653b

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie mentioned this pull request Jun 3, 2024

Delete load balancers before subnets #1252

Merged

3 tasks

bfournie added a commit to bfournie/cluster-api-provider-gcp that referenced this pull request Jun 3, 2024

WIP: Add e2e test for internal load balancer

32fe7ee

Adds an e2e test when the internal load balancer is configured. This requires kubernetes-sigs#1222

bfournie deleted the internal-load-balancer branch June 4, 2024 12:54

jwmay2012 mentioned this pull request Jun 5, 2024

Internal Only LoadBalancer doesn't pass healthcheck out of the box. #1256

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an Internal Load Balancer if configured #1222

Create an Internal Load Balancer if configured #1222

bfournie commented May 1, 2024 •

edited

k8s-ci-robot commented May 1, 2024

netlify bot commented May 1, 2024 •

edited

cpanato left a comment

barbacbd May 8, 2024

bfournie May 8, 2024

cpanato May 20, 2024

bfournie May 20, 2024

damdo May 24, 2024

damdo commented May 9, 2024

bfournie commented May 14, 2024

bfournie commented May 16, 2024

bfournie commented May 29, 2024

cpanato commented May 29, 2024

cpanato commented May 29, 2024

k8s-ci-robot commented May 29, 2024

bfournie commented May 29, 2024

cpanato commented May 29, 2024

bfournie commented May 29, 2024

jwmay2012 commented Jun 4, 2024

bfournie commented Jun 5, 2024

jwmay2012 commented Jun 5, 2024

Create an Internal Load Balancer if configured #1222

Create an Internal Load Balancer if configured #1222

Conversation

bfournie commented May 1, 2024 • edited

k8s-ci-robot commented May 1, 2024

netlify bot commented May 1, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

cpanato left a comment

Choose a reason for hiding this comment

barbacbd May 8, 2024

Choose a reason for hiding this comment

bfournie May 8, 2024

Choose a reason for hiding this comment

cpanato May 20, 2024

Choose a reason for hiding this comment

bfournie May 20, 2024

Choose a reason for hiding this comment

damdo May 24, 2024

Choose a reason for hiding this comment

damdo commented May 9, 2024

bfournie commented May 14, 2024

bfournie commented May 16, 2024

bfournie commented May 29, 2024

cpanato commented May 29, 2024

cpanato commented May 29, 2024

k8s-ci-robot commented May 29, 2024

bfournie commented May 29, 2024

cpanato commented May 29, 2024

bfournie commented May 29, 2024

jwmay2012 commented Jun 4, 2024

bfournie commented Jun 5, 2024

jwmay2012 commented Jun 5, 2024

bfournie commented May 1, 2024 •

edited

netlify bot commented May 1, 2024 •

edited