Support opensearch-ruby 4.0

[issei-m]

Description

As while discussions in this PR, we figured out the upcoming opensearch-ruby broken compatibility a bit but makes our library completely being no longer working.

So this PR resolves the issue.

I haven't yet tested on an actual OpenSearch cluster.

Issues Resolved

n/a

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.

[dblock]

First, thanks for picking this up!

So, this replaces the transport in the subclassed client? I think we should not rely on assigning @transport. I'll merge this once we're done with the 3.x line as it makes things work.

As a next step however I think we should revisit this approach. My first idea was to expose transport as a client option in the 4.0 opensearch-ruby and getting rid of Sigv4Client altogether. Users would instantiate a Sigv4Transport.

But IMO this is wrong too. A sigv4 signer is about request signing. I think users want to pass it as an auth: parameter, and the opensearch-ruby library should support basic auth, sigv4, and others this way. WDYT?

[dblock]

Is this actually intended to be missing super?

[issei-m]

Yes. I don't want the superclass to meaninglessly instantiate a hard-coded transport, that will be overwritten soon after.

[dblock]

Makes sense, this "interface" is borked, see my comment above for what I think we should do here. LMK what you think!

[nhtruong]

@dblock does adding sigv4 as an auth option in opensearch-ruby basically move this feature back into opensearch-ruby? I couldn't find the discussion where we decided to move sigv4 out the core library but I remember we extracted sigv4 out of opensearch-ruby because sigv4 is a propriety auth method owned by Amazon, and as an opensource project we do not want any favoritism by including Amazon's sigv4 and its dependencies in opensearch-ruby gem.

[dblock]

does adding sigv4 as an auth option in opensearch-ruby basically move this feature back into opensearch-ruby?

it would but that's not what I am proposing

I am proposing adding auth: [something that can be called or intercept perform_request]. The client doesn't need to know what that something is, just support an interface.

[nhtruong]

Gotcha. I've created a separated issue for that :)

[issei-m]

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.

[dblock]

@issei-m It's quite unusual to subclass a transport for the purposes of signing requests, try to change the implementation to support callbacks that allow for adding headers instead?

[nhtruong]

The solution that @issei-m has here makes sense! It makes this lib work with opensearch-ruby 4 the same way that it did with opensearch-ruby 3:

• The opensearch-ruby prior to 4.x had a wacky way of invoking the transport.perform_request method via the client.perform_request method. Version 4.0 refactored this part of the library and removed the middle-man methods, including client.perform_request (Check the How the perform_request method is invoked section in this PR
• This sigv4 library intercepts client.perform_request (via sub-classing the OpenSearch::Client class to override this method) to inject the sigv4 signature. However, since client.perform_request has been removed in 4.x and all API methods now call the transport.perform_request directly, @issei-m's solution is overriding the transport's method instead, and without introducing breaking changes for the current sigv4 users.

Instead of us asking the user every bit of auth info to perform a sigv4 request, we delegate this to the official Sigv4 gem, as the user has to provide an instance of the Aws::Sigv4::Signer class when instantiating the Sigv4 client. This insulates us from changes in Sigv4 (as it will be handled by the AWS Sigv4 gem), and allows the user to use any version of Sigv4.

[nhtruong]

The decorator approach described here is a bit too complicated to use, IMO. It requires the user to know the implementation details of the transport layer and create an instance of OpenSearch::Transport::HTTP::Faraday

[nhtruong]

This looks good to me. Just need to rebase and update the change log.

I don't think this will introduce breaking changes and we can release this as a minor version? What do yall thing?

[dblock]

I don't think this will introduce breaking changes and we can release this as a minor version? What do yall thing?

I would add CI with opensearch-ruby 3.x and 4.x to be sure.

[issei-m]

@nhtruong Thanks for your review and approval.

You mean we can go with this approach for both opensearch-ruby 3.x and 4.x?

If so, I need to reintroduce the ignore param removal I incorporated in my previous PR as I've removed it while porting the signature-making method to the transport layer in this PR, since I originally thought this would be shipped for only 4.x line.

[nhtruong]

@issei-m the ignore flag has always been a feature of the Ruby client. Its implementation is confusing due to how it's passed to the method, and doing it differently will introduce a breaking change. Since we the next major release is still in beta, we should consider re-implementing this feature. What do you think?

cc @dblock

[issei-m]

@nhtruong IMPO, mixing the client options with the hash that holds the OpenSearch server options is confusing. If breaking changes are allowed, it might be worth considering a different approach (such as nesting them under client_options).

Also, since there is already allow_no_indices, maybe this option itself isn’t necessary? (Errors other than 404 should generally be handled by the client caller.)

[nhtruong]

I don't think allow_no_indices is the same as the ignore flag in the Ruby client. While most APIs return a 200 OK when you try to delete a resource that has already been deleted (i.e. the delete action is idempotent), OpenSeach API returns a 404 instead. ignore: 404 in the Ruby client attempts to simulate this convention while interacting with an OpenSearch cluster.

I'll come up with a PR for OpenSearch Ruby Beta.2 soon that will move this feature into the client_options while instantiating the client, instead of the user passing this flag in every API method call. Then with version 4, Sigv4 and other wrappers wont have to worry about excluding the ignore params anymore.

[issei-m]

@nhtruong Ah, it looks like I misunderstood allow_no_indices.
btw all CI failures have been resolved now. I've kept the ignore param removal since I'm not sure how the beta library will evolve. If it turns out to be unnecessary, please remove it.