apply multi-tenancy and sdk client in Connector (Create + Get + Delete) #3382

dhrubo-os · 2025-01-11T21:18:13Z

Description

[apply multi-tenancy and sdk client in Connector (Create + Get + Delete)]

By default, this is single tenant. If we enable multi-tenancy settings, then the request will expect an header x-tenant-id. And one tenant won't be able to access other tenant's resource.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dbwiddis

Generally LGTM, with some comments/questions.

dbwiddis · 2025-01-12T21:25:14Z

client/src/main/java/org/opensearch/ml/client/MachineLearningNodeClient.java

-        client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, ActionListener.wrap(deleteResponse -> {
-            listener.onResponse(deleteResponse);
-        }, listener::onFailure));
+        client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, ActionListener.wrap(listener::onResponse, listener::onFailure));


I'm missing the need to wrap the action listener here without doing anything else with it.

(Same comment for multiple of these.)

Sorry I didn't clearly follow you here.

if you are not writing custom OnResponse or onFailure rule, you can do similar

client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, listener);

^^^ what @mingshl said. No need to wrap it, it adds no value.

Well I haven't changed much here. Just added minor changes of code refactoring what intellij editor suggested :D. So I'll keep that way for now. If we need to change, I'll raise another PR only targeting to remove the wrapper.

dbwiddis · 2025-01-12T21:28:14Z

client/src/main/java/org/opensearch/ml/client/MachineLearningClient.java

+        deleteConnector(connectorId, tenantId, actionFuture);
+        return actionFuture;
+    }
+
    void deleteConnector(String connectorId, ActionListener<DeleteResponse> listener);


Do we want this one to delegate to the other one with a default (null) tenant ID?

dbwiddis · 2025-01-12T21:30:35Z

common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java

+        if (streamInputVersion.onOrAfter(VERSION_2_19_0)) {
+            this.tenantId = input.readOptionalString();
+        }


Does this work with BWC tests on 3.x branch? I seem to remember having to commit with CURRENT and wait until 2.x backport to make this change and once it's on 2.19.0-SNAPSHOT changing it to this on main.

Yeah I think it works. Main vs 2.x

Yeah it needs to eventually be that way.

The issue is for BWC tests on this PR; if this change isn't merged to 2.x yet, it won't try to send/read that optional string on 2.19.0-SNAPSHOT, but a 3.0.0 node will detect that the stream is 2.19.0 and try to send/read. So rolling BWC will fail until it's backported.

dbwiddis · 2025-01-12T21:32:15Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLConnectorDeleteRequest.java

-    @Getter
-    String connectorId;
+    private final String connectorId;
+    private final String tenantId;


making this final creates a lot of extra noise elsewhere.

Could you please clarify in more detail what kind of noise you are referring to?

Having to do the else block below, for example.

dbwiddis · 2025-01-12T21:33:51Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLConnectorDeleteRequest.java

+            this.tenantId = input.readOptionalString();
+        } else {
+            this.tenantId = null;
+        }


If you choose to keep tenantId final this may be more readable with a ternary operator

jngz-es · 2025-01-13T18:25:39Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/MultiTenantPredictable.java

+     * @param encryptor encryptor
+     * @param tenantId tenantId
+     */
+    void initModel(MLModel model, Map<String, Object> params, Encryptor encryptor, String tenantId);


Not sure why we need this new dedicated interface only for multi-tenant. Could we just include the tenantId as a parameter in the existing params Map?

Good call out. Yeah Initially I wanted to have a separate interface for multi-tenancy but then I ended up having tenantId in the Encryptor class. I will remove this interface class in the next follow up PR if that's ok.

Sure, I am OK.

mingshl · 2025-01-13T19:07:13Z

client/src/main/java/org/opensearch/ml/client/MachineLearningNodeClient.java

-        client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, ActionListener.wrap(deleteResponse -> {
-            listener.onResponse(deleteResponse);
-        }, listener::onFailure));
+        client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, ActionListener.wrap(listener::onResponse, listener::onFailure));


if you are not writing custom OnResponse or onFailure rule, you can do similar

client.execute(MLModelDeleteAction.INSTANCE, mlModelDeleteRequest, listener);

mingshl · 2025-01-13T19:07:33Z

client/src/main/java/org/opensearch/ml/client/MachineLearningNodeClient.java

