Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #5496 (2025-05-15 13:24:25)

:bug: Launchtemplate needs to be updated if spot options are changed
**What type of PR is this?**

/kind bug

**What this PR does / why we need it**:

When the spot instances options are changed, we want to update the launch template so that the changes take effect.

**Special notes for your reviewer**:

I've changed the unit tests for the `LaunchTemplateNeedsUpdate` function, because some cases were passing by pure chance. Basically these security groups are always added to the incoming `AWSLaunchTemplate`
```
{ID: aws.String("sg-111")},
{ID: aws.String("sg-222")},
```

So if the existing `AWSLaunchTemplate` that we use in the tests don't have these security groups, the function will always return true. Tests like the one testing changes to the `IamInstanceProfile` field were passing because of the different security groups, rather than the change on the `IamInstanceProfile` field.

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted 

 Please add an icon to the title of this PR, the icon will be either ⚠️ (:warning:, major or breaking changes), ✨ (:sparkles:, feature additions), 🐛 (:bug:, patch and bugfixes), 📖 (:book:, documentation or proposals), or 🌱 (:seedling:, minor or other) 
-->

- [ ] squashed commits
- [ ] includes documentation
- [ ] includes emoji in title 
- [ ] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
LaunchTemplate needs to be updated if spot options are changed
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [dlipovetsky](https://github.com/dlipovetsky) for approval. For more information see [the Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dlipovetsky"]} -->
fiunchinho: /test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks
k8s-ci-robot: LGTM label has been added.  <details>Git tree hash: d778b6d36ce012772b2bb23779e7a386fca21354</details>
k8s-ci-robot: New changes are detected. LGTM label has been removed.
k8s-ci-robot: @fiunchinho: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | 4a57ee5a10c308900a285481f15fd82c4988510d | [link](https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/5496/pull-cluster-api-provider-aws-test/1924145770181693440) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=5496). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Afiunchinho). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go
index 148f8d486..000abdfde 100644
--- a/pkg/cloud/services/ec2/launchtemplate.go
+++ b/pkg/cloud/services/ec2/launchtemplate.go
@@ -680,6 +680,24 @@ func (s *Service) deleteLaunchTemplateVersion(id string, version *int64) error {
 	return nil
 }
 
