Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add workspace and multi tenancy check #1822

Closed
wants to merge 6 commits into from
Closed

Add workspace and multi tenancy check #1822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6fbdf71
Add workspace and multi tenancy check
Hailong-am Mar 12, 2024
e69766d
fix lint
Hailong-am Mar 12, 2024
c1a3ba0
update wording
Hailong-am Mar 12, 2024
b213334
fix lint
Hailong-am Mar 12, 2024
dfa03fd
check dynamic tenancy config
Hailong-am Mar 25, 2024
9e484d9
throw CriticalError instead of procees.exit
Hailong-am Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion opensearch_dashboards.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
"savedObjectsManagement"
],
"optionalPlugins": [
"managementOverview"
"managementOverview",
"workspace"
],
"server": true,
"ui": true
Expand Down
9 changes: 9 additions & 0 deletions public/apps/configuration/panels/tenant-list/configure_tab1.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@
handleSave={async (updatedConfiguration1: TenancyConfigSettings) => {
try {
console.log('Calling API');
if (
updatedConfiguration1.multitenancy_enabled &&
props.coreStart.application.capabilities.workspaces.enabled
) {
throw new Error(

Check warning on line 105 in public/apps/configuration/panels/tenant-list/configure_tab1.tsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L105 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

'Multi-tenancy is not allowed to enable as workspace is enabled, you can disable workspace and retry.'
);
}
await updateTenancyConfiguration(props.coreStart.http, updatedConfiguration1);
setSaveChangesModal(null);
setChangeInMultiTenancyOption(0);
Expand Down Expand Up @@ -349,6 +357,7 @@
label={'Enabled'}
checked={updatedConfiguration.multitenancy_enabled}
onChange={() => onSwitchChangeTenancyEnabled()}
disabled={props.coreStart.application.capabilities.workspaces.enabled}
/>
</EuiDescribedFormGroup>
</EuiPageContentHeaderSection>
Expand Down
23 changes: 21 additions & 2 deletions server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import { first } from 'rxjs/operators';
import { Observable } from 'rxjs';
import { CriticalError } from '../../../src/core/server/errors';
import {
PluginInitializerContext,
CoreSetup,
Expand Down Expand Up @@ -46,6 +47,7 @@ import { createMigrationOpenSearchClient } from '../../../src/core/server/saved_
import { SecuritySavedObjectsClientWrapper } from './saved_objects/saved_objects_wrapper';
import { addTenantParameterToResolvedShortLink } from './multitenancy/tenant_resolver';
import { ReadonlyService } from './readonly/readonly_service';
import { WorkspacePluginSetup } from '../../../src/plugins/workspace/server';

export interface SecurityPluginRequestContext {
logger: Logger;
Expand All @@ -68,7 +70,12 @@ declare module 'opensearch-dashboards/server' {
}
}

export class SecurityPlugin implements Plugin<SecurityPluginSetup, SecurityPluginStart> {
interface SecurityPluginSetupDeps {
workspace: WorkspacePluginSetup;
}

export class SecurityPlugin
implements Plugin<SecurityPluginSetup, SecurityPluginStart, SecurityPluginSetupDeps> {
private readonly logger: Logger;
// FIXME: keep an reference of admin client so that it can be used in start(), better to figureout a
// decent way to get adminClient in start. (maybe using getStartServices() from setup?)
Expand All @@ -83,7 +90,7 @@ export class SecurityPlugin implements Plugin<SecurityPluginSetup, SecurityPlugi
this.savedObjectClientWrapper = new SecuritySavedObjectsClientWrapper();
}

public async setup(core: CoreSetup) {
public async setup(core: CoreSetup, { workspace }: SecurityPluginSetupDeps) {
this.logger.debug('opendistro_security: Setup');

const config$ = this.initializerContext.config.create<SecurityPluginConfigType>();
Expand Down Expand Up @@ -136,6 +143,18 @@ export class SecurityPlugin implements Plugin<SecurityPluginSetup, SecurityPlugi
defineRoutes(router);
defineAuthTypeRoutes(router, config);

// multitenancyinfo is application level
const dashboardsInfo = await esClient.callAsInternalUser(
'opensearch_security.multitenancyinfo'
);

if (workspace && config.multitenancy?.enabled && dashboardsInfo.multitenancy_enabled) {
const message =
'Both workspace and multi-tenancy features are enabled, only one of them can be enabled at the same time.';
this.logger.error(message);
throw new CriticalError(message, 'InvalidConfig', 64);
}

// set up multi-tenant routes
if (config.multitenancy?.enabled) {
setupMultitenantRoutes(router, securitySessionStorageFactory, this.securityClient);
Expand Down
Loading