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.

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bfournie
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

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

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

Name	Link
🔨 Latest commit	`eb4a0ad`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-gcp/deploys/664f4e5274ebca000856f350
😎 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

cpanato · 2024-05-20T11:53:49Z

/test ls

k8s-ci-robot · 2024-05-20T11:53:51Z

@cpanato: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-gcp-build
/test pull-cluster-api-provider-gcp-e2e-test
/test pull-cluster-api-provider-gcp-make
/test pull-cluster-api-provider-gcp-test
/test pull-cluster-api-provider-gcp-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-gcp-apidiff
/test pull-cluster-api-provider-gcp-capi-e2e
/test pull-cluster-api-provider-gcp-conformance
/test pull-cluster-api-provider-gcp-conformance-ci-artifacts
/test pull-cluster-api-provider-gcp-coverage
/test pull-cluster-api-provider-gcp-e2e-workload-upgrade

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-gcp-apidiff
pull-cluster-api-provider-gcp-build
pull-cluster-api-provider-gcp-e2e-test
pull-cluster-api-provider-gcp-make
pull-cluster-api-provider-gcp-test
pull-cluster-api-provider-gcp-verify

In response to this:

/test ls

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.

cpanato · 2024-05-20T11:54:01Z

/test pull-cluster-api-provider-gcp-conformance

jwmay2012 · 2024-05-20T15:15:24Z

Thanks for working on this @bfournie. We would really like this as well!

I pulled this down and when running it with INTERNAL_ONLY it seems to get stuck on deletion.
It was only deleting global resources like s.forwardingrules.Delete but the existing resources were created as regionalforwardingrules.

When I added stuff like

if err := s.regionalforwardingrules.Delete(ctx, key); err != nil && !gcperrors.IsNotFound(err) {
	log.Error(err, "Error updating a regional forwardingrule", "name", spec.Name)
	return err
}

to loadbalancers/reconcile.go functions

deleteForwardingRule()
deleteBackendService()
deleteHealthCheck()

I was then able to reliably get cluster deletion to work without manual intervention or getting stuck.

I'm also noticing the Internal Passthrough LB is getting created with 6443 as the kube api endpoint, but the kubeconfig generated is expecting 443 and never finishes setting up the cluster. I think I understand that passthrough LBs don't allow port mapping so would the solution be to either?--

change the kubeconfig port from 443 to 6443
change the kube api service and LB from 6443 to 443

I haven't found the code to change the generated kubeconfig port yet, but I think ClusterNetwork.APIServerPort is where I can change the kube api port.
Not sure what the plans are here, but I'll continue to play round with it.

bfournie · 2024-05-20T16:01:27Z

api/v1beta1/types.go

+	// LoadBalancerType defines the type of Load Balancer that should be created.
+	// If not set, a Global External Proxy Load Balancer will be created by default.
+	// +optional
+	LoadBalancerType *LoadBalancerType `json:"loadBalancerType,omitempty"`


I had thought about but felt that type of API presents too much ambiguity and potential for error. For example with that type of array you could have the following potential combinations, all of which are in error:

empty (no LBs defined)

INTERNAL/INTERNAL

EXTERNAL/EXTERNAL

Explicitly defining the Load Balancer configuration options seems more useful.

sadasu · 2024-05-20T16:24:38Z

api/v1beta1/types.go

+	Name *string `json:"name,omitempty"`
+
+	// Subnet is the name of the subnet to use for a regional Load Balancer.
+	Subnet *string `json:"subnet,omitempty"`


Can the Internal Load Balancer be created without a subnet? Afaik, it can't. So, omitempty is not a valid value.

Yes, it must be created with a subnet. Currently it uses the first subnet if one is not defined here, so setting it in the API is optional.
https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1222/files#diff-a7274316ed5d60f25cc61e1bcb771a612c3566832a59ecc3a13b279a849c95ebR728-R730

I can change this to require it to be explicitly defined if that seems like the best direction.

Yes, it must be created with a subnet. Currently it uses the first subnet if one is not defined here, so setting it in the API is optional.
https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1222/files#diff-a7274316ed5d60f25cc61e1bcb771a612c3566832a59ecc3a13b279a849c95ebR728-R730

I'd explicitly write this in the comment for this field.

cloud/services/compute/loadbalancers/reconcile.go

bfournie · 2024-05-20T18:38:51Z

Thanks for working on this @bfournie. We would really like this as well!

I pulled this down and when running it with INTERNAL_ONLY it seems to get stuck on deletion. It was only deleting global resources like s.forwardingrules.Delete but the existing resources were created as regionalforwardingrules.

When I added stuff like
if err := s.regionalforwardingrules.Delete(ctx, key); err != nil && !gcperrors.IsNotFound(err) {
	log.Error(err, "Error updating a regional forwardingrule", "name", spec.Name)
	return err
}
to loadbalancers/reconcile.go functions