+// SDKToSpotMarketOptions converts EC2 instance market options to SpotMarketOptions.
+func SDKToSpotMarketOptions(instanceMarketOptions *ec2.LaunchTemplateInstanceMarketOptions) *infrav1.SpotMarketOptions {
+	if instanceMarketOptions == nil || aws.StringValue(instanceMarketOptions.MarketType) != ec2.MarketTypeSpot {
+		return nil
+	}
+
+	if instanceMarketOptions.SpotOptions == nil {
+		return &infrav1.SpotMarketOptions{}
+	}
+
+	result := &infrav1.SpotMarketOptions{}
+	if instanceMarketOptions.SpotOptions.MaxPrice != nil {
+		result.MaxPrice = instanceMarketOptions.SpotOptions.MaxPrice
+	}
+
+	return result
+}
+
 // SDKToLaunchTemplate converts an AWS EC2 SDK instance to the CAPA instance type.
 func (s *Service) SDKToLaunchTemplate(d *ec2.LaunchTemplateVersion) (*expinfrav1.AWSLaunchTemplate, string, *apimachinerytypes.NamespacedName, error) {
 	v := d.LaunchTemplateData
@@ -688,9 +706,10 @@ func (s *Service) SDKToLaunchTemplate(d *ec2.LaunchTemplateVersion) (*expinfrav1
 		AMI: infrav1.AMIReference{
 			ID: v.ImageId,
 		},
-		InstanceType:  aws.StringValue(v.InstanceType),
-		SSHKeyName:    v.KeyName,
-		VersionNumber: d.VersionNumber,
+		InstanceType:      aws.StringValue(v.InstanceType),
+		SSHKeyName:        v.KeyName,
+		SpotMarketOptions: SDKToSpotMarketOptions(v.InstanceMarketOptions),
+		VersionNumber:     d.VersionNumber,
 	}
 
 	if v.MetadataOptions != nil {
@@ -777,6 +796,9 @@ func (s *Service) LaunchTemplateNeedsUpdate(scope scope.LaunchTemplateScope, inc
 	if !cmp.Equal(incoming.InstanceMetadataOptions, existing.InstanceMetadataOptions) {
 		return true, nil
 	}
+	if !cmp.Equal(incoming.SpotMarketOptions, existing.SpotMarketOptions) {
+		return true, nil
+	}
 
 	incomingIDs, err := s.GetAdditionalSecurityGroupsIDs(incoming.AdditionalSecurityGroups)
 	if err != nil {


Test patch:
diff --git a/pkg/cloud/services/ec2/launchtemplate_test.go b/pkg/cloud/services/ec2/launchtemplate_test.go
index a15243fe1..b04907b7c 100644
--- a/pkg/cloud/services/ec2/launchtemplate_test.go
+++ b/pkg/cloud/services/ec2/launchtemplate_test.go
@@ -353,6 +353,139 @@ func TestServiceSDKToLaunchTemplate(t *testing.T) {
 			wantHash:          testUserDataHash,
 			wantDataSecretKey: nil, // respective tag is not given
 		},
+		{
+			name: "spot market options",
+			input: &ec2.LaunchTemplateVersion{
+				LaunchTemplateId:   aws.String("lt-12345"),
+				LaunchTemplateName: aws.String("foo"),
+				LaunchTemplateData: &ec2.ResponseLaunchTemplateData{
+					ImageId: aws.String("foo-image"),
+					IamInstanceProfile: &ec2.LaunchTemplateIamInstanceProfileSpecification{
+						Arn: aws.String("instance-profile/foo-profile"),
+					},
+					KeyName: aws.String("foo-keyname"),
+					InstanceMarketOptions: &ec2.LaunchTemplateInstanceMarketOptions{
+						MarketType: aws.String(ec2.MarketTypeSpot),
+						SpotOptions: &ec2.LaunchTemplateSpotMarketOptions{
+							MaxPrice: aws.String("0.05"),
+						},
+					},
+					UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(testUserData))),
+				},
+				VersionNumber: aws.Int64(1),
+			},
+			wantLT: &expinfrav1.AWSLaunchTemplate{
+				Name: "foo",
+				AMI: infrav1.AMIReference{
+					ID: aws.String("foo-image"),
+				},
+				IamInstanceProfile: "foo-profile",
+				SSHKeyName:         aws.String("foo-keyname"),
+				VersionNumber:      aws.Int64(1),
+				SpotMarketOptions: &infrav1.SpotMarketOptions{
+					MaxPrice: aws.String("0.05"),
+				},
+			},
+			wantHash:          testUserDataHash,
+			wantDataSecretKey: nil,
+		},
+		{
+			name: "spot market options with no max price",
+			input: &ec2.LaunchTemplateVersion{
+				LaunchTemplateId:   aws.String("lt-12345"),
+				LaunchTemplateName: aws.String("foo"),
+				LaunchTemplateData: &ec2.ResponseLaunchTemplateData{
+					ImageId: aws.String("foo-image"),
+					IamInstanceProfile: &ec2.LaunchTemplateIamInstanceProfileSpecification{
+						Arn: aws.String("instance-profile/foo-profile"),
+					},
+					KeyName: aws.String("foo-keyname"),
+					InstanceMarketOptions: &ec2.LaunchTemplateInstanceMarketOptions{
+						MarketType:  aws.String(ec2.MarketTypeSpot),
+						SpotOptions: &ec2.LaunchTemplateSpotMarketOptions{},
+					},
+					UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(testUserData))),
+				},
+				VersionNumber: aws.Int64(1),
+			},
+			wantLT: &expinfrav1.AWSLaunchTemplate{
+				Name: "foo",
+				AMI: infrav1.AMIReference{
+					ID: aws.String("foo-image"),
+				},
+				IamInstanceProfile: "foo-profile",
+				SSHKeyName:         aws.String("foo-keyname"),
+				VersionNumber:      aws.Int64(1),
+				SpotMarketOptions:  &infrav1.SpotMarketOptions{},
+			},
+			wantHash:          testUserDataHash,
+			wantDataSecretKey: nil,
+		},
+		{
+			name: "spot market options without SpotOptions",
+			input: &ec2.LaunchTemplateVersion{
+				LaunchTemplateId:   aws.String("lt-12345"),
+				LaunchTemplateName: aws.String("foo"),
+				LaunchTemplateData: &ec2.ResponseLaunchTemplateData{
+					ImageId: aws.String("foo-image"),
+					IamInstanceProfile: &ec2.LaunchTemplateIamInstanceProfileSpecification{
+						Arn: aws.String("instance-profile/foo-profile"),
+					},
+					KeyName: aws.String("foo-keyname"),
+					InstanceMarketOptions: &ec2.LaunchTemplateInstanceMarketOptions{
+						MarketType: aws.String(ec2.MarketTypeSpot),
+					},
+					UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(testUserData))),
+				},
+				VersionNumber: aws.Int64(1),
+			},
+			wantLT: &expinfrav1.AWSLaunchTemplate{
+				Name: "foo",
+				AMI: infrav1.AMIReference{
+					ID: aws.String("foo-image"),
+				},
+				IamInstanceProfile: "foo-profile",
+				SSHKeyName:         aws.String("foo-keyname"),
+				VersionNumber:      aws.Int64(1),
+				SpotMarketOptions:  &infrav1.SpotMarketOptions{},
+			},
+			wantHash:          testUserDataHash,
+			wantDataSecretKey: nil,
+		},
+		{
+			name: "non-spot market type",
+			input: &ec2.LaunchTemplateVersion{
+				LaunchTemplateId:   aws.String("lt-12345"),
+				LaunchTemplateName: aws.String("foo"),
+				LaunchTemplateData: &ec2.ResponseLaunchTemplateData{
+					ImageId: aws.String("foo-image"),
+					IamInstanceProfile: &ec2.LaunchTemplateIamInstanceProfileSpecification{
+						Arn: aws.String("instance-profile/foo-profile"),
+					},
+					KeyName: aws.String("foo-keyname"),
+					InstanceMarketOptions: &ec2.LaunchTemplateInstanceMarketOptions{
+						MarketType: aws.String("different-market-type"),
+						SpotOptions: &ec2.LaunchTemplateSpotMarketOptions{
+							MaxPrice: aws.String("0.05"),
+						},
+					},
+					UserData: aws.String(base64.StdEncoding.EncodeToString([]byte(testUserData))),
+				},
+				VersionNumber: aws.Int64(1),
+			},
+			wantLT: &expinfrav1.AWSLaunchTemplate{
+				Name: "foo",
+				AMI: infrav1.AMIReference{
+					ID: aws.String("foo-image"),
+				},
+				IamInstanceProfile: "foo-profile",
+				SSHKeyName:         aws.String("foo-keyname"),
+				VersionNumber:      aws.Int64(1),
+				SpotMarketOptions:  nil, // Should be nil since market type is not "spot"
+			},
+			wantHash:          testUserDataHash,
+			wantDataSecretKey: nil,
+		},
 		{
 			name: "tag of bootstrap secret",
 			input: &ec2.LaunchTemplateVersion{
@@ -440,6 +573,20 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
 		want     bool
 		wantErr  bool
 	}{
+		{
+			name: "only core security groups, order shouldn't matter",
+			incoming: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{},
+			},
+			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-222")},
+					{ID: aws.String("sg-111")},
+				},
+			},
+			want:    false,
+			wantErr: false,
+		},
 		{
 			name: "the same security groups",
 			incoming: &expinfrav1.AWSLaunchTemplate{
@@ -497,6 +644,10 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
 				IamInstanceProfile: DefaultAmiNameFormat,
 			},
 			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
 				IamInstanceProfile: "some-other-profile",
 			},
 			want: true,
