Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport #978

Merged
merged 2 commits into from
May 14, 2024

Conversation

reta
Copy link
Collaborator

@reta reta commented May 9, 2024

Description

Generic HTTP Actions in Java Client does not work with AwsSdk2Transport

Issues Resolved

Closes #969

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@reta reta force-pushed the issue-969 branch 2 times, most recently from 4372193 to 7f51f12 Compare May 9, 2024 20:50
@BrendonFaleiro
Copy link
Contributor

BrendonFaleiro commented May 10, 2024

Hey @reta . I had something similar as well. But I am stuck on the request signing bit. With the standard client requests (SearchRequest and so on), the request parameter of prepareRequestBody is just the body of the request. However with generic requests, the request sent to prepareRequestBody has all the fields and the Body is an Optional.

With this difference, buffer.addContent(request) doesn't just get the body content, but gets what would otherwise be in the endpoint.

And then the request signing gets messed up resulting in:

ResponseCode: 403, ErrorType: security_exception, ErrorReason: The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.
The Canonical String for this request should have been
'GET
/index-name/_search
cancel_after_time_interval=30s
content-length:
content-type:application/json
host:search-ps-os-poc-rgs2zxfej6ggjpsc6fadtijgca.us-west-2.es.amazonaws.com
x-amz-content-sha256:{some-sha-256}
x-amz-date:20240510T014712Z
x-amz-security-token:{someToken}
content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-security-token
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'
The String-to-Sign should have been
'AWS4-HMAC-SHA256
20240510T014712Z
20240510/us-west-2/es/aws4_request
eec266967bc7058668f377cceb217586b148bbfbd356ddeeb916657f05f3d2fd'

In the case of generic requests, we'd have to pull out the body and pass just the body to buffer.addContent(request).

I am not sure how to get that done in a way that it is de/serialized properly.

I thought I'd let you know in case you hit the same errors.

@reta
Copy link
Collaborator Author

reta commented May 10, 2024

Hey @reta . I had something similar as well. But I am stuck on the request signing bit.

Thanks a lot for heads up @BrendonFaleiro , one of the reasons I keep it as a draft is to verify e2e works (waiting for test AWS account to be provisioned), thanks again!

@BrendonFaleiro
Copy link
Contributor

Oh.. and one more. Before passing the bodyStream to the responseDeserializer, you will have to make a copy of the bodyStream into a new InputStream. The stream is only read from when the user gets the result, but we close the stream in the finally block of executeSync. So when the user tries to access the body, they will get
java.io.IOException: Attempted read on closed stream.

In the ApacheHttpClient5Transport and RestTransport, this is achieved by copying the Entity with the stream entity = new BufferedHttpEntity(entity).

@reta
Copy link
Collaborator Author

reta commented May 10, 2024

Oh.. and one more. Before passing the bodyStream to the responseDeserializer, you will have to make a copy of the bodyStream into a new InputStream.

That should fixed already, the stream is read fully now and kept as byte[] (for exactly the same reasons), thanks

@reta reta force-pushed the issue-969 branch 2 times, most recently from b50d185 to d79be37 Compare May 13, 2024 20:09
@dblock
Copy link
Member

dblock commented May 13, 2024

Thanks a lot for heads up @BrendonFaleiro , one of the reasons I keep it as a draft is to verify e2e works (waiting for test AWS account to be provisioned), thanks again!

@VachaShah or I can test this quickly if you want, PR using this code into https://github.com/dblock/opensearch-java-client-demo (there are a few branches)

@reta
Copy link
Collaborator Author

reta commented May 13, 2024

@VachaShah or I can test this quickly if you want, PR using this code into https://github.com/dblock/opensearch-java-client-demo (there are a few branches)

That would be great @dblock , I very much stuck on the way to test it e2e due to complexity of crosssacounts, MFA and assumed roles combinations (this is the way I have access to it), not able to overcome HTTP/403 yet.

@dblock
Copy link
Member

