Validate fields of OpenstackDataPlaneServiceCert in OpenStackDataPlaneServiceSpec #852
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpodivin 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 |
| // +kubebuilder:validation:Optional | ||
| KeyUsages []certmgrv1.KeyUsage `json:"keyUsages,omitempty" yaml:"keyUsages,omitempty"` | ||
| // +kubebuilder:default={"key encipherment","digital signature","server auth"} | ||
| KeyUsages []certmgrv1.KeyUsage `json:"keyUsages" yaml:"keyUsages"` |
There was a problem hiding this comment.
Making the default explicit is good. I thought though, that when you mentioned this tightening of the spec would include turning the Contents field into an enum - with "dnsnames" and "ips" as the two possible options. That would result in validation errors in the same way that KeyUsages currently does.
There was a problem hiding this comment.
Contents is list, not a string, enum wouldn't work there because you need list a approved values. Hence the ValidateContents method.
There was a problem hiding this comment.
Yes but we can -- and that what I thought you wanted to do -- redefine Contents as a list of enums - just like keyusages is a list of certmgrv1.KeyUsage. We can define a new enum of CertificateContents for example.
There was a problem hiding this comment.
That isn't really the point. Kubebuilder enum [0] should be used in cases when field must have only one of the values from approved a list. Not when the field is supposed to be a list of approved values.
Enum validation just doesn't fit our use case here, and we shouldn't try to change the API just to make it fit.
Putting the validation in a function is, imho, a cleaner approach.
[0] https://book.kubebuilder.io/reference/markers/crd-validation.html
There was a problem hiding this comment.
Maybe enum wasn't the right thing here. My point is that we expect contents to be one of a set of approved values. KeyUsages (https://github.com/cert-manager/cert-manager/blob/eb9f8880fdf9a02c2f85e73cc08617c350717378/pkg/api/util/usages.go#L26) is a similar thing, and we can use an equivalent structure to make that explicit - and thereby enforce it in the same way that KeyUsages are enforced.
| r.KeyUsages, | ||
| fmt.Sprintf("error validating contents of TLSCert, %s, only valid contents are %v ", val, allowedContents))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Um -- this validation function is all confused. You are defining the allowed contents for "Contents" and checking that - while putting up an error message associated with KeyUsages. That said - if we changed Contents to be an enum - with the appropriate default - would this whole function be necessary at all?
There was a problem hiding this comment.
That's because it started is function for keyusages. I wanted to see the test results, before rewriting the comment's, logs etc.
jpodivin
changed the title
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/c084a42e51dc48049e85622f9d5dd6f0 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 08m 30s |
…eServiceSpec Set default KeyUsages to "key encipherment","digital signature","server auth". Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
No description provided.