✨ CAPD: add DockerClusterTemplate type #4903

ykakarap · 2021-07-08T23:41:06Z

What this PR does / why we need it:
Add DockerClusterTemplate type to CAPD provider. This type will be used by 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 #4900

test/infrastructure/docker/config/rbac/dockerclustertemplate_editor_role.yaml

test/infrastructure/docker/config/rbac/dockerclustertemplate_viewer_role.yaml

sbueringer · 2021-07-09T09:35:50Z

Nothing additional from me, lgtm apart from @fabriziopandini's comments

fabriziopandini

Change lgtm pending adding web hook ensuring template are immutable.
Also, even do there are not yet validation rules on create, should we create an empty func called on creation by both DockerClusterTemplate and DockerCluster web hooks, so we ensure that future validation rules applies to both types consistently?

ykakarap · 2021-07-14T00:17:58Z

Also, even do there are not yet validation rules on create, should we create an empty func called on creation by both DockerClusterTemplate and DockerCluster web hooks, so we ensure that future validation rules applies to both types consistently?

+1 to this

ykakarap · 2021-07-14T00:33:28Z

/retest

ykakarap · 2021-07-14T03:35:05Z

/retest

fabriziopandini

only two small nits left, otherwise lgtm

test/infrastructure/docker/api/v1alpha4/dockercluster_webhook.go

test/infrastructure/docker/api/v1alpha4/dockercluster_default.go

sbueringer · 2021-07-15T16:27:43Z

test/infrastructure/docker/api/v1alpha4/dockerclustertemplate_webhook.go

+		Complete()
+}
+
+//+kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1alpha4-dockerclustertemplate,mutating=true,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=dockerclustertemplates,versions=v1alpha4,name=validation.dockerclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1


I think you have to put a space between // and +. So //+ => // +
I'm not sure if there are cases where those comments are not picked up without a space. (but saw similar issues with these kinds of magic comments)

Can you grep for occurrences of //+ there are two other cases in DockerMachinePool which we probably should also fix.

I will make sure to fix all of them :)

sbueringer · 2021-07-15T16:29:40Z

test/infrastructure/docker/api/v1alpha4/dockerclustertemplate_webhook.go

+
+// Default implements webhook.Defaulter so a webhook will be registered for the type.
+func (r *DockerClusterTemplate) Default() {
+	dockerclustertemplatelog.Info("default", "name", r.Name)


In case we drop the logs in the other webhook, we should also drop them here

sbueringer · 2021-07-15T16:34:57Z

test/infrastructure/docker/config/webhook/manifests.yaml

@@ -1,11 +1,100 @@

+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration


I think adding this requires additions to webhookcainjection_patch.yaml

Are there example of the kinds of additions I need to do?

Also do you think this could this be the reason for the piepline failing with the following error:

Error from server (InternalError): error when creating "STDIN": Internal error occurred: failed calling webhook "validation.dockercluster.infrastructure.cluster.x-k8s.io": Post "https://capd-webhook-service.capd-system.svc:443/mutate-infrastructure-cluster-x-k8s-io-v1alpha4-dockercluster?timeout=10s": x509: certificate signed by unknown authority

Roughly: (we should also have it in the webhookcainjection_patch.yaml files of other controllers)

apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: name: mutating-webhook-configuration annotations: cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)

P.S. Yup, that's the cause of the error :)

ykakarap · 2021-07-15T20:38:49Z

/retest

ykakarap · 2021-07-15T21:16:51Z

/retest

sbueringer · 2021-07-16T05:52:18Z

@ykakarap You have to run make generate-go

ykakarap · 2021-07-16T16:37:00Z

/retest

ykakarap · 2021-07-16T21:40:10Z

@fabriziopandini addressed all the comments on the PR and all pipelines are green.
Please review when you can.

fabriziopandini · 2021-07-19T10:32:21Z

/lgtm
@sbueringer @stmcginnis PTAL

sbueringer · 2021-07-19T15:23:42Z

test/infrastructure/docker/api/v1alpha4/dockerclustertemplate_webhook.go

