[Saved Objects] Add a root level managed property to all saved object documents #154515

TinaHeiligers · 2023-04-06T07:07:04Z

Summary

ATM, fleet uses a custom tag to identify saved objects installed during package installation. The current implementation has a few disadvantages, such as needing different tag ids per space, making it complex to determine if a saved object is really managed.

As mentioned in #140364 (comment), a better solution would be if there were some sort of property on a document that indicated if it was managed or not, which can then be read from the UI and the behavior toggled accordingly.

The property would default to false and allows the fleet plugin to set it to true on importing objects during package installation.

This PR:

adds an optional managed root-level property to all saved object documents
backfills/migrates managed on existing objects using a core transform
manually migrates sample data (sample data's already on 8.8.0).

I'll add the option to set managed to the import typescript API in a follow-up PR. The option will only be available to plugins and won't have a specific API.

PR deployed on Cloud here

How to test this:

run the PR locally or using the cloud deployment
Install a sample dataset
Navigate to Saved Objects Management and inspect any saved object (except a dataview, the inspect action renders in the Data View App). A simple SO such as Global Settings [8.8.0] is easier.
Ensure that the saved object has a managed property that is false (the default).
You can repeat the inspect action for the other saved objects too and ensure they all have the default for managed set.
Changing managed to true will be implemented in a follow-up PR where we limit being able to set this to Fleet alone, on package install.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Note:

Kibana needs to run through a migration to 8.8.0 to add managed. If already on 8.8.0 (dev and, potentially, the Observability clusters that periodically pull from main) the transform won't run. If it turns out to be an issue, we either have to wait for 8.9 to backfill existing SO’s (assuming we'll allow that), work around it by diverging the core version from the kibana version, or come up with another plan.

…Create option

elasticmachine · 2023-04-06T11:24:03Z

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

jloleysens

I tested this by running locally and LGTM! Great work @TinaHeiligers !

I just had a q about needing to do the backfill. IMO could be nice to skip if we don't need to do it?

...ation-server-internal/src/document_migrator/migrations/transform_set_managed_default.test.ts

jloleysens · 2023-04-06T12:14:11Z

...-migration-server-internal/src/document_migrator/migrations/transform_set_managed_default.ts

