✨ Add validation webhook for AWSMachine #1218

tahsinrahman · 2019-10-15T14:05:51Z

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 #

detiber · 2019-10-15T14:11:39Z

api/v1alpha3/awsmachine_webhook.go

+		Complete()
+}
+
+// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=vawsmachine.kb.io


It looks like we can override the odd vawsmachine.kb.io naming through this annotation... Could we name it validating.awsmachine.x-k8s.io here instead? @ncdc @vincepri thoughts?

Probably would be clearer. We'll have to remember to make this change for every webhook.

@ncdc we could add a quick script to validate the default naming isn't used for CI. Might be able to even use something as naive as: if git --no-pager grep kb.io; then echo "kb.io type prefix found"; exit 1; fi

I created a PR for renaming the validation webhook for AWSMachineTemplate here, and we can discuss more on that PR: #1219

detiber · 2019-10-18T19:34:19Z

api/v1alpha3/awsmachine_webhook.go

+		Complete()
+}
+
+// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=vawsmachine.kb.io


Suggested change

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=vawsmachine.kb.io

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.x-k8s.io

ncdc · 2019-11-07T16:19:00Z

api/v1alpha3/awsmachine_webhook.go

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


Suggested change

return errors.New("awsMachineSpec is immutable")

return field.Forbidden(field.NewPath("spec"), "cannot be modified")

I suspect we'll need to allow updates to at least a subset of the spec (controller setting awsmachine.Spec.ProviderID for example).

We could probably allow updates to the AdditionalTags and AdditionalSecurityGroups fields safely as well (since those should be updated on each reconcile).

It might also be helpful for people that are troubleshooting issues with initial instance creation to be able to modify fields like the AMI, ImageLookupOrg, InstanceType, IAMInstanceProfile, AvailabilityZone, Subnet, SSHKeyName, NetworkInterfaces, etc assuming the instance doesn't exist.

Would it make sense to allow all updates as long as we haven't created an ec2 instance yet? We might need to worry about race conditions such as:

Simultaneously:

Reconciler receives AWSMachine at ResourceVersion=1

User quickly gets in some updates, now ResourceVersion=2

Reconciler creates ec2 instance using data from ResourceVersion=1 (maybe it's slow today)

Reconciler sees updated AWSMachine at ResourceVersion=2

But it can't do anything at this point, so the data doesn't match what happened in ec2

@ncdc as long as we don't necessarily rely on something like the providerID being set on the awsmachine.Spec, and fallback to doing a describe for the instance we should be able to avoid race conditions.

@detiber I don't think that helps what I described above?

@ncdc our current implementation is also susceptible to this race condition you mentioned, right? so if we move the logic that's now deleted in this PR into the webhook validation code, it can also face this scenario?

@tahsinrahman you are correct. I guess this comes back to my question from https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/1218/files#r344289646: is it better to allow updates with the possibility of a race, or err on the side of caution and reject all updates?

I'm on the side of disallowing all updates for now and see how it affects user experience. But lets come to an agreement on what to do! :)

sgtm, seems like the general consensus is to disallow updates for now :)

I think we have consensus!

ncdc · 2019-11-07T16:20:15Z

1 small comment. @detiber @vincepri how does this look to you?

detiber · 2019-11-07T16:29:34Z

api/v1alpha3/awsmachine_webhook.go

+		Complete()
+}
+
+// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.x-k8s.io


Suggested change

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.x-k8s.io

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.cluster.x-k8s.io

detiber · 2019-11-07T16:35:56Z

api/v1alpha3/awsmachine_webhook.go

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


I suspect we'll need to allow updates to at least a subset of the spec (controller setting awsmachine.Spec.ProviderID for example).

We could probably allow updates to the AdditionalTags and AdditionalSecurityGroups fields safely as well (since those should be updated on each reconcile).

It might also be helpful for people that are troubleshooting issues with initial instance creation to be able to modify fields like the AMI, ImageLookupOrg, InstanceType, IAMInstanceProfile, AvailabilityZone, Subnet, SSHKeyName, NetworkInterfaces, etc assuming the instance doesn't exist.

chuckha

Should we be documenting anything on this PR? Such as what validations we are enforcing and how to set up the validation web hook? Or is everything opaque to the user and no documentation is required?

ncdc · 2019-11-13T15:12:07Z

