Migrate aws sdk go v1 to v2 #3844

shraddhabang · 2024-09-05T21:51:15Z

Issue

Description

As AWS SDK for GO v1 has gone into maintenance mode, this PR migrates the AWS SDK for GO v1 to v2

The migration includes the following key changes:

Updated dependencies: The go.mod file has been modified to use the AWS SDK for Go v2 dependencies.
Package imports: All imports referencing the v1 SDK have been updated to use the v2 packages.
Service clients: Service clients have been created using the NewFromConfig method in v2.
Waiters: The waiter functionality has been migrated to use the new waiter pattern in v2.
API calls: API calls have been updated to use the correct parameters and data types in v2.

Benefits:

Improved performance and efficiency: The AWS SDK for Go v2 offers performance enhancements and new features.
Enhanced security: The v2 SDK incorporates security best practices and addresses potential vulnerabilities.
Simplified usage: The v2 SDK provides a more intuitive and streamlined API.

Testing:

Unit tests: Unit tests have been updated to ensure the code works as expected with the v2 SDK.
Integration tests: Integration tests have been run to verify compatibility with other components of the application.

Please review the changes carefully and provide any feedback or suggestions

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

shraddhabang · 2024-09-19T18:10:55Z

/retest

oliviassss · 2024-09-18T21:55:41Z

pkg/annotations/parser.go

@@ -106,6 +110,20 @@ func (p *suffixAnnotationParser) ParseInt64Annotation(annotation string, value *
 	return true, nil
 }

