Closed alexzielenski closed 7 months ago
This stems from the required field not having the correct (non-pointer) type right? This tag's main use case would be for legacy omitempty
fields that should not have been a pointer? Do you see any new use cases for this tag?
This stems from the required field not having the correct (non-pointer) type right? This tag's main use case would be for legacy omitempty fields that should not have been a pointer? Do you see any new use cases for this tag?
I don't think in general we have guidance for API types that all required fields should be non-omitempty or non-pointer. A pointer historically has been the only way to check for if the field was supplied (so that the required error can be thrown in the first place) especially if empty
is a valid value.
I think API authors will set the type to whatever they'd like it to be, and should make able to make the schema reflect their desired validation logic without impacting the type system. I'm also not sure if we should even be looking at omitempty
, since its usage is pretty inconsistent. But that is a larger change.
A newer more recent field affected by this problem is ValidatingAdmissionPolicy's matchConstraints
: https://github.com/kubernetes/kubernetes/blob/460df86f2a6f5c8c92b957707d1088c9e79c4b50/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go#L173-L178. It is a required field that is a pointer but not listed as required in the schema.
I think API authors will set the type to whatever they'd like it to be, and should make able to make the schema reflect their desired validation logic without impacting the type system
This is true to an extent. However, the types do have an impact on what kind of validation would be available, especially around ptr vs struct fields. eg: nullable. I do see your point of checking for existence and throwing the error though.
Adding a +required tag seems fine for fixing that restriction, although this feels more like a bandaid solution.
However, the types do have an impact on what kind of validation would be available, especially around ptr vs struct fields. eg: nullable.
Struct types which override (Un)MashalJSON
to handle null
are also nullable, even if they aren't pointers. This is another example of why I don't think we should put too much weight on the Go type system.
Adding a +required tag seems fine for fixing that restriction, although this feels more like a bandaid solution.
I had also considered +optional=false
, but nothing seems perfect. Open to other ideas to solve this problem
Ah right 😆, yeah I agree it's hard to come up with a perfect solution without revamping a large part of the validation system. Overall +1 on this approach
+1 to +required
. The problem here seems to be that we infer optionality from information in go structs, but sometimes we need to be able to override what is inferred. This seems reasonable. Using +required
seems right because it aligns with what we want to to communicate to the user. I'm supportive of updating any +required
usages to match this approach. That seems like a win for users and for us.
That makes sense, the tags should take precedent over go structs. Don't forget to update the k8s docs for usage and best practices. LGTM once TODO comment on L329 is removed.
/lgtm /approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: alexzielenski, Jefftree
The full list of commands accepted by this bot can be found here.
The pull request process is described here
While annotating native type schemas using our marker comments, I am finding that there are a few native types that have
omitempty
but are treated asrequired
.One example is
VolumeNodeAffinity
which has a pointer fieldRequired
that is not included in the list of required fields due to itsomitempty
tag:https://github.com/kubernetes/kubernetes/blob/2ce04fc04bf2cbbbacf2f184fd9ebd4e99d65430/staging/src/k8s.io/api/core/v1/types.go#L383-L387
https://github.com/kubernetes/kubernetes/blob/7972f0309ce8bad3292f3291718361367b2e58fe/pkg/apis/core/validation/validation.go#L7308-L7312
/cc @Jefftree wdyt. I also thought of
+optional=false
since that has benefit of not adding another tag to represent the same concept; but its kinda weird