AFAIK we do not have any pointer-to-{slice,map} fields and I have told people in the past that we DO NOT want them. This makes it sound like it would be OK, but I don't think it would (I strongly suspect some codegen would break). I was actually a bit surprised that JSON unmarshal handles it properly.

One use case I have seen is *[]corev1.Taint. Where the use case will default "I have no opinion" to include a set of default taints, but, if you specifically do not want those taints, it documents that you should explicitly provide an empty list [] for the field.

Do you think that would not actually be possible then? What would you recommend instead for this use case?

I suspect it doesn't work as described. We tried to do this in network policy and gave up because it did not work.

The only answer I know, and the way we did it in straight proto (caveat: it's been a while) is to put a (pointer to) struct in between. Nil struct means default. Non-nul means the list is present. All languages can decode that into something intelligible.

Not sure about builtin types, for CRDs it looks like controller-gen works (we generate deep copy, conversion and CRD YAMLs). I just replaced a custom MarshalJSON func for []corev1.Taint (to preserve []) with *[]corev1.Taint and it worked out of the box

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

What about omitzero?

Omitzero should work, but as far as I know, we haven't tested this yet, or explored what would happen re-protobuf for built-in types. For now I'd rather not mention omitzero until we've had a chance to test it.

I'd see that as a follow up.

+1 to this -- we should be clear that "encoding" means a lot more than just JSON :)

This is not in sync with Kal with whenrequired setting, right?

Correct. KAL's optionalfields implementation of WhenRequired is intended for CRD authors where the behaviour differs slightly.

For CRDs, because the validation of required vs optional happens at the openapi stage, and for any field where the zero value is not valid, openapi would pick this up before a structured go client were to see the object, we can safely assume for CRDs that for a field where the zero value isn't valid, that the user did not specify the zero value when we see the zero value in the Go struct.

With built-in types, all of the validation above is done using Go. And as such, we must be able to distinguish between unset and the zero value to be able to enforce that the zero value is not valid, hence a pointer is always required in these cases.

I think we should probably explain this somewhere as an addendum to this advice.

See comment above - I am not sure we want to allow this. IIRC protobuf doesn't have a way to represent this without an intermediate struct.

Hrmm... you're probably right about proto-persisted things.

For typed clients that use go but are backed on the server by a CRD, that means that serialization is always json and this is representable. I agree a pointer to a map or slice is weird. I think it's useful to describe this implication / edge, but would be fine discouraging making use of it ("avoid map or slice fields where {} or [] is valid and treated differently from an absent value")

Have one use case in #8486 (comment), but yes, when I've thought through this before, I've never worked out an issue with doing this in CRDs. If it's an issue for proto, then perhaps the right thing to do here is update the wording to say never do this for built-in (?) types, but it can work for custom resources?

perhaps the right thing to do here is update the wording to say never do this for built-in (?) types

There are three areas rationale is based on:

1. client-side behavior... making sure a client can transmit on the wire what it means
2. server-side validation / defaulting behavior ... making sure a server can distinguish what the client sent in ways that let it validate and default sensibly
3. server-side persistence / round-tripping behavior ... making sure a server can persist distinctions that matter

2 and 3 are definitely different for custom-resources vs built-in/server-typed resources, and custom resources are actually more predictable since they store as json and don't conflate absent and present-with-zero-value on the server side. I guess we could branch the guidance early on if we think it would lead to different/simpler/better recommendations for one case or the other.

I would argue that the word "pointer" has no real meaning in CRD and in fact our API docs should describe a language-neutral data model. We happen to use Go to implement that model, and while Go has the concept of array, nil-slice, empty-slice, and pointer-to-slice, those are not part of our data model.

The question then is whether CRD should be able to do things that proto can't, because proto is static and CRD less so? I am fairly sure we could put tag numbers into CRDs and synthesize a proto descriptor, but we don't.

I think our data model should be limited to the common subset (basically "what proto can do"), for CRD /and/ built-in, otherwise we end up with divergent API patterns depending on implementation. Blech.

We support list (aka repeated) fields. They can have 0 or more items. They are not present or absent, they just have a size.

