add VSphereClusterTemplate type #1207

ykakarap · 2021-07-08T21:07:13Z

What this PR does / why we need it:
Add VSphereClusterTemplate type. This type is needed to work with ClusterClass
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 #1206

Release note:

Adds VSphereClusterTemplate type

ykakarap · 2021-07-08T21:07:25Z

/assign @srm09

fabriziopandini

@ykakarap thanks for this PR!

Could you kindly enable web hooks for the new type ensuring that template type applies the same defaulting & validation rules of the actual type, otherwise this could trigger continuous reconcile loop down the path when comparing the desired state (the VSphereTemplate object) with the actual state (the instance of VSphereCluster generated from the template)?

fabriziopandini · 2021-07-12T07:59:17Z

api/v1alpha4/vsphereclustertemplate_webhook.go

+	spec := ct.Spec.Template.Spec
+
+	if spec.Thumbprint != "" && spec.Insecure != nil && *spec.Insecure {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec", "Insecure"), spec.Insecure, "cannot be set to true at the same time as .spec.template.spec.Thumbprint"))
+	}


Might be an implementation detail to be considered by the CAPV mantainers, but I'm wondering if this should be a func shared between with VSphereCluster webhook, so we are sure that the two validations web hooks are kept in sync over time?

fabriziopandini · 2021-07-12T08:01:53Z

api/v1alpha4/vsphereclustertemplate_webhook.go

+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+func (ct *VSphereClusterTemplate) ValidateUpdate(old runtime.Object) error {


Should we make this immutable, like the other templates?

ykakarap · 2021-07-16T21:44:26Z

/retest

fabriziopandini · 2021-07-19T08:22:03Z

thanks for updating, changes lgtm to me
I leave the final word to CAPV maintainers

srm09

The error in the tests is due to the new defaulting webhook being introduced for VSphereCluster type. If the webhook isn't necessary, we should remove it, which in turn would remove the error. Looking forward to hearing about the reasoning behind the webhook.

srm09 · 2021-07-19T17:42:14Z

config/rbac/vsphereclustertemplate_editor_role.yaml

@@ -0,0 +1,24 @@
+# permissions for end users to edit vsphereclustertemplates.


This file is not required, could you please remove this.

The file config/rbac/role.yaml gets updated once we make changes to the controllers to allow them to read/update/delete/reconcile the template objects.

srm09 · 2021-07-19T17:42:55Z

config/rbac/vsphereclustertemplate_viewer_role.yaml

@@ -0,0 +1,20 @@
+# permissions for end users to view vsphereclustertemplates.


same as above.

srm09 · 2021-07-19T17:43:13Z

config/samples/infrastructure_v1alpha4_vsphereclustertemplate.yaml

@@ -0,0 +1,7 @@
+apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4


same as above.

srm09 · 2021-07-19T17:44:52Z

api/v1alpha4/vspherecluster_webhook.go

+	if s.Thumbprint != "" && s.Insecure != nil && *s.Insecure {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "Insecure"), s.Insecure, "cannot be set to true at the same time as .spec.Thumbprint"))
+	}


This is an FYI, the struct variable Insecure is deprecated and is scheduled for removal in v1alpha4. So this will go away once the PR for removing deprecated types gets merged.

srm09 · 2021-07-19T17:48:39Z

api/v1alpha4/vspherecluster_webhook.go

+func (c *VSphereCluster) Default() {
+	defaultVSphereCluterSpec(&c.Spec)
+}


Is there a specific reason that the defaulting webhook is being included here? I see the implementation of the method is empty. If this is for the sake of future usage, let's introduce it only when there is some specific defaulting logic that needs to be implemented.

Yes, it is for future use cases. So that we make sure to introduce consistent defaulting for template and the cluster type when ever we do that.

My vote is to wait till the need arises for a separate webhook. Adding a note in the clustertemplate_webhook to make sure this needs to be replicated across the two types would be a way to move forward.
Thoughts @gab-satchi @yastij ?

+1 on not adding dead code

Thanks for the feedback @srm09 @yastij. Will update the PR :)

ykakarap · 2021-07-26T01:49:38Z

