Skip to content

Commit

Permalink
[Saved Objects] Compatible mappings PR check (#148656)
Browse files Browse the repository at this point in the history
## Summary

This PR adds a technical control to prevent incompatible mappings
changes. These include:

1. Removing mapped fields. For the foreseeable future we require that
teams only introduce new fields - in short: this avoids the "reindex"
step in our migrations.
2. Changing the type of a field. We leverage ES to determine whether a
given set of mappings can be applied "on top" of another. Similarly,
this avoids the "reindex" step in migrations.

The above checks depend on a snapshot of the mappings from `main`, these
are the "current" mappings and are extracted from plugin code. This PR
will bootstrap `main` with an initial set of mappings extracted from
plugins (bulk of new lines added).

## The new CLI

See the added `README.md` for details on how the CLI works.

## How will it work?

Any new PR that introduces compatible mappings changes will result in a
new snapshot being captured, then merged to main for other PRs to merge
and run the same checks against (currently committing new snapshots
happens in the CI check so there is no manual step of maintaining the
snapshot).

## Additional

We should consider combining this CI check with the existing check in
`src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts`.
Hopefully we can automate the check such that no manual review is needed
from Core, not sure how we might cover the hash of the non-mappings
related fields. We could consider narrowing the Jest test to exclude
mappings.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: spalger <[email protected]>
  • Loading branch information
3 people authored Apr 27, 2023
1 parent a0cd724 commit e9197ad
Show file tree
Hide file tree
Showing 27 changed files with 3,598 additions and 4 deletions.
1 change: 1 addition & 0 deletions .buildkite/scripts/steps/checks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export DISABLE_BOOTSTRAP_VALIDATION=false
.buildkite/scripts/steps/checks/test_projects.sh
.buildkite/scripts/steps/checks/test_hardening.sh
.buildkite/scripts/steps/checks/ftr_configs.sh
.buildkite/scripts/steps/checks/saved_objects_compat_changes.sh
14 changes: 14 additions & 0 deletions .buildkite/scripts/steps/checks/saved_objects_compat_changes.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/usr/bin/env bash

set -euo pipefail

source .buildkite/scripts/common/util.sh

echo --- Check Mappings Update
cmd="node scripts/check_mappings_update"
if is_pr && ! is_auto_commit_disabled; then
cmd="$cmd --fix"
fi

eval "$cmd"
check_for_changed_files "$cmd" true
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ packages/kbn-cell-actions @elastic/security-threat-hunting-explore
src/plugins/chart_expressions/common @elastic/kibana-visualizations
packages/kbn-chart-icons @elastic/kibana-visualizations
src/plugins/charts @elastic/kibana-visualizations
packages/kbn-check-mappings-update-cli @elastic/kibana-core
packages/kbn-ci-stats-core @elastic/kibana-operations
packages/kbn-ci-stats-performance-metrics @elastic/kibana-operations
packages/kbn-ci-stats-reporter @elastic/kibana-operations
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,7 @@
"@kbn/babel-register": "link:packages/kbn-babel-register",
"@kbn/babel-transform": "link:packages/kbn-babel-transform",
"@kbn/bazel-runner": "link:packages/kbn-bazel-runner",
"@kbn/check-mappings-update-cli": "link:packages/kbn-check-mappings-update-cli",
"@kbn/ci-stats-core": "link:packages/kbn-ci-stats-core",
"@kbn/ci-stats-performance-metrics": "link:packages/kbn-ci-stats-performance-metrics",
"@kbn/ci-stats-reporter": "link:packages/kbn-ci-stats-reporter",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* 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 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* @internal
*/
export const ENABLE_ALL_PLUGINS_CONFIG_PATH = 'forceEnableAllPlugins' as const;

/**
* Set this to true in the raw configuration passed to {@link Root} to force
* enable all plugins.
* @internal
*/
export const PLUGIN_SYSTEM_ENABLE_ALL_PLUGINS_CONFIG_PATH =
`plugins.${ENABLE_ALL_PLUGINS_CONFIG_PATH}` as const;
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,15 @@ describe('PluginsConfig', () => {
const config = new PluginsConfig(rawConfig, env);
expect(config.additionalPluginPaths).toEqual(['some-path', 'another-path']);
});

it('retrieves shouldEnableAllPlugins', () => {
const env = Env.createDefault(REPO_ROOT, getEnvOptions({ cliArgs: { dev: true } }));
const rawConfig: any = {
initialize: true,
paths: ['some-path', 'another-path'],
forceEnableAllPlugins: true,
};
const config = new PluginsConfig(rawConfig, env);
expect(config.shouldEnableAllPlugins).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,29 @@
*/

import { schema, TypeOf } from '@kbn/config-schema';
import { get } from 'lodash';
import { Env } from '@kbn/config';
import type { ServiceConfigDescriptor } from '@kbn/core-base-server-internal';

import { ENABLE_ALL_PLUGINS_CONFIG_PATH } from './constants';

const configSchema = schema.object({
initialize: schema.boolean({ defaultValue: true }),

/**
* Defines an array of directories where another plugin should be loaded from.
*/
paths: schema.arrayOf(schema.string(), { defaultValue: [] }),
/**
* Internal config, not intended to be used by end users. Only for specific
* internal purposes.
*/
forceEnableAllPlugins: schema.maybe(schema.boolean({ defaultValue: false })),
});

export type PluginsConfigType = TypeOf<typeof configSchema>;
type InternalPluginsConfigType = TypeOf<typeof configSchema>;

export type PluginsConfigType = Omit<InternalPluginsConfigType, '__internal__'>;

export const config: ServiceConfigDescriptor<PluginsConfigType> = {
path: 'plugins',
Expand All @@ -43,9 +53,17 @@ export class PluginsConfig {
*/
public readonly additionalPluginPaths: readonly string[];

/**
* Whether to enable all plugins.
*
* @note this is intended to be an undocumented setting.
*/
public readonly shouldEnableAllPlugins: boolean;

constructor(rawConfig: PluginsConfigType, env: Env) {
this.initialize = rawConfig.initialize;
this.pluginSearchPaths = env.pluginSearchPaths;
this.additionalPluginPaths = rawConfig.paths;
this.shouldEnableAllPlugins = get(rawConfig, ENABLE_ALL_PLUGINS_CONFIG_PATH, false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ import { PluginDiscoveryError } from './discovery';
import { PluginWrapper } from './plugin';
import { PluginsService } from './plugins_service';
import { PluginsSystem } from './plugins_system';
import { config } from './plugins_config';
import { config, PluginsConfigType } from './plugins_config';
import { take } from 'rxjs/operators';
import type { PluginConfigDescriptor } from '@kbn/core-plugins-server';
import { DiscoveredPlugin, PluginType } from '@kbn/core-base-common';

const MockPluginsSystem: jest.Mock<PluginsSystem<PluginType>> = PluginsSystem as any;

let pluginsService: PluginsService;
let pluginsConfig: PluginsConfigType;
let config$: BehaviorSubject<Record<string, any>>;
let configService: ConfigService;
let coreId: symbol;
Expand Down Expand Up @@ -130,7 +131,8 @@ async function testSetup() {
coreId = Symbol('core');
env = Env.createDefault(REPO_ROOT, getEnvOptions());

config$ = new BehaviorSubject<Record<string, any>>({ plugins: { initialize: true } });
pluginsConfig = { initialize: true, paths: [] };
config$ = new BehaviorSubject<Record<string, any>>({ plugins: pluginsConfig });
const rawConfigService = rawConfigServiceMock.create({ rawConfig$: config$ });
configService = new ConfigService(rawConfigService, env, logger);
await configService.setSchema(config.path, config.schema);
Expand Down Expand Up @@ -496,6 +498,52 @@ describe('PluginsService', () => {
`);
});

describe('forceEnableAllPlugins', () => {
it('enables all plugins when "true"', async () => {
(pluginsConfig as any).forceEnableAllPlugins = true;
jest
.spyOn(configService, 'isEnabledAtPath')
.mockImplementation((path) => Promise.resolve(!path.includes('disabled')));
prebootMockPluginSystem.setupPlugins.mockResolvedValue(new Map());
standardMockPluginSystem.setupPlugins.mockResolvedValue(new Map());
await pluginsService.setup(setupDeps);

mockDiscover.mockReturnValue({
error$: from([]),
plugin$: from([
createPlugin('explicitly-disabled-plugin-preboot', {
type: PluginType.preboot,
disabled: true,
path: 'path-1-preboot',
configPath: 'path-1-preboot',
}),
createPlugin('explicitly-disabled-plugin-standard', {
disabled: true,
path: 'path-1-standard',
configPath: 'path-1-standard',
}),
createPlugin('plugin-with-missing-required-deps-preboot', {
type: PluginType.preboot,
path: 'path-2-preboot',
configPath: 'path-2-preboot',
requiredPlugins: ['missing-plugin-preboot'],
}),
]),
});

await pluginsService.discover({ environment: environmentPreboot, node: nodePreboot });
await pluginsService.preboot(prebootDeps);

expect(loggingSystemMock.collect(logger).info).toMatchInlineSnapshot(`
Array [
Array [
"Plugin \\"plugin-with-missing-required-deps-preboot\\" has been disabled since the following direct or transitive dependencies are missing, disabled, or have incompatible types: [missing-plugin-preboot]",
],
]
`);
});
});

it('does not throw in case of mutual plugin dependencies', async () => {
const prebootPlugins = [
createPlugin('first-plugin-preboot', {
Expand Down Expand Up @@ -725,6 +773,7 @@ describe('PluginsService', () => {
resolve(REPO_ROOT, '..', 'kibana-extra'),
resolve(REPO_ROOT, 'plugins'),
],
shouldEnableAllPlugins: false,
},
coreContext: { coreId, env, logger, configService },
instanceInfo: { uuid: 'uuid' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,19 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
}
}

const config = await firstValueFrom(this.config$);
const enableAllPlugins = config.shouldEnableAllPlugins;
if (enableAllPlugins) {
this.log.warn('Detected override configuration; will enable all plugins');
}

// Validate config and handle enabled statuses.
// NOTE: We can't do both in the same previous loop because some plugins' deprecations may affect others.
// Hence, we need all the deprecations to be registered before accessing any config parameter.
for (const plugin of plugins) {
const isEnabled = await this.coreContext.configService.isEnabledAtPath(plugin.configPath);
const isEnabled =
enableAllPlugins ||
(await this.coreContext.configService.isEnabledAtPath(plugin.configPath));

if (pluginEnableStatuses.has(plugin.name)) {
throw new Error(`Plugin with id "${plugin.name}" is already registered!`);
Expand Down
7 changes: 7 additions & 0 deletions packages/kbn-check-mappings-update-cli/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# @kbn/check-mappings-update-cli

Saved Objects CLI tool that can be used to check whether a snapshot of current
mappings (i.e., mappings on main) is compatible with mappings we can extract
from the current code.

See `node scripts/check_mappings_update --help` for more info.
Loading

0 comments on commit e9197ad

Please sign in to comment.