diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 82ba81e8a9..8d9fe12b7f 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -773,7 +773,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) // no change - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).DoAndReturn(func(scope *scope.MachinePoolScope) (*expinfrav1.AutoScalingGroup, error) { g.Expect(scope.Name()).To(Equal("test")) @@ -822,7 +822,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-different"), nil) - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil) ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil) ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Any(), gomock.Any(), gomock.Eq(ptr.To[string]("ami-different")), gomock.Eq(apimachinerytypes.NamespacedName{Namespace: "default", Name: "bootstrap-data"}), gomock.Any(), gomock.Any()).Return(nil) @@ -878,7 +878,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil) ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil) ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Any(), gomock.Any(), gomock.Eq(ptr.To[string]("ami-existing")), gomock.Eq(apimachinerytypes.NamespacedName{Namespace: "default", Name: "bootstrap-data"}), gomock.Any(), gomock.Any()).Return(nil) @@ -970,7 +970,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil) ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil) ec2Svc.EXPECT().CreateLaunchTemplateVersion(gomock.Any(), gomock.Any(), gomock.Eq(ptr.To[string]("ami-existing")), gomock.Eq(apimachinerytypes.NamespacedName{Namespace: "default", Name: "bootstrap-data-new"}), gomock.Any(), gomock.Any()).Return(nil) @@ -1056,7 +1056,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) s3Mock.EXPECT().PutObject(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, input *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) { g.Expect(*input.Key).To(Equal(fmt.Sprintf("machine-pool/test/%s", userdata.ComputeHash([]byte("shell-script"))))) @@ -1106,7 +1106,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { nil, nil) ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) - ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil) + ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil) s3Mock.EXPECT().PutObject(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, input *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) { g.Expect(*input.Key).To(Equal(fmt.Sprintf("machine-pool/test/%s", userdata.ComputeHash([]byte("shell-script"))))) diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go index 4a1c652fc6..af8867453f 100644 --- a/pkg/cloud/services/ec2/launchtemplate.go +++ b/pkg/cloud/services/ec2/launchtemplate.go @@ -233,7 +233,7 @@ func (s *Service) ReconcileLaunchTemplate( // Check if the instance tags were changed. If they were, create a new LaunchTemplate. tagsChanged, _, _, _ := tagsChanged(annotation, scope.AdditionalTags()) //nolint:dogsled - needsUpdate, err := ec2svc.LaunchTemplateNeedsUpdate(scope, scope.GetLaunchTemplate(), launchTemplate) + needsUpdate, needsUpdateReason, err := ec2svc.LaunchTemplateNeedsUpdate(scope, scope.GetLaunchTemplate(), launchTemplate) if err != nil { return nil, err } @@ -280,7 +280,7 @@ func (s *Service) ReconcileLaunchTemplate( // Create a new launch template version if there's a difference in configuration, tags, // userdata, OR we've discovered a new AMI ID. if needsUpdate || tagsChanged || amiChanged || userDataHashChanged || userDataSecretKeyChanged || launchTemplateNeedsUserDataSecretKeyTag { - scope.Info("creating new version for launch template", "existing", launchTemplate, "incoming", scope.GetLaunchTemplate(), "needsUpdate", needsUpdate, "tagsChanged", tagsChanged, "amiChanged", amiChanged, "userDataHashChanged", userDataHashChanged, "userDataSecretKeyChanged", userDataSecretKeyChanged) + scope.Info("creating new version for launch template", "existing", launchTemplate, "incoming", scope.GetLaunchTemplate(), "needsUpdate", needsUpdate, "needsUpdateReason", needsUpdateReason, "tagsChanged", tagsChanged, "amiChanged", amiChanged, "userDataHashChanged", userDataHashChanged, "userDataSecretKeyChanged", userDataSecretKeyChanged) // There is a limit to the number of Launch Template Versions. // We ensure that the number of versions does not grow without bound by following a simple rule: Before we create a new version, we delete one old version, if there is at least one old version that is not in use. @@ -999,58 +999,59 @@ func (s *Service) SDKToLaunchTemplate(d types.LaunchTemplateVersion) (*expinfrav // // 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. -func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, error) { +func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, string, error) { if incoming.IamInstanceProfile != existing.IamInstanceProfile { - return true, nil + return true, "IamInstanceProfile", nil } if incoming.InstanceType != existing.InstanceType { - return true, nil + return true, "InstanceType", nil } if !cmp.Equal(incoming.InstanceMetadataOptions, existing.InstanceMetadataOptions) { - return true, nil + return true, "InstanceMetadataOptions", nil } if !cmp.Equal(incoming.SpotMarketOptions, existing.SpotMarketOptions) { - return true, nil + return true, "SpotMarketOptions", nil } if !cmp.Equal(incoming.CapacityReservationID, existing.CapacityReservationID) { - return true, nil + return true, "CapacityReservationID", nil } if !cmp.Equal(incoming.PrivateDNSName, existing.PrivateDNSName) { - return true, nil + return true, "PrivateDNSName", nil } - if !cmp.Equal(incoming.SSHKeyName, existing.SSHKeyName) { - return true, nil + // We treat nil and empty string the same (see `createLaunchTemplateData`) + if !cmp.Equal(ptr.Deref(incoming.SSHKeyName, ""), ptr.Deref(existing.SSHKeyName, "")) { + return true, "SSHKeyName", nil } incomingIDs, err := s.GetAdditionalSecurityGroupsIDs(incoming.AdditionalSecurityGroups) if err != nil { - return false, err + return false, "", err } coreIDs, err := s.GetCoreNodeSecurityGroups(scope) if err != nil { - return false, err + return false, "", err } incomingIDs = append(incomingIDs, coreIDs...) existingIDs, err := s.GetAdditionalSecurityGroupsIDs(existing.AdditionalSecurityGroups) if err != nil { - return false, err + return false, "", err } sort.Strings(incomingIDs) sort.Strings(existingIDs) if !cmp.Equal(incomingIDs, existingIDs) { - return true, nil + return true, "AdditionalSecurityGroupsIDs", nil } - return false, nil + return false, "", nil } // DiscoverLaunchTemplateAMI will discover the AMI launch template. diff --git a/pkg/cloud/services/ec2/launchtemplate_test.go b/pkg/cloud/services/ec2/launchtemplate_test.go index cb3b53ad77..c4be8769fd 100644 --- a/pkg/cloud/services/ec2/launchtemplate_test.go +++ b/pkg/cloud/services/ec2/launchtemplate_test.go @@ -588,12 +588,13 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { defer mockCtrl.Finish() tests := []struct { - name string - incoming *expinfrav1.AWSLaunchTemplate - existing *expinfrav1.AWSLaunchTemplate - expect func(m *mocks.MockEC2APIMockRecorder) - want bool - wantErr bool + name string + incoming *expinfrav1.AWSLaunchTemplate + existing *expinfrav1.AWSLaunchTemplate + expect func(m *mocks.MockEC2APIMockRecorder) + want bool + wantNeedsUpdateReason string + wantErr bool }{ { name: "only core security groups, order shouldn't matter", @@ -639,8 +640,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { {ID: aws.String("sg-999")}, }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs", + wantErr: false, }, { name: "new additional security group", @@ -657,8 +659,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { {ID: aws.String("sg-999")}, }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs", + wantErr: false, }, { name: "Should return true if incoming IamInstanceProfile is not same as existing IamInstanceProfile", @@ -672,7 +675,8 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, IamInstanceProfile: "some-other-profile", }, - want: true, + want: true, + wantNeedsUpdateReason: "IamInstanceProfile", }, { name: "Should return true if incoming InstanceType is not same as existing InstanceType", @@ -686,7 +690,8 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, InstanceType: "t3.large", }, - want: true, + want: true, + wantNeedsUpdateReason: "InstanceType", }, { name: "new additional security group with filters", @@ -706,8 +711,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { m.DescribeSecurityGroups(context.TODO(), gomock.Eq(&ec2.DescribeSecurityGroupsInput{Filters: []ec2types.Filter{{Name: aws.String("sg-2"), Values: []string{"test-2"}}}})). Return(&ec2.DescribeSecurityGroupsOutput{SecurityGroups: []ec2types.SecurityGroup{{GroupId: aws.String("sg-2")}}}, nil) }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs", + wantErr: false, }, { name: "new launch template instance metadata options, requiring IMDSv2", @@ -723,8 +729,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { {ID: aws.String("sg-222")}, }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "InstanceMetadataOptions", + wantErr: false, }, { name: "new launch template instance metadata options, removing IMDSv2 requirement", @@ -735,8 +742,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { HTTPTokens: infrav1.HTTPTokensStateRequired, }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "InstanceMetadataOptions", + wantErr: false, }, { name: "Should return true if incoming SpotMarketOptions is different from existing SpotMarketOptions", @@ -754,8 +762,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { MaxPrice: aws.String("0.05"), }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "SpotMarketOptions", + wantErr: false, }, { name: "Should return true if incoming adds SpotMarketOptions and existing has none", @@ -771,8 +780,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, SpotMarketOptions: nil, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "SpotMarketOptions", + wantErr: false, }, { name: "Should return true if incoming removes SpotMarketOptions and existing has some", @@ -788,8 +798,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { MaxPrice: aws.String("0.05"), }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "SpotMarketOptions", + wantErr: false, }, { name: "Should return true if SSH key names are different", @@ -803,8 +814,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, SSHKeyName: aws.String("old-key"), }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "SSHKeyName", + wantErr: false, }, { name: "Should return true if one has SSH key name and other doesn't", @@ -818,8 +830,25 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, SSHKeyName: nil, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "SSHKeyName", + wantErr: false, + }, + { + name: "Should return false if no SSH key is set in the spec and AWS returns no key pair as well", + incoming: &expinfrav1.AWSLaunchTemplate{ + SSHKeyName: aws.String(""), // explicit empty string + }, + existing: &expinfrav1.AWSLaunchTemplate{ + AdditionalSecurityGroups: []infrav1.AWSResourceReference{ + {ID: aws.String("sg-111")}, + {ID: aws.String("sg-222")}, + }, + SSHKeyName: nil, + }, + want: false, + wantNeedsUpdateReason: "", + wantErr: false, }, { name: "Should return true if incoming PrivateDNSName is different from existing PrivateDNSName", @@ -841,8 +870,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { HostnameType: aws.String("ip-name"), }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "PrivateDNSName", + wantErr: false, }, { name: "Should return true if incoming adds PrivateDNSName and existing has none", @@ -860,8 +890,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, PrivateDNSName: nil, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "PrivateDNSName", + wantErr: false, }, { name: "Should return true if incoming removes PrivateDNSName and existing has some", @@ -879,8 +910,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { HostnameType: aws.String("resource-name"), }, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "PrivateDNSName", + wantErr: false, }, { name: "Should return true if capacity reservation IDs are different", @@ -894,8 +926,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, CapacityReservationID: aws.String("old-reservation"), }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "CapacityReservationID", + wantErr: false, }, { name: "Should return true if one has capacity reservation ID and other doesn't", @@ -909,8 +942,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { }, CapacityReservationID: nil, }, - want: true, - wantErr: false, + want: true, + wantNeedsUpdateReason: "CapacityReservationID", + wantErr: false, }, } for _, tt := range tests { @@ -948,12 +982,13 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) { tt.expect(mockEC2Client.EXPECT()) } - got, err := s.LaunchTemplateNeedsUpdate(machinePoolScope, tt.incoming, tt.existing) + got, gotNeedsUpdateReason, err := s.LaunchTemplateNeedsUpdate(machinePoolScope, tt.incoming, tt.existing) if tt.wantErr { g.Expect(err).To(HaveOccurred()) return } g.Expect(err).NotTo(HaveOccurred()) + g.Expect(gotNeedsUpdateReason).Should(Equal(tt.wantNeedsUpdateReason)) g.Expect(got).Should(Equal(tt.want)) }) } diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go index 5b4eccb30e..c8e4df8ef6 100644 --- a/pkg/cloud/services/interfaces.go +++ b/pkg/cloud/services/interfaces.go @@ -86,7 +86,7 @@ type EC2Interface interface { CreateLaunchTemplateVersion(id string, scope scope.LaunchTemplateScope, imageID *string, userDataSecretKey apimachinerytypes.NamespacedName, userData []byte, bootstrapDataHash string) error PruneLaunchTemplateVersions(id string) (*ec2types.LaunchTemplateVersion, error) DeleteLaunchTemplate(id string) error - LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, error) + LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, string, error) DeleteBastion() error ReconcileBastion() error // ReconcileElasticIPFromPublicPool reconciles the elastic IP from a custom Public IPv4 Pool. diff --git a/pkg/cloud/services/mock_services/ec2_interface_mock.go b/pkg/cloud/services/mock_services/ec2_interface_mock.go index 6b923ac502..03e3121acc 100644 --- a/pkg/cloud/services/mock_services/ec2_interface_mock.go +++ b/pkg/cloud/services/mock_services/ec2_interface_mock.go @@ -310,12 +310,13 @@ func (mr *MockEC2InterfaceMockRecorder) InstanceIfExists(arg0 interface{}) *gomo } // LaunchTemplateNeedsUpdate mocks base method. -func (m *MockEC2Interface) LaunchTemplateNeedsUpdate(arg0 scope.LaunchTemplateScope, arg1, arg2 *v1beta20.AWSLaunchTemplate) (bool, error) { +func (m *MockEC2Interface) LaunchTemplateNeedsUpdate(arg0 scope.LaunchTemplateScope, arg1, arg2 *v1beta20.AWSLaunchTemplate) (bool, string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "LaunchTemplateNeedsUpdate", arg0, arg1, arg2) ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 } // LaunchTemplateNeedsUpdate indicates an expected call of LaunchTemplateNeedsUpdate.