Add OpenAPI required tag for all required types

[killianmuldoon]

In our API types we currently conceive 'Required' as meaning not optional.

Our optional types have the following definition:

An optional field MUST be marked with +optional and include an omitempty JSON tag.
Fields SHOULD be pointers if there is a good reason for it, for example:
    the nil and the zero values (by Go standards) have semantic differences.
        Note: This doesn't apply to map or slice types as they are assignable to nil.
    the field is of a struct type, contains only fields with omitempty and you want to prevent that it shows up as an empty object after marshalling (e.g. kubectl get)

Currently with Kubernetes this is enough as fields not marked optional are required in CRDs.

We could be explicit with Optional vs Required in our API definitions by explicitly setting Required on the OpenAPI specs. This would make our API more explicit and holistic on the OpenAPI level.

It could add safety:

You can also specify // +kubebuilder:validation:Optional on a package level, which then switches this defaulting round so that fields are optional if no tag is specified. If someone sets that on your package, it can be quite easy to miss, so it's better to be explicit about what you want on each field

But complicate our UX:

Also the fact that any conceptually required field that you want to default needs to be declared optional for client side validation to not complain e.g kubectl, is suboptimal and confusing.

See the original discussion at: #6383 (comment)

@fabriziopandini @enxebre @JoelSpeed @sbueringer

/kind feature
/area api

[killianmuldoon]

A quick test of what adding // +kubebuilder:validation:Optional on a package level surprisingly (for me at least) shows that the change would pass all of our unit integration and e2e tests.

It fails the apidiff test though so the risk of this happening accidentally is extremely low IMO.

[fabriziopandini]

I don't have strong opinions on using this tag, but IMO it seems to be something we can defer to the next API bump.

From a quick test adding +kubebuilder:validation:Required on a required field doesn't affect the generated CRD (the field is already treated as required no matter of the marker), the same when adding +kubebuilder:validation:Optional on a not required field defined according to our current standard.

TL;DR; following our current standard we are slightly less explicit in the code, but we are already ok in generated CRDs.

[JoelSpeed]

My vote, to avoid potential surprises later is that we should be explicit on all fields and have either +kubebuilder:validation:Required or +optional|+kubebuilder:validation:Optional.

Being explicit means should mean that we prevent surprises later should anything change in the logic of the generation (unlikely) or if a user accidentally, or deliberately, adds a package level optional tag (which would change the defaults).
It also has the added benefit of being easier to read as the user doesn't have to know the implicit rules of, required being something that is not optional.

[sbueringer]

I think it's hard to accidentally add the optional marker on the package level and miss it in reviews as a lot of CRDs change / or the verify job fails.

But I think it would be still nice to be explicit so that on every single field there is an explicit decision to make a field optional or required.

My only concern is if it's sustainable to enforce via manual reviews that every single field has an optional or required marker. It's easier to ensure nobody adds the package-level optional (as the impact is really obvious).

So overall I'm in favor of making it explicit, but it would be nice to add a linter as follow-up to enforce this automatically on all fields.

P.S. nit: I think we're talking about markers, tags in Go are something else.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[killianmuldoon]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/lifecycle frozen
/triage accepted

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[sbueringer]

/triage accepted

[fabriziopandini]

/priority important-longterm