@chuckha the examples already set up cert-manager and webhooks. However, ultimately we need more comprehensive documentation around webhook setup, including how to use something other than cert-manager. I'd like to add that to kubernetes-sigs/cluster-api#1757.

As far as documenting what validations we're enforcing, this will disallow all updates to AWSMachine.Spec... Although we have to address @detiber's comment:

I suspect we'll need to allow updates to at least a subset of the spec (controller setting awsmachine.Spec.ProviderID for example).

And potentially:

We could probably allow updates to the AdditionalTags and AdditionalSecurityGroups fields safely as well (since those should be updated on each reconcile).

I'm not sure where the best place is to document this. Any ideas?

detiber · 2019-11-13T15:14:30Z

Should we be documenting anything on this PR? Such as what validations we are enforcing and how to set up the validation web hook? Or is everything opaque to the user and no documentation is required?

We should likely add a doc similar to https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/providers/v1alpha2-to-v1alpha3.md and add an item discussing the more strict validation there.

I don't think we should have to inform users how to deploy the validation webhook, since it will be deployed as part of the published provider components yaml.

chuckha · 2019-11-13T15:37:27Z

Should we be documenting anything on this PR? Such as what validations we are enforcing and how to set up the validation web hook? Or is everything opaque to the user and no documentation is required?

We should likely add a doc similar to https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/book/src/providers/v1alpha2-to-v1alpha3.md and add an item discussing the more strict validation there.

I don't think we should have to inform users how to deploy the validation webhook, since it will be deployed as part of the published provider components yaml.

I think we could have it in that doc for the current upgrade. More informational changes to be aware of. Perhaps changes to your existing YAML you may need to make.

Then, I was thinking about future uses and where I'd go to find out validation rules. I'd probably look at either godoc or reference documentation (like the k8s reference docs). So godoc seems like a good place to put this from my perspective

tahsinrahman · 2019-11-14T15:43:56Z

Do I have to manually go through each fields in awsMachine.spec one by one? or is there any helper functions that'll allow me to check only providerID, AdditionalTags and AdditionalSecurityGroups is changed?

ncdc · 2019-11-14T15:48:18Z

Oh and we'd presumably want to allow annotations and labels to change too.

I think if you want an "easy" way, you could deep-copy both old & new, clear our the fields that we allow to be changed in both of the copies, and then compare with reflect.DeepEqual... maybe?

main.go

ncdc · 2019-11-22T18:27:51Z

api/v1alpha3/awsmachine_webhook.go

+	delete(newAWSMachineSpec, "additionalSecurityGroups")
+
+	if !reflect.DeepEqual(oldAWSMachineSpec, newAWSMachineSpec) {
+		return field.Forbidden(field.NewPath("spec"), "cannot be modified")


I just learned about a better way to return validation errors - please see https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation.html / xref kubernetes-sigs/cluster-api#1795 too.

@ncdc sorry for being late. I updated the pr, ptal :)

ncdc · 2019-12-02T15:29:06Z

api/v1alpha3/awsmachine_webhook.go