deleteForwardingRule()

deleteBackendService()

deleteHealthCheck()

I was then able to reliably get cluster deletion to work without manual intervention or getting stuck.

I'm also noticing the Internal Passthrough LB is getting created with 6443 as the kube api endpoint, but the kubeconfig generated is expecting 443 and never finishes setting up the cluster. I think I understand that passthrough LBs don't allow port mapping so would the solution be to either?--

change the kubeconfig port from 443 to 6443

change the kube api service and LB from 6443 to 443

I haven't found the code to change the generated kubeconfig port yet, but I think ClusterNetwork.APIServerPort is where I can change the kube api port. Not sure what the plans are here, but I'll continue to play round with it.

Thank you for testing this! I wasn't seeing an error on deletion but agree that the regional interfaces were not being explicitly called, so I've changed this to do explicit deletions on the regional resources.

Regarding the port being used, the ControlPlaneEndpoint should be set to 443 if not overridden by APIServerPort https://github.com/kubernetes-sigs/cluster-api-provider-gcp/blob/main/cloud/scope/cluster.go#L148-L154 and this should be used in the Internal only case here https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1222/files#diff-a7274316ed5d60f25cc61e1bcb771a612c3566832a59ecc3a13b279a849c95ebR235-R240 but perhaps its not. I will investigate this.

api/v1beta1/types.go

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.

k8s-ci-robot · 2024-05-23T14:14:02Z

