From 8ac7392bb01d996c1d4bace661437bb373986b18 Mon Sep 17 00:00:00 2001 From: Chris Doherty Date: Mon, 11 Sep 2023 16:11:14 -0500 Subject: [PATCH] Prevent bare metal machine config references from changing In bare metal clusters users shouldn't be able to change machine configs. However, EKS-A incorrectly allowed users to change the machine config references to existing machine configs. --- pkg/providers/tinkerbell/upgrade.go | 62 ++++++++++++++--- pkg/providers/tinkerbell/upgrade_test.go | 84 +++++++++++++++++++++++- 2 files changed, 134 insertions(+), 12 deletions(-) diff --git a/pkg/providers/tinkerbell/upgrade.go b/pkg/providers/tinkerbell/upgrade.go index 29633a179ba77..2b4314b7d457a 100644 --- a/pkg/providers/tinkerbell/upgrade.go +++ b/pkg/providers/tinkerbell/upgrade.go @@ -9,6 +9,7 @@ import ( rufiov1 "github.com/tinkerbell/rufio/api/v1alpha1" tinkv1alpha1 "github.com/tinkerbell/tink/pkg/apis/core/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/aws/eks-anywhere/pkg/api/v1alpha1" "github.com/aws/eks-anywhere/pkg/cluster" @@ -244,7 +245,21 @@ func (p *Provider) ValidateNewSpec(ctx context.Context, cluster *types.Cluster, oSpec := prevDatacenterConfig.Spec nSpec := p.datacenterConfig.Spec - prevMachineConfigRefs := machineRefSliceToMap(prevSpec.MachineConfigRefs()) + err = validateControlPlaneMachineConfigRefUnchanged(prevSpec, clusterSpec.Cluster) + if err != nil { + return err + } + + err = validateWorkerNodeGroupMachineConfigRefsUnchanged(prevSpec.Spec.WorkerNodeGroupConfigurations, + clusterSpec.Cluster.Spec.WorkerNodeGroupConfigurations) + if err != nil { + return err + } + + // Validate worker node group machine configs are the same. + prevMachineConfigRefs := collection.ToMap(prevSpec.MachineConfigRefs(), func(v v1alpha1.Ref) string { + return v.Name + }) desiredWorkerNodeGroups := clusterSpec.Cluster.Spec.WorkerNodeGroupConfigurations currentWorkerNodeGroups := collection.ToMap(prevSpec.Spec.WorkerNodeGroupConfigurations, @@ -279,7 +294,7 @@ func (p *Provider) ValidateNewSpec(ctx context.Context, cluster *types.Cluster, } if nSpec.TinkerbellIP != oSpec.TinkerbellIP { - return fmt.Errorf("spec.TinkerbellIP is immutable. Previous value %s, New value %s", oSpec.TinkerbellIP, nSpec.TinkerbellIP) + return fmt.Errorf("spec.tinkerbellIP is immutable; previous = %s, new = %s", oSpec.TinkerbellIP, nSpec.TinkerbellIP) } // for any operation other than k8s version change, hookImageURL is immutable @@ -292,6 +307,41 @@ func (p *Provider) ValidateNewSpec(ctx context.Context, cluster *types.Cluster, return nil } +func validateControlPlaneMachineConfigRefUnchanged(current, desired *v1alpha1.Cluster) error { + curr := current.Spec.ControlPlaneConfiguration.MachineGroupRef + desi := desired.Spec.ControlPlaneConfiguration.MachineGroupRef + + if !curr.Equal(desi) { + return errors.New("control plane machine config reference is immutable") + } + + return nil +} + +func validateWorkerNodeGroupMachineConfigRefsUnchanged(current, desired []v1alpha1.WorkerNodeGroupConfiguration) error { + lookup := collection.ToMap(desired, func(v v1alpha1.WorkerNodeGroupConfiguration) string { + return v.Name + }) + + var errs []error + + // For every current worker node group that still exists in the desired config, ensure the + // machine config is still the same. + for _, curr := range current { + desi, exists := lookup[curr.Name] + if !exists { + continue + } + + if !curr.MachineGroupRef.Equal(desi.MachineGroupRef) { + errs = append(errs, + fmt.Errorf("%v: worker node group machine config reference is immutable", curr.Name)) + } + } + + return kerrors.NewAggregate(errs) +} + func (p *Provider) UpgradeNeeded(_ context.Context, _, _ *cluster.Spec, _ *types.Cluster) (bool, error) { // TODO: Figure out if something is needed here return false, nil @@ -368,14 +418,6 @@ func (p *Provider) validateMachineConfigImmutability(ctx context.Context, cluste return nil } -func machineRefSliceToMap(machineRefs []v1alpha1.Ref) map[string]v1alpha1.Ref { - refMap := make(map[string]v1alpha1.Ref, len(machineRefs)) - for _, ref := range machineRefs { - refMap[ref.Name] = ref - } - return refMap -} - // PreCoreComponentsUpgrade staisfies the Provider interface. func (p *Provider) PreCoreComponentsUpgrade( ctx context.Context, diff --git a/pkg/providers/tinkerbell/upgrade_test.go b/pkg/providers/tinkerbell/upgrade_test.go index 8b24ac175f7ab..cda9b09431761 100644 --- a/pkg/providers/tinkerbell/upgrade_test.go +++ b/pkg/providers/tinkerbell/upgrade_test.go @@ -799,6 +799,86 @@ func TestProvider_ValidateNewSpec_ChangeWorkerNodeGroupMachineRef(t *testing.T) docker, helm, kubectl) provider.stackInstaller = stackInstaller + kubectl.EXPECT(). + GetEksaCluster(ctx, desiredClusterSpec.ManagementCluster, + desiredClusterSpec.Cluster.Spec.ManagementCluster.Name). + Return(currentClusterSpec.Cluster, nil) + kubectl.EXPECT(). + GetEksaTinkerbellDatacenterConfig(ctx, currentClusterSpec.Cluster.Spec.DatacenterRef.Name, + cluster.KubeconfigFile, currentClusterSpec.Cluster.Namespace). + Return(datacenterConfig, nil) + + err := provider.ValidateNewSpec(ctx, desiredClusterSpec.ManagementCluster, desiredClusterSpec) + if err == nil || !strings.Contains(err.Error(), "cannot add or remove MachineConfigs") { + t.Fatalf("Expected error containing 'cannot add or remove MachineConfigs' but received: %v", err) + } +} + +func TestProvider_ValidateNewSpec_ChangeControlPlaneMachineRefToExisting(t *testing.T) { + clusterSpecManifest := "cluster_tinkerbell_stacked_etcd.yaml" + mockCtrl := gomock.NewController(t) + currentClusterSpec := givenClusterSpec(t, clusterSpecManifest) + datacenterConfig := givenDatacenterConfig(t, clusterSpecManifest) + machineConfigs := givenMachineConfigs(t, clusterSpecManifest) + docker := stackmocks.NewMockDocker(mockCtrl) + helm := stackmocks.NewMockHelm(mockCtrl) + kubectl := mocks.NewMockProviderKubectlClient(mockCtrl) + stackInstaller := stackmocks.NewMockStackInstaller(mockCtrl) + writer := filewritermocks.NewMockFileWriter(mockCtrl) + ctx := context.Background() + + cluster := &types.Cluster{Name: "test", KubeconfigFile: "kubeconfig-file"} + currentClusterSpec.ManagementCluster = cluster + + desiredClusterSpec := currentClusterSpec.DeepCopy() + + // Change an existing worker node groups machine config reference to an existing machine config. + desiredClusterSpec.Cluster.Spec.ControlPlaneConfiguration.MachineGroupRef.Name = "test-md" + + provider := newTinkerbellProvider(datacenterConfig, machineConfigs, currentClusterSpec.Cluster, writer, + docker, helm, kubectl) + provider.stackInstaller = stackInstaller + + kubectl.EXPECT(). + GetEksaCluster(ctx, desiredClusterSpec.ManagementCluster, + desiredClusterSpec.Cluster.Spec.ManagementCluster.Name). + Return(currentClusterSpec.Cluster, nil) + kubectl.EXPECT(). + GetEksaTinkerbellDatacenterConfig(ctx, currentClusterSpec.Cluster.Spec.DatacenterRef.Name, + cluster.KubeconfigFile, currentClusterSpec.Cluster.Namespace). + Return(datacenterConfig, nil) + + err := provider.ValidateNewSpec(ctx, desiredClusterSpec.ManagementCluster, desiredClusterSpec) + if err == nil || !strings.Contains(err.Error(), "control plane machine config reference is immutable") { + t.Fatalf("Expected error containing 'control plane machine config reference is immutable' but received: %v", err) + } +} + +func TestProvider_ValidateNewSpec_ChangeWorkerNodeGroupMachineRefToExisting(t *testing.T) { + clusterSpecManifest := "cluster_tinkerbell_stacked_etcd.yaml" + mockCtrl := gomock.NewController(t) + currentClusterSpec := givenClusterSpec(t, clusterSpecManifest) + datacenterConfig := givenDatacenterConfig(t, clusterSpecManifest) + machineConfigs := givenMachineConfigs(t, clusterSpecManifest) + docker := stackmocks.NewMockDocker(mockCtrl) + helm := stackmocks.NewMockHelm(mockCtrl) + kubectl := mocks.NewMockProviderKubectlClient(mockCtrl) + stackInstaller := stackmocks.NewMockStackInstaller(mockCtrl) + writer := filewritermocks.NewMockFileWriter(mockCtrl) + ctx := context.Background() + + cluster := &types.Cluster{Name: "test", KubeconfigFile: "kubeconfig-file"} + currentClusterSpec.ManagementCluster = cluster + + desiredClusterSpec := currentClusterSpec.DeepCopy() + + // Change an existing worker node groups machine config reference to an existing machine config. + desiredClusterSpec.Cluster.Spec.WorkerNodeGroupConfigurations[0].MachineGroupRef.Name = "test-cp" + + provider := newTinkerbellProvider(datacenterConfig, machineConfigs, currentClusterSpec.Cluster, writer, + docker, helm, kubectl) + provider.stackInstaller = stackInstaller + kubectl.EXPECT(). GetEksaCluster(ctx, desiredClusterSpec.ManagementCluster, desiredClusterSpec.Cluster.Spec.ManagementCluster.Name). @@ -820,8 +900,8 @@ func TestProvider_ValidateNewSpec_ChangeWorkerNodeGroupMachineRef(t *testing.T) AnyTimes() err := provider.ValidateNewSpec(ctx, desiredClusterSpec.ManagementCluster, desiredClusterSpec) - if err == nil || !strings.Contains(err.Error(), "cannot add or remove MachineConfigs") { - t.Fatalf("Expected error containing 'cannot add or remove MachineConfigs' but received: %v", err) + if err == nil || !strings.Contains(err.Error(), "worker node group machine config reference is immutable") { + t.Fatalf("Expected error containing 'worker node group machine config reference is immutable' but received: %v", err) } }