Update awskms to aws-sdk-go-v2 #293

emilia-grant · 2025-10-14T19:30:09Z

This PR updates awsutil to use aws-sdk-go-v2 because v1 has gone EOL. This is also part of addressing ICU-17267.

It required an API breaking change, so I bumped the mod version to v3. Not sure if this is the correct way to do it here (does this remove v2?)

I also refactored the tests into a suite pattern as a demo. This is a particularly good spot to use the suite pattern because the code only gets touched once in a blue moon, so encoding as much understanding about how the code should function directly into tests will help the next person two years from now.

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

hashicorp-cla-app · 2025-10-14T19:30:23Z

All committers have signed the CLA.

hashicorp-cla-app · 2025-10-14T19:30:23Z

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

johanbrandhorst

Thank you so much for updating the tests - they were in dire need of some love! I'm not sure I see the utility in using gomock or a test suite in this specific case though. Do you think you could try refactoring the tests so that they use the basic Go test setup (e.g. TestFoo(t *testing.T) and t.Run for sub tests), so we can compare? I'd also appreciate if you could separate the changes into separate commits, so I can view:

Just the dependency upgrade (with any changes to existing tests required). This can also include the module version bump
Just the test changes

I love the new tests but I don't know if we need the suite or gomock integratoin. What do you think?

johanbrandhorst · 2025-10-15T03:40:02Z

wrappers/awskms/awskms.go

+	Encrypt(ctx context.Context, input *kms.EncryptInput, opts ...func(*kms.Options)) (*kms.EncryptOutput, error)
+	Decrypt(ctx context.Context, input *kms.DecryptInput, opts ...func(*kms.Options)) (*kms.DecryptOutput, error)
+	DescribeKey(ctx context.Context, inpput *kms.DescribeKeyInput, opts ...func(*kms.Options)) (*kms.DescribeKeyOutput, error)
+}


Was the old interface removed? Please add a doc comment since this is exported.

johanbrandhorst · 2025-10-15T03:40:32Z

wrappers/awskms/awskms.go

 const (
 	EnvAwsKmsWrapperKeyId   = "AWSKMS_WRAPPER_KEY_ID"
 	EnvVaultAwsKmsSealKeyId = "VAULT_AWSKMS_SEAL_KEY_ID"
+	EnvAwsKmsEndpoint       = "AWS_KMS_ENDPOINT"


I see below that we need to keep this environment variable, but could we perhaps also look for AWSKMS_ENDPOINT and deprecate AWS_KMS_ENDPOINT? It's unfortunate that the formatting differs to AWSKMS_WRAPPER_KEY_ID IMO.

johanbrandhorst · 2025-10-15T03:43:00Z

wrappers/awskms/awskms.go

 				return nil, errors.New("no key information returned")
 			}
-			k.currentKeyId.Store(aws.StringValue(keyInfo.KeyMetadata.KeyId))
+			k.currentKeyId.Store(*keyInfo.KeyMetadata.KeyId)


what is the type of KeyId? Are we storing a pointer?

k.currentKeyId stores a string but keyinfo.KeyMetadata stores a *string. The reason for this was that aws v1 used pointers for basically all fields & parameters and so provided aws.String() to work around the awkwardness of taking the address of a module level const string.

They changed philosophies in sdk v2 and use fewer pointers, so I figured I'd use the plain pointer syntax to make clearer that there's nothing special going on with the types here.

johanbrandhorst · 2025-10-15T03:45:03Z

wrappers/awskms/awskms.go

+	credsConfig := &awsutil.CredentialsConfig{
+		AccessKey:            k.accessKey,
+		SecretKey:            k.secretKey,
+		SessionToken:         k.sessionToken,
+		Filename:             k.sharedCredsFilename,
+		Profile:              k.sharedCredsProfile,
+		RoleARN:              k.roleArn,
+		RoleSessionName:      k.roleSessionName,
+		WebIdentityTokenFile: k.webIdentityTokenFile,
+		Region:               k.region,
+		Logger:               k.logger,
+		HTTPClient:           cleanhttp.DefaultClient(),
+	}


johanbrandhorst · 2025-10-15T03:46:17Z

wrappers/awskms/awskms.go

+	kmsOpts := []func(*kms.Options){}
+
+	client := kms.NewFromConfig(cfg, kmsOpts...)


Do we intend to add any options here? I think we can just omit this variable if not.

johanbrandhorst · 2025-10-15T04:01:29Z

wrappers/awskms/go.mod

