✨ Add validating webhook for AWSMachineTemplate #1116

tahsinrahman · 2019-09-10T19:35:35Z

What this PR does / why we need it:

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

Dockerfile

api/v1alpha2/awsmachinetemplate_webhook.go

api/v1alpha2/zz_generated.deepcopy.go

config/certmanager/certificate.yaml

config/default/kustomization.yaml

config/webhook/kustomizeconfig.yaml

detiber · 2019-09-11T14:08:45Z

api/v1alpha2/awsmachinetemplate_webhook.go

+}
+
+// validateUpdate checks that no immutable fields have been updated
+func validateUpdate(spec *AWSMachineSpec, oldSpec *AWSMachineSpec) error {


Are there any fields that we want to be mutable in the template types? @ncdc @vincepri I thought we had wanted to avoid allowing mutability of the template types and instead wanted to require users to update the reference in the MachineDeployment/MachineSet?

If we do want to force full immutability, then we can likely create a generic webhook in CAPI that blocks any update, and just register the webhook for the AWSMachineTemplate type here in CAPA.

We can allow updates to metadata, but probably all other updates should be rejected. I think we could probably write some generic validation helpers in CAPI, but afaik we still need to do the full webhook code generation & registration for each type.

According to the docs, we can configure which resources are subject to which admission webhooks at runtime: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#configure-admission-webhooks-on-the-fly

Oh, I didn't realize you were thinking about doing it that way. I was thinking of a kubebuilder-ism with its way of registering webhooks.

We should avoid naming changes to resources given that they're references elsewhere, unless the OwnerReferences get updated automagically?

As long as it's documented, I think that should be fine. Technically, the same applies for today's default RBAC permissions in CAPI, where we assume bootstrap.<> and infrastructure.<> as default

@ncdc agreed that clusteradm would be the best place long term to handle this

@vincepri @ncdc the validating webhook requests are initiated by the API Server, I'm not sure there is a way to use API discovery to register types to watch at runtime as we do with the controllers.

@detiber that's right. So the only way to handle this is with the details in the ValidatingWebhookConfiguration.

What do you all think about putting the generic *Template validation as a helper function in cluster-api, but at least for now, having each provider implement their own webhooks for their own templates (using the generic function from cluster-api)?

@ncdc I think that's fine as long as we track improving that experience later when clusteradm is available.

liztio · 2019-10-07T18:25:19Z

api/v1alpha2/awsmachinetemplate_webhook.go

+	// spec.Subnet is a *AWSResourceReference and could technically be
+	// a *string, ARN or Filter. However, elsewhere in the code it is only used
+	// as a *string, so do the same here.
+	if aws.StringValue(spec.Subnet.ID) != aws.StringValue(oldSpec.Subnet.ID) {


We should probably check that spec.Subnet isn't nil

liztio · 2019-10-07T18:26:27Z

config/webhook/manifests.yaml

+  name: validating-webhook-configuration
+webhooks:
+- clientConfig:
+    caBundle: Cg==


What does this do?

This is generated by kubebuilder. I guess it's a required field. Maybe cert-manager or something else updates it? It decodes to \n.

detiber

A few nits, but otherwise lgtm

main.go

config/webhook/service.yaml

vincepri

/approve

k8s-ci-robot · 2019-10-08T16:37:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tahsinrahman, 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

tahsinrahman · 2019-10-08T17:09:02Z

/hold

chuckha · 2019-10-08T17:36:17Z

controllers/awsmachine_controller.go

@@ -282,13 +280,6 @@ func (r *AWSMachineReconciler) reconcileNormal(ctx context.Context, machineScope
 		return reconcile.Result{}, nil
 	}

-	// TODO(ncdc): move this validation logic into a validating webhook


I don't see this logic moved anywhere, am i misunderstanding this Webhook?

It was in an earlier version of this PR. Not sure why it got deleted?

as entire spec is immutable, instead of checking every field of spec, i did this

if !reflect.DeepEqual(newAWSMachineTemplate.Spec, oldAWSMachineTemplate.Spec) { return errors.New("awsMachineTemplateSpec is immutable") }

This validation was being used by the AWSMachine controller, where the current webhook is targeting AWSMachineTemplate.

got it!
that means, we need to deploy another validating webhook for AWSMachine types, to move these logic, right?

correct, thanks :)

@tahsinrahman are you going to add the AWSMachine validation webhook in this PR?

Sure, I can add it here, or if it becomes too big for review, i can do it in separate pr, which one works best for you? 😄

Either way is fine. If you keep them separate, you can retitle this PR to "Add validating webhook for AWSMachineTemplate"?

config/crd/kustomization.yaml

vincepri · 2019-10-10T14:38:06Z

config/webhook/manifests.yaml

+      namespace: system
+      path: /validate-infrastructure-cluster-x-k8s-io-v1alpha2-awsmachinetemplate
+  failurePolicy: Fail
+  name: vawsmachinetemplate.kb.io


How was this name generated?

kubebuilder does it this way

Got it, was mostly curious as it reads funny

detiber · 2019-10-10T14:45:43Z

/lgtm

vincepri · 2019-10-10T14:46:14Z

/hold

vincepri · 2019-10-10T14:46:27Z

Do we need to cut release-0.4 first?

ncdc · 2019-10-10T14:47:04Z

Do we need to cut release-0.4 first?

No, we can branch from any commit at any time.

tahsinrahman · 2019-10-10T14:47:29Z

Do we want to merge this now? It'll break clusterctl

vincepri · 2019-10-10T14:52:21Z

Let's hold on to this until we have all the others open PR merged in 0.4 and we cut the release branch

ncdc · 2019-10-10T14:52:52Z

We also don't have to rush this in. Let's think more about the "clusterctl on master" impact.

detiber · 2019-10-10T14:53:10Z

/lgtm cancel

ncdc · 2019-10-11T21:23:53Z

I'm thinking this will need to move into the v1alpha3 api package (#1202), right?

tahsinrahman · 2019-10-15T12:42:20Z

I updated the pr with v1alpha3 package!

detiber · 2019-10-15T13:27:29Z

/lgtm
I believe the hold can be lifted now that we have the release-0.4 branch as well. @ncdc?

ncdc · 2019-10-15T13:28:53Z

Yes, master is open for v1alpha3.
/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 10, 2019

k8s-ci-robot requested review from ncdc and sethp-nr September 10, 2019 19:35

tahsinrahman commented Sep 10, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

ncdc reviewed Sep 10, 2019

View reviewed changes

tahsinrahman force-pushed the webhook branch 3 times, most recently from 7461e79 to 690d2ec Compare September 11, 2019 12:07

detiber reviewed Sep 11, 2019

View reviewed changes

joonas mentioned this pull request Sep 11, 2019

Make the webhook server port configurable via flag #1120

Closed

ncdc added this to the v0.4.x milestone Sep 23, 2019

vincepri self-assigned this Sep 23, 2019

liztio suggested changes Oct 7, 2019

View reviewed changes

liztio mentioned this pull request Oct 7, 2019

Setting a natGatewayId on a subnet causes segfaults #1175

Closed

tahsinrahman force-pushed the webhook branch from 690d2ec to 4b7b40e Compare October 8, 2019 15:08

detiber reviewed Oct 8, 2019

View reviewed changes

main.go Outdated Show resolved Hide resolved

config/webhook/service.yaml Outdated Show resolved Hide resolved

config/webhook/service.yaml Show resolved Hide resolved

vincepri approved these changes Oct 8, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2019

chuckha reviewed Oct 8, 2019

View reviewed changes

tahsinrahman force-pushed the webhook branch from 4b7b40e to d4e8093 Compare October 9, 2019 12:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2019

tahsinrahman force-pushed the webhook branch 2 times, most recently from f3bb900 to 35a1361 Compare October 9, 2019 12:32

ncdc reviewed Oct 9, 2019

View reviewed changes

config/crd/kustomization.yaml Show resolved Hide resolved

tahsinrahman changed the title ~~✨ Add validating webhooks~~ ✨ Add validating webhook for AWSMachineTemplate Oct 10, 2019

vincepri reviewed Oct 10, 2019

View reviewed changes

k8s-ci-robot assigned detiber Oct 10, 2019

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

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2019

ncdc modified the milestones: v0.4.x, v0.5.0 Oct 10, 2019

tahsinrahman added 2 commits October 15, 2019 15:16

Update config/ to match with kubebuilder 2.0.1

9c2983a

Add validating webhook for AWSMachineTemplate

24430fd

tahsinrahman force-pushed the webhook branch from c595029 to 24430fd Compare October 15, 2019 12:35

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2019

k8s-ci-robot merged commit ccf11ac into kubernetes-sigs:master Oct 15, 2019

tahsinrahman deleted the webhook branch October 15, 2019 13:30

✨ Add validating webhook for AWSMachineTemplate #1116

✨ Add validating webhook for AWSMachineTemplate #1116

Conversation

tahsinrahman commented Sep 10, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detiber left a comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 8, 2019

tahsinrahman commented Oct 8, 2019

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

Choose a reason for hiding this comment

detiber commented Oct 10, 2019

vincepri commented Oct 10, 2019

vincepri commented Oct 10, 2019

ncdc commented Oct 10, 2019

tahsinrahman commented Oct 10, 2019

vincepri commented Oct 10, 2019

ncdc commented Oct 10, 2019

detiber commented Oct 10, 2019

ncdc commented Oct 11, 2019

tahsinrahman commented Oct 15, 2019

detiber commented Oct 15, 2019

ncdc commented Oct 15, 2019