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

[PROPOSAL] Release v. Next #48

Open
dblock opened this issue Feb 23, 2025 · 6 comments
Open

[PROPOSAL] Release v. Next #48

dblock opened this issue Feb 23, 2025 · 6 comments
Assignees

Comments

@dblock
Copy link
Member

dblock commented Feb 23, 2025

What/Why

What are you proposing?

Release v. next before merging #47.

What users have asked for this feature?

Fixes #46.

@dblock
Copy link
Member Author

dblock commented Feb 23, 2025

cc: @issei-m @nhtruong

@issei-m
Copy link
Contributor

issei-m commented Feb 23, 2025

Just copy-pasting this my comment:


My initial idea was quite similar, and what I was actually trying to do was something like this:

signer = Aws::Sigv4::Signer.new(...)

transport = OpenSearch::Transport::HTTP::Faraday.new(
  url: 'http://localhost:9200',
  options: {
    headers: { 'Content-Type' => 'application/json' },
    transport_options: {
      ssl: { verify: false }
    }
  }
)

sigv4_signing_transport_decorator = OpenSearch::Aws::Transport::Transport::Sigv4SigningTransportDecorator.new(transport, signer)

client = OpenSearch::Client.new({ transport: sigv4_signing_transport_decorator })

This decorator simply receives perform_request and delegates the request to transport using the signed headers. In fact, this approach looks clean. However, the decorated OpenSearch::Transport::Transport::Base actually has many public methods, and I'm a bit concerned that if a decorator is created too easily, it might not behave as intended by the base library. In reality, it doesn't seem like perform_request is being called from a higher-level context, but it might be worth reviewing this a bit more.

@issei-m
Copy link
Contributor

issei-m commented Feb 24, 2025

@dblock I briefly read through the source code of opensearch-ruby. As is already known, it has a three-layer structure:

OpenSearch::Client -> OpenSearch::Transport::Client -> OpenSearch::Transport::Transport::Base (default: HTTP::Faraday)

The final layer, OpenSearch::Transport::Transport::Base, is the lowest level and is responsible for actually communicating with OpenSearch nodes. This layer is quite abstract and not highly specific to OpenSearch itself. Besides, as you mentioned, it involves slightly complex tasks like managing multiple connections to nodes, so it's better not to modify this layer.

This means that the second layer, OpenSearch::Transport::Client, is where we have the opportunity to make changes.

My previous PR solves this in a somewhat forceful way, but how about extending it in the following manner?

client = OpenSearch::Client.new(...)

signer = Aws::Sigv4::Signer.new(...)
OpenSearch::AWS::Sigv4::enable_sigv4_to_client(client, signer)

What we're doing here is simple: just extending the instance method client.transport#perform_request and embedding the SigV4 signature.

@nhtruong nhtruong self-assigned this Feb 24, 2025
@nhtruong
Copy link
Collaborator

I think @issei-m's solution makes sense (explained here), especially when our immediate goal is making this gem work with opensearch-ruby 4 like it did with previously version of opensearch-ruby

@nhtruong
Copy link
Collaborator

@dblock the release workflow is being blocked by this PR.

@nhtruong
Copy link
Collaborator

@issei-m this is being blocked by opensearch-project/opensearch-build#5417

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

No branches or pull requests

3 participants