Add admission webhook #241
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
approved
|
/retest |
|
i will take a look during the week, but looks great! great job! /assign @timoreimann |
|
/retest |
| return apierrors.NewBadRequest(fmt.Sprintf("expected an DOCluster but got a %T", old)) | ||
| } | ||
|
|
||
| if r.Spec.Region != oldDOCluster.Spec.Region { |
There was a problem hiding this comment.
The one case I can think of where changing the region might be helpful is when a wrong region was specified in the first place and you were trying to fix a reconcile that fails early on. Might not be worth considering though.
There was a problem hiding this comment.
make sense. but I prefer to make the region immutable in some cases the user trying to change region in an existing cluster and then the next machine will be created in a different region from the other old machines.
how about make validation in the ValidateCreate that region must be valid do region? so it will reduce the failure because of the invalid region.
There was a problem hiding this comment.
I like the idea to check if the region is a valid one. We should then use the regions endpoint to list all valid regions.
(Doesn't have to implemented in this PR.)
| newDOMachineSpec := newDOMachine["spec"].(map[string]interface{}) | ||
| oldDOMachineSpec := oldDOMachine["spec"].(map[string]interface{}) | ||
|
|
||
| // allow changes to providerID |
There was a problem hiding this comment.
AFAICS, the provider ID is set from the (read-only) droplet ID. What's the use case for it to be allowed to change? 🤔
There was a problem hiding this comment.
afaik, it required for the controller to set the providerID kubernetes-sigs/cluster-api-provider-aws#1218 (comment) although, I also think if we can reject updating providerID only if it already set.
but can we leave it as is? and keeping an eye on the other providers on how they handle this. since it's not clearly documented yet what validations should be enforcing kubernetes-sigs/cluster-api-provider-aws#1218 (comment)
There was a problem hiding this comment.
Ah you're right, I missed the fact that the provider ID is set by the CAPDO controller itself which constitutes an update.
Disallowing updates once the provider ID is set sounds reasonable. Happy to keep things the way you propose them in your PR for now though and see what consensus is made with regards to webhook validations.
api/v1alpha3/docluster_webhook.go
Outdated
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "region"), r.Spec.Region, "field is immutable")) | ||
| } | ||
|
|
||
| if allErrs != nil { |
There was a problem hiding this comment.
Should we also prevent changes to the controlPlaneEndpoint field?
It consists of the IP address of the LB which is immutable once set, and the LB port which I don't think we allow updating right now?
(I did wonder at some point why those were spec fields in the first place as opposed to status ones, but maybe I'm missing some context here...)
There was a problem hiding this comment.
+1
(I did wonder at some point why those were spec fields in the first place as opposed to status ones, but maybe I'm missing some context here...)
It's required by capi to identify the endpoint used to connect to the target’s cluster apiserver.
https://cluster-api.sigs.k8s.io/developer/architecture/controllers/cluster.html#infrastructure-provider
There was a problem hiding this comment.
Right, what why is the endppint part of the spec? AFAIU, CAPDO does not expect the user/operator to set or modify the field -- it's always the controller that does it. In that sense, a status field seems better?
(Definitely a side discussion since this isn't something we can change without CAPI approval.)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prksu, timoreimann 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Add support for adission webhook
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 #198
Release note: