[#367] Enhanced multi-lang `AWSCredentialsProvider=...` decoder and c… #1184

stair-aws · 2023-08-03T19:38:49Z

Issue #, if available:
#367

Description of changes:
[#367] Enhanced multi-lang AWSCredentialsProvider=... decoder and construction.

added support for external ids (issue STSAssumeRoleSessionCredentialsProvider with external ID #367)
added support for endpoint+region (e.g., STS via VPC)

mvn clean verify yields:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Amazon Kinesis Client Library 2.5.2-SNAPSHOT:
[INFO] 
[INFO] Amazon Kinesis Client Library ...................... SUCCESS [  1.018 s]
[INFO] Amazon Kinesis Client Library for Java ............. SUCCESS [14:57 min]
[INFO] amazon-kinesis-client-multilang .................... SUCCESS [ 13.918 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  15:12 min
[INFO] Finished at: 2023-08-03T15:04:21-04:00

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

stair-aws · 2023-08-03T19:49:03Z

With this commit, KCL multi-lang customers should have greater control over the STS provider. For example, a CredentialsProvider string might change from:

AWSCredentialsProvider = STSAssumeRoleSessionCredentialsProvider|<arn>|<sessionName>

... to:

AWSCredentialsProvider = KclSTSAssumeRoleSessionCredentialsProvider|<arn>|<sessionName>|externalId=<eid>

... which satisfies the external id request (issue #367).

Additionally, STS-via-VPC should be accessible via:

AWSCredentialsProvider = KclSTSAssumeRoleSessionCredentialsProvider|<arn>|<sessionName>|endpoint=<url>^<region>

externalId= and endpoint= can also co-exist, in any order. Please see NestedPropertyKeys enum for further details.

…r and construction. + added support for external ids (issue awslabs#367) + added support for endpoint+region (e.g., STS via VPC)

akidambisrinivasan

Looks good

+ added `ENDPOINT_REGION` nested key for a simpler Cx experience + deduplicated, and improved, logic w.r.t. CredentialsProvider construction to NOT swallow Exceptions

stair-aws · 2023-08-05T00:05:32Z

A new simpler option now exists for STS-via-VPC:

AWSCredentialsProvider = KclSTSAssumeRoleSessionCredentialsProvider|<arn>|<sessionName>|endpointRegion=<region>

With this, a Cx only needs to provide the region (e.g., |endpointRegion=us-east-1), and the SDK client will determine the endpoint URL on its own. If Cx need to specify the endpoint and region, the |endpoint=<url>^<region> optional parameter provides more explicit control.

akidambisrinivasan · 2023-08-07T19:02:49Z

I have a general question, how can customers find out all the supported property kv pairs that can provided to MultiLangDaemon and what the valid values are? I see your detailed javadoc for NestedPropertyKey, but I as a customer would not know to look there unless I read the code. I see MultiLangDaemon has javadoc for the minimum properties that must be passed in.
At the minimum we should add a comment there to look at NestedPropertyKey javadoc to know how to format the value of the AWSCredentialsProvider property.

akidambisrinivasan · 2023-08-07T18:45:29Z

amazon-kinesis-client-multilang/src/main/resources/multilang.properties

@@ -0,0 +1,93 @@
+# The script that abides by the multi-language protocol. This script will


If this used only for test, can it be in src/test/resources?

Absolutely, and the correct thing to do; was an oversight in folder assignment. Thanks!

akidambisrinivasan · 2023-08-07T18:46:36Z

...ent-multilang/src/test/java/software/amazon/kinesis/multilang/MultiLangDaemonConfigTest.java

@@ -200,4 +198,9 @@ public void testPropertyValidation() {
        }
    }

+    @Test
+    public void testActualPropertiesFile() throws Exception {
+        new MultiLangDaemonConfig(FILENAME);


What is this testing? Just constructor works fine?
Maybe we want to assert constructed instance is not null?

What is this testing? Just constructor works fine?

I added Javadoc. Snippet: "catch any issues which might arise if there is a discrepancy between reality and mocking".

Maybe we want to assert constructed instance is not null?

assertNotNull(...) gets marked as redundant because the construction is guaranteed non-null iff an Exception is not thrown.

But if you dont assert and the test throws exception it may not show as failed, isnt it?

I think it does mark the test failed, so this is okay.

stair-aws · 2023-08-07T19:46:14Z

#1184 (comment)

I have a general question, how can customers find out all the supported property kv pairs that can provided to MultiLangDaemon and what the valid values are? I see your detailed javadoc for NestedPropertyKey, but I as a customer would not know to look there unless I read the code. I see MultiLangDaemon has javadoc for the minimum properties that must be passed in.
At the minimum we should add a comment there to look at NestedPropertyKey javadoc to know how to format the value of the AWSCredentialsProvider property.

Yes, discoverability is an issue. This will be addressed by the following:

WIP, PR pending: provide a README-esque doc in this KCL repo
- PR purposefully kept separate soas not to slow, or impede, the approval+merge of this PR
WIP, PR pending: point to this new documentation in https://github.com/awslabs/amazon-kinesis-client-python/
- We can propagate outward to other KCL multilang variants, but Python is the need-now
Being explored: work w/ the AWS docs team to author a top-level doc (e.g., https://docs.aws.amazon.com/streams/latest/dev/kcl-migration.html)

If you have further questions/suggestions about the discovery process, let's bring that conversation internal so I can share more details. I believe we're aligned in wanting to equip our KCL Cx to self-service (and make it easy for Cx to contribute new use cases).

…rces`

stair-aws added enhancement v2.x Issues related to the 2.x version labels Aug 3, 2023

stair-aws requested a review from akidambisrinivasan August 3, 2023 19:38

stair-aws force-pushed the creds-provider branch 2 times, most recently from 5389a0f to c0be0cb Compare August 3, 2023 21:46

[awslabs#367] Enhanced multi-lang AWSCredentialsProvider=... decode…

1295088

…r and construction. + added support for external ids (issue awslabs#367) + added support for endpoint+region (e.g., STS via VPC)

stair-aws force-pushed the creds-provider branch from c0be0cb to 1295088 Compare August 4, 2023 01:17

akidambisrinivasan previously approved these changes Aug 4, 2023

View reviewed changes

Multiple multi-lang edits to introduce logging and additional tests.

6809f29

+ added `ENDPOINT_REGION` nested key for a simpler Cx experience + deduplicated, and improved, logic w.r.t. CredentialsProvider construction to NOT swallow Exceptions

stair-aws dismissed akidambisrinivasan’s stale review via 6809f29 August 4, 2023 23:58

akidambisrinivasan reviewed Aug 7, 2023

View reviewed changes

Relocated multilang.properties from main/resources to `test/resou…

6fc3fae

…rces`

akidambisrinivasan approved these changes Aug 7, 2023

View reviewed changes

stair-aws merged commit 2f4ff65 into awslabs:master Aug 7, 2023
1 check passed

stair-aws added the multi lang daemon label Aug 7, 2023

stair-aws mentioned this pull request Aug 7, 2023

Provided documentation for multilang's new NestedPropertyKey enhancem… #1186

Merged

stair-aws deleted the creds-provider branch August 7, 2023 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#367] Enhanced multi-lang `AWSCredentialsProvider=...` decoder and c… #1184

[#367] Enhanced multi-lang `AWSCredentialsProvider=...` decoder and c… #1184

stair-aws commented Aug 3, 2023

stair-aws commented Aug 3, 2023

akidambisrinivasan left a comment

stair-aws commented Aug 5, 2023

akidambisrinivasan commented Aug 7, 2023

akidambisrinivasan Aug 7, 2023

stair-aws Aug 7, 2023

akidambisrinivasan Aug 7, 2023

stair-aws Aug 7, 2023

akidambisrinivasan Aug 7, 2023

akidambisrinivasan Aug 7, 2023

stair-aws commented Aug 7, 2023

		@@ -0,0 +1,93 @@
		# The script that abides by the multi-language protocol. This script will

[#367] Enhanced multi-lang AWSCredentialsProvider=... decoder and c… #1184

[#367] Enhanced multi-lang AWSCredentialsProvider=... decoder and c… #1184

Conversation

stair-aws commented Aug 3, 2023

stair-aws commented Aug 3, 2023

akidambisrinivasan left a comment

Choose a reason for hiding this comment

stair-aws commented Aug 5, 2023

akidambisrinivasan commented Aug 7, 2023

akidambisrinivasan Aug 7, 2023

Choose a reason for hiding this comment

stair-aws Aug 7, 2023

Choose a reason for hiding this comment

akidambisrinivasan Aug 7, 2023

Choose a reason for hiding this comment

stair-aws Aug 7, 2023

Choose a reason for hiding this comment

akidambisrinivasan Aug 7, 2023

Choose a reason for hiding this comment

akidambisrinivasan Aug 7, 2023

Choose a reason for hiding this comment

stair-aws commented Aug 7, 2023

[#367] Enhanced multi-lang `AWSCredentialsProvider=...` decoder and c… #1184

[#367] Enhanced multi-lang `AWSCredentialsProvider=...` decoder and c… #1184