From a0a81a1ca21cafe7c36ba38981f5908e8e185d54 Mon Sep 17 00:00:00 2001 From: adil ghaffar Date: Mon, 13 Jun 2022 17:58:31 +0300 Subject: [PATCH] Add checks for not topology owned templates to never reconcile. --- .../topology/cluster/current_state.go | 24 +++++++++++++++ .../topology/cluster/current_state_test.go | 29 ++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/internal/controllers/topology/cluster/current_state.go b/internal/controllers/topology/cluster/current_state.go index de8af7c4ee91..125b06e55e14 100644 --- a/internal/controllers/topology/cluster/current_state.go +++ b/internal/controllers/topology/cluster/current_state.go @@ -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("referenced control plane InfrastructureMachineTemplate object %s is not topology owned", tlog.KObj{Obj: res.InfrastructureMachineTemplate}) + } mhc := &clusterv1.MachineHealthCheck{} // MachineHealthCheck always has the same name and namespace as the ControlPlane object it belongs to. @@ -158,6 +164,12 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, clust for i := range md.Items { m := &md.Items[i] + // check that the MachineDeployment 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(m) { + return nil, fmt.Errorf("%s is not topology owned", tlog.KObj{Obj: m}) + } // Retrieve the name which is assigned in Cluster's topology // from a well-defined label. mdTopologyName, ok := m.ObjectMeta.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] @@ -181,6 +193,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("referenced BootstrapTemplate object %s is not topology owned", tlog.KObj{Obj: b}) + } // Gets the InfrastructureMachineTemplate infraRef := m.Spec.Template.Spec.InfrastructureRef @@ -191,6 +209,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("referenced InfrastructureMachineTemplate object %s is not topology owned", tlog.KObj{Obj: infra}) + } // Gets the MachineHealthCheck. mhc := &clusterv1.MachineHealthCheck{} diff --git a/internal/controllers/topology/cluster/current_state_test.go b/internal/controllers/topology/cluster/current_state_test.go index 3a102446feee..62b97360f783 100644 --- a/internal/controllers/topology/cluster/current_state_test.go +++ b/internal/controllers/topology/cluster/current_state_test.go @@ -57,9 +57,15 @@ 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: ""}) @@ -67,7 +73,9 @@ func TestGetCurrentState(t *testing.T) { 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").