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.

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 deprecation warning when inline scripting is disabled #111865

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions src/core/server/elasticsearch/deprecations/deprecation_provider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* 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.
*/

import type { RegisterDeprecationsConfig } from '../../deprecations';
import { getScriptingDisabledDeprecations } from './scripting_disabled_deprecation';

export const getElasticsearchDeprecationsProvider = (): RegisterDeprecationsConfig => {
return {
getDeprecations: async (context) => {
return [...(await getScriptingDisabledDeprecations({ esClient: context.esClient }))];
},
};
};
9 changes: 9 additions & 0 deletions src/core/server/elasticsearch/deprecations/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* 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.
*/

export { getElasticsearchDeprecationsProvider } from './deprecation_provider';
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* 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.
*/

import { estypes } from '@elastic/elasticsearch';
import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_service.mock';
import { isInlineScriptingDisabled } from './is_scripting_disabled';

describe('isInlineScriptingDisabled', () => {
let client: ReturnType<typeof elasticsearchServiceMock.createElasticsearchClient>;

beforeEach(() => {
client = elasticsearchServiceMock.createElasticsearchClient();
});

const mockSettingsValue = (settings: estypes.ClusterGetSettingsResponse) => {
client.cluster.getSettings.mockReturnValue(
elasticsearchServiceMock.createSuccessTransportRequestPromise(settings)
);
};

it('returns `false` if all settings are empty', async () => {
mockSettingsValue({
transient: {},
persistent: {},
defaults: {},
});

expect(await isInlineScriptingDisabled({ client })).toEqual(false);
});

it('returns `false` if `defaults.script.allowed_types` is `inline`', async () => {
mockSettingsValue({
transient: {},
persistent: {},
defaults: {
'script.allowed_types': ['inline'],
},
});
Comment on lines +36 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure of the value of unit tests given that the PR also adds integration tests, but anyway...


expect(await isInlineScriptingDisabled({ client })).toEqual(false);
});

it('returns `true` if `defaults.script.allowed_types` is `none`', async () => {
mockSettingsValue({
transient: {},
persistent: {},
defaults: {
'script.allowed_types': ['none'],
},
});

expect(await isInlineScriptingDisabled({ client })).toEqual(true);
});

it('returns `true` if `defaults.script.allowed_types` is `stored`', async () => {
mockSettingsValue({
transient: {},
persistent: {},
defaults: {
'script.allowed_types': ['stored'],
},
});

expect(await isInlineScriptingDisabled({ client })).toEqual(true);
});

it('respect the persistent->defaults priority', async () => {
mockSettingsValue({
transient: {},
persistent: {
'script.allowed_types': ['inline'],
},
defaults: {
'script.allowed_types': ['stored'],
},
});

expect(await isInlineScriptingDisabled({ client })).toEqual(false);
});

it('respect the transient->persistent priority', async () => {
mockSettingsValue({
transient: {
'script.allowed_types': ['stored'],
},
persistent: {
'script.allowed_types': ['inline'],
},
defaults: {},
});

expect(await isInlineScriptingDisabled({ client })).toEqual(true);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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.
*/

import { ElasticsearchClient } from '../../elasticsearch';

const scriptAllowedTypesKey = 'script.allowed_types';

export const isInlineScriptingDisabled = async ({
client,
}: {
client: ElasticsearchClient;
}): Promise<boolean> => {
const { body: settings } = await client.cluster.getSettings({
include_defaults: true,
flat_settings: true,
});

// priority: transient -> persistent -> default
const scriptAllowedTypes: string[] =
settings.transient[scriptAllowedTypesKey] ??
settings.persistent[scriptAllowedTypesKey] ??
settings.defaults![scriptAllowedTypesKey] ??
[];
Comment on lines +24 to +28
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 doubt transient and persistent settings are that much used for things like script.allowed_types, but just in case, we follow ES's priority for settings


// when unspecified, the setting as a default `[]` value that means that both scriptings are allowed.
const scriptAllowed = scriptAllowedTypes.length === 0 || scriptAllowedTypes.includes('inline');

return !scriptAllowed;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* 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.
*/

export const isInlineScriptingDisabledMock = jest.fn();
jest.doMock('./is_scripting_disabled', () => ({
isInlineScriptingDisabled: isInlineScriptingDisabledMock,
}));
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* 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.
*/

import { isInlineScriptingDisabledMock } from './scripting_disabled_deprecation.test.mocks';
import { elasticsearchServiceMock } from '../../elasticsearch/elasticsearch_service.mock';
import { getScriptingDisabledDeprecations } from './scripting_disabled_deprecation';

describe('getScriptingDisabledDeprecations', () => {
let esClient: ReturnType<typeof elasticsearchServiceMock.createScopedClusterClient>;

beforeEach(() => {
esClient = elasticsearchServiceMock.createScopedClusterClient();
});

afterEach(() => {
isInlineScriptingDisabledMock.mockReset();
});

it('calls `isInlineScriptingDisabled` with the correct arguments', async () => {
await getScriptingDisabledDeprecations({
esClient,
});

expect(isInlineScriptingDisabledMock).toHaveBeenCalledTimes(1);
expect(isInlineScriptingDisabledMock).toHaveBeenCalledWith({
client: esClient.asInternalUser,
});
});

it('returns no deprecations if scripting is not disabled', async () => {
isInlineScriptingDisabledMock.mockResolvedValue(false);

const deprecations = await getScriptingDisabledDeprecations({
esClient,
});

expect(deprecations).toHaveLength(0);
});

it('returns a deprecation if scripting is disabled', async () => {
isInlineScriptingDisabledMock.mockResolvedValue(true);

const deprecations = await getScriptingDisabledDeprecations({
esClient,
});

expect(deprecations).toHaveLength(1);
expect(deprecations[0]).toEqual({
title: expect.any(String),
message: expect.any(String),
level: 'critical',
requireRestart: false,
correctiveActions: {
manualSteps: expect.any(Array),
},
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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.
*/

import { i18n } from '@kbn/i18n';
import type { DeprecationsDetails } from '../../deprecations';
import { IScopedClusterClient } from '../../elasticsearch';
import { isInlineScriptingDisabled } from './is_scripting_disabled';

interface GetScriptingDisabledDeprecations {
esClient: IScopedClusterClient;
}

export const getScriptingDisabledDeprecations = async ({
esClient,
}: GetScriptingDisabledDeprecations): Promise<DeprecationsDetails[]> => {
const deprecations: DeprecationsDetails[] = [];
if (await isInlineScriptingDisabled({ client: esClient.asInternalUser })) {
deprecations.push({
title: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.title', {
defaultMessage: 'Inline scripting is disabled on elasticsearch',
}),
message: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.message', {
Copy link
Contributor

Choose a reason for hiding this comment

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

@pgayvallet I don't think this message adequately explains what inline scripting is. Ideally, we want to communicate to the user the benefits of taking an action. At minimum, we want to explain the reason behind the deprecation so the user feels in control and like they know why they're taking action.

Also, are there any docs we can link to that will help the user? For example https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-scripting-security.html ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please ping a writer for help with copy (@debadair or @gchaps).

Copy link
Contributor

Choose a reason for hiding this comment

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

You can find more guidelines here: #109166

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we want to communicate to the user the benefits of taking an action

Having Kibana properly boot in 8.0+ 😄 ?

At minimum, we want to explain the reason behind the deprecation

Imho we should not spam the user with implementation details. Inline scripting is required for technical, not functional, reasons. I really don't think an end user should have more details on what we're using inline scripting for? If you think otherwise, would you mind telling me what you think we should inform the user of regarding this new restriction?

Copy link
Contributor

@cjcenizal cjcenizal Oct 11, 2021

Choose a reason for hiding this comment

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

@pgayvallet Sure, I'd love to help. Can you tell me what Kibana is using inline scripting for? From #96106, it sounds like there are certain features that depend upon it, but I coudn't find any mention of specific features. If a user has disabled inline scripting, we should assume they have a good reason for it and give them information they can use to make tradeoffs, or to provide us with informed feedback.

Copy link
Contributor Author

@pgayvallet pgayvallet Oct 12, 2021

Choose a reason for hiding this comment

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

Can you tell me what Kibana is using inline scripting for?

Can answer for core features: some SO apis are using inline scripts under the hood (such as, from a quick grep, deleteByNamespace, removeReferencesTo, incrementCounter)

However, core is not an isolated case. Some plugins / solutions are using inline scripting too for some ES queries, but unfortunately I lack the technical / functional knowledge to give you more details. Maybe @kobelb can help answering this question.

TBH, even in 7.x, Kibana does not properly function without scripting enabled. We just chose to kinda ignore it because we weren't allowed to introduce breaking changes in minors, but some features (not only core's) can't be implemented without inline scripting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this great info! I'd suggest reframing the deprecation message to be something like, "In 8.0, Kibana uses inline scripting to create and manage saved objects, which are used for storing visualizations, dashboards, and other artifacts. Please enable inline scripting to continue using Kibana in 8.0."

My point is that our users choose to use Elastic and Kibana, and we're lucky to have them. Because we can't be in the room with them when they're upgrading, we have to use these kinds of messages to address any questions or concerns they might have, and we need to convey empathy and respect in how we phrase them. I know this can seem like a lot of effort for small messages like these, but the effect is cumulative -- and users will either be left with a positive impression or a negative one. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Task-manager also uses scripting to claim tasks, so a whole number of things could fail. I'd suggest we change the deprecation to the following:

Kibana uses inline scripting to efficiently manage its internal data that is stored in Elasticsearch. Please enable inline scripting to continue using Kibana in 8.0.

defaultMessage:
'Starting in 8.0, Kibana will require inline scripting to be enabled,' +
'and will fail to start otherwise.',
}),
Comment on lines +24 to +31
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improvements on the deprecation's title/message/manualSteps are welcome

level: 'critical',
requireRestart: false,
correctiveActions: {
manualSteps: [
i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.manualSteps.1', {
defaultMessage: 'Set `script.allowed_types=inline` in your elasticsearch config ',
}),
],
},
});
}
return deprecations;
};
39 changes: 31 additions & 8 deletions src/core/server/elasticsearch/elasticsearch_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,39 @@ import { loggingSystemMock } from '../logging/logging_system.mock';
import { httpServiceMock } from '../http/http_service.mock';
import { executionContextServiceMock } from '../execution_context/execution_context_service.mock';
import { configSchema, ElasticsearchConfig } from './elasticsearch_config';
import { ElasticsearchService } from './elasticsearch_service';
import { ElasticsearchService, SetupDeps } from './elasticsearch_service';
import { elasticsearchClientMock } from './client/mocks';
import { deprecationsServiceMock } from '../deprecations/deprecations_service.mock';
import { duration } from 'moment';
import { isValidConnection as isValidConnectionMock } from './is_valid_connection';
import { pollEsNodesVersion as pollEsNodesVersionMocked } from './version_check/ensure_es_version';

const { pollEsNodesVersion: pollEsNodesVersionActual } = jest.requireActual(
'./version_check/ensure_es_version'
);

const delay = async (durationMs: number) =>
await new Promise((resolve) => setTimeout(resolve, durationMs));

let elasticsearchService: ElasticsearchService;
const configService = configServiceMock.create();
const setupDeps = {
http: httpServiceMock.createInternalSetupContract(),
executionContext: executionContextServiceMock.createInternalSetupContract(),
};

let elasticsearchService: ElasticsearchService;
let env: Env;
let coreContext: CoreContext;

let mockClusterClientInstance: ReturnType<typeof elasticsearchClientMock.createCustomClusterClient>;

let mockConfig$: BehaviorSubject<any>;
let setupDeps: SetupDeps;
let deprecationsSetup: ReturnType<typeof deprecationsServiceMock.createInternalSetupContract>;

beforeEach(() => {
deprecationsSetup = deprecationsServiceMock.createInternalSetupContract();

setupDeps = {
http: httpServiceMock.createInternalSetupContract(),
executionContext: executionContextServiceMock.createInternalSetupContract(),
deprecations: deprecationsSetup,
};

env = Env.createDefault(REPO_ROOT, getEnvOptions());

mockConfig$ = new BehaviorSubject({
Expand Down Expand Up @@ -174,6 +181,22 @@ describe('#setup', () => {
);
});

it('registers its deprecation provider', async () => {
const registry = deprecationsServiceMock.createSetupContract();

deprecationsSetup.getRegistry.mockReturnValue(registry);

await elasticsearchService.setup(setupDeps);

expect(deprecationsSetup.getRegistry).toHaveBeenCalledTimes(1);
expect(deprecationsSetup.getRegistry).toHaveBeenCalledWith('elasticsearch');

expect(registry.registerDeprecations).toHaveBeenCalledTimes(1);
expect(registry.registerDeprecations).toHaveBeenCalledWith({
getDeprecations: expect.any(Function),
});
});

it('esNodeVersionCompatibility$ only starts polling when subscribed to', async (done) => {
const mockedClient = mockClusterClientInstance.asInternalUser;
mockedClient.nodes.info.mockImplementation(() =>
Expand Down
10 changes: 9 additions & 1 deletion src/core/server/elasticsearch/elasticsearch_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { ClusterClient, ElasticsearchClientConfig } from './client';
import { ElasticsearchConfig, ElasticsearchConfigType } from './elasticsearch_config';
import type { InternalHttpServiceSetup, GetAuthHeaders } from '../http';
import type { InternalExecutionContextSetup, IExecutionContext } from '../execution_context';
import type { InternalDeprecationsServiceSetup } from '../deprecations';
import {
InternalElasticsearchServicePreboot,
InternalElasticsearchServiceSetup,
Expand All @@ -27,9 +28,11 @@ import type { NodesVersionCompatibility } from './version_check/ensure_es_versio
import { pollEsNodesVersion } from './version_check/ensure_es_version';
import { calculateStatus$ } from './status';
import { isValidConnection } from './is_valid_connection';
import { getElasticsearchDeprecationsProvider } from './deprecations';

interface SetupDeps {
export interface SetupDeps {
http: InternalHttpServiceSetup;
deprecations: InternalDeprecationsServiceSetup;
executionContext: InternalExecutionContextSetup;
}
Comment on lines +33 to 37
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only exported to ease testing


Expand Down Expand Up @@ -78,6 +81,10 @@ export class ElasticsearchService
this.executionContextClient = deps.executionContext;
this.client = this.createClusterClient('data', config);

deps.deprecations
.getRegistry('elasticsearch')
.registerDeprecations(getElasticsearchDeprecationsProvider());

const esNodesCompatibility$ = pollEsNodesVersion({
internalClient: this.client.asInternalUser,
log: this.log,
Expand All @@ -96,6 +103,7 @@ export class ElasticsearchService
status$: calculateStatus$(esNodesCompatibility$),
};
}

public async start(): Promise<InternalElasticsearchServiceStart> {
if (!this.client || !this.esNodesCompatibility$) {
throw new Error('ElasticsearchService needs to be setup before calling start');
Expand Down
Loading