-
Notifications
You must be signed in to change notification settings - Fork 129
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
[Feature/multi_tenancy] Fix SdkClient credential handling for AWS endpoints #2934
[Feature/multi_tenancy] Fix SdkClient credential handling for AWS endpoints #2934
Conversation
plugin/src/main/java/org/opensearch/ml/sdkclient/SdkClientFactory.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/org/opensearch/ml/sdkclient/SdkClientFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Daniel Widdis <[email protected]>
3f89857
to
ae8a4fa
Compare
if (Strings.isBlank(remoteMetadataEndpoint) || Strings.isBlank(region)) { | ||
throw new OpenSearchException(clientType + " client requires a metadata endpoint and region."); | ||
} | ||
if (!"es".equals(serviceName) && !"aoss".equals(serviceName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if an opensource community wants to use some other service other than es
or aoss
? I think this part needs to be generic? Isn't that the whole point we moved this service name to a settings REMOTE_METADATA_SERVICE_NAME_KEY
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are specific to case SdkClientSettings.AWS_OPENSEARCH_SERVICE:
and case SdkClientSettings.AWS_DYNAMO_DB
for the remoteMetadata
type. Any other service or any other provider can be added to that configuration.
These two particular clients use the AWS SDK for connection and "es" and "aoss" are the only documented options right now, see https://opensearch.org/docs/latest/clients/java/#connecting-to-amazon-opensearch-service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if an opensource community wants to use some other service other than
es
oraoss
? I think this part needs to be generic? Isn't that the whole point we moved this service name to a settingsREMOTE_METADATA_SERVICE_NAME_KEY
?
For a concrete example, let's say OCI wants to have a client connect to their service through the OCI OpenSearch SDK. They would:
- Define a new
SdkClientSettings.REMOTE_METADATA_TYPE
. We already have "RemoteOpenSearch" (a cluster anywhere) and "AWSOpenSearchService" (AOS). They'd add a new "OCISearchService" (or whatever name) - Instead of calling validation via
validateAWSParams()
which requires an endpoint, region, and one of two service names, they'd create a newvalidateOCIParams()
checker method for whichever settings they needed to support methods on https://github.com/oracle/oci-java-sdk. They could probably use the Service Name Key setting for "OPENSEARCHCLUSTER" if relevant (I'm guessing here, see here). So the setting itself is open-ended, it's only in combination with the metadata type that it gains any restrictions on what it can be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for detailed explanation. Makes sense, sounds good to me.
ae8a4fa
to
a2a3d19
Compare
Signed-off-by: Daniel Widdis <[email protected]>
a2a3d19
to
cdfa8eb
Compare
Signed-off-by: Daniel Widdis <[email protected]>
aefb745
to
f2abd4e
Compare
f5345da
into
opensearch-project:feature/multi_tenancy
Description
Refactors SdkClient code as required to support (working) DDB/AOSS integration tests
Check List
--signoff
.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.