Stop using owner references to determine ClusterResourceSetBinding cluster

[killianmuldoon]

The current implementation of the ClusterResourceSetBinding controller uses the owner references in objectMeta to determine which cluster the resource binding applies to.

As a result the ClusterResourceSetBinding requires two ownerReferences - one to the ClusterResourceSet and a second to the Cluster.

Currently the ClusterResourceSetBinding is always named after the Cluster it's associated with, so this name could be used to link the two. Alternatively the ClusterResourceSetBinding .spec could contain a clusterName field to link it to the Cluster. The second approach is more stable and explicit IMO.

The approach I'd propose is:

1. add a clusterName to the ClusterResourceSetBinding spec
2. use spec.clusterName to link ClusterResourceSetBinding to the Cluster's they link to - and use that link to decide when to delete.
3. remove the Cluster owner reference from the ClusterResourceSetBinding

[sbueringer]

Sounds fine to me in general.

Do you know when we currently use .spec.clusterName vs the cluster name label?

I think ~ core CRDs are using the spec field? (not sure IIRC)

[killianmuldoon]

We do it in MachinePools in exp - but not in KCP or CAPBK .

[fabriziopandini]

+1 to add a field; we can infer the value from the ownerRef while dropping it from existing objects.

/triage accepted
/help

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

+1 to add a field; we can infer the value from the ownerRef while dropping it from existing objects.

/triage accepted
/help

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/test-infra repository.

[killianmuldoon]

+1 to add a field; we can infer the value from the ownerRef while dropping it from existing objects.

I think we can update the field in the CRS controller to update existing CRSbindings with the new field.

[chaunceyjiang]

hi, I'm a newbie. Can I pick up this issues? I'm not sure I can solve it.

[chaunceyjiang]

Four Tasks:

• add a clusterName to the ClusterResourceSetBinding spec.
• update the field in the CRS controller to update existing CRSbindings with the new field.
• remove the Cluster owner reference from the ClusterResourceSetBinding.
• use that link to decide when to delete.
• adding a check in the validation webhook preventing this value to be changed/removed once it is set

[killianmuldoon]

Thanks for volunteering @chaunceyjiang - I think we're agreed on the approach, so it would be awesome if you could pick this up!

/assign @chaunceyjiang

[sbueringer]

@killianmuldoon Can we close this given #7680 has been merged?

[killianmuldoon]

Yup - this is fully covered.

Thanks again @chaunceyjiang!

/close

[k8s-ci-robot]

@killianmuldoon: Closing this issue.

In response to this:

Yup - this is fully covered.

Thanks again @chaunceyjiang!

/close

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/test-infra repository.