@srm09 Addressed all review comments. PTAL.

srm09 · 2021-07-27T23:20:47Z

/lgtm
/assign @yastij for final +1 before merging it in

ykakarap · 2021-08-05T23:58:08Z

@srm09 Had to rebase because of conflicts. Can you please re-lgtm? 🙂

srm09 · 2021-08-06T00:00:08Z

/lgtm

randomvariable · 2021-08-11T17:17:47Z

/approve

k8s-ci-robot · 2021-08-11T17:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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~~ [randomvariable]

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 8, 2021

k8s-ci-robot requested review from chuckha and detiber July 8, 2021 21:07

k8s-ci-robot assigned srm09 Jul 8, 2021

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 8, 2021

ykakarap force-pushed the vsphereclustertemplate branch 2 times, most recently from b6f603a to 5fa0c51 Compare July 9, 2021 00:07

fabriziopandini reviewed Jul 9, 2021

View reviewed changes

ykakarap force-pushed the vsphereclustertemplate branch from 5fa0c51 to 446b630 Compare July 9, 2021 18:58

fabriziopandini reviewed Jul 12, 2021

View reviewed changes

ykakarap changed the title ~~add VSphereClusterTemplate type~~ [WIP] add VSphereClusterTemplate type Jul 16, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2021

ykakarap force-pushed the vsphereclustertemplate branch from 446b630 to 0126201 Compare July 16, 2021 21:36

ykakarap force-pushed the vsphereclustertemplate branch 2 times, most recently from fec3a24 to 85fa631 Compare July 16, 2021 22:11

srm09 reviewed Jul 19, 2021

View reviewed changes

ykakarap force-pushed the vsphereclustertemplate branch 2 times, most recently from b6141aa to 7878ef2 Compare July 26, 2021 00:54

ykakarap changed the title ~~[WIP] add VSphereClusterTemplate type~~ add VSphereClusterTemplate type Jul 26, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 27, 2021

add VSphereClusterTemplate type

e332e5e

ykakarap force-pushed the vsphereclustertemplate branch from 7878ef2 to e332e5e Compare August 5, 2021 23:57

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2021

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

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

k8s-ci-robot merged commit 42649cc into kubernetes-sigs:master Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add VSphereClusterTemplate type #1207

add VSphereClusterTemplate type #1207

ykakarap commented Jul 8, 2021 •

edited

Loading

ykakarap commented Jul 8, 2021

fabriziopandini left a comment

fabriziopandini Jul 12, 2021

fabriziopandini Jul 12, 2021

ykakarap commented Jul 16, 2021

fabriziopandini commented Jul 19, 2021

srm09 left a comment

srm09 Jul 19, 2021

srm09 Jul 20, 2021

srm09 Jul 19, 2021

srm09 Jul 19, 2021

srm09 Jul 19, 2021

srm09 Jul 19, 2021

ykakarap Jul 19, 2021

srm09 Jul 19, 2021

yastij Jul 20, 2021

ykakarap Jul 20, 2021

ykakarap commented Jul 26, 2021

srm09 commented Jul 27, 2021

ykakarap commented Aug 5, 2021

srm09 commented Aug 6, 2021

randomvariable commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

		@@ -0,0 +1,24 @@
		# permissions for end users to edit vsphereclustertemplates.

		@@ -0,0 +1,20 @@
		# permissions for end users to view vsphereclustertemplates.

		@@ -0,0 +1,7 @@
		apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4

add VSphereClusterTemplate type #1207

add VSphereClusterTemplate type #1207

Conversation

ykakarap commented Jul 8, 2021 • edited Loading

ykakarap commented Jul 8, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykakarap commented Jul 16, 2021

fabriziopandini commented Jul 19, 2021

srm09 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

Choose a reason for hiding this comment

ykakarap commented Jul 26, 2021

srm09 commented Jul 27, 2021

ykakarap commented Aug 5, 2021

srm09 commented Aug 6, 2021

randomvariable commented Aug 11, 2021

k8s-ci-robot commented Aug 11, 2021

ykakarap commented Jul 8, 2021 •

edited

Loading