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

Inherit AwsSigV4 in .child #725

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

bandinib-amzn
Copy link
Member

@bandinib-amzn bandinib-amzn commented Feb 29, 2024

Description

There are some use cases where you may need multiple instances of the client. You can easily do that by calling new Client() as many times as you need, but you will lose all the benefits of using one single client, such as the long living connections and the connection pool handling.

As of today, we have client.child API, which returns a new client instance that shares the connection pool with the parent client. But this only supported for Basic Authentication type (with username and password). This PR adds the child client support for AWSSigv4.

Example:

const client = new Client({
  ...AwsSigv4Signer({
    region: 'us-west-2',
    service: 'es',
    getCredentials: () =>
      new Promise((resolve, reject) => {
        AWS.config.getCredentials((err, credentials) => {
          if (err) {
            reject(err);
          } else {
            resolve(credentials);
          }
        });
      }),
  }),
  node: 'https://search-xxx.region.es.amazonaws.com', // OpenSearch domain URL
});

const child = client.child({
  auth: {
        credentials: {
          accessKeyId: 'foo',
          secretAccessKey: 'bar',
          sessionToken: 'foobar',
        },
        region: 'eu-west-1',
        service: 'es',
      }
});

Issues Resolved

#696

Check List

  • New functionality includes testing.
    • All tests pass
  • Linter check was successfull - yarn run lint doesn't show any errors
  • Commits are signed per the DCO using --signoff
  • Changelog was updated.

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.

@seraphjiang
Copy link
Member

Thanks @bandinib-amzn would you explain what's usage for this enhancement. e.g. support what feature in OSD. and targeting which version of OSD release.

@ananzh @kavilla @dblock and team would you help to take a look.

@seraphjiang
Copy link
Member

@Flyingliuhub @BionIT @ZilongX @zengyan-amazon would you also help to take a look.

lib/aws/AwsSigv4Signer.js Outdated Show resolved Hide resolved
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

LGTM, just needs that console.log removed

CHANGELOG.md Outdated Show resolved Hide resolved
.catch(t.fail);

child.on('request', (err, { meta }) => {
debug('Count', count);
Copy link
Member

Choose a reason for hiding this comment

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

getCredentialsCalled and count are used similarly to track the count. Should we use a consistent name, or should we make count more meaningful name here?

Copy link
Member Author

Choose a reason for hiding this comment

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

getCredentialsCalled is tracking how many times getCredentials function called. and count is tracking requests. I would keep count to be consistent with other UT.

Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
@bandinib-amzn bandinib-amzn force-pushed the awssigv4-child-client branch from fbd1ee6 to 49971dc Compare March 5, 2024 17:56
@bandinib-amzn
Copy link
Member Author

bandinib-amzn commented Mar 5, 2024

Thanks @bandinib-amzn would you explain what's usage for this enhancement. e.g. support what feature in OSD. and targeting which version of OSD release.

@ananzh @kavilla @dblock and team would you help to take a look.

We already have the client.child API, which returns a new client instance that shares the connection pool with the parent client. However, this feature is currently only supported for Basic Authentication (with username and password). This pull request incorporates the AwsSigV4 functionality into .child. Utilizing connection pooling offers numerous benefits, including connection reuse and recycling.

In OpenSearch-Dashboards, when utilizing the multi-data source feature to create data sources, we employ the opensearch-js library to instantiate the client. Currently, we leverage .child for basic authentication, as demonstrated here.

However, similar support for AWSSigV4 has not been implemented, necessitating the creation of a new client every time. With this modification, we can generate multiple child clients from the same parent client (on the same node and SSL) by passing credentials via auth.

We are targeting OSD 2.13 release. @dblock How is the release schedule look like for opensearch-js? Thanks!

@bandinib-amzn bandinib-amzn changed the title Add child client support for AwsSigV4 Inherit AwsSigV4 in .child Mar 5, 2024
Signed-off-by: Bandini Bhopi <bandinib@amazon.com>
@ananzh ananzh merged commit 8966691 into opensearch-project:main Mar 5, 2024
64 checks passed
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.

7 participants