[Alerts][Actions] Removed isESOUsingEphemeralEncryptionKey determination. Set ESO plugin as a required dependency for Actions and Alerts plugins. Changed UI to show the proper info for disabled ESO plugin. #87936

YulNaumenko · 2021-01-11T22:23:31Z

Removed usingEphemeralEncryptionKey check for alerts and actions plugins.
This is needed due to the upcoming changes in the ESO plugin, where the plugin won't be enabled if encryptionKey was not set: 'enabled cannot be set to true until encryptionKey is specified.'

If ESO is disabled, alerts and actions plugins are disabled too with the proper Kibana start log messages:

Extended triggers_actions_ui plugin to have an own health api, which return the current state based on the different factors like availability of the related plugins.
Used this health API to replace the obsolete alerting framework health option hasPermanentEncryptionKey.
Modified UI warning message from:

to:

Resolves #80678

…ion. Set ESO plugin as an optional dependancy for actions and alerts plugins.

elasticmachine · 2021-01-12T19:53:11Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

gchaps · 2021-01-12T20:03:39Z

Suggested text:

Encrypted saved objects are not available

Set a value for xpack.encryptionSavedObjects.encryptionKey in your kibana.yml file and ensure the Encrypted Saved Objects plugin is enabled. Learn how.

ymao1

LGTM!

x-pack/plugins/actions/server/lib/action_executor.ts

x-pack/plugins/actions/server/plugin.ts

gmmorris · 2021-01-13T18:05:01Z

x-pack/plugins/actions/kibana.json

-  "requiredPlugins": ["licensing", "taskManager", "encryptedSavedObjects", "eventLog", "features"],
-  "optionalPlugins": ["usageCollection", "spaces", "security"],
+  "requiredPlugins": ["licensing", "taskManager", "eventLog", "features"],
+  "optionalPlugins": ["usageCollection", "spaces", "security", "encryptedSavedObjects"],


What's the expected behaviour when ESO is disabled?
Should Actions and Alerts still works without encryption?
If not, seems like this should still be in requiredPlugins.
If so, then seems like we're missing a whole test suite of e2e tests where Alerting & Actions run without ESOs

What's the expected behaviour when ESO is disabled?

In the alerting management and in the API responses we show the message that the user should configure ESO (like it was done for ephemeral encryption key). So I kept the same behavior.

Should Actions and Alerts still works without encryption?

Actions and Alerts won't normally work without the encryption. For the existing alerts and actions executions user will get the error messages, where we are proposing to the user to enable the ESO plugin by the link Learn how in the UI message and configuration link in the API message.

If so, then seems like we're missing a whole test suite of e2e tests where Alerting & Actions run without ESOs

Alerts and actions cannot run without ESO.
ESO is in the optional plugins list, because the users should be able to get the info about possible missing configuration, I think..
Do you think we should disable it at all?
To clarify: this PR changes is needed because there won't be support for ephemeral encryption key any more. But currently, with this key our framework is also not working properly for create and execute and we are not disabling plugins and just saying to the user that he should be able to change the configuration to make it works normal.

I suspect that if we leave it as a required plugin, we'll end up with no alert/action UI at all, and the only diagnostic would be the list of plugins listed at Kibana startup, where alerts/actions wouldn't be listed. Perhaps some messaging that the plugins were disabled because a required plugin wasn't available? Or would Kibana just not start up at that point?

So seems odd, but the only reason to keep the alerts/actions plugins loadable is to display the message. Which is probably important enough that we'd want to make this optional instead of required. Presumably some read-only activities could still work (maybe not now, but could be made to work?)? Eg, listing alerts, alert instances?

@elastic/apm-ui, @elastic/siem, @elastic/uptime please let us know here if you have any objections about disabling alerts and actions plugins if encrypted_saved_objects is disabled.

Will this impact if/when we move the alerting UI code to the alerts plugin?

Good point. Yes, it will be disabled as well..

I don't see how we have much of a choice here, this is required for things to be secure right? Is there any action we need to take on Uptime? If not, of course I support keeping things secure.

@andrewvc just want to let you know that if we decide to make ESO as a required dependency for alerts and actions and uptime app will keep the alerts as required plugin it will be disabled as well on Kibana start.

pmuellr · 2021-01-13T20:32:24Z

