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

add isExportable SO export API #101860

Merged
merged 23 commits into from
Jun 21, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jun 10, 2021

Summary

Fix #99680

  • Add a new isExportable property to SavedObjectsTypeManagementDefinition to let type owners have per-object exportability control.
  • Adapt SO export to use this new feature
  • Add generic support for SO type attributes when registering a SO type (SavedObjectsType and underlying types)

Checklist

@pgayvallet pgayvallet added 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.14.0 labels Jun 10, 2021
Comment on lines 61 to 65
// first, evict current objects that are not exportable
const {
exportable: untransformedExportableInitialObjects,
nonExportable: nonExportableInitialObjects,
} = await splitByExportability(currentObjects, isExportable);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #99680 (comment) about the logic of the order.

registerType: (type: SavedObjectsType) => void;
registerType: <Attributes = any>(type: SavedObjectsType<Attributes>) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added generic for the attributes on SavedObjectsType, SavedObjectsTypeManagementDefinition and underlying types. However for BWC, I was forced to default to any instead of unknown for the public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

How much does this break if we switch to unknown? I'm guessing a lot 😓

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, basically all types registering a getUrl or getTitle management handler, which is a lot. This is why I don't think we should do it in this PR. I can open a follow-up and ping the teams, though

Comment on lines +141 to +143
const EMPTY_RESULT = {
excludedObjects: [],
excludedObjectsCount: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me a while to understand why adding the new properties at the end of the definition was still causing a failure. TIL, for some test suites, we're asserting against the stringified so the properties needs to be in the exact same order.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 @jportner do recall why we are doing a stringified test here? Can we make this more robust by sorting the keys before stringifying, and switch to using json-stable-stringify instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC it's because the export API returns Content-Type: application/ndjson and the current approach was cleaner. Otherwise we would have to do this

diff --git a/x-pack/test/saved_object_api_integration/common/suites/export.ts b/x-pack/test/saved_object_api_integration/common/suites/export.ts
index d9ebbac8102..ea474a88092 100644
--- a/x-pack/test/saved_object_api_integration/common/suites/export.ts
+++ b/x-pack/test/saved_object_api_integration/common/suites/export.ts
@@ -135,8 +135,6 @@ export const createRequest = ({ type, id }: ExportTestCase) =>
 const getTestTitle = ({ failure, title }: ExportTestCase) =>
   `${failure?.reason || 'success'} ["${title}"]`;
 
-const EMPTY_RESULT = { exportedCount: 0, missingRefCount: 0, missingReferences: [] };
-
 export function exportTestSuiteFactory(esArchiver: any, supertest: SuperTest<any>) {
   const expectSavedObjectForbiddenBulkGet = expectResponses.forbiddenTypes('bulk_get');
   const expectResponseBody = (testCase: ExportTestCase): ExpectResponseBody => async (
@@ -151,7 +149,12 @@ export function exportTestSuiteFactory(esArchiver: any, supertest: SuperTest<any
       } else if (failure.statusCode === 200) {
         // "find" was unauthorized, which returns an empty result
         expect(response.body).not.to.have.property('error');
-        expect(response.text).to.equal(JSON.stringify(EMPTY_RESULT));
+        const exportDetails = JSON.parse(response.text);
+        expect(exportDetails).to.eql({
+          exportedCount: 0,
+          missingRefCount: 0,
+          missingReferences: [],
+        });
       } else {
         throw new Error(`Unexpected failure status code: ${failure.statusCode}`);
       }

The success case assertions further down also have to parse the ndjson.

@pgayvallet pgayvallet marked this pull request as ready for review June 13, 2021 19:49
@pgayvallet pgayvallet requested review from a team as code owners June 13, 2021 19:49
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

  • Do we have a known use case for async isExportable implementations? If not, this could be something we opt not to support at this time to avoid any performance issues with this new hook.

registerType: (type: SavedObjectsType) => void;
registerType: <Attributes = any>(type: SavedObjectsType<Attributes>) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

How much does this break if we switch to unknown? I'm guessing a lot 😓

src/core/server/saved_objects/types.ts Outdated Show resolved Hide resolved
src/core/server/saved_objects/types.ts Show resolved Hide resolved
src/core/server/saved_objects/types.ts Outdated Show resolved Hide resolved
},
]
`);
Array [
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh all these snapshots shakes fist

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

A few nits below, and a general comment:

I want to make sure that we don't use this new isExportable function to enforce authorization/access control. We currently grant users with the Saved Objects Management feature access to any exportable saved object type, so if we start relying on isExportable to refine that authorization, then we will have a mismatch of expectations. It doesn't sound like that's the intent here, but I wanted to double check.

const nonExportableObjects: SavedObject[] = [];
await Promise.all(
objects.map(async (obj) => {
const exportable = await isExportable(obj);
Copy link
Member

Choose a reason for hiding this comment

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

If saved object migrations have taught me anything, it's that plugin authors can/will create buggy functions to interrogate their saved objects. Can we be defensive here, and handle any thrown errors explicitly? I'm open to suggestions, but I'm thinking we could either:

  1. fail open, and treat failed checks as "exportable"
  2. fail closed, and treat failed checks as "unexportable"
  3. raise our own error in response to a failed check

Option 3 is my least favorite, because it prevents administrators from exporting saved objects due to a bug in our code, without any recourse. That said, I don't know the intent of preventing objects from being excluded, so I don't know if it's safe for us to always fail open/closed.

Perhaps the safest option is to "fail closed", and report the failure within the export metadata and/or in a server log entry. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If saved object migrations have taught me anything, it's that plugin authors can/will create buggy functions to interrogate their saved objects

Can't disagree on that

Perhaps the safest option is to "fail closed", and report the failure within the export metadata and/or in a server log entry

That's a perfect use case for the currently unused SavedObjectsExportExcludedObject.reason

Comment on lines +141 to +143
const EMPTY_RESULT = {
excludedObjects: [],
excludedObjectsCount: 0,
Copy link
Member

Choose a reason for hiding this comment

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

🤔 @jportner do recall why we are doing a stringified test here? Can we make this more robust by sorting the keys before stringifying, and switch to using json-stable-stringify instead?

@pgayvallet
Copy link
Contributor Author

@legrego

We currently grant users with the Saved Objects Management feature access to any exportable saved object type, so if we start relying on isExportable to refine that authorization, then we will have a mismatch of expectations. It doesn't sound like that's the intent here, but I wanted to double check.

I confirm that it is not the intent at all. This is only required for the '8.0 exportability' project, and AFAIK it may even be only temporary. See #99680 and #50266 (comment) for more context.

@pgayvallet
Copy link
Contributor Author

@joshdover @legrego I think I addressed all of your feedbacks. PTAL

@pgayvallet pgayvallet requested review from joshdover and legrego June 16, 2021 07:13
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the edits!

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
core 1053 1054 +1

Async chunks

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

id before after diff
savedObjectsManagement 138.5KB 138.9KB +430.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
core 26 27 +1
Unknown metric groups

API count

id before after diff
core 2276 2283 +7

History

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

@pgayvallet pgayvallet merged commit 59d7f33 into elastic:master Jun 21, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jun 21, 2021
* add `isExportable` SO export API

* add warning when export contains excluded objects

* add FTR test

* fix API integration assertions

* lint

* fix assertions again

* doc

* update generated doc

* fix esarchiver paths

* use maps instead of objects

* SavedObjectsExportablePredicate is no longer async

* more docs

* generated doc

* use info instead of warning when export contains excluded objects

* try/catch on isExportable call and add exclusion reason

* add FTR test for errored objects

* log error if isExportable throws
pgayvallet added a commit that referenced this pull request Jun 21, 2021
* add `isExportable` SO export API (#101860)

* add `isExportable` SO export API

* add warning when export contains excluded objects

* add FTR test

* fix API integration assertions

* lint

* fix assertions again

* doc

* update generated doc

* fix esarchiver paths

* use maps instead of objects

* SavedObjectsExportablePredicate is no longer async

* more docs

* generated doc

* use info instead of warning when export contains excluded objects

* try/catch on isExportable call and add exclusion reason

* add FTR test for errored objects

* log error if isExportable throws

* fix dataset for 7.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to filter out items onExport or onImport
6 participants