🌱 KCP: improve validation webhooks #6129

sbueringer · 2022-02-15T08:32:04Z

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
While investigating #6113 I found out that our KCP validation has a few issues:

field paths in KubeadmConfigSpec.Validate weren't correct in all cases (prefix)
semantic version validation of CoreDNS image tag was only done on update, not on create
errors in semantic version parsing weren't propagated in some cases
KubeadmConfigSpec was not validated on KubeadmControlPlaneTemplate create

Overall I think that should make especially ClusterClass/KubeadmControlPlaneTemplate safer to use as the errors are already returned on creation of KubeadmControlPlaneTemplate. Not during topology reconcile when KubeadmControlPlane cannot be created.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #6113

Signed-off-by: Stefan Büringer [email protected]

sbueringer · 2022-02-15T08:32:13Z

/cc @fabriziopandini

killianmuldoon

Overall looks good, but looks like it's a breaking change. We probably want to move these webhooks to an internal package like the others, right? Do you want to tackle that in this PR? Or we could revert the path change for this PR and include it in a change that makes the webhook implementation internal.

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go

sbueringer · 2022-02-16T16:31:57Z

Overall looks good, but looks like it's a breaking change. We probably want to move these webhooks to an internal package like the others, right? Do you want to tackle that in this PR? Or we could revert the path change for this PR and include it in a change that makes the webhook implementation internal.

I'm not sure if we should apply the same stability guarantees to the Validate func as to our API usually. This func is not really meant to be used externally. It is just exported because KCP has to use it too.

I'm not sure if it's worth moving the webhook out of the API package as that is a breaking change itself (even though we could do that with deprecation). I think for now I would prefer staying with our policy of only moving the webhooks to a separate package if we are using a client.

Overall I think this breaking change should be acceptable as it improves error messages, probably nobody uses this func apart from us and it's very easy to adopt to this breaking change if folks upgrade to v1.2.0.

killianmuldoon · 2022-02-16T16:33:38Z

Would it be better to move them internally just to avoid someone asking the next time there's a PR. Seems like it could be a good help wanted issue. 😝

sbueringer · 2022-02-16T16:34:52Z

Would it be better to move them internally just to avoid someone asking the next time there's a PR. Seems like it could be a good help wanted issue. 😝

I'm not sure. I think it has more downsides for us to move it elsewhere as it makes it just more complicated :)

But let's see what others think.

killianmuldoon · 2022-02-16T16:40:52Z

I'm not sure. I think it has more downsides for us to move it elsewhere as it makes it just more complicated :)

Right, but given that's the decision we made with other webhooks and controllers what's the difference that makes it unsuitable here?

sbueringer · 2022-02-16T16:55:44Z

I'm not sure. I think it has more downsides for us to move it elsewhere as it makes it just more complicated :)

Right, but given that's the decision we made with other webhooks and controllers what's the difference that makes it unsuitable here?

In my opinion the only policy we have/implemented right now is that we only did it for webhooks which require additional dependencies because they are using a client and other things. I think at the moment this only applies to the Cluster and ClusterClass webhooks.

Otherwise the question becomes, why don't we move all webhooks out of the API package. I think that something we can discuss (ideally on a separate issue). I think for now we kept them there so they are closer to the types they are validating/defaulting.

(btw, I'm not really against moving all webhooks. I'm just not sure if I see the benefit yet and I would want to have a discussion / consensus about that before opening help wanted issues)

vincepri · 2022-02-17T04:49:06Z

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go

@@ -63,7 +63,7 @@ func (c *KubeadmConfig) ValidateDelete() error {
 }

 func (c *KubeadmConfigSpec) validate(name string) error {
-	allErrs := c.Validate()
+	allErrs := c.Validate(field.NewPath("spec"))


Is passing the spec field inside for code or is it solving an actual problem? While going through the changes, the older version with e.g. "spec" "x" "y" "z" is a bit more clear than the current proposed changes

This solves the problem that we are reporting wrong field paths in our field errors in some webhooks.

We have different places where KubeadmConfigSpec are used, e.g. in KCP. But today all our webhooks report that the error is in .spec... without considering that it's actually e.g. .spec.kubeadmConfigSpec...

Furthermore (in my opinion) it's a best practice to build up those field paths incrementally, so each function only has to append new relative paths and we don't have to look up and down complex call hierarchies to figure out if the resulting field error has a correct field path. We are e.g. using that in our ClusterClass validation quite extensively (and in my opinion successfully :) ).

k8s-ci-robot · 2022-02-17T10:21:27Z

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`7a02662`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sbueringer · 2022-02-25T14:47:34Z

@vincepri PTAL, if you have some time, thank you! :)

vincepri

/approve

vincepri · 2022-03-10T19:13:08Z

/lgtm

k8s-ci-robot · 2022-03-10T19:13:36Z

[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

KCP: improve validation webhooks

2f4474a

Signed-off-by: Stefan Büringer [email protected]

k8s-ci-robot requested review from fabriziopandini, JoelSpeed and stmcginnis February 15, 2022 08:32

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 15, 2022

sbueringer mentioned this pull request Feb 15, 2022

Topology controller keeps rolling out InfrastructureMachinteTemplates when ClusterClass is supplied with Array typed patches #6113

Closed

killianmuldoon suggested changes Feb 16, 2022

View reviewed changes

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go Show resolved Hide resolved

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go Show resolved Hide resolved

vincepri reviewed Feb 17, 2022

View reviewed changes

fix review finding

7a02662

vincepri approved these changes Mar 10, 2022

View reviewed changes

k8s-ci-robot assigned vincepri Mar 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2022

k8s-ci-robot merged commit ffc9943 into kubernetes-sigs:main Mar 10, 2022

k8s-ci-robot added this to the v1.2 milestone Mar 10, 2022

sbueringer deleted the pr-improve-kcp-validation branch March 10, 2022 19:32

Jont828 mentioned this pull request Mar 18, 2022

Improve consistency of errors returned from core webhooks #6249

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 KCP: improve validation webhooks #6129

🌱 KCP: improve validation webhooks #6129

sbueringer commented Feb 15, 2022 •

edited

Loading

sbueringer commented Feb 15, 2022

killianmuldoon left a comment

sbueringer commented Feb 16, 2022 •

edited

Loading

killianmuldoon commented Feb 16, 2022

sbueringer commented Feb 16, 2022

killianmuldoon commented Feb 16, 2022

sbueringer commented Feb 16, 2022 •

edited

Loading

vincepri Feb 17, 2022

sbueringer Feb 17, 2022

k8s-ci-robot commented Feb 17, 2022

sbueringer commented Feb 25, 2022

vincepri left a comment

vincepri commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

🌱 KCP: improve validation webhooks #6129

🌱 KCP: improve validation webhooks #6129

Conversation

sbueringer commented Feb 15, 2022 • edited Loading

sbueringer commented Feb 15, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer commented Feb 16, 2022 • edited Loading

killianmuldoon commented Feb 16, 2022

sbueringer commented Feb 16, 2022

killianmuldoon commented Feb 16, 2022

sbueringer commented Feb 16, 2022 • edited Loading

vincepri Feb 17, 2022

Choose a reason for hiding this comment

sbueringer Feb 17, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 17, 2022

sbueringer commented Feb 25, 2022

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Mar 10, 2022

k8s-ci-robot commented Mar 10, 2022

sbueringer commented Feb 15, 2022 •

edited

Loading

sbueringer commented Feb 16, 2022 •

edited

Loading

sbueringer commented Feb 16, 2022 •

edited

Loading