dblock commented May 14, 2024

#969

Worked like a charm, built with this change, then used code in https://github.com/dblock/opensearch-java-client-demo/tree/generic-next.

export AWS_...
export ENDPOINT=...
export AWS_REGION=us-west-2
$ mvn compile exec:java   -Dexec.mainClass="Generic"   -Dlog4j.configurationFile=target/log4j2.xml   -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.SimpleLog   -Dorg.apache.commons.logging.simplelog.log.org.apache.http.wire=INFO
Picked up JAVA_TOOL_OPTIONS: -Dlog4j2.formatMsgNoLookups=true
[INFO] Scanning for projects...
[INFO] 
[INFO] ---------------------< org.openearch:java-client >----------------------
[INFO] Building java-client 1.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- resources:3.3.1:resources (default-resources) @ java-client ---
[WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
[INFO] Copying 1 resource from src/main/resources to target
[INFO] 
[INFO] --- compiler:3.10.1:compile (default-compile) @ java-client ---
[INFO] Nothing to compile - all classes are up to date
[INFO] 
[INFO] --- exec:3.1.0:java (default-cli) @ java-client ---
{
  "name" : "fcecc570a4b1e0527475991424f81e8d",
  "cluster_name" : "013306242651:dblock-test-opensearch-27",
  "cluster_uuid" : "CDx1PKXnQnuym3oTrEPglA",
  "version" : {
    "distribution" : "opensearch",
    "number" : "2.7.0",
    "build_type" : "tar",
    "build_hash" : "unknown",
    "build_date" : "2023-06-28T07:07:20.444435752Z",
    "build_snapshot" : false,
    "lucene_version" : "9.5.0",
    "minimum_wire_compatibility_version" : "7.10.0",
    "minimum_index_compatibility_version" : "7.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

{"_index":"movies","_id":"1","_version":1,"result":"created","_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1}
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  7.221 s
[INFO] Finished at: 2024-05-14T08:01:24-04:00
[INFO] ------------------------------------------------------------------------

CHANGELOG.md Outdated
@@ -40,6 +40,7 @@ This section is for maintaining a changelog for all breaking changes for the cli

### Fixed
- Fix the deserialization of SortOptions ([#981](https://github.com/opensearch-project/opensearch-java/pull/981))
- [BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport ([#978](https://github.com/opensearch-project/opensearch-java/pull/978))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a feature I think. I would say something like "Add support for AwsSdk2Transport to generic client" and move it to features.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no, I consider it as a bug to be fair because the Generic HTTP Actions implementation completely missed this transport (but covered RestClient and AHC5). This is a core transport that we committed to support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's not something that we implemented but broke, it's something that was just not implemented. If you feel strongly about this, let's remove the word "[BUG]", it's already in the fixed section.

@reta
Copy link
Collaborator Author

reta commented May 14, 2024

Lint check fails, https://www.javadoc.io/doc/org.opensearch.client/opensearch-java/latest/index.html](https://www.javadoc.io/doc/org.opensearch.client/opensearch-java/latest/index.html is not available at the moment ...

Signed-off-by: Andriy Redko <[email protected]>
@dblock dblock merged commit b8e0dad into opensearch-project:main May 14, 2024
53 of 54 checks passed
@dblock dblock added the backport 2.x Backport to 2.x branch label May 14, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 14, 2024
…ransport (#978)

* [BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport

Signed-off-by: Andriy Redko <[email protected]>

* Address code review comments

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b8e0dad)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request May 14, 2024
…ransport (#978)

* [BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport

Signed-off-by: Andriy Redko <[email protected]>

* Address code review comments

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b8e0dad)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Andriy Redko <[email protected]>
reta pushed a commit that referenced this pull request May 14, 2024
…ransport (#978) (#984)

* [BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport



* Address code review comments



---------


(cherry picked from commit b8e0dad)

Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Generic HTTP Actions in Java Client does not work with AwsSdk2Transport
3 participants