🌱 Added C-R validating webhook for KCP scale subresource #5697

dharmjit · 2021-11-18T09:26:41Z

Signed-off-by: Dharmjit Singh sdharmjit@vmware.com

What this PR does / why we need it:
This PR introduces a Controller-Runtime validating webhook for kubeadmControlPlane scale subresource.

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 #5466

k8s-ci-robot · 2021-11-18T09:26:49Z

Hi @dharmjit. 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.

sbueringer · 2021-11-18T09:31:18Z

/ok-to-test

@dharmjit Can you please rebase on top of main to get rid of the merge conflict? (otherwise the tests won't run)

linux-foundation-easycla · 2021-11-18T09:41:46Z

The committers are authorized under a signed CLA.

✅ Dharmjit Singh (f9325fd, d9fc061)

sbueringer

Looks great, thx! A bunch of nits, but nothing major :)

controlplane/kubeadm/config/webhook/manifests.yaml

controlplane/kubeadm/main.go

sbueringer · 2021-11-18T10:34:29Z

controlplane/kubeadm/webhooks/scale.go

+// +kubebuilder:webhook:verbs=create;update,path=/validate-scale-controlplane-cluster-x-k8s-io-v1beta1-kubeadmcontrolplane,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=controlplane.cluster.x-k8s.io,resources=kubeadmcontrolplanes/scale,versions=v1beta1,name=validation-scale.kubeadmcontrolplane.controlplane.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
+
+// KCPScaleValidator validates KCP for replicas.
+type KCPScaleValidator struct {


Not sure about the name, maybe we should call it just KCPScale or KubeadmControlPlane
(let's wait for other opinions before potentially changing it)

Not KubeadmControlPlane as it's distinctly the scale subresource, and it's very much an admission webhook, so it's more or less fine, however maybe ScaleValidator to avoid philosophically stuttering..

controlplane/kubeadm/webhooks/doc.go

controlplane/kubeadm/webhooks/scale.go

sbueringer · 2021-11-18T10:42:02Z

controlplane/kubeadm/webhooks/scale.go

+// KCPScaleValidator implements admission.DecoderInjector.
+// A decoder will be automatically injected.
+
+// InjectDecoder injects the decoder.


Suggested change

// KCPScaleValidator implements admission.DecoderInjector.

// A decoder will be automatically injected.

// InjectDecoder injects the decoder.

// InjectDecoder injects the decoder.

// KCPScaleValidator implements admission.DecoderInjector.

// A decoder will be automatically injected.

sbueringer · 2021-11-18T10:48:59Z

Regrading back porting: Let's discuss that later, usually it's just triggering the cherry pick bot (but we have to decide to which branches we want to backport)

Regarding testing: I would suggest a table test ~ like it's done here:

cluster-api/api/v1beta1/machinedeployment_webhook_test.go

Line 60 in ec1fa65

func TestMachineDeploymentValidation(t *testing.T) {

But you have to additionally:

create the webhook with a fakeclient (you can see in cluster_test.go how to do that)
set a decoder. I think you can do that with admission.NewDecoder(scheme). You probably have to create a scheme for the test and add the KCP types ~:

fakeScheme   = runtime.NewScheme()
_ = kcpv1.AddToScheme(scheme)

dharmjit · 2021-11-25T17:05:09Z

@sbueringer @randomvariable, PTAL, I had fixed the review comments and also added the test for the webhook.

sbueringer

Thx!!

One last nit from my side, otherwise lgtm.

Feel free to resolve the resolved conversations.

controlplane/kubeadm/webhooks/scale_test.go

sbueringer · 2021-11-29T12:49:54Z

/lgtm

/assign @randomvariable

vincepri · 2021-12-01T16:23:14Z

controlplane/kubeadm/main.go

+	mgr.GetWebhookServer().Register("/validate-scale-controlplane-cluster-x-k8s-io-v1beta1-kubeadmcontrolplane", &webhook.Admission{
+		Handler: &kcpwebhooks.ScaleValidator{
+			Client: mgr.GetClient(),
+		},
+	})


@sbueringer Sidenote, the Scale subresource should probably have top-level support in Controller Runtime

vincepri · 2021-12-01T16:24:34Z

controlplane/kubeadm/webhooks/scale.go

+
+	if !externalEtcd {
+		if scale.Spec.Replicas%2 == 0 {
+			return admission.Denied("replicas cannot be an even number when using managed etcd")


Suggested change

return admission.Denied("replicas cannot be an even number when using managed etcd")

return admission.Denied("replicas cannot be an even number when etcd is stacked")

I think this message is the same in the regular webhook, if yes, let's update both

Updated the message at both places.

sbueringer · 2021-12-02T07:33:22Z

@dharmjit Looks good, can you please squash the commits?

Signed-off-by: Dharmjit Singh <sdharmjit@vmware.com>

sbueringer · 2021-12-03T16:12:30Z

/lgtm

vincepri · 2021-12-03T20:25:37Z

/approve

k8s-ci-robot · 2021-12-03T20:25:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 18, 2021

k8s-ci-robot requested review from JoelSpeed and sbueringer November 18, 2021 09:26

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 18, 2021

dharmjit marked this pull request as draft November 18, 2021 09:27

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 Nov 18, 2021

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 Nov 18, 2021

dharmjit mentioned this pull request Nov 18, 2021

Implement validation for the KCP scale sub-resource #5466

Closed

dharmjit force-pushed the kcp-scale-validatingwebhook branch from 946920e to f9325fd Compare November 18, 2021 09:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 18, 2021

sbueringer reviewed Nov 18, 2021

View reviewed changes

dharmjit force-pushed the kcp-scale-validatingwebhook branch from 0d14b00 to 51a49b9 Compare November 25, 2021 12:26

dharmjit marked this pull request as ready for review November 25, 2021 16:55

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 Nov 25, 2021

dharmjit force-pushed the kcp-scale-validatingwebhook branch from 51a49b9 to 38347d8 Compare November 26, 2021 07:19

sbueringer reviewed Nov 29, 2021

View reviewed changes

controlplane/kubeadm/webhooks/scale_test.go Outdated Show resolved Hide resolved

dharmjit force-pushed the kcp-scale-validatingwebhook branch from 38347d8 to 28138de Compare November 29, 2021 11:34

k8s-ci-robot assigned randomvariable and sbueringer Nov 29, 2021

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

vincepri reviewed Dec 1, 2021

View reviewed changes

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

Added C-R validating webhook for KCP scale subresource

035e915

Signed-off-by: Dharmjit Singh <sdharmjit@vmware.com>

dharmjit force-pushed the kcp-scale-validatingwebhook branch from c9b23d9 to 035e915 Compare December 2, 2021 07:40

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

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

k8s-ci-robot merged commit 137861a into kubernetes-sigs:main Dec 3, 2021

k8s-ci-robot added this to the v1.1 milestone Dec 3, 2021

dharmjit mentioned this pull request Jan 27, 2022

REQUEST: New membership for dharmjit kubernetes/org#3216

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Added C-R validating webhook for KCP scale subresource #5697

🌱 Added C-R validating webhook for KCP scale subresource #5697

dharmjit commented Nov 18, 2021

k8s-ci-robot commented Nov 18, 2021

sbueringer commented Nov 18, 2021 •

edited

linux-foundation-easycla bot commented Nov 18, 2021 •

edited

sbueringer left a comment •

edited

sbueringer Nov 18, 2021

randomvariable Nov 19, 2021

sbueringer Nov 18, 2021

sbueringer commented Nov 18, 2021

dharmjit commented Nov 25, 2021

sbueringer left a comment

sbueringer commented Nov 29, 2021

vincepri Dec 1, 2021

vincepri Dec 1, 2021

sbueringer Dec 1, 2021

dharmjit Dec 2, 2021

sbueringer commented Dec 2, 2021

sbueringer commented Dec 3, 2021

vincepri commented Dec 3, 2021

k8s-ci-robot commented Dec 3, 2021

	return admission.Denied("replicas cannot be an even number when using managed etcd")
	return admission.Denied("replicas cannot be an even number when etcd is stacked")

🌱 Added C-R validating webhook for KCP scale subresource #5697

🌱 Added C-R validating webhook for KCP scale subresource #5697

Conversation

dharmjit commented Nov 18, 2021

k8s-ci-robot commented Nov 18, 2021

sbueringer commented Nov 18, 2021 • edited

linux-foundation-easycla bot commented Nov 18, 2021 • edited

sbueringer left a comment • edited

Choose a reason for hiding this comment

sbueringer Nov 18, 2021

Choose a reason for hiding this comment

randomvariable Nov 19, 2021

Choose a reason for hiding this comment

sbueringer Nov 18, 2021

Choose a reason for hiding this comment

sbueringer commented Nov 18, 2021

dharmjit commented Nov 25, 2021

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Nov 29, 2021

vincepri Dec 1, 2021

Choose a reason for hiding this comment

vincepri Dec 1, 2021

Choose a reason for hiding this comment

sbueringer Dec 1, 2021

Choose a reason for hiding this comment

dharmjit Dec 2, 2021

Choose a reason for hiding this comment

sbueringer commented Dec 2, 2021

sbueringer commented Dec 3, 2021

vincepri commented Dec 3, 2021

k8s-ci-robot commented Dec 3, 2021

sbueringer commented Nov 18, 2021 •

edited

linux-foundation-easycla bot commented Nov 18, 2021 •

edited

sbueringer left a comment •

edited