-
Notifications
You must be signed in to change notification settings - Fork 183
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
Adding x-amz-content-sha256 header for signed requests #339
Changes from 4 commits
bfc1ac1
81f1045
301e2e4
4d917fe
e88486c
eed32d5
57492fc
66c4eb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,7 +297,7 @@ public void close() { | |
} | ||
|
||
@CheckForNull | ||
private <RequestT> OpenSearchRequestBodyBuffer prepareRequestBody( | ||
public <RequestT> OpenSearchRequestBodyBuffer prepareRequestBody( | ||
RequestT request, | ||
Endpoint<RequestT, ?, ?> endpoint, | ||
TransportOptions options | ||
|
@@ -321,7 +321,7 @@ private <RequestT> OpenSearchRequestBodyBuffer prepareRequestBody( | |
return null; | ||
} | ||
|
||
private <RequestT> SdkHttpFullRequest prepareRequest( | ||
public <RequestT> SdkHttpFullRequest prepareRequest( | ||
RequestT request, | ||
Endpoint<RequestT, ?, ?> endpoint, | ||
@CheckForNull TransportOptions options, | ||
|
@@ -363,6 +363,9 @@ private <RequestT> SdkHttpFullRequest prepareRequest( | |
} | ||
req.putHeader("Content-Length", String.valueOf(body.getContentLength())); | ||
req.contentStreamProvider(body::getInputStream); | ||
// To add the "X-Amz-Content-Sha256" header, it needs to set as required. | ||
// It is a required header for Amazon OpenSearch Serverless. | ||
req.putHeader("x-amz-content-sha256", "required"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What a weird API! Took me a while to find https://github.com/aws/aws-sdk-java-v2/blob/0510a17ae41d601cf5f03e7af01e4519a6b3a744/core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java#L97 that handles this. Is there a higher level method we can use like https://github.com/aws/aws-sdk-java-v2/blob/0510a17ae41d601cf5f03e7af01e4519a6b3a744/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4UnsignedPayloadSigner.java#L69 so we don't have to hard-code "required"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it took me a while to find too. We use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it works we sure can I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried the |
||
} | ||
|
||
boolean responseCompression = Optional.ofNullable(options) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those should probably not be public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do that since I had to write a test for the header :( The AwsSdk2Transport returns a parsed response which does not have the headers for me to test. So I had to test it when the request gets signed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to find a way. How about making it protected, subclassing AwsSdk2Transport into a test class and saving headers into it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like this?
where the
prepareRequest
andprepareRequestBody
in AwsSdk2Transport areprotected
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Looking at your implementation I think that was not a good suggestion, sorry. I say we open a bug for this and call it a day instead of adding the child class.