feat: new resource - LaunchTemplate #255

michaelhtm · 2025-03-28T18:25:42Z

Issue #1841

Description of changes:
Adding support for LaunchTemplate.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

michaelhtm · 2025-03-28T23:01:25Z

/retest

a-hilaly

Thanks @michaelhtm ! some comments inline

apis/v1alpha1/types.go

a-hilaly · 2025-03-29T01:08:01Z

pkg/resource/launch_template/hooks.go

+		return err
+	}
+
+	for _, elem := range resp.LaunchTemplateVersions {


i think function can be generated using custom templates and sdk_file_end

putting setResourceForLaunchTemplateData there since that's the setter with the most LOC

pkg/resource/launch_template/hooks.go

a-hilaly · 2025-03-29T01:14:50Z

pkg/resource/launch_template/sdk.go

+	}
+	var resp *svcsdk.DescribeLaunchTemplatesOutput
+	resp, err = rm.sdkapi.DescribeLaunchTemplates(ctx, input)
+	rm.metrics.RecordAPICall("READ_MANY", "DescribeLaunchTemplates", err)


Do we want make the DescribeLaunchVersion here instead?

i'm not opposed to it..

pkg/resource/launch_template/hooks.go

a-hilaly · 2025-03-29T01:19:01Z

apis/v1alpha1/generator.yaml

+      DefaultVersion:
+        from:
+          operation: ModifyLaunchTemplate
+          path: LaunchTemplate.DefaultVersionNumber


Can we make this field a string instead? i like integer, but looks like the api takes a string, maybe cause they support non integer versions?

There's only one use case where it's used as a string...the rest of the time it's a number..
The Create API returns a string, while the ModifyLaunchTemplate accepts a string

a-hilaly · 2025-03-29T01:19:47Z

pkg/resource/launch_template/hooks.go

+	// TODO (michaelhtm) Not sure if we should check if
+	// the defaultVersion is greater than latestVersion
+	//
+	// My proposal would be to return a terminal error
+	// since the launchTemplate's latestVersion will never
+	// increase intil we make updates...˘\\/(ヅ)\/˘˘
+	// if *desired.ko.Spec.DefaultVersion > *latest.ko.Status.LatestVersion {
+	// 	return ackerr.NewTerminalError(fmt.Errorf("desired version number is ahead of the latest version"))
+	// }


pkg/resource/launch_template/hooks.go

michaelhtm

adding thoughts..

apis/v1alpha1/generator.yaml

michaelhtm · 2025-03-31T08:01:55Z

apis/v1alpha1/launch_template.go

+	LatestVersion *int64 `json:"latestVersion,omitempty"`
+	// The entity that manages the launch template.
+	// +kubebuilder:validation:Optional
+	Operator *OperatorResponse `json:"operator,omitempty"`


I believe this field does not make sense for kubernetes..if another entity is managing this launchTemplate, we might encounter diffs whenever the entity makes changes

shall we remove it?

it's not affecting anything significant since it's in the status..so idk

michaelhtm · 2025-03-31T08:09:35Z

apis/v1alpha1/launch_template.go

+	// +kubebuilder:validation:Required
+	Data *RequestLaunchTemplateData `json:"data"`
+	// The version number of the default version of the launch template.
+	DefaultVersion *int64 `json:"defaultVersion,omitempty"`


Having DefaultVersion in the spec is risky, and a one way decision. We need to discuss this before merging..

Currently, when a launchTemplate is created, and later updated, the DefaultVersion will no longer be matching the data in the spec. This may not be ideal IMO.

If we put the defaultVersion in the status, we can start ignoring the latestVersion there and combine them both into one field called Version.
This Version will reflect the data in the spec, and with every update, we can call the APIs necessary to Create a new version with the latest data, ensure that version is the defaultVersion, delete the older version.

Thoughts?

michaelhtm · 2025-03-31T08:13:43Z

pkg/resource/launch_template/hooks.go

