Dynamic tenancy configurations #2607

abhivka7 · 2023-03-30T14:03:47Z

Description

Category : Enhancement
Why these changes are required?
These changes are required to dynamically enable/disable multi-tenancy, private tenant and set a default tenant.
What is the old behavior before changes and new behavior after changes?
Earlier users had to change YAML files in each node and restart node to enable/disable multi-tenancy and private tenant. With our changes, users will be able to change these settings dynamically. We will also give admins the option to set a default tenant for all users for first time log in.
More details can be found here:
[RFC] Dynamically Configurable Tenancy Feature OpenSearch#5853
[FEATURE] Enable/Disable Multi-Tenancy, Private Tenant and set Default Tenant in OS Dashboards. security-dashboards-plugin#1302

Previous PR for the same issue: #2444

Issues Resolved

opensearch-project/security-dashboards-plugin#1302

Is this a backport? If so, please add backport PR # and/or commits #
Nope

Testing

We have done manual testing for all changes on our local server. We have also unit tests for our api changes.

Check List

[ ✅ ] New functionality includes testing
[] New functionality has been documented. : Will document in further commits.
[ ✅ ] Commits are signed per the DCO using --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.

DEVELOPER_GUIDE.md

expani · 2023-03-30T14:23:00Z

src/test/java/org/opensearch/security/multitenancy/test/TenancyDefaultTenantTests.java

