kubernetes
Update guidance on required field serialization
@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
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.
/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
(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)
@JoelSpeed thanks for this PR, this clarifications are really helpful!
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: dims, JoelSpeed, liggitt
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~contributors/devel/sig-architecture/OWNERS~~ [dims]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
