Bump KMS and Metadata Clients to AWS SDK Go V2 #1169

gargipanatula · 2025-06-18T21:50:39Z

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Continues work on upgrading repo to AWS SDK Go V2. (PR 1: #1146, PR 2: #1157)

Notable Changes:
The IMDS client's signing name and signing region can no longer be overridden. This is because IMDS, unlike other SDK clients, doesn't support signing. (Standard SDK clients use SigV4 while IMDS uses a different protocol).

Trivial Changes:

Upgrades usage of the AWS SDK for Go KMS and IMDS packages to V2.
Combines Ec2V2 client with the EC2 client returned by Compute()
Removed assumeRoleProvider since this was only used by SDK Go V1 clients
Added to e2e SDK client testing in aws_sdk_test.go. Now checks that the signing region & name, not only the URL, were properly overridden.
Removed lots of unused code that was used for SDK V1 implementations, and updated naming for methods/fields that were distinguished as "V2".

Testing:
Ran unit tests with make test and e2e tests with the following commands:

export AWS_REGION=us-west-2
export TEST_PATH=./tests/e2e/...
export GINKGO_NODES=4
export GINKGO_FOCUS=[cloud-provider-aws-e2e]
export GINGKO_SKIP=[Disruptive]
go install github.com/onsi/ginkgo/[email protected]
export PATH=$PATH:$HOME/go/bin

pushd ./tests/e2e
ginkgo . -v -p --nodes=$GINKGO_NODES --focus=$GINKGO_FOCUS --skip=$GINKGO_SKIP

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The Metadata client's signing name and signing region can no longer be overridden.

k8s-ci-robot · 2025-06-18T21:50:49Z

Hi @gargipanatula. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

k8s-ci-robot · 2025-06-18T21:50:49Z

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

gargipanatula · 2025-06-18T22:14:31Z

pkg/providers/v1/aws_test.go

 	signingRegion, signingMethod string
 	signingName                  string
 }



Note: This test used to test both ValidateOverride and the endpoint/signingname/signingregion overriding functionality. Now, it only tests ValidateOverride, and checking the overrides themselves has been moved to aws_sdk_test.go since override logic is configured directly with the clients.

kmala · 2025-06-19T01:26:10Z

/ok-to-test

kmala · 2025-06-19T02:22:23Z

pkg/providers/v1/variant/fargate/fargate.go

 	"fmt"
 	"strings"

+	awsv2 "github.com/aws/aws-sdk-go-v2/aws"


nit: i see that the aws is only use for converting pointer to values and vice versa, can we remove that usage and such that we need not use awsv2

Totally agree, but I think I'll punt this to my final PR in this migration, which will be solely focused on upgrading the remaining packages (just github.com/aws/aws-sdk-go/aws and github.com/aws/aws-sdk-go/aws/awserr). Wanted to keep this PR as simple as possible for ease of review.

kmala · 2025-06-19T02:44:09Z

pkg/providers/v1/aws_ec2.go

 	return results, nil
 }

