🐛 Fix potential panic if ClusterResourceSetStrategy is not defined or incorrect

[dmvolod]

What this PR does / why we need it:
This is small fix which prevents potential situation when resourceScope and error values could be nil simalteniously.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[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 fabriziopandini 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

[dmvolod]

/area clusterresourceset

[chrischdi]

Does this PR make sense?

We have openapi validation to ensure it is either set to ApplyOnce or to Reconcile.

On top we have defaulting inside the webhook to default to ApplyOnce it it is not set.

or am I missing something here?

[dmvolod]

Does this PR make sense?

We have openapi validation to ensure it is either set to ApplyOnce or to Reconcile.

On top we have defaulting inside the webhook to default to ApplyOnce it it is not set.

or am I missing something here?

The answer: Yes and No. We have some specific installation, i.e. testing or embedded without web-hooks deployment and in this case, the operator will fails with panic instead of correct error message in the logs. For example MachineDeployment or ClusterResourceSet controllers will have correct error message if someone happens with defaults.

[fabriziopandini]

I'm neutral about this change, if other maintainers agree I don't mind considering it as defensive programming.

However IMO it is important to set expectations about the use case described in the sentence below:

We have some specific installation, i.e. testing or embedded without web-hooks deployment and in this case, the operator will fails with panic instead of correct error message in the logs. For example MachineDeployment or ClusterResourceSet controllers will have correct error message if someone happens with defaults.

As far as I'm aware of today CAPI is not providing any support for running controllers without web-hooks, and unless I'm mistaken, this was never discussed nor included in any roadmap.

That's means that If you are using CAPI controllers without web-hooks, you are doing this at your risk.

I would also add that IMO if other controllers don't have panic errors when used without web hooks it is just by chance, as well as I would expect other subtle and tricky errors to happen over time due to unexpected values being stored in etcd.