+func (p *suffixAnnotationParser) ParseInt32Annotation(annotation string, value *int32, annotations map[string]string, opts ...ParseOption) (bool, error) {


Why do we need this ParseInt32Annotation() in addition to ParseInt64Annotation()?

Theaws-sdk-v2 now migrated all the int64 types to int32. Hence to minimize the conversions after parsing. I have created a new parsing method.

pkg/aws/metrics/collector.go

pkg/backend/endpoint_types.go

pkg/deploy/elbv2/listener_utils.go

pkg/deploy/elbv2/load_balancer_manager.go

pkg/deploy/elbv2/target_group_manager.go

pkg/deploy/elbv2/target_group_manager_test.go

pkg/ingress/model_build_load_balancer.go

pkg/networking/backend_sg_provider.go

pkg/networking/node_eni_info_resolver.go

shraddhabang · 2024-09-19T23:33:42Z

/retest

shraddhabang · 2024-09-20T19:14:04Z

/retest pull-aws-load-balancer-controller-e2e-test

k8s-ci-robot · 2024-09-20T19:14:06Z

@shraddhabang: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-aws-load-balancer-controller-e2e-test
/test pull-aws-load-balancer-controller-lint
/test pull-aws-load-balancer-controller-unit-2
/test pull-aws-load-balancer-controller-unit-3
/test pull-aws-load-balancer-controller-unit-4
/test pull-aws-load-balancer-controller-unit-test

Use /test all to run the following jobs that were automatically triggered:

pull-aws-load-balancer-controller-e2e-test
pull-aws-load-balancer-controller-lint
pull-aws-load-balancer-controller-unit-test

In response to this:

/retest pull-aws-load-balancer-controller-e2e-test

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.

shraddhabang · 2024-09-20T19:14:23Z

/test pull-aws-load-balancer-controller-e2e-test

oliviassss · 2024-09-20T21:25:27Z

/lgtm
/assign @M00nF1sh
sending for a second opinion from @M00nF1sh just in case, given it's a large change.

M00nF1sh · 2024-09-20T22:16:41Z

pkg/annotations/parser.go

+	if !exists {
+		return false, nil
+	}
+	i, err := strconv.ParseInt(raw, 10, 64)


should be 32 instead of 64

M00nF1sh · 2024-09-20T22:22:38Z

pkg/aws/cloud.go

-	}
-	sess := session.Must(session.NewSessionWithOptions(opts))
-	injectUserAgent(&sess.Handlers)
+	awsConfig, err := config.LoadDefaultConfig(context.TODO(),


cfg.AWSEndpoints seems dropped support? It's a flag used to override api endpoints (e.g. FIPS ones).

seems endpoints.RegionalSTSEndpoint is not explicitly specified, is it the default behavior of AWSSDKv2? We don't want to use global STS endpoint.

I have verified that the GO SDK v2 uses regional STS endpoints by default. https://docs.aws.amazon.com/sdkref/latest/guide/feature-sts-regionalized-endpoints.html#:~:text=SDK%20for%20Go%20V2,Request%20failure

Good catch. I have added support for custom AWSEndpoints now in recent commit. Please take a look.

M00nF1sh · 2024-09-20T22:27:55Z

pkg/aws/cloud.go

 	if metricsRegisterer != nil {
 		metricsCollector, err := metrics.NewCollector(metricsRegisterer)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to initialize sdk metrics collector")
 		}
-		metricsCollector.InjectHandlers(&sess.Handlers)
+		awsConfig.APIOptions = append(awsConfig.APIOptions, func(stack *smithymiddleware.Stack) error {
+			return metrics.WithSDKCallMetricCollector(metricsCollector)(stack)


i'd prefer to have a single method like WithSDKMetricsCollector instead of two. If we want to support optional collect either call/request metrics only, we can pass the setting into WithSDKMetricsCollector.

Makes sense. I have fixed this in recent commit

M00nF1sh · 2024-09-20T22:48:14Z

left a few comments.
The major one that must change is aws-api-endpoints support is dropped. Which is needed for FIPS endpoint support.
The rest are minor one.

I'm ok to merge this into main branch and fix those via follow-up PR to unblock other changes (since we don't do releases from main).

shraddhabang · 2024-09-20T23:38:54Z

/test pull-aws-load-balancer-controller-e2e-test

shraddhabang · 2024-09-23T16:18:46Z

/test pull-aws-load-balancer-controller-e2e-test

oliviassss · 2024-09-23T21:17:29Z

/lgtm
/approve

shraddhabang · 2024-09-23T21:48:34Z

/test pull-aws-load-balancer-controller-e2e-test

oliviassss · 2024-09-23T21:58:05Z

/lgtm
/approve

k8s-ci-robot · 2024-09-23T21:58:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oliviassss, shraddhabang

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

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

* Migrate aws sdk go v1 to v2 * Migrate aws sdk go v1 to v2 * Adding pending tasks * Fix the endpoint ports to avoid reconciles * Addressing Yang's comments * Resolve merge conflicts

Migrate aws sdk go v1 to v2

3a5ffdb

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2024

k8s-ci-robot requested review from kishorj and oliviassss September 5, 2024 21:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 5, 2024

shraddhabang and others added 3 commits September 5, 2024 16:56

Migrate aws sdk go v1 to v2

9961470

Merge branch 'kubernetes-sigs:main' into main

6346fbe

Adding pending tasks

7796c0e

shraddhabang changed the title ~~[WIP] Migrate aws sdk go v1 to v2~~ Migrate aws sdk go v1 to v2 Sep 16, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2024

Fix the endpoint ports to avoid reconciles

886ada2

oliviassss reviewed Sep 19, 2024

View reviewed changes

k8s-ci-robot assigned M00nF1sh and oliviassss Sep 20, 2024

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

M00nF1sh reviewed Sep 20, 2024

View reviewed changes

Merge branch 'kubernetes-sigs:main' into main

77aaf66

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2024

Addressing Yang's comments

d4bf180

oliviassss added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 23, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 23, 2024

Merge branch 'main' into main

83f7110

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2024

Resolve merge conflicts

a1a71c9

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2024

k8s-ci-robot merged commit 356c904 into kubernetes-sigs:main Sep 24, 2024
9 checks passed

shraddhabang mentioned this pull request Sep 30, 2024

🥳 aws-load-balancer-controller v2.9.0 Automated Release! 🥑 aws/eks-charts#1165

Merged

phuhung273 mentioned this pull request Nov 2, 2024

The controller need upgrade to AWS SDK Go v2 aws/amazon-vpc-resource-controller-k8s#471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate aws sdk go v1 to v2 #3844

Migrate aws sdk go v1 to v2 #3844

shraddhabang commented Sep 5, 2024 •

edited

Loading

shraddhabang commented Sep 19, 2024

oliviassss Sep 18, 2024

shraddhabang Sep 19, 2024

shraddhabang commented Sep 19, 2024

shraddhabang commented Sep 20, 2024

k8s-ci-robot commented Sep 20, 2024

shraddhabang commented Sep 20, 2024

oliviassss commented Sep 20, 2024

M00nF1sh Sep 20, 2024

shraddhabang Sep 23, 2024

M00nF1sh Sep 20, 2024

shraddhabang Sep 23, 2024

M00nF1sh Sep 20, 2024

shraddhabang Sep 23, 2024

M00nF1sh commented Sep 20, 2024 •

edited

Loading

shraddhabang commented Sep 20, 2024

shraddhabang commented Sep 23, 2024

oliviassss commented Sep 23, 2024

shraddhabang commented Sep 23, 2024

oliviassss commented Sep 23, 2024

k8s-ci-robot commented Sep 23, 2024

Migrate aws sdk go v1 to v2 #3844

Migrate aws sdk go v1 to v2 #3844

Conversation

shraddhabang commented Sep 5, 2024 • edited Loading

Issue

Description

Benefits:

Testing:

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

shraddhabang commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shraddhabang commented Sep 19, 2024

shraddhabang commented Sep 20, 2024

k8s-ci-robot commented Sep 20, 2024

shraddhabang commented Sep 20, 2024

oliviassss commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh commented Sep 20, 2024 • edited Loading

shraddhabang commented Sep 20, 2024

shraddhabang commented Sep 23, 2024

oliviassss commented Sep 23, 2024

shraddhabang commented Sep 23, 2024

oliviassss commented Sep 23, 2024

k8s-ci-robot commented Sep 23, 2024

shraddhabang commented Sep 5, 2024 •

edited

Loading

M00nF1sh commented Sep 20, 2024 •

edited

Loading