🐛 handle error in case ownerRef is not set on crs binding

[aartij17]

What this PR does / why we need it:
In case of a backup, the CRS Binding does have a Cluster set in the ownerRef. This leads to a nil pointer exception.
This PR adds a check around the presence of this ownerRef.

Which issue(s) this PR fixes:
Fixes #5961

[k8s-ci-robot]

Hi @AartiJivrajani. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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]

Thanks for the PR @AartiJivrajani! I've taken a quick look and I'll come back to it on Monday.

[killianmuldoon]

Should this be if cluster == nil ?

[aartij17]

You are right @killianmuldoon. My bad.
When the ownerRef is not set, cluster and err would both be nil.

[killianmuldoon]

nit
No need to specify this is a check for the backup case - it's a check we should be doing in all cases.

[killianmuldoon]

Suggested change

	log.Info("ownerRef not found for the clusterResourceSetBinding")
	log.Info("ownerRef not found for the ClusterResourceSetBinding")

[vincepri]

/ok-to-test

[sbueringer]

/cherry-pick release-1.1

[k8s-infra-cherrypick-robot]

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1

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.

[sbueringer]

@AartiJivrajani What do you think about restructuring the whole block?

	if err != nil  {
		if apierrors.IsNotFound(err) {
			// If the owner cluster is already deleted, delete its ClusterResourceSetBinding.
			log.Info("deleting ClusterResourceSetBinding because the owner Cluster no longer exists")
			return ctrl.Result{}, r.Client.Delete(ctx, binding)
		}
		return ctrl.Result{}, err
	}
	
	if cluster == nil {
		log.Info("ownerRef not found for the ClusterResourceSetBinding")
		return ctrl.Result{}, nil
	}
	
	// If the owner cluster is in the deletion process, delete its ClusterResourceSetBinding.
	if !cluster.DeletionTimestamp.IsZero() {
		log.Info("deleting ClusterResourceSetBinding because the owner Cluster is currently being deleted")
		return ctrl.Result{}, r.Client.Delete(ctx, binding)
	}

	return ctrl.Result{}, nil

It just seems easier to me to reason through the various states err and cluster could have when separating the handling for different values of err and cluster like this

P.S. I dropped the // requeue the request comment as we're not requeuing in that case.

[aartij17]

This improves readability a lot, thank you @sbueringer :)

[sbueringer]

@AartiJivrajani Thank you!

[sbueringer]

/lgtm

[sbueringer]

/assign @fabriziopandini

[killianmuldoon]

Thanks for taking this on @AartiJivrajani

/lgtm

[killianmuldoon]

@AartiJivrajani I've noticed that there's a similar missing nil-check at

cluster-api/test/infrastructure/docker/internal/controllers/dockermachine_controller.go

Line 397 in c6e3049

cluster, err := util.GetOwnerCluster(context.TODO(), r.Client, c.ObjectMeta)

Do you want to fix it? It can be part of this PR or in a follow-up

[sbueringer]

@AartiJivrajani I've noticed that there's a similar missing nil-check at

cluster-api/test/infrastructure/docker/internal/controllers/dockermachine_controller.go

Line 397 in c6e3049

cluster, err := util.GetOwnerCluster(context.TODO(), r.Client, c.ObjectMeta)

** ** Do you want to fix it? It can be part of this PR or in a follow-up

If we want to include this fix (cherry-pick) in v1.1 I would prefer a follow-up PR as the current PR already fixes an issue which can occur in production.

[aartij17]

I have created a new issue for this @sbueringer @killianmuldoon -
#5984

[fabriziopandini]

/lgtm
/approve

/cherry-pick release-1.1

[k8s-infra-cherrypick-robot]

@fabriziopandini: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/lgtm
/approve

/cherry-pick release-1.1

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-infra-cherrypick-robot]

@sbueringer: new pull request created: #5996

In response to this:

/cherry-pick release-1.1

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.