We happen to use Go to implement that model, and while Go has the concept of array, nil-slice, empty-slice, and pointer-to-slice, those are not part of our data model.

Do we have any data on how many custom resource APIs for K8s are written not in Go? My assumption has always been that the majority are, and therefore having guidance that applies to the majority, and helps prevent them from making serialization mistakes has value. Perhaps the missing part is that we don't talk about these being Go specific in the guidance?

Thinking on this today, does this extend beyond slices and maps?

It seems for API server/aggregated types we want to avoid folks creating distinctions between unset and the empty value for lists and maps. And while this works for custom resources, it should also be discouraged since while it works with Go, if they support other languages for clients, it may be difficult for the other languages to represent this.

Now I can see a clear use case for allowing zero values for numbers, but what about strings? Do we have valid use cases for "" having a different semantic meaning from unset? Can "" be distinguished in proto? Same for []byte?

For structs, I can think of an example (emptyDir) in core API types, so I guess that does work somehow in proto, but I believe we discourage this same pattern now right?

I'm planning to submit a follow up for this to clarify our stance here, so I'd like to make sure we cover all data types in our update.

Do we have valid use cases for "" having a different semantic meaning from unset?

I am sure there are fields where "" is an in-bounds value for a string. More than that, it seems weird to say zero-values are in-bounds for bool and int, but not for string?

Can "" be distinguished in proto?

I am 99.99% sure yes. Proto says optional string

Same for []byte?

This is represented as optional bytes in proto, but I don't know how a list of 0 bytes is presented on the wire - worth crafting a small demonstration, I think. It could be treated like "repeated" but that would be super wasteful (each tag is at least as costly as the datum). https://protobuf.dev/programming-guides/encoding/ says it is incoded the same as a string: tag + len+payload

For structs, I can think of an example (emptyDir) in core API types, so I guess that does work

optional messages are supported, even if the message is empty of other tags.

I am sure there are fields where "" is an in-bounds value for a string.

Definitely. Example is a reference to a type that allows specifying API group, where "" is a valid value (the core group).

I am 99.99% sure yes. Proto says optional string

Strings serialize as tag+length+bytes, so happily serialize zero-length strings (which is why new optional non-pointer string fields immediately mess with proto serialization).

StorageClassName *string `json:"storageClassName,omitempty" protobuf:"bytes,5,opt,name=storageClassName"`

marshals as:

	if m.StorageClassName != nil {
		i -= len(*m.StorageClassName)
		copy(dAtA[i:], *m.StorageClassName)
		i = encodeVarintGenerated(dAtA, i, uint64(len(*m.StorageClassName)))
		i--
		dAtA[i] = 0x2a
	}

unmarshals as:

		case 5:
			if wireType != 2 {
				return fmt.Errorf("proto: wrong wireType = %d for field StorageClassName", wireType)
			}
			var stringLen uint64
			for shift := uint(0); ; shift += 7 {
				if shift >= 64 {
					return ErrIntOverflowGenerated
				}
				if iNdEx >= l {
					return io.ErrUnexpectedEOF
				}
				b := dAtA[iNdEx]
				iNdEx++
				stringLen |= uint64(b&0x7F) << shift
				if b < 0x80 {
					break
				}
			}
			intStringLen := int(stringLen)
			if intStringLen < 0 {
				return ErrInvalidLengthGenerated
			}
			postIndex := iNdEx + intStringLen
			if postIndex < 0 {
				return ErrInvalidLengthGenerated
			}
			if postIndex > l {
				return io.ErrUnexpectedEOF
			}
			s := string(dAtA[iNdEx:postIndex])
			m.StorageClassName = &s
			iNdEx = postIndex

That means proto can distinguish between present and absent on decode, but, unless the go type it is decoding into is a *string, the resulting go object can't distinguish presence and absence

Same for []byte?

This is represented as optional bytes in proto, but I don't know how a list of 0 bytes is presented on the wire

Our current proto marshaling doesn't treat []byte as a repeated field, it serializes non-nil []byte values as tag+length+bytes:

Request []byte `json:"request" protobuf:"bytes,1,opt,name=request"`

