🌱 Migrate gc to aws sdk v2 #5518

Danil-Grigorev · 2025-06-03T18:45:03Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate ELB garbage collection to AWS SDK v2

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5406

Special notes for your reviewer:

Checklist:

Release note:

Migrate ELB garbage collection to AWS SDK v2

Danil-Grigorev · 2025-06-04T07:06:25Z

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

damdo · 2025-06-04T14:13:22Z

/assign @nrb

pkg/cloud/convertersv2/tags.go

pkg/cloud/filterv2/ec2.go

nrb · 2025-06-25T21:06:58Z

Merging another PR unfortunately introduced some conflicts on this one. Also, the copyright for brand new files should be the current year.

Migrate the garbage collection service and related components to use the AWS SDK v2. This involves updating dependencies, refactoring client initialization, and updating API calls within the GC logic. Adds new v2 specific converters, filters, and mocks. Migrate the garbage collection logic for AWS Application Load Balancers (ALBs) and Network Load Balancers (NLBs) to use the AWS SDK v2. Migrate the garbage collection logic for Classic Load Balancers (ELB) from the AWS SDK v1 to v2. Signed-off-by: Danil-Grigorev <[email protected]>

Signed-off-by: Danil-Grigorev <[email protected]>

k8s-ci-robot · 2025-06-26T09:10:43Z

