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

Added support for Amazon OpenSearch Serverless. #96

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

dblock
Copy link
Member

@dblock dblock commented Dec 31, 2022

Signed-off-by: dblock dblock@amazon.com

Description

  • Lets you configure the service name, defaults to es (Managed OpenSearch).
  • Always adds a x-amz-content-sha256 header as per doc. Seems to work against AWS Managed OpenSearch too.
     let transport = TransportBuilder::new(conn_pool)
         .auth(aws_config.clone().try_into()?)
         .service_name("aoss")
         .build()?;

@Xtansia does this look right? I am a rust noob, so no idea if I got the String vs. str types right here and whether service_name is on the right object or whether we can do better. Please tell me what to do + it needs tests & docs.

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.

@codecov-commenter
Copy link

codecov-commenter commented Jan 1, 2023

Codecov Report

Merging #96 (b389eb7) into main (6ab6462) will increase coverage by 4.61%.
The diff coverage is 72.72%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   42.28%   46.90%   +4.61%     
==========================================
  Files          50       61      +11     
  Lines       28085    29075     +990     
==========================================
+ Hits        11877    13638    +1761     
+ Misses      16208    15437     -771     
Flag Coverage Δ
integration 42.29% <100.00%> (+<0.01%) ⬆️
unit 7.19% <72.72%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
opensearch/examples/cat_indices.rs 0.00% <0.00%> (ø)
opensearch/src/lib.rs 97.72% <ø> (+97.72%) ⬆️
opensearch/tests/auth.rs 100.00% <ø> (ø)
opensearch/tests/cert.rs 100.00% <ø> (ø)
opensearch/tests/error.rs 100.00% <ø> (ø)
opensearch/src/http/transport.rs 64.09% <100.00%> (+14.29%) ⬆️
opensearch/tests/aws_auth.rs 100.00% <100.00%> (ø)
opensearch/tests/common/client.rs 33.33% <100.00%> (ø)
opensearch/examples/aws_auth.rs 0.00% <0.00%> (ø)
api_generator/src/lib.rs 100.00% <0.00%> (ø)
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Xtansia
Copy link
Collaborator

Xtansia commented Jan 3, 2023

@dblock This looks good to me. I think where you have service_name is the safest, the only potentially more ideal would be to expand the Credentials::AwsSigV4 variant to take some kind of struct rather than being a flat tuple, which would improve backwards compatibility in the case of adding more settings/fields specific to SigV4, but at the cost of breaking API compatibility now.
Something like:

let creds = AwsSigV4Credentials::from(sdk_config).service_name("es");
transport_builder.credentials(Credentials::AwsSigV4(creds))

@dblock dblock force-pushed the opensearch-serverless branch 6 times, most recently from 073874d to d088be4 Compare February 1, 2023 18:40
@dblock dblock marked this pull request as ready for review February 1, 2023 18:40
@dblock
Copy link
Member Author

dblock commented Feb 1, 2023

@dblock This looks good to me. I think where you have service_name is the safest, the only potentially more ideal would be to expand the Credentials::AwsSigV4 variant to take some kind of struct rather than being a flat tuple, which would improve backwards compatibility in the case of adding more settings/fields specific to SigV4, but at the cost of breaking API compatibility now. Something like:

let creds = AwsSigV4Credentials::from(sdk_config).service_name("es");
transport_builder.credentials(Credentials::AwsSigV4(creds))

I think you're saying that service name would be part of credentials, but I think it belongs in the client, hence in the AWS transport. Most of clients know what their service signing name is, but in our case we need to switch between es and aoss. WDYT?

@dblock
Copy link
Member Author

dblock commented Feb 1, 2023

Updated PR on top of #114 and #112, with tests. This is ready to review (but look at the last commit).

Working sample in dblock/opensearch-rust-client-demo@33a7826.

Signed-off-by: dblock <dblock@amazon.com>
@Xtansia
Copy link
Collaborator

Xtansia commented Feb 1, 2023

@dblock This looks good to me. I think where you have service_name is the safest, the only potentially more ideal would be to expand the Credentials::AwsSigV4 variant to take some kind of struct rather than being a flat tuple, which would improve backwards compatibility in the case of adding more settings/fields specific to SigV4, but at the cost of breaking API compatibility now. Something like:

let creds = AwsSigV4Credentials::from(sdk_config).service_name("es");
transport_builder.credentials(Credentials::AwsSigV4(creds))

I think you're saying that service name would be part of credentials, but I think it belongs in the client, hence in the AWS transport. Most of clients know what their service signing name is, but in our case we need to switch between es and aoss. WDYT?

My thinking was that service name is purely a signing parameter of AwsSigV4. Quite possibly Credentials should really be named Authentication.
I'd also thought about changing the whole thing to a middleware type pattern rather than an ever growing enum of auth methods + match/if statements. But async functions on traits in Rust currently doesn't have a very pleasant developer experience. So I may have a draft PR for that at some point.

That said I agree with your approach here as the most pragmatic and least friction.

@dblock
Copy link
Member Author

dblock commented Feb 1, 2023

@Xtansia Maybe open an issue to refactor this as you think makes more sense?

@dblock dblock merged commit a398d94 into opensearch-project:main Feb 1, 2023
@dblock dblock deleted the opensearch-serverless branch February 1, 2023 23:59
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.

3 participants