Lenient validation and merging for non-strict `listType=map`

[apelisse]

Right now, we have some lists that are marked as listType=map that are not strictly maps in Kubernetes. In that case, everything goes wrong in smd because the list is invalid (and can't be handled), so it's fully rejected. We can't even track the fields, which causes us to fail and return an error which is ignored by Kubernetes, and the managed fields are entirely reset.

Unfortunately, it means that objects that have such a list can't be server-side applied at all. We think it'd be simpler to merely ignore these (fairly rare) occurences and try to do the best thing possibe rather than fail.

This means:

1. When we validate an object with such a list, we should probably not fail altogether (though we should probably reject a request to server-side apply an object like this).
2. When tracking the fields, we should give duplicate fields to the same owner.
3. When server-side applying without the field, we need to be careful about how the field should be removed.
4. When we merge a list that has such a field, we need to be careful, especially if the server-side apply contains the field (should we remove both other occurences? Probably).

[apelisse]

cc @thockin @jpbetz

[jpbetz]

Do we have the list of fields that fall into this "marked as listType=map but not validated as having unique keys" category?

[apelisse]

Not really but, that's a great question:

1. I'm pretty sure CRDs don't allow that and never will, so there's no chance people start abusing that mechanism there (IOW, actual validation matches SSA validation)
2. Since it's only in built-ins, and because a lot of these were not designed for that initially (unfortunately), we have:
   environment variables and a few containerports.

I considered adding a new, separate tag, but that would require a KEP and make things impossible to backport. I think we need to make it clear that this is a work-around and shouldn't be used for any new list (again, only for built-ins so we have a lot more control over that).

[thockin]

There are two distinct failure modes, the one we have usually run into is client-side apply vs. Service.spec.ports, where the keys are looser than validation, and SSA is the SOLUTION (the keys match validation).

The more recent one is where SSA's keys are more strict than validation. So far we've found:

• Pod.spec.containers[].ports
• Pod.spec.containers[].env
• Pod.spec.containers[].envFrom

I admit that I haven't scoured the API for this case, and I don't honestly know how I would in any reasonable amount of time. :( IOW, there's ikely to be more

[jpbetz]

I did a scan. Here's what I found:

• StorageVersionStatus.StorageVersions - listMapKey=apiServerID - validation missing (Add merge map key validation to StorageVersions kubernetes/kubernetes#118520)
• PodSpec.TopologySpreadConstraints - validation missing, listMapKey=whenUnsatisfiable (validation found)
• Container.Ports, EphemeralContainerCommon.Ports - validates unique name (if set) unique {hostPort, protocol, hostIP} (if hostPort set), listMapKey=containerPort, protocol (validation found)(https://github.com/kubernetes/kubernetes/blob/c5100c0d11fb32930dc49fd0a6e2ec3a5aa99d9e/pkg/apis/core/validation/validation.go#L4977-L4982) - same as ContainerPorts (except keys are port, protocol)
• Container.Env - duplicates and repeated keys allowed, order matters, patchMergeKey=name
• PodSpec.ImagePullSecrets - validation missing, patchMergeKey=name
• PodSpec.HostAliases - validation missing, patchMergeKey=ip
• ServiceAccount.Secrets - validation missing, patchMergeKey=name
• ObjectMeta.OwnerReferences - validation missing, patchMergeKey=uid
• LabelSelectorRequirement.Key - validation missing, patchMergeKey=key (Remove invalid merge key kubernetes/kubernetes#118522)

A second pair of eyes on this would be really helpful. I only looked at the validation code. I haven't actually tried all these cases.

[apelisse]

I wouldn't be surprised if some people think this will automatically write the validation for them. For most people authoring new resources and using this, it should tbh. Maybe we should generate and have an opt-out flag?

[jpbetz]

Maybe we should generate and have an opt-out flag?

kubernetes/enhancements#3938 proposes that we automatically validate. An opt-out flag sounds useful.

For these 4 ports cases, should we chnage listMapKey to 'name' to match what is validated? Can SSA handle a map key change?

[apelisse]

For these 4 ports cases

Which 4?

If you mean:

Container.Ports - validates unique name, listMapKey=containerPort, protocol

I don't think that's correct, name is unique but I don't know if that's good enough, name isn't required.

[jpbetz]

I don't think that's correct, name is unique but I don't know if that's good enough, name isn't required.

Ah, we don't have a key then, because name is the only field that validation checks when looking for duplicates.

We might need to go with atomic?

[apelisse]

Actually, if we're careful we might be able to make it work with name, especially if it's omitempty, we can default it to "". Then as you mentioned, we would never have two keys with the same name.

[apelisse]

If that works by the way, that might be a great way to fix a lot of these problems. For env, we might be able to make a key with name AND value. Then we can only have one such pair, I think that's fine, we can still merge the list properly.

[jpbetz]

If that works by the way, that might be a great way to fix a lot of these problems. For env, we might be able to make a key with name AND value. Then we can only have one such pair, I think that's fine, we can still merge the list properly.

I've love to see a test suite trying out various operations on a listType=map defined this way. For CEL in CRDs we defined map key to be the CEL expression and it works just fine...

[apelisse]

I've love to see a test suite trying out various operations on a listType=map defined this way.

We have a lot of tests in this repo, what do you have in mind? We already have multi-key tests here, that should work fine. We can reason and see what happens (and maybe fix) when we change the keys, I think we had thought about this a while back.

[jpbetz]

We can reason and see what happens (and maybe fix) when we change the keys, I think we had thought about this a while back.

Good point.

Maybe open a PR that tries to fix the broken cases and we can manually try various apply requests out?

• Env: Use "key", "value"? This is still not exactly what we validate-- right now you could repeat A=x{$A} twice... should we go this route or just mark it as atomic?
• Ports: Use enough fields to ensure uniqueness in each case? E.g. all the fields of ContainerPort?
• Cases with no validation: Mark them as atomic?

ObjectMeta.OwnerReferences worries me..

[thockin]

Actually, if we're careful we might be able to make it work with name, especially if it's omitempty, we can default it to "". Then as you mentioned, we would never have two keys with the same name.

For ContainerPort, the name is optional. If specified, it must be unique, but many ports may be named "".