+func (r *AWSMachine) ValidateUpdate(old runtime.Object) error {
+	newAWSMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(r)
+	if err != nil {
+		return errors.Wrap(err, "failed to convert new AWSMachine to unstructured object")


Should we return apierrors.NewInvalid() here?

ncdc · 2019-12-02T15:29:10Z

api/v1alpha3/awsmachine_webhook.go

+	}
+	oldAWSMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(old)
+	if err != nil {
+		return errors.Wrap(err, "failed to convert old AWSMachine to unstructured object")


Should we return apierrors.NewInvalid() here?

ncdc · 2019-12-02T15:35:04Z

api/v1alpha3/awsmachine_webhook_test.go

+	tests := []struct {
+		name    string
+		old     *AWSMachine
+		new     *AWSMachine


nit: I usually try to avoid using new as a variable name, given that it's a go keyword. If you want to change the name, you could use something like updated instead.

detiber · 2019-12-02T15:54:34Z

/approve
leaving lgtm to @ncdc

k8s-ci-robot · 2019-12-02T15:54:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, tahsinrahman

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ncdc · 2019-12-03T18:52:43Z

@tahsinrahman if you'd like, we can merge as-is and I can open a small follow-up PR to address my remaining comments. Please let me know your preference. Thanks!

tahsinrahman · 2019-12-03T18:54:54Z

@tahsinrahman if you'd like, we can merge as-is and I can open a small follow-up PR to address my remaining comments. Please let me know your preference. Thanks!

oops, i somehow missed your review. let me update it in this pr

update: @ncdc updated!

ncdc · 2019-12-03T19:23:10Z

Thanks!
/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 15, 2019

k8s-ci-robot requested review from ashish-amarnath and rudoi October 15, 2019 14:05

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 15, 2019

detiber reviewed Oct 15, 2019

View reviewed changes

detiber reviewed Oct 18, 2019

View reviewed changes

tahsinrahman force-pushed the validate-awsmachine branch 2 times, most recently from ed244d3 to 3f6ef4e Compare October 19, 2019 06:05

ncdc self-assigned this Oct 21, 2019

ncdc added this to the v0.5.0 milestone Oct 21, 2019

ncdc reviewed Nov 7, 2019

View reviewed changes

detiber reviewed Nov 7, 2019

View reviewed changes

chuckha reviewed Nov 13, 2019

View reviewed changes

tahsinrahman force-pushed the validate-awsmachine branch from 3f6ef4e to 6c45d2d Compare November 14, 2019 15:43

wfernandes reviewed Nov 14, 2019

View reviewed changes

main.go Outdated Show resolved Hide resolved

tahsinrahman force-pushed the validate-awsmachine branch from 6c45d2d to dc50585 Compare November 16, 2019 13:19

ncdc reviewed Nov 22, 2019

View reviewed changes

tahsinrahman force-pushed the validate-awsmachine branch from dc50585 to ee472bf Compare November 28, 2019 06:40

ncdc reviewed Dec 2, 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 Dec 2, 2019

Add validation webhook for AWSMachine

af1e8a3

tahsinrahman force-pushed the validate-awsmachine branch from ee472bf to af1e8a3 Compare December 3, 2019 19:09

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

k8s-ci-robot merged commit 06ec59c into kubernetes-sigs:master Dec 3, 2019

tahsinrahman deleted the validate-awsmachine branch December 3, 2019 19:25

prksu mentioned this pull request Mar 9, 2021

Add admission webhook kubernetes-sigs/cluster-api-provider-digitalocean#241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add validation webhook for AWSMachine #1218

✨ Add validation webhook for AWSMachine #1218

tahsinrahman commented Oct 15, 2019

detiber Oct 15, 2019

ncdc Oct 15, 2019

detiber Oct 15, 2019

detiber Oct 15, 2019

detiber Oct 18, 2019

ncdc Nov 7, 2019

detiber Nov 7, 2019

ncdc Nov 7, 2019

detiber Nov 7, 2019

ncdc Nov 7, 2019

tahsinrahman Nov 10, 2019

ncdc Nov 11, 2019

tahsinrahman Nov 13, 2019

detiber Nov 13, 2019

ncdc Nov 13, 2019

ncdc commented Nov 7, 2019

detiber Nov 7, 2019

detiber Nov 7, 2019

chuckha left a comment

ncdc commented Nov 13, 2019

detiber commented Nov 13, 2019

chuckha commented Nov 13, 2019

tahsinrahman commented Nov 14, 2019

ncdc commented Nov 14, 2019

ncdc Nov 22, 2019

tahsinrahman Nov 28, 2019

ncdc Dec 2, 2019

ncdc Dec 2, 2019

ncdc Dec 2, 2019

detiber commented Dec 2, 2019

k8s-ci-robot commented Dec 2, 2019

ncdc commented Dec 3, 2019

tahsinrahman commented Dec 3, 2019 •

edited

ncdc commented Dec 3, 2019

	// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=vawsmachine.kb.io
	// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.x-k8s.io

	return errors.New("awsMachineSpec is immutable")
	return field.Forbidden(field.NewPath("spec"), "cannot be modified")

✨ Add validation webhook for AWSMachine #1218

✨ Add validation webhook for AWSMachine #1218

Conversation

tahsinrahman commented Oct 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

ncdc commented Nov 13, 2019

detiber commented Nov 13, 2019

chuckha commented Nov 13, 2019

tahsinrahman commented Nov 14, 2019

ncdc commented Nov 14, 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

detiber commented Dec 2, 2019

k8s-ci-robot commented Dec 2, 2019

ncdc commented Dec 3, 2019

tahsinrahman commented Dec 3, 2019 • edited

ncdc commented Dec 3, 2019

tahsinrahman commented Dec 3, 2019 •

edited