+func (s *awsSdkEC2) DescribeInstanceTopology(ctx context.Context, input *ec2.DescribeInstanceTopologyInput, optFns ...func(*ec2.Options)) ([]ec2types.InstanceTopology, error) {


I see we are moving the functionality of the https://github.com/kubernetes/cloud-provider-aws/blob/master/pkg/services/aws_ec2.go to here, can we deprecate that file or is it used somewhere else?

Thanks for the catch - these have indeed been moved over so we can deprecate these files.

kmala · 2025-06-19T02:49:38Z

pkg/providers/v1/aws.go

-	instance, err := c.getInstanceByID(ctx, instanceID)
+	instanceIDBytes, err := io.ReadAll(instanceIDMetadata.Content)
+	if err != nil {
+		panic("unable to parse instance id")


why panic here?

Ah yeah, this should probably be an error to match the rest of the code. Will change this in the next commit.

kmala · 2025-06-19T02:52:12Z

pkg/providers/v1/aws_sdk.go

+	p.addAPILoggingMiddleware(&cfg)
+	imdsClient := imds.New(imds.Options{ClientEnableState: imds.ClientEnabled})
+	getInstanceIdentityDocumentOutput, err := imdsClient.GetInstanceIdentityDocument(context.Background(), &imds.GetInstanceIdentityDocumentInput{})
+	identity := getInstanceIdentityDocumentOutput.InstanceIdentityDocument


will getInstanceIdentityDocumentOutput have any value if there is error ?

Thanks for the catch - we should check if err != nil here and return an error, because we get an error if the request fails/is unable to be parsed.

Yeah this line identity := getInstanceIdentityDocumentOutput.InstanceIdentityDocument should be moved under err==nil

kmala · 2025-06-19T02:53:18Z

pkg/providers/v1/aws_sdk.go


 func (p *awsSDKProvider) Metadata() (config.EC2Metadata, error) {
-	sess, err := session.NewSession(&aws.Config{
-		EndpointResolver: p.cfg.GetResolver(),


This should be migrated right?

Thanks for the catch - migrated endpoint resolution. IMDS doesn't support signing, so its resolver will only override the URL. Standard SDK clients use SigV4 while IMDS uses a different protocol.

haoranleo · 2025-06-19T18:13:08Z

pkg/providers/v1/aws_sdk.go

-	sess, err := session.NewSession(&aws.Config{
-		EndpointResolver: p.cfg.GetResolver(),
-	})
+	cfg, err := awsConfig.LoadDefaultConfig(context.TODO())


nit: use the default mode whenever possible

haoranleo · 2025-06-19T18:14:25Z

pkg/providers/v1/aws_sdk.go

+	p.addAPILoggingMiddleware(&cfg)
+	imdsClient := imds.New(imds.Options{ClientEnableState: imds.ClientEnabled})
+	getInstanceIdentityDocumentOutput, err := imdsClient.GetInstanceIdentityDocument(context.Background(), &imds.GetInstanceIdentityDocumentInput{})
+	identity := getInstanceIdentityDocumentOutput.InstanceIdentityDocument


Yeah this line identity := getInstanceIdentityDocumentOutput.InstanceIdentityDocument should be moved under err==nil

gargipanatula · 2025-06-22T01:31:16Z

pkg/providers/v1/config/config.go

+func (cfg *CloudConfig) GetIMDSEndpointOpts() []func(*imds.Options) {
+	opts := []func(*imds.Options){}
+	for _, override := range cfg.ServiceOverride {
+		if override.Service == imds.ServiceID {


Note: Instead of checking the ServiceID and region here, like we do for other clients, we only check ServiceID. This does not result in a behavioral change - the resolver for the SDK V1 ec2metadata package always had an empty value for the region at the time of the request, so only the serviceID field was used for determining whether to override.

Also, we are not overriding the signing name and region here since IMDS does not support signing.

kmala · 2025-06-22T06:55:29Z

pkg/providers/v1/aws.go

 	}
+	macsBytes, err := io.ReadAll(macsMetadata.Content)
+	if err != nil {
+		panic("unable to parse macs")


i don't think we should panic here?

kmala · 2025-06-22T07:17:11Z

pkg/providers/v1/aws_sdk.go

+	// But IMDS uses a different request pattern: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html
+	var opts []func(*imds.Options) = p.cfg.GetIMDSEndpointOpts()
+	opts = append(opts, func(o *imds.Options) {
+		o.ClientEnableState = imds.ClientEnabled // enable requests, otherwise the AWS_EC2_METADATA_DISABLED env var will need to be set


for my understanding what is the difference between default enabled and enabled https://github.com/aws/aws-sdk-go-v2/blob/feature/ec2/imds/v1.16.32/feature/ec2/imds/api_client.go#L37 as that is the default value ?

If we do not configure this variable, it is set to defaultEnable and falls back to whatever the AWS_EC2_METADATA_DISABLED env var is set to.

If we do configure it, then this overrides the value of AWS_EC2_METADATA_DISABLED and acts as if AWS_EC2_METADATA_DISABLED = false.

However, I dug into the old ec2metadata code and it essentially behaves the same way (it needs the same env var to be set to make successful requests). So, I'm going to remove this line so we don't override AWS_EC2_METADATA_DISABLED with o.ClientEnableState = imds.ClientEnabled and we can maintain parity with the old code. Thanks for the callout!

Info about AWS_EC2_METADATA_DISABLED from the docs:

AWS_EC2_METADATA_DISABLED - environment variable Whether or not to attempt to use Amazon EC2 Instance Metadata Service (IMDS) to obtain credentials. Default value: false. Valid values: true – Do not use IMDS to obtain credentials. false – Use IMDS to obtain credentials.

kmala · 2025-06-22T07:17:50Z

pkg/providers/v1/aws.go

 	}
+	macsBytes, err := io.ReadAll(macsMetadata.Content)
+	if err != nil {
+		panic("unable to parse macs")


there shouldn't be panic here right?

kmala · 2025-06-22T07:30:52Z

pkg/providers/v1/aws_test.go

 				t.Errorf("Should succeed for case: %s, got %v", test.name, err)
 			}
-
-			if len(cfg.ServiceOverride) != len(test.servicesOverridden) {


why is this removed? can't we test some if not all?

This part of the test was checking that the signing fields and URL were properly overridden using the generic resolver from V1. Since we now have resolvers on a per client basis, we are now testing the resolvers on a more e2e scale, creating a client and making mock requests (all in aws_sdk_test.go). We're still testing the override functionality, just in a different place.

This test used to test both ValidateOverride() and overriding, now it just tests ValidateOverride().

Let me know if I'm missing something here, though.

i was thinking if we can keep the check len(cfg.ServiceOverride) != len(test.servicesOverridden) but i think it should be okay as this should be tested indirectly in the other function.

gargipanatula · 2025-06-23T21:00:47Z

/retest

kmala · 2025-06-23T23:48:31Z

/lgtm
/approve

k8s-ci-robot · 2025-06-23T23:48:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kmala

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

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

…#1146-#1157-#1169-#1177-upstream-release-1.33 Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2

…#1146-#1157-#1169-#1177-upstream-release-1.32 Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2

#1157-#1169-#1177-upstream-release-1.30 Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2

#1157-#1169-#1177-upstream-release-1.31 Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 18, 2025

k8s-ci-robot requested a review from nckturner June 18, 2025 21:50

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 18, 2025

k8s-ci-robot requested a review from olemarkus June 18, 2025 21:50

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2025

gargipanatula commented Jun 18, 2025

View reviewed changes

k8s-ci-robot 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 Jun 19, 2025

gargipanatula marked this pull request as ready for review June 19, 2025 01:37

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 Jun 19, 2025

k8s-ci-robot requested a review from kmala June 19, 2025 01:37

kmala reviewed Jun 19, 2025

View reviewed changes

haoranleo reviewed Jun 19, 2025

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 22, 2025

gargipanatula commented Jun 22, 2025

View reviewed changes

kmala reviewed Jun 22, 2025

View reviewed changes

aws sdk go upgrade

23058a7

gargipanatula force-pushed the aws-sdk-go-client-bump branch from 21f4dd2 to 23058a7 Compare June 23, 2025 18:29

gargipanatula added 2 commits June 23, 2025 19:11

added enable state

335b81a

reset env var

ced0bae

k8s-ci-robot assigned kmala Jun 23, 2025

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

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

k8s-ci-robot merged commit 561fe08 into kubernetes:master Jun 23, 2025
11 checks passed

This was referenced Jun 24, 2025

Update aws & awserr packages to AWS SDK Go V2 #1177

Merged

Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2 #1178

Merged

gargipanatula mentioned this pull request Jul 1, 2025

Automated cherry pick of #1146: upgraded to ec2 v2, buggy #1157: Update ELB and ELBV2 packages to AWS SDK Go V2 #1169: aws sdk go upgrade #1177: update aws & awserr to go sdk v2 #1180

Merged

Uh oh!

Bump KMS and Metadata Clients to AWS SDK Go V2 #1169

Bump KMS and Metadata Clients to AWS SDK Go V2 #1169

Uh oh!

Conversation

gargipanatula commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 18, 2025

Uh oh!

k8s-ci-robot commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmala commented Jun 19, 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!

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!

gargipanatula Jun 23, 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!

gargipanatula commented Jun 23, 2025

Uh oh!

kmala commented Jun 23, 2025

Uh oh!

k8s-ci-robot commented Jun 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gargipanatula commented Jun 18, 2025 •

edited

Loading

gargipanatula Jun 23, 2025 •

edited

Loading