From 89a59a6df601cbcb4bb5fbe04a9a64ac63f75b91 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Thu, 11 Jul 2024 12:49:31 +0200 Subject: [PATCH] Fix subtests running in same environment and reconciliation bug which was not covered because of that (#602) --- .../awsmachinepool_controller_test.go | 41 +++++++++++++++---- pkg/cloud/services/ec2/launchtemplate.go | 4 +- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 55b15dd9ab..0e10cd75bc 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -472,15 +472,15 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) t.Run("ReconcileLaunchTemplate not mocked", func(t *testing.T) { - g := NewWithT(t) - setup(t, g) - reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) - reconSvc = nil // not used - defer teardown(t, g) - launchTemplateIDExisting := "lt-existing" t.Run("nothing exists, so launch template and ASG must be created", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) + reconSvc = nil // not used + defer teardown(t, g) + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return(nil, "", nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(pointer.String("ami-abcdef123"), nil) ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(pointer.String("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script"))).Return("lt-ghijkl456", nil) @@ -497,6 +497,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) t.Run("launch template and ASG exist and need no update", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) + reconSvc = nil // not used + defer teardown(t, g) + // Latest ID and version already stored, no need to retrieve it ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting ms.AWSMachinePool.Status.LaunchTemplateVersion = pointer.String("1") @@ -538,6 +544,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) t.Run("launch template and ASG exist and only AMI ID changed", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) + reconSvc = nil // not used + defer teardown(t, g) + // Latest ID and version already stored, no need to retrieve it ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting ms.AWSMachinePool.Status.LaunchTemplateVersion = pointer.String("1") @@ -585,6 +597,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) t.Run("launch template and ASG exist and only bootstrap data secret name changed", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) + reconSvc = nil // not used + defer teardown(t, g) + // Latest ID and version already stored, no need to retrieve it ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting ms.AWSMachinePool.Status.LaunchTemplateVersion = pointer.String("1") @@ -635,6 +653,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) t.Run("launch template and ASG created from zero, then bootstrap config reference changes", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`) + reconSvc = nil // not used + defer teardown(t, g) + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return(nil, "", nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(pointer.String("ami-abcdef123"), nil) ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(pointer.String("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script"))).Return("lt-ghijkl456", nil) @@ -650,7 +674,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) { g.Expect(err).To(Succeed()) g.Expect(ms.AWSMachinePool.Status.LaunchTemplateID).ToNot(BeEmpty()) - g.Expect(pointer.StringDeref(ms.AWSMachinePool.Status.LaunchTemplateVersion, "")).ToNot(BeEmpty()) // Data secret name changes newBootstrapSecret := &corev1.Secret{ @@ -665,6 +688,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) { g.Expect(testEnv.Create(ctx, newBootstrapSecret)).To(Succeed()) ms.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName = pointer.String(newBootstrapSecret.Name) + // Since `AWSMachinePool.status.launchTemplateVersion` isn't set yet, + // the controller will ask for the current version and then set the status. + ec2Svc.EXPECT().GetLaunchTemplateLatestVersion(gomock.Any()).Return("1", nil) + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return( &expinfrav1.AWSLaunchTemplate{ Name: "test", diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go index 3b1a2d6006..89ec9d9e75 100644 --- a/pkg/cloud/services/ec2/launchtemplate.go +++ b/pkg/cloud/services/ec2/launchtemplate.go @@ -199,7 +199,9 @@ func (s *Service) ReconcileLaunchTemplate( return nil, err } scope.SetLaunchTemplateLatestVersionStatus(launchTemplateVersion) - return nil, scope.PatchObject() + if err = scope.PatchObject(); err != nil { + return nil, err + } } annotation, err := MachinePoolAnnotationJSON(scope, TagsLastAppliedAnnotation)