Fix snow upgrade bugs encountered during upgrade from beta (#2773)

* Support snow upgrade from beta with different naming format

* Add snow ChangeDiff

* Fix bug, add unit tests

* Introduce IncrementNameWithFallbackDefault

* Change log level

pkg/clusterapi/name.go

@@ -7,10 +7,16 @@ import (
 
 	"github.com/aws/eks-anywhere/pkg/api/v1alpha1"
 	"github.com/aws/eks-anywhere/pkg/cluster"
+	"github.com/aws/eks-anywhere/pkg/logger"
 )
 
 var nameRegex = regexp.MustCompile(`(.*?)(-)(\d+)$`)
 
+// IncrementName takes an object name and increments the suffix number by one.
+// This method is used for updating objects (e.g. machinetemplate, kubeadmconfigtemplate) that are either immutable
+// or require recreation to trigger machine rollout. The original object name should follow the name convention of
+// alphanumeric followed by dash digits, e.g. abc-1, md-0, kct-2. An error will be raised if the original name does not follow
+// this pattern.
 func IncrementName(name string) (string, error) {
 	match := nameRegex.FindStringSubmatch(name)
 	if match == nil {
@@ -25,6 +31,19 @@ func IncrementName(name string) (string, error) {
 	return ObjectName(match[1], n+1), nil
 }
 
+// IncrementNameWithFallbackDefault calls the IncrementName and fallbacks to use the default name if IncrementName
+// returns an error. This method is used to accommodate for any objects with name breaking changes from a previous version.
+// For example, in beta capi snowmachinetemplate is named after the eks-a snowmachineconfig name, without the '-1' suffix.
+// We set the object name to the default new machinetemplate name after detecting the invalid old name.
+func IncrementNameWithFallbackDefault(name, defaultName string) string {
+	n, err := IncrementName(name)
+	if err != nil {
+		logger.V(4).Info("Unable to increment object name (might due to changes of name format), fallback to the default name", "error", err.Error())
+		return defaultName
+	}
+	return n
+}
+
 func ObjectName(baseName string, version int) string {
 	return fmt.Sprintf("%s-%d", baseName, version)
 }

pkg/clusterapi/name_test.go

@@ -42,6 +42,35 @@ func TestIncrementName(t *testing.T) {
 	}
 }
 
+func TestIncrementNameWithFallbackDefault(t *testing.T) {
+	tests := []struct {
+		name        string
+		oldName     string
+		defaultName string
+		want        string
+	}{
+		{
+			name:        "valid",
+			oldName:     "cluster-1",
+			defaultName: "default",
+			want:        "cluster-2",
+		},
+		{
+			name:        "invalid format",
+			oldName:     "cluster-1a",
+			defaultName: "default",
+			want:        "default",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			got := clusterapi.IncrementNameWithFallbackDefault(tt.oldName, tt.defaultName)
+			g.Expect(got).To(Equal(tt.want))
+		})
+	}
+}
+
 func TestObjectName(t *testing.T) {
 	tests := []struct {
 		name    string

pkg/constants/constants.go

@@ -13,6 +13,7 @@ const (
 	CapvSystemNamespace                     = "capv-system"
 	CaptSystemNamespace                     = "capt-system"
 	CapaSystemNamespace                     = "capa-system"
+	CapasSystemNamespace                    = "capas-system"
 	CertManagerNamespace                    = "cert-manager"
 	DefaultNamespace                        = "default"
 	EtcdAdmBootstrapProviderSystemNamespace = "etcdadm-bootstrap-provider-system"

pkg/executables/clusterctl.go

@@ -300,6 +300,7 @@ var providerNamespaces = map[string]string{
 	constants.DockerProviderName:     constants.CapdSystemNamespace,
 	constants.CloudStackProviderName: constants.CapcSystemNamespace,
 	constants.AWSProviderName:        constants.CapaSystemNamespace,
+	constants.SnowProviderName:       constants.CapasSystemNamespace,
 	etcdadmBootstrapProviderName:     constants.EtcdAdmBootstrapProviderSystemNamespace,
 	etcdadmControllerProviderName:    constants.EtcdAdmControllerSystemNamespace,
 	kubeadmBootstrapProviderName:     constants.CapiKubeadmBootstrapSystemNamespace,

pkg/providers/snow/objects.go

@@ -23,11 +23,8 @@ func ControlPlaneObjects(ctx context.Context, clusterSpec *cluster.Spec, kubeCli
 	if err != nil {
 		return nil, err
 	}
-	name, err := NewMachineTemplateName(new, old)
-	if err != nil {
-		return nil, err
-	}
-	new.SetName(name)
+
+	new.SetName(NewMachineTemplateName(new, old))
 
 	kubeadmControlPlane, err := KubeadmControlPlane(clusterSpec, new)
 	if err != nil {
@@ -98,17 +95,11 @@ func WorkersMachineAndConfigTemplate(ctx context.Context, kubeClient kubernetes.
 		}
 
 		// compare the old and new kubeadmConfigTemplate to determine whether to recreate new kubeadmConfigTemplate object
-		configName, err := NewKubeadmConfigTemplateName(newConfigTemplate, oldConfigTemplate)
-		if err != nil {
-			return nil, nil, err
-		}
+		configName := NewKubeadmConfigTemplateName(newConfigTemplate, oldConfigTemplate)
 
 		// compare the old and new machineTemplate as well as kubeadmConfigTemplate to determine whether to recreate
 		// new machineTemplate object
-		machineName, err := NewWorkerMachineTemplateName(newMachineTemplate, oldMachineTemplate, newConfigTemplate, oldConfigTemplate)
-		if err != nil {
-			return nil, nil, err
-		}
+		machineName := NewWorkerMachineTemplateName(newMachineTemplate, oldMachineTemplate, newConfigTemplate, oldConfigTemplate)
 
 		newConfigTemplate.SetName(configName)
 		newMachineTemplate.SetName(machineName)
@@ -120,48 +111,42 @@ func WorkersMachineAndConfigTemplate(ctx context.Context, kubeClient kubernetes.
 	return machines, configs, nil
 }
 
-func NewMachineTemplateName(new, old *snowv1.AWSSnowMachineTemplate) (string, error) {
+func NewMachineTemplateName(new, old *snowv1.AWSSnowMachineTemplate) string {
 	if old == nil {
-		return new.GetName(), nil
+		return new.GetName()
 	}
 
 	if equality.Semantic.DeepDerivative(new.Spec, old.Spec) {
-		return old.GetName(), nil
+		return old.GetName()
 	}
 
-	return clusterapi.IncrementName(old.GetName())
+	return clusterapi.IncrementNameWithFallbackDefault(old.GetName(), new.GetName())
 }
 
-func NewWorkerMachineTemplateName(newMt, oldMt *snowv1.AWSSnowMachineTemplate, newKct, oldKct *bootstrapv1.KubeadmConfigTemplate) (string, error) {
-	name, err := NewMachineTemplateName(newMt, oldMt)
-	if err != nil {
-		return "", err
-	}
+func NewWorkerMachineTemplateName(newMt, oldMt *snowv1.AWSSnowMachineTemplate, newKct, oldKct *bootstrapv1.KubeadmConfigTemplate) string {
+	name := NewMachineTemplateName(newMt, oldMt)
 
 	if oldKct == nil {
-		return name, nil
+		return name
 	}
 
 	if recreateKubeadmConfigTemplateNeeded(newKct, oldKct) {
-		name, err = clusterapi.IncrementName(oldMt.GetName())
-		if err != nil {
-			return "", err
-		}
+		name = clusterapi.IncrementNameWithFallbackDefault(oldMt.GetName(), newMt.GetName())
 	}
 
-	return name, nil
+	return name
 }
 
-func NewKubeadmConfigTemplateName(new, old *bootstrapv1.KubeadmConfigTemplate) (string, error) {
+func NewKubeadmConfigTemplateName(new, old *bootstrapv1.KubeadmConfigTemplate) string {
 	if old == nil {
-		return new.GetName(), nil
+		return new.GetName()
 	}
 
 	if recreateKubeadmConfigTemplateNeeded(new, old) {
-		return clusterapi.IncrementName(old.GetName())
+		return clusterapi.IncrementNameWithFallbackDefault(old.GetName(), new.GetName())
 	}
 
-	return old.GetName(), nil
+	return old.GetName()
 }
 
 func recreateKubeadmConfigTemplateNeeded(new, old *bootstrapv1.KubeadmConfigTemplate) bool {

pkg/providers/snow/objects_test.go

@@ -58,6 +58,45 @@ func TestControlPlaneObjects(t *testing.T) {
 	g.Expect(got).To(Equal([]runtime.Object{wantCAPICluster(), wantSnowCluster(), kcp, mt}))
 }
 
+func TestControlPlaneObjectsUpgradeFromBetaMachineTemplateName(t *testing.T) {
+	g := newSnowTest(t)
+	mt := wantSnowMachineTemplate()
+	g.kubeconfigClient.EXPECT().
+		Get(
+			g.ctx,
+			"snow-test",
+			constants.EksaSystemNamespace,
+			&controlplanev1.KubeadmControlPlane{},
+		).
+		DoAndReturn(func(_ context.Context, _, _ string, obj *controlplanev1.KubeadmControlPlane) error {
+			obj.Spec.MachineTemplate.InfrastructureRef.Name = "test-cp"
+			return nil
+		})
+	g.kubeconfigClient.EXPECT().
+		Get(
+			g.ctx,
+			"test-cp",
+			constants.EksaSystemNamespace,
+			&snowv1.AWSSnowMachineTemplate{},
+		).
+		DoAndReturn(func(_ context.Context, _, _ string, obj *snowv1.AWSSnowMachineTemplate) error {
+			mt.DeepCopyInto(obj)
+			obj.SetName("test-cp")
+			obj.Spec.Template.Spec.InstanceType = "updated-instance-type"
+			return nil
+		})
+
+	wantMachineTemplateName := "snow-test-control-plane-1"
+	mt.SetName(wantMachineTemplateName)
+	mt.Spec.Template.Spec.InstanceType = "sbe-c.large"
+	kcp := wantKubeadmControlPlane()
+	kcp.Spec.MachineTemplate.InfrastructureRef.Name = wantMachineTemplateName
+
+	got, err := snow.ControlPlaneObjects(g.ctx, g.clusterSpec, g.kubeconfigClient)
+	g.Expect(err).To(Succeed())
+	g.Expect(got).To(Equal([]runtime.Object{wantCAPICluster(), wantSnowCluster(), kcp, mt}))
+}
+
 func TestControlPlaneObjectsOldControlPlaneNotExists(t *testing.T) {
 	g := newSnowTest(t)
 	mt := wantSnowMachineTemplate()
@@ -198,6 +237,65 @@ func TestWorkersObjects(t *testing.T) {
 	g.Expect(got).To(Equal([]runtime.Object{md, wantKubeadmConfigTemplate(), mt}))
 }
 
+func TestWorkersObjectsFromBetaMachineTemplateName(t *testing.T) {
+	g := newSnowTest(t)
+	mt := wantSnowMachineTemplate()
+	g.kubeconfigClient.EXPECT().
+		Get(
+			g.ctx,
+			"snow-test-md-0",
+			constants.EksaSystemNamespace,
+			&clusterv1.MachineDeployment{},
+		).
+		DoAndReturn(func(_ context.Context, _, _ string, obj *clusterv1.MachineDeployment) error {
+			wantMachineDeployment().DeepCopyInto(obj)
+			obj.Spec.Template.Spec.InfrastructureRef.Name = "test-wn"
+			obj.Spec.Template.Spec.Bootstrap.ConfigRef.Name = "test-wn"
+			return nil
+		})
+	g.kubeconfigClient.EXPECT().
+		Get(
+			g.ctx,
+			"test-wn",
+			constants.EksaSystemNamespace,
+			&bootstrapv1.KubeadmConfigTemplate{},
+		).
+		DoAndReturn(func(_ context.Context, _, _ string, obj *bootstrapv1.KubeadmConfigTemplate) error {
+			wantKubeadmConfigTemplate().DeepCopyInto(obj)
+			obj.SetName("test-wn")
+			obj.Spec.Template.Spec.PreKubeadmCommands = []string{
+				"new command",
+			}
+			return nil
+		})
+	g.kubeconfigClient.EXPECT().
+		Get(
+			g.ctx,
+			"test-wn",
+			constants.EksaSystemNamespace,
+			&snowv1.AWSSnowMachineTemplate{},
+		).
+		DoAndReturn(func(_ context.Context, _, _ string, obj *snowv1.AWSSnowMachineTemplate) error {
+			mt.DeepCopyInto(obj)
+			obj.SetName("test-wn")
+			obj.Spec.Template.Spec.InstanceType = "updated-instance-type"
+			return nil
+		})
+
+	wantMachineTemplateName := "snow-test-md-0-1"
+	mt.SetName(wantMachineTemplateName)
+	md := wantMachineDeployment()
+	md.Spec.Template.Spec.InfrastructureRef.Name = wantMachineTemplateName
+	kct := wantKubeadmConfigTemplate()
+	wantKctName := "snow-test-md-0-1"
+	kct.SetName(wantKctName)
+	md.Spec.Template.Spec.Bootstrap.ConfigRef.Name = wantKctName
+
+	got, err := snow.WorkersObjects(g.ctx, g.clusterSpec, g.kubeconfigClient)
+	g.Expect(err).To(Succeed())
+	g.Expect(got).To(Equal([]runtime.Object{md, kct, mt}))
+}
+
 func TestWorkersObjectsOldMachineDeploymentNotExists(t *testing.T) {
 	g := newSnowTest(t)
 	mt := wantSnowMachineTemplate()

pkg/providers/snow/snow.go

@@ -184,7 +184,7 @@ func (p *SnowProvider) EnvMap(clusterSpec *cluster.Spec) (map[string]string, err
 
 func (p *SnowProvider) GetDeployments() map[string][]string {
 	return map[string][]string{
-		"capas-system": {"capas-controller-manager"},
+		constants.CapasSystemNamespace: {"capas-controller-manager"},
 	}
 }
 
@@ -231,7 +231,15 @@ func (p *SnowProvider) GenerateMHC(clusterSpec *cluster.Spec) ([]byte, error) {
 }
 
 func (p *SnowProvider) ChangeDiff(currentSpec, newSpec *cluster.Spec) *types.ComponentChangeDiff {
-	return nil
+	if currentSpec.VersionsBundle.Snow.Version == newSpec.VersionsBundle.Snow.Version {
+		return nil
+	}
+
+	return &types.ComponentChangeDiff{
+		ComponentName: constants.SnowProviderName,
+		NewVersion:    newSpec.VersionsBundle.Snow.Version,
+		OldVersion:    currentSpec.VersionsBundle.Snow.Version,
+	}
 }
 
 func (p *SnowProvider) RunPostControlPlaneUpgrade(ctx context.Context, oldClusterSpec *cluster.Spec, clusterSpec *cluster.Spec, workloadCluster *types.Cluster, managementCluster *types.Cluster) error {

pkg/providers/snow/snow_test.go

@@ -739,3 +739,25 @@ func TestUpgradeNeededMachineConfigs(t *testing.T) {
 		})
 	}
 }
+
+func TestChangeDiffNoChange(t *testing.T) {
+	g := NewWithT(t)
+	provider := givenProvider(t)
+	clusterSpec := givenEmptyClusterSpec()
+	g.Expect(provider.ChangeDiff(clusterSpec, clusterSpec)).To(BeNil())
+}
+
+func TestChangeDiffWithChange(t *testing.T) {
+	g := NewWithT(t)
+	provider := givenProvider(t)
+	clusterSpec := givenEmptyClusterSpec()
+	newClusterSpec := clusterSpec.DeepCopy()
+	clusterSpec.VersionsBundle.Snow.Version = "v1.0.2"
+	newClusterSpec.VersionsBundle.Snow.Version = "v1.0.3"
+	want := &types.ComponentChangeDiff{
+		ComponentName: "snow",
+		NewVersion:    "v1.0.3",
+		OldVersion:    "v1.0.2",
+	}
+	g.Expect(provider.ChangeDiff(clusterSpec, newClusterSpec)).To(Equal(want))
+}