✨ Support storing Ignition user data in S3 bucket for `AWSMachinePool` #5318

AndiDog · 2025-01-22T15:48:43Z

What this PR does / why we need it:

Reopened from #5172 because of branch issues.

EC2 user data is insecure and very limited in size (16 KB). Use the S3 bucket storage also for machine pools. The difference to AWSMachine is that the S3 objects must stay around because a new instance could be started by the ASG at any time, and user data with a valid token must exist. Both a lifecycle policy for regular deletion, and explicit S3 object deletion requests, are used for cleanup, so that normally, only 1-3 launch template versions' user data are stored in S3. Each launch template version relates to one S3 object.

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:

Didn't find an existing issue. My company ran into EC2 user data size limit problems.

Special notes for your reviewer:

This change has been successfully used in the Giant Swarm fork of CAPA since July 2024, on production clusters. I have adapted the change to the latest upstream changes, in particular the introduction of IgnitionStorageTypeOption (#4700) and made sure the previous default storage type is kept. Therefore, I retested this feature using Tilt on a plain CAPI@main + CAPA@main setup, with AWSCluster + Flatcar AMI + AWSMachinePool in my manifests.

Checklist:

Release note:

action required
Support storing Ignition user data in S3 bucket for `AWSMachinePool`. If you plan to use this feature then you will need to update the permissions for CAPA using clusterawsadm.

AndiDog · 2025-01-24T08:32:16Z

/test pull-cluster-api-provider-aws-test

AndiDog · 2025-01-24T09:11:33Z

/test pull-cluster-api-provider-aws-e2e

AndiDog · 2025-01-27T08:24:06Z

/test pull-cluster-api-provider-aws-e2e

fiunchinho · 2025-01-28T11:28:56Z

exp/controllers/awsmachinepool_controller.go

@@ -280,7 +296,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 		machinePoolScope.Info("starting instance refresh", "number of instances", machinePoolScope.MachinePool.Spec.Replicas)
 		return asgsvc.StartASGInstanceRefresh(machinePoolScope)
 	}
-	if err := reconSvc.ReconcileLaunchTemplate(machinePoolScope, ec2Svc, canUpdateLaunchTemplate, runPostLaunchTemplateUpdateOperation); err != nil {
+	if err := reconSvc.ReconcileLaunchTemplate(machinePoolScope, machinePoolScope, s3Scope, ec2Svc, objectStoreSvc, canUpdateLaunchTemplate, runPostLaunchTemplateUpdateOperation); err != nil {


why do we need to pass the same machinePoolScope twice?

It gets cast to two different interfaces (IgnitionScope and LaunchTemplateScope). The number of small interfaces is to blame here, but many of them are needed to easily mock functionality or share functionality between regular and EKS cluster code and types.

fiunchinho · 2025-01-28T11:44:31Z

exp/controllers/awsmachinepool_controller.go

@@ -607,7 +623,7 @@ func machinePoolToInfrastructureMapFunc(gvk schema.GroupVersionKind) handler.Map
 	}
 }

