From b0f83a8aa8254ae44a5ce928c2e39da50f44480d Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Tue, 25 Apr 2023 13:55:20 +0200 Subject: [PATCH] Trigger machine pool instance refresh also on user data change (unless it's only the bootstrap token) --- exp/controllers/awsmachinepool_controller.go | 11 +- .../awsmachinepool_controller_test.go | 215 ++++++++++++++++-- pkg/cloud/scope/machinepool.go | 2 +- pkg/cloud/services/ec2/launchtemplate.go | 70 ++++-- pkg/cloud/services/ec2/launchtemplate_test.go | 29 +-- pkg/cloud/services/interfaces.go | 2 +- .../mock_services/ec2_interface_mock.go | 4 +- 7 files changed, 272 insertions(+), 61 deletions(-) diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go index 8095b267c9..60dd7647c8 100644 --- a/exp/controllers/awsmachinepool_controller.go +++ b/exp/controllers/awsmachinepool_controller.go @@ -229,7 +229,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP canUpdateLaunchTemplate := func() (bool, error) { // If there is a change: before changing the template, check if there exist an ongoing instance refresh, // because only 1 instance refresh can be "InProgress". If template is updated when refresh cannot be started, - // that change will not trigger a refresh. Do not start an instance refresh if only userdata changed. + // that change will not trigger a refresh. return asgsvc.CanStartASGInstanceRefresh(machinePoolScope) } runPostLaunchTemplateUpdateOperation := func() error { @@ -238,15 +238,14 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP machinePoolScope.Debug("instance refresh disabled, skipping instance refresh") return nil } + // After creating a new version of launch template, instance refresh is required // to trigger a rolling replacement of all previously launched instances. - // If ONLY the userdata changed, previously launched instances continue to use the old launch - // template. + // If ONLY the the bootstrap token in userdata changed, previously launched instances continue + // using the old launch template. // // FIXME(dlipovetsky,sedefsavas): If the controller terminates, or the StartASGInstanceRefresh returns an error, - // this conditional will not evaluate to true the next reconcile. If any machines use an older - // Launch Template version, and the difference between the older and current versions is _more_ - // than userdata, we should start an Instance Refresh. + // this conditional will not evaluate to true the next reconcile. machinePoolScope.Info("starting instance refresh", "number of instances", machinePoolScope.MachinePool.Spec.Replicas) return asgsvc.StartASGInstanceRefresh(machinePoolScope) } diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 3ad850f8d8..412609dbab 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -21,8 +21,10 @@ import ( "context" "flag" "fmt" + "strings" "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/go-logr/logr" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" @@ -30,6 +32,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" "k8s.io/utils/pointer" @@ -40,6 +43,7 @@ import ( "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" + ec2Service "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/ec2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/mock_services" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -50,17 +54,38 @@ import ( "sigs.k8s.io/cluster-api/util/patch" ) +const ( + clusterApiTestUserData string = ` +--- +- path: /run/kubeadm/kubeadm-join-config.yaml + owner: root:root + permissions: '0640' + content: | + --- + apiVersion: kubeadm.k8s.io/v1beta3 + discovery: + bootstrapToken: + apiServerEndpoint: my-apiserver-123.eu-west-2.elb.amazonaws.com:6443 + caCertHashes: + - sha256:45d45ge54dc2ea64cd30b133713371337133771ac2cffc43932f984f3e76babc + token: 31o123.abcd4lsoz1ruwxyz + +...some other stuff that is not valid YAML, since it should not matter... +` +) + func TestAWSMachinePoolReconciler(t *testing.T) { var ( - reconciler AWSMachinePoolReconciler - cs *scope.ClusterScope - ms *scope.MachinePoolScope - mockCtrl *gomock.Controller - ec2Svc *mock_services.MockEC2Interface - asgSvc *mock_services.MockASGInterface - recorder *record.FakeRecorder - awsMachinePool *expinfrav1.AWSMachinePool - secret *corev1.Secret + reconciler AWSMachinePoolReconciler + cs *scope.ClusterScope + ms *scope.MachinePoolScope + mockCtrl *gomock.Controller + ec2Svc *mock_services.MockEC2Interface + asgSvc *mock_services.MockASGInterface + recorder *record.FakeRecorder + awsMachinePool *expinfrav1.AWSMachinePool + secret *corev1.Secret + bootstrapDataToCreate = []byte("shell-script") ) setup := func(t *testing.T, g *WithT) { t.Helper() @@ -75,10 +100,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) { } ctx := context.TODO() + ns := string(uuid.NewUUID()) + awsMachinePool = &expinfrav1.AWSMachinePool{ ObjectMeta: metav1.ObjectMeta{ Name: "test", - Namespace: "default", + Namespace: ns, }, Spec: expinfrav1.AWSMachinePoolSpec{ MinSize: int32(0), @@ -89,16 +116,24 @@ func TestAWSMachinePoolReconciler(t *testing.T) { secret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "bootstrap-data", - Namespace: "default", + Namespace: ns, }, Data: map[string][]byte{ - "value": []byte("shell-script"), + "value": bootstrapDataToCreate, }, } + g.Expect(testEnv.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: ns, + }, + })).To(Succeed()) g.Expect(testEnv.Create(ctx, awsMachinePool)).To(Succeed()) g.Expect(testEnv.Create(ctx, secret)).To(Succeed()) + cs, err = setupCluster("test-cluster") + g.Expect(err).To(BeNil()) + ms, err = scope.NewMachinePoolScope( scope.MachinePoolScopeParams{ Client: testEnv.Client, @@ -110,7 +145,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MachinePool: &expclusterv1.MachinePool{ ObjectMeta: metav1.ObjectMeta{ Name: "mp", - Namespace: "default", + Namespace: ns, }, Spec: expclusterv1.MachinePoolSpec{ ClusterName: "test", @@ -130,9 +165,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) { ) g.Expect(err).To(BeNil()) - cs, err = setupCluster("test-cluster") - g.Expect(err).To(BeNil()) - mockCtrl = gomock.NewController(t) ec2Svc = mock_services.NewMockEC2Interface(mockCtrl) asgSvc = mock_services.NewMockASGInterface(mockCtrl) @@ -170,7 +202,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { getASG := func(t *testing.T, g *WithT) { t.Helper() - ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", expectedErr).AnyTimes() + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, nil, expectedErr).AnyTimes() asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, expectedErr).AnyTimes() } t.Run("should exit immediately on an error state", func(t *testing.T) { @@ -364,7 +396,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes() asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1) asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes() - ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", nil).AnyTimes() + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, nil, nil).AnyTimes() ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(nil, nil).AnyTimes() ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return("", nil).AnyTimes() ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() @@ -464,7 +496,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { finalizer(t, g) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, nil) - ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", nil).AnyTimes() + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, nil, nil).AnyTimes() buf := new(bytes.Buffer) klog.SetOutput(buf) @@ -486,7 +518,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { Status: expinfrav1.ASGStatusDeleteInProgress, } asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&inProgressASG, nil) - ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", nil).AnyTimes() + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, nil, nil).AnyTimes() buf := new(bytes.Buffer) klog.SetOutput(buf) @@ -496,6 +528,149 @@ func TestAWSMachinePoolReconciler(t *testing.T) { g.Eventually(recorder.Events).Should(Receive(ContainSubstring("DeletionInProgress"))) }) }) + + t.Run("Reconciling the launch template", func(t *testing.T) { + // Test `ReconcileLaunchTemplate` which is called by reconciliation + + t.Run("should create launch template if none exists", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, nil, nil) + ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(aws.String("ami-123456"), nil) + ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Eq([]byte("shell-script"))).Return("lt-456789", nil) + + instanceRefreshTriggered := false + ec2Service.ReconcileLaunchTemplate(ms, func() (bool, error) { return true, nil }, func() error { + instanceRefreshTriggered = true + return nil + }, ec2Svc) + // First launch template, so no instance refresh needed + g.Expect(instanceRefreshTriggered).To(BeFalse()) + }) + + t.Run("should not update launch template with unchanged AMI and user data", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(&expinfrav1.AWSLaunchTemplate{ + AMI: infrav1.AMIReference{ + ID: aws.String("ami-123456"), + }, + }, []byte("shell-script"), nil) + ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(aws.String("ami-123456"), nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + + ms.AWSMachinePool.Status.LaunchTemplateID = "lt-foo123" + ms.AWSMachinePool.Status.LaunchTemplateVersion = aws.String("1") + + instanceRefreshTriggered := false + ec2Service.ReconcileLaunchTemplate(ms, func() (bool, error) { return true, nil }, func() error { + instanceRefreshTriggered = true + return nil + }, ec2Svc) + // No changes, so no instance refresh needed + g.Expect(instanceRefreshTriggered).To(BeFalse()) + }) + + t.Run("should trigger instance refresh if user data change is more than only the bootstrap token", func(t *testing.T) { + g := NewWithT(t) + bootstrapDataToCreate = []byte(clusterApiTestUserData + "this has changed") + setup(t, g) + defer teardown(t, g) + + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(&expinfrav1.AWSLaunchTemplate{ + AMI: infrav1.AMIReference{ + ID: aws.String("ami-123456"), + }, + }, []byte(clusterApiTestUserData), nil) + ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(aws.String("ami-123456"), nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + + ms.AWSMachinePool.Status.LaunchTemplateID = "lt-foo123" + ms.AWSMachinePool.Status.LaunchTemplateVersion = aws.String("1") + + // User data changed, so a new launch template version should be created + ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Eq("lt-foo123")).Return(nil) + ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Eq("lt-foo123"), gomock.Any(), gomock.Eq(aws.String("ami-123456")), gomock.Eq([]byte(clusterApiTestUserData+"this has changed"))).Return(nil) + ec2Svc.EXPECT().GetLaunchTemplateLatestVersion(gomock.Eq("lt-foo123")).Return("2", nil) + + instanceRefreshTriggered := false + ec2Service.ReconcileLaunchTemplate(ms, func() (bool, error) { return true, nil }, func() error { + instanceRefreshTriggered = true + return nil + }, ec2Svc) + // User data changed (more than just the bootstrap token), so an instance refresh is desired + g.Expect(instanceRefreshTriggered).To(BeTrue()) + }) + + t.Run("should not trigger instance refresh if only bootstrap token changed in user data", func(t *testing.T) { + g := NewWithT(t) + bootstrapDataToCreate = []byte(strings.Replace(clusterApiTestUserData, "31o123.abcd4lsoz1ruwxyz", "some-refreshed-token", 1)) + setup(t, g) + defer teardown(t, g) + + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(&expinfrav1.AWSLaunchTemplate{ + AMI: infrav1.AMIReference{ + ID: aws.String("ami-123456"), + }, + }, []byte(clusterApiTestUserData), nil) + ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(aws.String("ami-123456"), nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + + ms.AWSMachinePool.Status.LaunchTemplateID = "lt-foo123" + ms.AWSMachinePool.Status.LaunchTemplateVersion = aws.String("1") + + // User data changed, so a new launch template version should be created + ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Eq("lt-foo123")).Return(nil) + ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Eq("lt-foo123"), gomock.Any(), gomock.Eq(aws.String("ami-123456")), gomock.Eq([]byte(strings.Replace(clusterApiTestUserData, "31o123.abcd4lsoz1ruwxyz", "some-refreshed-token", 1)))).Return(nil) + ec2Svc.EXPECT().GetLaunchTemplateLatestVersion(gomock.Eq("lt-foo123")).Return("2", nil) + + instanceRefreshTriggered := false + ec2Service.ReconcileLaunchTemplate(ms, func() (bool, error) { return true, nil }, func() error { + instanceRefreshTriggered = true + return nil + }, ec2Svc) + // Only the bootstrap token changed within the user data. This should not trigger an instance + // refresh because the token is refreshed regularly. + g.Expect(instanceRefreshTriggered).To(BeFalse()) + }) + + t.Run("should not trigger instance refresh if the bootstrap token cannot be parsed in user data", func(t *testing.T) { + g := NewWithT(t) + bootstrapDataToCreate = []byte(strings.Replace(clusterApiTestUserData, "token: 31o123.abcd4lsoz1ruwxyz", "--unparseable--", 1)) + setup(t, g) + defer teardown(t, g) + + ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(&expinfrav1.AWSLaunchTemplate{ + AMI: infrav1.AMIReference{ + ID: aws.String("ami-123456"), + }, + }, []byte(clusterApiTestUserData), nil) + ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(aws.String("ami-123456"), nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + + ms.AWSMachinePool.Status.LaunchTemplateID = "lt-foo123" + ms.AWSMachinePool.Status.LaunchTemplateVersion = aws.String("1") + + // User data changed, so a new launch template version should be created + ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Eq("lt-foo123")).Return(nil) + ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Eq("lt-foo123"), gomock.Any(), gomock.Eq(aws.String("ami-123456")), gomock.Eq([]byte(strings.Replace(clusterApiTestUserData, "token: 31o123.abcd4lsoz1ruwxyz", "--unparseable--", 1)))).Return(nil) + ec2Svc.EXPECT().GetLaunchTemplateLatestVersion(gomock.Eq("lt-foo123")).Return("2", nil) + + instanceRefreshTriggered := false + ec2Service.ReconcileLaunchTemplate(ms, func() (bool, error) { return true, nil }, func() error { + instanceRefreshTriggered = true + return nil + }, ec2Svc) + // If we cannot parse the bootstrap token out of the user data, we should go the safe route and + // not trigger an instance refresh (since that may lead to re-creating nodes all the time, meaning + // workload application downtime). + g.Expect(instanceRefreshTriggered).To(BeFalse()) + }) + }) } //TODO: This was taken from awsmachine_controller_test, i think it should be moved to elsewhere in both locations like test/helpers diff --git a/pkg/cloud/scope/machinepool.go b/pkg/cloud/scope/machinepool.go index 9d2d2935b4..b26377cb2b 100644 --- a/pkg/cloud/scope/machinepool.go +++ b/pkg/cloud/scope/machinepool.go @@ -167,7 +167,7 @@ func (m *MachinePoolScope) getBootstrapData() ([]byte, string, error) { key := types.NamespacedName{Namespace: m.Namespace(), Name: *m.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName} if err := m.Client.Get(context.TODO(), key, secret); err != nil { - return nil, "", errors.Wrapf(err, "failed to retrieve bootstrap data secret for AWSMachine %s/%s", m.Namespace(), m.Name()) + return nil, "", errors.Wrapf(err, "failed to retrieve bootstrap data secret for AWSMachinePool %s/%s", m.Namespace(), m.Name()) } value, ok := secret.Data["value"] diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go index 062b3ee3c7..6aa00c83e5 100644 --- a/pkg/cloud/services/ec2/launchtemplate.go +++ b/pkg/cloud/services/ec2/launchtemplate.go @@ -17,8 +17,10 @@ limitations under the License. package ec2 import ( + "bytes" "encoding/base64" "encoding/json" + "regexp" "sort" "strconv" "strings" @@ -34,6 +36,7 @@ import ( expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/userdata" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/record" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -50,10 +53,17 @@ const ( TagsLastAppliedAnnotation = "sigs.k8s.io/cluster-api-provider-aws-last-applied-tags" ) -func (s *Service) ReconcileLaunchTemplate( +var ( + // Matches bootstrap token and value as found in cluster-api's node join configuration + // (see `cluster-api/bootstrap/kubeadm/internal/cloudinit/`) + clusterApiCloudInitUserDataBootstrapTokenRegex = regexp.MustCompile(`(?m)\btoken: \S+$`) +) + +func ReconcileLaunchTemplate( scope scope.LaunchTemplateScope, canUpdateLaunchTemplate func() (bool, error), runPostLaunchTemplateUpdateOperation func() error, + ec2svc services.EC2Interface, ) error { bootstrapData, err := scope.GetRawBootstrapData() if err != nil { @@ -61,14 +71,31 @@ func (s *Service) ReconcileLaunchTemplate( } bootstrapDataHash := userdata.ComputeHash(bootstrapData) - ec2svc := NewService(scope.GetEC2Scope()) - scope.Info("checking for existing launch template") - launchTemplate, launchTemplateUserDataHash, err := ec2svc.GetLaunchTemplate(scope.LaunchTemplateName()) + launchTemplate, launchTemplateUserData, err := ec2svc.GetLaunchTemplate(scope.LaunchTemplateName()) if err != nil { conditions.MarkUnknown(scope.GetSetter(), expinfrav1.LaunchTemplateReadyCondition, expinfrav1.LaunchTemplateNotFoundReason, err.Error()) return err } + launchTemplateUserDataHash := userdata.ComputeHash(launchTemplateUserData) + + userDataHasRelevantChanges := false + bootstrapDataWithoutToken := clusterApiCloudInitUserDataBootstrapTokenRegex.ReplaceAllLiteral(bootstrapData, []byte("")) + launchTemplateUserDataWithoutToken := clusterApiCloudInitUserDataBootstrapTokenRegex.ReplaceAllLiteral(launchTemplateUserData, []byte("")) + + // Only consider instance refresh if the token could be parsed in the old and new user data. + // In other words: if this diffing feature breaks, we don't want to roll nodes every few minutes + // and cause application downtime. + if !bytes.Equal(bootstrapData, bootstrapDataWithoutToken) { + if !bytes.Equal(bootstrapDataWithoutToken, launchTemplateUserDataWithoutToken) { + // More than the bootstrap token changed, so consider instance refresh + userDataHasRelevantChanges = true + } + } else { + // Since we make assumptions about the user data content above, we should notice when the feature + // breaks, so log this case + scope.Warn("could not parse bootstrap token from userdata, not considering instance refresh if the user data changed") + } imageID, err := ec2svc.DiscoverLaunchTemplateAMI(scope) if err != nil { @@ -123,7 +150,7 @@ func (s *Service) ReconcileLaunchTemplate( return err } - if needsUpdate || tagsChanged || *imageID != *launchTemplate.AMI.ID { + if needsUpdate || tagsChanged || *imageID != *launchTemplate.AMI.ID || userDataHasRelevantChanges { canUpdate, err := canUpdateLaunchTemplate() if err != nil { return err @@ -157,7 +184,7 @@ func (s *Service) ReconcileLaunchTemplate( } } - if needsUpdate || tagsChanged || *imageID != *launchTemplate.AMI.ID { + if needsUpdate || tagsChanged || *imageID != *launchTemplate.AMI.ID || userDataHasRelevantChanges { if err := runPostLaunchTemplateUpdateOperation(); err != nil { conditions.MarkFalse(scope.GetSetter(), expinfrav1.PostLaunchTemplateUpdateOperationCondition, expinfrav1.PostLaunchTemplateUpdateOperationFailedReason, clusterv1.ConditionSeverityError, err.Error()) return err @@ -168,6 +195,15 @@ func (s *Service) ReconcileLaunchTemplate( return nil } +func (s *Service) ReconcileLaunchTemplate( + scope scope.LaunchTemplateScope, + canUpdateLaunchTemplate func() (bool, error), + runPostLaunchTemplateUpdateOperation func() error, +) error { + ec2svc := NewService(scope.GetEC2Scope()) + return ReconcileLaunchTemplate(scope, canUpdateLaunchTemplate, runPostLaunchTemplateUpdateOperation, ec2svc) +} + func (s *Service) ReconcileTags(scope scope.LaunchTemplateScope, resourceServicesToUpdate []scope.ResourceServiceToUpdate) error { additionalTags := scope.AdditionalTags() @@ -321,9 +357,9 @@ func tagsChanged(annotation map[string]interface{}, src map[string]string) (bool // GetLaunchTemplate returns the existing LaunchTemplate or nothing if it doesn't exist. // For now by name until we need the input to be something different. -func (s *Service) GetLaunchTemplate(launchTemplateName string) (*expinfrav1.AWSLaunchTemplate, string, error) { +func (s *Service) GetLaunchTemplate(launchTemplateName string) (*expinfrav1.AWSLaunchTemplate, []byte, error) { if launchTemplateName == "" { - return nil, "", nil + return nil, nil, nil } s.scope.Debug("Looking for existing LaunchTemplates") @@ -336,13 +372,13 @@ func (s *Service) GetLaunchTemplate(launchTemplateName string) (*expinfrav1.AWSL out, err := s.EC2Client.DescribeLaunchTemplateVersions(input) switch { case awserrors.IsNotFound(err): - return nil, "", nil + return nil, nil, nil case err != nil: - return nil, "", err + return nil, nil, err } if out == nil || out.LaunchTemplateVersions == nil || len(out.LaunchTemplateVersions) == 0 { - return nil, "", nil + return nil, nil, nil } return s.SDKToLaunchTemplate(out.LaunchTemplateVersions[0]) @@ -637,7 +673,7 @@ func (s *Service) deleteLaunchTemplateVersion(id string, version *int64) error { } // SDKToLaunchTemplate converts an AWS EC2 SDK instance to the CAPA instance type. -func (s *Service) SDKToLaunchTemplate(d *ec2.LaunchTemplateVersion) (*expinfrav1.AWSLaunchTemplate, string, error) { +func (s *Service) SDKToLaunchTemplate(d *ec2.LaunchTemplateVersion) (*expinfrav1.AWSLaunchTemplate, []byte, error) { v := d.LaunchTemplateData i := &expinfrav1.AWSLaunchTemplate{ Name: aws.StringValue(d.LaunchTemplateName), @@ -669,20 +705,20 @@ func (s *Service) SDKToLaunchTemplate(d *ec2.LaunchTemplateVersion) (*expinfrav1 } if v.UserData == nil { - return i, userdata.ComputeHash(nil), nil + return i, nil, nil } decodedUserData, err := base64.StdEncoding.DecodeString(*v.UserData) if err != nil { - return nil, "", errors.Wrap(err, "unable to decode UserData") + return nil, nil, errors.Wrap(err, "unable to decode UserData") } - return i, userdata.ComputeHash(decodedUserData), nil + return i, decodedUserData, nil } // LaunchTemplateNeedsUpdate checks if a new launch template version is needed. // -// FIXME(dlipovetsky): This check should account for changed userdata, but does not yet do so. -// Although userdata is stored in an EC2 Launch Template, it is not a field of AWSLaunchTemplate. +// It does not compare user data since that is not a field of AWSLaunchTemplate. +// See `ReconcileLaunchTemplate` for that logic. func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, error) { if incoming.IamInstanceProfile != existing.IamInstanceProfile { return true, nil diff --git a/pkg/cloud/services/ec2/launchtemplate_test.go b/pkg/cloud/services/ec2/launchtemplate_test.go index 96395f681a..95f52962cd 100644 --- a/pkg/cloud/services/ec2/launchtemplate_test.go +++ b/pkg/cloud/services/ec2/launchtemplate_test.go @@ -87,13 +87,13 @@ func TestGetLaunchTemplate(t *testing.T) { name string launchTemplateName string expect func(m *mocks.MockEC2APIMockRecorder) - check func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) + check func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) }{ { name: "Should not return launch template if empty launch template name passed", - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { g.Expect(err).NotTo(HaveOccurred()) - g.Expect(userDataHash).Should(BeEmpty()) + g.Expect(userData).Should(BeNil()) g.Expect(launchTemplate).Should(BeNil()) }, }, @@ -111,9 +111,9 @@ func TestGetLaunchTemplate(t *testing.T) { nil, )) }, - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { g.Expect(err).NotTo(HaveOccurred()) - g.Expect(userDataHash).Should(BeEmpty()) + g.Expect(userData).Should(BeNil()) g.Expect(launchTemplate).Should(BeNil()) }, }, @@ -126,9 +126,9 @@ func TestGetLaunchTemplate(t *testing.T) { Versions: []*string{aws.String("$Latest")}, })).Return(nil, awserrors.NewFailedDependency("dependency failure")) }, - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { g.Expect(err).To(HaveOccurred()) - g.Expect(userDataHash).Should(BeEmpty()) + g.Expect(userData).Should(BeNil()) g.Expect(launchTemplate).Should(BeNil()) }, }, @@ -141,9 +141,9 @@ func TestGetLaunchTemplate(t *testing.T) { Versions: []*string{aws.String("$Latest")}, })).Return(nil, nil) }, - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { g.Expect(err).NotTo(HaveOccurred()) - g.Expect(userDataHash).Should(BeEmpty()) + g.Expect(userData).Should(BeNil()) g.Expect(launchTemplate).Should(BeNil()) }, }, @@ -189,7 +189,7 @@ func TestGetLaunchTemplate(t *testing.T) { }, }, nil) }, - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { wantLT := &expinfrav1.AWSLaunchTemplate{ Name: "foo", AMI: infrav1.AMIReference{ @@ -202,7 +202,7 @@ func TestGetLaunchTemplate(t *testing.T) { } g.Expect(err).NotTo(HaveOccurred()) - g.Expect(userDataHash).Should(Equal(testUserDataHash)) + g.Expect(userdata.ComputeHash(userData)).Should(Equal(testUserDataHash)) g.Expect(launchTemplate).Should(Equal(wantLT)) }, }, @@ -247,7 +247,7 @@ func TestGetLaunchTemplate(t *testing.T) { }, }, nil) }, - check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) { + check: func(g *WithT, launchTemplate *expinfrav1.AWSLaunchTemplate, userData []byte, err error) { wantLT := &expinfrav1.AWSLaunchTemplate{ Name: "foo", AMI: infrav1.AMIReference{ @@ -260,7 +260,7 @@ func TestGetLaunchTemplate(t *testing.T) { } g.Expect(err).NotTo(HaveOccurred()) - g.Expect(userDataHash).Should(Equal(userdata.ComputeHash(nil))) + g.Expect(userData).Should(BeNil()) g.Expect(launchTemplate).Should(Equal(wantLT)) }, }, @@ -344,7 +344,8 @@ func TestServiceSDKToLaunchTemplate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{} - gotLT, gotHash, err := s.SDKToLaunchTemplate(tt.input) + gotLT, gotUserData, err := s.SDKToLaunchTemplate(tt.input) + gotHash := userdata.ComputeHash(gotUserData) if (err != nil) != tt.wantErr { t.Fatalf("error mismatch: got %v, wantErr %v", err, tt.wantErr) } diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go index 992b2c5b8d..6cafd5912b 100644 --- a/pkg/cloud/services/interfaces.go +++ b/pkg/cloud/services/interfaces.go @@ -69,7 +69,7 @@ type EC2Interface interface { ReconcileTags(scope scope.LaunchTemplateScope, resourceServicesToUpdate []scope.ResourceServiceToUpdate) error DiscoverLaunchTemplateAMI(scope scope.LaunchTemplateScope) (*string, error) - GetLaunchTemplate(id string) (lt *expinfrav1.AWSLaunchTemplate, userDataHash string, err error) + GetLaunchTemplate(id string) (lt *expinfrav1.AWSLaunchTemplate, userData []byte, err error) GetLaunchTemplateID(id string) (string, error) GetLaunchTemplateLatestVersion(id string) (string, error) CreateLaunchTemplate(scope scope.LaunchTemplateScope, imageID *string, userData []byte) (string, error) diff --git a/pkg/cloud/services/mock_services/ec2_interface_mock.go b/pkg/cloud/services/mock_services/ec2_interface_mock.go index 7f8cb07aaf..0d95b2b6fd 100644 --- a/pkg/cloud/services/mock_services/ec2_interface_mock.go +++ b/pkg/cloud/services/mock_services/ec2_interface_mock.go @@ -199,11 +199,11 @@ func (mr *MockEC2InterfaceMockRecorder) GetInstanceSecurityGroups(arg0 interface } // GetLaunchTemplate mocks base method. -func (m *MockEC2Interface) GetLaunchTemplate(arg0 string) (*v1beta20.AWSLaunchTemplate, string, error) { +func (m *MockEC2Interface) GetLaunchTemplate(arg0 string) (*v1beta20.AWSLaunchTemplate, []byte, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetLaunchTemplate", arg0) ret0, _ := ret[0].(*v1beta20.AWSLaunchTemplate) - ret1, _ := ret[1].(string) + ret1, _ := ret[1].([]byte) ret2, _ := ret[2].(error) return ret0, ret1, ret2 }