-                searchRequest,
-                ActionListener.wrap(searchResponse -> { listener.onResponse(searchResponse); }, listener::onFailure)
-            );
+        client.execute(MLModelSearchAction.INSTANCE, searchRequest, ActionListener.wrap(listener::onResponse, listener::onFailure));


same here, if you are not writing custom OnResponse or onFailure rule, you can do similar

client.execute(MLModelSearchAction.INSTANCE, searchRequest, listener);

same comment here.

client/src/main/java/org/opensearch/ml/client/MachineLearningNodeClient.java

mingshl · 2025-01-13T19:15:41Z

plugin/src/main/java/org/opensearch/ml/action/connector/DeleteConnectorTransportAction.java

+        try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
+            ActionListener<DeleteResponse> restoringListener = ActionListener.runBefore(actionListener, context::restore);
+            SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
+            sourceBuilder.query(QueryBuilders.matchQuery(MLModel.CONNECTOR_ID_FIELD, connectorId));


will suggest using term query here because when finding connector id, it's case sensitive, and non-tokenized, it's better to use exact match with term.

Well the scope of this PR is to add multi-tenancy related changes. I haven't modified the search functionality. So I'll keep this as it is.

rbhavna · 2025-01-13T20:41:33Z

plugin/src/main/java/org/opensearch/ml/helper/ConnectorAccessControlHelper.java

        } catch (Exception e) {
            log.error("Failed to validate Access for connector:" + connectorId, e);
            listener.onFailure(e);
        }
    }

+    public void validateConnectorAccess(


Will the above method be removed once all APIs are refactored?

rbhavna · 2025-01-13T21:05:39Z

plugin/src/main/java/org/opensearch/ml/action/connector/DeleteConnectorTransportAction.java

-                                            + Arrays.toString(modelIds.toArray(new String[0])),
-                                        RestStatus.CONFLICT
-                                    )
+                                    new OpenSearchStatusException("Failed to parse search response", RestStatus.INTERNAL_SERVER_ERROR)


If the response has this exception message Failed to parse search response, user might be confused as to why search is being invoked in delete.

Do you have any suggestion?

jngz-es · 2025-01-13T20:40:48Z

common/src/main/java/org/opensearch/ml/common/transport/connector/MLConnectorGetResponse.java

+    public Connector getMlConnector() {
+        return mlConnector;
+    }
+


Use @Getter instead of this function?

Sure, will address that in the subsequent PR

jngz-es · 2025-01-13T21:16:44Z

plugin/src/main/java/org/opensearch/ml/helper/ConnectorAccessControlHelper.java

