Correct wrong list type on set fields

[erikgb]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This fixes some list-type markers in APIs from atomic to set. The main benefit of this change is that multiple SSA managers can act on the same list field, as each value is tracked separately. This is technically a breaking change, as the API will now reject duplicate values. But I cannot imagine that duplicate values make any sense here, and order is irrelevant, which is also a requirement for the set list type.

Extracted from #3964.

I asked for advice on changes like this in the #sig-api-machinery Slack channel and received some feedback. Here are the main quotes:

Technically breaking. But when we introduced them in Kube itself apparently this was done without version change.

I would expect fun side effects, even if you do it as part of an API version bump. We did it in CAPI and it was not a good experience. Depending on use case you may be able to tolerate the issues that generally come with this change,

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Hostname and retry status code fields are now treated as sets

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

[robscott]

Thanks for all the work on this @erikgb! The Slack thread you linked to definitely gives some reason for pause here, particularly the link to the corresponding k8s docs that recommend against a change like this: https://kubernetes.io/docs/reference/using-api/server-side-apply/#compatibility-across-topology-changes. Given that most of these are changes to fields in standard channel, I'd recommend avoiding this correction. With that said, there is one change that covers a field that is currently only in experimental channel - HTTPRoute retries. It might make sense to try to make that change before it makes it to standard channel.

If you're able to make it to the community meeting in ~9h, this could be a good topic for that meeting.

/cc @mikemorris
/hold

[youngnick]

Yeah, I agree with @robscott here that changing this for an experimental feature first seems like a good idea, so we can see what happens in that case.

Alternatively, working through a process like :

• Install CRD YAML from main
• install an implementation and then some example Gateways that use the field (hostnames for example), confirm working
• Make the change in the Go types, generate new YAML
• install the new YAML
• see what happens with the implementation

since duplicating hostnames within that field doesn't really make sense anyway, we might be lucky and get away with just changing it. But given this is mainly relevant for server-side apply, it may need an implementation which is using SSA properly to check the effects.

[erikgb]

@robscott @youngnick Thanks for the feedback! Your stand here is fully understandable. This could potentially break downstream users/projects, even if I think the probability is low. Kubernetes did similar bugfixes without new API versions, and I think we also should do it, if we think it's the correct way for the fields to behave in the API. But you are the maintainers and make the decisions here. ❤️

I neither have the interest nor the bandwidth to follow up on this, as it turned out to be a debatable fix. 😆 Anyone is free to pick up my work on this if they can/want, but I am not planning to do more now. I mainly came here to add the missing list markers, which ended up being added in #3964. Thanks a lot!