Add deprecation warning when unknown SO types are present #111268

pgayvallet · 2021-09-06T10:08:00Z

Summary

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pgayvallet

@Bamieh @lukeelmers this is still in POC state.

Just to be sure we're all on the same page regarding the approach, could you take a quick look, to make sure I took the correct direction?

src/core/server/saved_objects/service/lib/repository.ts

src/core/server/saved_objects/routes/deprecations/delete_unknown_types.ts

src/core/server/saved_objects/deprecations/unknown_object_types.ts

pgayvallet · 2021-09-06T10:12:35Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+  const { body } = await esClient.asInternalUser.search<SavedObjectsRawDocSource>({
+    index: targetIndices,
+    body: {
+      size: 10000,


What should I do if this throws? Should I just not register the deprecation?

i think the deprecation service needs to support sending an error inside the returned array from the getDeprecation function.

Currently if the whole getDeprecation throws then the service will show failed to fetch deprecation for x feature. We might need to mimic this behavior inside the array entries just like we do for the whole function.

Maybe until we support this we are OK with throwing everything (es connectivity issue usually means ES is unreachable or something).

Bamieh · 2021-09-06T10:18:44Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+      }),
+      level: 'critical',
+      requireRestart: false,
+      deprecationType: undefined, // not config nor feature...


Do you have in ind a specific deprecation type we can add or are you comfortable with undefined?

As suggested by the UA team in another issue, I think we should have this field mandatory and at least add an other category

Bamieh

Approach LGTM aligned with what I had in mind.

Bamieh · 2021-09-06T10:26:01Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+  const { body } = await esClient.asInternalUser.search<SavedObjectsRawDocSource>({
+    index: targetIndices,
+    body: {
+      size: 10000,


i think the deprecation service needs to support sending an error inside the returned array from the getDeprecation function.

Currently if the whole getDeprecation throws then the service will show failed to fetch deprecation for x feature. We might need to mimic this behavior inside the array entries just like we do for the whole function.

Maybe until we support this we are OK with throwing everything (es connectivity issue usually means ES is unreachable or something).

joshdover · 2021-09-06T12:12:59Z

Did we already reach a decision on the long-term support for unknown types in 8.0 #107678?

pgayvallet · 2021-09-06T13:26:15Z

@joshdover

Did we already reach a decision on the long-term support for unknown types in 8.0 #107678?

@rudolf @kobelb @lukeelmers please correct me if I'm wrong here, but from our last conversations, the consensus was that due to both the timeframe and the complexity of introducing a quarantine mechanism, the target solution for 8.0 was to have Kibana fail to start when unknown types are encountered, if this option was considered acceptable by all parties.

…n-types-deprecation-warning

elasticmachine · 2021-09-07T10:51:53Z

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

pgayvallet

self review

pgayvallet · 2021-09-07T11:00:11Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+        manualSteps: [
+          i18n.translate('core.savedObjects.deprecations.unknownTypes.manualSteps.1', {
+            defaultMessage: 'If plugins are disabled, re-enable the, then restart Kibana.',
+          }),
+          i18n.translate('core.savedObjects.deprecations.unknownTypes.manualSteps.2', {
+            defaultMessage:
+              'If no plugins are disabled, or if enabling them does not fix the issue, delete the documents.',
+          }),


Not really sure what to put in the manualSteps, as we're mostly expecting the user to use the automated resolve instead.

We could potentially list all the faulty objects as we're doing in the checkForUnknownDocs migration task's warning, but that really don't work well in the UA UI.

The steps make sense to me. Maybe we can specify how to find these document ids by checking the server logs.

pgayvallet · 2021-09-07T11:05:37Z

src/core/server/saved_objects/routes/deprecations/delete_unknown_types.ts

+  router.post(
+    {
+      path: '/deprecations/_delete_unknown_types',
+      validate: false,
+    },


A small security concern here: as we can't leverage the SOR to perform the cleanup, we're using the internal ES client instead (which also make sense, because we want to be able to delete all the unknown docs anyway, and because the SO security does not work with unknown types...).

However, this means that any authenticated user can potentially hit this endpoint, and cause the removal of the unknown docs. We could restrict the route with a access: tag, however this endpoint needs to be reachable by anyone having access to the UA management section to resolve the deprecation.

I don't think this is that big of a deal, but still worth a comment: are we fine with this?

The likelihood is low that this gets abused and in most cases the impact will be low, it will probably just delete unused data.

But as a pattern this feels wrong, it feels like something that will crop up for other teams too that have destructive correctiveActions e.g. https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/server/routes/deprecations.ts#L100

In reporting's case, security is enforced by Elasticsearch. So there's some precedent that just because you're able to use the upgrade assistant doesn't mean you'll automatically be able to perform all corrective actions.

I wonder if it wouldn't be fine to require the kibana_admin to perform these upgrade assistant fixes?

I wonder if it wouldn't be fine to require the kibana_admin to perform these upgrade assistant fixes?

we could use the scoped client instead of the internal one, that would do it, but then other users would still see the deprecation, and clicking the button would throw a server error... I don't really like it either to be honest.

pgayvallet · 2021-09-07T11:07:12Z

test/api_integration/apis/saved_objects/delete_unknown_types.ts

+        const afterDelete = await fetchIndexContent();
+        // we're deleting with `wait_for_completion: false` and we don't surface
+        // the task ID in the API, so we're forced to use pooling for the FTR tests
+        if (afterDelete.map((obj) => obj.type).includes('unknown-type') && i < 10) {
+          await delay(1000);
+          continue;
+        }


As explained in the comment, we're deleting with wait_for_completion: false, and the API does not surface the taskId. I could have added it to the deleteUnknownTypeObjects return value and to the HTTP endpoint's response, but that didn't felt right, as the only need was for FTR tests, so I'm using a dumb loop instead.

src/core/server/saved_objects/saved_objects_service.ts

pgayvallet · 2021-09-07T11:09:57Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+      title: i18n.translate('core.savedObjects.deprecations.unknownTypes.title', {
+        defaultMessage: 'Saved objects with unknown types are present in Kibana system indices',
+      }),
+      message: i18n.translate('core.savedObjects.deprecations.unknownTypes.message', {
+        defaultMessage:
+          'Upgrades will fail for 8.0+ because documents were found for unknown saved object types.' +
+          `To ensure that upgrades will succeed in the future, either re-enable plugins or delete these documents from the Kibana indices`,
+      }),


Should we surface the number of unknown docs, of their types, in the deprecation's message?

In the deprecation guide we recommend not writing upgrade will fail in version x.x

If we already have the number of unknown docs i think it makes sense to surface them here.

joshdover · 2021-09-07T13:44:54Z

please correct me if I'm wrong here, but from our last conversations, the consensus was that due to both the timeframe and the complexity of introducing a quarantine mechanism, the target solution for 8.0 was to have Kibana fail to start when unknown types are encountered, if this option was considered acceptable by all parties.

Thank you for the clarification!

…n-types-deprecation-warning

Bamieh · 2021-09-10T12:05:03Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+      title: i18n.translate('core.savedObjects.deprecations.unknownTypes.title', {
+        defaultMessage: 'Saved objects with unknown types are present in Kibana system indices',
+      }),
+      message: i18n.translate('core.savedObjects.deprecations.unknownTypes.message', {
+        defaultMessage:
+          'Upgrades will fail for 8.0+ because documents were found for unknown saved object types.' +
+          `To ensure that upgrades will succeed in the future, either re-enable plugins or delete these documents from the Kibana indices`,
+      }),


In the deprecation guide we recommend not writing upgrade will fail in version x.x

Bamieh · 2021-09-10T12:05:32Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+      title: i18n.translate('core.savedObjects.deprecations.unknownTypes.title', {
+        defaultMessage: 'Saved objects with unknown types are present in Kibana system indices',
+      }),
+      message: i18n.translate('core.savedObjects.deprecations.unknownTypes.message', {
+        defaultMessage:
+          'Upgrades will fail for 8.0+ because documents were found for unknown saved object types.' +
+          `To ensure that upgrades will succeed in the future, either re-enable plugins or delete these documents from the Kibana indices`,
+      }),


If we already have the number of unknown docs i think it makes sense to surface them here.

src/core/server/saved_objects/deprecations/unknown_object_types.ts

Bamieh · 2021-09-10T12:08:28Z

src/core/server/saved_objects/deprecations/unknown_object_types.ts

+        manualSteps: [
+          i18n.translate('core.savedObjects.deprecations.unknownTypes.manualSteps.1', {
+            defaultMessage: 'If plugins are disabled, re-enable the, then restart Kibana.',
+          }),
+          i18n.translate('core.savedObjects.deprecations.unknownTypes.manualSteps.2', {
+            defaultMessage:
+              'If no plugins are disabled, or if enabling them does not fix the issue, delete the documents.',
+          }),


The steps make sense to me. Maybe we can specify how to find these document ids by checking the server logs.

…n-types-deprecation-warning

kibanamachine · 2021-09-14T08:23:47Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #152824 succeeded 0a745d2
💚 Build #152160 succeeded 58de766
💔 Build #152129 failed a4f750c
💚 Build #151409 succeeded 694ab57
💚 Build #151394 succeeded 83c1bf9

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

Bamieh

lgtm

kibanamachine · 2021-09-14T14:00:59Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 111268

…1268) * Add deprecation warning when unknown types are present * fix and add service tests * remove export * plug deprecation route * add integration test for new route * add unit test for getIndexForType * add unit tests * improve deprecation messages * add FTR test * fix things due to merge * change the name of the deprecation provider * improve message * improve message again # Conflicts: # src/core/server/saved_objects/saved_objects_service.ts

) (#112112) * Add deprecation warning when unknown SO types are present (#111268) * Add deprecation warning when unknown types are present * fix and add service tests * remove export * plug deprecation route * add integration test for new route * add unit test for getIndexForType * add unit tests * improve deprecation messages * add FTR test * fix things due to merge * change the name of the deprecation provider * improve message * improve message again # Conflicts: # src/core/server/saved_objects/saved_objects_service.ts * fix types

Add deprecation warning when unknown types are present

5e0be08

pgayvallet commented Sep 6, 2021

View reviewed changes

Bamieh reviewed Sep 6, 2021

View reviewed changes

pgayvallet added 2 commits September 6, 2021 13:39

fix and add service tests

9e8552d

remove export

fb96168

pgayvallet added 7 commits September 6, 2021 15:58

plug deprecation route

4c33e2e

add integration test for new route

a912050

add unit test for getIndexForType

fc11738

add unit tests

18678d4

improve deprecation messages

3f49db4

Merge remote-tracking branch 'upstream/master' into kbn-105272-unknow…

83c1bf9

…n-types-deprecation-warning

add FTR test

694ab57

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

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

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

pgayvallet commented Sep 7, 2021

View reviewed changes

pgayvallet requested review from rudolf and Bamieh September 7, 2021 11:10

pgayvallet added 2 commits September 9, 2021 10:44

Merge remote-tracking branch 'upstream/master' into kbn-105272-unknow…

a4f750c

…n-types-deprecation-warning

Merge remote-tracking branch 'upstream/master' into kbn-105272-unknow…

58de766

…n-types-deprecation-warning

Bamieh reviewed Sep 10, 2021

View reviewed changes

pgayvallet added 5 commits September 13, 2021 14:52

Merge remote-tracking branch 'upstream/master' into kbn-105272-unknow…

4d92be0

…n-types-deprecation-warning

fix things due to merge

6352eeb

change the name of the deprecation provider

0a745d2

improve message

026a80d

improve message again

d3e47f6

pgayvallet requested a review from Bamieh September 14, 2021 06:08

Bamieh approved these changes Sep 14, 2021

View reviewed changes

rudolf approved these changes Sep 14, 2021

View reviewed changes

pgayvallet added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 14, 2021

pgayvallet merged commit 138371e into elastic:master Sep 14, 2021

pgayvallet mentioned this pull request Sep 14, 2021

[7.x] Add deprecation warning when unknown SO types are present (#111268) #112112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation warning when unknown SO types are present #111268

Add deprecation warning when unknown SO types are present #111268

pgayvallet commented Sep 6, 2021 •

edited

Loading

pgayvallet left a comment

pgayvallet Sep 6, 2021

Bamieh Sep 6, 2021 •

edited

Loading

Bamieh Sep 6, 2021

pgayvallet Sep 6, 2021

Bamieh left a comment

Bamieh Sep 6, 2021 •

edited

Loading

joshdover commented Sep 6, 2021

pgayvallet commented Sep 6, 2021

elasticmachine commented Sep 7, 2021

pgayvallet left a comment

pgayvallet Sep 7, 2021

Bamieh Sep 10, 2021

pgayvallet Sep 7, 2021

rudolf Sep 8, 2021

pgayvallet Sep 13, 2021

pgayvallet Sep 7, 2021

pgayvallet Sep 7, 2021

Bamieh Sep 10, 2021

Bamieh Sep 10, 2021

joshdover commented Sep 7, 2021

Bamieh Sep 10, 2021

Bamieh Sep 10, 2021

Bamieh Sep 10, 2021

kibanamachine commented Sep 14, 2021

Bamieh left a comment

kibanamachine commented Sep 14, 2021

Add deprecation warning when unknown SO types are present #111268

Add deprecation warning when unknown SO types are present #111268

Conversation

pgayvallet commented Sep 6, 2021 • edited Loading

Summary

Checklist

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bamieh Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bamieh left a comment

Choose a reason for hiding this comment

Bamieh Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

joshdover commented Sep 6, 2021

pgayvallet commented Sep 6, 2021

elasticmachine commented Sep 7, 2021

pgayvallet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Sep 14, 2021

💚 Build Succeeded

Metrics [docs]

History

Bamieh left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 14, 2021

💔 Backport failed

pgayvallet commented Sep 6, 2021 •

edited

Loading

Bamieh Sep 6, 2021 •

edited

Loading

Bamieh Sep 6, 2021 •

edited

Loading