-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from all commits
56545f7
5f7bb36
dba789e
817156c
1bebf8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 }))]; | ||
}, | ||
}; | ||
}; |
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'], | ||
}, | ||
}); | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I doubt |
||
|
||
// 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', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can find more guidelines here: #109166 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Having Kibana properly boot in 8.0+ 😄 ?
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can answer for core features: some SO apis are using inline scripts under the hood (such as, from a quick grep, 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only exported to ease testing |
||
|
||
|
@@ -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, | ||
|
@@ -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'); | ||
|
There was a problem hiding this comment.
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...