Update unions guidance to confrom with latest serialization guidance

[JoelSpeed]

Advice around union serialization has changed since Go 1.24, now that we have omitzero. This is all implemented in KAL already but useful to update the guidance here too

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci]

@JoelSpeed: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[saschagrunert]

Suggested change

	* All structs within the union **MUST** not allow the zero value (i.e. avoid allowing `{}` for structs with required fields)
	* All structs within the union **MUST** not allow the zero value (i.e. each struct should have at least 1 required field, or a `MinProperties` validation to prevent `{}` from being valid)

Nit: the current wording could be read as "for structs that have required fields, don't allow {}", which is already the case since required fields prevent {}. I think the intent is the other direction: here's how to prevent {}. This phrasing matches what the Pointers to structs section already says.

[patrickdillon]

@JoelSpeed can you provide some more context on what the latest serialization guidance is? Prior to this change, it was clear: the members were always pointers, now we need to choose between pointer or value type, so how do we choose?

The reason this came up is because we were struggling to validate how to ensure non-pointer union types were not set. You pointed out that the simple obj.Foo == Foo{} would suffice. I'm not sure if you want to go into that level of detail here, but that suggestion may be helpful to include.

As part of that discussion, I reviewed as much of the discussion and PRs I could find in openshift/api, and everything still indicated that pointers were needed for union members. Perhaps I need to go further upstream to understand this, particularly the motivation: yes, now we can use omitzero but why?

I would guess that the guidance is that now we prefer value types, unless the type includes a slice or map, in which case obj.Foo == Foo{} doesn't compile. But even then, isn't that risky because we will then never be able to update those members to include fields of those types? That seems like a very substantial risk, and it's unclear to me why we take it.

[JoelSpeed]

Forewarning, this is a can of worms. But it's worthwhile reading and understanding the upstream serialization guidance, in particular the parts that call out CRD specific behaviour.

We have always (in openshift 4 at least) aimed to avoid pointers in our APIs as much as possible. Historically, there were plagues of issues leading from NPEs, so the target was avoid pointers, avoid NPEs.

Prior to omitzero, a zero valued or empty value type struct would not be omitted by the json serialization. So you'd end up with foo: {}. To have it truly omitted on the wire, you'd need a pointer.

Now we have omitzero, it will be omitted on the wire without a pointer. This means we can now treat structs similarly to other fields for CRDs. You only need a pointer if there is a semantic difference between unset and the zero value.
For structs, they should always have a required field, or have a minproperties style validation meaning that foo: {} is never valid (and any API adding something where that is valid today would face serious scrutiny).

Why does on the wire matter? Because for CRDs the required vs optional checks, and presence detection in CEL, is based on the presence of the json key, not the value. So our CEL rules for unions only work when the empty structs are properly omitted on the wire.

For the installer in particular, this is slightly tricker because you don't validate in the same way CRDs do (maybe you should?). But you can build out a similar validation in Go.

if obj.Type == "Foo" && obj.Foo == (Foo{}) {
  return errors.New("obj.Foo is required when obj.Type is Foo")
} else if obj.Type != "Foo" && obj.Foo != (Foo{}) {
  return errors.New("obj.Foo is forbidden when obj.Type is not Foo")
}

Repeat the above block for however many union members you have.

[patrickdillon]

You only need a pointer if there is a semantic difference between unset and the zero value.

Or if you want to check that a union member containing a map/slice is unset without iterating over all fields, right?

But yeah I think the zero-value value-type check you suggested should work fine enough

[JoelSpeed]

Or if you want to check that a union member containing a map/slice is unset without iterating over all fields, right?

Substitute the if check above with a reflect.DeepEqual and you should still have similar semantics

[patrickdillon]

Would it be worth adding dumb simple guidance (somewhere not necessarily here), like:

use omitempty for required fields and omitzero for optional (or union discriminator) members?

This may be obvious to others, but it wasn't obvious to me why we need omitempty for required fields.

Also it seems like there are many other examples in this document where a field is required but not omitempty, we should probably clean those up. For example:

// LocalDefabulatorReference references a defabulator.
type LocalDefabulatorReference struct {
	// name is the metadata.name of the referenced defabulator object.
	// +kubebuilder:validation:Required
	// +required
	Name string `json:"name"`
}

This should be omitempty right? There are at least two or three more like this. If scope is creeping we can follow up, but LMK if my understanding is correct.

[everettraven]

In general, we would push for that field to have omitempty and validations that don't allow the value to be the empty string. The exception to this would be if the empty string is a valid value.

I agree that we should probably do a sweeping update of the document to update all the examples, but I'm also okay with incremental updates if we'd like to keep this tightly scoped.