Vpc reconcile lbs #2023

cjschaef · 2024-10-22T20:45:07Z

What this PR does / why we need it: Add support to reconcile extended VPC Load Balancers

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add reconciliation support for extended VPC Load Balancer definitions

k8s-ci-robot · 2024-10-22T20:45:17Z

Hi @cjschaef. 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.

netlify · 2024-10-22T20:45:25Z

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

Name	Link
🔨 Latest commit	`48eebb2`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/672d13499f850f00084a8577
😎 Deploy Preview	https://deploy-preview-2023--kubernetes-sigs-cluster-api-ibmcloud.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.

Karthik-K-N · 2024-10-23T05:55:53Z

/ok-to-test

Karthik-K-N · 2024-10-23T05:55:59Z

/cc @dharaneeshvrd

Karthik-K-N · 2024-10-23T12:03:35Z

controllers/ibmvpccluster_controller.go


 	// Mark cluster as ready.
 	clusterScope.IBMVPCCluster.Status.Ready = true
+	clusterScope.Info("cluster infrastructure is now ready for cluster", "clusterName", clusterScope.IBMVPCCluster.Name)


Before setting status.Ready to true, we need to update the spec.Controlplane with loadbalancer hostname, This is as per the CAPI contract.
For reference this is what we do currently for PowerVS.

Okay, I will add the necessary logic to retrieve those values to set.

Karthik-K-N · 2024-10-23T12:07:35Z

cloud/scope/vpc_cluster.go

