🐛 Use namespace of the reference on external.Get

[Danil-Grigorev]

What this PR does / why we need it:

Using external.Get method may be problematic when the resources referenced are located in a different namespace from the originating resource, leading to not found errors on the lookup attempts.

Template definitions provide all information, including namespace of the object in the references, so using those instead.

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

/area clusterclass
/area api

[Danil-Grigorev]

/test pull-cluster-api-e2e-mink8s-main

[sbueringer]

/test pull-cluster-api-e2e-mink8s-main

[sbueringer]

Thx, one nit

(if e2e tests work)

[sbueringer]

/test pull-cluster-api-e2e-main

[Danil-Grigorev]

/test pull-cluster-api-e2e-main

[sbueringer]

Pretty far up in my review TODO list now. @Danil-Grigorev Can you please rebase?

[sbueringer]

/test pull-cluster-api-e2e-main

[furkatgofurov7]

Looks good to me

[sbueringer]

@Danil-Grigorev Did some analysis (sharing here for future-us :))

Ref fields with namespace defaulting before this PR:

• Cluster.Spec.InfrastructureRef
• Cluster.Spec.ControlPlaneRef
• Machine.Spec.InfrastructureRef
• Machine.Spec.Bootstrap.ConfigRef
• MachinePool.Spec.Template.Spec.Bootstrap.ConfigRef
• MachinePool.Spec.Template.Spec.InfrastructureRef
• MHC.Spec.RemediationTemplate
• KCP.spec.machineTemplate.infrastructureRef
• ClusterClass (all refs)

Namespace fields for which we have to add defaulting:

• MachineDeployment (not used anymore for rollout decisions, namespace is set in most cases by Cluster topology controller)
  • Defaulting already added with this PR
  • DeepCopy ref and set namespace if empty directly before external.Get (in reconcileExternalTemplateReference)
• MachineSet (not used anymore for rollout decisions, namespace is set in most cases by the MD controller)
  • Defaulting already added with this PR
  • DeepCopy ref and set namespace if empty directly before external.Get (in reconcileExternalTemplateReference)

So I think we're almost good. I think we just can't assume that the defaulting/mutating webhook always run before our controllers (e.g. if a MD/MS is never modified). I think for this case it would be good to add safeguards in the reconcileExternalTemplateReference funcs to set the namespace if it is empty directly before we use it (but after UpdateReferenceAPIContract))

[sbueringer]

(DeepCopy has been added now, updated the tasks accordingly)

[k8s-ci-robot]

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	ca36516	link	false	/test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[sbueringer]

/test pull-cluster-api-e2e-main

Thank you very much!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b688a46b1bcb5744c4ee8aa65c01efc8883d9392

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 [sbueringer]

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