Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Saved Objects] Adds managed to import options #155677

Merged

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Apr 25, 2023

Summary

Part of #140364

This PR is a follow up from #154515, where we added a new root property to all saved objects to indicate if they are managed or unmanaged. The property allows plugin developers to implement custom read-only UI's for managed objects.

In this PR, we implement a way to declare if objects should be managed or not, through the Saved Objects' import API.

Note:

  • The implementation is restricted to plugin developers only and we explicitly don't want to make declaring managed or not using the HTTP APIs. We can revisit this decision at a later stage if we need to.
  • We specifically don't overwrite managed on import (set a default) if it's already defined on the object that's being imported[1].
  • Creating new objects from imported ones that don't have managed specified will default to false.
  • SavedObjectsImportOptions now includes an optional managed?: boolean that, when specified will be applied to all the imports.

For example:
obj1 has managed:true,
obj2 doesn't have that set,
import options include managed:false
Result: Both obj1 & obj2 get created as managed:false

How to test this:

Ensure the default is set on import:
Run Kibana locally on an ES snapshot using any license
In Saved Objects Management (SOM), import previously exported saved objects from before 8.8.0.
Inspect one of the objects from SOM and ensure the raw json output includes managed: false

Import objects with managed already set
From SOM, inspect and copy a Saved Object to clipboard
In a text editor, change managed: false to managed: true (not recommended for anything other than testing).
Either delete the original object and import the object you edited. Inspect it and ensure managed is set to true.

Import with declaring the option as true
We haven't added the option to the Saved Object HTTP APIs on purpose. The intent is to restrict managed/unmanaged to plugins only, using core.savedObjects.getImporter(client)

As a demo, I've hard-coded the option in the call to importer.import({...}) in the saved objects service itself.
To test this, run the demo PR and follow the same process as in "Ensure the default is set on import", but this time, all the objects that were imported will be flagged as managed.
Note: the sample datasets already have managed: false in installing them. If one exports those and re-imports them (create new copies), we'll override the prop.

[1] This allows us to import explicit managed/unmanaged objects as-is (or similar) objects that were exported.
Not overriding the managed property does bring a risk that folks 'fiddle' with their raw documents to change that flag. We don't support manipulating raw documents. If they are changed, we can't guarantee that things will just work™.

A typical scenario where this could happen is if someone wants to create an editable copy of a managed object. Mitigation: we need the UI to support copy & edit and explicitly set managed:false during import/create/bulk_create

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Not being able to set managed on import using an HTTP API could cause people to change the raw es documents, leading to potential errors reading the changed versions. Low Low Migrations run on importing objects and any issues with a saved object definition will surface as errors. Integration tests verify that there isn't a saved object HTTP way to set managed.

@TinaHeiligers TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc 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 labels Apr 25, 2023
@elastic elastic deleted a comment from kibana-ci Apr 25, 2023
@TinaHeiligers TinaHeiligers marked this pull request as ready for review April 25, 2023 23:07
@TinaHeiligers TinaHeiligers requested review from a team as code owners April 25, 2023 23:07
@elasticmachine
Copy link
Contributor

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

@TinaHeiligers
Copy link
Contributor Author

@elastic/kibana-security I had to update a few tests. The changes are limited to one-liners adding the prop.

};
const createSavedObjectsResult = await createSavedObjects(createSavedObjectsParams);
errorAccumulator = [...errorAccumulator, ...createSavedObjectsResult.errors];

