Add deprecation warning when inline scripting is disabled #111865

pgayvallet · 2021-09-10T14:09:06Z

Summary

Fix #110566

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-09-10T15:15:27Z

Pinging @elastic/kibana-core (Team:Core)

pgayvallet · 2021-09-10T15:16:03Z

src/core/server/elasticsearch/deprecations/is_scripting_disabled.test.ts

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


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

pgayvallet · 2021-09-10T15:16:47Z

src/core/server/elasticsearch/elasticsearch_service.ts

+export interface SetupDeps {
  http: InternalHttpServiceSetup;
+  deprecations: InternalDeprecationsServiceSetup;
  executionContext: InternalExecutionContextSetup;
 }


Only exported to ease testing

pgayvallet · 2021-09-10T20:31:14Z

src/core/server/elasticsearch/deprecations/scripting_disabled_deprecation.ts

+      title: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.title', {
+        defaultMessage: 'Inline scripting is disabled on elasticsearch',
+      }),
+      message: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.message', {
+        defaultMessage:
+          'Starting in 8.0, Kibana will require inline scripting to be enabled,' +
+          'and will fail to start otherwise.',
+      }),


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

pgayvallet · 2021-09-10T20:40:50Z

src/core/server/elasticsearch/deprecations/is_scripting_disabled.ts

+  const scriptAllowedTypes: string[] =
+    settings.transient[scriptAllowedTypesKey] ??
+    settings.persistent[scriptAllowedTypesKey] ??
+    settings.defaults![scriptAllowedTypesKey] ??
+    [];


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

pgayvallet · 2021-09-10T20:41:19Z

retest

…ing-disabled-deprecation

kibanamachine · 2021-09-13T08:41:13Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #152657 succeeded 817156c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…1865) * initial implementation * extract isScriptingDisabled to own file * add unit tests for scripting deprecation * add unit tests

kibanamachine · 2021-09-13T10:18:44Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…111932) * initial implementation * extract isScriptingDisabled to own file * add unit tests for scripting deprecation * add unit tests

…111933) * initial implementation * extract isScriptingDisabled to own file * add unit tests for scripting deprecation * add unit tests Co-authored-by: Pierre Gayvallet <pierre.gayvallet@gmail.com>

cjcenizal · 2021-10-09T21:22:44Z

src/core/server/elasticsearch/deprecations/scripting_disabled_deprecation.ts

