⚠️ Add x-kubernetes unique validation for the Conditions list

[m-messiah]

What this PR does / why we need it:

The cluster-api/util/conditions.Set method is already applying deduplication using Type as a key (https://github.com/m-messiah/cluster-api/blob/ff54cbff5c72de1aa486b09e4e866b37637c59bd/util/conditions/setter.go#L41), but the constraint was not visible in CRDs.
The PR adds two kubebuilder comments to treat Conditions as a map with Type as a key and prevent duplications on the kube-apiserver level.

For example, CRD manifest for CR that has Conditions clusterv1.Conditions in its status:

+                x-kubernetes-list-map-keys:
+                - type
+                x-kubernetes-list-type: map
                  type: array

/area api

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval. For more information see the Kubernetes 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

[sbueringer]

In general this would be okay. Also because the recommendation for metav1.Conditions (that we're probably moving to with v1beta2) is also recommending this map key (https://github.com/kubernetes/kubernetes/blob/3f7d4b787b74a2d110901d333f83e99c0e5e49c9/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1526-L1527).

But I'm concerned about what happens to resources that exists today where this key is not unique.

Do you have concrete cases where duplicate conditions occur today / this PR would fix it? What would happen to pre-existing resources if we merge this change?

[m-messiah]

Do you have concrete cases where duplicate conditions occur today / this PR would fix it? What would happen to pre-existing resources if we merge this change?

I made an example of having the CR with duplicated condition and after I applied the new CRD to the cluster - nothing happened - I could still get, list and even update the CR without any issues. No new duplicates to conditions could be added though

[sbueringer]

Interesting. Do the managed fields look correctly? Because they should then start using the map key to track ownership?

But to be honest, overall I would be much more comfortable waiting for v1beta2 with introducing the map key.

We are planning to move to upstream conditions anyway (see #10897)

(cc @fabriziopandini)

[JoelSpeed]

For something where we know that the intention (conditions across K8s) is that the type should be the map key, I think it makes sense to add this in.

The conditions in our APIs are written to by controllers that we own, and we expect, not by end users right? I believe the current controller logic would stomp the whole list at the moment, and given our utils, should already be deduplicated.

Given the intentions and the above, I think this should be an ok change to make even without bumping the API version. When we introduce the next API version, we should make sure all lists have some sort of listType marker

[vincepri]

Could we add a few tests using envtest that show the behavior you mentioned @m-messiah ?

[sbueringer]

I'm just a bit worried about how managedFields react to something like this (introducing a map key when preexisting array elements might not be unique according to that key). I wonder how ownership can then be tracked correctly.

Not sure if it's safe to assume that core CAPI controllers are the only ones writing conditions to our types

(btw if we make that assumption, then there is also no reason why we have to make this change now, because our controllers are already not adding conditions with duplicate types?)

[JoelSpeed]

I'm just a bit worried about how managedFields react to something like this (introducing a map key when preexisting array elements might not be unique according to that key). I wonder how ownership can then be tracked correctly.

The default when you do not specify otherwise, is that the list will be atomic. Therefore whoever last wrote to the conditions field will be considered by SSA as the owner of the entire list. I would expect this is likely the CAPI controllers themselves, since they are likely writing frequently, but it is also possible it would be someone else.

Once the change is implemented, the managed fields will now represent individual conditions with their respective owners. It won't be aware that there were previous owners of individual conditions but it will now only allow one of the owners to own each condition type.

If SSA is used, the actors on the conditions may now see a conflict and have to use the "force" flag to force ownership (this is common in controllers), but I don't think SSA is very widely used right now?

If the actors continue to use the update methods without using SSA, then they must specify the whole list (ergo every condition within), which means each time someone writes to the conditions, they will take ownership of every condition in the list, so the managed fields will just end up flip-flopping on whoever wrote last

then there is also no reason why we have to make this change now

That is fair, provided we don't lose track of this as we move to the next API version

[sbueringer]

then there is also no reason why we have to make this change now

That is fair, provided we don't lose track of this as we move to the next API version

I think when we implement #10897 we should already add the map key to the new field (which, according to plan, will be already with v1beta1)
(cc @fabriziopandini maybe you can call this out explicitly in the proposal)

[fabriziopandini]

The new proposal already adds listType and listMapKey to conditions:

    // Conditions represent the observations of a Machine's current state.
    // +optional
    // +listType=map
    // +listMapKey=type
    Conditions []metav1.Condition `json:"conditions,omitempty"`

WRT to changing existing types, if the goal is only to avoid duplicates, I'm personally -1 (also considering we are shifting our energies on the transition to the target state defined by #10897)

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.