MultiLang AWS SDK v1 to v2

[ethkatnic]

No description provided.

[ethkatnic]

This is sloppy and needs a second look. The issue here is that we relied on this provider constructor to create any provider class that is passed to this function.

However, in our tests, we use software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider. This used to be invoked like new DefaultCredentialsProvider but now is constructed with DefaultCredentialsProvider.create() - https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/migration-client-credentials.html

I tried to generalize this to work with both, but its quite sloppy.

[chenylee-aws]

Given that we just want v2 support now and the v2 credential providers are either created using create or builder do we still need this check?

[ethkatnic]

Users can still create classes that implement the v2 AwsCredentialsProvider that use a default constructor clazz::newInstance instead of a builder/create.

For instance, we create a basic AwsCredentialsProvider that relies on a default constructor in our testing.

Since this is still a valid way to create v2 providers, I think this code should support both a .create() method and a constructor method of instantiating providers.

[ethkatnic]

Another thing to consider is that we currently support passing args to default constructors through the config like ConstructorCredentialsProvider|arg1|arg2 . This eventually calls a constructor like provider = ConstructorCredentialsProvider(arg1, arg2), allowing the user to pass in a provider string with constructor args. This still works as expected for classes with public static constructors.

I don't think this is possible for the providers that require builders, though. For instance, if a user wanted to create a DefaultCredentialsProvider with profileName = "test-profile" like DefaultCredentialsProvider.builder().profileName("Test").build(), the code currently doesn't support this.

In this version of the code DefaultCredentialsProvider|test-profile, we would try and fail to call a new instance like DefaultCredentialsProvider("test-profile"), and so it would invoke DefaultCredentialsProvider.create() to create a default instance of the class.

So currently, that .create() block is just to create a default builder instance if there's no basic constructor.

[chenylee-aws]

Yes, we should come up with a way that's easy for customer to use the default credential providers without having to peek into the multilang code or documentation to pick the providers. Let's iterate on this

[chenylee-aws]

The new V2 providers are constructed using builders correct? Are they able use the same constructing logic as v1?

[ethkatnic]

New providers are constructed with builders / a create() call to build a generic instance, old providers were created with simple constructors.

To build the old providers generically, we used clazz::newInstance. This will fail for the v2 versions and throw a InstantiationException. To build the new providers generically, my only idea was to catch this InstantiationException and try something like

Method createMethod = clazz.getDeclaredMethod("create");
Object provider = createMethod.invoke(null);

This makes things a bit complicated, as we are trying to generically build these classes in 2 different ways in one function.

[ethkatnic]

Simplified this a bit. Now added an additional attempt to construct create() without touching constructProvider().

[chenylee-aws]

Given that we just want v2 support now and the v2 credential providers are either created using create or builder do we still need this check?

[chenylee-aws]

not a blocker for this PR - This class is an extension of the StsProvider. I feel like the users should not have to pick between the normal stsProvider and KCLstsProvider if they want to add extra fields like externalId etc... We can direct all construction of stsProvider to this class internally so users can use all of its feature by declaring the generic stsProvider in the property file.

I think this is especially important for sdk v2 because credential providers don't provide constructors anymore and this stsProvider doesn't have a create() method either meaning that we will need to rely on this class to build stsProvider

[ethkatnic]

Yeah, agreed. The old docs described creating an STSAssumeRoleSessionCredentialsProvider from the config, which relied on a deprecated constructor for the STSAssumeRoleSessionCredentialsProvider.

As you said, this is no longer possible because the v2 StsAssumeRoleCredentialsProvider requires a builder, so we can't use a simple constructor. In that way, we are forcing users to use the KclStsAssumeRoleCredentialsProvider as I mentioned in the documentation changes.

[chenylee-aws]

Maybe we could try passing in all the args that are in the property file to the create() method? This provides more flexibility and It might not create the expected credential provider if not taking the extra args anyway. But let's see what solution we decide to use for builder based credential providers and we can iterate on it

[chenylee-aws]

Yes, we should come up with a way that's easy for customer to use the default credential providers without having to peek into the multilang code or documentation to pick the providers. Let's iterate on this