Add workspace and multi tenancy check #1822

Hailong-am · 2024-03-12T07:15:14Z

Description

Add check for both workspace and multi tenancy turn on case. If they are both on, an error message will print on console and whole OSD process will exit.

Enable both workspace and multi tenancy

Run the command to start OSD
yarn start --workspace.enabled=true --opensearch_security.multitenancy.enabled=true and dynamic multi-tenancy flag is true

The output

server    log   [07:12:26.988] [error][plugins][securityDashboards] Both workspace and multi-tenancy features are enabled, only one of them could enabled at same time.
server crashed  with status code 1

Enable multi tenancy only

Run the command to start OSD
yarn start --no-base-path --opensearch_security.multitenancy.enabled=true

Output

server    log   [07:14:28.546] [info][listening] Server running at http://0.0.0.0:5601
server    log   [07:14:28.612] [info][server][OpenSearchDashboards][http] http server running at http://0.0.0.0:5601

Enable workspace only

Run the command to start OSD
yarn start --no-base-path --workspace.enabled=true --opensearch_security.multitenancy.enabled=false

Output

server    log   [07:21:27.487] [info][plugins-system] Starting [40] plugins: [usageCollection,opensearchDashboardsUsageCollection,opensearchDashboardsLegacy,mapsLegacy,share,opensearchUiShared,legacyExport,embeddable,expressions,data,home,apmOss,savedObjects,workspace,dashboard,visualizations,visTypeVega,visTypeTimeline,visTypeTable,visTypeMarkdown,visBuilder,visAugmenter,tileMap,regionMap,inputControlVis,visualize,management,indexPatternManagement,advancedSettings,console,dataExplorer,charts,visTypeTimeseries,visTypeVislib,visTypeTagcloud,visTypeMetric,discover,savedObjectsManagement,securityDashboards,bfetch]
server    log   [07:21:27.700] [info][listening] Server running at http://0.0.0.0:5601
server    log   [07:21:27.755] [info][server][OpenSearchDashboards][http] http server running at http://0.0.0.0:5601

When workspace enabled

Multi-tenancy check is disabled that will not all cx turn on multi-tenancy dynamiclly.

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

opensearch-project/OpenSearch-Dashboards#1819

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

New functionality includes testing
New functionality has been documented
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.

Hailong-am · 2024-03-12T07:19:53Z

cc @xluo-aws

server/plugin.ts

codecov · 2024-03-12T10:11:22Z

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 68.00%. Comparing base (3bff6db) to head (9e484d9).

Files	Patch %	Lines
...onfiguration/panels/tenant-list/configure_tab1.tsx	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1822      +/-   ##
==========================================
- Coverage   68.04%   68.00%   -0.05%     
==========================================
  Files          94       94              
  Lines        2422     2425       +3     
  Branches      330      332       +2     
==========================================
+ Hits         1648     1649       +1     
- Misses        696      698       +2     
  Partials       78       78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

derek-ho · 2024-03-12T15:11:23Z

server/plugin.ts

+        this.logger.error(
+          'Both workspace and multi-tenancy features are enabled, only one of them can be enabled at the same time.'
+        );
+        process.exit(1);


Why are we killing OSD? Is there a better way here? Can we default to only setup workspace and log this out instead of killing the process?

Why are we killing OSD? Is there a better way here?

The idea is that if we don't kill the process, user still use multi tenancy and workspace at the same time if they choose to ignore the error or warning message.

Can we default to only setup workspace and log this out

does this mean we don't setup multi tenancy if workspace is enabled. It would be better let user choose which features they want to try.

Yea maybe it is a product/UX question but from my perspective killing the process seems bad. If we are encouraging people to migrate to workspaces anyway, why can't we default to workspaces and simply tell them we are doing so? Since workspaces is a new feature and opt in I don't think this is a big stretch - @kgcreative have any thoughts here?

Most likely user doesn't know workspace and multi tenant are mutually exclusive if they have both feature flags on. If we turn off multi tenant automatically silently, will it be a problem? I feel it's better to stop the booting process to let user make the decision. User should already stop the OSD process before they make the configuration change and just restart the process so the impact to stop it is not serious.

