Fixing dynamic tenancy changes for issues 1412

[abhivka7]

Description

With https://github.com/opensearch-project/security-dashboards-plugin/pull/1394/files, we removed the use of flag opensearch_security.multitenancy.enabled in the opensearchdashboard.yaml file.

This caused some breaking changes like: #1412
With this CR, we will ensure that tenancy for Dashboards UI will also be disabled when opensearch_security.multitenancy.enabled in the opensearchdashboard.yaml file is disabled.

Category

Bug fix

Why these changes are required?

Fixing: #1412

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

Earlier tenancy was disabled only by making change in SecurityIndex using API or Dashboards UI. With these changes, dashboards tenancy will be disabled with flag opensearch_security.multitenancy.enabled in the opensearchdashboard.yaml file, the removing breaking behaviour introduced in https://github.com/opensearch-project/security-dashboards-plugin/pull/1394/files.

Issues Resolved

#1412

Testing

Tested manually. This is still a draft PR and will add a few unit tests after some review on PR.

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.

[stephen-crawford]

Hi @abhivka7, looks good but is there a way we can add a test that will check that the behavior remains as expected for the future? Thanks.

[abhivka7]

@scrawfor99 I will write the tests tomorrow, waiting for others to review this for issue: #1412

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.47%. Comparing base (68738c2) to head (3450d12).
⚠️ Report is 233 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/configuration/panels/tenant-list/tenant-list.tsx	50.00%	1 Missing ⚠️
public/plugin.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
- Coverage   65.51%   65.47%   -0.05%     
==========================================
  Files          93       93              
  Lines        2294     2297       +3     
  Branches      307      308       +1     
==========================================
+ Hits         1503     1504       +1     
- Misses        723      725       +2     
  Partials       68       68              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DarshitChanpura]

@abhivka7 Thank you for pushing this fix. I have a couple of recommendations that I've added as a comment.

[DarshitChanpura]

This can be extracted as a class-level variable:

const { enabled: enabledInYmlConfig = true } = props.config?.multitenancy;

const shouldDisplaySwitchTenantsPanel = isMultiTenancyEnabled && enabledInYmlConfig;

and use the variable across the file.

[abhivka7]

@DarshitChanpura isMultiTenancyEnabled can change if we change the value of multitenancy_enabled in Security config. Therefore making a constant using it might not be the best idea.

[DarshitChanpura]

could you also update the fetchDatat method to be:

const { http } = props.coreStart;

const fetchData = useCallback(async () => {
    try {
      setLoading(true);
      const rawTenantData = await fetchTenants(http);
      const processedTenantData = transformTenantData(rawTenantData);
      const activeTenant = await fetchCurrentTenant(http);
      const currentUser = await getCurrentUser(http);
      setCurrentUsername(currentUser);
      setCurrentTenant(resolveTenantName(activeTenant, currentUser));
      setTenantData(processedTenantData);
      const tenancyConfig = await getDashboardsInfo(http;
      setIsMultiTenancyEnabled tenancyConfig.multitenancy_enabled);
      setIsPrivateTenantEnabled(tenancyConfig.private_tenant_enabled);
      setDashboardsDefaultTenant(tenancyConfig.default_tenant);
    } catch (e) {
      console.log(e);
      setErrorFlag(true);
    } finally {
      setLoading(false);
    }
  }, [http]);

[abhivka7]

Sure will do. Thanks.

[cwperks]

Can you create a follow-up issue to switch back to use the opensearch-project repository after any changes you have on your fork are merged?

[abhivka7]

@cwperks This is just for testing so that I can show in Functional test PR, will remove this commit soon.

[abhivka7]

PR for Integ tests: opensearch-project/opensearch-dashboards-functional-test#653

@scrawfor99 @cwperks Can you guys review both PRs? Thanks.

[cwperks]

Some of these settings look like they are not needed and may even cause an invalid setup.

Is it possible to remove opensearch_security.multitenancy.tenants.preferred and opensearch_security.multitenancy.enable_aggregation_view: true here? Are they necessary for the cypress tests here?

[abhivka7]

Cool will do that.

[cwperks]

@abhivka7 I have a general question based on this approach.

What happens if opensearch_security.multitenancy.enabled is set to false in opensearch_dashboards.yml, but true on the backend? This is changing the original definition of the opensearch_security.multitenancy.enabled setting and now treating this as a feature flag to gate the dynamic multi-tenancy feature. Changing this flag would require an OSD reboot as this is a static setting.

As @kavilla brought up here, the value in the frontend defaults to true but then has to wait for an asynchronous API call to the backend to get the true value of the setting:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(true);

Do you think the default value should be removed here and wait for the response from the backend before updating the state? If its still loading can it display a spinner or loading message? Would there ever be a flicker of the 'choose tenant' modal if there's a delay in the API response to get dashboards info?

[abhivka7]

@abhivka7 I have a general question based on this approach.

What happens if opensearch_security.multitenancy.enabled is set to false in opensearch_dashboards.yml, but true on the backend?

@cwperks If opensearch_security.multitenancy.enabled is set to false in opensearch_dashboards.ym then user won't be able to access any tenant related feature from Dashboards UI.

This is changing the original definition of the opensearch_security.multitenancy.enabled setting and now treating this ?>as a feature flag to gate the dynamic multi-tenancy feature. Changing this flag would require an OSD reboot as this is a >static setting.

This behaviour was always there. opensearch_security.multitenancy.enabled` was the flag to disable tenancy from Dashboards.

As @kavilla brought up, the value in the frontend defaults to true but then has to wait for an asynchronous API call to the backend to get the true value of the setting:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(true);

Do you think the default value should be removed here and wait for the response from the backend before updating the state? If its still loading can it display a spinner or loading message? Would there ever be a flicker of the 'choose tenant' modal if there's a delay in the API response to get dashboards info?

As far as I have tested I haven't seen this issue so far. It is a smooth process and doesn't take much time.

[stephen-crawford]

Hi @abhivka7, overall the code changes look fine but I was hoping to clarify something with you. In your response to Craig's previous comment, you mentioned that if the opensearch_security.multitenancy.enabled is set to false, then all tenancy features are turned off. Unless I misunderstand this, why would this scenario not result in the user always using the Global tenant? The issue in the PR body shows that Tao found he could not use the Global tenant option without having multitenancy turned on. This seems odd as I didn't think multitenancy included the global tenant directly.

Is this in-line with the existing behavior or would this be a restructuring of the code that could cause trouble down the road?

[SuZhou-Joe]

Backport to 2.x has failed and the issue still exists in branch 2.8 and 2.x. Could you please backport this PR to 2.x and 2.8 manually? @abhivka7

[cwperks]

@SuZhou-Joe Creating a manual backport to 2.8 now. Thank you for raising this.