[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 ask for approval from andidog. For more information see the 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

damdo · 2025-06-26T09:35:51Z

@alexander-demicev would you be able to have a second look and re-add your LGTM if you are happy with it?
Thanks!

alexander-demicev

/lgtm

k8s-ci-robot · 2025-06-26T10:07:35Z

LGTM label has been added.

Git tree hash: cef4df8dd78cc2740930f3275f6bb2f33d481ebf

alexander-demicev · 2025-06-26T10:07:43Z

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

damdo · 2025-06-26T10:15:12Z

/hold

for testing to pass

richardcase · 2025-06-26T10:26:41Z

pkg/cloud/scope/clients.go

-	ResourceTagging resourcegroupstaggingapiiface.ResourceGroupsTaggingAPIAPI
+	ASG               autoscaling.Client
+	EC2               ec2iface.EC2API
+	ELB               elbiface.ELBAPI


Should we not be adding the ELBv2 interface here?

It is not used anywhere in the updated code as far as I can recall, and to keep changes slim I didn’t add it here. Do you think this needs to be a part of the PR?

AndiDog · 2025-06-25T15:28:09Z

Makefile

@@ -606,7 +606,7 @@ promote-images: $(KPROMO) $(YQ)

 .PHONY: release-binaries
 release-binaries: $(GORELEASER) ## Builds only the binaries, not a release.
-	$(GORELEASER) build --config $(GORELEASER_CONFIG) --snapshot --clean
+	GOMAXPROCS=2 $(GORELEASER) build --config $(GORELEASER_CONFIG) --snapshot --clean


Change is unrelated – can we take this out?

I don’t think so, this change fixes stuck goreleaser build in CI: #5518 (comment) I was able to reproduce it locally, and this change fixes it. Some other AWS SDK v2 rewrite PRs are affected by this too.

AndiDog · 2025-06-26T15:06:56Z

pkg/cloud/filterv2/types.go

+*/
+
+// Package filterv2 contains the ec2 sdk v2 related filters.
+package filterv2


If you refer to filterv2 in general, it is needed and used for example in https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/5518/files#diff-fb7618ec3e9301951d4d093fa53e72a5f8ee1ef919d4213dcf6f530125f800ecR29 I simply copied filter package, the types.go there is empty as well.

AndiDog · 2025-06-26T15:12:54Z

pkg/cloud/services/gc/cleanup_test.go

 									{
-										Key:   aws.String("kubernetes.io/cluster/cluster1"),
+										Key:   aws.String("kubernetes.io/cluster/eks-test-cluster"),


Why does the cluster name need to be changed in this test?

AndiDog · 2025-06-26T15:16:36Z

pkg/cloud/services/gc/loadbalancer.go

@@ -80,7 +80,8 @@ func (s *Service) deleteTargetGroups(ctx context.Context, resources []*AWSResour
 }

 func (s *Service) isELBResourceToDelete(resource *AWSResource, resourceName string) bool {
-	if !s.isMatchingResource(resource, elb.ServiceName, resourceName) {
+	// Need to update this to use the v2 service name if it's different


Is this a TODO? It's unclear what needs to be done / what's left to do. Which v2 (ELBv2 or AWS SDK Go v2), ...?

V2 packages stopped using ServiceName variables. I think this can be removed though.

AndiDog · 2025-06-26T15:18:03Z

pkg/cloud/services/gc/loadbalancer.go

@@ -134,14 +135,17 @@ func (s *Service) deleteTargetGroup(ctx context.Context, targetGroupARN string)
 // describeLoadBalancers gets all elastic LBs.
 func (s *Service) describeLoadBalancers(ctx context.Context) ([]string, error) {
 	var names []string
-	err := s.elbClient.DescribeLoadBalancersPagesWithContext(ctx, &elb.DescribeLoadBalancersInput{}, func(r *elb.DescribeLoadBalancersOutput, last bool) bool {
-		for _, lb := range r.LoadBalancerDescriptions {
+	// AWS SDK v2 does not have PagesWithContext, need to use paginator


Comment can be removed since we get the context timeout functionality via NextPage(ctx)

k8s-ci-robot · 2025-06-27T11:10:52Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Danil-Grigorev <[email protected]>

k8s-ci-robot · 2025-06-27T11:37:04Z

@Danil-Grigorev: 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
pull-cluster-api-provider-aws-e2e	`c2c6a4d`	link	false	`/test pull-cluster-api-provider-aws-e2e`
pull-cluster-api-provider-aws-apidiff-main	`8ee8a1e`	link	false	`/test pull-cluster-api-provider-aws-apidiff-main`

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.

k8s-ci-robot requested review from Ankitasw and richardcase June 3, 2025 18:45

k8s-ci-robot added needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 3, 2025

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from 7b8080f to 1822ed1 Compare June 3, 2025 19:05

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

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from 1822ed1 to adc451e Compare June 3, 2025 19:06

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

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from a75be30 to b39b276 Compare June 4, 2025 06:31

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

Danil-Grigorev changed the title ~~[WIP] Migrate gc to aws sdk v2~~ 🌱 Migrate gc to aws sdk v2 Jun 4, 2025

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch 5 times, most recently from fccf39b to 12a5c0a Compare June 4, 2025 07:04

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jun 4, 2025

Danil-Grigorev mentioned this pull request Jun 4, 2025

Migrate gc code to AWS SDK v2 #5406

Open

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from 12a5c0a to 2cfd1e6 Compare June 4, 2025 08:28

k8s-ci-robot assigned nrb Jun 4, 2025

k8s-ci-robot assigned richardcase Jun 24, 2025

nrb reviewed Jun 25, 2025

View reviewed changes

pkg/cloud/convertersv2/tags.go Outdated Show resolved Hide resolved

nrb reviewed Jun 25, 2025

View reviewed changes

pkg/cloud/filterv2/ec2.go Outdated Show resolved Hide resolved

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

Danil-Grigorev added 2 commits June 26, 2025 11:10

Set GOMAXPROCS to allow release-binaries to pass

13f92c2

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from e991120 to 13f92c2 Compare June 26, 2025 09:10

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

alexander-demicev approved these changes Jun 26, 2025

View reviewed changes

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

damdo requested a review from nrb June 26, 2025 10:14

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

richardcase reviewed Jun 26, 2025

View reviewed changes

damdo requested a review from richardcase June 26, 2025 14:39

AndiDog reviewed Jun 26, 2025

View reviewed changes

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from c2c6a4d to 5012b2c Compare June 27, 2025 11:10

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

k8s-ci-robot requested review from alexander-demicev, AndiDog and dlipovetsky June 27, 2025 11:10

Update licence year, address review

8ee8a1e

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the migrate-gc-to-aws-sdk-v2 branch from 5012b2c to 8ee8a1e Compare June 27, 2025 11:27

🌱 Migrate gc to aws sdk v2 #5518

Are you sure you want to change the base?

🌱 Migrate gc to aws sdk v2 #5518

Uh oh!

Conversation

Danil-Grigorev commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danil-Grigorev commented Jun 4, 2025

Uh oh!

damdo commented Jun 4, 2025

Uh oh!

Uh oh!

Uh oh!

nrb commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

damdo commented Jun 26, 2025

Uh oh!

alexander-demicev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

alexander-demicev commented Jun 26, 2025

Uh oh!

damdo commented Jun 26, 2025

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!

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!

k8s-ci-robot commented Jun 27, 2025

Uh oh!

k8s-ci-robot commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Danil-Grigorev commented Jun 3, 2025 •

edited

Loading

k8s-ci-robot commented Jun 27, 2025 •

edited

Loading