Merge pull request #12341 from sbueringer/pr-fix-infinite-reconcile

🐛 Fix continuous reconciles because of apiVersion differences in Cluster topology controller

Author: k8s-ci-robot

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"
@@ -83,7 +84,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))
 	}
@@ -108,7 +109,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))
 	}
@@ -133,7 +134,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))
 	}
@@ -228,11 +229,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)))
 				}
@@ -348,11 +349,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)))
 			}
@@ -397,17 +398,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

@@ -1186,43 +1186,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": clusterv1.GroupVersionInfrastructure.String(),
-				"kind":       "DockerCluster",
+				"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": clusterv1.GroupVersionInfrastructure.String(),
-				"kind":       "DockerCluster",
+				"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.
+				// Group & kind is the same (+/- Template suffix) => apiVersion is taken from ClusterClass.
 				APIVersion: clusterv1.GroupVersionInfrastructure.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": clusterv1.GroupVersionBootstrap.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: clusterv1.GroupVersionBootstrap.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{}{
@@ -1235,6 +1259,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: clusterv1.GroupVersionBootstrap.String(),
@@ -1258,6 +1283,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: clusterv1.GroupVersionBootstrap.String(),
@@ -1271,7 +1297,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