AwsSdk2Transport implementation #177

mtimmerm · 2022-07-07T19:18:37Z

This PR adds a transport implementation that can connect to AWS OpenSearch service using IAM authentication. Http clients and request signers from the AWS SDK v2 are used directly.

In order to test with the integration tests, you should set system property tests.awsSdk2support.domainHost to the FQDN of an OpenSearch service domain, and tests.awsSdk2support.domainRegion to its region if it's not us-east-1. It must accept IAM authentication using your configured default credentials. You can set the AWS_PROFILE environment variable if necessary.

The AwsSdk2Transport supports both synchronous and asynchronous requests with either synchronous or asynchronous AWS HTTP clients, as well as automatic request and response compression and bulk requests.

This PR also adds a builder for TransportOptions, which has always been missing since RestClientTransport was the only transport.

An optional dependency on the required AWS SDK packages has been added.

dblock

First, thank you! Amazing.

I did a quick review to get started and left some relatively small comments. I'll take a more thorough look as we go.

How does this interact with wanting compressed/chunked requests?

@reta you might be interested in looking at this too

java-client/build.gradle.kts

...client/src/main/java/org/opensearch/client/transport/aws/AsyncByteArrayContentPublisher.java

...-client/src/main/java/org/opensearch/client/transport/aws/AsyncCapturingResponseHandler.java

java-client/src/main/java/org/opensearch/client/transport/aws/AsyncCapturingSubscriber.java

java-client/src/main/java/org/opensearch/client/transport/aws/AwsV2OpenSearchTransport.java

reta · 2022-07-08T17:39:53Z

@mtimmerm @dblock I think we should not bundle anything vendor-specific (like AWS fe) into java-client, with that being said, it may not be worth creating separate repository but new java-client-aws or java-client-transport-aws module would make sense, wdyt?

VachaShah

Thank you very much @mtimmerm!

java-client/src/test/java/org/opensearch/client/opensearch/integTest/awsv2/AwsV2SearchTest.java

java-client/src/main/java/org/opensearch/client/transport/aws/AwsV2OpenSearchTransport.java

mtimmerm · 2022-07-09T12:39:53Z

@reta About the idea of publishing a separate package out of the same repo:

I don't really have a strong opinion on that, but I think that is something the code owners would do after this is merged. I would certainly not be offended.

My not-so-strong opinion, though, is that this library could really use a few transport implementations (this one, a java.net.http one, and maybe an apache client one), and it already includes a transport that is dependent on the RestClient. As long as the rest client one is in here, it's consistent to have the others here. Moving the RestClient transport out will require a major version bump.

Also, it is harmless, since the dependencies are optional, and the gradle "feature variant" functionality was designed to accomplish exactly this sort of thing: https://blog.gradle.org/optional-dependencies

mtimmerm · 2022-07-09T12:47:05Z

Switched to [DRAFT] for a bit. I'm going to add a specific transport options class with compression params.

dblock · 2022-07-09T21:52:08Z

@mtimmerm @dblock I think we should not bundle anything vendor-specific (like AWS fe) into java-client, with that being said, it may not be worth creating separate repository but new java-client-aws or java-client-transport-aws module would make sense, wdyt?

Is the only difference of making a module or creating separate projects with using optional dependencies (gradle features) that we tell users to include software.amazon.awssdk .auth in their dependencies themselves vs. a hypothetical java-client-aws which includes it as its own dependency? If so, feels like making RestClient an optional dependency as well, along with apache client and AWS SDK v2, is a good option. It also solves the fact that opensearch-java is dependent on including a specific version of OpenSearch REST client, we might want to boot that altogether in favor of the apache transport client as @mtimmerm was suggesting to restore JDK8 support.

I do agree that it feels fishy when we bundle something vendor specific. Do you feel the same way about having code like AWS Sigv4 support, that's vendor specific, in the same library @reta?

