Merge pull request kubernetes-sigs#6646 from Nordix/add-checks-for-not-topology-owned-templates-to-never-reconcile/adil

✨ Add checks for not topology owned templates to never reconcile.

Author: k8s-ci-robot

internal/controllers/topology/cluster/current_state.go

@@ -81,7 +81,7 @@ func (r *Reconciler) getCurrentInfrastructureClusterState(ctx context.Context, c
 	// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
 	// owned by the topology.
 	if !labels.IsTopologyOwned(infra) {
-		return nil, fmt.Errorf("referenced infra cluster object %s is not topology owned", tlog.KObj{Obj: infra})
+		return nil, fmt.Errorf("infra cluster object %s referenced from cluster %s is not topology owned", tlog.KObj{Obj: infra}, tlog.KObj{Obj: cluster})
 	}
 	return infra, nil
 }
@@ -102,7 +102,7 @@ func (r *Reconciler) getCurrentControlPlaneState(ctx context.Context, cluster *c
 	// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
 	// owned by the topology.
 	if !labels.IsTopologyOwned(res.Object) {
-		return nil, fmt.Errorf("referenced control plane object %s is not topology owned", tlog.KObj{Obj: res.Object})
+		return nil, fmt.Errorf("control plane object %s referenced from cluster %s is not topology owned", tlog.KObj{Obj: res.Object}, tlog.KObj{Obj: cluster})
 	}
 
 	// If the clusterClass does not mandate the controlPlane has infrastructureMachines, return.
@@ -119,6 +119,12 @@ func (r *Reconciler) getCurrentControlPlaneState(ctx context.Context, cluster *c
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate for %s", tlog.KObj{Obj: res.Object})
 	}
+	// check that the referenced object has the ClusterTopologyOwnedLabel label.
+	// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
+	// owned by the topology.
+	if !labels.IsTopologyOwned(res.InfrastructureMachineTemplate) {
+		return nil, fmt.Errorf("control plane InfrastructureMachineTemplate object %s referenced from cluster %s is not topology owned", tlog.KObj{Obj: res.InfrastructureMachineTemplate}, tlog.KObj{Obj: cluster})
+	}
 
 	mhc := &clusterv1.MachineHealthCheck{}
 	// MachineHealthCheck always has the same name and namespace as the ControlPlane object it belongs to.
@@ -181,6 +187,12 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, clust
 		if err != nil {
 			return nil, errors.Wrap(err, fmt.Sprintf("%s Bootstrap reference could not be retrieved", tlog.KObj{Obj: m}))
 		}
+		// check that the referenced object has the ClusterTopologyOwnedLabel label.
+		// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
+		// owned by the topology.
+		if !labels.IsTopologyOwned(b) {
+			return nil, fmt.Errorf("BootstrapTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: b}, tlog.KObj{Obj: m})
+		}
 
 		// Gets the InfrastructureMachineTemplate
 		infraRef := m.Spec.Template.Spec.InfrastructureRef
@@ -191,6 +203,12 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, clust
 		if err != nil {
 			return nil, errors.Wrap(err, fmt.Sprintf("%s Infrastructure reference could not be retrieved", tlog.KObj{Obj: m}))
 		}
+		// check that the referenced object has the ClusterTopologyOwnedLabel label.
+		// Nb. This is to make sure that a managed topology cluster does not have a reference to an object that is not
+		// owned by the topology.
+		if !labels.IsTopologyOwned(infra) {
+			return nil, fmt.Errorf("InfrastructureMachineTemplate object %s referenced from MD %s is not topology owned", tlog.KObj{Obj: infra}, tlog.KObj{Obj: m})
+		}
 
 		// Gets the MachineHealthCheck.
 		mhc := &clusterv1.MachineHealthCheck{}

internal/controllers/topology/cluster/current_state_test.go

@@ -57,17 +57,25 @@ func TestGetCurrentState(t *testing.T) {
 	// ControlPlane and ControlPlaneInfrastructureMachineTemplate objects.
 	controlPlaneInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfraTemplate").
 		Build()
+	controlPlaneInfrastructureMachineTemplate.SetLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""})
+	controlPlaneInfrastructureMachineTemplateNotTopologyOwned := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfraTemplate").
+		Build()
 	controlPlaneTemplateWithInfrastructureMachine := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cpTemplateWithInfra1").
 		WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate).
 		Build()
+	controlPlaneTemplateWithInfrastructureMachineNotTopologyOwned := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cpTemplateWithInfra1").
+		WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplateNotTopologyOwned).
+		Build()
 	controlPlane := builder.ControlPlane(metav1.NamespaceDefault, "cp1").
 		Build()
 	controlPlane.SetLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""})
 	controlPlaneWithInfra := builder.ControlPlane(metav1.NamespaceDefault, "cp1").
 		WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate).
 		Build()
 	controlPlaneWithInfra.SetLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""})
-
+	controlPlaneWithInfraNotTopologyOwned := builder.ControlPlane(metav1.NamespaceDefault, "cp1").
+		WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplateNotTopologyOwned).
+		Build()
 	controlPlaneNotTopologyOwned := builder.ControlPlane(metav1.NamespaceDefault, "cp1").
 		Build()
 
@@ -76,6 +84,10 @@ func TestGetCurrentState(t *testing.T) {
 		WithControlPlaneTemplate(controlPlaneTemplateWithInfrastructureMachine).
 		WithControlPlaneInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate).
 		Build()
+	clusterClassWithControlPlaneInfraNotTopologyOwned := builder.ClusterClass(metav1.NamespaceDefault, "class1").
+		WithControlPlaneTemplate(controlPlaneTemplateWithInfrastructureMachineNotTopologyOwned).
+		WithControlPlaneInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplateNotTopologyOwned).
+		Build()
 	clusterClassWithNoControlPlaneInfra := builder.ClusterClass(metav1.NamespaceDefault, "class2").
 		Build()
 
@@ -84,8 +96,10 @@ func TestGetCurrentState(t *testing.T) {
 
 	machineDeploymentInfrastructure := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").
 		Build()
+	machineDeploymentInfrastructure.SetLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""})
 	machineDeploymentBootstrap := builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").
 		Build()
+	machineDeploymentBootstrap.SetLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""})
 
 	machineDeployment := builder.MachineDeployment(metav1.NamespaceDefault, "md1").
 		WithLabels(map[string]string{
@@ -312,6 +326,19 @@ func TestGetCurrentState(t *testing.T) {
 					"md1": {Object: machineDeployment, BootstrapTemplate: machineDeploymentBootstrap, InfrastructureMachineTemplate: machineDeploymentInfrastructure}},
 			},
 		},
+		{
+			name: "Fails if the ControlPlane references an InfrastructureMachineTemplate that is not topology owned",
+			cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
+				// No InfrastructureCluster!
+				WithControlPlane(controlPlaneWithInfraNotTopologyOwned).
+				Build(),
+			class: clusterClassWithControlPlaneInfraNotTopologyOwned,
+			objects: []client.Object{
+				controlPlaneWithInfraNotTopologyOwned,
+				controlPlaneInfrastructureMachineTemplateNotTopologyOwned,
+			},
+			wantErr: true,
+		},
 		{
 			name: "Fails if the ControlPlane references an InfrastructureMachineTemplate that does not exist",
 			cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").