feature: add explicit +required

[alexzielenski]

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 as required.

One example is VolumeNodeAffinity which has a pointer field Required that is not included in the list of required fields due to its omitempty 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

[Jefftree]

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?

[alexzielenski]

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.

https://github.com/kubernetes/kubernetes/blob/f17468ff060e118a22a69c876f637b33871d657c/pkg/apis/admissionregistration/validation/validation.go#L744-L752

[Jefftree]

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.

[alexzielenski]

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

[Jefftree]

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

[jpbetz]

+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.

[Jefftree]

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.

[Jefftree]

/lgtm
/approve

[k8s-ci-robot]

[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

Needs approval from an approver in each of these files:

• OWNERS [Jefftree,alexzielenski]

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