VPC: Add infrastructure creation proposal #1657

cjschaef · 2024-03-12T15:52:02Z

Add a new proposal with details and images for creating and deleting VPC resources for the IBMVPCCluster definition.

What this PR does / why we need it:
Adds a new proposal to extend VPC Cluster support to create necessary resources based on design.

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 # N/A

Special notes for your reviewer:
We expect to break up our enhancements into the following sets:

API updates - likely break down into multiple smaller PR's
Implement missing/new creation/reconcile logic for new resources
Implement deletion/cleanup of resources as necessary.

/area provider/ibmcloud

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

Release note:

k8s-ci-robot · 2024-03-12T15:52:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cjschaef
Once this PR has been reviewed and has the lgtm label, please assign jichenjc 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

k8s-ci-robot · 2024-03-12T15:52:12Z

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/test-infra repository.

netlify · 2024-03-12T15:52:18Z

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

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

mkumatag · 2024-03-13T05:49:01Z

docs/proposal/20240308-vpc-infra-creation.md

+
+	// cisInstance defines the IBM Cloud CIS instance to create DNS Records for the cluster.
+	// +optional
+	CISInstance *CISInstance `json:"cisInstance,omitempty"`


Usually DNS is handled by the openshift itself for other providers like powervs, aws etc.. any more information of the need for adding DNS will be helpful for further discussions

Sure, I will remove this then, with that expectation.

Removed references.

mkumatag · 2024-03-13T05:49:08Z

docs/proposal/20240308-vpc-infra-creation.md

+
+	// dnsServicesInstance defines the IBM Cloud DNS Services instance to create DNS Records for the cluster.
+	// +optional
+	DNSServicesInstance *DNSServicesInstance `json:"dnsServicesInstance,omitempty"`


Usually DNS is handled by the openshift itself for other providers like powervs, aws etc.. any more information of the need for adding DNS will be helpful for further discussions

Sure, I will remove this then, with that expectation.

Removed references.

mkumatag · 2024-03-13T14:16:52Z

/cc @Karthik-K-N

Karthik-K-N

I have added Initial review comments.

It would be great if you can add more documentation on api types, like usage and its dependency on setting of other variables, default values or system/controller set constants.

Karthik-K-N · 2024-03-14T05:01:28Z

docs/proposal/20240308-vpc-infra-creation.md

+# Dynamically create infrastructure required for VPC cluster
+
+## Motivation
+Currently, inorder to create a VPC cluster using cluster api we need to create


Currently capi does create various resources automatically, like vpc, subnet, loadbalancer so on, May be we can rephrase the sentence or mention the resources which we need to create manually.

+1 the present CAPIBM flow for VPC creates everything on its own.

Note - can you please elaborate here which prerequisites you are referring too?

Sure, I can attempt to make things a bit clearer.

Updated, hopefully that makes things a bit easier to follow and explains what we are trying to do, expanding the current VPC support. options.

Karthik-K-N · 2024-03-14T05:16:45Z

docs/proposal/20240308-vpc-infra-creation.md

+
+	// controlPlaneLoadBalancer is optional configuration for customizing control plane behavior.
+	// +optional
+	ControlPlaneLoadBalancer []*VPCLoadBalancerSpec `json:"controlPlaneLoadBalancer,omitempty"`


This filed is already exist in current spec, so it will be a breaking change to make it slice.

Oh, sorry, missed that change before making the change to deprecated fields. Will attempt to redesign.

Fixed, added new field, restored and marked this one as deprecated.

Karthik-K-N · 2024-03-14T05:19:21Z

docs/proposal/20240308-vpc-infra-creation.md

+	ID string `json:id"`
+
+	// type defines the type of IBM Cloud resource.
+	Type VPCResourceType `json:type"`


is resource type necessary here?