+		return err
+	}
+
+	for _, elem := range resp.LaunchTemplateVersions {


putting setResourceForLaunchTemplateData there since that's the setter with the most LOC

michaelhtm · 2025-03-31T18:16:28Z

/retest

a-hilaly

Thanks @michaelhtm ! i left a couple questions below

templates/hooks/launch_template/sdk_update_pre_build_request.go.tpl

a-hilaly · 2025-03-31T18:34:09Z

test/e2e/tests/test_launch_template.py

+        assert default_version == cr['spec']['defaultVersion']
+        assert latest_version == cr['status']['latestVersion']


So even if the user doesn't specific a default version, we patch one back to the CR? do we want that behaviour?

yes, at least after the create Operation...or would it make sense to put it in status?

test/e2e/tests/test_launch_template.py

a-hilaly · 2025-03-31T21:32:20Z

apis/v1alpha1/launch_template.go

+	LatestVersion *int64 `json:"latestVersion,omitempty"`
+	// The entity that manages the launch template.
+	// +kubebuilder:validation:Optional
+	Operator *OperatorResponse `json:"operator,omitempty"`


shall we remove it?

templates/hooks/launch_template/sdk_update_pre_build_request.go.tpl

a-hilaly

👍

michaelhtm · 2025-04-01T00:26:22Z

/retest

Adding support for LaunchTemplate. Co-Authored-By: Rohit Kurdukar - Amazon <[email protected]>

michaelhtm · 2025-04-01T00:47:12Z

/test ec2-kind-e2e

michaelhtm · 2025-04-01T01:28:22Z

/retest

a-hilaly · 2025-04-01T02:11:51Z

Mmmm ... the test failures seem unrelated to the changes

michaelhtm · 2025-04-01T02:22:08Z

/retest

michaelhtm · 2025-04-01T02:57:02Z

/retest

ack-prow · 2025-04-01T02:57:42Z

@michaelhtm: 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
ec2-verify-attribution	`d71330d`	link	false	`/test ec2-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

a-hilaly · 2025-04-01T17:10:41Z

/lgtm

a-hilaly · 2025-04-01T17:10:58Z

/lgtm cancel

a-hilaly · 2025-04-01T22:23:52Z

/lgtm

ack-prow · 2025-04-01T22:23:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly,michaelhtm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-prow bot requested review from a-hilaly and jlbutler March 28, 2025 18:26

ack-prow bot added the approved label Mar 28, 2025

michaelhtm force-pushed the newResource/LaunchTemplate branch 5 times, most recently from c42abc5 to c739883 Compare March 28, 2025 22:48

michaelhtm force-pushed the newResource/LaunchTemplate branch 3 times, most recently from f7ac8eb to ac413db Compare March 29, 2025 01:07

a-hilaly reviewed Mar 29, 2025

View reviewed changes

michaelhtm force-pushed the newResource/LaunchTemplate branch 5 times, most recently from 27c2335 to 3c8e066 Compare March 31, 2025 08:15

michaelhtm commented Mar 31, 2025

View reviewed changes

michaelhtm force-pushed the newResource/LaunchTemplate branch 3 times, most recently from 5229cca to 5c88415 Compare March 31, 2025 17:41

a-hilaly reviewed Mar 31, 2025

View reviewed changes

michaelhtm force-pushed the newResource/LaunchTemplate branch from 5c88415 to 77bc52a Compare March 31, 2025 21:25

a-hilaly reviewed Mar 31, 2025

View reviewed changes

a-hilaly approved these changes Mar 31, 2025

View reviewed changes

michaelhtm force-pushed the newResource/LaunchTemplate branch 2 times, most recently from 39b8a4c to f1b371f Compare March 31, 2025 23:43

feat: new resource - LaunchTemplate

d71330d

Adding support for LaunchTemplate. Co-Authored-By: Rohit Kurdukar - Amazon <[email protected]>

michaelhtm force-pushed the newResource/LaunchTemplate branch from f1b371f to d71330d Compare April 1, 2025 00:27

ack-prow bot assigned a-hilaly Apr 1, 2025

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2025

ack-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2025

a-hilaly approved these changes Apr 1, 2025

View reviewed changes

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2025

ack-prow bot merged commit 298fc7d into aws-controllers-k8s:main Apr 1, 2025
6 of 7 checks passed

		assert default_version == cr['spec']['defaultVersion']
		assert latest_version == cr['status']['latestVersion']

feat: new resource - LaunchTemplate #255

feat: new resource - LaunchTemplate #255

Uh oh!

Conversation

michaelhtm commented Mar 28, 2025

Uh oh!

michaelhtm commented Mar 28, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelhtm commented Mar 31, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

michaelhtm commented Apr 1, 2025

Uh oh!

michaelhtm commented Apr 1, 2025

Uh oh!

michaelhtm commented Apr 1, 2025

Uh oh!

a-hilaly commented Apr 1, 2025

Uh oh!

michaelhtm commented Apr 1, 2025

Uh oh!

michaelhtm commented Apr 1, 2025

Uh oh!

ack-prow bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelhtm Mar 31, 2025 •

edited

Loading

michaelhtm Mar 31, 2025 •

edited

Loading

ack-prow bot commented Apr 1, 2025 •

edited

Loading