Vpc extended apis #1895

cjschaef · 2024-07-23T20:38:02Z

Extending VPC related Cluster API's in order to provide additional VPC Infrastructure reconciliation support.

What this PR does / why we need it: introduces API's that will be used for extended VPC Cluster (Infrastructure) support

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 additional API's for extended VPC Cluster support

k8s-ci-robot · 2024-07-23T20:38: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-sigs/prow repository.

netlify · 2024-07-23T20:38:20Z

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

Name	Link
🔨 Latest commit	`4012d7c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/66c60b58c96a7000085b9642
😎 Deploy Preview	https://deploy-preview-1895--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-07-24T06:20:40Z

/cc @Karthik-K-N

mkumatag · 2024-07-24T06:20:46Z

/ok-to-test

Karthik-K-N · 2024-07-24T09:59:01Z

api/v1beta2/ibmvpccluster_types.go

+
+	// securityGroups defines the load balancer's security groups.
+	// +optional
+	SecurityGroups []VPCResource `json:"securityGroups,omitempty"`


I think you are planing to have SG seperatly for LB as well as VPC, do you have any use case in mind?
Also we recently implemented it for PowerVS see if it helps

cluster-api-provider-ibmcloud/api/v1beta2/ibmpowervscluster_types.go

Line 109 in 104ee47

VPCSecurityGroups []VPCSecurityGroup `json:"vpcSecurityGroups,omitempty"`

The SG's listed for the LB merely list the SecurityGroups the LB expects to have attached during LB creation.

The SecurityGroup (VPCSecurityGroup) reconciliation occurs separately, so we expect the SG's should already exist by the time we reach LB reconciliation (or error if they do not).

In that case can you please reframe the description the same for better understanding?
It looks like, you are defining the SG from scratch.

I added some details that the SG's are expected to exist for LB reconciliation

I've also added subnet definitions to LB's, since we don't expect to default to using all subnets.

Karthik-K-N · 2024-07-24T10:10:52Z

api/v1beta2/ibmvpccluster_types.go

+
+	// backendPools defines the load balancer's backend pools.
+	// +optional
+	BackendPools []BackendPoolSpec `json:"backendPools,omitempty"`


Whats the use case you have in mind to expose Backedpool,Do you think user should worry about it? Also what happens if user does not pass anything here?

I currently have some default pools implemented in test code. I have not yet decided whether to leave them in, or force the user to have to define pools (no defaults).

Karthik-K-N · 2024-07-24T10:14:17Z

api/v1beta2/ibmvpccluster_types.go

+
+	// resourceGroup is the Resource Group to create the Custom Image in.
+	// +optional
+	ResourceGroup *IBMCloudResourceReference `json:"resourceGroup,omitempty"`


I think we have ResourceGroup at various place. Do you think this option is needed, can't we create image in cluster resource group?

The ResourceGroup is imbedded within Power code, I do not wish reuse Power dependent code

cluster-api-provider-ibmcloud/api/v1beta2/ibmpowervscluster_types.go

Lines 175 to 182 in 112f968

// ResourceReference identifies a resource with id.

type ResourceReference 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"`

}

The ResourceGroup can be used for Image creation, or to retrieve an existing image. So, the RG doesn't have to match the Cluster RG.

Karthik-K-N · 2024-07-24T10:21:21Z

api/v1beta2/ibmvpccluster_types.go

-	// resourceGroup is the name of the Resource Group containing all of the newtork resources.
+	// loadBalancers is a set of VPC Load Balancer definitions to use for the cluster.
+	// +optional
+	LoadBalancers []VPCLoadBalancerSpec `json:"loadbalancers,omitempty"`


Since we are defining here as well, We may need to add proper comment for VPCSpec.ControlplaneLoadbalancer.

Sure, I can try to add something to the old field.

Added some details about legacy use versus extended support.

Karthik-K-N · 2024-07-25T07:09:25Z

@dharaneeshvrd please take a look when you get some time.

mkumatag · 2024-08-01T04:50:01Z

/cc @dharaneeshvrd

