Update guidance on required field serialization

[JoelSpeed]

@liggitt and I were discussing serialization of required fields in a thread and realised that there are valid cases where a required field should be a pointer and should be omitempty.

Take the example:

// +required
// +k8s:minimum=0
Foo *int32 `json:"foo,omitempty"`

Here, the value 0 is a valid user choice.

For a structured Go client, if the field were not a pointer, and not omitempty per the existing guidance, then the structured client is not actually required to set a value for the field, as the json marshalling process will marshal foo: 0 and set a choice for them.

This defeats the purpose of the field being required for structured clients, and creates a discrepancy in behaviour between structured and unstructured clients.

Would appreciate thoughts of other API reviewers on this
CC @thockin @deads2k

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JoelSpeed
Once this PR has been reviewed and has the lgtm label, please assign dims for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• contributors/devel/sig-architecture/OWNERS

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

[JoelSpeed]

Thinking through different types we might see, and the implications of this suggestion

Nullable

Anything that needs to be a pointer to allow the zero value to be set, that also has +nullable, does not need to be omitempty. +nullable and omitempty are incompatible.

String

Any required string should have a minimum length. If that minimum length is 0, or unset, then the string should be a pointer+omitempty to allow the empty string to be an explicit choice.

Except when the required string is not actually a string.

• Enums would not need to be pointer+omitempty unless the empty string is a valid value in the enum
• Time: Parsing the empty string returns an error when unmarshalling

Integer/Float

As per example in the decsription, if the 0 is in the valid range of values (based on minimum/maximum markers) then pointer and omitempty should be used to allow 0 to be a valid choice.

Bool

Any required bool should be a pointer+omitempty unless validation only allows the value true. (I have no idea if we have that in any APIs?)

Lists

A required list should have a minimum length validation (+k8s:minItems). If that minimum length is 0, or unset, then the list should be a pointer and omitempty (Foo *[]string json:"foo,omitempty"`).

If the list wasn't a pointer, but had omitempty, the structured client would not be able to explicitly set an empty list.

Maps

The same as lists? Not sure what the appropriate marker is for the minimum size of a map (it's +kubebuilder:validation:MinProperties in CRDs)

Objects

For the zero value to be valid, an object should have no required fields, and have no validation that requires a minimum number of fields be set (e.g. +kubeubilder:validationMinProperties, not sure on equivalent +k8s marker). In these cases then the object should be pointer+omitempty to be able to explicitly set the zero value {}.

If the object does have a required field, but that required field would marshal the zero value and that would be accepted by validation, then the zero value of the struct would also be considered valid. I think this would happen generally on a poorly validated set of fields. E.g. a string that doesn't have an explicit minimum length, but has not been added based on the rules outlined here.

The object "bar": {"foo": ""} might be the minimally serialized object, and this would pass the bar and foo validations for being required. But "bar": {} would not pass the validation of foo required, and as such, the object does not need to be a pointer in this case.

[sbueringer]

I don't have enough context, but could we simplify this guidance to required fields are only marked via the +required marker (given that's how it should be)

Or at least recommend it

[thockin]

+1 - explicit FTW. If declarative validation flies, I want explicit.

[JoelSpeed]

I also believe in being explicit, but I know some tooling is implicitly assuming this, and we have a long history of that implicitness.

If we say they should be explicitly marked, should we also add an N.B. that explains that historically this has been implicitly detected?

[sbueringer]

If we say they should be explicitly marked, should we also add an N.B. that explains that historically this has been implicitly detected?

I was thinking something like this, yes. Recommend to be explicit and then mention in some way how some tools behave (probably these tools (thinking about controller-gen as an example) also won't change to avoid breaking changes)

[JoelSpeed]

/hold

I've been through this with @liggitt and we need to make some more updates to the guidance. Will look at creating a decision tree to help folks understand whether or not they should be setting fields as pointers/omitempty etc

[liggitt]

(notes from our discussion in https://docs.google.com/document/d/1k3XxUTEjcQifSXjoy5CcQY9QttdhQ-EG-R3vrKq6SUc/edit?pli=1&resourcekey=0-5xMYchKvoisABPw9tsR3nA&tab=t.0#bookmark=id.ks1ziis4vgzz - shared with https://groups.google.com/a/kubernetes.io/g/dev, can join that to get read access)

[thockin]

"may" or "must" ?

[JoelSpeed]

I think this is must and likely paired, should remove likely

[fabriziopandini]

@JoelSpeed thanks for this PR, this clarifications are really helpful!

[fabriziopandini]

q: should this (openAPI documentation) change now that we can have omitempty on required fields?

[JoelSpeed]

We will need to make sure that it respects +required tags. If the field doesn't have +required and has omitempty, then I think there are many tools that still assume optional and we shouldn't change that.

In the long run, we need to make sure all fields are marked as +optional xor +required explicitly