Replacing webhook validation with CRD validation for AddressType #1204
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
approved
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern=`^([a-zA-Z0-9])+$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])\/[a-zA-Z0-9]+$` | ||
| // +kubebuilder:validation:Pattern=`^Hostname|IPAddress|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$` |
There was a problem hiding this comment.
Note: This is copied from the ControllerName regex with Hostname|IPAddress| added as a prefix. https://regex101.com/r/k892is/1 for reference.
robscott
force-pushed
the
regex-address-type-validation
branch
from
e8a6cba to
c9b2d68
Compare
| @@ -480,6 +480,20 @@ type AnnotationKey string | |||
| type AnnotationValue string | |||
|
|
|||
| // AddressType defines how a network address is represented as a text string. | |||
| // This may take two possible forms: | |||
| // | |||
| // * A predefined CamelCase string identifier (currently limited to `IPAddress` or `Hostname`) | |||
There was a problem hiding this comment.
Just because these types of references with point-in-time statements of facts about other symbols in the code run the risk of bit-rotting and being missed when other things change, would it be worth making this change?
| // * A predefined CamelCase string identifier (currently limited to `IPAddress` or `Hostname`) | |
| // * A predefined CamelCase string identifier (e.g. `IPAddress`, `Hostname`) |
There was a problem hiding this comment.
I definitely agree that it's a risk that they get out of sync, but I prefer this to the alternative of requiring users to parse a difficult to understand regex to understand which values are currently valid. We do need to be very careful with all reviews around enums like this that when we change the set of allowed values, we update that everywhere.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott, shaneutt 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 |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
In response to feedback on #1086, this moves validation from the webhook into the CRD.
Which issue(s) this PR fixes:
Fixes #1202
Does this PR introduce a user-facing change?: