Fix infinite reconcile because of apiVersions in Cluster topology controller

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

internal/controllers/topology/cluster/current_state.go

@@ -19,6 +19,7 @@ package cluster
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -84,7 +85,7 @@ func (r *Reconciler) getCurrentState(ctx context.Context, s *scope.Scope) (*scop
 // getCurrentInfrastructureClusterState looks for the state of the InfrastructureCluster. If a reference is set but not
 // found, either from an error or the object not being found, an error is thrown.
 func (r *Reconciler) getCurrentInfrastructureClusterState(ctx context.Context, blueprintInfrastructureClusterTemplate *unstructured.Unstructured, cluster *clusterv1.Cluster) (*unstructured.Unstructured, error) {
-	ref, err := alignRefAPIVersion(blueprintInfrastructureClusterTemplate, cluster.Spec.InfrastructureRef)
+	ref, err := alignRefAPIVersion(blueprintInfrastructureClusterTemplate, cluster.Spec.InfrastructureRef, false)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to read %s %s", cluster.Spec.InfrastructureRef.Kind, klog.KRef(cluster.Spec.InfrastructureRef.Namespace, cluster.Spec.InfrastructureRef.Name))
 	}
@@ -109,7 +110,7 @@ func (r *Reconciler) getCurrentControlPlaneState(ctx context.Context, blueprintC
 	res := &scope.ControlPlaneState{}
 
 	// Get the control plane object.
-	ref, err := alignRefAPIVersion(blueprintControlPlane.Template, cluster.Spec.ControlPlaneRef)
+	ref, err := alignRefAPIVersion(blueprintControlPlane.Template, cluster.Spec.ControlPlaneRef, false)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to read %s %s", cluster.Spec.ControlPlaneRef.Kind, klog.KRef(cluster.Spec.ControlPlaneRef.Namespace, cluster.Spec.ControlPlaneRef.Name))
 	}
@@ -134,7 +135,7 @@ func (r *Reconciler) getCurrentControlPlaneState(ctx context.Context, blueprintC
 	if err != nil {
 		return res, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate reference for %s %s", res.Object.GetKind(), klog.KObj(res.Object))
 	}
-	ref, err = alignRefAPIVersion(blueprintControlPlane.InfrastructureMachineTemplate, machineInfrastructureRef)
+	ref, err = alignRefAPIVersion(blueprintControlPlane.InfrastructureMachineTemplate, machineInfrastructureRef, true)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get InfrastructureMachineTemplate for %s %s", res.Object.GetKind(), klog.KObj(res.Object))
 	}
@@ -229,11 +230,11 @@ func (r *Reconciler) getCurrentMachineDeploymentState(ctx context.Context, bluep
 				if !ok {
 					return nil, fmt.Errorf("failed to find MachineDeployment class %s in ClusterClass", mdClassName)
 				}
-				bootstrapRef, err = alignRefAPIVersion(mdBluePrint.BootstrapTemplate, bootstrapRef)
+				bootstrapRef, err = alignRefAPIVersion(mdBluePrint.BootstrapTemplate, bootstrapRef, true)
 				if err != nil {
 					return nil, errors.Wrap(err, fmt.Sprintf("MachineDeployment %s Bootstrap reference could not be retrieved", klog.KObj(m)))
 				}
-				infraRef, err = alignRefAPIVersion(mdBluePrint.InfrastructureMachineTemplate, infraRef)
+				infraRef, err = alignRefAPIVersion(mdBluePrint.InfrastructureMachineTemplate, infraRef, true)
 				if err != nil {
 					return nil, errors.Wrap(err, fmt.Sprintf("MachineDeployment %s Infrastructure reference could not be retrieved", klog.KObj(m)))
 				}
@@ -349,11 +350,11 @@ func (r *Reconciler) getCurrentMachinePoolState(ctx context.Context, blueprintMa
 			if !ok {
 				return nil, fmt.Errorf("failed to find MachinePool class %s in ClusterClass", mpClassName)
 			}
-			bootstrapRef, err = alignRefAPIVersion(mpBluePrint.BootstrapTemplate, bootstrapRef)
+			bootstrapRef, err = alignRefAPIVersion(mpBluePrint.BootstrapTemplate, bootstrapRef, false)
 			if err != nil {
 				return nil, errors.Wrap(err, fmt.Sprintf("MachinePool %s Bootstrap reference could not be retrieved", klog.KObj(m)))
 			}
-			infraRef, err = alignRefAPIVersion(mpBluePrint.InfrastructureMachinePoolTemplate, infraRef)
+			infraRef, err = alignRefAPIVersion(mpBluePrint.InfrastructureMachinePoolTemplate, infraRef, false)
 			if err != nil {
 				return nil, errors.Wrap(err, fmt.Sprintf("MachinePool %s Infrastructure reference could not be retrieved", klog.KObj(m)))
 			}
@@ -398,17 +399,27 @@ func (r *Reconciler) getCurrentMachinePoolState(ctx context.Context, blueprintMa
 // If group or kind was changed in the ClusterClass, an exact copy of the currentRef is returned because
 // it will end up in a diff and a rollout anyway.
 // Only bootstrap template refs in a ClusterClass can change their group and kind.
-func alignRefAPIVersion(templateFromClusterClass *unstructured.Unstructured, currentRef *corev1.ObjectReference) (*corev1.ObjectReference, error) {
+func alignRefAPIVersion(templateFromClusterClass *unstructured.Unstructured, currentRef *corev1.ObjectReference, isCurrentTemplate bool) (*corev1.ObjectReference, error) {
 	currentGV, err := schema.ParseGroupVersion(currentRef.APIVersion)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to parse apiVersion: %q", currentRef.APIVersion)
 	}
 
 	apiVersion := currentRef.APIVersion
 	// Use apiVersion from ClusterClass if group and kind is the same.
-	if templateFromClusterClass.GroupVersionKind().Group == currentGV.Group &&
-		templateFromClusterClass.GetKind() == currentRef.Kind {
-		apiVersion = templateFromClusterClass.GetAPIVersion()
+	if templateFromClusterClass.GroupVersionKind().Group == currentGV.Group {
+		if isCurrentTemplate {
+			// If the current object is a template, kind has to be identical.
+			if templateFromClusterClass.GetKind() == currentRef.Kind {
+				apiVersion = templateFromClusterClass.GetAPIVersion()
+			}
+		} else {
+			// If the current object is not a template, currentRef.Kind should be the kind from CC without the Template suffix,
+			// e.g. KubeadmControlPlaneTemplate == KubeadmControlPlane
+			if strings.TrimSuffix(templateFromClusterClass.GetKind(), "Template") == currentRef.Kind {
+				apiVersion = templateFromClusterClass.GetAPIVersion()
+			}
+		}
 	}
 
 	return &corev1.ObjectReference{

internal/controllers/topology/cluster/current_state_test.go

@@ -27,6 +27,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -1187,43 +1188,67 @@ func TestAlignRefAPIVersion(t *testing.T) {
 		name                     string
 		templateFromClusterClass *unstructured.Unstructured
 		currentRef               *corev1.ObjectReference
+		isCurrentTemplate        bool
 		want                     *corev1.ObjectReference
 		wantErr                  bool
 	}{
 		{
 			name: "Error for invalid apiVersion",
 			templateFromClusterClass: &unstructured.Unstructured{Object: map[string]interface{}{
-				"apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1",
-				"kind":       "DockerCluster",
+				"apiVersion": schema.GroupVersion{Group: "infrastructure.cluster.x-k8s.io", Version: "v1beta1"}.String(),
+				"kind":       "DockerClusterTemplate",
 			}},
 			currentRef: &corev1.ObjectReference{
 				APIVersion: "invalid/api/version",
 				Kind:       "DockerCluster",
 				Name:       "my-cluster-abc",
 				Namespace:  metav1.NamespaceDefault,
 			},
-			wantErr: true,
+			isCurrentTemplate: false,
+			wantErr:           true,
 		},
 		{
-			name: "Use apiVersion from ClusterClass: group and kind is the same",
+			name: "Use apiVersion from ClusterClass: group and kind is the same (+/- Template suffix)",
 			templateFromClusterClass: &unstructured.Unstructured{Object: map[string]interface{}{
-				"apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1",
-				"kind":       "DockerCluster",
+				"apiVersion": schema.GroupVersion{Group: "infrastructure.cluster.x-k8s.io", Version: "v1beta1"}.String(),
+				"kind":       "DockerClusterTemplate",
 			}},
 			currentRef: &corev1.ObjectReference{
-				APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2",
+				APIVersion: "infrastructure.cluster.x-k8s.io/different", // should be overwritten with version from CC
 				Kind:       "DockerCluster",
 				Name:       "my-cluster-abc",
 				Namespace:  metav1.NamespaceDefault,
 			},
+			isCurrentTemplate: false,
 			want: &corev1.ObjectReference{
-				// Group & kind is the same => apiVersion is taken from ClusterClass.
-				APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+				// Group & kind is the same (+/- Template suffix) => apiVersion is taken from ClusterClass.
+				APIVersion: schema.GroupVersion{Group: "infrastructure.cluster.x-k8s.io", Version: "v1beta1"}.String(),
 				Kind:       "DockerCluster",
 				Name:       "my-cluster-abc",
 				Namespace:  metav1.NamespaceDefault,
 			},
 		},
+		{
+			name: "Use apiVersion from ClusterClass: group and kind is the same",
+			templateFromClusterClass: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": schema.GroupVersion{Group: "bootstrap.cluster.x-k8s.io", Version: "v1beta1"}.String(),
+				"kind":       "KubeadmConfigTemplate",
+			}},
+			currentRef: &corev1.ObjectReference{
+				APIVersion: "bootstrap.cluster.x-k8s.io/different", // should be overwritten with version from CC
+				Kind:       "KubeadmConfigTemplate",
+				Name:       "my-cluster-abc",
+				Namespace:  metav1.NamespaceDefault,
+			},
+			isCurrentTemplate: true,
+			want: &corev1.ObjectReference{
+				// Group & kind is the same => apiVersion is taken from ClusterClass.
+				APIVersion: schema.GroupVersion{Group: "bootstrap.cluster.x-k8s.io", Version: "v1beta1"}.String(),
+				Kind:       "KubeadmConfigTemplate",
+				Name:       "my-cluster-abc",
+				Namespace:  metav1.NamespaceDefault,
+			},
+		},
 		{
 			name: "Use apiVersion from currentRef: kind is different",
 			templateFromClusterClass: &unstructured.Unstructured{Object: map[string]interface{}{
@@ -1236,6 +1261,7 @@ func TestAlignRefAPIVersion(t *testing.T) {
 				Name:       "my-cluster-abc",
 				Namespace:  metav1.NamespaceDefault,
 			},
+			isCurrentTemplate: true,
 			want: &corev1.ObjectReference{
 				// kind is different => apiVersion is taken from currentRef.
 				APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1",
@@ -1256,6 +1282,7 @@ func TestAlignRefAPIVersion(t *testing.T) {
 				Name:       "my-cluster-abc",
 				Namespace:  metav1.NamespaceDefault,
 			},
+			isCurrentTemplate: true,
 			want: &corev1.ObjectReference{
 				// group is different => apiVersion is taken from currentRef.
 				APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1",
@@ -1269,7 +1296,7 @@ func TestAlignRefAPIVersion(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			got, err := alignRefAPIVersion(tt.templateFromClusterClass, tt.currentRef)
+			got, err := alignRefAPIVersion(tt.templateFromClusterClass, tt.currentRef, tt.isCurrentTemplate)
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 				return