Skip to content

Commit 7535124

Browse files
AndiDogAndiDog
committed
Print reason for launch template needing update, fix constant instance refreshes for AWSMachinePool by considering SSHKeyName nil and empty string the same (#643)
1 parent ea774f5 commit 7535124

File tree

5 files changed

+102
-65
lines changed

5 files changed

+102
-65
lines changed

‎exp/controllers/awsmachinepool_controller_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -773,7 +773,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
773773
nil,
774774
nil)
775775
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil) // no change
776-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
776+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
777777

778778
asgSvc.EXPECT().GetASGByName(gomock.Any()).DoAndReturn(func(scope *scope.MachinePoolScope) (*expinfrav1.AutoScalingGroup, error) {
779779
g.Expect(scope.Name()).To(Equal("test"))
@@ -822,7 +822,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
822822
nil,
823823
nil)
824824
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-different"), nil)
825-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
825+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
826826
asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil)
827827
ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil)
828828
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) {
878878
nil,
879879
nil)
880880
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil)
881-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
881+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
882882
asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil)
883883
ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil)
884884
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) {
970970
nil,
971971
nil)
972972
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil)
973-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
973+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
974974
asgSvc.EXPECT().CanStartASGInstanceRefresh(gomock.Any()).Return(true, nil, nil)
975975
ec2Svc.EXPECT().PruneLaunchTemplateVersions(gomock.Any()).Return(nil, nil)
976976
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) {
10561056
nil,
10571057
nil)
10581058
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil)
1059-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
1059+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
10601060

10611061
s3Mock.EXPECT().PutObject(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, input *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
10621062
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) {
11061106
nil,
11071107
nil)
11081108
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any(), gomock.Any()).Return(ptr.To[string]("ami-existing"), nil)
1109-
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
1109+
ec2Svc.EXPECT().LaunchTemplateNeedsUpdate(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, "", nil)
11101110

