Use local routing in transit gateway when PowerVS and VPC are from same region #1777

dharaneeshvrd · 2024-05-15T09:56:44Z

What this PR does / why we need it:

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 #1763

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:

Use local routing in transit gateway when PowerVS and VPC are from same region

netlify · 2024-05-15T09:57:07Z

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

Name	Link
🔨 Latest commit	`c0ec9d7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/664d90ff77527a00086578f2
😎 Deploy Preview	https://deploy-preview-1777--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.

dharaneeshvrd · 2024-05-16T04:30:35Z

/cc @mkumatag @Karthik-K-N

Karthik-K-N · 2024-05-16T06:14:09Z

api/v1beta2/ibmpowervscluster_types.go

@@ -251,6 +251,9 @@ type TransitGateway struct {
 	// id of resource.
 	// +optional
 	ID *string `json:"id,omitempty"`
+	// globalRouting indicates whether to set global routing true or not while creating the transit gateway.


Lets also mention a liner about global routing and local routing and when to use which.
Also what system will do when the field is omitted like as we mentioned in other api types. Lets keep as descriptive as possible.

Karthik-K-N · 2024-05-16T06:26:07Z

cloud/scope/powervs_cluster.go

+		return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc region used requires global routing")
+	}
+	// setting global routing to true when it is set by user.
+	if s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && *s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting && !*globalRouting {


I think here we need not check whats globalRouting, simply use Spec.TransitGateway.GlobalRouting

Karthik-K-N · 2024-05-16T06:43:31Z

cloud/scope/powervs_cluster.go

 	}
 	region := endpoints.ConstructRegionFromZone(*zone)
-	vpcRegion, err := genUtil.VPCRegionForPowerVSRegion(region)
+
+	if s.IBMPowerVSCluster.Spec.VPC != nil {


We can use s.VPC()

Karthik-K-N · 2024-05-16T06:44:21Z

cloud/scope/powervs_cluster.go

+// getTransitGatewayLocationAndRouting returns appropriate location and routing suitable for transit gateway.
+// routing indicates whether to enable global routing on transit gateway or not.
+// returns true when powervs and vpc region are not common otherwise false.
+func (s *PowerVSClusterScope) getTransitGatewayLocationAndRouting() (*string, *bool) {


I dont see any advantage of using pointer to bool , I think we can simply use bool instead.

Anyway I need to make it a pointer in called func, so just made it here and used as it is.

that makes sense.

Karthik-K-N · 2024-05-16T06:47:00Z

cloud/scope/powervs_cluster.go

-	vpcRegion, err := genUtil.VPCRegionForPowerVSRegion(region)
+
+	if s.IBMPowerVSCluster.Spec.VPC != nil {
+		routing := !genUtil.IsCommonPowerVSAndVPCRegion(region, *s.IBMPowerVSCluster.Spec.VPC.Region)


Isn't it confusing, can we simplify, Instead of negating the return value, Can we make this function to return localRouting instead of globalRouting?

Refactored the func a bit, I would like to keep the var as globalRouting since we are going to assign it to CreateTransitGatewayOptions.Global field.

that makes sense.

Karthik-K-N · 2024-05-16T09:54:35Z

api/v1beta2/ibmpowervscluster_types.go

@@ -251,6 +251,11 @@ type TransitGateway struct {
 	// id of resource.
 	// +optional
 	ID *string `json:"id,omitempty"`
+	// globalRouting indicates whether to set global routing true or not while creating the transit gateway.
+	// set this flag to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the flag to false.


Suggested change

// set this flag to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the flag to false.

// set this field to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the field to false.

Karthik-K-N · 2024-05-16T11:58:11Z

api/v1beta2/ibmpowervscluster_types.go

@@ -251,6 +251,11 @@ type TransitGateway struct {
 	// id of resource.
 	// +optional
 	ID *string `json:"id,omitempty"`
+	// globalRouting indicates whether to set global routing true or not while creating the transit gateway.
+	// set this flag to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the flag to false.
+	// if this flag is not set, based on where PowerVS and VPC regions are from would decide whether to enable globalRouting or not.


Suggested change

// if this flag is not set, based on where PowerVS and VPC regions are from would decide whether to enable globalRouting or not.

// when the field is omitted, based on PowerVS region (region associated with IBMPowerVSCluster.Spec.Zone) and VPC region(IBMPowerVSCluster.Spec.VPC.Region) system will decide whether to enable globalRouting or not.

Karthik-K-N

One suggestion otherwise LGTM
Thank you.

Karthik-K-N · 2024-05-17T02:56:31Z

cloud/scope/powervs_cluster.go

+		return nil, fmt.Errorf("failed to get transit gateway location")
+	}
+
+	// throw error when user tries to use local routing where global routing is required.


We can have this check in webhook as well, If required we can add a TODO.

Yes, we need to have this in another PR, currently util is importing v1beta2 pkg, which is causing circular import issue when accessing the region map from util in Webhook, added a todo handle this later.
Moved the GetTransitGatewayLocationAndRouting() to util to consume from Webhook validation later.

nice, Thanks

Karthik-K-N · 2024-05-17T02:59:38Z

cloud/scope/powervs_cluster.go

+	if s.VPC() != nil {
+		routing := genUtil.IsGlobalRoutingRequiredForTG(region, *s.IBMPowerVSCluster.Spec.VPC.Region)
+		return s.IBMPowerVSCluster.Spec.VPC.Region, &routing
+	}


Suggested change

if s.VPC() != nil {

routing := genUtil.IsGlobalRoutingRequiredForTG(region, *s.IBMPowerVSCluster.Spec.VPC.Region)

return s.IBMPowerVSCluster.Spec.VPC.Region, &routing

}

if vpc := s.VPC(); vpc != nil {

routing := genUtil.IsGlobalRoutingRequiredForTG(region, *vpc.Region)

return vpc.Region, &routing

}

Karthik-K-N · 2024-05-20T04:20:18Z

cloud/scope/powervs_cluster.go

+		return nil, fmt.Errorf("failed to get transit gateway location")
+	}
+
+	// throw error when user tries to use local routing where global routing is required.


nice, Thanks

Karthik-K-N · 2024-05-20T04:38:16Z

util/util.go

+	}
+
+	// returning routing as true since VPC region is not set and used PowerVS region to calculate the transit gateway location.
+	return &location, ptr.To(true), nil


Just curious to know for setting true, This case may not occur as we have a webhook check for making sure the vpc region is set, but incase it occurs any how we are getting VPC region associated with PowerVS so can't we use local routing?

thanks for pointing it out, its supposed to be local routing!😁

mkumatag

minor comment, otherwise LGTM

mkumatag · 2024-05-22T04:10:28Z

cloud/scope/powervs_cluster.go

+	// throw error when user tries to use local routing where global routing is required.
+	// TODO: Add a webhook validation for below condition.
+	if s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && !*s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting && *globalRouting {
+		return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc region used requires global routing")


Suggested change

return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc region used requires global routing")

return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")

Fixed it. PTAL now!

mkumatag · 2024-05-22T06:11:29Z

cloud/scope/powervs_cluster.go

+	// throw error when user tries to use local routing where global routing is required.
+	// TODO: Add a webhook validation for below condition.
+	if s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting != nil && !*s.IBMPowerVSCluster.Spec.TransitGateway.GlobalRouting && *globalRouting {
+		return nil, fmt.Errorf("ailed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")


there is a typo!

Suggested change

return nil, fmt.Errorf("ailed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")

return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")

…me region

mkumatag

/lgtm

k8s-ci-robot · 2024-05-22T06:37:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharaneeshvrd, 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

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

k8s-ci-robot requested review from Amulyam24 and Prajyot-Parab May 15, 2024 09:56

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 15, 2024

dharaneeshvrd force-pushed the tg-local-route branch from c04c94f to a176099 Compare May 15, 2024 10:24

k8s-ci-robot requested review from Karthik-K-N and mkumatag May 16, 2024 04:30

Karthik-K-N reviewed May 16, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-local-route branch from a176099 to fd798e1 Compare May 16, 2024 09:42

Karthik-K-N reviewed May 16, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-local-route branch from fd798e1 to ef2033a Compare May 16, 2024 12:46

Karthik-K-N reviewed May 17, 2024

View reviewed changes

mkumatag added this to the v0.8 milestone May 17, 2024

dharaneeshvrd force-pushed the tg-local-route branch 4 times, most recently from a2e596f to abc6f94 Compare May 20, 2024 04:00

Karthik-K-N reviewed May 20, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-local-route branch from abc6f94 to b561b94 Compare May 20, 2024 04:52

mkumatag requested changes May 22, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-local-route branch from b561b94 to 99c0c62 Compare May 22, 2024 04:23

mkumatag requested changes May 22, 2024

View reviewed changes

Use local routing in transit gateway when PowerVS and VPC are from sa…

c0ec9d7

…me region

dharaneeshvrd force-pushed the tg-local-route branch from 99c0c62 to c0ec9d7 Compare May 22, 2024 06:30

mkumatag approved these changes May 22, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag May 22, 2024

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

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

k8s-ci-robot merged commit 4c5b9da into kubernetes-sigs:main May 22, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use local routing in transit gateway when PowerVS and VPC are from same region #1777

Use local routing in transit gateway when PowerVS and VPC are from same region #1777

dharaneeshvrd commented May 15, 2024 •

edited

netlify bot commented May 15, 2024 •

edited

dharaneeshvrd commented May 16, 2024

Karthik-K-N May 16, 2024

dharaneeshvrd May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N May 16, 2024

dharaneeshvrd May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N May 16, 2024

dharaneeshvrd May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N May 16, 2024

Karthik-K-N left a comment

Karthik-K-N May 17, 2024

dharaneeshvrd May 20, 2024

Karthik-K-N May 20, 2024

Karthik-K-N May 17, 2024

Karthik-K-N May 20, 2024

Karthik-K-N May 20, 2024

dharaneeshvrd May 20, 2024

mkumatag left a comment

mkumatag May 22, 2024

dharaneeshvrd May 22, 2024

mkumatag May 22, 2024

dharaneeshvrd May 22, 2024

mkumatag left a comment

k8s-ci-robot commented May 22, 2024

	// set this flag to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the flag to false.
	// set this field to true only when PowerVS and VPC are from different regions, if they are same it's suggested to use local routing by setting the field to false.

	// if this flag is not set, based on where PowerVS and VPC regions are from would decide whether to enable globalRouting or not.
	// when the field is omitted, based on PowerVS region (region associated with IBMPowerVSCluster.Spec.Zone) and VPC region(IBMPowerVSCluster.Spec.VPC.Region) system will decide whether to enable globalRouting or not.

	return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc region used requires global routing")
	return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")

	return nil, fmt.Errorf("ailed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")
	return nil, fmt.Errorf("failed to use local routing for transit gateway since powervs and vpc are in different region and requires global routing")

Use local routing in transit gateway when PowerVS and VPC are from same region #1777

Use local routing in transit gateway when PowerVS and VPC are from same region #1777

Conversation

dharaneeshvrd commented May 15, 2024 • edited

netlify bot commented May 15, 2024 • edited

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

dharaneeshvrd commented May 16, 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

Karthik-K-N 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

mkumatag 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

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 22, 2024

dharaneeshvrd commented May 15, 2024 •

edited

netlify bot commented May 15, 2024 •

edited