I'm a little concerned on the timing of #81511 - which is the PR which changes the ESO plugin behavior. I believe that all the plugins depending on ESO will need to switch within the same Kibana release, so we should double check that all the other plugins are ready for 7.12, before merging. Even when we do switch, there will be some span between when the ESO-using plugins land and #81511 lands, in which the encryptionKey check won't be made, as the ESO-using plugins will always think it's available, but you'll get a different key on each run and so see decryption errors. That seems ok-ish, it would only be a problem for elasticians I think, and only a short while.

YulNaumenko · 2021-01-13T20:36:06Z

I'm a little concerned on the timing of #81511 - which is the PR which changes the ESO plugin behavior. I believe that all the plugins depending on ESO will need to switch within the same Kibana release, so we should double check that all the other plugins are ready for 7.12, before merging.

There are only our plugins and fleet plugin which is relying onto the ESO.

gmmorris

This is much better than the original approach!
Looks like we still have some leftover from that though? Looks like we now have some places where values are nullable though they don't need to be? 🤔 I've added notes in all of them :)

gmmorris · 2021-01-21T10:50:48Z

x-pack/plugins/actions/server/plugin.test.ts

    describe('routeHandlerContext.getActionsClient()', () => {
-      it('should not throw error when ESO plugin not using a generated key', async () => {


Now that actions is dependent on ESO I think we can remove this all together, no?
It should never be possible for getActionsClient to be available when ESO is not... right? 🤔

gmmorris · 2021-01-21T10:51:22Z

x-pack/plugins/actions/server/plugin.test.ts

@@ -227,34 +191,19 @@ describe('Actions Plugin', () => {
        expect(pluginStart.isActionExecutable('preconfiguredServerLog', '.server-log')).toBe(true);
      });

-      it('should not throw error when ESO plugin not using a generated key', async () => {
+      it('should not throw error when ESO plugin is available', async () => {


Same with this test... I think we can remove it altogether now, no?

gmmorris · 2021-01-21T10:54:41Z

x-pack/plugins/actions/server/plugin.ts

+    const encryptedSavedObjectsClient = plugins.encryptedSavedObjects.getClient({
+      includedHiddenTypes,
+    });
+


This code used to reside outside of getActionsClientWithRequest to prevent us from creating a new encryptedSavedObjectsClient every time an ActionsClient is created.
Is there a reason why this has been moved in now? 🤔

gmmorris · 2021-01-21T10:55:56Z

x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts

+          if (!encryptedSavedObjects) {
+            throw new Error(
+              `Unable to run task because the Encrypted Saved Objects plugin is not available. Please set xpack.encryptedSavedObjects.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.`
+            );
+          }


This shouldn't happen, right? Now that it's a required plugin it should always be there, so this is redundant code (unless I'm missing something)

gmmorris · 2021-01-21T10:58:42Z

x-pack/plugins/alerts/server/health/get_health.ts

+  if (!internalSavedObjectsRepository) {
+    healthStatuses.decryptionHealth = {
+      status: HealthStatus.Warning,
+      timestamp: new Date().toISOString(),
+    };
+    return healthStatuses;
+  }


Why have we made internalSavedObjectsRepository optional here? Am I missing something?
It doesn't look like we've changed the call to getHealth so this looks like it might be leftovers from the original approach?

Thank you for catching this leftovers!

kibanamachine · 2021-01-22T06:55:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b380f70

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	333	334	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	1.6MB	1.6MB	-3.5KB

History

💔 Build #100451 failed a65a64c
💔 Build #100434 failed 36f95fe
💔 Build #100235 failed 8d4d83e
💚 Build #99856 succeeded b88357b
💚 Build #98688 succeeded 7885a93

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

YulNaumenko · 2021-02-09T18:07:45Z

Closed in favor of the PR #81511 and a follow up issue

[Alerts][Actions] Changed isESOUsingEphemeralEncryptionKey determinat…

322fab4

…ion. Set ESO plugin as an optional dependancy for actions and alerts plugins.

YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 labels Jan 11, 2021

YulNaumenko self-assigned this Jan 11, 2021

YulNaumenko added 2 commits January 11, 2021 15:35

fixed faling typechecks

e74f4f4

fixed faling typechecks

fddb69c

YulNaumenko linked an issue Jan 12, 2021 that may be closed by this pull request

[alerting][actions] change isESOUsingEphemeralEncryptionKey determination #80678

Closed

YulNaumenko marked this pull request as ready for review January 12, 2021 19:53

YulNaumenko requested a review from a team as a code owner January 12, 2021 19:53

fixed health framework status message

628e20c

ymao1 approved these changes Jan 13, 2021

View reviewed changes

x-pack/plugins/actions/server/lib/action_executor.ts Outdated Show resolved Hide resolved

gmmorris reviewed Jan 13, 2021

View reviewed changes

x-pack/plugins/actions/server/plugin.ts Outdated Show resolved Hide resolved

gmmorris reviewed Jan 13, 2021

View reviewed changes

YulNaumenko requested a review from gmmorris January 13, 2021 18:29

YulNaumenko added 2 commits January 13, 2021 10:31

fixed due to comments

4d4d859

fixed faling test

7885a93

Merge remote-tracking branch upstream/master

b88357b

YulNaumenko requested a review from pmuellr January 21, 2021 05:43

changed approach

8d4d83e

gmmorris suggested changes Jan 21, 2021

View reviewed changes

fixed due to comments

8008968

YulNaumenko added 3 commits January 21, 2021 12:00

Merge remote-tracking branch upstream/master

9b958ff

fixed due to comments

79e9e64

fixed tests

36f95fe

YulNaumenko requested a review from gmmorris January 21, 2021 20:04

YulNaumenko added 2 commits January 21, 2021 13:36

fixed tests

a65a64c

fixed tests

b380f70

YulNaumenko closed this Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerts][Actions] Removed isESOUsingEphemeralEncryptionKey determination. Set ESO plugin as a required dependency for Actions and Alerts plugins. Changed UI to show the proper info for disabled ESO plugin. #87936

[Alerts][Actions] Removed isESOUsingEphemeralEncryptionKey determination. Set ESO plugin as a required dependency for Actions and Alerts plugins. Changed UI to show the proper info for disabled ESO plugin. #87936

YulNaumenko commented Jan 11, 2021 •

edited

Loading

elasticmachine commented Jan 12, 2021

gchaps commented Jan 12, 2021

ymao1 left a comment

gmmorris Jan 13, 2021

YulNaumenko Jan 13, 2021 •

edited

Loading

YulNaumenko Jan 13, 2021 •

edited

Loading

YulNaumenko Jan 13, 2021 •

edited

Loading

pmuellr Jan 13, 2021

YulNaumenko Jan 21, 2021

mikecote Jan 25, 2021

YulNaumenko Jan 25, 2021

andrewvc Jan 25, 2021

YulNaumenko Jan 25, 2021

pmuellr commented Jan 13, 2021

YulNaumenko commented Jan 13, 2021

gmmorris left a comment

gmmorris Jan 21, 2021

gmmorris Jan 21, 2021

gmmorris Jan 21, 2021

gmmorris Jan 21, 2021

gmmorris Jan 21, 2021

YulNaumenko Jan 21, 2021

kibanamachine commented Jan 22, 2021

YulNaumenko commented Feb 9, 2021

		describe('routeHandlerContext.getActionsClient()', () => {
		it('should not throw error when ESO plugin not using a generated key', async () => {

[Alerts][Actions] Removed isESOUsingEphemeralEncryptionKey determination. Set ESO plugin as a required dependency for Actions and Alerts plugins. Changed UI to show the proper info for disabled ESO plugin. #87936

[Alerts][Actions] Removed isESOUsingEphemeralEncryptionKey determination. Set ESO plugin as a required dependency for Actions and Alerts plugins. Changed UI to show the proper info for disabled ESO plugin. #87936

Conversation

YulNaumenko commented Jan 11, 2021 • edited Loading

elasticmachine commented Jan 12, 2021

gchaps commented Jan 12, 2021

ymao1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YulNaumenko Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

YulNaumenko Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

YulNaumenko Jan 13, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr commented Jan 13, 2021

YulNaumenko commented Jan 13, 2021

gmmorris left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jan 22, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

YulNaumenko commented Feb 9, 2021

YulNaumenko commented Jan 11, 2021 •

edited

Loading

YulNaumenko Jan 13, 2021 •

edited

Loading

YulNaumenko Jan 13, 2021 •

edited

Loading

YulNaumenko Jan 13, 2021 •

edited

Loading