Resource Claims must be a map type, not set

[JoelSpeed]

What type of PR is this?

/kind bug
/kind api-change

What this PR does / why we need it:

This PR adds the structType to the ResourceClaim struct within the corev1 API group.

When the structType is not set, it defaults to granular (out of a choice of granular or atomic).
Granular structs are not compatible with being used in a listType=set slice.
This type was introduced for the Claims member of ResourceRequirements which has the listType=set semantic.

Without this fix, CRD schemas generated, including the ResourceRequirements struct are not valid and produce an error when installed into a cluster.

Which issue(s) this PR fixes:

Currently, if you import the corev1.ResourceRequirements type into a custom resource definition, the generator generates an invalid schema, which results in errors such as below when you attempt to create the CRD.

unable to create CRD instances: unable to create CRD \"configs.imageregistry.operator.openshift.io\": CustomResourceDefinition.apiextensions.k8s.io \"configs.imageregistry.operator.openshift.io\" is invalid: spec.validation.openAPIV3Schema.properties[spec].properties[resources].properties[claims].items.x-kubernetes-map-type: Invalid value: \"null\": must be atomic as item of a list with x-kubernetes-list-type=set

Special notes for your reviewer:

The validation error we are hitting is this piece of code.

It was introduced by @sttts in e4e1c2c, and we are specifically hitting the object case here. Looking at the PR, seems @apelisse may be the expert on why this validation was introduced and may be able to help ascertain that this is the correct fix.

This validation sets a requirement that any struct used as the underlying type of a slice, must be an atomic struct when the slice itself is a set.

The ResourceRequirements was recently updated to add the new field Claims in #111023 which then introduced this bug. CC @pohly. Tthis is a very obscure bug and I have no idea how anyone would know to set the struct tag correctly.

I noticed while search k/k that there are many instances of this throughout the codebase where we have invalid combinations of listType=set and no structType set. We may want to fix the other instances too.

CC @deads2k

Does this PR introduce a user-facing change?

Fixes a 1.26 regression in the published openapi of Pod types caused by an incorrect list-type of the alpha resourceClaims field. This fix modifies that field list type from "set" to "map", resolving an incompatibility with use of this schema in CustomResourceDefinitions and with server-side apply.

[pohly]

Thanks for tracking this down. I have no idea what +structType=atomic does and thus need to rely on the experts to review this change.

What worries me is that this wasn't detected by any check in Kubernetes itself. If this annotation is required, shouldn't it be checked for in Kubernetes?

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[JoelSpeed]

Hmm, this line is going to be picked up in openapi, I wonder if I can hack this to prevent it being picked up there

In the past I've put + at the beginning of the line to prevent generators including dev targeted comments, do we think that would be appropriate here?

[apelisse]

I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map, listMapKey=name?

With regards to validating these, yeah, we should do a better job.

https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy

I don't know if the validation is correct, it should probably only allow scalar types.

[JoelSpeed]

I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map, listMapKey=name?

In a more complex example where you do want a set, eg with 3 or 4 members in the struct, would that then mean you have to make every field a map key to get the same semantic?

[apelisse]

I guess, but the keys have to be scalars too

[JoelSpeed]

I don't think this should be allowed tbh, though that looks like something can be fixed by having listType=map, listMapKey=name?

Would this be your suggested path forward for this particular API then? Convert it to a map?

[JoelSpeed]

What worries me is that this wasn't detected by any check in Kubernetes itself. If this annotation is required, shouldn't it be checked for in Kubernetes?

I think if we can add a check for this somewhere (not sure where yet) then that would help others down the line, perhaps the sig-apimachinery folks have some ideas on where best to stick that

[apelisse]

That's how I would write it if this was new code, but I don't know the history here, so it's a little concerning to change it. how long has this been broken?

[JoelSpeed]

It was merged just over a month ago

[pohly]

This API first appeared in 1.26.0.

[JoelSpeed]

I've just noticed controller-tools have put their own fix into the CRD generator kubernetes-sigs/controller-tools#753

It definitely seems to solve the issue, but I'm concerned given the conversation whether this is the right path forward? Should we recommend against this?

Looking at the issue that it's linked to, this has been a direct result of the bug I'm trying to resolve

[apelisse]

Thanks for finding that. Where is the type here used? Is it used only in (currently broken) CRDs or is it used in built-in types?

[JoelSpeed]

Where is the type here used?

The type that's mentioned here, corev1.ResourceRequirements is being used in both CRDs and in the PodSpec and PersistentVolumeClaimSpec APIs, so the built-ins are also broken right now I guess? At least for SSA?

[apelisse]

Yeah, probably :-(

[JoelSpeed]

Given we think that the behaviour intended, would be the same with listType=map and listMapKey=name, should I update the PR to that? Would that be an acceptable API change given this has made it to a release?

[pohly]

Looks good to me. The map type also matches possible future extensions with additional parameters attached to each name, which was the whole reason why Claims contains structs instead of just plain strings.

[liggitt]

The map type also matches possible future extensions with additional parameters attached to each name, which was the whole reason why Claims contains structs instead of just plain strings.

In the event that those parameters were added, names would still be required to be unique in the list (as they are today), right? If so, type=map, key=name is the right approach

[pohly]

Correct.

[liggitt]

/hold cancel

[liggitt]

looks like verify is happy. @JoelSpeed can you go ahead and pick this to release-1.26?

[JoelSpeed]

Will the cherry pick bot work? Is there a reason not to use it?

I'm away from my laptop but can manually pick later if need be

[liggitt]

Will the cherry pick bot work? Is there a reason not to use it?

cherry-pick bot isn't enabled for k/k, hack/cherry_pick_pull.sh is used in this repo