mkumatag

I have added few comments about adding validation to all the new fields added, lets add validation for all the newly introduced fields referring the apispec from the doc here - https://cloud.ibm.com/apidocs/vpc/latest#create-load-balancer

mkumatag · 2024-08-05T04:28:05Z

api/v1beta2/ibmvpccluster_types.go

+
+	// algorithm defines the load balancing algorithm to use.
+	// +required
+	Algorithm string `json:"algorithm"`


lets add a validation here for the Allowable values: [least_connections,round_robin,weighted_round_robin] and also a default value set when no option mentioned

added an enum

mkumatag · 2024-08-05T04:28:59Z

api/v1beta2/ibmvpccluster_types.go

+
+	// protocol defines the protocol to use for the Backend Pool.
+	// +required
+	Protocol string `json:"protocol"`


can we add validation here for the algorithm like Allowable values: [http,https,tcp,udp]

added an enum

mkumatag · 2024-08-05T04:29:56Z

api/v1beta2/ibmvpccluster_types.go

+	// name defines the name of the Backend Pool.
 	// +optional
-	WorkerSubnets []Subnet `json:"workerSubnets,omitempty"`
+	Name *string `json:"name,omitempty"`


lets add a regex for the validation here - Possible values: 1 ≤ length ≤ 63, Value must match regular expression ^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$

dharaneeshvrd

LGTM overall!

dharaneeshvrd · 2024-08-05T07:04:37Z

api/v1beta2/ibmvpccluster_types.go

+
+	// securityGroups defines the load balancer's security groups.
+	// +optional
+	SecurityGroups []VPCResource `json:"securityGroups,omitempty"`


In that case can you please reframe the description the same for better understanding?
It looks like, you are defining the SG from scratch.

cjschaef · 2024-08-06T00:35:03Z

Based on further investigation on requirements for Catalog Offerings, I have extended the ImageSpec to include a CatalogOffering resource now as well, to allow user to define whether an existing VPC Image is used, a Catalog Offering, or a new VPC Image gets created during Infrastructure reconciliation.

dharaneeshvrd · 2024-08-07T03:56:18Z

api/v1beta2/ibmvpccluster_types.go

