new resource ec2launch template and test caes #151

rkurduka · 2023-09-20T14:48:23Z

Issue #, if available:
1841

Description of changes:

New resource added - LaunchTemplate
Test cases added.

Features:

Create new EC2 launch template
Delete existing EC2 launch template

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

ack-prow · 2023-09-20T14:48:35Z

Hi @rkurduka. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

a-hilaly · 2023-09-20T14:58:26Z

/ok-to-test

a-hilaly

Good stuff, thank you @rkurduka ! I left few comments in-line

apis/v1alpha1/ack-generate-metadata.yaml

cmd/controller/controller

config/controller/deployment.yaml

apis/v1alpha1/launch_template.go

test/e2e/resources/launchtemplate_invalid.yaml

test/e2e/tests/test_launch_template.py

a-hilaly · 2023-09-20T15:38:20Z

test/e2e/tests/test_launch_template.py

+        # Validate LaunchTemplate
+        launch_template = ec2_validator.get_launch_template(resource_id)
+        assert launch_template["LaunchTemplateId"] == resource_id
+


can we also add an update test?

To update launch template only one parameter is supported in "ModifyLaunchTemplate" SDK API call.

DefaultVersionNumber -- for this to test , there has to be multiple "launchtemplateversions" must be available and "launchtemplateversion" is separate resource in EC2. So once we have that , so we can add update test case here for it.

Note:
update tags is not supported "ModifyLaunchTemplate" SDK API call , which is used in SDKUPDATE function, hence not added update test case yet .

Great, thanks for the clearance! We could support add some hooks to call CreateTags/UpdateTags for the Spec.Tags field?
If i understand well, this funciton can be used with any ec2 resource? https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CreateTags.html

https://github.com/aws-controllers-k8s/ec2-controller/blob/main/pkg/resource/vpc/hooks.go#L306-L310

Yes , looking into this

added tags -creation/update logic , including test cases for it as well .

test/e2e/tests/test_launch_template.py

rkurduka · 2023-09-25T14:42:31Z

fixed - conflicts

a-hilaly

Very nice!! Thank you @rkurduka !

I have few comments/questions bellow.

helm/Chart.yaml

a-hilaly · 2023-09-26T00:31:27Z

pkg/resource/launch_template/hooks.go

+func (rm *resourceManager) setDefaultTemplateVersion(r *resource, input *svcsdk.ModifyLaunchTemplateInput) error {
+
+	if r.ko.Spec.DefaultVersionNumber != nil {
+		input.SetDefaultVersion(strconv.Itoa(int(*r.ko.Spec.DefaultVersionNumber)))


So the API accept an int the create but a string in the update... wow

In "CreateLaunchTemplate" we dont need to provide "DefaultTemplateVersion" - but once it is created. In "DescribeLaunchTemplateOutput" Its *int64 , So while modifying same attribute in ModifyLaunchTemplate its *string , hence conversion is require

a-hilaly · 2023-09-26T00:34:06Z

test/e2e/tests/test_launch_template.py

+        # Validate LaunchTemplate
+        launch_template = ec2_validator.get_launch_template(resource_id)
+        assert launch_template["LaunchTemplateId"] == resource_id
+


Great, thanks for the clearance! We could support add some hooks to call CreateTags/UpdateTags for the Spec.Tags field?
If i understand well, this funciton can be used with any ec2 resource? https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CreateTags.html

a-hilaly · 2023-09-26T00:35:04Z

test/e2e/tests/test_launch_template.py

+        # Check LaunchTemplate no longer exists in AWS
+        ec2_validator.assert_launch_template(resource_id, exists=False)
+
+    def test_terminal_condition_invalid_parameter_value(self):


a-hilaly · 2023-09-26T00:35:06Z

test/e2e/tests/test_launch_template.py

+        # Validate LaunchTemplate
+        launch_template = ec2_validator.get_launch_template(resource_id)
+        assert launch_template["LaunchTemplateId"] == resource_id
+


https://github.com/aws-controllers-k8s/ec2-controller/blob/main/pkg/resource/vpc/hooks.go#L306-L310

ack-bot · 2024-03-29T12:48:33Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

kejne · 2024-06-27T13:53:39Z

Is there anything particular blocking this PR from being merged, other than having a merge conflict after having been stale for 6m?

kejne · 2024-06-28T06:22:46Z

@rkurduka ok if i cherry pick your changes to keep working on the changes you started so we can get this functionality in?

a-hilaly · 2024-07-02T21:56:06Z

@kejne please go for it :)

a-hilaly · 2024-09-05T20:10:21Z

@kejne are you still thinking of picking up this PR?

kejne · 2024-09-05T20:41:51Z

I'm afraid other projects fit our use case better, so haven't looked further into it.

a-hilaly · 2024-09-05T20:45:13Z

@kejne no worries :) may I ask what projects are you using to workaround this and manage launch templates?

