From 00646f6bd5a2ba6ddc41e65278347bd08c9bced5 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Thu, 4 Jan 2024 15:37:12 +0100 Subject: [PATCH] Don't rely on GVK being set on regular objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../controllers/kubeadmconfig_controller.go | 4 +-- .../kubeadmconfig_controller_test.go | 8 +++--- .../locking/control_plane_init_mutex.go | 4 +-- .../clusterresourcesetbinding_types.go | 4 +-- .../clusterresourceset_controller.go | 6 ++--- .../controllers/machinepool_controller.go | 2 +- .../machinepool_controller_noderef.go | 4 +-- internal/contract/references.go | 5 ++-- .../clusterclass_controller_test.go | 10 ++++---- .../controllers/clusterclass/suite_test.go | 9 ++++--- .../machine/machine_controller_noderef.go | 4 +-- .../machinedeployment/suite_test.go | 4 +-- internal/controllers/machineset/suite_test.go | 4 +-- .../topology/cluster/desired_state.go | 25 +++++++++++-------- .../topology/cluster/desired_state_test.go | 6 ++--- .../topology/cluster/patches/engine.go | 18 ++++++------- .../topology/cluster/patches/template.go | 9 ++++--- .../topology/cluster/reconcile_state.go | 6 ++--- .../topology/cluster/reconcile_state_test.go | 16 ++++++------ internal/test/builder/builders.go | 5 ++-- .../dockermachinepool_controller_phases.go | 4 +-- .../dockermachine_controller_test.go | 8 +++--- 22 files changed, 88 insertions(+), 77 deletions(-) diff --git a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go index 39138ce78f75..81ba1d1fa52a 100644 --- a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go +++ b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go @@ -416,7 +416,7 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex if scope.Config.Spec.InitConfiguration == nil { scope.Config.Spec.InitConfiguration = &bootstrapv1.InitConfiguration{ TypeMeta: metav1.TypeMeta{ - APIVersion: "kubeadm.k8s.io/v1beta1", + APIVersion: "kubeadm.k8s.io/v1beta3", Kind: "InitConfiguration", }, } @@ -431,7 +431,7 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex if scope.Config.Spec.ClusterConfiguration == nil { scope.Config.Spec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{ TypeMeta: metav1.TypeMeta{ - APIVersion: "kubeadm.k8s.io/v1beta1", + APIVersion: "kubeadm.k8s.io/v1beta3", Kind: "ClusterConfiguration", }, } diff --git a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go index a7690421a155..ea814e14b5b7 100644 --- a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go +++ b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go @@ -142,8 +142,8 @@ func TestKubeadmConfigReconciler_TestSecretOwnerReferenceReconciliation(t *testi config := newKubeadmConfig(metav1.NamespaceDefault, "cfg") config.SetOwnerReferences(util.EnsureOwnerRef(config.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: machine.APIVersion, - Kind: machine.Kind, + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Machine", Name: machine.Name, UID: machine.UID, })) @@ -207,8 +207,8 @@ func TestKubeadmConfigReconciler_TestSecretOwnerReferenceReconciliation(t *testi actual.SetOwnerReferences([]metav1.OwnerReference{ { - APIVersion: machine.APIVersion, - Kind: machine.Kind, + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Machine", Name: machine.Name, UID: machine.UID, Controller: ptr.To(true), diff --git a/bootstrap/kubeadm/internal/locking/control_plane_init_mutex.go b/bootstrap/kubeadm/internal/locking/control_plane_init_mutex.go index ace07f149c34..ff249e4ccbd7 100644 --- a/bootstrap/kubeadm/internal/locking/control_plane_init_mutex.go +++ b/bootstrap/kubeadm/internal/locking/control_plane_init_mutex.go @@ -181,8 +181,8 @@ func (s *semaphore) setMetadata(cluster *clusterv1.Cluster) { }, OwnerReferences: []metav1.OwnerReference{ { - APIVersion: cluster.APIVersion, - Kind: cluster.Kind, + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", Name: cluster.Name, UID: cluster.UID, }, diff --git a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go index 37591da520cd..ccae4cfa6f13 100644 --- a/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go +++ b/exp/addons/api/v1beta1/clusterresourcesetbinding_types.go @@ -119,8 +119,8 @@ func (c *ClusterResourceSetBinding) DeleteBinding(clusterResourceSet *ClusterRes } } c.OwnerReferences = removeOwnerRef(c.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: clusterResourceSet.APIVersion, - Kind: clusterResourceSet.Kind, + APIVersion: GroupVersion.String(), + Kind: "ClusterResourceSet", Name: clusterResourceSet.Name, }) } diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 2713dba76864..a9febf3242f9 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -200,8 +200,8 @@ func (r *ClusterResourceSetReconciler) reconcileDelete(ctx context.Context, clus clusterResourceSetBinding.RemoveBinding(crs) clusterResourceSetBinding.OwnerReferences = util.RemoveOwnerRef(clusterResourceSetBinding.GetOwnerReferences(), metav1.OwnerReference{ - APIVersion: crs.APIVersion, - Kind: crs.Kind, + APIVersion: addonsv1.GroupVersion.String(), + Kind: "ClusterResourceSet", Name: crs.Name, }) @@ -296,7 +296,7 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte // Ensure that the owner references are set on the ClusterResourceSetBinding. clusterResourceSetBinding.SetOwnerReferences(util.EnsureOwnerRef(clusterResourceSetBinding.GetOwnerReferences(), metav1.OwnerReference{ APIVersion: addonsv1.GroupVersion.String(), - Kind: clusterResourceSet.Kind, + Kind: "ClusterResourceSet", Name: clusterResourceSet.Name, UID: clusterResourceSet.UID, })) diff --git a/exp/internal/controllers/machinepool_controller.go b/exp/internal/controllers/machinepool_controller.go index fc89c4970a57..dadaf09a8d8d 100644 --- a/exp/internal/controllers/machinepool_controller.go +++ b/exp/internal/controllers/machinepool_controller.go @@ -225,7 +225,7 @@ func (r *MachinePoolReconciler) reconcile(ctx context.Context, cluster *clusterv // Ensure the MachinePool is owned by the Cluster it belongs to. mp.SetOwnerReferences(util.EnsureOwnerRef(mp.GetOwnerReferences(), metav1.OwnerReference{ APIVersion: clusterv1.GroupVersion.String(), - Kind: cluster.Kind, + Kind: "Cluster", Name: cluster.Name, UID: cluster.UID, })) diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index 15791b2cdb85..c6caee55fe03 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -189,8 +189,8 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client. ready++ } nodeRefs = append(nodeRefs, corev1.ObjectReference{ - Kind: node.Kind, - APIVersion: node.APIVersion, + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Node", Name: node.Name, UID: node.UID, }) diff --git a/internal/contract/references.go b/internal/contract/references.go index b81c8687d763..8f6e68bb403d 100644 --- a/internal/contract/references.go +++ b/internal/contract/references.go @@ -23,7 +23,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/client" ) // Ref provide a helper struct for working with references in Unstructured objects. @@ -88,7 +87,9 @@ func SetNestedRef(obj, refObj *unstructured.Unstructured, fields ...string) erro } // ObjToRef returns a reference to the given object. -func ObjToRef(obj client.Object) *corev1.ObjectReference { +// Note: This function only operates on Unstructured instead of client.Object +// because it is only safe to assume for Unstructured that the GVK is set. +func ObjToRef(obj *unstructured.Unstructured) *corev1.ObjectReference { gvk := obj.GetObjectKind().GroupVersionKind() return &corev1.ObjectReference{ Kind: gvk.Kind, diff --git a/internal/controllers/clusterclass/clusterclass_controller_test.go b/internal/controllers/clusterclass/clusterclass_controller_test.go index cf1614c39dd4..6301b56eeeae 100644 --- a/internal/controllers/clusterclass/clusterclass_controller_test.go +++ b/internal/controllers/clusterclass/clusterclass_controller_test.go @@ -185,7 +185,7 @@ func assertInfrastructureClusterTemplate(ctx context.Context, actualClusterClass if err := env.Get(ctx, actualInfraClusterTemplateKey, actualInfraClusterTemplate); err != nil { return err } - if err := assertHasOwnerReference(actualInfraClusterTemplate, *ownerReferenceTo(actualClusterClass)); err != nil { + if err := assertHasOwnerReference(actualInfraClusterTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil { return err } @@ -205,7 +205,7 @@ func assertControlPlaneTemplate(ctx context.Context, actualClusterClass *cluster if err := env.Get(ctx, actualControlPlaneTemplateKey, actualControlPlaneTemplate); err != nil { return err } - if err := assertHasOwnerReference(actualControlPlaneTemplate, *ownerReferenceTo(actualClusterClass)); err != nil { + if err := assertHasOwnerReference(actualControlPlaneTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil { return err } @@ -226,7 +226,7 @@ func assertControlPlaneTemplate(ctx context.Context, actualClusterClass *cluster if err := env.Get(ctx, actualInfrastructureMachineTemplateKey, actualInfrastructureMachineTemplate); err != nil { return err } - if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass)); err != nil { + if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil { return err } @@ -260,7 +260,7 @@ func assertMachineDeploymentClass(ctx context.Context, actualClusterClass *clust if err := env.Get(ctx, actualInfrastructureMachineTemplateKey, actualInfrastructureMachineTemplate); err != nil { return err } - if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass)); err != nil { + if err := assertHasOwnerReference(actualInfrastructureMachineTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil { return err } @@ -280,7 +280,7 @@ func assertMachineDeploymentClass(ctx context.Context, actualClusterClass *clust if err := env.Get(ctx, actualBootstrapTemplateKey, actualBootstrapTemplate); err != nil { return err } - if err := assertHasOwnerReference(actualBootstrapTemplate, *ownerReferenceTo(actualClusterClass)); err != nil { + if err := assertHasOwnerReference(actualBootstrapTemplate, *ownerReferenceTo(actualClusterClass, clusterv1.GroupVersion.WithKind("ClusterClass"))); err != nil { return err } diff --git a/internal/controllers/clusterclass/suite_test.go b/internal/controllers/clusterclass/suite_test.go index 525202211bea..41f0dc2004a6 100644 --- a/internal/controllers/clusterclass/suite_test.go +++ b/internal/controllers/clusterclass/suite_test.go @@ -90,12 +90,15 @@ func TestMain(m *testing.M) { })) } -func ownerReferenceTo(obj client.Object) *metav1.OwnerReference { +// ownerReferenceTo converts an object to an OwnerReference. +// Note: We pass in gvk explicitly as we can't rely on GVK being set on all objects +// (only on Unstructured). +func ownerReferenceTo(obj client.Object, gvk schema.GroupVersionKind) *metav1.OwnerReference { return &metav1.OwnerReference{ - Kind: obj.GetObjectKind().GroupVersionKind().Kind, + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, Name: obj.GetName(), UID: obj.GetUID(), - APIVersion: obj.GetObjectKind().GroupVersionKind().GroupVersion().String(), } } diff --git a/internal/controllers/machine/machine_controller_noderef.go b/internal/controllers/machine/machine_controller_noderef.go index 7f32c72c48c5..adf08c5bf7bf 100644 --- a/internal/controllers/machine/machine_controller_noderef.go +++ b/internal/controllers/machine/machine_controller_noderef.go @@ -87,8 +87,8 @@ func (r *Reconciler) reconcileNode(ctx context.Context, s *scope) (ctrl.Result, // Set the Machine NodeRef. if machine.Status.NodeRef == nil { machine.Status.NodeRef = &corev1.ObjectReference{ - Kind: node.Kind, - APIVersion: node.APIVersion, + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Node", Name: node.Name, UID: node.UID, } diff --git a/internal/controllers/machinedeployment/suite_test.go b/internal/controllers/machinedeployment/suite_test.go index 88eb100bfb0b..0c3058652ca2 100644 --- a/internal/controllers/machinedeployment/suite_test.go +++ b/internal/controllers/machinedeployment/suite_test.go @@ -196,8 +196,8 @@ func fakeMachineNodeRef(m *clusterv1.Machine, pid string, g *WithT) { patchMachine = client.MergeFrom(m.DeepCopy()) m.Status.NodeRef = &corev1.ObjectReference{ - APIVersion: node.APIVersion, - Kind: node.Kind, + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Node", Name: node.Name, } g.Expect(env.Status().Patch(ctx, m, patchMachine)).To(Succeed()) diff --git a/internal/controllers/machineset/suite_test.go b/internal/controllers/machineset/suite_test.go index 782885a77a7f..f45ed3ad6013 100644 --- a/internal/controllers/machineset/suite_test.go +++ b/internal/controllers/machineset/suite_test.go @@ -205,8 +205,8 @@ func fakeMachineNodeRef(m *clusterv1.Machine, pid string, g *WithT) { patchMachine = client.MergeFrom(m.DeepCopy()) m.Status.NodeRef = &corev1.ObjectReference{ - APIVersion: node.APIVersion, - Kind: node.Kind, + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Node", Name: node.Name, } g.Expect(env.Status().Patch(ctx, m, patchMachine)).To(Succeed()) diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index b2d56865a36d..34a7b52a24f9 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -208,7 +208,7 @@ func computeControlPlaneInfrastructureMachineTemplate(_ context.Context, s *scop // Note: we are adding an ownerRef to Cluster so the template will be automatically garbage collected // in case of errors in between creating this template and updating the Cluster object // with the reference to the ControlPlane object using this template. - ownerRef: ownerReferenceTo(s.Current.Cluster), + ownerRef: ownerReferenceTo(s.Current.Cluster, clusterv1.GroupVersion.WithKind("Cluster")), }) } @@ -623,7 +623,7 @@ func computeMachineDeployment(ctx context.Context, s *scope.Scope, machineDeploy // Note: we are adding an ownerRef to Cluster so the template will be automatically garbage collected // in case of errors in between creating this template and creating/updating the MachineDeployment object // with the reference to this template. - ownerRef: ownerReferenceTo(s.Current.Cluster), + ownerRef: ownerReferenceTo(s.Current.Cluster, clusterv1.GroupVersion.WithKind("Cluster")), }) if err != nil { return nil, err @@ -651,7 +651,7 @@ func computeMachineDeployment(ctx context.Context, s *scope.Scope, machineDeploy // Note: we are adding an ownerRef to Cluster so the template will be automatically garbage collected // in case of errors in between creating this template and creating/updating the MachineDeployment object // with the reference to this template. - ownerRef: ownerReferenceTo(s.Current.Cluster), + ownerRef: ownerReferenceTo(s.Current.Cluster, clusterv1.GroupVersion.WithKind("Cluster")), }) if err != nil { return nil, err @@ -719,8 +719,8 @@ func computeMachineDeployment(ctx context.Context, s *scope.Scope, machineDeploy desiredMachineDeploymentObj := &clusterv1.MachineDeployment{ TypeMeta: metav1.TypeMeta{ - Kind: clusterv1.GroupVersion.WithKind("MachineDeployment").Kind, APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", }, ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -977,7 +977,7 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c // Note: we are adding an ownerRef to Cluster so the template will be automatically garbage collected // in case of errors in between creating this template and creating/updating the MachinePool object // with the reference to this template. - ownerRef: ownerReferenceTo(s.Current.Cluster), + ownerRef: ownerReferenceTo(s.Current.Cluster, clusterv1.GroupVersion.WithKind("Cluster")), }) if err != nil { return nil, errors.Wrapf(err, "failed to compute bootstrap object for topology %q", machinePoolTopology.Name) @@ -1005,7 +1005,7 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c // Note: we are adding an ownerRef to Cluster so the template will be automatically garbage collected // in case of errors in between creating this template and creating/updating the MachinePool object // with the reference to this template. - ownerRef: ownerReferenceTo(s.Current.Cluster), + ownerRef: ownerReferenceTo(s.Current.Cluster, clusterv1.GroupVersion.WithKind("Cluster")), }) if err != nil { return nil, errors.Wrapf(err, "failed to compute infrastructure object for topology %q", machinePoolTopology.Name) @@ -1068,8 +1068,8 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c desiredMachinePoolObj := &expv1.MachinePool{ TypeMeta: metav1.TypeMeta{ - Kind: expv1.GroupVersion.WithKind("MachinePool").Kind, APIVersion: expv1.GroupVersion.String(), + Kind: "MachinePool", }, ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -1345,12 +1345,15 @@ func templateToTemplate(in templateToInput) (*unstructured.Unstructured, error) return template, nil } -func ownerReferenceTo(obj client.Object) *metav1.OwnerReference { +// ownerReferenceTo converts an object to an OwnerReference. +// Note: We pass in gvk explicitly as we can't rely on GVK being set on all objects +// (only on Unstructured). +func ownerReferenceTo(obj client.Object, gvk schema.GroupVersionKind) *metav1.OwnerReference { return &metav1.OwnerReference{ - Kind: obj.GetObjectKind().GroupVersionKind().Kind, + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, Name: obj.GetName(), UID: obj.GetUID(), - APIVersion: obj.GetObjectKind().GroupVersionKind().GroupVersion().String(), } } @@ -1358,8 +1361,8 @@ func computeMachineHealthCheck(ctx context.Context, healthCheckTarget client.Obj // Create a MachineHealthCheck with the spec given in the ClusterClass. mhc := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ - Kind: clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind, APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineHealthCheck", }, ObjectMeta: metav1.ObjectMeta{ Name: healthCheckTarget.GetName(), diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index f304450fe192..a4d1e3d5e4bc 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -144,7 +144,7 @@ func TestComputeInfrastructureCluster(t *testing.T) { // aggregating current cluster objects into ClusterState (simulating getCurrentState) scope := scope.New(clusterWithInfrastructureRef) scope.Current.InfrastructureCluster = infrastructureClusterTemplate.DeepCopy() - scope.Current.InfrastructureCluster.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim)}) + scope.Current.InfrastructureCluster.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))}) scope.Blueprint = blueprint obj, err := computeInfrastructureCluster(ctx, scope) @@ -623,7 +623,7 @@ func TestComputeControlPlane(t *testing.T) { }). Build(), } - s.Current.ControlPlane.Object.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim)}) + s.Current.ControlPlane.Object.SetOwnerReferences([]metav1.OwnerReference{*ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))}) s.Blueprint = blueprint r := &Reconciler{} @@ -2877,8 +2877,8 @@ func Test_computeMachineHealthCheck(t *testing.T) { clusterName := "cluster1" want := &clusterv1.MachineHealthCheck{ TypeMeta: metav1.TypeMeta{ - Kind: clusterv1.GroupVersion.WithKind("MachineHealthCheck").Kind, APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineHealthCheck", }, ObjectMeta: metav1.ObjectMeta{ Name: "md1", diff --git a/internal/controllers/topology/cluster/patches/engine.go b/internal/controllers/topology/cluster/patches/engine.go index 54bfe9cb0b55..da55a7a03689 100644 --- a/internal/controllers/topology/cluster/patches/engine.go +++ b/internal/controllers/topology/cluster/patches/engine.go @@ -193,7 +193,7 @@ func addVariablesForPatch(blueprint *scope.ClusterBlueprint, desired *scope.Clus if item.HolderReference.Kind == "MachineDeployment" { md, ok := mdStateIndex[item.HolderReference.Name] if !ok { - return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KObj(md.Object)) + return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KRef(item.HolderReference.Namespace, item.HolderReference.Name)) } mdTopology, err := getMDTopologyFromMD(blueprint, md.Object) if err != nil { @@ -209,7 +209,7 @@ func addVariablesForPatch(blueprint *scope.ClusterBlueprint, desired *scope.Clus } else if item.HolderReference.Kind == "MachinePool" { mp, ok := mpStateIndex[item.HolderReference.Name] if !ok { - return errors.Errorf("could not find desired state for MachinePool %s", klog.KObj(mp.Object)) + return errors.Errorf("could not find desired state for MachinePool %s", klog.KRef(item.HolderReference.Namespace, item.HolderReference.Name)) } mpTopology, err := getMPTopologyFromMP(blueprint, mp.Object) if err != nil { @@ -263,7 +263,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the InfrastructureClusterTemplate. t, err := newRequestItemBuilder(blueprint.InfrastructureClusterTemplate). - WithHolder(desired.Cluster, "spec.infrastructureRef"). + WithHolder(desired.Cluster, clusterv1.GroupVersion.WithKind("Cluster"), "spec.infrastructureRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare InfrastructureCluster template %s for patching", @@ -273,7 +273,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the ControlPlaneTemplate. t, err = newRequestItemBuilder(blueprint.ControlPlane.Template). - WithHolder(desired.Cluster, "spec.controlPlaneRef"). + WithHolder(desired.Cluster, clusterv1.GroupVersion.WithKind("Cluster"), "spec.controlPlaneRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare ControlPlane template %s for patching", @@ -285,7 +285,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // add the InfrastructureMachineTemplate for control plane machines. if blueprint.HasControlPlaneInfrastructureMachine() { t, err := newRequestItemBuilder(blueprint.ControlPlane.InfrastructureMachineTemplate). - WithHolder(desired.ControlPlane.Object, strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), ".")). + WithHolder(desired.ControlPlane.Object, desired.ControlPlane.Object.GroupVersionKind(), strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), ".")). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare ControlPlane's machine template %s for patching", @@ -315,7 +315,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the BootstrapTemplate. t, err := newRequestItemBuilder(mdClass.BootstrapTemplate). - WithHolder(md.Object, "spec.template.spec.bootstrap.configRef"). + WithHolder(md.Object, clusterv1.GroupVersion.WithKind("MachineDeployment"), "spec.template.spec.bootstrap.configRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare BootstrapConfig template %s for MachineDeployment topology %s for patching", @@ -325,7 +325,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the InfrastructureMachineTemplate. t, err = newRequestItemBuilder(mdClass.InfrastructureMachineTemplate). - WithHolder(md.Object, "spec.template.spec.infrastructureRef"). + WithHolder(md.Object, clusterv1.GroupVersion.WithKind("MachineDeployment"), "spec.template.spec.infrastructureRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare InfrastructureMachine template %s for MachineDeployment topology %s for patching", @@ -355,7 +355,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the BootstrapTemplate. t, err := newRequestItemBuilder(mpClass.BootstrapTemplate). - WithHolder(mp.Object, "spec.template.spec.bootstrap.configRef"). + WithHolder(mp.Object, expv1.GroupVersion.WithKind("MachinePool"), "spec.template.spec.bootstrap.configRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare BootstrapConfig template %s for MachinePool topology %s for patching", @@ -365,7 +365,7 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat // Add the InfrastructureMachineTemplate. t, err = newRequestItemBuilder(mpClass.InfrastructureMachinePoolTemplate). - WithHolder(mp.Object, "spec.template.spec.infrastructureRef"). + WithHolder(mp.Object, expv1.GroupVersion.WithKind("MachinePool"), "spec.template.spec.infrastructureRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare InfrastructureMachinePoolTemplate %s for MachinePool topology %s for patching", diff --git a/internal/controllers/topology/cluster/patches/template.go b/internal/controllers/topology/cluster/patches/template.go index 55264cdfdd02..2029aa818c62 100644 --- a/internal/controllers/topology/cluster/patches/template.go +++ b/internal/controllers/topology/cluster/patches/template.go @@ -24,6 +24,7 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/uuid" @@ -56,10 +57,12 @@ func newRequestItemBuilder(template *unstructured.Unstructured) *requestItemBuil } // WithHolder adds holder to the requestItemBuilder. -func (t *requestItemBuilder) WithHolder(object client.Object, fieldPath string) *requestItemBuilder { +// Note: We pass in gvk explicitly as we can't rely on GVK being set on all objects +// (only on Unstructured). +func (t *requestItemBuilder) WithHolder(object client.Object, gvk schema.GroupVersionKind, fieldPath string) *requestItemBuilder { t.holder = runtimehooksv1.HolderReference{ - APIVersion: object.GetObjectKind().GroupVersionKind().GroupVersion().String(), - Kind: object.GetObjectKind().GroupVersionKind().Kind, + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, Namespace: object.GetNamespace(), Name: object.GetName(), FieldPath: fieldPath, diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 014523c88c3d..a375f58bf9b6 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -126,12 +126,12 @@ func (r *Reconciler) reconcileClusterShim(ctx context.Context, s *scope.Scope) e // Add the shim as a temporary owner for the InfrastructureCluster. ownerRefs := s.Desired.InfrastructureCluster.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(shim)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))) s.Desired.InfrastructureCluster.SetOwnerReferences(ownerRefs) // Add the shim as a temporary owner for the ControlPlane. ownerRefs = s.Desired.ControlPlane.Object.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(shim)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(shim, corev1.SchemeGroupVersion.WithKind("Secret"))) s.Desired.ControlPlane.Object.SetOwnerReferences(ownerRefs) } @@ -168,7 +168,7 @@ func clusterShim(c *clusterv1.Cluster) *corev1.Secret { Name: fmt.Sprintf("%s-shim", c.Name), Namespace: c.Namespace, OwnerReferences: []metav1.OwnerReference{ - *ownerReferenceTo(c), + *ownerReferenceTo(c, clusterv1.GroupVersion.WithKind("Cluster")), }, }, Type: clusterv1.ClusterSecretType, diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 80e33b5f9f59..7dffa9f535be 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -174,10 +174,10 @@ func TestReconcileShim(t *testing.T) { // Add the shim as a temporary owner for the InfrastructureCluster and ControlPlane. ownerRefs := s.Current.InfrastructureCluster.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1Shim)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1Shim, corev1.SchemeGroupVersion.WithKind("Secret"))) s.Current.InfrastructureCluster.SetOwnerReferences(ownerRefs) ownerRefs = s.Current.ControlPlane.Object.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1Shim)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1Shim, corev1.SchemeGroupVersion.WithKind("Secret"))) s.Current.ControlPlane.Object.SetOwnerReferences(ownerRefs) // Pre-create a shim @@ -221,14 +221,14 @@ func TestReconcileShim(t *testing.T) { ownerRefs := s.Current.InfrastructureCluster.GetOwnerReferences() ownerRefs = append( ownerRefs, - *ownerReferenceTo(cluster1Shim), - *ownerReferenceTo(cluster1)) + *ownerReferenceTo(cluster1Shim, corev1.SchemeGroupVersion.WithKind("Secret")), + *ownerReferenceTo(cluster1, clusterv1.GroupVersion.WithKind("Cluster"))) s.Current.InfrastructureCluster.SetOwnerReferences(ownerRefs) ownerRefs = s.Current.ControlPlane.Object.GetOwnerReferences() ownerRefs = append( ownerRefs, - *ownerReferenceTo(cluster1Shim), - *ownerReferenceTo(cluster1)) + *ownerReferenceTo(cluster1Shim, corev1.SchemeGroupVersion.WithKind("Secret")), + *ownerReferenceTo(cluster1, clusterv1.GroupVersion.WithKind("Cluster"))) s.Current.ControlPlane.Object.SetOwnerReferences(ownerRefs) // Pre-create a shim @@ -269,10 +269,10 @@ func TestReconcileShim(t *testing.T) { // Add the cluster as a final owner for the InfrastructureCluster and ControlPlane (reconciled). ownerRefs := s.Current.InfrastructureCluster.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1, clusterv1.GroupVersion.WithKind("Cluster"))) s.Current.InfrastructureCluster.SetOwnerReferences(ownerRefs) ownerRefs = s.Current.ControlPlane.Object.GetOwnerReferences() - ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1)) + ownerRefs = append(ownerRefs, *ownerReferenceTo(cluster1, clusterv1.GroupVersion.WithKind("Cluster"))) s.Current.ControlPlane.Object.SetOwnerReferences(ownerRefs) // Run reconcileClusterShim using a nil client, so an error will be triggered if any operation is attempted diff --git a/internal/test/builder/builders.go b/internal/test/builder/builders.go index 24e00c201161..0b9cb6567e6b 100644 --- a/internal/test/builder/builders.go +++ b/internal/test/builder/builders.go @@ -24,7 +24,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" - "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" @@ -1859,7 +1858,9 @@ func (m *MachineBuilder) Build() *clusterv1.Machine { } // objToRef returns a reference to the given object. -func objToRef(obj client.Object) *corev1.ObjectReference { +// Note: This function only operates on Unstructured instead of client.Object +// because it is only safe to assume for Unstructured that the GVK is set. +func objToRef(obj *unstructured.Unstructured) *corev1.ObjectReference { gvk := obj.GetObjectKind().GroupVersionKind() return &corev1.ObjectReference{ Kind: gvk.Kind, diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go index 414a44c164e7..73d70380f714 100644 --- a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go +++ b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go @@ -264,8 +264,8 @@ func computeDesiredDockerMachine(name string, cluster *clusterv1.Cluster, machin // Note: Since the MachinePool controller has not created its owner Machine yet, we want to set the DockerMachinePool as the owner so it's not orphaned. dockerMachine.SetOwnerReferences(util.EnsureOwnerRef(dockerMachine.OwnerReferences, metav1.OwnerReference{ - APIVersion: dockerMachinePool.APIVersion, - Kind: dockerMachinePool.Kind, + APIVersion: infraexpv1.GroupVersion.String(), + Kind: "DockerMachinePool", Name: dockerMachinePool.Name, UID: dockerMachinePool.UID, })) diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller_test.go b/test/infrastructure/docker/internal/controllers/dockermachine_controller_test.go index e6f92db96367..83c8750b7685 100644 --- a/test/infrastructure/docker/internal/controllers/dockermachine_controller_test.go +++ b/test/infrastructure/docker/internal/controllers/dockermachine_controller_test.go @@ -75,10 +75,10 @@ func newCluster(clusterName string, dockerCluster *infrav1.DockerCluster) *clust } if dockerCluster != nil { cluster.Spec.InfrastructureRef = &corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "DockerCluster", Name: dockerCluster.Name, Namespace: dockerCluster.Namespace, - Kind: dockerCluster.Kind, - APIVersion: dockerCluster.GroupVersionKind().GroupVersion().String(), } } return cluster @@ -111,10 +111,10 @@ func newMachine(clusterName, machineName string, dockerMachine *infrav1.DockerMa } if dockerMachine != nil { machine.Spec.InfrastructureRef = corev1.ObjectReference{ + APIVersion: infrav1.GroupVersion.String(), + Kind: "DockerMachine", Name: dockerMachine.Name, Namespace: dockerMachine.Namespace, - Kind: dockerMachine.Kind, - APIVersion: dockerMachine.GroupVersionKind().GroupVersion().String(), } } return machine