-type VPCNetworkSpec struct {
-	// workerSubnets is a set of Subnet's which define the Worker subnets.
+// BackendPoolSpec defines the desired configuration of a VPC Load Balancer Backend Pool.
+type BackendPoolSpec struct {


Can we prefix with VPCLoadBalancer to align with other structs?

dharaneeshvrd · 2024-08-07T03:57:02Z

api/v1beta2/ibmvpccluster_types.go

+
+// HealthMonitorSpec defines the desired state of a Health Monitor resource for a VPC Load Balancer Backend Pool.
+// kubebuilder:validation:XValidation:rule="self.dely > self.timeout",message="health monitor's delay must be greater than the timeout"
+type HealthMonitorSpec struct {


Same as other comment, it would be good to prefix this with VPCLoadBalancer to be more clear IMO.
Also can we keep it as VPCLoadBalancerHealthCheckSpec to align with VPC term and here also I can see you have referred as health check in many places?

Updated the struct name. I was trying to stay aligned with the IBM Cloud API documentation, as it refers to the fields for health checks.

dharaneeshvrd · 2024-08-07T04:09:01Z

api/v1beta2/types.go

 )

+// CatalogOffering represents an IBM Cloud Catalog Offering resource.
+type CatalogOffering struct {


Suggested change

type CatalogOffering struct {

type IBMCloudCatalogOffering struct {

dharaneeshvrd · 2024-08-07T04:13:33Z

api/v1beta2/ibmvpccluster_types.go

+
+	// catalogOffering defines an existing Catalog Offering Image.
+	// +optional
+	CatalogOffering *CatalogOffering `json:"catalogOffering,omitempty"`


CatalogOffering's spec looks like to identify generic ibm cloud catalog offering, how are you planning to use that to identify the catalog image?

Using a Catalog Offering for a VSI "image" requires either a Catalog Offering CRN or Catalog Offering Version CRN (with an optional Catalog Offering Plan CRN for either) per the IBM Cloud VPC API's.

Supplying the catalogOffering field for the ImageSpec, with one of the offeringCRN or versionCRN, with result in the VSI API calls to be made using an InstancePrototypeInstanceByCatalogOffering instead of the ...ByImage for the API calls to create a VSI.

I dropped the Catalog Offering since we may not have permission to validate the Image as an Offering. We will have to rely on results/errors during Machine creation.

cjschaef · 2024-08-09T14:39:21Z

Rebased to resolve mock generation.

cjschaef · 2024-08-09T17:18:12Z

I dropped the Catalog Offering configuration for the Image definition, as I expect to handle it only within Machine API's and we may not have permission to validate a Catalog Offering, thus providing it in the Infrastructure is not useful.

dharaneeshvrd · 2024-08-12T04:35:06Z

It seems you need to regenerate the CRD def yamls, otherwise it LGTM!

cjschaef · 2024-08-12T15:18:50Z

@dharaneeshvrd Is there something specific I need to run to do that?

dharaneeshvrd · 2024-08-14T04:50:23Z

make generate would do that.

cjschaef · 2024-08-14T13:04:47Z

@dharaneeshvrd I have run that prior, I don't think it passes CI tests unless the CRD files are updated.
Is there something specific missing, or perhaps something I need to add into a list to generate what appears to be missing?

dharaneeshvrd · 2024-08-16T05:18:41Z

@cjschaef Apologies for the confusion, it is updated properly.
LGTM, @mkumatag please review!

mkumatag

I don't see validations working fine, please fix them.

e.g: I created following spec and it got created without any issues

apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: IBMVPCCluster
metadata:
  labels:
    cluster.x-k8s.io/cluster-name: test-1
  name: test-1
spec:
  region: us-south
  resourceGroup: Default
  vpc: vpc-1
  zone: us-south-1
  controlPlaneLoadBalancer:
    name: lb-test
    backendPools:
    - name: pool-1
      algorithm: invalid-algo
      healthMonitor:
        delay: 2
        retries: 1
        timeout: 1
        type: invalidtype

oc get IBMVPCCluster -A -o=yaml
apiVersion: v1
items:
- apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
  kind: IBMVPCCluster
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"infrastructure.cluster.x-k8s.io/v1beta2","kind":"IBMVPCCluster","metadata":{"annotations":{},"labels":{"cluster.x-k8s.io/cluster-name":"test-1"},"name":"test-1","namespace":"default"},"spec":{"controlPlaneLoadBalancer":{"backendPools":[{"algorithm":"invalid-algo","healthMonitor":{"delay":2,"retries":1,"timeout":1,"type":"invalidtype"},"name":"pool-1"}],"name":"lb-test"},"region":"us-south","resourceGroup":"Default","vpc":"vpc-1","zone":"us-south-1"}}
    creationTimestamp: "2024-08-16T07:37:55Z"
    generation: 1
    labels:
      cluster.x-k8s.io/cluster-name: test-1
    name: test-1
    namespace: default
    resourceVersion: "2534"
    uid: 5147b1a5-2251-484c-b3c4-335b0d6c57f3
  spec:
    controlPlaneEndpoint:
      host: ""
      port: 0
    controlPlaneLoadBalancer:
      backendPools:
      - algorithm: invalid-algo
        healthMonitor:
          delay: 2
          retries: 1
          timeout: 1
          type: invalidtype
        name: pool-1
        protocol: ""
      name: lb-test
      public: true
    region: us-south
    resourceGroup: Default
    vpc: vpc-1
    zone: us-south-1
kind: List
metadata:
  resourceVersion: ""

Extending VPC related Cluster API's in order to provide additional VPC Infrastructure reconciliation support.

cjschaef · 2024-08-21T15:52:19Z

@mkumatag I added some enum validation to some of the type fields, but I cannot validate locally to confirm things work as expected (issues with CRD resource creation). Please let me know if you see further issues.

mkumatag · 2024-08-28T09:29:21Z

@mkumatag I added some enum validation to some of the type fields, but I cannot validate locally to confirm things work as expected (issues with CRD resource creation). Please let me know if you see further issues.

Getting a proper message now,

k apply -f test/vpc_cluster.yaml 
The IBMVPCCluster "test-1" is invalid: 
* spec.controlPlaneLoadBalancer.backendPools[0].algorithm: Unsupported value: "invalid-algo": supported values: "least_connections", "round_robin", "weighted_round_robin"
* spec.controlPlaneLoadBalancer.backendPools[0].healthMonitor.type: Unsupported value: "invalidtype": supported values: "http", "https", "tcp"
* spec.controlPlaneLoadBalancer.backendPools[0].protocol: Unsupported value: "": supported values: "http", "https", "tcp", "udp"
(base)

mkumatag

/lgtm

k8s-ci-robot · 2024-08-30T02:39:26Z

[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

k8s-ci-robot added area/provider/ibmcloud Issues or PRs related to ibmcloud provider 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 Jul 23, 2024

k8s-ci-robot requested review from Prajyot-Parab and mkumatag July 23, 2024 20:38

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 23, 2024

k8s-ci-robot requested a review from Karthik-K-N July 24, 2024 06:20

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 Jul 24, 2024

Karthik-K-N reviewed Jul 24, 2024

View reviewed changes

cjschaef force-pushed the vpc_extended_apis branch from 5ac1389 to 3b807d2 Compare July 24, 2024 14:17

k8s-ci-robot requested a review from dharaneeshvrd August 1, 2024 04:50

mkumatag requested changes Aug 5, 2024

View reviewed changes

dharaneeshvrd reviewed Aug 5, 2024

View reviewed changes

cjschaef force-pushed the vpc_extended_apis branch 2 times, most recently from 8b2f334 to ee67c25 Compare August 6, 2024 00:13

mkumatag added this to the v0.9.0 milestone Aug 6, 2024

cjschaef force-pushed the vpc_extended_apis branch from ee67c25 to 4f33c89 Compare August 6, 2024 18:42

dharaneeshvrd reviewed Aug 7, 2024

View reviewed changes

cjschaef force-pushed the vpc_extended_apis branch 4 times, most recently from 7b0425c to 0e232d8 Compare August 9, 2024 14:39

cjschaef force-pushed the vpc_extended_apis branch from 0e232d8 to 4548245 Compare August 9, 2024 17:17

mkumatag requested changes Aug 16, 2024

View reviewed changes

cjschaef added 2 commits August 19, 2024 11:33

VPC: API updates for extended VPC support

45bb559

Extending VPC related Cluster API's in order to provide additional VPC Infrastructure reconciliation support.

VPC: Update Network RG reference and lookup per API

4012d7c

cjschaef force-pushed the vpc_extended_apis branch from 4548245 to 4012d7c Compare August 21, 2024 15:44

cjschaef mentioned this pull request Aug 26, 2024

Extend VPC (x86_64) Infrastructure reconciliation support #1896

Closed

mkumatag approved these changes Aug 30, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Aug 30, 2024

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

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

k8s-ci-robot merged commit 73983fd into kubernetes-sigs:main Aug 30, 2024

cjschaef deleted the vpc_extended_apis branch August 30, 2024 13:26

	// ResourceReference identifies a resource with id.
	type ResourceReference 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"`
	}

	type CatalogOffering struct {
	type IBMCloudCatalogOffering struct {

Vpc extended apis #1895

Vpc extended apis #1895

Uh oh!

Conversation

cjschaef commented Jul 23, 2024

Uh oh!

k8s-ci-robot commented Jul 23, 2024

Uh oh!

netlify bot commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

mkumatag commented Jul 24, 2024

Uh oh!

mkumatag commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N commented Jul 25, 2024

Uh oh!

mkumatag commented Aug 1, 2024

Uh oh!

mkumatag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjschaef commented Aug 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Jul 23, 2024 •

edited

Loading