@bfournie: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-gcp-apidiff	`eb4a0ad`	link	false	`/test pull-cluster-api-provider-gcp-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

damdo

Thanks @bfournie, great effort with this feature!
I left my review, most are nitpicks and some questions.

Also were you planning on writing E2E tests for this?
I think this is a big enough feature that could warrant that.

damdo · 2024-05-24T08:09:58Z

api/v1beta1/types.go

+	// ExternalLoadBalancerOnly creates a Global External Proxy Load Balancer
+	// to manage traffic to backends in multiple regions. This is the default Load
+	// Balancer and will be created if no LoadBalancerType is defined.
+	ExternalLoadBalancerOnly = LoadBalancerType("EXTERNAL_ONLY")
+
+	// InternalLoadBalancerOnly creates a Regional Internal Passthrough Load
+	// Balancer to manage traffic to backends in the configured region.
+	InternalLoadBalancerOnly = LoadBalancerType("INTERNAL_ONLY")
+
+	// DualLoadBalancer creates both External and Internal Load Balancers to provide
+	// separate endpoints for managing both external and internal traffic.
+	DualLoadBalancer = LoadBalancerType("Dual")


I'd try and stick to values that are following the API Conventions:

values should be PascalCase and should be equivalent to the camelCase field name (json tag)

So I'd drop the _ONLY (IMO it is already clear without it) and change them to:

External, Internal, InternalExternal (Dual could be ambiguous if we add a third option or different combination in the future),

damdo · 2024-05-24T09:27:47Z

api/v1beta1/types.go

+	// will be used. For an Internal Load Balancer service the default
+	// name is "api-internal".
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:MaxLength=32


Is this a limitation on the AWS side?

damdo · 2024-05-24T09:29:27Z

api/v1beta1/types.go

+	Name *string `json:"name,omitempty"`
+
+	// Subnet is the name of the subnet to use for a regional Load Balancer.
+	Subnet *string `json:"subnet,omitempty"`


Yes, it must be created with a subnet. Currently it uses the first subnet if one is not defined here, so setting it in the API is optional.
https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1222/files#diff-a7274316ed5d60f25cc61e1bcb771a612c3566832a59ecc3a13b279a849c95ebR728-R730

I'd explicitly write this in the comment for this field.

damdo · 2024-05-24T09:33:30Z

api/v1beta1/types.go

+
+	// APIInternalAddress is the IPV4 regional address assigned to the
+	// internal Load Balancer.
+	// +optional
+	APIInternalAddress *string `json:"apiInternalIpAddress,omitempty"`
+
+	// APIInternalHealthCheck is the full reference to the health check
+	// created for the internal Load Balancer.
+	// +optional
+	APIInternalHealthCheck *string `json:"apiInternalHealthCheck,omitempty"`
+
+	// APIInternalBackendService is the full reference to the backend service
+	// created for the internal Load Balancer.
+	// +optional
+	APIInternalBackendService *string `json:"apiInternalBackendService,omitempty"`
+
+	// APIInternalForwardingRule is the full reference to the forwarding rule
+	// created for the internal Load Balancer.
+	// +optional
+	APIInternalForwardingRule *string `json:"apiInternalForwardingRule,omitempty"`


Are these fields all required when the Internal Load Balancer is set?
If not are they defaulted to something in code?

damdo · 2024-05-24T09:34:36Z

cloud/services/compute/loadbalancers/reconcile.go

+// loadBalancingMode describes the load balancing mode that the backend performs.
+type loadBalancingMode string
+
+const (
+	// Utilization determines how the traffic load is spread based on the
+	// utilization of instances.
+	loadBalancingModeUtilization = loadBalancingMode("UTILIZATION")
+
+	// Connection determines how the traffic load is spread based on the
+	// total number of connections that a backend can handle. This is
+	// only mode available for passthrough Load Balancers.
+	loadBalancingModeConnection = loadBalancingMode("CONNECTION")
+
+	loadBalanceTrafficInternal = "INTERNAL"
+)


Also here let's adhere to the PascalCase conventions

damdo · 2024-05-24T09:41:39Z

cloud/services/compute/loadbalancers/reconcile.go

+	if err := s.deleteForwardingRule(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIServerForwardingRule = nil
+
+	if err := s.deleteAddress(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIServerAddress = nil
+
+	if err := s.deleteTargetTCPProxy(ctx); err != nil {
+		return err
+	}
+	s.scope.Network().APIServerTargetProxy = nil
+
+	if err := s.deleteBackendService(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIServerBackendService = nil
+
+	if err := s.deleteHealthCheck(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIServerHealthCheck = nil


I'd add an fmt.Errorf( wrap for all these errors for easier debugging when following the log error chain

damdo · 2024-05-24T09:41:57Z

cloud/services/compute/loadbalancers/reconcile.go

+		return err
+	}
+	s.scope.Network().APIInternalForwardingRule = nil
+
+	if err := s.deleteInternalAddress(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIInternalAddress = nil
+
+	if err := s.deleteRegionalBackendService(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIInternalBackendService = nil
+
+	if err := s.deleteRegionalHealthCheck(ctx, name); err != nil {
+		return err
+	}
+	s.scope.Network().APIInternalHealthCheck = nil


Error wrapping here too

damdo · 2024-05-24T09:44:15Z

cloud/services/compute/loadbalancers/reconcile.go

-		})
+		}
+		if mode == loadBalancingModeConnection {
+			be.MaxConnections = int64(2 ^ 32)


Can we add a comment specifying where this value is coming from?

damdo · 2024-05-24T09:46:15Z

cloud/services/compute/loadbalancers/reconcile.go

+	spec.Region = s.scope.Region()
+	spec.BackendService = backendSvc.SelfLink
+	// Ports are used instead or PortRange for passthrough Load Balancer
+	spec.Ports = []string{"6443", "22623"}


Where are these values coming from?

damdo · 2024-05-24T09:48:43Z

cloud/services/compute/loadbalancers/reconcile.go

+			return nil, err
+		}
+		// Return subnet that matches configuration or first one if not configured
+		if cfgSubnet == "" || strings.HasSuffix(subnet.Name, cfgSubnet) {


What is the reason for using HasSuffix instead of a full match?

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 a review from cpanato May 1, 2024 15:42

k8s-ci-robot requested a review from 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 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024

bfournie commented May 20, 2024

View reviewed changes

sadasu reviewed May 20, 2024

View reviewed changes

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

sadasu reviewed May 20, 2024

View reviewed changes

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

bfournie force-pushed the internal-load-balancer branch from f5670bf to b80ab99 Compare May 20, 2024 18:16

bfournie force-pushed the internal-load-balancer branch from b80ab99 to a7ea938 Compare May 20, 2024 18:49

sadasu reviewed May 23, 2024

View reviewed changes

api/v1beta1/types.go Show resolved Hide resolved

bfournie force-pushed the internal-load-balancer branch from a7ea938 to eb4a0ad Compare May 23, 2024 14:10

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2024

damdo reviewed May 24, 2024

View reviewed changes

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

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

cpanato commented May 20, 2024

k8s-ci-robot commented May 20, 2024

cpanato commented May 20, 2024

jwmay2012 commented May 20, 2024 •

edited

bfournie May 20, 2024 •

edited

sadasu May 20, 2024

bfournie May 20, 2024

damdo May 24, 2024

bfournie commented May 20, 2024

k8s-ci-robot commented May 23, 2024

damdo left a comment

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

damdo May 24, 2024

Create an Internal Load Balancer if configured #1222

Are you sure you want to change the base?

Create an Internal Load Balancer if configured #1222

Conversation

bfournie commented May 1, 2024 • edited

k8s-ci-robot commented May 1, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damdo commented May 9, 2024

bfournie commented May 14, 2024

bfournie commented May 16, 2024

cpanato commented May 20, 2024

k8s-ci-robot commented May 20, 2024

cpanato commented May 20, 2024

jwmay2012 commented May 20, 2024 • edited

bfournie May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfournie commented May 20, 2024

k8s-ci-robot commented May 23, 2024

damdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfournie commented May 1, 2024 •

edited

netlify bot commented May 1, 2024 •

edited

jwmay2012 commented May 20, 2024 •

edited

bfournie May 20, 2024 •

edited