Delete legacy URL aliases when objects are deleted or unshared by jportner · Pull Request #117056 · elastic/kibana

jportner · 2021-11-01T23:57:44Z

⚠️ This PR is best reviewed on a per-commit basis! ⚠️

Overview

Whenever a Saved Objects Client operation causes an existing legacy URL alias to no longer having a valid target, the alias should be deleted.

There are three situations where a saved object can be removed from a space in Kibana:

The entire space is deleted (Improve space deletion cleanup #115422)
The object is deleted
The object is unshared from the space

This PR aims to address (2) and (3) above.

Implementation

I introduced a new deleteLegacyUrlAliases module in Core. This has to be able to delete an alias given an object type, ID, and the space(s) that the object has been removed from. However, if an object is shared, it may have previously existed in numerous spaces and/or "*" (all current and future spaces).

Therefore the alias deletion cannot be accomplished with a simple delete or even a bulk operation, we need to query for any "inbound aliases" for a given object in a given set of spaces. The implementation uses the Elasticsearch updateByQuery API under the hood, which supports three operations: update, delete, and noop. We only need to use delete and noop.

There are two consumers of this new module:

SavedObjectsRepository.delete
updateObjectsSpaces

This module is optimized in a few ways:

The query uses a single painless script which is cached by Elasticsearch
The query does not wait for an index refresh, as this is not important; the index will be refreshed on its own within ~30s which is enough for our needs (we just want to make sure that the alias isn't around so that it doesn't interfere with later create/share API calls)
In the updateObjectsSpaces module, we may be potentially updating spaces for dozens or hundreds of different saved objects at the same time, but we potentially have to call deleteLegacyUrlAliases for each one; as such, we use p-map to limit the concurrency of deleteLegacyUrlAliases calls to 10 at a time (we've done this before in the import API)

Testing

Functional tests have been added to test both "inclusive" and "exclusive" alias deletion in both consumers listed above.

Manual testing for this case is not too straightforward, but you could accomplish it by 1. generating staging data for objects and aliases, 2. adding it to Kibana, 3. calling the HTTP APIs for delete or updateObjectsSpaces for some of the objects, then 4. use Dev Tools to check and confirm the aliases were deleted as expected.

Also needed to update the legacy-url-alias type mapping to make the query work.

…ases

elasticmachine · 2021-11-03T02:53:25Z

Pinging @elastic/kibana-security (Team:Security)

pgayvallet

LGTM. A few minor comments

pgayvallet · 2021-11-03T11:07:48Z

src/core/server/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.ts

+  // Legacy URL aliases cannot exist in the default space; filter that out
+  const filteredNamespaces = namespaces.filter(
+    (namespace) => namespace !== DEFAULT_NAMESPACE_STRING
+  );


I wasn't even aware of that tbh...

Yeah, aliases are only created when objects in non-default spaces are converted, and I also designed resolve with that assumption in mind. Technically we could have chosen to support aliases in the default space (with a default: prefix in the raw ID) but there wasn't any practical reason to do so.

pgayvallet · 2021-11-03T11:24:28Z

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

+    deleteLegacyUrlAliases({
+      mappings,
+      registry,
+      client,
+      getIndexForType,
+      type,
+      id,
+      namespaces,
+      deleteBehavior,


That's a shame we can't bulkDeleteLegacyUrlAliases, but given that namespaces and deleteBehavior can be different per-object, I'm not even sure this would be doable with a single query/script.

Yeah, I thought about it for a while and I just don't see a way to do this with a single query, unless we passed in tons of painless script parameters (a map that has different values for each object type:id), and I'm not sure how that would perform.

Since deleting and unsharing objects should happen infrequently, I think it's OK to make individual calls to deleteLegacyUrlAliases, and a simple query and script is much safer and more reliable than a big complex one.

src/core/server/saved_objects/service/lib/update_objects_spaces.test.ts

pgayvallet · 2021-11-03T11:38:42Z

x-pack/test/spaces_api_integration/common/suites/delete.ts


-    // The test fixture contains three legacy URL aliases:
-    // (1) one for "space_1", (2) one for "space_2", and (3) one for "other_space", which is a non-existent space.
+    // The test fixture contains six legacy URL aliases:


pgayvallet · 2021-11-03T11:42:26Z

x-pack/test/spaces_api_integration/common/suites/update_objects_spaces.ts

+
+            if (expectAliasDifference !== undefined) {
+              // if we deleted an object that had an alias pointing to it, the alias should have been deleted as well
+              await es.indices.refresh({ index: '.kibana' }); // alias deletion uses refresh: false, so we need to manually refresh the index before searching


NIT/optional: I think we could refresh only once if objects.find((obj) => obj.expectAliasDifference !== undefined)

…-unshare

src/core/server/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.ts

legrego · 2021-11-03T16:29:51Z

src/core/server/saved_objects/service/lib/legacy_url_aliases/delete_legacy_url_aliases.ts

+      { ignore: [404] }
+    );
+  } catch (err) {
+    const { error } = err.body;


We're handling expected ES errors here, but an unexpected error may not have a body property, or that body property may not have an error property. If we encounter an error during this error handling, then we'll lose the origial error context and have a difficult time tracking down exactly what happened

Good call, I found this function that the core ES client module provides:

kibana/src/core/server/elasticsearch/client/configure_client.ts

Lines 83 to 93 in 2cd007e

/**

* Returns a debug message from an Elasticsearch error in the following format:

* [error type] error reason

*/

export function getErrorMessage(error: errors.ElasticsearchClientError): string {

if (error instanceof errors.ResponseError) {

const errorBody = error.meta.body as ElasticsearchErrorDetails;

return `[${errorBody?.error?.type}]: ${errorBody?.error?.reason ?? error.message}`;

}

return `[${error.name}]: ${error.message}`;

}

I'll reuse it here

legrego · 2021-11-03T16:41:00Z

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

+    }).catch((err) => {
+      // The object has already been unshared, but we caught an error when attempting to delete aliases.
+      // A consumer cannot attempt to unshare the object again, so just log the error and swallow it.
+      logger.error(err.message);


Should this error have more context?

Suggested change

logger.error(err.message);

logger.error(`Failed to delete legacy URL aliases: ${err.message}`);

The error originates from deleteLegacyUrlAliases, and those error messages are pretty detailed.
Are you suggesting we should change this message to mention that this is happening during updateObjectsSpaces?

The error originates from deleteLegacyUrlAliases, and those error messages are pretty detailed. Are you suggesting we should change this message to mention that this is happening during updateObjectsSpaces?

I agree that the expected error messages are pretty detailed, and there is currently little room for unexpected errors to occur the way that deleteLegacyUrlAliases is constructed. Yeah I think mentioning that this is happening during updateObjectsSpaces would be sufficient. It gives us another piece of "grepable" text in the error message should we ever have to trace this in production

Great, done for both consumers in ed54e64

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

...test/saved_object_api_integration/common/fixtures/es_archiver/saved_objects/spaces/data.json

…-unshare

legrego

LGTM on green CI!

kibanamachine · 2021-11-04T02:39:11Z

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`legacy-url-alias`	6	7	+1

History

💚 Build #4100 succeeded 450dd2a
💔 Build #4014 failed 04db6f2
💚 Build #3831 succeeded 4236bb9
💛 Build #3465 was flaky bd79e83087d728565b97dced224cead16965f619

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

…ic#117056)

kibanamachine · 2021-11-04T02:42:16Z

💚 Backport successful

Status	Branch	Result
✅	8.0

This backport PR will be merged automatically after passing CI.

…) (#117461) Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v8.1.0 Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature labels Nov 1, 2021

jportner added 2 commits November 2, 2021 14:20

Refactor

140e7d9

Add deleteLegacyUrlAliases module

1e8fc63

Also needed to update the legacy-url-alias type mapping to make the query work.

jportner force-pushed the issue-116235-disable-aliases-after-delete-or-unshare branch from bd79e83 to 1be71e8 Compare November 2, 2021 21:08

jportner added 2 commits November 2, 2021 17:10

Change saved object delete API to use deleteLegacyUrlAliases

7cbedaa

Change saved object updateObjectsSpaces API to use deleteLegacyUrlAli…

4236bb9

…ases

jportner force-pushed the issue-116235-disable-aliases-after-delete-or-unshare branch from 1be71e8 to 4236bb9 Compare November 2, 2021 21:10

jportner marked this pull request as ready for review November 3, 2021 02:53

jportner requested review from a team as code owners November 3, 2021 02:53

jportner mentioned this pull request Nov 3, 2021

Investigate risk of performance regression from share-capable saved object types #113743

Closed

pgayvallet mentioned this pull request Nov 3, 2021

Convert remaining Core-owned *.js files into *.ts #117123

Closed

pgayvallet approved these changes Nov 3, 2021

View reviewed changes

Merge branch 'main' into issue-116235-disable-aliases-after-delete-or…

7373db6

…-unshare

rudolf approved these changes Nov 3, 2021

View reviewed changes

PR review feedback

04db6f2

legrego reviewed Nov 3, 2021

View reviewed changes

jportner added 3 commits November 3, 2021 13:40

More PR review feedback

450dd2a

Merge branch 'main' into issue-116235-disable-aliases-after-delete-or…

ee325f7

…-unshare

PR review feedback, part 3

ed54e64

legrego approved these changes Nov 4, 2021

View reviewed changes

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 4, 2021

jportner enabled auto-merge (squash) November 4, 2021 01:33

jportner merged commit 2e9d0c0 into elastic:main Nov 4, 2021

kibanamachine mentioned this pull request Nov 4, 2021

[8.0] Delete legacy URL aliases when objects are deleted or unshared (#117056) #117461

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 4, 2021

Delete legacy URL aliases when objects are deleted or unshared (elast…

967291e

…ic#117056)

kibanamachine added a commit that referenced this pull request Nov 4, 2021

Delete legacy URL aliases when objects are deleted or unshared (#117056…

3e6fe7c

…) (#117461) Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner deleted the issue-116235-disable-aliases-after-delete-or-unshare branch November 9, 2021 15:55

TinaHeiligers mentioned this pull request Sep 19, 2022

[saved objects] Adds bulkDelete API #139680

Merged

2 tasks

	/**
	* Returns a debug message from an Elasticsearch error in the following format:
	* [error type] error reason
	*/
	export function getErrorMessage(error: errors.ElasticsearchClientError): string {
	if (error instanceof errors.ResponseError) {
	const errorBody = error.meta.body as ElasticsearchErrorDetails;
	return `[${errorBody?.error?.type}]: ${errorBody?.error?.reason ?? error.message}`;
	}
	return `[${error.name}]: ${error.message}`;
	}

	logger.error(err.message);
	logger.error(`Failed to delete legacy URL aliases: ${err.message}`);

Conversation

jportner commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Implementation

Testing

Uh oh!

elasticmachine commented Nov 3, 2021

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jportner Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 4, 2021

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

History

Uh oh!

kibanamachine commented Nov 4, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jportner commented Nov 1, 2021 •

edited

Loading

jportner Nov 3, 2021 •

edited

Loading