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

Introduce AddCredentialsFactory() extension methods #3715

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

Conversation

chase-miller
Copy link

@chase-miller chase-miller commented Mar 20, 2025

Description

This PR updates the AWSSDK.Extensions.NETCore.Setup nuget package with the following:

  • Introduces IServiceCollection.[Try]AddCredentialsFactory extension methods
  • Provides a DefaultAWSCredentialsFactory that is effectively a move of the private ClientFactory.CreateCredentials method
  • Updates the ClientFactory to use the injected IAWSCredentialsFactory
  • Adds a registration for AWSCredentials so that it can be injected by consuming code.

Motivation and Context

There are scenarios where it's necessary to obtain AWSCredentials. One scenario is adding sigv4 headers when making RESTful calls using the idiomatic HttpClient; as an aside I happen to be using the AwsSignatureVersion4 nuget package for this. Today a consumer has to effectively copy the ClientFactory.CreateCredentials method to be able to register an AWSCredentials object that is created in a way that leverages AWSOptions. With these changes a consumer simply needs to call services.AddCredentialsFactory() and then inject AWSOptions where needed.

These updates also provide a hook for a consumer to customize the way in which AWSCredentials are provided to ServiceClients.

Testing

I added 10 unit tests to the DependencyInjectionTests. I'm also using what is effectively a fork with these changes in production code.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [?] My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@dscpinheiro
Copy link
Contributor

Is there an existing issue with more details on the problem you're trying to solve?

We made significant changes to both the AWSSDK.Extensions.NETCore.Setup package and credential resolution in our upcoming V4 version (#3362) and I'm afraid a lot of the work you've done cannot be reviewed as is.

@chase-miller
Copy link
Author

chase-miller commented Mar 20, 2025

Is there an existing issue with more details on the problem you're trying to solve?

Nope I haven't created an issue, but I'd be happy to do so. What additional detail are you looking for? Maybe code samples?

We made significant changes to both the AWSSDK.Extensions.NETCore.Setup package and credential resolution in our upcoming V4 version (#3362) and I'm afraid a lot of the work you've done cannot be reviewed as is.

How should it change so that it can be reviewed?

@chase-miller
Copy link
Author

chase-miller commented Mar 20, 2025

P.S. Would it be better if I opened a PR that targets a v4 branch? I looked at the v4-release branch, and it appears that these changes could be applied against that version of the lib. I think the problem I'm trying to solve for remains in v4 (ClientFactory still uses a private method to resolve credentials), but I only spent a couple minutes looking at the code.

@chase-miller
Copy link
Author

@dscpinheiro FYI that I just created issue #3716 to provide additional context. Let me know if it makes sense to implement these changes against the v4-development branch.

@chase-miller
Copy link
Author

@dscpinheiro bump :-)

…k breaks FallbackCredentialsFactory's assumptions

RefreshingAWSCredentials will throw an exception if it's been disposed of (and used via FallbackCredentialsFactory)
@chase-miller chase-miller force-pushed the add-credentials-factory-extensions branch from 4014711 to 288b90e Compare April 1, 2025 17:06
@normj
Copy link
Member

normj commented Apr 10, 2025

The feature of having an opt-in mechanism on AWSSDK.Extensions.NETCore.Setup that adds AWSCredentials to the dependency injection container makes sense to me. We do need to retarget this PR to v4-development. V4 is close to being GA and at this point any new features should be first done in V4 and then we can debate about back porting to V3.

A question I have is should we injecting AWSCredentials instead of a IAWSCredentialsFactory since that is what is likely going to to be used on other service's constructor. There could also be registered the IAWSCredentialsFactory and then the service register for AWSCredentials would grab the IAWSCredentialsFactory and use that. And the register method would be AddAWSCredentials instead of AddCredentialsFactory. I'm just not sure how many users will not what the factory is and that would appear more of an implementation detail to get to what users want which is the credentials.

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.

3 participants