Update guidance on required field serialization #8486

JoelSpeed · 2025-06-12T09:51:52Z

@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 · 2025-06-12T09:52:01Z

[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 · 2025-06-12T10:28:24Z

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 · 2025-06-12T12:55:18Z

contributors/devel/sig-architecture/api-conventions.md

@@ -843,10 +843,10 @@ having the `+optional` comment tag is highly recommended.
 Required fields have the opposite properties, namely:

 - They do not have an `+optional` comment tag.
- They do not have an `omitempty` struct tag.
- They are not a pointer type in the Go definition (e.g. `AnotherFlag SomeFlag`).
+- They mark themselves as required explicitly with a `+required` comment tag, or implicitly by not setting an `omitempty` struct tag.


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

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

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?

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 · 2025-06-12T17:17:13Z

/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 · 2025-06-12T17:19:50Z

(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 · 2025-06-12T23:29:24Z

contributors/devel/sig-architecture/api-conventions.md

 - The API server should not allow POSTing or PUTing a resource with this field
 unset.
+- They _typically_ do not use pointer types in the Go definition (e.g. `AnotherFlag SomeFlag`), though required fields where the zero value is a valid value may use pointer types, likely paired with an `omitempty` struct tag to avoid spurious null serializations.


"may" or "must" ?

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

fabriziopandini · 2025-06-17T15:46:48Z

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

fabriziopandini · 2025-06-17T15:52:08Z

contributors/devel/sig-architecture/api-conventions.md

 - The API server should not allow POSTing or PUTing a resource with this field
 unset.
+- They _typically_ do not use pointer types in the Go definition (e.g. `AnotherFlag SomeFlag`), though required fields where the zero value is a valid value must use pointer types, paired with an `omitempty` struct tag to avoid spurious null serializations.
+
+For more details on how to use pointers and `omitempty` with fields, see [Serialization of optional/required fields](#serialization-of-optionalrequired-fields).

 Using the `+optional` or the `omitempty` tag causes OpenAPI documentation to


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

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

liggitt · 2025-06-24T14:16:37Z

contributors/devel/sig-architecture/api-conventions.md

+This is expected to change in the future and having the `+optional` comment tag is
+now required on new optional fields.


nit: maybe rephrase this to avoid interleaving optional ... required ... optional wording (even though it refers to different contexts)

Maybe something like this:

This is expected to change in the future, and new fields should explicitly set either an +optional or +required comment tag.

liggitt · 2025-06-24T14:17:28Z

contributors/devel/sig-architecture/api-conventions.md

+    B -- No --> D{Is the field optional, or required?};
+    D -- Optional --> E[Does the field have a built-in nil value?];
+    E -- Yes --> F[Do not use a pointer, use omitempty];
+    E -- no --> C;


Suggested change

E -- no --> C;

E -- No --> C;

liggitt · 2025-06-24T14:17:50Z

contributors/devel/sig-architecture/api-conventions.md

+    A[Start] --> B{Is the zero value a valid user choice?};
+    B -- Yes --> C[Use a pointer and omitempty];
+    B -- No --> D{Is the field optional, or required?};
+    D -- Optional --> E[Does the field have a built-in nil value?];


Suggested change

D -- Optional --> E[Does the field have a built-in nil value?];

D -- Optional --> E[Does the field type have a built-in nil value?];

liggitt · 2025-06-24T14:18:04Z

contributors/devel/sig-architecture/api-conventions.md

+graph TD;
+    A[Start] --> B{Is the zero value a valid user choice?};
+    B -- Yes --> C[Use a pointer and omitempty];
+    B -- No --> D{Is the field optional, or required?};


Suggested change

B -- No --> D{Is the field optional, or required?};

B -- No --> D{Is the field optional or required?};

liggitt · 2025-06-24T14:20:22Z

contributors/devel/sig-architecture/api-conventions.md

+    A[Start] --> B{Is the zero value a valid user choice?};
+    B -- Yes --> C[Use a pointer and omitempty];
+    B -- No --> D{Is the field optional, or required?};
+    D -- Optional --> E[Does the field have a built-in nil value?];


would it be simpler to inline the types which do have built-in nil values? "Does the field type have a built-in nil value (map or slice)?")

liggitt · 2025-06-24T14:23:29Z

contributors/devel/sig-architecture/api-conventions.md

+- When a field is required, and the zero value is not valid, a structured client who has not expressed an explicit choice will have their request rejected by the API server based on the invalid value, rather than the field being unset.
+  - For example, a string with a minimum length of 1; Validation would not understand if the field was unset, or set to the empty string deliberately, but would still reject the request because it did not meet the length requirements.
+  - Technically, using a pointer in these cases is also acceptable, but not advised as it makes coding more complex, and increases the risk of nil pointer exceptions.
+  - In these cases, not using `omitempty` provides the same result, but pollutes the marhsalled object with zero values and is not recommended.


Suggested change

- In these cases, not using `omitempty` provides the same result, but pollutes the marhsalled object with zero values and is not recommended.

- In these cases, not using `omitempty` provides the same result, but pollutes the marshaled object with zero values and is not recommended.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 12, 2025

k8s-ci-robot requested review from dims and jbeda June 12, 2025 09:51

k8s-ci-robot added the area/developer-guide Issues or PRs related to the developer guide label Jun 12, 2025

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 12, 2025

sbueringer reviewed Jun 12, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2025

thockin reviewed Jun 12, 2025

View reviewed changes

JoelSpeed force-pushed the update-required-fields-conventions branch from c2ee563 to 8856289 Compare June 13, 2025 13:13

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 13, 2025

JoelSpeed force-pushed the update-required-fields-conventions branch from 8856289 to 22f7099 Compare June 13, 2025 13:14

Update guidance on optional/required field serialization

a40a583

JoelSpeed force-pushed the update-required-fields-conventions branch from 22f7099 to a40a583 Compare June 13, 2025 13:39

JoelSpeed mentioned this pull request Jun 13, 2025

Discourage usage of nullable on API types #8488

Open

fabriziopandini reviewed Jun 17, 2025

View reviewed changes

liggitt reviewed Jun 24, 2025

View reviewed changes

		This is expected to change in the future and having the `+optional` comment tag is
		now required on new optional fields.

	D -- Optional --> E[Does the field have a built-in nil value?];
	D -- Optional --> E[Does the field type have a built-in nil value?];

	B -- No --> D{Is the field optional, or required?};
	B -- No --> D{Is the field optional or required?};

	- In these cases, not using `omitempty` provides the same result, but pollutes the marhsalled object with zero values and is not recommended.
	- In these cases, not using `omitempty` provides the same result, but pollutes the marshaled object with zero values and is not recommended.

Update guidance on required field serialization #8486

Are you sure you want to change the base?

Update guidance on required field serialization #8486

Conversation

JoelSpeed commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 12, 2025

Uh oh!

JoelSpeed commented Jun 12, 2025

Nullable

String

Integer/Float

Bool

Lists

Maps

Objects

Uh oh!

sbueringer Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Jun 12, 2025

Uh oh!

liggitt commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini commented Jun 17, 2025

Uh oh!

fabriziopandini Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoelSpeed commented Jun 12, 2025 •

edited

Loading

sbueringer Jun 12, 2025 •

edited

Loading

sbueringer Jun 13, 2025 •

edited

Loading

fabriziopandini Jun 17, 2025 •

edited

Loading