Primary setup for Multi-tenancy

[dhrubo-os]

Description

In this PR, I'm just adding the primary setup required for Multi-tenancy feature. After this PR, I'll start raising other PRs for adding multi-tenancy in connector, model, agents etc.

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.

[mingshl]

@dhrubo-os do we have issues to explain this change?

[dhrubo-os]

@dhrubo-os do we have issues to explain this change?

RFC: #2358

another issue: #2650

[brianf-aws]

I didnt get to reviewing the tests but left some comments hope they help :)

[brianf-aws]

Suggested change

	// Whether multi-tenancy is enabled in ML Commons.
	// This is a static setting which must be set before starting OpenSearch by (in priority order):
	// 1. As a command-line argument using the -E flag (overrides other options):
	// ./bin/opensearch -Eplugins.ml_commons.multi_tenancy_enabled=true
	// 2. As a system property using OPENSEARCH_JAVA_OPTS (overrides opensearch.yml):
	// export OPENSEARCH_JAVA_OPTS="-Dplugins.ml_commons.multi_tenancy_enabled=true"
	// ./bin/opensearch
	// Or inline when starting OpenSearch:
	// OPENSEARCH_JAVA_OPTS="-Dplugins.ml_commons.multi_tenancy_enabled=true" ./bin/opensearch
	// 3. In the opensearch.yml configuration file:
	// plugins.ml_commons.multi_tenancy_enabled: true
	// After setting it, a full cluster restart is required for the changes to take effect.
	public static final Setting<Boolean> ML_COMMONS_MULTI_TENANCY_ENABLED = Setting
	.boolSetting("plugins.ml_commons.multi_tenancy_enabled", false, Setting.Property.NodeScope);
	/** This setting sets the remote metadata type */
	public static final Setting<String> REMOTE_METADATA_TYPE = Setting
	.simpleString("plugins.ml_commons." + REMOTE_METADATA_TYPE_KEY, Setting.Property.NodeScope, Setting.Property.Final);
	/**
	* Indicates whether multi-tenancy is enabled in ML Commons.
	*
	* This is a static setting that must be configured before starting OpenSearch. It can be set in the following ways, in priority order:
	*
	* <ol>
	* <li>As a command-line argument using the <code>-E</code> flag (this overrides other options):
	* <pre>
	* ./bin/opensearch -Eplugins.ml_commons.multi_tenancy_enabled=true
	* </pre>
	* </li>
	* <li>As a system property using <code>OPENSEARCH_JAVA_OPTS</code> (this overrides <code>opensearch.yml</code>):
	* <pre>
	* export OPENSEARCH_JAVA_OPTS="-Dplugins.ml_commons.multi_tenancy_enabled=true"
	* ./bin/opensearch
	* </pre>
	* Or inline when starting OpenSearch:
	* <pre>
	* OPENSEARCH_JAVA_OPTS="-Dplugins.ml_commons.multi_tenancy_enabled=true" ./bin/opensearch
	* </pre>
	* </li>
	* <li>In the <code>opensearch.yml</code> configuration file:
	* <pre>
	* plugins.ml_commons.multi_tenancy_enabled: true
	* </pre>
	* </li>
	* </ol>
	*
	* After setting this option, a full cluster restart is required for the changes to take effect.
	*/
	public static final Setting<Boolean> ML_COMMONS_MULTI_TENANCY_ENABLED = Setting
	.boolSetting("plugins.ml_commons.multi_tenancy_enabled", false, Setting.Property.NodeScope);

:)

[dhrubo-os]

Nice!!

[brianf-aws]

This part is a little bit hard to follow, maybe we can extract it? I say this because you state you are going to add more in the future too.

[dhrubo-os]

We are just building the SDKClient here. Similar like this

[brianf-aws]

lets drop the else and just return true

[brianf-aws]

same here

[dhrubo-os]

Same applies here as well. And if wrong tenant id is provided customer shouldn't be able to access the resources.

[brianf-aws]

i see the javadoc states if the tenant id is valid but looks like the tenantId checks if its null or not. Is it possible to pass a invalid tenantId while having multiTenancy Enabled or is this impossible?

[dhrubo-os]

From the plugin level, we don't have visibility if the tenant id is valid or not. We perform this validation from the interceptor level. Here we just check if multi-tenancy is enabled then tenant id needs to be present.

[brianf-aws]

There happens to be a lot of nesting maybe lets refactor to fail early (gaurd clause)?

 public static String getTenantID(Boolean isMultiTenancyEnabled, RestRequest restRequest) {
   if (!isMultiTenancyEnabled)
        return null;
    Map<String, List<String>> headers = restRequest.getHeaders();
  if (headers == null)
     throw new OpenSearchStatusException("Rest request header can't be null", RestStatus.FORBIDDEN);
  if (!headers.containsKey(Constants.TENANT_ID_HEADER))
      throw new OpenSearchStatusException("Tenant ID header is missing", RestStatus.FORBIDDEN);
  List<String> tenantIdList = headers.get(Constants.TENANT_ID_HEADER);
  
   if (tenantIdList == null || tenantIdList.isEmpty()) 
         throw new OpenSearchStatusException("Tenant ID header is missing", RestStatus.FORBIDDEN);
         
  String tenantId = tenantIdList.get(0);
  if (tenantId == null)
       throw new OpenSearchStatusException("Tenant ID can't be null", RestStatus.FORBIDDEN);
               
    return tenantId
}

My indentation is off but the point is to check your edge cases first and let it trickle down to the happy case

[dhrubo-os]

Yeah this is better.

[dbwiddis]

if (headers == null)
throw new OpenSearchStatusException("Rest request header can't be null", RestStatus.FORBIDDEN);

FYI, this case is impossible. I tried to write a unit test for the Flow Framework version of this util and could not (short of using reflection) make the headers null. The only way to make it null is using a constructor, where it passes through a headers.get() to get the content type from the header; and if that didn't stop it, Collections.unmodifiableMap() would throw an NPE on null headers as well.

[brianf-aws]

A little bit confused on this part why is it a list if we only get one item or is this how it just is?

[dhrubo-os]

HTTP headers are represented as a Map<String, List> because the HTTP protocol allows multiple values for a single header key (e.g., Set-Cookie). Even though most headers, like Tenant-ID, typically have only one value, the list structure ensures flexibility and compatibility. In practice, we usually retrieve the first value (list.get(0)) since only one value is needed in most cases.

[dhrubo-os]

Tests with failures:
 - org.opensearch.ml.tools.VisualizationsToolIT.testVisualizationNotFound
 - org.opensearch.ml.tools.VisualizationsToolIT.testVisualizationFound
111 tests completed, 2 failed, 10 skipped

Same flaky errors

[b4sjoo]

Do you know why use a force here? If the version matter then you should be aware that it will cause version conflict when goes to the managed service.

[dhrubo-os]

refer to this: opensearch-project/opensearch-remote-metadata-sdk#25 (comment)

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3307-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 80de6e30b59500c2746ca3cecbca080fbcb0f1e0
# Push it to GitHub
git push --set-upstream origin backport/backport-3307-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3307-to-2.x.