We should add a toggle here instead of killing the process. When we enable one we automatically disable the other one and log that.

Dynamic tenancy configurations #2607 - this PR allows enabling / disabling multi-tenancy dynamically.

@Hailong-am wdyt about disabling the ability to toggle multi-tenancy dynamically from off -> on if workspaces is enabled?

The code for dynamic multi-tenancy UX can be found here: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/apps/configuration/panels/tenant-list/save_changes_modal.tsx#L84-L88

@Hailong-am wdyt about disabling the ability to toggle multi-tenancy dynamically from off -> on if workspaces is enabled?

The code for dynamic multi-tenancy UX can be found here: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/apps/configuration/panels/tenant-list/save_changes_modal.tsx#L84-L88

Yeah, make sense.

@cwperks @DarshitChanpura I have update the PR base on your suggestions, could you help to review it again?

@Hailong-am I still feel strongly that process.exit is the wrong way for a service to handle an invalid configuration after it has started. What other options have been explored?

for a service to handle an invalid configuration after it has started.

@peternied For invalid configuration, if we have unrecognized configuration in yml file OSD will crash when start up. That's current behavior. The check we added here is slightly different, but we can follow the same pattern.

An example

log [05:27:15.151] [fatal][root] InvalidConfigurationError: Unknown configuration key(s): "abc.enabled". Check for spelling errors and ensure that expected plugins are installed. at ensureValidConfiguration (/Users/ihailong/code/OpenSearch-Dashboards-workspace/src/core/server/legacy/config/ensure_valid_configuration.ts:50:11) at Server.setup (/Users/ihailong/code/OpenSearch-Dashboards-workspace/src/core/server/server.ts:152:5) at Root.setup (/Users/ihailong/code/OpenSearch-Dashboards-workspace/src/core/server/root/index.ts:64:14) at bootstrap (/Users/ihailong/code/OpenSearch-Dashboards-workspace/src/core/server/bootstrap.ts:132:5) at Command.<anonymous> (/Users/ihailong/code/OpenSearch-Dashboards-workspace/src/cli/serve/serve.js:279:5) { code: 'InvalidConfig', processExitCode: 64, cause: undefined } FATAL Error: Unknown configuration key(s): "abc.enabled". Check for spelling errors and ensure that expected plugins are installed.

Here are the options we have think about:

Document the prerequisites of enabling workspace in document site and let user to do the right configuration

Instead of exit the process, log the warning message. Most of time cx will choose to ignore warning, even for today when OpenSearch dashboard start there have few warnings, not everyone cares about warning message.

Disable multi-tenancy automatically when workspace is enabled

Option 1 and 2 will not enforce cx to just one of them and have the possibility to mix them together, we don't want to support this mixed situation.

Option 3, As @xluo-aws mentioned here #1822 (comment) , we don't know the intention of cx and disable multi-tenancy by default is not a good decision.

cwperks · 2024-03-14T14:05:00Z

@Hailong-am Are you still working on this PR? Can you flip this from Draft when it is ready for review?

Hailong-am · 2024-03-15T00:39:57Z

@Hailong-am Are you still working on this PR? Can you flip this from Draft when it is ready for review?

Yes, i am working on that.

peternied

@Hailong-am The multi-tenancy feature be enabled/disabled on a running OpenSearch cluster, checking only startup is not sufficient - another mechanism will be needed.

Data is inside of a tenant cannot be accessed when the feature is disabled. This would be a large regression for our customer base that depends on this feature, a way to access/migrate this data needs to be provided before we can get 'locked' into this state.

Hailong-am · 2024-03-19T03:41:13Z

The multi-tenancy feature be enabled/disabled on a running OpenSearch cluster, checking only startup is not sufficient - another mechanism will be needed.

If we disable multi-tenancy in yml file, then multi-tenancy feature will disabled on OSD side. Bases on this, does user have other ways to dynamic enable/disable this feature?

Data is inside of a tenant cannot be accessed when the feature is disabled. This would be a large regression for our customer base that depends on this feature, a way to access/migrate this data needs to be provided before we can get 'locked' into this state.

Yes, make sense. User could export all tenant data(saved objects) from Saved Objects Page first and then turn workspace feature on and import the data into a workspace.