marshals as:

	if m.Request != nil {
		i -= len(m.Request)
		copy(dAtA[i:], m.Request)
		i = encodeVarintGenerated(dAtA, i, uint64(len(m.Request)))
		i--
		dAtA[i] = 0xa
	}

unmarshals as:

		case 1:
			if wireType != 2 {
				return fmt.Errorf("proto: wrong wireType = %d for field Request", wireType)
			}
			var byteLen int
			for shift := uint(0); ; shift += 7 {
				if shift >= 64 {
					return ErrIntOverflowGenerated
				}
				if iNdEx >= l {
					return io.ErrUnexpectedEOF
				}
				b := dAtA[iNdEx]
				iNdEx++
				byteLen |= int(b&0x7F) << shift
				if b < 0x80 {
					break
				}
			}
			if byteLen < 0 {
				return ErrInvalidLengthGenerated
			}
			postIndex := iNdEx + byteLen
			if postIndex < 0 {
				return ErrInvalidLengthGenerated
			}
			if postIndex > l {
				return io.ErrUnexpectedEOF
			}
			m.Request = append(m.Request[:0], dAtA[iNdEx:postIndex]...)
			if m.Request == nil {
				m.Request = []byte{}
			}
			iNdEx = postIndex

For structs, I can think of an example (emptyDir) in core API types, so I guess that does work

optional messages are supported, even if the message is empty of other tags.

Right. We even have zero-field types in a few places where presence / absence drives behavior.

// CustomResourceSubresourceStatus defines how to serve the status subresource for CustomResources.
// Status is represented by the `.status` JSON path inside of a CustomResource. When set,
// * exposes a /status subresource for the custom resource
// * PUT requests to the /status subresource take a custom resource object, and ignore changes to anything except the status stanza
// * PUT/POST/PATCH requests to the custom resource ignore changes to the status stanza
type CustomResourceSubresourceStatus struct{}

// CustomResourceSubresources defines the status and scale subresources for CustomResources.
type CustomResourceSubresources struct {
	// status indicates the custom resource should serve a `/status` subresource.
	// When enabled:
	// 1. requests to the custom resource primary endpoint ignore changes to the `status` stanza of the object.
	// 2. requests to the custom resource `/status` subresource ignore changes to anything other than the `status` stanza of the object.
	// +optional
	Status *CustomResourceSubresourceStatus `json:"status,omitempty" protobuf:"bytes,1,opt,name=status"`
	...

for that to be usable in the resulting go object, the field must be a pointer to be able to distinguish presence / absence

Ok great, so we have examples of zero values being accepted for most types, and that working for proto, so it appears it really is just *[]T and *map[T]S that we need to worry about per #8486 (comment)

I'll post an update to discourage distinguishing between absent and zero length lists/maps

Hi,

I wanted to ask about this.

For maps and slices which have a built-in nil value, such as a map or a slice, and your use case means that you need to be able to distinguish between "field not set" and "field set to an empty value", you should use a pointer to the type, even though it has a built-in nil value. But otherwise not.

Woudn't similar logic applies for bool types. If in an use case bool "false" is same as "field not set" , then it doesn't make to have this be a pointer. Being a pointer implies a choice between 3 options (nil, false, true) but there are only 2 options (true/false) with false being default value

For maps and slices which have a built-in nil value, such as a map or a slice, and your use case means that you need to be able to distinguish between "field not set" and "field set to an empty value", you should use a pointer to the type, even though it has a built-in nil value. But otherwise not.

In this case, you have several options. Field is omitted, no validation occurs. Field is present and empty - this should be rejected as the map/list requires at least one entry. Field is present and has at least one entry, all good.

For Go types, the first and the second are the same if you do not have a pointer. Omitempty would mean that the field isn't serialized and so the second can't actually happen/ is not representable in Go. But it can happen for unstructured types which is why the minimum entries validation must exist.

For bools, false is always a valid choice, so you need to be able to round trip false for every bool, even if that means the same as omitted. omitempty for a bool would omit a false value, so you must use a pointer to avoid this. If a structured client wrote false to the field, and you then re-wrote the object with a Go type, you'd actually drop the field completely which is an issue