Accept and use the new AWS Credentials interfaces #3829

gosar · 2023-03-10T21:04:44Z

Motivation and Context

As part of SRA, earlier PR added the new interfaces. This PR updates existing code to accept and use the new interfaces.

Modifications

This PR updates existing interfaces like AwsClientBuilder and others where customers used to be able to provide AwsCredentialsProvider, to be able to provide the new interface - IdentityProvider<? extends AwsCredentialsIdentity>.

Pending:

~~Code generator changes for endpoint discovery case - SyncClientClass.~~
~~I'm still working on adding tests.~~
Also need to update few more classes like AwsCredentialsProviderChain called out in design doc, and confirm if there are any more places. But to wrap up this PR and not make it bigger, will do that in separate PR.
Will update code for new Token interfaces later too, in separate PR.
Potentially move CREDENTIALS_IDENTITY_PROVIDER into SdkClientBuilder to support generic clients using AWS Credentials. Maybe in separate PR.

Testing

Updated existing tests. Added new tests.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
Ran ./mvnw clean install -pl :auth,:aws-core,:s3,:polly,:rds -am -Pquick locally
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

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

In AwsClientBuilder and other places where customers used to be able to provide AwsCredentialsProvider.

...test/resources/software/amazon/awssdk/codegen/poet/client/test-endpoint-discovery-async.java

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/CredentialUtils.java

millems · 2023-03-17T21:13:44Z

core/auth/src/main/java/software/amazon/awssdk/auth/signer/AwsSignerExecutionAttribute.java

+    // TODO: Can the type be changed to IdentityProvider? This class is @SdkProtectedApi
    public static final ExecutionAttribute<AwsCredentials> AWS_CREDENTIALS = new ExecutionAttribute<>("AwsCredentials");


We might be able to get away with it if we did a minor version bump and told customers we'll be breaking protected APIs, but we probably want to save that until the Smithy code generator migration, where we'll likely have to start breaking a lot of protected things.

For now, is there a way to add an AWS_CREDENTIALS_IDENTITY and support both of them? (AWS_CREDENTIALS for old consumer versions and AWS_CREDENTIALS_IDENTITY for new ones?) We could add a TODO comment or annotation for these things we want to clean up with the Smithy code generator migration so that we can make sure we don't forget them?

Hmm, I put this comment as I thought I may make some changes to signer code to accept the new types as part of this PR, but Anna-Karin and I decided we can tackle that in part of the signer changes. And for now using toCredentials when interfacing with the signer code. As part of signer changes we can add AWS_CREDENTIALS_IDENTITY and support both, with a TODO comment as you said. For now, same comment applies to AwsClientOption.AWS_CREDENTIALS I think, so added a TODO there. Using a // smithy codegen TODO: convention. LMK if you have a different suggestion for that.

core/aws-core/src/main/java/software/amazon/awssdk/awscore/AwsRequestOverrideConfiguration.java

...ain/java/software/amazon/awssdk/services/polly/internal/presigner/DefaultPollyPresigner.java

services/rds/src/main/java/software/amazon/awssdk/services/rds/DefaultRdsUtilities.java

.../main/java/software/amazon/awssdk/services/rds/model/GenerateAuthenticationTokenRequest.java

gosar

There are a few comments from Matt still pending, but responding with some here.

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/CredentialUtils.java

core/aws-core/src/main/java/software/amazon/awssdk/awscore/AwsRequestOverrideConfiguration.java

...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

gosar · 2023-03-23T22:59:17Z

...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