+    ) {
+
+        sdkClient
+            .getDataObjectAsync(getDataObjectRequest, client.threadPool().executor(GENERAL_THREAD_POOL))


It turns out we are using the single general_thread_pool for all create/get/delete actions. The general_thread_pool is for some infrequent usages with small pool size which might cause some performance issues in edge cases.

Do you have any suggestion what should we use here?

I ran into issues on flow framework with too-small threadpools I was repurposing. I plan to create a new threadpool just for these client wrapper calls.

Yeah, agree with Dan on having a new pool with a larger size which would be better.

FixedExecutorBuilder sdkClientThreadPool = new FixedExecutorBuilder( settings, SDK_CLIENT_THREAD_POOL, OpenSearchExecutors.allocatedProcessors(settings) * 4, 10000, ML_THREAD_POOL_PREFIX + SDK_CLIENT_THREAD_POOL, false );

I'm planning to add this in my subsequent PR and to use this for all the sdkClient related operations. Does this sound good?

Yeah, LGTM, thanks.

jngz-es · 2025-01-13T21:20:05Z

plugin/src/main/java/org/opensearch/ml/rest/RestMLDeleteConnectorAction.java

@@ -45,8 +51,8 @@ public List<Route> routes() {
    @Override
    protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
        String connectorId = request.param(PARAMETER_CONNECTOR_ID);
-
-        MLConnectorDeleteRequest mlConnectorDeleteRequest = new MLConnectorDeleteRequest(connectorId);
+        String tenantId = getTenantID(mlFeatureEnabledSetting.isMultiTenancyEnabled(), request);


Can this setting be modified by users?

This won't be a dynamic settings [ref]

Got it, thanks for the ref.

mingshl · 2025-01-13T23:11:51Z

LGTM, please link the following PR to address the remaining comments.

…e) (#3382) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit bcb00d1)

…e) (#3382) (#3385) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit bcb00d1) Co-authored-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os changed the title ~~apply multi-tenancy and sdk client in Connector (Create + Get + Delete)~~ [Draft] apply multi-tenancy and sdk client in Connector (Create + Get + Delete) Jan 11, 2025

dhrubo-os had a problem deploying to ml-commons-cicd-env January 11, 2025 21:18 — with GitHub Actions Failure

dhrubo-os force-pushed the main branch from 5a01bcf to a28b9b6 Compare January 11, 2025 21:48

dhrubo-os had a problem deploying to ml-commons-cicd-env January 11, 2025 21:48 — with GitHub Actions Failure

dhrubo-os force-pushed the main branch from a28b9b6 to e474e2e Compare January 11, 2025 22:05

dhrubo-os had a problem deploying to ml-commons-cicd-env January 11, 2025 22:05 — with GitHub Actions Failure

dhrubo-os force-pushed the main branch from e474e2e to a90bb7f Compare January 12, 2025 06:20

dhrubo-os had a problem deploying to ml-commons-cicd-env January 12, 2025 06:20 — with GitHub Actions Failure

dhrubo-os force-pushed the main branch from a90bb7f to 545513d Compare January 12, 2025 06:33

dhrubo-os had a problem deploying to ml-commons-cicd-env January 12, 2025 06:34 — with GitHub Actions Failure

apply multi-tenancy and sdk client in Connector (Create + Get + Delete)

a795fce

Signed-off-by: Dhrubo Saha <dhrubo@amazon.com>

dhrubo-os force-pushed the main branch from 545513d to a795fce Compare January 12, 2025 11:33

dhrubo-os temporarily deployed to ml-commons-cicd-env January 12, 2025 11:33 — with GitHub Actions Inactive

dhrubo-os had a problem deploying to ml-commons-cicd-env January 12, 2025 11:33 — with GitHub Actions Failure

dhrubo-os temporarily deployed to ml-commons-cicd-env January 12, 2025 16:38 — with GitHub Actions Inactive

dhrubo-os marked this pull request as ready for review January 12, 2025 16:38

dhrubo-os requested review from b4sjoo, mingshl, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt and austintlee as code owners January 12, 2025 16:38

dhrubo-os requested review from HenryL27 and xinyual as code owners January 12, 2025 16:38

dhrubo-os added the backport 2.x label Jan 12, 2025

dhrubo-os temporarily deployed to ml-commons-cicd-env January 12, 2025 17:36 — with GitHub Actions Inactive

dbwiddis reviewed Jan 12, 2025

View reviewed changes

dhrubo-os changed the title ~~[Draft] apply multi-tenancy and sdk client in Connector (Create + Get + Delete)~~ apply multi-tenancy and sdk client in Connector (Create + Get + Delete) Jan 13, 2025

jngz-es reviewed Jan 13, 2025

View reviewed changes

mingshl reviewed Jan 13, 2025

View reviewed changes

rbhavna reviewed Jan 13, 2025

View reviewed changes

jngz-es reviewed Jan 13, 2025

View reviewed changes

jngz-es approved these changes Jan 13, 2025

View reviewed changes

mingshl approved these changes Jan 13, 2025

View reviewed changes

dhrubo-os merged commit bcb00d1 into opensearch-project:main Jan 13, 2025
9 checks passed

dhrubo-os added backport 2.x and removed backport 2.x labels Jan 14, 2025

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 14, 2025

apply multi-tenancy and sdk client in Connector (Create + Get + Delet…

2c27a05

…e) (#3382) Signed-off-by: Dhrubo Saha <dhrubo@amazon.com> (cherry picked from commit bcb00d1)

opensearch-trigger-bot bot mentioned this pull request Jan 14, 2025

[Backport 2.x] apply multi-tenancy and sdk client in Connector (Create + Get + Delete) #3385

Merged

dbwiddis mentioned this pull request Jan 15, 2025

Fix tests to handle ambiguous mlClient method signature opensearch-project/flow-framework#995

Merged

2 tasks

apply multi-tenancy and sdk client in Connector (Create + Get + Delete) #3382

apply multi-tenancy and sdk client in Connector (Create + Get + Delete) #3382

Conversation

dhrubo-os commented Jan 11, 2025 • edited Loading

Description

Related Issues

Check List

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingshl commented Jan 13, 2025

dhrubo-os commented Jan 11, 2025 •

edited

Loading