On the other hand, this is not one-way door. User could still turn off workspace feature and back to multi-tenancy.

peternied · 2024-03-19T17:50:37Z

Thanks for the response @Hailong-am

The multi-tenancy feature be enabled/disabled on a running OpenSearch cluster, checking only startup is not sufficient - another mechanism will be needed.

If we disable multi-tenancy in yml file, then multi-tenancy feature will disabled on OSD side. Bases on this, does user have other ways to dynamic enable/disable this feature?

Please see this pull request [1] contributed by @abhivka7 that initially added this functionality. I believe you can fan out starting from public/apps/configuration/panels/tenant-list/manage_tab.tsx to find many of the different APIs and hooks that were added for this flow.

On the other hand, this is not one-way door. User could still turn off workspace feature and back to multi-tenancy.

If 'Users' means sysadmins - I definitely agree. However, the majority of dashboard users are not sysadmin and do not have such control. This would be confusing as their experience would drastically shift - making it seem as if there was data loss.

I don't think we need to have this problem fully solved for this PR and I think we need to track the implications of this decisions and the work associated with it - are there an meta issue(s) that capture these considerations, if not can you create them so that we have a strategy as we iterate towards the expected outcome?

[1] Dynamic tenancy configurations #1394

Hailong-am · 2024-03-21T09:00:06Z

Please see this pull request [1] contributed by @abhivka7 that initially added this functionality. I believe you can fan out starting from public/apps/configuration/panels/tenant-list/manage_tab.tsx to find many of the different APIs and hooks that were added for this flow.

@peternied Thanks for the info, after went through the dynamic multi-tenancy logic, If multi-tenancy is turned off in yml, the multi-tenancy router will not register at Node.js side that means dynamic enable/disable multi-tenancy API is not available and sysadmin can't dynamic turn on multi-tenancy in OpenSearch Dashboards.

Even user set default tenancy to private and the turn multi-tenancy on in system index and turn off multi-tenancy in yml file and login, it will not switch to private tenancy after login next time, since switch tenancy API is not available also. The session context will not switch to any tenancy.

are there an meta issue(s) that capture these considerations, if not can you create them so that we have a strategy as we iterate towards the expected outcome?

We have a meta issue for whole workspace feature opensearch-project/OpenSearch-Dashboards#4944, and create one sub task for tenant data migration #1847.

Signed-off-by: Hailong Cui <[email protected]>

xluo-aws · 2024-03-25T09:54:41Z

public/apps/configuration/panels/tenant-list/configure_tab1.tsx

