Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go SDK v2 #429

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Go SDK v2 #429

wants to merge 4 commits into from

Conversation

micahhausler
Copy link
Member

@micahhausler micahhausler commented Feb 11, 2025

Issue #, if available:
N/A

Description of changes:

Upgrade project to use AWS SDK Go v2. The AWS SDK Go (v1)'s End of Life is July 31, 2025, this PR upgrades the SKD for this project.

Depends on the following PRs being merged first. I'll rebase after those are done

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@micahhausler micahhausler force-pushed the go-sdk-v2 branch 2 times, most recently from 4cfb60c to bd7a968 Compare February 12, 2025 15:44
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 47.09677% with 82 lines in your changes missing coverage. Please review.

Project coverage is 51.30%. Comparing base (e9d3aca) to head (330f388).

Files with missing lines Patch % Lines
credential_provider/irsa_credential_provider.go 55.00% 18 Missing ⚠️
provider/secrets_manager_provider.go 0.00% 15 Missing ⚠️
auth/auth.go 54.83% 13 Missing and 1 partial ⚠️
provider/parameter_store_provider.go 0.00% 14 Missing ⚠️
...ntial_provider/pod_identity_credential_provider.go 61.29% 12 Missing ⚠️
server/server.go 75.00% 2 Missing and 2 partials ⚠️
provider/secret_provider.go 0.00% 3 Missing ⚠️
main.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
- Coverage   52.62%   51.30%   -1.32%     
==========================================
  Files          11       11              
  Lines         952      879      -73     
==========================================
- Hits          501      451      -50     
+ Misses        436      417      -19     
+ Partials       15       11       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member Author

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few comments that should help with explaining the changes. I'm happy to schedule an online/live review and walkthrough if that is helpful

podName: podName,
preferredAddressType: preferredAddressType,
usePodIdentity: usePodIdentity,
k8sClient: k8sClient,
stsClient: stsClient,
ctx: ctx,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context is meant to be passed through function calls. To quote from the context package documentation

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. This is discussed further in https://go.dev/blog/context-and-structs.

Rather than putting it in the struct, it can be synchronously passed in GetAWSConfig(context.Context) below.

// by using a private TokenFetcher helper.
func (p Auth) GetAWSSession() (awsSession *session.Session, e error) {
var credProvider credential_provider.CredentialProvider
func (p Auth) GetAWSConfig(ctx context.Context) (aws.Config, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer a "Session" type in V2 of the SDK, just aws.Config{}, so I've renamed that appropriately

sess.Handlers.Build.PushFront(func(r *request.Request) {
request.AddToUserAgent(r, ProviderName)
// add the user agent to the config
cfg.APIOptions = append(cfg.APIOptions, func(stack *middleware.Stack) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header/UserAgent functionality is different in V2, See the docs on request customization

// It matches stscreds.TokenFetcher interface.
type authTokenFetcher interface {
FetchToken(ctx credentials.Context) ([]byte, error)
GetAWSConfig(ctx context.Context) (aws.Config, error)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than force a unifying method signature for getting a token and creating unnecessary wrappers, we can use the interface types provided in each credential method for retrieving tokens, and just make the ConfigProvider return an aws.Config{} with the correct AWS cred provider

Comment on lines +38 to +39
k8sv1.CoreV1Interface // satisfy the interface
fake k8sv1.CoreV1Interface // plumb down to the ServiceAccounts method
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both the interface satisfied, and an addressable field so that .ServiceAccounts().CreateToken() and .ServiceAccounts().Get() can be called from the same mock type.


tokenOut, err := fetcher.FetchToken(nil)
cfg, _ := provider.GetAWSConfig(context.Background())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config creation won't ever error: Getting the token is async, and happens on cfg.Credentials.Retrieve()

@@ -33,11 +31,15 @@ type ParameterStoreProvider struct {
clients []ParameterStoreClient
}

type ParameterStoreGetter interface {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no longer an ssmiface package, we just create an interface for the one call we need to make

@@ -31,10 +29,15 @@ type SecretsManagerProvider struct {
clients []SecretsManagerClient
}

type SecretsManagerGetDescriber interface {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same story on secretsmanageriface, just a custom interface to satisfy the type for easier testing

Region: *awsSession.Config.Region,
Client: secretsmanager.New(awsSession, aws.NewConfig().WithRegion(regions[i])),
Region: regions[i],
Client: secretsmanager.NewFromConfig(cfg),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to double check, but I may need to override the cfg.Region = regions[i].

"github.com/aws/aws-sdk-go-v2/service/secretsmanager"
secretsmanagertypes "github.com/aws/aws-sdk-go-v2/service/secretsmanager/types"
"github.com/aws/aws-sdk-go-v2/service/ssm"
ssmtypes "github.com/aws/aws-sdk-go-v2/service/ssm/types"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is worth skimming, but I'm really not doing anything other than accounting for how SDK v2's types are slightly different structures, and error messages print differently

@micahhausler micahhausler marked this pull request as ready for review February 12, 2025 16:35
@micahhausler micahhausler requested a review from a team as a code owner February 12, 2025 16:36
@micahhausler micahhausler force-pushed the go-sdk-v2 branch 2 times, most recently from 6125874 to 9ee7592 Compare February 12, 2025 16:51
Signed-off-by: Micah Hausler <[email protected]>
Replate by running:

    go install golang.org/x/tools/cmd/goimports@latest
    goimports -format-only -w ./

Signed-off-by: Micah Hausler <[email protected]>
Go has strong convention around passing context.Contxt as the first call
to functions.

I intentionally did not fix auth/ and credential_provider/ misuses of
context, as that will be fixed in a later PR updating the AWS Go SDK.

Signed-off-by: Micah Hausler <[email protected]>
Signed-off-by: Micah Hausler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants