-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add options for customizing credential providers #4174
Conversation
ping @jasdel |
@jasdel @skmcgrail please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to create this PR @rittneje. This pattern restricts future credential provider initialization options used by the Session
. For example, if a new parameter is needed for a future usaged of the WebIdentityRoleProvider
, a new factory method needs to be added, but the Session
probably must still use the old factory functions if the application has not be updated to use the new method yet. This would impact new features being added to credential providers.
I don't think adding factories for credential provider them self is the pattern the v1 SDK should use. I propose we follow a pattern more similar to the v2 SDK's LoadOptions. Where SessionOptions
has members for providing functional option overrides used when initializing credential providers.
type SessionOptions struct {
// ...
// Set of options that Session will use to modify used credential
// provider's default options.
AssumeRoleProviderOptions func(*stscreds.AssumeRoleProvider)
WebIdentityProviderOptions func(*stscreds.WebIdentityProvider)
// other providers
}
The v2 SDK addresses some of these issues with the LoadOptions type that has functions for specifying Options
for components that could to be created by LoadDefaultConfig](https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/config#LoadDefaultConfig). I've created aws/aws-sdk-go-v2#1523 in the V2 SDK that addresses the issue this PR and issue brought up of not being able to specify the ExpiryWindow
for credentials. Since the V2 SDK moved expiry window concepts to the CredentialsCache
this is a bit easier to work with than V1.
V1's Session and credential providers did not use functional options as a core design principle when they were originally defined. Though, it looks like most of the credential providers did have a way to specify functional options for their Provider
. Except for the WebIdentity credential provider Which would need to be updated have a new constructor taking functional option similar to the other credential providers.
This really sounds more like a statement on the existing
I don't think adding a new constructor is necessary as part of these changes. The session package can invoke the callback itself. I can change the new struct to: type CredentialsProviderOptions struct {
WebIdentityRoleProviderOptions func(*stscreds.WebIdentityRoleProvider)
} to align with what you are proposing for v2. (I believe organizing them under one struct instead of adding them directly to session.Options is cleaner.) While normally the options are a variadic list, I feel a slice here would just add unnecessary complexity. (The API client can just do multiple things in the callback anyway.) |
649261d
to
9a46073
Compare
ping @jasdel |
🚀 |
9a46073
to
15e70a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for taking the time to create this PR @rittneje I've pushed a minor update to add a NewWebIdentityRoleProviderWithOptions
constructor to be similar to the other credential providers defined by the SDK.
Fixes #4160. It's a little different than what I originally proposed, since (1) this avoid an import cycle, and (2) this is a little more flexible.