+
+        final HttpResponse getSettingResponse = nonSslRestHelper().executeGetRequest("/_plugins/_security/api/tenancy/default_tenant", asAdminUser);
+        assertThat(getSettingResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
+        assertThat(getSettingResponse.findValueInJson("value"), equalTo(""));


nit: Can we read "" from a constant for Global tenant instead.

peternied

Thanks for iterating on this @abhivka7 huge improvement. Many comments on here, but most of these should be fairly straight forward. Let me know if you need help.

Another area I'd like us to shine some visibility on is the permission names and the REST API paths. Can you enumerate those in a comment and walk through them?

From the draft pull request there was a lot of back and forth and those permissions / paths never really gelled, lets make sure we nail these down since we will have to live with them for a long time.

src/main/java/org/opensearch/security/rest/SecurityInfoAction.java

peternied · 2023-03-30T22:47:54Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -525,6 +525,15 @@ public boolean multitenancyEnabled() {
                && dcm.isDashboardsMultitenancyEnabled();
    }

+    public boolean privateTenantEnabled() {
+        return privilegesInterceptor.getClass() != PrivilegesInterceptor.class


Do you have context on what this check? IMO I don't see any reason we need to restrict these results based on PrivilegesInterceptor used to construct this class.

@peternied I don't have much context on why we are using this check. But the function above (multitenancyEnabled) is using the same check that's why I thought of using it.

As long as this is exercised in end to end tests, ok.

src/main/java/org/opensearch/security/action/tenancy/DefaultTenantRestHandler.java

src/main/java/org/opensearch/security/action/tenancy/PrivateTenantEnabledRestHandler.java

src/main/java/org/opensearch/security/action/tenancy/StringSettingUpdateRequest.java

src/test/java/org/opensearch/security/multitenancy/test/TenancyDefaultTenantTests.java

peternied · 2023-03-30T23:39:23Z

src/test/java/org/opensearch/security/multitenancy/test/TenancyDefaultTenantTests.java

+        assertThat(getSettingResponseAfterUpdate.getStatusCode(), equalTo(HttpStatus.SC_OK));
+        assertThat(getSettingResponseAfterUpdate.findValueInJson("value"), equalTo("Private"));
+
+        getAuthinfoResponse = nonSslRestHelper().executeGetRequest("/_plugins/_security/authinfo", asAdminUser);


End to end test should replicate the workflow that is used by dashboards. If we just call the config APIs / confirm we are reading out the same value we put in it only indicates that the config was updated not that the users are seeing the correct behavior.

Same issue for the TenancyPrivateTenantEnabledTests

Those tests will be covered in Security Dashboards plugin PR. Anyways the Dashboards also will be calling this API (DashboardsInfo) as replicated in test to get final config so that should be fine.

We need the end to end coverage of this scenarios in the security plugin. Otherwise breaking changes could be make in this repo.

The example from TenancyMultitenancyEnabledTests.java shows how when the feature is activated behavior changes, please use this as a starting point. Let me know if you need help to test these scenarios.

I have added that for TenancyPrivateTenantEnabledTests. Not sure what else to add for Default tenant tests. What test scenario are you thinking @peternied ?

src/test/java/org/opensearch/security/multitenancy/test/TenancyPrivateTenantEnabledTests.java

expani · 2023-04-01T05:57:23Z

@abhivka7 We are using separate Rest Actions for every configuration being added as part of this PR like default_tenant and private_tenant

Since these are individual properties, the approach will require adding multiple classes for every property added in the future as well. This will in-turn end up increasing the metaspace of OS JVM.

Can we try an approach where separate endpoints/routes are not required for every property ?

peternied · 2023-04-01T14:01:43Z

@anijain-Amazon Thanks for the comments, data consistency was a considerable trade-offs with the previous iteration that used a single API, see the discussion in #2444.

We've got an issue centered around granular access of security config, we would welcome your feedback in this space.

[FEATURE] Create a way to add action oriented APIs to update configuration values #2559

Since these are individual properties, the approach will require adding multiple classes for every property added in the future as well. This will in-turn end up increasing the metaspace of OS JVM.

This is a good consideration. Is this an immediate issue, can you quantify the severity?

I expect all OpenSearch components + plugins + extensions to be impacting the memory usage of OpenSearch as we incorporate more features. I don't see an issue in the OpenSearch repo, could you create one?

expani · 2023-04-01T15:27:52Z

data consistency was a considerable trade-offs with the previous iteration that used a single API, see the discussion in #2444.

I could see the comments in previous iteration are w.r.t. using a new document vs reusing the existing security config document. Didn't find anything mentioning the flaws of using a single API.

Having separate APIs for every property requires the users/clients to take the overhead of ensuring transaction while updating multiple properties OR consider the drawbacks of having partial failures. For instance, if a user wants to update propertyA and propertyB, then either they have to do it individually OR do it in a transaction to ensure consistency.

We've got an issue centered around granular access of security config, we would welcome your feedback in this space.

Ack will take a look into it. This should help bring more flexibility for OpenSearch users to control admin privileges.

This is a good consideration. Is this an immediate issue, can you quantify the severity?

My concern is how will this approach scale with more properties in future. From a quick glance at this PR, it looks like we are adding at least 3 classes per property which can be simplified. Although, this will immediately not affect OpenSearch but will sure eat up the JVM Metaspace in future with say like 30-50 properties requiring 60-150 classes for updating their settings.

expani · 2023-04-01T15:34:58Z

I expect all OpenSearch components + plugins + extensions to be impacting the memory usage of OpenSearch as we incorporate more features. I don't see an issue in the OpenSearch repo, could you create one?

Since the classes are statically referenced, I expect OpenSearch will detect any increase in its JVM Metaspace while running performance tests and adjust it's xmS and xmX accordingly.

prabhat-chaturvedi · 2023-04-03T07:02:53Z

This PR should have v2.7.0 label on this PR? @peternied can you help please?

abhivka7 · 2023-04-03T15:06:31Z

Thanks a lot @anijain-Amazon for bringing this up. After a lot of considerations and POC, I have decided to move to a single API model for cleaner user experience and better metaspace utilisation. I will raise a new revision with that approach. Please take a look and give your feedbacks.
cc: @prabhat-chaturvedi @shikharj05 @devardee @peternied

peternied

Refactoring looks good, notable callouts:

API / permissions naming needs iteration, I've made suggestions, but product team might need to be engaged.
Need end to end verification in the test cases. Cannot depend on test cases in other repositories.
Looks like many test cases are broken. Check the CI results.

peternied · 2023-04-04T18:04:01Z

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyConfigurations.java

+
+package org.opensearch.security.action.tenancy;
+
+public class MultiTenancyConfigurations {


This seems like it can be removed as the object isn't serialized/deserialized directly, replace it with using Request/Response classes with fields.

Going forward we want to contribute more to tenancy in OpenSearch and therefore add new properties. It makes more sense to have an object for same for better accessibility and keeping everything together in a structured form.

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyConfigurationsRestHandler.java

...n/java/org/opensearch/security/action/tenancy/MultiTenancyConfigurationsRetrieveActions.java

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyConfigurationsRestHandler.java

...main/java/org/opensearch/security/action/tenancy/MultiTenancyConfigurationsUpdateAction.java

.../org/opensearch/security/action/tenancy/MultiTenancyConfigurationsUpdateTransportAction.java

peternied · 2023-04-04T22:20:24Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -525,6 +525,15 @@ public boolean multitenancyEnabled() {
                && dcm.isDashboardsMultitenancyEnabled();
    }

+    public boolean privateTenantEnabled() {
+        return privilegesInterceptor.getClass() != PrivilegesInterceptor.class


As long as this is exercised in end to end tests, ok.

src/main/java/org/opensearch/security/action/tenancy/EmptyRequest.java

peternied · 2023-04-04T22:29:44Z

src/test/java/org/opensearch/security/multitenancy/test/TenancyDefaultTenantTests.java

+        assertThat(getSettingResponseAfterUpdate.getStatusCode(), equalTo(HttpStatus.SC_OK));
+        assertThat(getSettingResponseAfterUpdate.findValueInJson("value"), equalTo("Private"));
+
+        getAuthinfoResponse = nonSslRestHelper().executeGetRequest("/_plugins/_security/authinfo", asAdminUser);


We need the end to end coverage of this scenarios in the security plugin. Otherwise breaking changes could be make in this repo.

The example from TenancyMultitenancyEnabledTests.java shows how when the feature is activated behavior changes, please use this as a starting point. Let me know if you need help to test these scenarios.

src/test/java/org/opensearch/security/multitenancy/test/TenancyDefaultTenantTests.java

Signed-off-by: Abhi Kalra <[email protected]>

expani · 2023-04-06T18:41:37Z

src/main/java/org/opensearch/security/action/tenancy/TenancyConfigUpdateTransportAction.java

+        Set<String> availableTenants = getAllConfiguredTenantNames();
+
+        if(!availableTenants.contains(updatedConfig.dynamic.kibana.default_tenant)){
+            throw new IllegalArgumentException(updatedConfig.dynamic.kibana.default_tenant + " can not be set to default tenant. Default tenant should be selected from one of the available tenants.");


Intentionally not adding the availableTenants in the logs.

stephen-crawford

Hi @abhivka7, I see that a few of my questions got answered. Reviewing the remaining ones and the state of the PR, I am fine to merge this without any further conversation. Thank you for the contribution and being so quick to respond throughout. Great job!

abhivka7 · 2023-04-10T06:41:25Z

Thanks @peternied and @scrawfor99 for reviewing and merging this PR. Also thanks @prabhat-chaturvedi @anijain-Amazon @devardee @shikharj05 @cliu123 for helping with this feature.

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

opensearch-trigger-bot · 2023-04-17T21:05:45Z

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-2607-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9fca0dae5c1e3fe4746381d9f87c4d4abb0f0fda
# Push it to GitHub
git push --set-upstream origin backport/backport-2607-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-2607-to-2.x.

opensearch-trigger-bot · 2023-04-17T21:08:32Z

The backport to 2.7 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.7 2.7
# Navigate to the new working tree
cd .worktrees/backport-2.7
# Create a new branch
git switch --create backport/backport-2607-to-2.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9fca0dae5c1e3fe4746381d9f87c4d4abb0f0fda
# Push it to GitHub
git push --set-upstream origin backport/backport-2607-to-2.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.7

Then, create a pull request where the base branch is 2.7 and the compare/head branch is backport/backport-2607-to-2.7.

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]> Signed-off-by: Maciej Mierzwa <[email protected]>

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]> Signed-off-by: Sam <[email protected]>

abhivka7 requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners March 30, 2023 14:03

abhivka7 mentioned this pull request Mar 30, 2023

Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… #2444

Closed

expani reviewed Mar 30, 2023

View reviewed changes

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved

expani reviewed Mar 30, 2023

View reviewed changes

peternied requested changes Mar 30, 2023

View reviewed changes

abhivka7 force-pushed the Dynamic_Tenancy_Configurations branch from b54d66f to b6f70da Compare April 4, 2023 06:55

abhivka7 requested a review from reta as a code owner April 4, 2023 06:55

abhivka7 force-pushed the Dynamic_Tenancy_Configurations branch 4 times, most recently from 0d0c7e4 to 9009e30 Compare April 4, 2023 11:54

peternied requested changes Apr 4, 2023

View reviewed changes

abhivka7 force-pushed the Dynamic_Tenancy_Configurations branch 3 times, most recently from c843cd6 to 51e1148 Compare April 5, 2023 11:12

Dynamic_Tenancy_Configurations changes

bfbe371

Signed-off-by: Abhi Kalra <[email protected]>

abhivka7 force-pushed the Dynamic_Tenancy_Configurations branch from 6e7daa7 to bfbe371 Compare April 5, 2023 14:45

abhivka7 mentioned this pull request Apr 5, 2023

Dynamic tenancy configurations opensearch-project/security-dashboards-plugin#1394

Merged

3 tasks

expani reviewed Apr 6, 2023

View reviewed changes

peternied approved these changes Apr 6, 2023

View reviewed changes

stephen-crawford self-requested a review April 7, 2023 13:12

stephen-crawford approved these changes Apr 7, 2023

View reviewed changes

stephen-crawford merged commit 9fca0da into opensearch-project:main Apr 7, 2023

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Apr 11, 2023

Dynamic_Tenancy_Configurations changes (opensearch-project#2607)

354b980

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

MaciejMierzwa pushed a commit to MaciejMierzwa/security that referenced this pull request Apr 11, 2023

Dynamic_Tenancy_Configurations changes (opensearch-project#2607)

d096bd3

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

stephen-crawford mentioned this pull request Apr 12, 2023

[BUG] SecuritySSLReloadCertsActionTests is using outdated certificates #2675

Closed

cwperks added the backport 2.x backport to 2.x branch label Apr 17, 2023

stephen-crawford added the backport 2.7 backport to 2.7 branch label Apr 17, 2023

stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Apr 17, 2023

Dynamic_Tenancy_Configurations changes (opensearch-project#2607)

8ae482a

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

stephen-crawford mentioned this pull request Apr 17, 2023

[Backport 2.7] Dynamic_Tenancy_Configurations changes (#2607) #2692

Closed

3 tasks

stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Apr 17, 2023

Dynamic_Tenancy_Configurations changes (opensearch-project#2607)

e7e820f

Signed-off-by: Abhi Kalra <[email protected]> Co-authored-by: Abhi Kalra <[email protected]>

stephen-crawford mentioned this pull request Apr 17, 2023

[Backport 2.7] Manual Backport of Dynamic tenancy configurations #2693

Merged

3 tasks

RyanL1997 mentioned this pull request Apr 18, 2023

[Backport 2.x] Manual Backport of Dynamic tenancy configurations #2696

Merged

3 tasks

davidlago mentioned this pull request Aug 21, 2023

[FEATURE] Create a way to add action oriented APIs to update configuration values #2559

Open

DarshitChanpura mentioned this pull request Mar 20, 2024

Add workspace and multi tenancy check opensearch-project/security-dashboards-plugin#1822

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic tenancy configurations #2607

Dynamic tenancy configurations #2607

abhivka7 commented Mar 30, 2023

expani Mar 30, 2023

peternied left a comment

peternied Mar 30, 2023

abhivka7 Apr 3, 2023

peternied Apr 4, 2023

peternied Mar 30, 2023

abhivka7 Apr 3, 2023

peternied Apr 4, 2023

abhivka7 Apr 5, 2023

expani commented Apr 1, 2023 •

edited

Loading

peternied commented Apr 1, 2023

expani commented Apr 1, 2023

expani commented Apr 1, 2023 •

edited

Loading

prabhat-chaturvedi commented Apr 3, 2023

abhivka7 commented Apr 3, 2023

peternied left a comment •

edited

Loading

peternied Apr 4, 2023

abhivka7 Apr 5, 2023

peternied Apr 4, 2023

peternied Apr 4, 2023

expani Apr 6, 2023

stephen-crawford left a comment

abhivka7 commented Apr 10, 2023

opensearch-trigger-bot bot commented Apr 17, 2023

opensearch-trigger-bot bot commented Apr 17, 2023


		package org.opensearch.security.action.tenancy;

		public class MultiTenancyConfigurations {

Dynamic tenancy configurations #2607

Dynamic tenancy configurations #2607

Conversation

abhivka7 commented Mar 30, 2023

Description

Issues Resolved

Testing

Check List

Choose a reason for hiding this comment

peternied 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

expani commented Apr 1, 2023 • edited Loading

peternied commented Apr 1, 2023

expani commented Apr 1, 2023

expani commented Apr 1, 2023 • edited Loading

prabhat-chaturvedi commented Apr 3, 2023

abhivka7 commented Apr 3, 2023

peternied left a comment • edited Loading

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

stephen-crawford left a comment

Choose a reason for hiding this comment

abhivka7 commented Apr 10, 2023

opensearch-trigger-bot bot commented Apr 17, 2023

opensearch-trigger-bot bot commented Apr 17, 2023

expani commented Apr 1, 2023 •

edited

Loading

expani commented Apr 1, 2023 •

edited

Loading

peternied left a comment •

edited

Loading