@@ -507,6 +658,10 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
 				InstanceType: "t3.micro",
 			},
 			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
 				InstanceType: "t3.large",
 			},
 			want: true,
@@ -540,9 +695,14 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
 					HTTPTokens:              infrav1.HTTPTokensStateRequired,
 				},
 			},
-			existing: &expinfrav1.AWSLaunchTemplate{},
-			want:     true,
-			wantErr:  false,
+			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
+			},
+			want:    true,
+			wantErr: false,
 		},
 		{
 			name:     "new launch template instance metadata options, removing IMDSv2 requirement",
@@ -556,6 +716,59 @@ func TestServiceLaunchTemplateNeedsUpdate(t *testing.T) {
 			want:    true,
 			wantErr: false,
 		},
+		{
+			name: "Should return true if incoming SpotMarketOptions is different from existing SpotMarketOptions",
+			incoming: &expinfrav1.AWSLaunchTemplate{
+				SpotMarketOptions: &infrav1.SpotMarketOptions{
+					MaxPrice: aws.String("0.10"),
+				},
+			},
+			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
+				SpotMarketOptions: &infrav1.SpotMarketOptions{
+					MaxPrice: aws.String("0.05"),
+				},
+			},
+			want:    true,
+			wantErr: false,
+		},
+		{
+			name: "Should return true if incoming adds SpotMarketOptions and existing has none",
+			incoming: &expinfrav1.AWSLaunchTemplate{
+				SpotMarketOptions: &infrav1.SpotMarketOptions{
+					MaxPrice: aws.String("0.10"),
+				},
+			},
+			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
+				SpotMarketOptions: nil,
+			},
+			want:    true,
+			wantErr: false,
+		},
+		{
+			name: "Should return true if incoming removes SpotMarketOptions and existing has some",
+			incoming: &expinfrav1.AWSLaunchTemplate{
+				SpotMarketOptions: nil,
+			},
+			existing: &expinfrav1.AWSLaunchTemplate{
+				AdditionalSecurityGroups: []infrav1.AWSResourceReference{
+					{ID: aws.String("sg-111")},
+					{ID: aws.String("sg-222")},
+				},
+				SpotMarketOptions: &infrav1.SpotMarketOptions{
+					MaxPrice: aws.String("0.05"),
+				},
+			},
+			want:    true,
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {