Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/v1beta2/ibmpowervscluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// 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.
// 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.
// +optional
GlobalRouting *bool `json:"globalRouting,omitempty"`
}

// VPCResourceReference is a reference to a specific VPC resource by ID or Name
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta2/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 15 additions & 25 deletions cloud/scope/powervs_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -1334,16 +1334,26 @@ func (s *PowerVSClusterScope) createTransitGateway() (*string, error) {
return nil, fmt.Errorf("error getting resource group id for resource group %v, id is empty", s.ResourceGroup())
}

vpcRegion := s.getVPCRegion()
if vpcRegion == nil {
return nil, fmt.Errorf("failed to get vpc region")
location, globalRouting, err := genUtil.GetTransitGatewayLocationAndRouting(s.Zone(), s.VPC().Region)
if err != nil {
return nil, fmt.Errorf("failed to get transit gateway location and routing: %w", err)
}

// throw error when user tries to use local routing where global routing is required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, Thanks

// 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 are in different region and 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 = ptr.To(true)
}

tgName := s.GetServiceName(infrav1beta2.ResourceTypeTransitGateway)
tg, _, err := s.TransitGatewayClient.CreateTransitGateway(&tgapiv1.CreateTransitGatewayOptions{
Location: vpcRegion,
Location: location,
Name: tgName,
Global: ptr.To(true),
Global: globalRouting,
ResourceGroup: &tgapiv1.ResourceGroupIdentity{ID: ptr.To(resourceGroupID)},
})
if err != nil {
Expand Down Expand Up @@ -1744,26 +1754,6 @@ func (s *PowerVSClusterScope) fetchResourceGroupID() (string, error) {
return "", err
}

// getVPCRegion returns region associated with VPC zone.
func (s *PowerVSClusterScope) getVPCRegion() *string {
if s.IBMPowerVSCluster.Spec.VPC != nil {
return s.IBMPowerVSCluster.Spec.VPC.Region
}
// if vpc region is not set try to fetch corresponding region from power vs zone
zone := s.Zone()
if zone == nil {
s.Info("powervs zone is not set")
return nil
}
region := endpoints.ConstructRegionFromZone(*zone)
vpcRegion, err := genUtil.VPCRegionForPowerVSRegion(region)
if err != nil {
s.Error(err, fmt.Sprintf("failed to fetch vpc region associated with powervs region %s", region))
return nil
}
return &vpcRegion
}

// fetchVPCCRN returns VPC CRN.
func (s *PowerVSClusterScope) fetchVPCCRN() (*string, error) {
vpcID := s.GetVPCID()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,12 @@ spec:
when TransitGateway.ID is set, its expected that there exist a TransitGateway with ID or else system will give error.
when TransitGateway.Name is set, system will first check for TransitGateway with Name, if not exist system will create new TransitGateway.
properties:
globalRouting:
description: |-
globalRouting indicates whether to set global routing true or not while creating the transit gateway.
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.
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.
type: boolean
id:
description: id of resource.
type: string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,12 @@ spec:
when TransitGateway.ID is set, its expected that there exist a TransitGateway with ID or else system will give error.
when TransitGateway.Name is set, system will first check for TransitGateway with Name, if not exist system will create new TransitGateway.
properties:
globalRouting:
description: |-
globalRouting indicates whether to set global routing true or not while creating the transit gateway.
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.
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.
type: boolean
id:
description: id of resource.
type: string
Expand Down
33 changes: 33 additions & 0 deletions util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ import (
"fmt"
"strconv"

"k8s.io/utils/ptr"

"sigs.k8s.io/controller-runtime/pkg/client"

infrav1beta2 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2"
"sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/endpoints"
)

// GetClusterByName finds and return a Cluster object using the specified params.
Expand Down Expand Up @@ -197,3 +200,33 @@ func VPCZonesForPowerVSRegion(region string) ([]string, error) {
}
return nil, fmt.Errorf("VPC zones corresponding to a PowerVS region %s not found ", region)
}

// IsGlobalRoutingRequiredForTG returns true when powervs and vpc regions are different.
func IsGlobalRoutingRequiredForTG(powerVSRegion string, vpcRegion string) bool {
if r, ok := Regions[powerVSRegion]; ok && r.VPCRegion == vpcRegion {
return false
}
return true
}

// 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 same otherwise false.
func GetTransitGatewayLocationAndRouting(powerVSZone *string, vpcRegion *string) (*string, *bool, error) {
if powerVSZone == nil {
return nil, nil, fmt.Errorf("powervs zone is not set")
}
powerVSRegion := endpoints.ConstructRegionFromZone(*powerVSZone)

if vpcRegion != nil {
routing := IsGlobalRoutingRequiredForTG(powerVSRegion, *vpcRegion)
return vpcRegion, &routing, nil
}
location, err := VPCRegionForPowerVSRegion(powerVSRegion)
if err != nil {
return nil, nil, fmt.Errorf("failed to fetch vpc region associated with powervs region '%s': %w", powerVSRegion, err)
}

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