Add a RdsUtilities with the ability to generate an IAM auth token

[abrooksv]

Description

Add a new Rds Utilities class that can generate the auth token.
V1 Parity feature
More info: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.Connecting.AWSCLI.html

Utilities pattern is used instead of Presigner due to this is a pseudo request. There does not exist a modeled API call so it does not make sense to make a presigner for it.

Motivation and Context

Upstream this code out of the AWS Toolkit for JetBrains
#1157

Testing

Need to write the 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
• 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 read the README document
• I have added tests to cover my changes
• All new and existing tests passed
• A short description of the change has been added to the CHANGELOG
• 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

[abrooksv]

This was based on a conversation with Matt. Looking for feedback on direction before writing tests and finishing testing + docs

[hunterwerlla]

This probably needs a link to the doc where it states this must be 15 min

[abrooksv]

Ya, our doc pages all say 15 mins: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.html

[sonarqubecloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

[abrooksv]

Ping. Still looking for feedback on direction before committing any more time to this.

[zoewangg]

Looks good overall.

[abrooksv]

Sorry, re:invent kept me from getting back to this Zoe. Thank you for the initial review. Will work on marching this forward

[abrooksv]

@zoewangg Should we stick region/credProvider in:

1. the RdsUtiltiies as fields
2. The request GenerateAuthenticationTokenRequest
3. Both and request takes precedence?

S3 seems to do both 3

[zoewangg]

@abrooksv I'd vote # 3 to be consistent with s3 utilities.

[abrooksv]

Okay, new revision pushed:

1. Interfaces are in place. Went back and forth on how to lay them out.
   SDK Clients make them separate files, but things like Apache builders and request/response builders use inner classes.

Let me know if you want them broken out into 4 files, happy to do it. Or w/e layout you feel is best.

2. CodeGen was modified to allow interfaces for utility classes, new field is instanceType
3. First pass of the Javadoc written.

Need to figure out how we should test this. V1 has no tests for its version of the class 😢

Standard pre-signer methods have access to AwsRequestOverrideConfiguration and use that to inject signer it looks like, but this does not. Should it? (If so, maybe actually SdkRequestOverrideConfiguration since the request already has credentials?)

Should we have a test only constructor on DefaultRdsUtilities to takes a signer?

[codecov-io]

Codecov Report

Merging #2057 (7cedded) into master (f48fa01) will increase coverage by 0.01%.
The diff coverage is 78.35%.

@@             Coverage Diff              @@
##             master    #2057      +/-   ##
============================================
+ Coverage     77.62%   77.64%   +0.01%     
  Complexity      367      367              
============================================
  Files          1237     1240       +3     
  Lines         38904    39001      +97     
  Branches       3064     3068       +4     
============================================
+ Hits          30201    30283      +82     
- Misses         7239     7253      +14     
- Partials       1464     1465       +1     

Flag	Coverage Δ	Complexity Δ
unittests	77.64% <78.35%> (+0.01%)	0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...en/model/config/customization/UtilitiesMethod.java	81.81% <33.33%> (-18.19%)	0.00 <0.00> (ø)
...tware/amazon/awssdk/services/rds/RdsUtilities.java	50.00% <50.00%> (ø)	0.00 <0.00> (?)
...n/awssdk/codegen/poet/client/AsyncClientClass.java	92.00% <75.00%> (-0.31%)	0.00 <0.00> (ø)
...on/awssdk/codegen/poet/client/SyncClientClass.java	92.39% <75.00%> (-0.39%)	0.00 <0.00> (ø)
.../rds/model/GenerateAuthenticationTokenRequest.java	75.00% <75.00%> (ø)	0.00 <0.00> (?)
...mazon/awssdk/services/rds/DefaultRdsUtilities.java	86.00% <86.00%> (ø)	0.00 <0.00> (?)
...ssdk/core/internal/async/FileAsyncRequestBody.java	84.90% <0.00%> (+0.94%)	0.00% <0.00%> (ø%)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f48fa01...1ef3854. Read the comment docs.

[zoewangg]

WRT testing, I don't think it should have access to AwsRequestOverrideConfiguration or SdkRequestOverrideConfiguration. A test ctor with a test singer injected sounds good to me.

[abrooksv]

Test data came from AWS CLI: https://github.com/aws/aws-cli/blob/master/tests/functional/rds/test_generate_db_auth_token.py#L50

[zoewangg]

Left one minor comment, otherwise LGTM. Can you squash all commits into one?

[lbruun]

Just a few clarifications.
Is it correctly understood that no network traffic is involved in generating an IAM Auth Token?

If so, then why model it as if there is? Specifically:

• The PR has a model.*Request class. Such classes are used throughout the SDK for request/replies that travel across the network. Yet in this case the GenerateAuthenticationTokenRequest class serves a completely different purpose, namely solely as a basis for providing input to a method. Comparing it to the SDKv1 implementation you'll notice that at least in that implementation they have not lumped the Request class into the same package where network request/reply classes exist.

• In the PR the RdsUtilities class can be created from the RDS client class too, instead of directly. Again this tricks the user into thinking that the client is somehow involved (it isn't !). The benefits for the API user of creating the RdsUtilities via the client are close to zero, IMO (not having to specify CredentialsProvider and Region again). You can imagine use-cases where the SDK is indeed only being used for creating an IAM Auth Token and I bet you'll then see users who thinks - partly lured by the Javadoc documentation too - that they first have to create an RdsClient in order to create an IAM Auth Token.

Overall, I reckon the idea is to model it after S3Utilities class? Well, that implementation suffers from the same strange type of complexity in my opinion. At least that class has in its Javadoc "Note: This class does not make network calls." which is not the case for RdsUtilities.

Why not simply a class with static methods, named Utils, not Utilities ? I think this would be inline with how the rest of SDK does it. Even in a Utils class you can still springle it with Builder pattern if the method calls would become overly elaborate.

[zoewangg]

Hi @lbruun, first of all, thank you for reviewing the design and providing feedback!

Is it correctly understood that no network traffic is involved in generating an IAM Auth Token?

Yes, it is correct.

The PR has a model.*Request class. Such classes are used throughout the SDK for request/replies that travel across the network. Yet in this case the GenerateAuthenticationTokenRequest class serves a completely different purpose, namely solely as a basis for providing input to a method. Comparing it to the SDKv1 implementation you'll notice that at least in that implementation they have not lumped the Request class into the same package where network request/reply classes exist.

software.amazon.awssdk.services.*.model packages contains POJOs, but they are not limited only to AWS service requests classes.
If a model class extends AwsRequest class, it means this class is a request class to send to AWS services over wire. GenerateAuthenticationTokenRequest is not a subclass of AwsRequest in this case.

In the PR the RdsUtilities class can be created from the RDS client class too, instead of directly. Again this tricks the user into thinking that the client is somehow involved (it isn't !). The benefits for the API user of creating the RdsUtilities via the client are close to zero, IMO (not having to specify CredentialsProvider and Region again). You can imagine use-cases where the SDK is indeed only being used for creating an IAM Auth Token and I bet you'll then see users who thinks - partly lured by the Javadoc documentation too - that they first have to create an RdsClient in order to create an IAM Auth Token.

RdsUtilities can be created either from the builder or the RDS client class. I understand it might be a bit confusing to some customers, and we definitely should improve the Javadocs. The main reason we create a static factory method in the client class is to improve the discoverability of the utility methods. We had internal usability study before where we interviewed customers asking their opinions, and the result indicates that providing a static method to instantiate a class in the SDK client makes the class far more discoverable. When customers type RdsClient in their IDEs, utilities method will show up in the code completion popup window whereas if it's a completely standalone class, customers might never find out the existence of the utilities class. Also, we've gotten a lot of feedback on 1.11.x that it's difficult to find such features hidden in a Utils class.

We will work on the javadocs to make it clear that RdsUtilities does not make network calls. Please let us know if you have further questions/suggestions.

[lbruun]

Hi @zoewangg
Thank you for good explanation. You argue well. I think my concerns can be mitigated with some Javadoc comments:

RdsUtilties class: add "This class does not perform network traffic".

GenerateAuthenticationTokenRequest class: Instead of "Request object to generate an auth token for IAM database authentication." then "Input parameters for generating an auth token for IAM database authentication". (to me at least, the word 'request' is in my brain associated with network traffic)

And speaking of hidden utility classes: Why not take advantage of AwsHostNameUtils.parseSigningRegion() method or at least reference it in comments in Javadoc ?

use like this:

AwsHostNameUtils.parseSigningRegion(hostname, "rds");

Of course it should be optional. User should still be able to provide the Region explicitly. But providing the Region this way is IMO very likely to be correct. In fact I struggle to see when it won't yield the correct result given that the value used for hostname must be the actual RDS endpoint hostname (it can't be an alias, a CNAME). But perhaps there's some Outpost scenario I'm missing when saying it will always yield correct result?

[zoewangg]

@lbruun thanks for the javadoc suggestions! I think they make sense. We have added a note in RdsUtilities to make it clear that it doesn't make network calls. @abrooksv can we update the javadocs for GenerateAuthenticationTokenRequest?

AwsHostNameUtils is more of an internal class, and it doesn't really work well for all endpoints such as VPC endpoint and gov endpoint. It's a a lot of maintenance to enumerate all existing patterns and keep up with the new endpoint patterns.

[zoewangg]

I'll go ahead and merge this PR.

@lbruun thanks again for your feedback 🙂 please let us know if you have further feedback by creating a new issue, and we will address them.

@abrooksv thank you for your contribution, Austin! 🎉

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

80.7% Coverage
0.0% Duplication

[hzmj9h]

I would like to check this changes committed to 2.17.14 version. I couldn't able to locate the software.amazon.awssdk.services.rds.DefaultRdsUtilities.DefaultBuilder newer version.