Skip to content

🐛 Print reason for launch template needing update, fix constant instance refreshes for AWSMachinePool by considering SSHKeyName nil and empty string the same #5788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AndiDog wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −65
Open

🐛 Print reason for launch template needing update, fix constant instance refreshes for AWSMachinePool by considering SSHKeyName nil and empty string the same #5788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions exp/controllers/awsmachinepool_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")))))
Expand Down Expand Up @@ -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")))))
Expand Down
33 changes: 17 additions & 16 deletions pkg/cloud/services/ec2/launchtemplate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
113 changes: 74 additions & 39 deletions pkg/cloud/services/ec2/launchtemplate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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 {
Expand Down Expand Up @@ -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))
})
}
Expand Down
Loading
Loading