-func (r *AWSMachinePoolReconciler) getInfraCluster(ctx context.Context, log *logger.Logger, cluster *clusterv1.Cluster, awsMachinePool *expinfrav1.AWSMachinePool) (scope.EC2Scope, error) {
+func (r *AWSMachinePoolReconciler) getInfraCluster(ctx context.Context, log *logger.Logger, cluster *clusterv1.Cluster, awsMachinePool *expinfrav1.AWSMachinePool) (scope.EC2Scope, scope.S3Scope, error) {


It looks like there is no case where the first and second returned values are different. They are always both the same value, either nil or clusterScope or managedControlplaneScope. If both always have the same value, couldn't we just return one instead of two values?

See above comment regarding interfaces

fiunchinho

/lgtm

AndiDog · 2025-03-25T10:35:49Z

/test pull-cluster-api-provider-aws-e2e

fiunchinho · 2025-03-27T08:26:42Z

/lgtm

richardcase · 2025-04-09T16:21:33Z

/test pull-cluster-api-provider-aws-e2e-eks

AndiDog · 2025-05-15T05:41:59Z

/retest

AndiDog · 2025-05-15T06:01:31Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

fiunchinho · 2025-05-20T13:30:54Z

/lgtm

k8s-ci-robot · 2025-05-20T13:31:01Z

LGTM label has been added.

Git tree hash: 7e4549c5a34ebafdffd2931285f452593d5dd11d

richardcase · 2025-05-28T09:00:14Z

/approve

k8s-ci-robot · 2025-05-28T09:00:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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~~ [richardcase]

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

fiunchinho · 2025-05-28T09:41:20Z

/retest

AndiDog · 2025-06-04T12:49:12Z

I've manually re-tested this after the major migration to AWS SDK for Go v2.

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

damdo · 2025-06-04T14:28:54Z

@fiunchinho can you re-add your LGTM here?

damdo · 2025-06-04T14:29:12Z

/test pull-cluster-api-provider-aws-e2e pull-cluster-api-provider-aws-e2e-eks

AndiDog · 2025-06-10T08:30:50Z

E2E test should hopefully now be fixed (problem in AWS SDK Go v2 change), trying again

/test pull-cluster-api-provider-aws-e2e

k8s-ci-robot · 2025-06-10T11:06:11Z

@AndiDog: 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-e2e	`460da84`	link	false	`/test pull-cluster-api-provider-aws-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

AndiDog · 2025-06-11T05:58:28Z

/test pull-cluster-api-provider-aws-e2e

fiunchinho · 2025-06-11T07:38:40Z

/lgtm

k8s-ci-robot · 2025-06-11T07:38:45Z

LGTM label has been added.

Git tree hash: 5877d0d7353d151f7da4cf50158f58b43e448858

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 22, 2025

k8s-ci-robot requested review from fiunchinho and serngawy January 22, 2025 15:48

k8s-ci-robot added needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 22, 2025

AndiDog force-pushed the s3-user-data branch from b424f32 to 3908c7c Compare January 24, 2025 07:52

AndiDog mentioned this pull request Jan 24, 2025

✨ Cancel instance refresh on any relevant change to ASG instead of blocking until previous one is finished (which may have led to failing nodes due to outdated join token) #5173

Closed

5 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025

AndiDog force-pushed the s3-user-data branch from 3908c7c to d4559f4 Compare January 24, 2025 08:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025

AndiDog force-pushed the s3-user-data branch 2 times, most recently from 0972f68 to 4a31f81 Compare January 24, 2025 08:59

fiunchinho reviewed Jan 28, 2025

View reviewed changes

fiunchinho approved these changes Jan 28, 2025

View reviewed changes

k8s-ci-robot assigned fiunchinho Jan 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2025

AndiDog force-pushed the s3-user-data branch from 4a31f81 to 635fea5 Compare March 25, 2025 10:35

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 25, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2025

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 28, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2025

AndiDog force-pushed the s3-user-data branch from 3cd1c55 to 22303c7 Compare June 4, 2025 12:48

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 4, 2025

AndiDog force-pushed the s3-user-data branch from 22303c7 to 1c56241 Compare June 4, 2025 14:08

AndiDog force-pushed the s3-user-data branch from 1c56241 to 460da84 Compare June 10, 2025 08:27

Support storing Ignition user data in S3 bucket for AWSMachinePool

e84e321

AndiDog force-pushed the s3-user-data branch from 460da84 to e84e321 Compare June 11, 2025 05:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2025

k8s-ci-robot merged commit e7dddb6 into kubernetes-sigs:main Jun 11, 2025
16 of 17 checks passed

k8s-ci-robot added this to the v2.8 milestone Jun 11, 2025

✨ Support storing Ignition user data in S3 bucket for AWSMachinePool #5318

✨ Support storing Ignition user data in S3 bucket for AWSMachinePool #5318

Uh oh!

Conversation

AndiDog commented Jan 22, 2025 • edited by richardcase Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndiDog commented Jan 24, 2025

Uh oh!

AndiDog commented Jan 24, 2025

Uh oh!

AndiDog commented Jan 27, 2025

Uh oh!

fiunchinho Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

AndiDog Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

fiunchinho Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

AndiDog Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

fiunchinho left a comment

Choose a reason for hiding this comment

Uh oh!

AndiDog commented Mar 25, 2025

Uh oh!

fiunchinho commented Mar 27, 2025

Uh oh!

richardcase commented Apr 9, 2025

Uh oh!

AndiDog commented May 15, 2025

Uh oh!

AndiDog commented May 15, 2025

Uh oh!

fiunchinho commented May 20, 2025

Uh oh!

k8s-ci-robot commented May 20, 2025

Uh oh!

richardcase commented May 28, 2025

Uh oh!

k8s-ci-robot commented May 28, 2025

Uh oh!

fiunchinho commented May 28, 2025

Uh oh!

AndiDog commented Jun 4, 2025

Uh oh!

damdo commented Jun 4, 2025

Uh oh!

damdo commented Jun 4, 2025

Uh oh!

AndiDog commented Jun 10, 2025

Uh oh!

k8s-ci-robot commented Jun 10, 2025

Uh oh!

AndiDog commented Jun 11, 2025

Uh oh!

fiunchinho commented Jun 11, 2025

Uh oh!

k8s-ci-robot commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

✨ Support storing Ignition user data in S3 bucket for `AWSMachinePool` #5318

✨ Support storing Ignition user data in S3 bucket for `AWSMachinePool` #5318

AndiDog commented Jan 22, 2025 •

edited by richardcase

Loading