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

Disable management plugins using contextRef #160671

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Disable management plugins using contextRef #160671

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c022176
update enabled config to use serverless contextRef
alisonelizabeth Jun 27, 2023
30221f3
add serverless tests
alisonelizabeth Jun 27, 2023
ef4e3da
Merge branch 'main' of github.com:elastic/kibana into management_disa…
alisonelizabeth Jun 28, 2023
1c817e7
add code comments
alisonelizabeth Jun 28, 2023
656f7e4
Merge branch 'main' into management_disabled_plugins
kibanamachine Jul 6, 2023
713fe60
move tests to common
alisonelizabeth Jul 6, 2023
72faae1
delete duplicate test file
alisonelizabeth Jul 6, 2023
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
7 changes: 6 additions & 1 deletion x-pack/plugins/cloud_integrations/cloud_data_migration/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ import { schema, TypeOf } from '@kbn/config-schema';
import { PluginConfigDescriptor } from '@kbn/core-plugins-server';

const configSchema = schema.object({
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this breaks the common functional tests (I think related to work done via #159272). If I run the tests using the common config:

node scripts/functional_tests_server.js --config test_serverless/api_integration/test_suites/common/config.ts

Then, I get the following error:

 FATAL  Error: [config validation of [xpack.cloud_integrations.data_migration].enabled]: a value wasn't expected to be present

Copy link
Member

Choose a reason for hiding this comment

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

Yeah while this approach makes things safer in terms of minimizing our API surface area, the tradeoff is that serverless & self-managed need to be tested separately & common tests won't really work anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually related to the common serverless test suite. It looks like in #159272, a change was made so that the common suite runs directly with the serverless.yml file instead of specifying a project type. See: https://github.com/elastic/kibana/pull/159272/files#diff-818fc0329318f82bcee0547dfb8edb3800cce36ea3fad7c62d58363197d91366R28

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I misunderstood your comment. Looking at the code I'm wondering if what's throwing it off is that we aren't getting a --serverless cli flag at all when running the common tests. Ultimately that's what we look at in the environment to set the context ref accordingly, so schema.contextRef('serverless') is likely returning false.

Poking around in src/cli/serve.js it appears we try to read from the provided config files as a backup method to determine if we're in serverless mode (if the --serverless flag isn't explicitly passed), however I think we are checking for a top-level serverless value to be in the yml files, which I don't think we provide anywhere... so we may have a bug in that logic.

FYI @jbudz, this might be a bug, but keep me honest if I'm misreading things :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, should have clarified 😅

Ah, I see. @jbudz let me know if you'd like me to open up an issue or if I can help in any way.

Copy link
Contributor Author

@alisonelizabeth alisonelizabeth Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The tests were fixed via #160783

schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By taking this approach, we also set the setting to false in the serverless.yml file. This would give us a clear view of all of the plugins that are disabled. Alternatively, we can set the default value to false here and then remove the configuration from the serverless.yml file.

Copy link
Member

Choose a reason for hiding this comment

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

++ I think the approach of explicitly setting false in the serverless.yml makes things easier to reason about and prevents our serverless configs from spreading out into more places, even if it seems counterintuitive to default to true here. You could also consider adding a comment to these to make that even clearer for folks in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Good idea about adding a comment 👍

schema.never()
),
});

export type CloudDataMigrationConfig = TypeOf<typeof configSchema>;
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/cross_cluster_replication/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/index_lifecycle_management/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/license_management/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/remote_clusters/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/rollup/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/snapshot_restore/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ const schemaLatest = schema.object(
* Disables the plugin.
* Added back in 8.8.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),
},
{ defaultValue: undefined }
);
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/upgrade_assistant/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ const configSchema = schema.object({
/**
* Disables the plugin.
*/
enabled: schema.boolean({ defaultValue: true }),
enabled: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({ defaultValue: true }),
schema.never()
),

featureSet: schema.object({
/**
Expand Down
3 changes: 3 additions & 0 deletions x-pack/test_serverless/functional/config.base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ export function createTestConfig(options: CreateTestConfigOptions) {
observability: {
pathname: '/app/observability',
},
management: {
pathname: '/app/management',
},
},
// choose where screenshots should be saved
screenshots: {
Expand Down
1 change: 1 addition & 0 deletions x-pack/test_serverless/functional/test_suites/search/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ import { FtrProviderContext } from '../../ftr_provider_context';
export default function ({ loadTestFile }: FtrProviderContext) {
describe('serverless search UI', function () {
loadTestFile(require.resolve('./landing_page'));
loadTestFile(require.resolve('./management'));
});
}
66 changes: 66 additions & 0 deletions x-pack/test_serverless/functional/test_suites/search/management.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getPageObject, getService }: FtrProviderContext) {
const commonPage = getPageObject('common');
const testSubjects = getService('testSubjects');

describe('Management', function () {
describe('Disabled UIs', () => {
const DISABLED_PLUGINS = [
{
appName: 'Upgrade Assistant',
url: '/stack/upgrade_assistant',
},
{
appName: 'Advanced Settings',
url: '/kibana/settings',
},
{
appName: 'Migrate',
url: '/data/migrate_data',
},
{
appName: 'Remote Clusters',
url: '/data/remote_clusters',
},
{
appName: 'Cross-Cluster Replication',
url: '/data/cross_cluster_replication',
},
{
appName: 'Snapshot and Restore',
url: '/data/snapshot_restore',
},
{
appName: 'Index Lifecycle Management',
url: '/data/index_lifecycle_management',
},
{
appName: 'Rollup Jobs',
url: '/data/rollup_jobs',
},
{
appName: 'License Management',
url: '/stack/license_management',
},
];

DISABLED_PLUGINS.forEach(({ appName, url }) => {
it(`${appName} is not accessible`, async () => {
await commonPage.navigateToUrl('management', url, {
shouldUseHashForSubUrl: false,
});
// If the route doesn't exist, the user will be redirected back to the Management landing page
await testSubjects.exists('managementHome');
});
});
});
});
}