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

add options for customizing credential providers #4174

Merged
merged 2 commits into from Jan 4, 2022

Conversation

rittneje
Copy link
Contributor

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.

@rittneje
Copy link
Contributor Author

ping @jasdel

@rittneje
Copy link
Contributor Author

@jasdel @skmcgrail please review

@rittneje
Copy link
Contributor Author

Copy link
Contributor

@jasdel jasdel left a 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.

@rittneje
Copy link
Contributor Author

rittneje commented Dec 7, 2021

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.

This really sounds more like a statement on the existing stscreds.NewWebIdentityRoleProvider constructor than on the factory per se, as you cannot add new parameters to that constructor.

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.

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.)

@rittneje rittneje changed the title add factory for customizing credential providers add options for customizing credential providers Dec 9, 2021
@rittneje rittneje requested a review from jasdel December 9, 2021 21:50
@rittneje
Copy link
Contributor Author

ping @jasdel

@davidmerwin
Copy link

🚀

@jasdel jasdel assigned skmcgrail and unassigned skmcgrail Jan 3, 2022
@jasdel jasdel requested a review from skmcgrail January 3, 2022 19:00
@jasdel jasdel force-pushed the credential-provider-factory branch from 9a46073 to 15e70a6 Compare January 4, 2022 00:06
Copy link
Contributor

@jasdel jasdel left a 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.

@jasdel jasdel merged commit 92b5621 into aws:main Jan 4, 2022
@rittneje rittneje deleted the credential-provider-factory branch January 4, 2022 01:16
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.

Add option callbacks to aws.Config for modifying default credentials
4 participants