This is a nice to have, as the ResourceReference (PowerVS was using) I was modeling after has a limited amount of data (Id only, so I was hoping to have a little more data included in such a resource.

It looks like my attempt to use something like this was included recently into types.go
3add9b2#diff-c82f4c3e71cee496ae15d0bb197a279d5911a9b5b86282fa3b6f4900673078e2R120-R141

To some extent anyway, I would like to expand that list to include the additional ones listed below.

Karthik-K-N · 2024-03-14T05:19:53Z

docs/proposal/20240308-vpc-infra-creation.md

+	CRN *string `json"crn,omitempty"`
+
+	// id defines the ID of the IBM Cloud resource.
+	ID string `json:id"`


is both crn and id required, doen't crn contains id?

CRN is another nice to have, as it includes the account Id, region, etc.

I can try to determine whether including the Id at all is useful (Id was the default required field), to potentially drop it in this definition. I just don't have enough information ATM.

Karthik-K-N · 2024-03-14T05:21:51Z

docs/proposal/20240308-vpc-infra-creation.md

+	//
+	// When enabled, the IAM instance profiles specified are not used.
+	// +optional
+	PresignedURLDuration *metav1.Duration `json:"presignedURLDuration,omitempty"`


We had hard time creating a url with presignedduration, may be an experiment to verify it works and on cofirmation of it we can add this field.

I think we tried the TOKEN based authorisation and it works fine, may be you can consider using that to keep it simple with the IAM token instead of access key/secret key for this presignedURL stuff.

+1 we already have pushed code using IAM token same can be adopted here.

This was simply suggesting moving the existing CosInstance struct out of ibmpowervscluster_types.go into a "shared" location, so VPC does not redefine the resource and so VPC is not referencing a CosInstance in PowerVS managed files.

If you wish to drop or make this field deprecated, I can do that. If you wish to have a new unique resource for COS, I can implement that in a shared location (that you can choose to migrate to, if you desire).

Karthik-K-N · 2024-03-14T05:26:49Z

docs/proposal/20240308-vpc-infra-creation.md

+such as the following:
+```go
+// PortRange represents a range of ports, minimum to maximum.
+type PortRange struct {


I dont have much idea about the security group section, may be will try to understand better before taking a look,

Prajyot-Parab · 2024-03-14T10:20:15Z

docs/proposal/20240308-vpc-infra-creation.md

+	ResourceGroup string `json:"resourceGroup"`
+
+	// The Name of VPC. 
+	// Deprecated: use NetworkSpec instead.


any specific reason for moving VPC inside NetworkSpec ?
cc @Karthik-K-N

VPC is considered a part of the Network definition, which in VPC's case, can be defined in a unique ResourceGroup, along with the VPC's Subnets, SecurityGroups, etc. So those related resources seemed to fit better within a common type.

Using a NetworkSpec seems to follow other platform's design, rather than a 'flat' VPCCluster struct.

Prajyot-Parab · 2024-03-14T10:22:43Z

docs/proposal/20240308-vpc-infra-creation.md

+	VPC string `json:"vpc,omitempty"`
+
+	// The Name of availability zone.
+	// Deprecated: use NetworkSpec instead.


I don't see any field indicating zone data in NetworkSpec, how is that being handled once we deprecate this.
cc @Karthik-K-N

Zones will be defined within sub-resources. VPC isn't restricted to a single zone, it can deploy (or use) Subnets, VSI's, etc. across multiple zones, of which I expect those resources to specify their Zone.

Prajyot-Parab · 2024-03-14T10:27:58Z

docs/proposal/20240308-vpc-infra-creation.md

+	ControlPlaneLoadBalancerState *VPCLoadBalancerState `json:"controlPlaneLoadBalancerState,omitempty"`
+
+	// COSInstance is the reference to the IBM Cloud COS Instance used for the cluster.
+	COSInstance *VPCResourceReference


missing marker details

Add a new proposal with details and images for creating and deleting VPC resources for the IBMVPCCluster definition.

Prajyot-Parab · 2024-03-18T07:03:56Z

docs/proposal/20240308-vpc-infra-creation.md

+```
+
+
+#### Share existing common resource definitions


@Karthik-K-N this might break few things in PowerVS right, if we take them out of APIs.

IMO, we should focus on only VPC flow and related resources as part of this proposal. Anything related to PowerVS we can deal with later as a separate activity once we are done with VPC flow.

mkumatag · 2024-03-18T13:36:02Z

@cjschaef as discussed in our last sync up - please start working on the controller part, we can keep this item and not blocking you to implement the controller and merge

mkumatag · 2024-03-18T13:36:44Z

/easycla
/ok-to-test

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

k8s-ci-robot requested review from jichenjc and Prajyot-Parab March 12, 2024 15:52

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 Mar 12, 2024

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

mkumatag requested changes Mar 13, 2024

View reviewed changes

k8s-ci-robot requested a review from Karthik-K-N March 13, 2024 14:16

cjschaef force-pushed the vpc_infra_proposal branch from 90d843a to 21457df Compare March 13, 2024 14:47

Karthik-K-N reviewed Mar 14, 2024

View reviewed changes

Prajyot-Parab reviewed Mar 14, 2024

View reviewed changes

VPC: Add infrastructure creation proposal

cd20ff1

Add a new proposal with details and images for creating and deleting VPC resources for the IBMVPCCluster definition.

cjschaef force-pushed the vpc_infra_proposal branch from 21457df to cd20ff1 Compare March 14, 2024 22:34

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 Mar 15, 2024

Prajyot-Parab reviewed Mar 18, 2024

View reviewed changes

mkumatag added this to the Next milestone May 17, 2024

VPC: Add infrastructure creation proposal #1657

Are you sure you want to change the base?

VPC: Add infrastructure creation proposal #1657

Conversation

cjschaef commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

netlify bot commented Mar 12, 2024 • edited

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

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 commented Mar 13, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjschaef Mar 14, 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

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

Prajyot-Parab Mar 14, 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

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 commented Mar 18, 2024

mkumatag commented Mar 18, 2024

netlify bot commented Mar 12, 2024 •

edited

cjschaef Mar 14, 2024 •

edited

Prajyot-Parab Mar 14, 2024 •

edited