Add identity/authorization headers into Rest Client calls

[kokibas]

Description

Here I create a new Request Options object based on Request Options.DEFAULT using the to Builder() method, add the "Authorization" header with the token, and build a new RequestOptions object using the build() method.

Issues Resolved

#430

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]

Codecov Report

Merging #613 (281e595) into main (97d51f4) will decrease coverage by 0.08%.
The diff coverage is 80.00%.

📣 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     #613      +/-   ##
============================================
- Coverage     63.70%   63.62%   -0.08%     
  Complexity      263      263              
============================================
  Files            52       52              
  Lines          1138     1141       +3     
  Branches         36       36              
============================================
+ Hits            725      726       +1     
- Misses          402      404       +2     
  Partials         11       11              

Impacted Files	Coverage Δ
src/main/java/org/opensearch/sdk/SDKClient.java	90.56% <80.00%> (-1.67%)	⬇️

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

[dbwiddis]

Thanks for this initial PR. It's very helpful that you have shown how to add the headers.

Can we make a setter for the client that updates the options? Something like setOptions(options) which you create this way?

[dbwiddis]

A few general comments here:

1. We need to continue to provide the default option if nothing is passed. Don't hard-code strings.
2. We should just have a single string for a header, and not require the user to pass two strings that we append to each other.
3. We should allow adding the header after instantiation. A setHeader() method would work for this but I think a much better idea is just having a setRequestOptions() method.

[dbwiddis]

Let's simplify this to just having a RequestOptions field.

[dbwiddis]

LGTM, not sure this is the "final solution" but should work for now until we figure out something better!

[owaiskazi19]

@dbwiddis I'm not clear with what does this change actually does? Talks about adding identity/authorization headers but all I see is a setter and the default value replaced?

[dbwiddis]

@dbwiddis I'm not clear with what does this change actually does? Talks about adding identity/authorization headers but all I see is a setter and the default value replaced?

Per the issue being addressed, #430:

SDKClient REST calls need to edit the RequestOptions to add headers.

There may be a need to close/re-initialize the clients or just change the headers, that will need to be determined later.

Such editing was explored earlier in this PR in commits now superceded, such as this one:

public void search(SearchRequest request, ActionListener<SearchResponse> listener) {
    RequestOptions options = RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", "Bearer TOKEN_HERE").build();
    restHighLevelClient.searchAsync(request, options, listener);
}

Of course, that solution hard codes a string as the token, which obviously doesn't work. But it points out the fact that if we want to use client.search(request, listener) we need the ability to pass other request options than the default.

Given the singleton nature of our RestClient, we can accept an incoming RestRequest from OpenSearch, which will (in the future) contain a token that we can use to send back. We can set this token into the SDKRestClient using the setter in this PR.

Is this the final solution? I doubt it. It's not thread safe. There are other client changes needed per #612. This is also just a hack to our (deprecated) client used to aid migration, see also #625.

Per my approval comment:

not sure this is the "final solution" but should work for now until we figure out something better!

If you have a better idea, this would be a good time to post it!

[owaiskazi19]

@dbwiddis this PR handles the Request Options and provides a way to add a new header to the async requets and not use only the DEFAULT one. It doesn't actually adds identity/authorization headers. A little confusing PR title got me thinking in the first place.

[owaiskazi19]

Still few places left to replace RequestOptions.DEFAULT; value with options like this. We should add options for all the clients which have DEFAULT currently.

[dbwiddis]

We should add options for all the clients which have DEFAULT currently.

I'm not so sure we should be adding more stuff to the Java Clients, rather than just documenting how to use them. Do we actually have anything wrapped in the other clients yet?

[owaiskazi19]

I'm not so sure we should be adding more stuff to the Java Clients, rather than just documenting how to use them. Do we actually have anything wrapped in the other clients yet?

That was regards to SDKIndicesClient which @kokibas has already taken care of. Are you saying we should not add options for SDKIndicesClient?

[dbwiddis]

That was regards to SDKIndicesClient which @kokibas has already taken care of. Are you saying we should not add options for SDKIndicesClient?

Ah, sorry, I missed that context. Yes the options should be at the highest level of the Rest Client and copied to the other clients. If we removed the static from these classes, they would have access to the outer class object.