-        return config.option(AwsClientOption.CREDENTIALS_PROVIDER) != null
-               ? config.option(AwsClientOption.CREDENTIALS_PROVIDER)
+    private IdentityProvider<? extends AwsCredentialsIdentity> resolveCredentialsIdentityProvider(SdkClientConfiguration config) {
+        // Note, that CREDENTIALS_PROVIDER is never set. It is replaced with CREDENTIALS_IDENTITY_PROVIDER, so just check that


Hmm, given your comment above, for older clients, we need to set CREDENTIALS_PROVIDER, so putting this comment at the definition of the option would be confusing. Given the comment above, I'm going to remove this comment here and instead add a comment above explaining why CREDENTIALS_PROVIDER is being set. LMK if you think I should comment things differently.

...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

...in/java/software/amazon/awssdk/awscore/internal/client/config/AwsClientOptionValidation.java

gosar · 2023-03-24T00:18:21Z

...n/java/software/amazon/awssdk/awscore/internal/authcontext/AuthorizationStrategyFactory.java

-        AwsCredentialsProvider defaultCredentialsProvider = clientConfiguration.option(AwsClientOption.CREDENTIALS_PROVIDER);
+        IdentityProvider<? extends AwsCredentialsIdentity> defaultCredentialsProvider =
+            clientConfiguration.option(AwsClientOption.CREDENTIALS_IDENTITY_PROVIDER);


Actually, similar to another comment, my thought on this being in the same aws-core module as AwsDefaultClientBuilder, makes me wonder if CREDENTIALS_IDENTITY_PROVIDER will still be set for older clients, that use the newer aws-core? Maybe there are other code paths or something I'm not thinking through.

To use the new credentialsIdentityProvider() instead.

...3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java

.../auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsCredentialsProviderChain.java

core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/config/AwsClientOption.java

millems · 2023-03-31T22:56:05Z

core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/config/AwsClientOption.java

+    // smithy codegen TODO: This could be removed when doing a minor version bump where we told customers we'll be breaking
+    //  protected APIs. Postpone this to when we do Smithy code generator migration, where we'll likely have to start
+    //  breaking a lot of protected things.


Should we put this somewhere that it doesn't mess up the javadoc for this field?

It's in comments, so shouldn't affect javadoc. Either ways, if you think I should do something different here, lmk.

Ah, I thought that since this comment separated the field from the javadoc that it would mess up the rendering.

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/CredentialUtils.java

sonarqubecloud · 2023-04-01T01:17:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
33 Code Smells

73.3% Coverage
4.2% Duplication

gosar requested a review from cenedhryn March 10, 2023 21:04

gosar referenced this pull request Mar 10, 2023

Use new AwsClientOption for IdentityProvider

8e10ec5

Accept and use the new AWS Credentials interfaces

8ac22a4

In AwsClientBuilder and other places where customers used to be able to provide AwsCredentialsProvider.

gosar force-pushed the gosar/sra-ia-identity-overload-3 branch from 342c987 to 8ac22a4 Compare March 10, 2023 21:23

Update client codegen for endpoint discovery

6c3095b

millems reviewed Mar 17, 2023

View reviewed changes

Address some of Matt's feedback

2247529

gosar commented Mar 27, 2023

View reviewed changes

gosar added 9 commits March 27, 2023 15:29

Switch one usage of overrideConfiguration.credentialsProvider

30635e2

To use the new credentialsIdentityProvider() instead.

Update tests to mock new IdentityProvider

093be1f

Handle null for CredentialUtils conversion methods

0a6bab3

Add IdentityProvider overload to S3CrtAsyncClientBuilder

96bce3a

Add a TODO for removing a join() later

4d372c9

Add TODO for AwsCredentialsProviderChain

952a4fc

Add unit tests and few other minor changes

51834e3

Remove unnecessary fallback from AuthorizationStrategyFactory

b2098a6

Fix test in S3

22370c2

cenedhryn reviewed Mar 31, 2023

View reviewed changes

...3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/DefaultS3CrtAsyncClient.java Show resolved Hide resolved

gosar marked this pull request as ready for review March 31, 2023 22:34

gosar requested a review from a team as a code owner March 31, 2023 22:34

cenedhryn approved these changes Mar 31, 2023

View reviewed changes

millems reviewed Mar 31, 2023

View reviewed changes

millems approved these changes Mar 31, 2023

View reviewed changes

Address PR feedback

c7fb92f

millems approved these changes Apr 3, 2023

View reviewed changes

gosar merged commit 53f5cb3 into feature/master/sra-identity-auth Apr 3, 2023

gosar deleted the gosar/sra-ia-identity-overload-3 branch April 3, 2023 19:15

This was referenced Apr 3, 2023

Remove unused methods in DefaultS3CrtClientBuilder #3879

Merged

Update AwsCredentialsProviderChain for new Identity type #3883

Merged

This was referenced Apr 25, 2023

Remove identity join in async client for endpoint discovery #3947

Merged

Remove identity join in async client for ExecutionAttributes #3959

Closed

		// TODO: Can the type be changed to IdentityProvider? This class is @SdkProtectedApi
		public static final ExecutionAttribute<AwsCredentials> AWS_CREDENTIALS = new ExecutionAttribute<>("AwsCredentials");

Accept and use the new AWS Credentials interfaces #3829

Accept and use the new AWS Credentials interfaces #3829

Uh oh!

Conversation

gosar commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Uh oh!

millems Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

gosar Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gosar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gosar Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gosar Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

millems Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

gosar Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

millems Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Apr 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gosar commented Mar 10, 2023 •

edited

Loading