+              updatedConfiguration1.multitenancy_enabled &&
+              props.coreStart.application.capabilities.workspaces.enabled
+            ) {
+              throw new Error(


Can we ever get into this logic? If workspace is enabled in yml, multitenancy won't be enabled in yml. And if multitenancy is disabled in yml, we won't be able to dynamically turn it on because the API is not available, right?

The check has been changed to check both multitenancy in yml and dynamic value from backend index.
we will stop the OSD server only if multitenancy in yml and dynamic value both are true.

I don't think we should prevent the server from starting. I think there should be an empty state in workspaces that asks the customer that they disable multitenancy via dynamic setting, and deep link them to multitenancy config in security plugin. We may need a short period of time where both multitenancy AND workspaces are enabled in order for us to be able to migrate tenants to worskpaces, so we may in the future, have an overlap period for doing deep copy of saved objects, then turning off multitenancy via dynamic config so that workspaces become usable

I think there should be an empty state in workspaces that asks the customer that they disable multitenancy via dynamic setting

@kgcreative could have some mockup of this empty state for better understanding? this is something like disable workspace automatically when multi-tenancy is enabled.

@kgcreative , Kevin, what you suggested is definitely a more elegant approach, but we don't have plan to support migration in the initial release due to time restraint(User can export then import as a workaround). If user starts creating objects with both workspace and multi tenancy one, it will bring unexpected result that's hard to fix later. We stop the instance because user has wrong configuration, as Hailong said, it's the same approach if user adds a parameter but didn't install related plugins or it's an invalid parameter.

Signed-off-by: Hailong Cui <[email protected]>

cwperks · 2024-04-17T17:56:09Z

@Hailong-am Is this PR still in progress?

Hailong-am · 2024-04-18T03:23:39Z

@Hailong-am Is this PR still in progress?

Given the comments from @kgcreative #1822 (comment) and we might not need to add this kind of check on startup. let me close it for now and re-open it when it's ready.

Hailong-am mentioned this pull request Mar 12, 2024

[Workspace] Add WorkspacePluginSetup interface opensearch-project/OpenSearch-Dashboards#6118

Merged

7 tasks

xluo-aws reviewed Mar 12, 2024

View reviewed changes

server/plugin.ts Outdated Show resolved Hide resolved

derek-ho reviewed Mar 12, 2024

View reviewed changes

Hailong-am marked this pull request as ready for review March 15, 2024 00:39

Hailong-am requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners March 15, 2024 00:39

peternied requested changes Mar 18, 2024

View reviewed changes

Hailong-am mentioned this pull request Mar 23, 2024

[Workspace] Migrate tenant data to workspace #1847

Closed

Hailong-am added 5 commits March 25, 2024 11:22

Add workspace and multi tenancy check

6fbdf71

Signed-off-by: Hailong Cui <[email protected]>

fix lint

e69766d

Signed-off-by: Hailong Cui <[email protected]>

update wording

c1a3ba0

Signed-off-by: Hailong Cui <[email protected]>

fix lint

b213334

Signed-off-by: Hailong Cui <[email protected]>

check dynamic tenancy config

dfa03fd

Signed-off-by: Hailong Cui <[email protected]>

Hailong-am force-pushed the workpsace_multi_tenancy_check branch from 3c70ab3 to dfa03fd Compare March 25, 2024 07:49

xluo-aws reviewed Mar 25, 2024

View reviewed changes

throw CriticalError instead of procees.exit

9e484d9

Signed-off-by: Hailong Cui <[email protected]>

Hailong-am marked this pull request as draft March 27, 2024 00:48

Hailong-am closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workspace and multi tenancy check #1822

Add workspace and multi tenancy check #1822

Hailong-am commented Mar 12, 2024 •

edited

Loading

Hailong-am commented Mar 12, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

derek-ho Mar 12, 2024

Hailong-am Mar 13, 2024 •

edited

Loading

derek-ho Mar 13, 2024

xluo-aws Mar 13, 2024

DarshitChanpura Mar 20, 2024 •

edited

Loading

cwperks Mar 22, 2024

Hailong-am Mar 23, 2024

Hailong-am Mar 25, 2024 •

edited

Loading

peternied Mar 25, 2024 •

edited

Loading

Hailong-am Mar 26, 2024

cwperks commented Mar 14, 2024

Hailong-am commented Mar 15, 2024

peternied left a comment •

edited

Loading

Hailong-am commented Mar 19, 2024

peternied commented Mar 19, 2024

Hailong-am commented Mar 21, 2024 •

edited

Loading

xluo-aws Mar 25, 2024

Hailong-am Mar 25, 2024

kgcreative Mar 26, 2024

Hailong-am Mar 27, 2024

xluo-aws Mar 27, 2024

cwperks commented Apr 17, 2024

Hailong-am commented Apr 18, 2024

Add workspace and multi tenancy check #1822

Add workspace and multi tenancy check #1822

Conversation

Hailong-am commented Mar 12, 2024 • edited Loading

Description

Category

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

Testing

Check List

Hailong-am commented Mar 12, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Hailong-am Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hailong-am Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

peternied Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks commented Mar 14, 2024

Hailong-am commented Mar 15, 2024

peternied left a comment • edited Loading

Choose a reason for hiding this comment

Hailong-am commented Mar 19, 2024

peternied commented Mar 19, 2024

Hailong-am commented Mar 21, 2024 • 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

cwperks commented Apr 17, 2024

Hailong-am commented Apr 18, 2024

Hailong-am commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

Hailong-am Mar 13, 2024 •

edited

Loading

DarshitChanpura Mar 20, 2024 •

edited

Loading

Hailong-am Mar 25, 2024 •

edited

Loading

peternied Mar 25, 2024 •

edited

Loading

peternied left a comment •

edited

Loading

Hailong-am commented Mar 21, 2024 •

edited

Loading