+      title: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.title', {
+        defaultMessage: 'Inline scripting is disabled on elasticsearch',
+      }),
+      message: i18n.translate('core.elasticsearch.deprecations.scriptingDisabled.message', {


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

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

You can find more guidelines here: #109166

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?

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

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.

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. :)

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.

gchaps · 2021-10-13T22:08:37Z

Based on the comments above and the guidelines for writing deprecations, here is an edit of the text:

Inline scripting is disabled on Elasticsearch (capitalize the E in Elasticsearch)
Kibana uses inline scripting to manage internal data that is stored in Elasticsearch. Enable inline scripting to continue using Kibana in 8.0.
Set "script.allowed_types=inline" in your Elasticsearch configuration.

Only specify a doc URL if the user truly needs to “learn more” to understand what actions they need to take. I think the line above is enough.

pgayvallet · 2021-10-14T09:11:59Z

Thanks a lot for your help @gchaps

Set "script.allowed_types=inline" in your Elasticsearch configuration.

Setting script.allowed_types=inline is potentially dangerous if their previous configuration was script.allowed_types=stored, as it would block stored scripts from executing, therefor breaking things for some of their usages. (would change from [stored] to [inline])

We could guide them to remove script.allowed_types from their config instead (which effectively enables both inline and stored script. That would work if their previous config setting was script.allowed_types=stored, as it could change from [stored] to [stored, inline]. But if their config was script.allowed_types=none, they'll probably want to set it to script.allowed_types=inline instead, to preserve blocking stored scripts.

Only specify a doc URL if the user truly needs to “learn more” to understand what actions they need to take. I think the line above is enough.

Given my previous paragraph, I do thing they do here, to properly decide if they need to set script.allowed_types=inline or to totally remove the script.allowed_types entry from the config. Again, the default ES behavior is to allow both kind of scripts, so this deprecation will only be displayed if the admin explicitely decided to block script. If they did, we can assume they had a very valid reason to do so, so we probably want them to decide how exactly they want to change their config themselves instead of pushing one option or the other?

(but I may be overthinking this, if you still think you edit is good after my explanations, just tell me and I'll perform the changes)

gchaps

@pgayvallet The reason I included that line in the list of edits is because I saw the copy in the scripting_disable_deprecation.ts file. My intent was to init capitalize Elasticsearch.

What you said is good, as well as linking to the doc.

pgayvallet added 4 commits September 10, 2021 14:52

initial implementation

56545f7

extract isScriptingDisabled to own file

5f7bb36

add unit tests for scripting deprecation

dba789e

add unit tests

817156c

pgayvallet added Feature:elasticsearch Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.16.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 10, 2021

pgayvallet marked this pull request as ready for review September 10, 2021 15:15

pgayvallet requested a review from a team as a code owner September 10, 2021 15:15

Bamieh approved these changes Sep 10, 2021

View reviewed changes

pgayvallet commented Sep 10, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-110566-script…

1bebf8b

…ing-disabled-deprecation

pgayvallet merged commit 200efca into elastic:master Sep 13, 2021

pgayvallet added auto-backport Deprecated - use backport:version if exact versions are needed and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Sep 13, 2021

pgayvallet mentioned this pull request Sep 13, 2021

[7.x] Add deprecation warning when inline scripting is disabled (#111865) #111932

Merged

kibanamachine mentioned this pull request Sep 13, 2021

[7.x] Add deprecation warning when inline scripting is disabled (#111865) #111933

Merged

pgayvallet added a commit that referenced this pull request Sep 13, 2021

Add deprecation warning when inline scripting is disabled (#111865) (#…

ae7e262

…111932) * initial implementation * extract isScriptingDisabled to own file * add unit tests for scripting deprecation * add unit tests

cjcenizal reviewed Oct 9, 2021

View reviewed changes

gchaps reviewed Oct 14, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation warning when inline scripting is disabled #111865

Add deprecation warning when inline scripting is disabled #111865

pgayvallet commented Sep 10, 2021 •

edited

Loading

elasticmachine commented Sep 10, 2021

pgayvallet Sep 10, 2021

pgayvallet Sep 10, 2021

pgayvallet Sep 10, 2021

pgayvallet Sep 10, 2021

pgayvallet commented Sep 10, 2021

kibanamachine commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

cjcenizal Oct 9, 2021

cjcenizal Oct 9, 2021

cjcenizal Oct 9, 2021

pgayvallet Oct 11, 2021

cjcenizal Oct 11, 2021 •

edited

Loading

pgayvallet Oct 12, 2021 •

edited

Loading

cjcenizal Oct 12, 2021

kobelb Oct 12, 2021

gchaps commented Oct 13, 2021

pgayvallet commented Oct 14, 2021

gchaps left a comment •

edited

Loading

Add deprecation warning when inline scripting is disabled #111865

Add deprecation warning when inline scripting is disabled #111865

Conversation

pgayvallet commented Sep 10, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Sep 10, 2021

kibanamachine commented Sep 13, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Sep 13, 2021

💚 Backport successful

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchaps commented Oct 13, 2021

pgayvallet commented Oct 14, 2021

gchaps left a comment • edited Loading

Choose a reason for hiding this comment

pgayvallet commented Sep 10, 2021 •

edited

Loading

cjcenizal Oct 11, 2021 •

edited

Loading

pgayvallet Oct 12, 2021 •

edited

Loading

gchaps left a comment •

edited

Loading