kubernetes: ContainerPorts SSA key is insufficiently unique - SSA fails with "duplicate" error when the contents are valid

https://github.com/kubernetes/kubernetes/pull/113245#discussion_r1009911070

Validation allows:

        ports:
        - containerPort: 93
          hostPort: 9376
        - containerPort: 93
          hostPort: 7693

but server-side apply does not.

As of 1.29 we can SSA unrelated fields in an object with “dup ports” and even do SSA “repairs” to those fields. What we can’t do is SSA them with this state in the apply. Important: this is allowed (more by accident than design) and semantically unambiguous.

The defined SSA key is not sufficiently unique (ontainerPort + protocol – needs to include hostPort). To evolve the key is tricky, but the only real alternative is atomic, which has it’s own issues.

Is it worth trying to find a safe way to expand an SSA key?

About this issue

Original URL
State: open
Created 2 years ago
Comments: 21 (21 by maintainers)

Most upvoted comments

So glad to hear we worked this out. Thank you Antoine.

thockin on Nov 1, 2023

When I filed this, I did not realize it would break all of SSA, even for unrelated fields.

Copying from https://github.com/kubernetes/kubernetes/issues/118261#issuecomment-1574155048

It’s valid to have containerPorts[] with same port+protocol and different hostPort or hostIP. In fact, it is (unfortunately) allowed to have the same hostPort and hostIP as long as they are unspecified (the Go zero values). The SSA keys are wrong. The current validation is overly loose, which practically means this field can not be treated as a map (or we break users who do this weird-but-valid thing).
An env var value can ref other env vars (including a previous instance of its own name!!). Ergo, it’s really not a map at all.

If I change both of these to +listType=atomic then SSA works for things which have PodSpec embedded (Pods, Deployments, etc). @apelisse points out that this is an unfortunate tradeoff - if any client actually wants to co-own container ports (unlikely) or env vars (somewhat more likely), they have to know that this field is atomic, and do a read-modify-write.

I have a branch which implements this. @apelisse is thinking about how to convince ourselves it is OK.

Also: https://github.com/kubernetes/kubernetes/issues/118418 and https://github.com/kubernetes/kubernetes/issues/118417 are some followup.

My branch: https://github.com/kubernetes/kubernetes/compare/master...thockin:kubernetes:ssa_hack_env_ports?expand=1

thockin on Jun 2, 2023

FYI https://github.com/kubernetes/kubernetes/issues/118261#issuecomment-1572482716

alculquicondor on Jun 1, 2023