+
+// ReconcileLoadBalancers reconciles Load Balancers.
+func (s *VPCClusterScope) ReconcileLoadBalancers() (bool, error) {
+	// TODO(cjschaef): Determine if we want to use default LB configuration or require at least one is defined in Cluster spec.


Like other we can make a note of it to add it to webhook for good UX.

Sure, I'll add another comment.

Karthik-K-N · 2024-10-23T12:11:49Z

cloud/scope/vpc_cluster.go

+// If state is active, true is returned (no requeue required).
+// In all other cases, it returns false, indicating a requeue for reconciliation.


In general in function comments, we just add what the function does and avoid what caller will do, Since it may be called at different places, So I think we can avoid mentioning(no reque required) here.

Okay, I'll only mention return values.

Karthik-K-N · 2024-10-23T12:13:24Z

cloud/scope/vpc_cluster.go

+		if lbStatus != nil {
+			s.SetLoadBalancerStatus(lbStatus)
+			// If the Load Balancer status isn't ready, flag for requeue and continue to next Load Balancer.
+			if isReady := s.isLoadBalancerReady(string(lbStatus.State)); !isReady {


why its converted to string, Why can't we use VPCLoadBalancerState instead?

I can move things around.

Karthik-K-N · 2024-10-23T12:19:06Z

cloud/scope/vpc_cluster.go

+			return nil, nil
+		}
+	} else {
+		loadBalancer, err = s.VPCClient.GetLoadBalancerByName(lb.Name)


Even both LB id and name can be nil right, Should we add a check and log proper message?

No, name cannot be nil for a LB based on the spec.

So, my assumption based on that, was if ID is not set, we expect the Name to be set.
If there is a use case to have ID == nil && Name == "", I can another check to try to use a default LB name instead.

Karthik-K-N · 2024-10-23T12:20:30Z

cloud/scope/vpc_cluster.go

+	if loadBalancer.BackendPools != nil {
+		for _, pool := range loadBalancer.BackendPools {
+			backendPool := s.buildLoadBalancerBackendPool(pool)
+


empty line can be removed.

Karthik-K-N · 2024-10-23T12:32:36Z

cloud/scope/vpc_cluster.go

+	}
+	options.SetListeners(listeners)
+
+	// Create the load balancer.


May be if required we can log the options with higher V level for easy debugging.

I can add a log call with v(5) with the options object prior to creation....unless you would rather have each field logged out separately in the log call?

I think logging options should be sufficient.

dharaneeshvrd · 2024-10-23T05:53:41Z

cloud/scope/vpc_cluster.go

+	if s.NetworkStatus() != nil {
+		if s.NetworkStatus().ControlPlaneSubnets != nil {


Suggested change

if s.NetworkStatus() != nil {

if s.NetworkStatus().ControlPlaneSubnets != nil {

if s.NetworkStatus() != nil && if s.NetworkStatus().ControlPlaneSubnets != nil {

Will consolidate.

dharaneeshvrd · 2024-10-23T06:37:12Z

cloud/scope/vpc_cluster.go

+// SetLoadBalancerStatus sets the status for a Load Balancer.
+func (s *VPCClusterScope) SetLoadBalancerStatus(loadBalancer *infrav1beta2.VPCLoadBalancerStatus) {
+	// Ignore attempts to set status without a load balancer.
+	if loadBalancer == nil {


Not needed IMO, since it's an internal func and you are populating loadBalancer status or doing a nil check before calling the func on both the places.

It is a public function, so it could be called wherever, I can make it private and remove the check. I'd like to avoid go panics whenever possible.

dharaneeshvrd · 2024-10-23T06:46:04Z

cloud/scope/vpc_cluster.go

+	if loadBalancer.Subnets != nil {
+		for _, subnet := range loadBalancer.Subnets {
+			if subnet.ID != nil {
+				subnetIDs = append(subnetIDs, *subnet.ID)


Don't you need to verify that subnet with this id exists? Since it's coming from user spec.

Sure, I could perform lookups to check if they exist.

dharaneeshvrd · 2024-10-23T06:46:57Z

cloud/scope/vpc_cluster.go

+	if loadBalancer.SecurityGroups != nil {
+		for _, securityGroup := range loadBalancer.SecurityGroups {
+			if securityGroup.ID != nil {
+				securityGroupIDs = append(securityGroupIDs, *securityGroup.ID)


Same as previous comment on verifying user provided SG.

Same, I could check if they exist.

dharaneeshvrd · 2024-10-24T04:26:58Z

cloud/scope/vpc_cluster.go

+}
+
+// getDefaultBalancerBackendPools returns a list of default Load Balancer Backend Pools for a Load Balancer.
+func (s *VPCClusterScope) getDefaultLoadBalancerBackendPools() []vpcv1.LoadBalancerPoolPrototype {


Any reason for creating a slice and appending one item and in caller again unpacking the slice?
Can't we return the item directly?

I did this if any additional pools were necessary to add to the defaults.

dharaneeshvrd · 2024-10-24T04:27:34Z

cloud/scope/vpc_cluster.go

+}
+
+// getDefaultLoadBalancerListeners returns a list of default Load Balancer Listeners for a Load Balancer.
+func (s *VPCClusterScope) getDefaultLoadBalancerListeners(defaultBackendPool bool) []vpcv1.LoadBalancerListenerPrototypeLoadBalancerContext {


Same comment as getDefaultLoadBalancerBackendPools()'s comment.

Same as the pools, regarding additional default listeners.

Karthik-K-N · 2024-11-04T05:05:47Z

cloud/scope/vpc_cluster.go

+// NOTE(cjschaef): A webhook validation check could help ensure this.
+func (s *VPCClusterScope) GetLoadBalancerHostName() (*string, error) {
+	// If no Status or Load Balancer Status is populated, assume the Load Balancer's are not ready (have not been reconciled), so no hostname will be available.
+	if s.NetworkStatus() == nil || s.NetworkStatus().LoadBalancers == nil || len(s.NetworkStatus().LoadBalancers) == 0 {


we are returning nil here for len(s.NetworkStatus().LoadBalancers) and same codition returns error next step, May be we can simplify or remove if not required.

So, the first set of checks looks at Status. Assuming if there is no Status or no LB status, the situation needs to wait for Status to exist or be updated.

The second check is against Spec. If Status was set, but Spec was not, that is an error. Checks are made immediately in the function to short circuit.

apologies, thanks for the explanation, Its correct, I just overlooked spec and status.

Karthik-K-N · 2024-11-04T05:17:16Z

cloud/scope/vpc_cluster.go

+			ID: lb.ID,
+		}
+		loadBalancer, detailedResponse, err = s.VPCClient.GetLoadBalancer(getLBOptions)
+		if detailedResponse != nil && detailedResponse.StatusCode == http.StatusNotFound {


I think we can log these cases with higher level V values for easy debugging in future.

Okay, I bumped to v5.

Karthik-K-N · 2024-11-04T05:24:35Z

cloud/scope/vpc_cluster.go

+	}
+
+	options.SetIsPublic(isPublic)
+	options.SetName(loadBalancer.Name)


What if loadbalancer.Name is empty?
In getLoadBalancer() function we have this condition

name := lb.Name if name == "" { name = *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer) }

Should we do something similar here?

I'm hesitant to create LB's with a default service name, especially if multiple LB's get defined with empty names.
Given the structure of the VPCLoadBalancerStatus, we rely on ID as a priority field. We can let VPC API generate a Name, and rely on the ID from Status.

The default service name was added by request, but I don't expect to use it for most use cases. This is likely to be refactored, likely not relying on a default name, if possible.

Karthik-K-N · 2024-11-04T05:25:41Z

cloud/scope/vpc_cluster.go

+	}
+	options.SetListeners(listeners)
+
+	// Create the load balancer.


I think logging options should be sufficient.

Karthik-K-N · 2024-11-04T05:33:31Z

cloud/scope/vpc_cluster.go

+
+	// If only one Load Balancer was provided, attempt to use that Load Balancer's hostname.
+	if len(s.NetworkSpec().LoadBalancers) == 1 {
+		return getHostName(s.NetworkSpec().LoadBalancers[0])


Any reason for handling this special case, In case of 1 lb defined it can be private as well is this the reason?

Yes, if a Public LB was not found, and there was only one LB, we expect it must be a Private LB, and Public traffic isn't expected to be enabled. This isn't the normal path, but still need to cover this design.

Karthik-K-N · 2024-11-04T05:34:14Z

cloud/scope/vpc_cluster.go

+
+		// NOTE(cjschaef): As a default Load Balancer isn't supported (when none are provided in Spec), this should result in an error.
+		// TODO(cjschaef): Likely would be beneficial to have webhook validation for this case.
+		if loadBalancer.Name == "" {


Not sure this check is needed here? in getLoadBalancer() we get the default name incase of name is empty.

I'll be refactoring, will see what makes sense.

Karthik-K-N · 2024-11-04T05:51:34Z

cloud/scope/vpc_cluster.go

+	} else {
+		name := lb.Name
+		if name == "" {
+			name = *s.GetServiceName(infrav1beta2.ResourceTypeLoadBalancer)


In case of multiple loadbalancers configured without name in spec, This may not work as expected as we end up getting same name for all.

That is why we do not create an LB with the default Name (let API generate one). But that will generate its own problems.

I will need to rethink how the default service name is being used, as I don't think it makes sense to use it as requested (perhaps only in the rare case that one LB was defined in Spec). Or perhaps I can try refactoring to append public/private and hope that doesn't cause overlap (although I think that case is just as susceptible to this same situation).
Perhaps I need to add webhook validation to limit the number of LB's defined in Spec to only be 2, in additional to the refactoring. But that can be very limiting to the design and logic as well.

In Power VS we do something like this , append the index https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/987191d98e364d4bbfc39423d433b791dbb6b102/cloud/scope/powervs_cluster.go#L1963-L1965

dharaneeshvrd · 2024-11-05T04:02:22Z

cloud/scope/vpc_cluster.go

+		}
+		loadBalancer, detailedResponse, err = s.VPCClient.GetLoadBalancer(getLBOptions)
+		if detailedResponse != nil && detailedResponse.StatusCode == http.StatusNotFound {
+			return nil, nil


Generally if user provides an id, we expect the resource to be exist with the id, if not we would throw an error and not proceed with the reconciliation. It's ok in case of name is provided and if resource not exist, we can create it.
Also If you return nil here and create a new load balancer, then the status and spec will have different ids.

Okay, I'll return an error.

Add support to reconcile VPC Load Balancers as part of the extended VPC support. Related: kubernetes-sigs#1896

Karthik-K-N · 2024-11-11T07:09:32Z

@cjschaef Is this PR updated or tested?

Karthik-K-N

/lgtm
/assign @dharaneeshvrd

dharaneeshvrd · 2024-11-12T12:39:55Z

/lgtm

Karthik-K-N · 2024-11-12T12:47:02Z

/assign @mkumatag @Prajyot-Parab

k8s-ci-robot · 2024-11-12T14:39:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjschaef, mkumatag

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~~ [mkumatag]

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

VPC: Add VPC LB Pool resource type to API

40935ed

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Oct 22, 2024

k8s-ci-robot requested review from Karthik-K-N and Prajyot-Parab October 22, 2024 20:45

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 22, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2024

cjschaef mentioned this pull request Oct 22, 2024

Extend VPC (x86_64) Infrastructure reconciliation support #1896

Closed

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 Oct 23, 2024

k8s-ci-robot requested a review from dharaneeshvrd October 23, 2024 05:56

Karthik-K-N reviewed Oct 23, 2024

View reviewed changes

dharaneeshvrd reviewed Oct 24, 2024

View reviewed changes

mkumatag added this to the v0.9.0 milestone Oct 29, 2024

cjschaef force-pushed the vpc_reconcile_lbs branch from b24c899 to ced6212 Compare October 29, 2024 15:01

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024

Karthik-K-N reviewed Nov 4, 2024

View reviewed changes

dharaneeshvrd reviewed Nov 5, 2024

View reviewed changes

cjschaef force-pushed the vpc_reconcile_lbs branch from ced6212 to 267c18f Compare November 5, 2024 21:12

VPC: Add VPC LB Reconciliation

48eebb2

Add support to reconcile VPC Load Balancers as part of the extended VPC support. Related: kubernetes-sigs#1896

cjschaef force-pushed the vpc_reconcile_lbs branch from 267c18f to 48eebb2 Compare November 7, 2024 19:21

cjschaef mentioned this pull request Nov 10, 2024

CORS-3748: IBMCloud: Setup IBM Cloud CAPI InfraReady step openshift/installer#9191

Merged

Karthik-K-N reviewed Nov 12, 2024

View reviewed changes

k8s-ci-robot assigned dharaneeshvrd Nov 12, 2024

k8s-ci-robot assigned Karthik-K-N Nov 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2024

k8s-ci-robot assigned mkumatag and Prajyot-Parab Nov 12, 2024

mkumatag approved these changes Nov 12, 2024

View reviewed changes

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

k8s-ci-robot merged commit a71c64a into kubernetes-sigs:main Nov 12, 2024
13 checks passed

cjschaef deleted the vpc_reconcile_lbs branch November 12, 2024 16:00

		// If state is active, true is returned (no requeue required).
		// In all other cases, it returns false, indicating a requeue for reconciliation.

		if s.NetworkStatus() != nil {
		if s.NetworkStatus().ControlPlaneSubnets != nil {

	if s.NetworkStatus() != nil {
	if s.NetworkStatus().ControlPlaneSubnets != nil {
	if s.NetworkStatus() != nil && if s.NetworkStatus().ControlPlaneSubnets != nil {

Vpc reconcile lbs #2023

Vpc reconcile lbs #2023

Conversation

cjschaef commented Oct 22, 2024

k8s-ci-robot commented Oct 22, 2024

netlify bot commented Oct 22, 2024 • edited Loading

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

Karthik-K-N commented Oct 23, 2024

Karthik-K-N commented Oct 23, 2024

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

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

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

Karthik-K-N Nov 5, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N commented Nov 11, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

dharaneeshvrd commented Nov 12, 2024

Karthik-K-N commented Nov 12, 2024

k8s-ci-robot commented Nov 12, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

Karthik-K-N Nov 5, 2024 •

edited

Loading

dharaneeshvrd Nov 5, 2024 •

edited

Loading