@@ -1,33 +1,48 @@
-module github.com/hashicorp/go-kms-wrapping/wrappers/awskms/v2
+module github.com/hashicorp/go-kms-wrapping/wrappers/awskms/v3


This seems reasonable to me since it's a major dependency change, thanks.

Is this the correct way to do it so that we don't break client code on v2? i.e. if I merge this, will other modules still be able to pull v2? I was debating this vs have a separate v2 and v3 folder

johanbrandhorst · 2025-10-15T04:01:39Z

wrappers/awskms/go.mod

+module github.com/hashicorp/go-kms-wrapping/wrappers/awskms/v3

-go 1.20
+go 1.24.0


Good call keeping this N-1

emilia-grant · 2025-10-17T20:01:48Z

Thank you so much for updating the tests - they were in dire need of some love! I'm not sure I see the utility in using gomock or a test suite in this specific case though. Do you think you could try refactoring the tests so that they use the basic Go test setup (e.g. TestFoo(t *testing.T) and t.Run for sub tests), so we can compare? I'd also appreciate if you could separate the changes into separate commits, so I can view:
1. Just the dependency upgrade (with any changes to existing tests required). This can also include the module version bump

2. Just the test changes
I love the new tests but I don't know if we need the suite or gomock integratoin. What do you think?

Yeah I'll split them out into a couple commits to see the difference. I don't think gomock is necessary here since we already have mocks created and we don't use thorough enough DI to get the benefits.

I do think the suite pattern here is my hill to die on, though. Once I split out the commits, it'll be crystal clear how much clarity it's providing here, but I have another example in the meantime from another module I'm working on: https://github.com/hashicorp/go-secure-stdlib/blob/main/awsutil/generate_credentials_test.go#L232. Take a look at the suite I wrote out in this PR and then compare it against the TestGenerateAwsConfigOptions tests (with no offense towards the person that wrote the tests. It's the pattern not the person)

We're already writing adhoc test suites, we're just doing it with worse tooling (inline test case types and loops with t.Run). This means every non-trivial test function we write gets its own bespoke test harness logic that's just different enough to trip you up.
This particular adhoc suite is actually three separate logical suites kludged together. You can tell by the three giant if statements on lines 409, 422, and 431. Each one tests a completely different subset of functionality.
- Now why did this happen? Because splitting it into three functions is a giant PITA. It requires three new loops, three sets of boilerplate, and three sets of assertions. Instead, all of the logic got mashed together and each new test case added now has to fulfill additional unrelated assertions, even if thing being tested doesn't interact with those assertions. (I know because I'm in the middle of divining exactly which fields need to be set to make my test pass.)
It might seem innocuous, but the secret, sinister part is that any of those test cases could have implicit, hidden dependencies on state set up by any of the tests run before it. How well do you know the side effects of the functions under test? (0% for me!) How much do you trust the developer that wrote the test to handle cleanup? (No idea!)
The kicker for me is imagining being the developer who gets to step into this codebase after no one's touched it for the next two years. Would you rather see a test suite or a kludge of test functions?

emilia-grant added 2 commits October 8, 2025 14:56

update to aws sdk v2 and restructure tests

8531584

add test for shared config, cleanup

3bf84a9

add error on no shared config test

a75e09f

psekar requested review from ddebko, johanbrandhorst, kheina, louisruch and sgmiller and removed request for sgmiller October 14, 2025 23:44

johanbrandhorst requested changes Oct 15, 2025

View reviewed changes

emilia-grant force-pushed the egrant-aws-sdk-v2 branch from b7302d0 to a75e09f Compare October 17, 2025 19:03

		kmsOpts := []func(*kms.Options){}

		client := kms.NewFromConfig(cfg, kmsOpts...)

		@@ -1,33 +1,48 @@
		module github.com/hashicorp/go-kms-wrapping/wrappers/awskms/v2
		module github.com/hashicorp/go-kms-wrapping/wrappers/awskms/v3

Update awskms to aws-sdk-go-v2 #293

Are you sure you want to change the base?

Update awskms to aws-sdk-go-v2 #293

Uh oh!

Conversation

emilia-grant commented Oct 14, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PCI review checklist

Uh oh!

hashicorp-cla-app bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashicorp-cla-app bot commented Oct 14, 2025

Uh oh!

johanbrandhorst left a comment • 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!

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!

emilia-grant commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

emilia-grant commented Oct 14, 2025 •

edited by atlassian bot

Loading

hashicorp-cla-app bot commented Oct 14, 2025 •

edited

Loading

johanbrandhorst left a comment •

edited

Loading