kubernetes-sigs / cluster-api-provider-ibmcloud

Cluster API Provider for IBM Cloud
https://cluster-api-ibmcloud.sigs.k8s.io
Apache License 2.0
62 stars 77 forks source link

Plan to handle exsiting TransitGateway and its connections #1887

Open Karthik-K-N opened 1 month ago

Karthik-K-N commented 1 month ago

/kind feature /area provider/ibmcloud

Describe the solution you'd like [A clear and concise description of what you want to happen.]

Currently when TransitGateway details are not set the controller will automatically creates a TransitGateway and necessary connections.

Also we support user to pass existing TransitGateway and which already has attached connections to it.

We dont support case where a TransitGatewa exist and which does not have any connections and user is expecting controller to attach the connections.

Anything else you would like to add: [Miscellaneous information that will assist in solving the issue.]

Goal of this issue is to decide on

  1. should we support this if not what are the alternatives
  2. Consequence of supporting it.
hamzy commented 1 month ago

I expect the use cases to be the following: Neither VPC nor TG are pre-existing. TG is pre-existing, VPC is not. VPC is pre-existing, TG is not. Both VPC and TG are pre-existing.

1 is already handled.

2 is new for IPI. I expect the provider to create both connections.

3 is not new.

4 is new for IPI. For consistency, I expect the provider to create both connections.

So for every case, I expect the provider to create both connections.

dharaneeshvrd commented 1 month ago

Need changes in API status to store the status of the vpc and powervs connections separately to use them to take decision while deleting them.

+++ b/api/v1beta2/ibmpowervscluster_types.go
@@ -181,6 +181,19 @@ type ResourceReference struct {
        ControllerCreated *bool `json:"controllerCreated,omitempty"`
 }

+// TransitGatewayStatus defines the status of transit gateway as well as it's connection's status.
+type TransitGatewayStatus struct {
+       // id represents the id of the resource.
+       ID *string `json:"id,omitempty"`
+       // +kubebuilder:default=false
+       // controllerCreated indicates whether the resource is created by the controller.
+       ControllerCreated *bool `json:"controllerCreated,omitempty"`
+       // vpcConnection defines the vpc connection status in transit gateway.
+       VPCConnection *ResourceReference `json:"vpcConnection,omitempty"`
+       // powerVSConnection defines the powervs connection status in transit gateway.
+       PowerVSConnection *ResourceReference `json:"powerVSConnection,omitempty"`
+}
+
 // IBMPowerVSClusterStatus defines the observed state of IBMPowerVSCluster.
 type IBMPowerVSClusterStatus struct {
        // ready is true when the provider resource is ready.
@@ -209,7 +222,7 @@ type IBMPowerVSClusterStatus struct {
        VPCSecurityGroups map[string]VPCSecurityGroupStatus `json:"vpcSecurityGroups,omitempty"`

        // transitGateway is reference to IBM Cloud TransitGateway.
-       TransitGateway *ResourceReference `json:"transitGateway,omitempty"`
+       TransitGateway *TransitGatewayStatus `json:"transitGateway,omitempty"`

        // cosInstance is reference to IBM Cloud COS Instance resource.
        COSInstance *ResourceReference `json:"cosInstance,omitempty"

@mkumatag @Karthik-K-N Hope this API status change is ok and I can continue with the development. Let me know if you have different thoughts on this!

Karthik-K-N commented 1 month ago

IBMPowerVSClusterStatus

Looks good to me, I think there is no need to do any spec changes right? I hope we should not allow user to create any connection with specific name and that requirement wont come.

mkumatag commented 1 month ago

@mkumatag @Karthik-K-N Hope this API status change is ok and I can continue with the development. Let me know if you have different thoughts on this!

I see a change here in the status, we need to be little cautious here..

hamzy commented 1 month ago

I see a change here in the status, we need to be little cautious here..

Sure, but still proceed, right?

dharaneeshvrd commented 1 month ago

I hope we should not allow user to create any connection with specific name and that requirement wont come.

Yes Let's not do that, it will add more complications!