kejne · 2024-09-05T20:56:01Z

It wasn't really just about Launch Templates, but more of the whole. For example, since we will be leveraging multiple cloud providers we are leaning more towards Crossplane atm.

cedricmckinnie · 2024-12-23T06:51:25Z

Hi everyone. This is a rather critical feature of ACK imo as it's a showstopper for customizing managed EKS nodes. Any chance we could revisit this PR?

cedricmckinnie · 2024-12-23T20:12:16Z

I will also add that the Services Page in the ACK docs indicates that the ec2-controller is GA but lacking the ability to create and maintain a fundamental feature like LaunchTemplates seems like a significant enough gap in it's functionality to consider it GA. This might be worth a little bit more attention.

My comment from related PR #155 (comment)

michaelhtm

Hey @rkurduka, thanks for this PR! I left a few inline comments

Also I was thinking, would it be best to call the CreateTemplateVersion api during an update? We do something similar to resources that have Versions..https://github.com/aws-controllers-k8s/iam-controller/blob/6828e1c2e8fbc0b8f8cc1886eee38ba43ad1df71/pkg/resource/policy/hooks.go#L233-L266

We also released sdk-go-v2, so the hooks need few changes

michaelhtm · 2025-02-24T22:15:44Z

generator.yaml

+          path: Status.LaunchTemplateID
+          name: LaunchTemplateID
+      DefaultVersionNumber:
+        type: int


do we need to make this an int? I saw the API accepts it as a string

@michaelhtm - yes Create and Modify APIs accept string , but describe API returns value as Int. And "DefaultVersionNumber" we will need in "Spec" as well for users to modify . So to make it consistent everywhere changed type as "int' . let me know your thoughts on this as well

I believe we should keep the type the same as the Create operation. But if this work too it should be okay with me!

michaelhtm · 2025-02-24T22:15:58Z

apis/v1alpha1/generator.yaml

is this removed by accident?

@michaelhtm - yes its added back now

michaelhtm · 2025-02-24T22:40:19Z

test/e2e/tests/test_launch_template.py

+        # Only user tags should be present in Spec
+        resource = k8s.get_resource(ref)
+        assert len(resource["spec"]["tags"]) == 1
+        assert resource["spec"]["tags"][0]["key"] == "newtagkey"
+        assert resource["spec"]["tags"][0]["value"] == "newtagvalue"
+
+        # Check user and ack tags on aws 
+        launch_template = ec2_validator.get_launch_template(resource_id)
+        assert len(launch_template['Tags']) == 3


can we use something like this to compare these two? https://github.com/aws-controllers-k8s/cloudfront-controller/blob/329497899bbb1bc3af575be075376e39abae9c56/test/e2e/tests/test_distribution.py#L169-L182

hi @michaelhtm - Made changes here as you suggested , let me know anything else is require

ack-prow · 2025-02-27T14:18:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rkurduka
Once this PR has been reviewed and has the lgtm label, please assign jlbutler for approval by writing /assign @jlbutler in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

rkurduka · 2025-02-27T14:34:59Z

Hey @rkurduka, thanks for this PR! I left a few inline comments

Also I was thinking, would it be best to call the CreateTemplateVersion api during an update? We do something similar to resources that have Versions..https://github.com/aws-controllers-k8s/iam-controller/blob/6828e1c2e8fbc0b8f8cc1886eee38ba43ad1df71/pkg/resource/policy/hooks.go#L233-L266

We also released sdk-go-v2, so the hooks need few changes