+func (r *DockerClusterTemplate) ValidateUpdate(oldRaw runtime.Object) error {
+	old := oldRaw.(*DockerClusterTemplate)
+	if !reflect.DeepEqual(r.Spec, old.Spec) {
+		return errors.New("DockerClusterTemplateSpec is immutable")


nit: Maybe make this error text consistent to the one used in: https://github.com/kubernetes-sigs/cluster-api/pull/4904/files

nit addressed.

sbueringer · 2021-07-20T08:21:23Z

test/infrastructure/docker/api/v1alpha4/dockermachinetemplate_webhook.go

-		return errors.New("DockerMachineTemplateSpec is immutable")
+func (m *DockerMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error {
+	var allErrs field.ErrorList
+	old := oldRaw.(*DockerMachineTemplate)


Sorry last one. We usually seem to check if the type case works and then return an error (instead of a panic which would happen otherwise)

oldM, ok := old.(*Machine) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", old)) }

(same for DockerClusterTemplate and the KubeadmControlPlaneTemplate PR)

fabriziopandini · 2021-07-20T15:33:51Z

/lgtm

ykakarap · 2021-07-20T15:35:17Z

/retest

ykakarap · 2021-07-20T16:01:04Z

/retest

sbueringer · 2021-07-20T16:13:12Z

@ykakarap Thank you very much!

/lgtm

stmcginnis · 2021-07-20T16:44:44Z

Just wondering - not saying it needs to happen, just looking to learn from the larger team - do we need to add any documentation about this to somewhere like docs/book/src/tasks/experimental-features/experimental-features.md?

ykakarap · 2021-07-23T01:46:52Z

@fabriziopandini had to rebase because of merge conflicts. Can you please re-lgtm this? 😄

sbueringer · 2021-07-23T11:44:57Z

/lgtm

fabriziopandini · 2021-07-23T11:49:50Z

/approve

k8s-ci-robot · 2021-07-23T11:49:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

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 JoelSpeed and sbueringer July 8, 2021 23:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 8, 2021

fabriziopandini reviewed Jul 9, 2021

View reviewed changes

test/infrastructure/docker/config/rbac/dockerclustertemplate_editor_role.yaml Outdated Show resolved Hide resolved

test/infrastructure/docker/config/rbac/dockerclustertemplate_viewer_role.yaml Outdated Show resolved Hide resolved

ykakarap force-pushed the dockerclustertemplate branch from b202ff0 to 3b95493 Compare July 9, 2021 18:09

fabriziopandini reviewed Jul 12, 2021

View reviewed changes

sbueringer mentioned this pull request Jul 12, 2021

✨ Add OpenStackClusterTemplates Type kubernetes-sigs/cluster-api-provider-openstack#933

Merged

3 tasks

ykakarap force-pushed the dockerclustertemplate branch from 3b95493 to c46cbfd Compare July 14, 2021 00:15

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2021

ykakarap force-pushed the dockerclustertemplate branch 2 times, most recently from 83fe825 to 2b85978 Compare July 14, 2021 00:23

ykakarap force-pushed the dockerclustertemplate branch from 2b85978 to a150e01 Compare July 14, 2021 02:48

ykakarap force-pushed the dockerclustertemplate branch 4 times, most recently from 27c4b6e to 903c2cb Compare July 14, 2021 05:55

fabriziopandini reviewed Jul 14, 2021

View reviewed changes

test/infrastructure/docker/api/v1alpha4/dockercluster_webhook.go Outdated Show resolved Hide resolved

test/infrastructure/docker/api/v1alpha4/dockercluster_default.go Outdated Show resolved Hide resolved

sbueringer reviewed Jul 15, 2021

View reviewed changes

ykakarap force-pushed the dockerclustertemplate branch from 903c2cb to 90001cf Compare July 15, 2021 20:33

ykakarap force-pushed the dockerclustertemplate branch from 90001cf to dd9ec25 Compare July 15, 2021 20:51

ykakarap force-pushed the dockerclustertemplate branch from dd9ec25 to 53d4ef6 Compare July 15, 2021 21:45

k8s-ci-robot assigned fabriziopandini Jul 19, 2021

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

sbueringer reviewed Jul 19, 2021

View reviewed changes

ykakarap force-pushed the dockerclustertemplate branch from 5afc219 to ef6017d Compare July 19, 2021 22:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2021

sbueringer reviewed Jul 20, 2021

View reviewed changes

ykakarap force-pushed the dockerclustertemplate branch from ef6017d to 565286f Compare July 20, 2021 15:30

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

k8s-ci-robot assigned sbueringer Jul 20, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021

CAPD: add DockerClusterTemplate type

fce43ed

ykakarap force-pushed the dockerclustertemplate branch from 565286f to fce43ed Compare July 23, 2021 01:45

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 Jul 23, 2021

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

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

k8s-ci-robot merged commit f85f37a into kubernetes-sigs:master Jul 23, 2021

k8s-ci-robot added this to the v0.4 milestone Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ CAPD: add DockerClusterTemplate type #4903

✨ CAPD: add DockerClusterTemplate type #4903

ykakarap commented Jul 8, 2021

sbueringer commented Jul 9, 2021

fabriziopandini left a comment •

edited

Loading

ykakarap commented Jul 14, 2021

ykakarap commented Jul 14, 2021

ykakarap commented Jul 14, 2021

fabriziopandini left a comment

sbueringer Jul 15, 2021

ykakarap Jul 15, 2021

sbueringer Jul 15, 2021

sbueringer Jul 15, 2021

ykakarap Jul 15, 2021

sbueringer Jul 15, 2021 •

edited

Loading

ykakarap commented Jul 15, 2021

ykakarap commented Jul 15, 2021

sbueringer commented Jul 16, 2021

ykakarap commented Jul 16, 2021

ykakarap commented Jul 16, 2021

fabriziopandini commented Jul 19, 2021

sbueringer Jul 19, 2021

ykakarap Jul 19, 2021

sbueringer Jul 20, 2021 •

edited

Loading

sbueringer Jul 20, 2021

fabriziopandini commented Jul 20, 2021

ykakarap commented Jul 20, 2021

ykakarap commented Jul 20, 2021

sbueringer commented Jul 20, 2021

stmcginnis commented Jul 20, 2021

ykakarap commented Jul 23, 2021

sbueringer commented Jul 23, 2021

fabriziopandini commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

✨ CAPD: add DockerClusterTemplate type #4903

✨ CAPD: add DockerClusterTemplate type #4903

Conversation

ykakarap commented Jul 8, 2021

sbueringer commented Jul 9, 2021

fabriziopandini left a comment • edited Loading

Choose a reason for hiding this comment

ykakarap commented Jul 14, 2021

ykakarap commented Jul 14, 2021

ykakarap commented Jul 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

ykakarap commented Jul 15, 2021

ykakarap commented Jul 15, 2021

sbueringer commented Jul 16, 2021

ykakarap commented Jul 16, 2021

ykakarap commented Jul 16, 2021

fabriziopandini commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 20, 2021

ykakarap commented Jul 20, 2021

ykakarap commented Jul 20, 2021

sbueringer commented Jul 20, 2021

stmcginnis commented Jul 20, 2021

ykakarap commented Jul 23, 2021

sbueringer commented Jul 23, 2021

fabriziopandini commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

fabriziopandini left a comment •

edited

Loading

sbueringer Jul 15, 2021 •

edited

Loading

sbueringer Jul 20, 2021 •

edited

Loading