Implementation of automatic request and response compression. Integration test Make sure bulk requests work Properly parse 403 errors from OpenSearch service (they don't follow OS format) Ensure that every transport error with a status code is reported as an OpenSearchError Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

reta · 2022-07-11T13:53:55Z

Thanks @mtimmerm

I don't really have a strong opinion on that, but I think that is something the code owners would do after this is merged. I would certainly not be offended.

Acceptable, sure

Also, it is harmless, since the dependencies are optional, and the gradle "feature variant" functionality was designed to accomplish exactly this sort of thing: https://blog.gradle.org/optional-dependencies

This is not only about the dependencies but more about separation of core from extensions: the code vast majority of the users do not need should not be bundled. Plus we could manage release / bugfixes separately, without asking everyone to update to the new Java client releases (I could also elaborate more on specifics, please let me know, but I think it makes sense). Also, this opens up the path to easily add other transports (GCP? Azure? ...)

Thanks @dblock

Is the only difference of making a module or creating separate projects with using optional dependencies (gradle features) that we tell users to include software.amazon.awssdk .auth in their dependencies themselves vs. a hypothetical java-client-aws which includes it as its own dependency?

With respect to dependencies - yes, it also make the intent very explicit: the user needs AWS client extension.

I do agree that it feels fishy when we bundle something vendor specific. Do you feel the same way about having code like AWS Sigv4 support, that's vendor specific, in the same library @reta?

Yes, I do, the vendor specific extensions should be out of core but available on demand.

dblock · 2022-07-11T18:07:40Z

Thanks for chiming in @reta!

@mtimmerm I would take almost anything that doesn't break the current code/project and implements Sigv4 since it's a huge user pain. Let me know how I can help get this over the hill (including doing module work @reta is suggesting after we merge something).

mtimmerm · 2022-07-11T18:16:10Z

@dblock this is ready to go as far as I'm concerned. Request/response compression is working and integration tested, and I think everything is in final form.

You have an outstanding change request that I think I've taken care of, and a pending review request to @madhusudhankonda.

Y'all can merge it at your leisure, and then it's yours.

dblock

Sorry I missed the update @mtimmerm.

There are a bunch of files with more than just SPDX headers still, like java-client/src/main/java/org/opensearch/client/util/OpenSearchRequestBodyBuffer.java. I just want to confirm that these are copied from other files that have that license header? If they are brand new, please remove all but the SPDX header.

I'm good with this code! I'd want @reta to give another pair of eyes for any must have's.

How does one use this? I think our README should really have some basic examples, maybe it's worth starting with this? I'll try to write some, so not an ask for this PR unless you feel super motivated.

dblock · 2022-07-11T18:43:36Z

...client/src/main/java/org/opensearch/client/transport/aws/AsyncByteArrayContentPublisher.java

+ * compatible open source license.
+ */
+
+/*


This is new code too, remove everything except SPDX.

dblock · 2022-07-11T18:43:48Z

...-client/src/main/java/org/opensearch/client/transport/aws/AsyncCapturingResponseHandler.java

+ * compatible open source license.
+ */
+
+/*


Same as above.

mtimmerm · 2022-07-11T18:55:59Z

Most of these files are new, but you have checkstyle configured to ensure that all 3 headers are present. I don't mind having them all the headers in this code, but if you want just the SPDX one then I can configure your checkstyle to skip the directories I put stuff in (your integration test directory is similarly skipped already). Please advise.

The integration test demonstrates how to use it: https://github.com/mtimmerm/opensearch-java/blob/7ba29d1069e1f0daaf8219e8c57d0ed34d92ba9c/java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2TransportTestCase.java#L90

dblock · 2022-07-11T19:12:34Z

Most of these files are new, but you have checkstyle configured to ensure that all 3 headers are present. I don't mind having them all the headers in this code, but if you want just the SPDX one then I can configure your checkstyle to skip the directories I put stuff in (your integration test directory is similarly skipped already). Please advise.

The integration test demonstrates how to use it: https://github.com/mtimmerm/opensearch-java/blob/7ba29d1069e1f0daaf8219e8c57d0ed34d92ba9c/java-client/src/test/java/org/opensearch/client/opensearch/integTest/aws/AwsSdk2TransportTestCase.java#L90

The rule is that we only add SPDX headers for all new code, and that we don't remove any existing copyright. I think checkstyle should just have SPDX rather than exclude directories? We can enforce non-removal in PRs I think.

Let me just take that separately, #178.

mtimmerm · 2022-07-11T23:41:34Z

OK, I'll update the license headers when #178 is merged.

dblock · 2022-07-12T19:17:02Z

Thanks for hanging in with me here @mtimmerm. I talked to the right people and confirmed #178, which is now merged. Care to update?

Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

dblock

I'll merge this and we can cleanup comments and other minor stuff as we go. I'll take on experimenting with this and writing some samples/docs.

dblock · 2022-07-12T21:22:20Z

java-client/src/main/java/org/opensearch/client/transport/aws/AsyncCapturingSubscriber.java

+    @Override
+    public void onError(Throwable e) {
+        if (e == null) {
+            e = new IllegalArgumentException("Subscriber.onError called with null paramter");


typo: parameter

VachaShah

LGTM, thank you @mtimmerm!

VachaShah · 2022-07-13T18:21:33Z

I'll merge this and we can cleanup comments and other minor stuff as we go. I'll take on experimenting with this and writing some samples/docs.

Merging this according to the comment.

mtimmerm · 2022-07-13T18:45:23Z

Thanks @dblock and @VachaShah . Feel free to contact me if you need anything related to this.

dblock · 2022-07-18T18:15:23Z

I have a working demo in https://github.com/dblock/opensearch-java-client-demo/tree/opensearch-java-2.1.0. Super easy! If you see anything that can be improved in that demo, please speak up, since I'm going to start incorporating some of this into opensearch/aws docs.

mtimmerm · 2022-07-19T20:46:09Z

That looks good to me. Maybe add a commented out config for a credential provider. Most of my use cases require that.

dblock · 2022-07-20T15:08:56Z

That looks good to me. Maybe add a commented out config for a credential provider. Most of my use cases require that.

What does it look like in your usage? Feel free to just PR into my repo, too.

dblock · 2022-08-03T18:47:07Z

Btw, this was released in 2.1.0 🥳

mtimmerm · 2022-08-03T18:53:16Z

@dblock That's awesome! Thanks for the note. I will be so happy to remove the bespoke version from my product.

mtimmerm requested review from a team and madhusudhankonda as code owners July 7, 2022 19:18

mtimmerm mentioned this pull request Jul 7, 2022

[FEATURE] Add IAM authentication option / support for SigV4 Signer #55

Closed

mtimmerm changed the title ~~[DRAFT] AwsV2OpenSearchTransport with tests~~ AwsV2OpenSearchTransport with tests Jul 8, 2022

dblock requested changes Jul 8, 2022

View reviewed changes

VachaShah reviewed Jul 8, 2022

View reviewed changes

java-client/src/test/java/org/opensearch/client/opensearch/integTest/awsv2/AwsV2SearchTest.java Outdated Show resolved Hide resolved

java-client/src/main/java/org/opensearch/client/transport/aws/AwsV2OpenSearchTransport.java Outdated Show resolved Hide resolved

mtimmerm changed the title ~~AwsV2OpenSearchTransport with tests~~ [DRAFT] AwsV2OpenSearchTransport with tests Jul 9, 2022

mtimmerm changed the title ~~[DRAFT] AwsV2OpenSearchTransport with tests~~ AwsV2OpenSearchTransport with tests Jul 10, 2022

mtimmerm changed the title ~~AwsV2OpenSearchTransport with tests~~ AwsSdk2Transport implementation Jul 10, 2022

mtimmerm force-pushed the awsv2transport branch from fe825c4 to 7ba29d1 Compare July 10, 2022 15:17

dblock requested changes Jul 11, 2022

View reviewed changes

dblock mentioned this pull request Jul 11, 2022

Only add SPDX license to new files. #178

Merged

mtimmermansTa added 4 commits July 12, 2022 16:17

Merge branch 'main' into awsv2transport

e0e6204

Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

Fix license headers on new files

cc8e635

Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

New AWS sdk release isn't all in the cache yet

e80ed3a

Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

Fix license headers in new file

9b17175

Signed-off-by: Matt Timmermans <mtimmermans@tripadvisor.com>

mtimmerm force-pushed the awsv2transport branch from 9e78139 to 9b17175 Compare July 12, 2022 20:47

mtimmerm requested a review from dblock July 12, 2022 20:57

dblock approved these changes Jul 12, 2022

View reviewed changes

dblock requested a review from VachaShah July 12, 2022 21:25

VachaShah approved these changes Jul 13, 2022

View reviewed changes

VachaShah merged commit 2336053 into opensearch-project:main Jul 13, 2022

dblock mentioned this pull request Jul 14, 2022

Updated Code samples to AWS SDK 2.x awsdocs/amazon-opensearch-service-developer-guide#63

Merged

dblock mentioned this pull request Jul 18, 2022

[META] Make it easy to consume next versions of clients opensearch-project/opensearch-clients#29

Open

1 task

This was referenced Jul 20, 2022

[PROPOSAL] Modularize/remove/make optional the dependency on opensearch-rest-client #182

Closed

[PROPOSAL] Release v2.1.0 #174

Closed

[DOC] AWS Sigv4 Support for Clients opensearch-project/documentation-website#814

Closed

mtimmerm deleted the awsv2transport branch August 3, 2022 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AwsSdk2Transport implementation #177

AwsSdk2Transport implementation #177

mtimmerm commented Jul 7, 2022 •

edited

Loading

dblock left a comment

reta commented Jul 8, 2022 •

edited

Loading

VachaShah left a comment

mtimmerm commented Jul 9, 2022

mtimmerm commented Jul 9, 2022

dblock commented Jul 9, 2022 •

edited

Loading

reta commented Jul 11, 2022 •

edited

Loading

dblock commented Jul 11, 2022

mtimmerm commented Jul 11, 2022

dblock left a comment

dblock Jul 11, 2022

dblock Jul 11, 2022

mtimmerm commented Jul 11, 2022

dblock commented Jul 11, 2022 •

edited

Loading

mtimmerm commented Jul 11, 2022

dblock commented Jul 12, 2022

dblock left a comment

dblock Jul 12, 2022

VachaShah left a comment

VachaShah commented Jul 13, 2022

mtimmerm commented Jul 13, 2022

dblock commented Jul 18, 2022 •

edited

Loading

mtimmerm commented Jul 19, 2022

dblock commented Jul 20, 2022

dblock commented Aug 3, 2022

mtimmerm commented Aug 3, 2022

AwsSdk2Transport implementation #177

AwsSdk2Transport implementation #177

Conversation

mtimmerm commented Jul 7, 2022 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

reta commented Jul 8, 2022 • edited Loading

VachaShah left a comment

Choose a reason for hiding this comment

mtimmerm commented Jul 9, 2022

mtimmerm commented Jul 9, 2022

dblock commented Jul 9, 2022 • edited Loading

reta commented Jul 11, 2022 • edited Loading

dblock commented Jul 11, 2022

mtimmerm commented Jul 11, 2022

dblock left a comment

Choose a reason for hiding this comment

dblock Jul 11, 2022

Choose a reason for hiding this comment

dblock Jul 11, 2022

Choose a reason for hiding this comment

mtimmerm commented Jul 11, 2022

dblock commented Jul 11, 2022 • edited Loading

mtimmerm commented Jul 11, 2022

dblock commented Jul 12, 2022

dblock left a comment

Choose a reason for hiding this comment

dblock Jul 12, 2022

Choose a reason for hiding this comment

VachaShah left a comment

Choose a reason for hiding this comment

VachaShah commented Jul 13, 2022

mtimmerm commented Jul 13, 2022

dblock commented Jul 18, 2022 • edited Loading

mtimmerm commented Jul 19, 2022

dblock commented Jul 20, 2022

dblock commented Aug 3, 2022

mtimmerm commented Aug 3, 2022

mtimmerm commented Jul 7, 2022 •

edited

Loading

reta commented Jul 8, 2022 •

edited

Loading

dblock commented Jul 9, 2022 •

edited

Loading

reta commented Jul 11, 2022 •

edited

Loading

dblock commented Jul 11, 2022 •

edited

Loading

dblock commented Jul 18, 2022 •

edited

Loading