@michaelhtm - CreateTemplateVersion is separate resource in EC2 , there is PR opened for that as well . So only change in update is allow to change defaultVersion of template . And NewVersion can be created separately and users can provide sourcetemplate to get values from to create new version as well. Let me know your thoughts.

Also generated Ec2-Controller again with latest code generator and made all require changes in hooks. Let me know if anything else require

michaelhtm · 2025-02-27T22:58:15Z

@michaelhtm - CreateTemplateVersion is separate resource in EC2 , there is PR opened for that as well . So only change in update is allow to change defaultVersion of template . And NewVersion can be created separately and users can provide sourcetemplate to get values from to create new version as well. Let me know your thoughts.

Also generated Ec2-Controller again with latest code generator and made all require changes in hooks. Let me know if anything else require

I see that. In the example I sent and here we have been calling CreateVersion as part of the update, so that ACK handles only one version of a resource.

This IAM PR has some explanation of the decision making>
aws-controllers-k8s/iam-controller#21

ack-prow · 2025-03-25T17:14:22Z

@rkurduka: The following tests 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	`e7a3297`	link	false	`/test ec2-verify-attribution`
ec2-kind-e2e	`e7a3297`	link	true	`/test ec2-kind-e2e`

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.

ack-prow bot requested review from a-hilaly and jljaco September 20, 2023 14:48

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2023

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2023

a-hilaly reviewed Sep 20, 2023

View reviewed changes

rkurduka closed this Sep 25, 2023

rkurduka force-pushed the ec2-template_without_version branch from 29f48f7 to dc920e9 Compare September 25, 2023 13:03

rkurduka reopened this Sep 25, 2023

a-hilaly reviewed Sep 26, 2023

View reviewed changes

rkurduka mentioned this pull request Oct 19, 2023

Ec2 template with version #155

Closed

a-hilaly mentioned this pull request Dec 31, 2023

Launch template aws-controllers-k8s/community#1952

Closed

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 29, 2024

a-hilaly removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2024

a-hilaly mentioned this pull request Jan 2, 2025

added launch template and launch templateversion as resource #146

Closed

rkurduka force-pushed the ec2-template_without_version branch from 04807c5 to 6750fe5 Compare January 5, 2025 05:35

michaelhtm reviewed Feb 24, 2025

View reviewed changes

rkurduka force-pushed the ec2-template_without_version branch from 5b42f5f to cb1452a Compare February 27, 2025 14:17

rkurduka force-pushed the ec2-template_without_version branch from cb1452a to 2d26a0c Compare March 3, 2025 07:18

rkurduka added 9 commits March 14, 2025 11:32

regenerated controller again after removing all conflicts

c5ba4ca

added test cases for launch templates

87d05f2

add distibution in e2e

b253f67

add helper function get_launch_template

957607c

added helper function to get launch template tags

5a73438

add test case for launch template

d743787

corrected tag comparision in test case

191ff2f

changes for adding support for launchtemplateversion

5cc4382

add createlaunchtemplateversion as update

e7a3297

rkurduka force-pushed the ec2-template_without_version branch from 03a142a to e7a3297 Compare March 25, 2025 16:52

a-hilaly closed this Apr 4, 2025

new resource ec2launch template and test caes #151

new resource ec2launch template and test caes #151

Uh oh!

Conversation

rkurduka commented Sep 20, 2023

Uh oh!

ack-prow bot commented Sep 20, 2023

Uh oh!

a-hilaly commented Sep 20, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

rkurduka commented Sep 25, 2023

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!

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!

ack-bot commented Mar 29, 2024

Uh oh!

kejne commented Jun 27, 2024

Uh oh!

kejne commented Jun 28, 2024

Uh oh!

a-hilaly commented Jul 2, 2024

Uh oh!

a-hilaly commented Sep 5, 2024

Uh oh!

kejne commented Sep 5, 2024

Uh oh!

a-hilaly commented Sep 5, 2024

Uh oh!

kejne commented Sep 5, 2024

Uh oh!

cedricmckinnie commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cedricmckinnie commented Dec 23, 2024

Uh oh!

michaelhtm left a comment

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!

Choose a reason for hiding this comment

Uh oh!

cedricmckinnie commented Dec 23, 2024 •

edited

Loading

michaelhtm commented Feb 27, 2025 •

edited

Loading