+export const transformSetManagedDefault = (
+  doc: SavedObjectUnsanitizedDoc<unknown> | undefined
+) => ({
+  transformedDoc: { ...doc!, managed: doc!.managed ?? false },


Just curious: is it necessary to backfill the managed field? Could we assume false and only accept doc.managed === true (i.e., when it is explicitly set)?

Good question! As it stands, the UI will need to implement the logic to handle different functionality based on if managed is true or not (or undefined).
However, we only really need the flag for fleet right now and don't intend to allow setting managed for any other plugin, otherwise, we'd need to build in a managed_by option too. Is it worth the extra effort based on the fact that we don't need that right now?

The other reason and the main one discussed with @rudolf is dev exp. If managed isn't defined, then everywhere we want to read the value we'd first need to check if it's defined.

We considered using the Serializer to do a sort of "runtime" migration but we'd be checking every doc indefinately.

We're also already migrating all documents in 8.8.0 so this would not introduce additional downtime. But if there was a downside to doing it I agree it could have been left out.

@jloleysens are you ok with keeping the migration for now?

...jects/core-saved-objects-migration-server-internal/src/document_migrator/migrations/index.ts

TinaHeiligers · 2023-04-09T07:09:43Z

@elasticmachine merge upstream

TinaHeiligers · 2023-04-10T07:07:21Z

@elasticmachine merge upstream

sebelga

LGTM! 👍

TinaHeiligers · 2023-04-10T10:51:51Z

@joshdover could you please encourage the @elastic/fleet team to review?

TinaHeiligers · 2023-04-10T14:08:23Z

@elasticmachine merge upstream

… in bulkCreate

… in create

jloleysens

Great work @TinaHeiligers , thanks for adding the thorough test coverage.

I have a few points I wanted to raise before approving:

We only allow setting managed at SO creation time, not as part of an update. I am guessing this is the intended design?
I left a comment about the setManaged utility I'd like to get your thoughts on (not a blocker)
I think we can simplify a few things about the current tests

Let me know what you think!

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts

jloleysens · 2023-04-20T08:54:32Z

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts

+          attributes: { title: 'Test One' },
+          references: [{ name: 'ref_0', type: 'test', id: '1' }],
+        };
+        const result = await bulkCreateSuccess(client, repository, [objWithoutManaged, obj2]);


Similar to another comment, WDYT about setting obj2 to managed: true for these tests? That way this test checks that we are not incorrectly setting true to false

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/internal_utils.ts

pgayvallet

Overall looking good!

A few remarks in addition to the ones from @jloleysens

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/internal_utils.ts

...ed-objects/core-saved-objects-api-server-internal/src/test_helpers/repository.test.common.ts

packages/core/test-helpers/core-test-helpers-test-utils/src/create_hidden_type_variants.ts

The changes requested have been made.

TinaHeiligers · 2023-04-20T18:12:05Z

@elasticmachine merge upstream

kibana-ci · 2023-04-20T19:14:50Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 65475e0

Failed CI Steps

FTR Configs #48

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-server`	99	100	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	352.9KB	353.0KB	+91.0B

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/saved-objects-service.html#_mappings

id	before	after	diff
`managed`	-	1	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-api-browser`	109	111	+2
`@kbn/core-saved-objects-api-server`	347	351	+4
`@kbn/core-saved-objects-server`	494	496	+2
total			+8

ESLint disabled line counts

id	before	after	diff
`securitySolution`	394	397	+3

References to deprecated APIs

id	before	after	diff
`@kbn/core-saved-objects-api-browser`	60	64	+4
`@kbn/core-saved-objects-api-server-internal`	8	10	+2
`@kbn/core-saved-objects-browser-internal`	251	255	+4
total			+10

Total ESLint disabled count

id	before	after	diff
`securitySolution`	474	477	+3

History

💚 Build #120911 succeeded afa0c31
💛 Build #120376 was flaky 1578aba
💚 Build #118662 succeeded 5c812ec
💔 Build #118598 failed afa1863
💚 Build #118559 succeeded f436c46
💛 Build #118384 was flaky 1185435

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

… documents (elastic#154515) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

rudolf · 2023-05-05T11:19:36Z

closes #154642

TinaHeiligers added 7 commits April 6, 2023 01:25

Adds a root level managed property to saved object documents

06fe4d2

Updates SOR unit tests to account for the new managed create and bulk…

de07fb9

…Create option

Updaes api_integration test for rewriting ids

3b33bcb

Manually migrates sample datasets to add the managed flag

c20f4a7

Updates batch_size_bytes integration test

f888cce

run all tests

b8b45a6

Updates ftr api_integration tests

46a1c43

TinaHeiligers mentioned this pull request Apr 6, 2023

Read-only UI behaviour for system managed dashboards installed as part of fleet packages #140364

Closed

TinaHeiligers added Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.8.0 Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Apr 6, 2023

TinaHeiligers marked this pull request as ready for review April 6, 2023 11:24

TinaHeiligers requested review from a team as code owners April 6, 2023 11:24

joshdover requested a review from a team April 6, 2023 13:45

jloleysens reviewed Apr 6, 2023

View reviewed changes

Asserts value on transform test

1185435

Merge branch 'main' into saved-objects-add-managed

f436c46

TinaHeiligers requested a review from jloleysens April 10, 2023 07:07

TinaHeiligers force-pushed the saved-objects-add-managed branch from afa1863 to f436c46 Compare April 10, 2023 08:13

sebelga approved these changes Apr 10, 2023

View reviewed changes

Merge branch 'main' into saved-objects-add-managed

5c812ec

kpollich mentioned this pull request Apr 10, 2023

[Fleet] Set root-level managed property on assets installed by Fleet #154655

Closed

TinaHeiligers added 9 commits April 13, 2023 18:13

Adds unit tests for specifying managed as option to bulkCreate

33e0c73

parameterizes utility function to set the managed property

ce9d5c5

Merge branch 'main' into saved-objects-add-managed

d06b6fd

Consistent implementation in internal_utils

6f43665

Adds api_integration test to check removing managed from request body…

11861bc

… in bulkCreate

Adds api_integration test to check removing managed from request body…

933d5ee

… in create

Updates legacy_import_export to include filtering out managed

a71c434

Merge branch 'main' into saved-objects-add-managed

1578aba

revert allowing managed in HTTP APIs

afa0c31

TinaHeiligers marked this pull request as ready for review April 19, 2023 00:12

TinaHeiligers requested review from pgayvallet and rudolf April 19, 2023 13:23

jloleysens reviewed Apr 20, 2023

View reviewed changes

pgayvallet approved these changes Apr 20, 2023

View reviewed changes

TinaHeiligers added 2 commits April 20, 2023 10:36

Final PR nits and comments

52682c3

repository.test cleanup

bebb896

Merge branch 'main' into saved-objects-add-managed

65475e0

TinaHeiligers enabled auto-merge (squash) April 20, 2023 18:22

TinaHeiligers merged commit 45449ac into elastic:main Apr 20, 2023

TinaHeiligers deleted the saved-objects-add-managed branch April 20, 2023 20:01

TinaHeiligers mentioned this pull request Apr 25, 2023

[Saved Objects] Adds managed to import options #155677

Merged

1 task

nikitaindik pushed a commit to nikitaindik/kibana that referenced this pull request Apr 25, 2023

[Saved Objects] Add a root level managed property to all saved object…

a7f7349

… documents (elastic#154515) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

rudolf mentioned this pull request May 5, 2023

Add a root level managed property to all saved object documents #154642

Closed

TinaHeiligers mentioned this pull request Sep 13, 2023

[Dashboard] Add Readonly State For Managed Dashboards #166204

Merged

1 task

drewdaemon mentioned this pull request Sep 19, 2023

[Lens] Prevent overwriting managed content from editor #166720

Closed

jillguyonnet mentioned this pull request Jun 12, 2024

[Fleet] Mark Fleet-managed Saved Object tags with managed: true #176924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Objects] Add a root level managed property to all saved object documents #154515

[Saved Objects] Add a root level managed property to all saved object documents #154515

TinaHeiligers commented Apr 6, 2023 •

edited

Loading

elasticmachine commented Apr 6, 2023

jloleysens left a comment

jloleysens Apr 6, 2023

TinaHeiligers Apr 6, 2023 •

edited

Loading

rudolf Apr 7, 2023

TinaHeiligers Apr 8, 2023

TinaHeiligers commented Apr 9, 2023

TinaHeiligers commented Apr 10, 2023

sebelga left a comment

TinaHeiligers commented Apr 10, 2023

TinaHeiligers commented Apr 10, 2023

jloleysens left a comment •

edited

Loading

jloleysens Apr 20, 2023

pgayvallet left a comment

TinaHeiligers commented Apr 20, 2023

kibana-ci commented Apr 20, 2023

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

rudolf commented May 5, 2023

[Saved Objects] Add a root level managed property to all saved object documents #154515

[Saved Objects] Add a root level managed property to all saved object documents #154515

Conversation

TinaHeiligers commented Apr 6, 2023 • edited Loading

Summary

How to test this:

Checklist

Note:

elasticmachine commented Apr 6, 2023

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Apr 6, 2023

Choose a reason for hiding this comment

TinaHeiligers Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

rudolf Apr 7, 2023

Choose a reason for hiding this comment

TinaHeiligers Apr 8, 2023

Choose a reason for hiding this comment

TinaHeiligers commented Apr 9, 2023

TinaHeiligers commented Apr 10, 2023

sebelga left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Apr 10, 2023

TinaHeiligers commented Apr 10, 2023

jloleysens left a comment • edited Loading

Choose a reason for hiding this comment

jloleysens Apr 20, 2023

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

TinaHeiligers commented Apr 20, 2023

kibana-ci commented Apr 20, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Page load bundle

Saved Objects .kibana field count

API count

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

rudolf commented May 5, 2023

TinaHeiligers commented Apr 6, 2023 •

edited

Loading

TinaHeiligers Apr 6, 2023 •

edited

Loading

jloleysens left a comment •

edited

Loading