Multilang - Provider Decoder Improvements #2
Conversation
ethkatnic
changed the base branch from
aws-sdk-v1-to-v2
to
multilang-sdk-upgrade
ethkatnic
changed the title
| */ | ||
| private static Class<? extends AwsCredentialsProvider> getClass(String providerName) { | ||
| // Convert any form of StsAssumeRoleCredentialsProvider string to KclStsAssumeRoleCredentialsProvider | ||
| if (providerName.equals(StsAssumeRoleCredentialsProvider.class.getSimpleName()) |
There was a problem hiding this comment.
We want to map a provided config string like StsAssumeRoleCredentialsProvider or software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider to our own KclProvider in order to parse the |<arn>|<sessionName> if provided in the config.
| if (!AwsCredentialsProvider.class.isAssignableFrom(c)) { | ||
| return null; | ||
| } | ||
| clazz = (Class<? extends AwsCredentialsProvider>) c; |
There was a problem hiding this comment.
nit: I think we can remove line 194 and 208 by just returning here?
| ``` | ||
| AWSCredentialsProvider = StsAssumeRoleCredentialsProvider|<arn>|<sessionName>` | ||
| AWSCredentialsProvider = StsAssumeRoleCredentialsProvider|<arn>|<sessionName> |
There was a problem hiding this comment.
I wonder if we should change this example to use [Aws]CredentialsProvider just to be consistent with SDK Java v2 naming. Non blocking but let's discuss
There was a problem hiding this comment.
Yeah, I thought of this as well. The downside of this is that it would break backwards compatibility to old multilang config files if we change this from AWSCredentialsProvider to AwsCredentialsProvider throughout the application. While it would better fit the v2 SDK naming, its just a naming convention change that would likely cause some headaches for users. I'm happy to update this, I just wasn't sure if it was worth the tradeoff.
I can make a separate MR for this, as it will be changes in ~8 files.
| if (provider != null) { | ||
| credentialsProviders.add(provider); | ||
| } | ||
| } | ||
| return credentialsProviders; | ||
| } | ||
|
|
||
| private static AwsCredentialsProvider tryConstructorWithArgs( |
There was a problem hiding this comment.
Can we combine tryConstructorWithArgs with tryConstructorWithNoArgs into one method since they take in similar arguments?
We can simplify the return logic like this
AwsCredentialsProvider provider = constructProvider(providerName, () -> getConstructorWithVarArgs(clazz, varargs));
if (provider == null) {
provider = constructProvider(providerName, () -> getConstructorWithArgs(clazz, varargs));
}
if (provider == null) {
provider = constructProvider(providerName, () -> getCreateMethod(clazz));
}
return provider;
We can apply this to the Create related methods too.
There was a problem hiding this comment.
Addressed, simplifying overall logic significantly.
Upon reviewing all of the old/new providers, we can support the v1 behavior quite easily. While we originally assumed that many of the old providers were being created with some constructor like Provider(string, string...) and we were losing that functionality with them moving to builders, there were only 2 providers that could be created like Constructor(String, String...) in the v1 SDK.
STSAssumeRoleSessionsCredentialsProvider
ProfileCredentialsProvider
Adds a mapping for StsAssumeRoleCredentialsProvider -> KclStsAssumeRoleCredentialsProvider. A user can now input config string
AWSCredentialsProvider = StsAssumeRoleSessionCredentialsProvider|<arn>|<sessionName>and it will parse as expected because we are internally mapping this to our custom processor. Adds a test to validate this.ProfileCredentialsProvider has a create(String) method that takes in a profile name. Modifies the provider creator to first try a
.create(arg1)before defaulting to.create(), so this version will supportAWSCredentialsProvider = ProfileCredentialsProvider|<profileName>this as well. Adds a provider test class and tests to validate this works for both the emptycreate()andcreate(String).Refactors the provider creator to attempt to invoke a constructor with args/varargs, create with args/varargs, constructor with no args, create with no args. This PR adds a try to
.create()with args/varargs to support providers that have.create(String...)methods. Other code changes here are just refactors.Updates documentation to reflect this.
With these changes, we support the same provider string configuration that was supported in the v1 version. We will still instruct users to create their own custom processor if they need a more configured provider, just like in the v1.