validation-gen: allow required/optional tags on non-pointer structs

Project: kubernetes/kubernetes | Language: Go | Concepts: api-design, error-handling, testing-strategy, refactoring
Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/136779

The problem

Previously, the Kubernetes validation-gen tool would error out if a developer tried to use the +k8s:required or +k8s:optional tags on a non-pointer struct field. This restriction made it impossible to pair OpenAPI schema tags with validation tags for such fields, limiting flexibility and making it harder to maintain consistent API definitions and linter rules.

Why it's non-trivial

A naive fix would simply allow these tags everywhere, but the runtime semantics for "required" or "optional" don't map cleanly to non-pointer structs. Unlike pointers (where presence/absence is clear), a non-pointer struct is always present (with a zero value), so enforcing presence or absence at runtime isn't meaningful. The fix must allow the tags for documentation/linting purposes without generating misleading or broken runtime validation code.

The approach

1. Allow tags on non-pointer struct fields, treat as documentation-only

The core logic change is in the validator code. Previously, using these tags on non-pointer structs would return an error:

-		// The +k8s:required tag on a non-pointer struct is not supported.
-		// If you encounter this error and believe you have a valid use case
-		// for forbiddening a non-pointer struct, please let us know! We need
-		// to understand your scenario to determine if we need to adjust
-		// this behavior or provide alternative validation mechanisms.
-		return Validations{}, fmt.Errorf("non-pointer structs cannot use the %q tag", requiredTagName)
+		// We cannot reliably enforce required presence semantics for non-pointer structs.
+		//
+		// Practically, a non-pointer struct becomes "implicitly required" if it has
+		// any validations which fail on the zero-value (e.g. required members, or
+		// union/oneOf semantics requiring at least one member to be set). If it has
+		// only optional members and no such semantic constraints, it is effectively
+		// optional at runtime.
+		//
+		// Still, OpenAPI +required / +optional are allowed on non-pointer structs,
+		// and we want to allow colocated declarative validation tags so linters can
+		// enforce tag pairing. Treat this tag as documentation-only.
+		return Validations{Comments: []string{"+k8s:required on non-pointer struct fields is purely documentation"}}, nil

The same logic is applied to the +k8s:optional tag:

-		// The +k8s:optional tag on a non-pointer struct is not supported.
-		// If you encounter this error and believe you have a valid use case
-		// for forbiddening a non-pointer struct, please let us know! We need
-		// to understand your scenario to determine if we need to adjust
-		// this behavior or provide alternative validation mechanisms.
-		return Validations{}, fmt.Errorf("non-pointer structs cannot use the %q tag", optionalTagName)
+		// We cannot reliably enforce optional presence semantics for non-pointer structs.
+		// See doRequired() for details.
+		//
+		// The Optional* validators are used primarily to short-circuit further
+		// field/type validations when a value was not specified
+		//
+		// Accept the tag to enable OpenAPI/declarative tag pairing (linting), but do
+		// not emit runtime validations.
+		return Validations{Comments: []string{"+k8s:optional on non-pointer struct fields is purely documentation"}}, nil

2. Update generated validation code to handle non-pointer struct fields

The generated validation functions now include a function for the non-pointer struct field, but only call the inner type's validation (no presence check):

+	{ // field Struct.OtherStructField
+		fn := func(
+			fldPath *field.Path,
+			obj, oldObj *OtherStruct,
+			oldValueCorrelated bool) (errs field.ErrorList) {
+			// +k8s:required on non-pointer struct fields is purely documentation
+			// don't revalidate unchanged data
+			if oldValueCorrelated && op.Type == operation.Update {
+				if obj == oldObj || (obj != nil && oldObj != nil && *obj == *oldObj) {
+					return nil
+				}
+			}
+			// call the type's validation function
+			errs = append(errs, Validate_OtherStruct(ctx, op, fldPath, obj, oldObj)...)
+			return
+		}
+		oldVal := safe.Field(oldObj,
+			func(oldObj *Struct) *OtherStruct {
+				return &oldObj.OtherStructField
+			})
+		errs = append(errs, fn(fldPath.Child("otherStructField"), &obj.OtherStructField, oldVal, oldObj != nil)...)
+	}

This pattern is used for both +k8s:required and +k8s:optional tags on non-pointer struct fields.

3. Update tests to reflect the new behavior

Tests are updated to allow and check the new field/tag combinations, and to assert that validation does not fail due to the presence of the tags:

-	// non-pointer struct fields cannot be optional
+	// +k8s:optional
+	OtherStructField OtherStruct `json:"otherStructField"`

And in the test logic:

-	st.Value(&Struct{
-		// All zero-values.
-	}).ExpectValid()
+	st.Value(&Struct{
+		// All zero-values.
+	}).ExpectMatches(field.ErrorMatcher{}.ByType().ByField(), field.ErrorList{
+		field.Invalid(field.NewPath("otherStructField"), nil, ""),
+	})

Similar changes are made for required fields.

From the review thread

JoelSpeed: Speaking with @lalitc375 earlier this week. +k8s:required is different from +required in that it ALSO means that the object should be non-zero. (This will never be compatible with openapi schema definitions FWIW - is it too late to revisit that decision?) But that means that logically, if a struct is marked +k8s:required we should check for the struct not being the zero value, right?

if spec.struct == new(StructType) {
  Error("at least one property is required")
}

arcceus: @JoelSpeed not sure if I follow, when you say the non-zero semantic "will never be compatible with openapi schema definitions," what's the problem? The linter PR (kubernetes-sigs/kube-api-linter#251) already checks that a struct marked +k8s:required has some presence-enforcing signal internally, so the non-zero enforcement does exist, it's just on the linter side rather than validation-gen. Is the concern that linter-only enforcement isn't strong enough, or is it something else?

JoelSpeed: I'm talking more widely. If you look at the semantics of the required field in an openapi schema (also CRD schemas because of this) it means that the key must be present. It has no reflection on the value of the field.

...

The issue is that we have inconsistency between what +k8s:required means for a pointer vs non-pointer field and that isn't something that can be represented in openapi.

If instead +k8s:required just meant field is present and had no reflection on the value (so you'd have to also set +k8s:minLength on a string for example), then the semantics would translate more easily to openapi, and therefore CRDs

Takeaways

• Allowing schema/validation tags for documentation and linting—even if not enforceable at runtime—can improve API consistency and developer experience.
• Validation logic for presence should match the field's type semantics; non-pointer structs are always present, so presence checks are not meaningful.
• Aligning code generation, runtime validation, and schema/linter expectations requires clear documentation and explicit handling of ambiguous cases.