11111111
s3Mock.EXPECT().PutObject(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, input *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
11121112
g.Expect(*input.Key).To(Equal(fmt.Sprintf("machine-pool/test/%s", userdata.ComputeHash([]byte("shell-script")))))

‎pkg/cloud/services/ec2/launchtemplate.go‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ func (s *Service) ReconcileLaunchTemplate(
233233
// Check if the instance tags were changed. If they were, create a new LaunchTemplate.
234234
tagsChanged, _, _, _ := tagsChanged(annotation, scope.AdditionalTags()) //nolint:dogsled
235235

236-
needsUpdate, err := ec2svc.LaunchTemplateNeedsUpdate(scope, scope.GetLaunchTemplate(), launchTemplate)
236+
needsUpdate, needsUpdateReason, err := ec2svc.LaunchTemplateNeedsUpdate(scope, scope.GetLaunchTemplate(), launchTemplate)
237237
if err != nil {
238238
return nil, err
239239
}
@@ -280,7 +280,7 @@ func (s *Service) ReconcileLaunchTemplate(
280280
// Create a new launch template version if there's a difference in configuration, tags,
281281
// userdata, OR we've discovered a new AMI ID.
282282
if needsUpdate || tagsChanged || amiChanged || userDataHashChanged || userDataSecretKeyChanged || launchTemplateNeedsUserDataSecretKeyTag {
283-
scope.Info("creating new version for launch template", "existing", launchTemplate, "incoming", scope.GetLaunchTemplate(), "needsUpdate", needsUpdate, "tagsChanged", tagsChanged, "amiChanged", amiChanged, "userDataHashChanged", userDataHashChanged, "userDataSecretKeyChanged", userDataSecretKeyChanged)
283+
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)
284284

285285
// There is a limit to the number of Launch Template Versions.
286286
// 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
999999
//
10001000
// FIXME(dlipovetsky): This check should account for changed userdata, but does not yet do so.
10011001
// Although userdata is stored in an EC2 Launch Template, it is not a field of AWSLaunchTemplate.
1002-
func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, error) {
1002+
func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, incoming *expinfrav1.AWSLaunchTemplate, existing *expinfrav1.AWSLaunchTemplate) (bool, string, error) {
10031003
if incoming.IamInstanceProfile != existing.IamInstanceProfile {
1004-
return true, nil
1004+
return true, "IamInstanceProfile", nil
10051005
}
10061006

10071007
if incoming.InstanceType != existing.InstanceType {
1008-
return true, nil
1008+
return true, "InstanceType", nil
10091009
}
10101010

10111011
if !cmp.Equal(incoming.InstanceMetadataOptions, existing.InstanceMetadataOptions) {
1012-
return true, nil
1012+
return true, "InstanceMetadataOptions", nil
10131013
}
10141014

10151015
if !cmp.Equal(incoming.SpotMarketOptions, existing.SpotMarketOptions) {
1016-
return true, nil
1016+
return true, "SpotMarketOptions", nil
10171017
}
10181018

10191019
if !cmp.Equal(incoming.CapacityReservationID, existing.CapacityReservationID) {
1020-
return true, nil
1020+
return true, "CapacityReservationID", nil
10211021
}
10221022

10231023
if !cmp.Equal(incoming.PrivateDNSName, existing.PrivateDNSName) {
1024-
return true, nil
1024+
return true, "PrivateDNSName", nil
10251025
}
10261026

1027-
if !cmp.Equal(incoming.SSHKeyName, existing.SSHKeyName) {
1028-
return true, nil
1027+
// We treat nil and empty string the same (see `createLaunchTemplateData`)
1028+
if !cmp.Equal(ptr.Deref(incoming.SSHKeyName, ""), ptr.Deref(existing.SSHKeyName, "")) {
1029+
return true, "SSHKeyName", nil
10291030
}
10301031

10311032
incomingIDs, err := s.GetAdditionalSecurityGroupsIDs(incoming.AdditionalSecurityGroups)
10321033
if err != nil {
1033-
return false, err
1034+
return false, "", err
10341035
}
10351036

10361037
coreIDs, err := s.GetCoreNodeSecurityGroups(scope)
10371038
if err != nil {
1038-
return false, err
1039+
return false, "", err
10391040
}
10401041

10411042
incomingIDs = append(incomingIDs, coreIDs...)
10421043
existingIDs, err := s.GetAdditionalSecurityGroupsIDs(existing.AdditionalSecurityGroups)
10431044
if err != nil {
1044-
return false, err
1045+
return false, "", err
10451046
}
10461047
sort.Strings(incomingIDs)
10471048
sort.Strings(existingIDs)
10481049

10491050
if !cmp.Equal(incomingIDs, existingIDs) {
1050-
return true, nil
1051+
return true, "AdditionalSecurityGroupsIDs", nil
10511052
}
10521053

1053-
return false, nil
1054+
return false, "", nil
10541055
}
10551056

10561057
// DiscoverLaunchTemplateAMI will discover the AMI launch template.

‎pkg/cloud/services/ec2/launchtemplate_test.go‎

Lines changed: 74 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -588,12 +588,13 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
588588
defer mockCtrl.Finish()
589589

590590
tests := []struct {
591-
name string
592-
incoming *expinfrav1.AWSLaunchTemplate
593-
existing *expinfrav1.AWSLaunchTemplate
594-
expect func(m *mocks.MockEC2APIMockRecorder)
595-
want bool
596-
wantErr bool
591+
name string
592+
incoming *expinfrav1.AWSLaunchTemplate
593+
existing *expinfrav1.AWSLaunchTemplate
594+
expect func(m *mocks.MockEC2APIMockRecorder)
595+
want bool
596+
wantNeedsUpdateReason string
597+
wantErr bool
597598
}{
598599
{
599600
name: "only core security groups, order shouldn't matter",
@@ -639,8 +640,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
639640
{ID: aws.String("sg-999")},
640641
},
641642
},
642-
want: true,
643-
wantErr: false,
643+
want: true,
644+
wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs",
645+
wantErr: false,
644646
},
645647
{
646648
name: "new additional security group",
@@ -657,8 +659,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
657659
{ID: aws.String("sg-999")},
658660
},
659661
},
660-
want: true,
661-
wantErr: false,
662+
want: true,
663+
wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs",
664+
wantErr: false,
662665
},
663666
{
664667
name: "Should return true if incoming IamInstanceProfile is not same as existing IamInstanceProfile",
@@ -672,7 +675,8 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
672675
},
673676
IamInstanceProfile: "some-other-profile",
674677
},
675-
want: true,
678+
want: true,
679+
wantNeedsUpdateReason: "IamInstanceProfile",
676680
},
677681
{
678682
name: "Should return true if incoming InstanceType is not same as existing InstanceType",
@@ -686,7 +690,8 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
686690
},
687691
InstanceType: "t3.large",
688692
},
689-
want: true,
693+
want: true,
694+
wantNeedsUpdateReason: "InstanceType",
690695
},
691696
{
692697
name: "new additional security group with filters",
@@ -706,8 +711,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
706711
m.DescribeSecurityGroups(context.TODO(), gomock.Eq(&ec2.DescribeSecurityGroupsInput{Filters: []ec2types.Filter{{Name: aws.String("sg-2"), Values: []string{"test-2"}}}})).
707712
Return(&ec2.DescribeSecurityGroupsOutput{SecurityGroups: []ec2types.SecurityGroup{{GroupId: aws.String("sg-2")}}}, nil)
708713
},
709-
want: true,
710-
wantErr: false,
714+
want: true,
715+
wantNeedsUpdateReason: "AdditionalSecurityGroupsIDs",
716+
wantErr: false,
711717
},
712718
{
713719
name: "new launch template instance metadata options, requiring IMDSv2",
@@ -723,8 +729,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
723729
{ID: aws.String("sg-222")},
724730
},
725731
},
726-
want: true,
727-
wantErr: false,
732+
want: true,
733+
wantNeedsUpdateReason: "InstanceMetadataOptions",
734+
wantErr: false,
728735
},
729736
{
730737
name: "new launch template instance metadata options, removing IMDSv2 requirement",
@@ -735,8 +742,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
735742
HTTPTokens: infrav1.HTTPTokensStateRequired,
736743
},
737744
},
738-
want: true,
739-
wantErr: false,
745+
want: true,
746+
wantNeedsUpdateReason: "InstanceMetadataOptions",
747+
wantErr: false,
740748
},
741749
{
742750
name: "Should return true if incoming SpotMarketOptions is different from existing SpotMarketOptions",
@@ -754,8 +762,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
754762
MaxPrice: aws.String("0.05"),
755763
},
756764
},
757-
want: true,
758-
wantErr: false,
765+
want: true,
766+
wantNeedsUpdateReason: "SpotMarketOptions",
767+
wantErr: false,
759768
},
760769
{
761770
name: "Should return true if incoming adds SpotMarketOptions and existing has none",
@@ -771,8 +780,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
771780
},
772781
SpotMarketOptions: nil,
773782
},
774-
want: true,
775-
wantErr: false,
783+
want: true,
784+
wantNeedsUpdateReason: "SpotMarketOptions",
785+
wantErr: false,
776786
},
777787
{
778788
name: "Should return true if incoming removes SpotMarketOptions and existing has some",
@@ -788,8 +798,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
788798
MaxPrice: aws.String("0.05"),
789799
},
790800
},
791-
want: true,
792-
wantErr: false,
801+
want: true,
802+
wantNeedsUpdateReason: "SpotMarketOptions",
803+
wantErr: false,
793804
},
794805
{
795806
name: "Should return true if SSH key names are different",
@@ -803,8 +814,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
803814
},
804815
SSHKeyName: aws.String("old-key"),
805816
},
806-
want: true,
807-
wantErr: false,
817+
want: true,
818+
wantNeedsUpdateReason: "SSHKeyName",
819+
wantErr: false,
808820
},
809821
{
810822
name: "Should return true if one has SSH key name and other doesn't",
@@ -818,8 +830,25 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
818830
},
819831
SSHKeyName: nil,
820832
},
821-
want: true,
822-
wantErr: false,
833+
want: true,
834+
wantNeedsUpdateReason: "SSHKeyName",
835+
wantErr: false,
836+
},
837+
{
838+
name: "Should return false if no SSH key is set in the spec and AWS returns no key pair as well",
839+
incoming: &expinfrav1.AWSLaunchTemplate{
840+
SSHKeyName: aws.String(""), // explicit empty string
841+
},
842+
existing: &expinfrav1.AWSLaunchTemplate{
843+
AdditionalSecurityGroups: []infrav1.AWSResourceReference{
844+
{ID: aws.String("sg-111")},
845+
{ID: aws.String("sg-222")},
846+
},
847+
SSHKeyName: nil,
848+
},
849+
want: false,
850+
wantNeedsUpdateReason: "",
851+
wantErr: false,
823852
},
824853
{
825854
name: "Should return true if incoming PrivateDNSName is different from existing PrivateDNSName",
@@ -841,8 +870,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
841870
HostnameType: aws.String("ip-name"),
842871
},
843872
},
844-
want: true,
845-
wantErr: false,
873+
want: true,
874+
wantNeedsUpdateReason: "PrivateDNSName",
875+
wantErr: false,
846876
},
847877
{
848878
name: "Should return true if incoming adds PrivateDNSName and existing has none",
@@ -860,8 +890,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
860890
},
861891
PrivateDNSName: nil,
862892
},
863-
want: true,
864-
wantErr: false,
893+
want: true,
894+
wantNeedsUpdateReason: "PrivateDNSName",
895+
wantErr: false,
865896
},
866897
{
867898
name: "Should return true if incoming removes PrivateDNSName and existing has some",
@@ -879,8 +910,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
879910
HostnameType: aws.String("resource-name"),
880911
},
881912
},
882-
want: true,
883-
wantErr: false,
913+
want: true,
914+
wantNeedsUpdateReason: "PrivateDNSName",
915+
wantErr: false,
884916
},
885917
{
886918
name: "Should return true if capacity reservation IDs are different",
@@ -894,8 +926,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
894926
},
895927
CapacityReservationID: aws.String("old-reservation"),
896928
},
897-
want: true,
898-
wantErr: false,
929+
want: true,
930+
wantNeedsUpdateReason: "CapacityReservationID",
931+
wantErr: false,
899932
},
900933
{
901934
name: "Should return true if one has capacity reservation ID and other doesn't",
@@ -909,8 +942,9 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
909942
},
910943
CapacityReservationID: nil,
911944
},
912-
want: true,
913-
wantErr: false,
945+
want: true,
946+
wantNeedsUpdateReason: "CapacityReservationID",
947+
wantErr: false,
914948
},
915949
}
916950
for _, tt := range tests {
@@ -948,12 +982,13 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
948982
tt.expect(mockEC2Client.EXPECT())
949983
}
950984

951-
got, err := s.LaunchTemplateNeedsUpdate(machinePoolScope, tt.incoming, tt.existing)
985+
got, gotNeedsUpdateReason, err := s.LaunchTemplateNeedsUpdate(machinePoolScope, tt.incoming, tt.existing)
952986
if tt.wantErr {
953987
g.Expect(err).To(HaveOccurred())
954988
return
955989
}
956990
g.Expect(err).NotTo(HaveOccurred())
991+
g.Expect(gotNeedsUpdateReason).Should(Equal(tt.wantNeedsUpdateReason))
957992
g.Expect(got).Should(Equal(tt.want))
958993
})
959994
}

0 commit comments

Comments
 (0)