const successResults = createSavedObjectsResult.createdObjects.map((createdObject) => {
const { type, id, destinationId, originId } = createdObject;
const { type, id, destinationId, originId, managed: createdObjectManaged } = createdObject;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts complains about mirrored types

@@ -139,8 +148,8 @@ describe('collectSavedObjects()', () => {
const result = await collectSavedObjects({ readStream, supportedTypes, objectLimit });

const collectedObjects = [
{ ...obj1, typeMigrationVersion: '' },
{ ...obj2, typeMigrationVersion: '' },
{ ...obj1, typeMigrationVersion: '', managed: false },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collectObjects calls createSavedObjects that calls savedObjectsClient.bulkCreate, which will set the default if not already specified on the doc.

return {
...obj,
...(!obj.migrationVersion && !obj.typeMigrationVersion ? { typeMigrationVersion: '' } : {}),
// override any managed flag on an object with that given as an option otherwise set the default to avoid having to do that with a core migration transform
// this is a bulk opperation, applied to all objects being imported
...{ managed: managed ?? obj.managed ?? false },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not making any assumptions about how to handle defaults for overwriting all abjects' managed flag if there isn't a managed option passed to the call. This allows us to retain the flag on objects being imported if it's already present on the object.

...object,
...(references && { references }),
...(originId && { originId }),
...{ managed: managed ?? object.managed ?? false }, // trictly speaking this shouldn't be needed since the objects passed in should already have the flag set
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking this shouldn't be needed since the objects passed in should already have the flag set

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self review comments.

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in Spaces API integration test LGTM.

});

describe('managed option', () => {
test('if not provided, does not override existing property when already present, defaults to false for others', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked briefly, but I think this test should focus on asserting the paramaters that we send to the create API

expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams)

We already test createSavedObjects function's behaviour in other places, so mocking ES responses and checking the success results are less useful and I think these are already covered in existing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already indirectly test that the params don't include the new option in creates saved objects.

I'll be more explicit in this one then.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored all of them in this group of tests to assert on the params rather than the objects getting created.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Great job!! 🧡

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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-common 39 40 +1
Unknown metric groups

API count

id before after diff
@kbn/core-saved-objects-common 71 73 +2
@kbn/core-saved-objects-server 510 512 +2
total +4

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

@TinaHeiligers TinaHeiligers merged commit 0858b38 into elastic:main Apr 27, 2023
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGT(K)M

A few questions and NITs:

Comment on lines +62 to +67
/**
* Flag indicating if a saved object is managed by Kibana (default=false).
* Only used when upserting a saved object. If the saved object already
* exist this option has no effect.
*/
managed?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of surfacing this managed flag on our legacy url alias definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to set all objects created during import as managed if the option is provided. Legacy alias get created if they need to and the repository will set the default for managed as false if it's not specified.
In short: to bypass adding a default.

@@ -91,6 +91,7 @@ export interface SavedObjectsImportFailure {
* If `overwrite` is specified, an attempt was made to overwrite an existing object.
*/
overwrite?: boolean;
managed?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, I don't see it as being a problem, but just so I understand, why do we need to surface the managed flag on SO import failures/successes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a reference, that's all.

@@ -170,6 +177,7 @@ export async function importSavedObjectsFromStream({
type,
id,
meta,
managed: createdObjectManaged ?? managed,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: we're passing managed to createSavedObjects, so in theory all the created objects will have correct value set, right? So I think managed: createdObjectManaged would have the same effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it would.

@@ -235,6 +243,7 @@ export async function resolveSavedObjectsImportErrors({
...(overwrite && { overwrite }),
...(destinationId && { destinationId }),
...(destinationId && !originId && !createNewCopies && { createNewCopy: true }),
...{ managed: createdObject.managed ?? managed ?? false }, // double sure that this already exists but doing a check just in case
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Yeah, I think managed: createdObject.managed would be sufficient (and we don't need the spread operator here given the value will always be present)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the comment: I was worried about missing some indirect method that's called. We can change that later if needs be,

@@ -69,6 +69,14 @@ export interface SavedObjectsImportOptions {
* different Kibana versions (e.g. generate legacy URL aliases for all imported objects that have to change IDs).
*/
compatibilityMode?: boolean;
/**
* If true, will import as a managed object, else will import as not managed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If true, will import as a managed object, else will import as not managed.

This description seems false. From the my understanding of the code:

  • if true => all imported objects will have managed: true
  • if false => all imported objects will have managed: false
  • if unset => all imported objects will retain the value their currently had for managed, defaulting to false if it was unset.

Or did I misunderstood the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand the code correctly and that's what the comment implies (rather poorly it seems 😓 )

Comment on lines +100 to +107
/**
* If true, will import as a managed object, else will import as not managed.
*
* This can be leveraged by applications to e.g. prevent edits to a managed
* saved object. Instead, users can be guided to create a copy first and
* make their edits to the copy.
*/
managed?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same as previous comment)

Comment on lines +151 to +158
resp.body.saved_objects[0].updated_at = mockDate;
resp.body.saved_objects[1].updated_at = mockDate;
resp.body.saved_objects[2].updated_at = mockDate;
resp.body.saved_objects[3].updated_at = mockDate;
resp.body.saved_objects[0].created_at = mockDate;
resp.body.saved_objects[1].created_at = mockDate;
resp.body.saved_objects[2].created_at = mockDate;
resp.body.saved_objects[3].created_at = mockDate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: be lazy. for obj in resp.body.saved_objects do ...

Or, if the intent of the test is to only test the managed property, we can be even more lazy and just map(obj => { id, managed}) and only assert against those values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Saved Objects 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 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants