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

Adding support for AWS SDK V2 request signer #116

Merged
merged 1 commit into from Jun 20, 2022

Conversation

matelang
Copy link
Contributor

@matelang matelang commented Jun 9, 2022

Description

This PR introduces support for a new package that allows to use the new AWS SDK V2 to sign the OpenSearch Go client's HTTP request messages according to the AWS Signature spec V4 .

I've also updated all project dependencies to latest versions for bonus maintenance.

Issues Resolved

No issues found in tracker relating to this.

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.

@VijayanB
Copy link
Member

@matelang Thanks for creating PR. Will review your PR this week.

@matelang
Copy link
Contributor Author

Thanks @VijayanB for taking a look and for allowing the Actions to run.

I can see that most tests pass, with the exception of the integration tests on version 2.
https://github.com/opensearch-project/opensearch-go/runs/6865787859

Wondering whether it is something I missed or the test is flaky since it seems to be an HTTP connection issue?

I can try and reproduce it locally as well.

@matelang
Copy link
Contributor Author

matelang commented Jun 14, 2022

For tracking, I've opened an Issue and PR on AWS SDK with the fix from the second commit:
aws/aws-sdk-go-v2#1728
aws/aws-sdk-go-v2#1729

@VachaShah
Copy link
Collaborator

For tracking, I've opened an Issue and PR on AWS SDK with the fix from the second commit: aws/aws-sdk-go-v2#1728 aws/aws-sdk-go-v2#1729

Thank you @matelang! Retrying the CI

@VijayanB
Copy link
Member

According to their documentation:

The v2 SDK requires a minimum version of Go 1.15.

I believe opensearch-go supports from go 1.11. Should we update this as well?

@matelang
Copy link
Contributor Author

matelang commented Jun 15, 2022

According to their documentation:

The v2 SDK requires a minimum version of Go 1.15.

I believe opensearch-go supports from go 1.11. Should we update this as well?

You are right. Sorry, I've missed it. I've added a new commit to address this:
2655d70

@matelang matelang force-pushed the main branch 2 times, most recently from a032cde to f6eea97 Compare June 15, 2022 20:22
@matelang
Copy link
Contributor Author

I've also fetched upstream and adapted the import path of the signer accordingly (appended v2 to import path).

@VachaShah
Copy link
Collaborator

@matelang Looks like one of the commits has a sign off missing. Can you please fix it?

@matelang
Copy link
Contributor Author

matelang commented Jun 16, 2022

@matelang Looks like one of the commits has a sign off missing. Can you please fix it?

@VachaShah I think it was the merge commit, I've fixed it by squashing all the work into just 1 commit. Have not changed anything else.

VachaShah
VachaShah previously approved these changes Jun 16, 2022
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Thank you @matelang for adding this!

@VachaShah VachaShah requested a review from VijayanB June 16, 2022 18:47
VijayanB
VijayanB previously approved these changes Jun 16, 2022
@VijayanB
Copy link
Member

@matelang Thanks for adding v2 support. Unfortunately i don't know way of writing integration test for aws iam feature. Did you get chance to test v2 support manually from opensearch-go?

@matelang
Copy link
Contributor Author

@matelang Thanks for adding v2 support. Unfortunately i don't know way of writing integration test for aws iam feature. Did you get chance to test v2 support manually from opensearch-go?

We already use the vendored version of the signer in production, so far it is working great for us.

* Also raises minimum go version to 1.15
* Updates dependencies

Signed-off-by: Máté Lang <langmatelaszlo@gmail.com>
@matelang
Copy link
Contributor Author

Is there anything else I can do to get this merged in?

@VachaShah
Copy link
Collaborator

Hi @matelang, can you please create an issue for the same and link this PR so it is easier to track? Also, it would be great if we can add this to the USER_GUIDE.

@VachaShah VachaShah merged commit f4a0b24 into opensearch-project:main Jun 20, 2022
@matelang
Copy link
Contributor Author

Hi @matelang, can you please create an issue for the same and link this PR so it is easier to track? Also, it would be great if we can add this to the USER_GUIDE.

Created the issue and linked it here.

I will open another PR later today modifying the User guide accordingly.
Thanks for your help in getting this merged in.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 20, 2022
* Also raises minimum go version to 1.15
* Updates dependencies

Signed-off-by: Máté Lang <langmatelaszlo@gmail.com>
(cherry picked from commit f4a0b24)
@matelang
Copy link
Contributor Author

@VachaShah added example and enhanced docs:
#131

igungor pushed a commit to igungor/opensearch-go that referenced this pull request Jun 28, 2022
* Also raises minimum go version to 1.15
* Updates dependencies

Signed-off-by: Máté Lang <langmatelaszlo@gmail.com>
@dblock
Copy link
Member

dblock commented Jun 28, 2022

@matelang Do you know if this works for chunked gzipped requests? Care to try? Coming from opensearch-project/opensearch-py#176 and acm19/aws-request-signing-apache-interceptor#20

@matelang
Copy link
Contributor Author

@matelang Do you know if this works for chunked gzipped requests? Care to try? Coming from opensearch-project/opensearch-py#176 and acm19/aws-request-signing-apache-interceptor#20

I have not tested it explicitly to be honest. We are using the default configurations for our OpenSearch client connection. I can try to reproduce it sometime time this week.

Thanks for letting me know.

This was referenced Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants