Change saved object bulkUpdate to work across multiple namespaces #75478

jportner · 2020-08-19T18:34:11Z

Overview

Now, using bulk update you can optionally specify a namespace string for each object. If an "object namespace" is not defined, the repository will fall back to the operation-wide namespace ID (which is defined by options.namespace).

This PR is best reviewed one commit at a time:

fa6ae1c -- @azasypkin and I were talking, and now that search-across-spaces is implemented, OSS exposes a namespaces array for each saved object (which is actually array of namespace strings); because of this, OSS should really provide a way to convert namespace strings <-> namespace IDs. This commit makes that change.
341052a -- This commit makes the actual change to bulkUpdate in the saved objects repository/client.

Implementation

Two changes were made to bulkUpdate:

The saved objects repository was changed to prefer an individual object's "namespace string" over the operation-wide "namespace ID"
The secure saved objects client wrapper was changed to add the appropriate AuthZ checks.

Example: update "foo" in the default space, and "bar" in the outer space:

One way:

bulkUpdate([
  { type: 'foo-type', id: 'foo', namespace: 'default' },
  { type: 'bar-type', id: 'bar', namespace: 'outer' },
]);

This would yield the same result:

const options = { namespace: 'outer' };
bulkUpdate([
  { type: 'foo-type', id: 'foo', namespace: 'default' },
  { type: 'bar-type', id: 'bar' }, // no object namespace string is defined, so this falls back to options.namespace
], options);

This would also yield the same result:

const options = { namespace: undefined };
bulkUpdate([
  { type: 'foo-type', id: 'foo' }, // no object namespace string is defined, so this falls back to options.namespace
  { type: 'bar-type', id: 'bar', namespace: 'outer' },
], options);

Resolves #75449.

Now that saved objects' `namespaces` are exposed, we should provide a method to compare these strings to namespace IDs. The Spaces plugin already provided utility functions for this; I changed them to be a facade over the new core functions. The reason for this is that other plugins (alerting, actions) depend on the Spaces plugin and will use an `undefined` namespace if the Spaces plugin is not enabled.

Includes docs changes and integration tests.

jportner

Author's notes for reviewers.

Note: I did not change the developer docs, because there are none for bulkUpdate (even though there are docs for docs/api/saved-objects/bulk_create.asciidoc and docs/api/saved-objects/bulk_get.asciidoc, for example!) I think creating brand new docs for bulkUpdate is out of scope for this PR.

jportner · 2020-08-20T13:20:42Z

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

+export const namespaceStringToId = (namespace: string) => {
+  if (!namespace) {
+    throw new TypeError('namespace must be a non-empty string');
+  }


This is a new error case that may be encountered when using the saved objects client now. That said, it should never happen organically, so I'm not sure we need to be concerned about it. Thoughts?

If we remove the error check here:

Single-namespace object types would be treated as if they are in the default space

Multi-namespace object types would be treated as if they are in the '' space

I don't like the difference in behavior, so I'm in favor of leaving the error check in.

I'm +1 to keeping the error check here

jportner · 2020-08-20T15:55:38Z

x-pack/plugins/spaces/server/lib/utils/namespace.ts

-export function spaceIdToNamespace(spaceId: string): string | undefined {
-  if (!spaceId) {
-    throw new TypeError('spaceId is required');
-  }
-
-  if (spaceId === DEFAULT_SPACE_ID) {
-    return undefined;
-  }
-
-  return spaceId;
+/**
+ * Converts a Space ID string to its namespace ID representation. Note that a Space ID string is equivalent to a namespace string.
+ *
+ * See also: {@link namespaceStringToId}.
+ */
+export function spaceIdToNamespace(spaceId: string) {
+  return namespaceStringToId(spaceId);
 }


The Spaces plugin exposes the spaceIdToNamespace method (and namespaceToSpaceId below). These are functionally identical to the new namespaceStringToId and namespaceIdToString methods in OSS.

Instead of changing all usages of these to the new OSS method, I left them as-is and converted them into a facade. My primary reasoning for this is that the Actions and Alerting plugins are checking to see if the Spaces plugin is enabled before using these; if it is not enabled, they fall back to the default undefined namespace.

It's also worth noting here that our nomenclature is a bit confusing:

"namespace ID" is either a non-empty string, or undefined

"namespace string" is a non-empty string

"space ID" is a non-empty string, and is identical to "namespace string"

I think "namespace ID" and "namespace string" make more sense, but "space ID" has been around for a long time now and it would be painful to change. Thoughts?

In a perfect world, the concept of a "namespace" would have never leaked out of the saved objects system. It was always meant to be an implementation detail of the Spaces feature, so my vote is to keep "Space ID".

I also agree with your decision to keep these functions around, even though they are functionally equivilent today.

jportner · 2020-08-20T15:57:20Z

x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts

  return {
+    ...(jest.requireActual('../../../../../../src/core/server') as Record<string, unknown>),
    exportSavedObjectsToStream: jest.fn(),
    importSavedObjectsFromStream: jest.fn(),
  };


Now that I'm exporting more methods from core, this mock was broken. Added a spread of requireActual to fix it (and two other unit tests as well).

jportner · 2020-08-20T15:59:53Z

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

-    await this.ensureAuthorized(
-      this.getUniqueObjectTypes(objects),
-      'bulk_update',
-      options && options.namespace,
-      { objects, options }
-    );
+    const objectNamespaces = objects
+      .filter(({ namespace }) => namespace !== undefined)
+      .map(({ namespace }) => namespace!);
+    const namespaces = uniq([namespaceIdToString(options?.namespace), ...objectNamespaces]);
+    await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_update', namespaces, {
+      objects,
+      options,
+    });


Instead of just doing an authorization check for options.namespace, do one for options.namespace and all of the individual "object namespaces" (if any are defined).

Note that we might not need to do an authorization check foroptions.namespace (if all objects specify an individual "object namespace"), but I left this here for consistency. It would be weird to be able to call bulkUpdate in a space that you don't have access to, and only update objects in other spaces.

jportner · 2020-08-26T19:31:32Z

@elasticmachine merge upstream

legrego · 2020-08-26T19:56:50Z

ACK: Started review today, hoping to finish tomorrow. I need to mull this one over a bit, but there's nothing critically "wrong" here 😄

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

legrego · 2020-08-26T19:29:27Z

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

+export const namespaceStringToId = (namespace: string) => {
+  if (!namespace) {
+    throw new TypeError('namespace must be a non-empty string');
+  }


I'm +1 to keeping the error check here

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

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

legrego · 2020-08-26T23:58:21Z

x-pack/plugins/spaces/server/lib/utils/namespace.ts

-export function spaceIdToNamespace(spaceId: string): string | undefined {
-  if (!spaceId) {
-    throw new TypeError('spaceId is required');
-  }
-
-  if (spaceId === DEFAULT_SPACE_ID) {
-    return undefined;
-  }
-
-  return spaceId;
+/**
+ * Converts a Space ID string to its namespace ID representation. Note that a Space ID string is equivalent to a namespace string.
+ *
+ * See also: {@link namespaceStringToId}.
+ */
+export function spaceIdToNamespace(spaceId: string) {
+  return namespaceStringToId(spaceId);
 }


In a perfect world, the concept of a "namespace" would have never leaked out of the saved objects system. It was always meant to be an implementation detail of the Spaces feature, so my vote is to keep "Space ID".

I also agree with your decision to keep these functions around, even though they are functionally equivilent today.

This partially reverts commit fa6ae1c.

Now, options.namespace can use `'default'` interchangeably with `undefined`.

jportner · 2020-09-01T16:17:59Z

@legrego this PR is ready for another pass

legrego

I need to do some manual testing yet, but the only other change I think we need to make is to the encrypted_saved_objects_client_wrapper (sorry I missed this the first time around). Single-namespace ESOs rely on the object's namespace as part of the AAD, and it's currently setup to assume that options?.namespace is the authoritative source of this information. We'll need to tweak this to ensure that we're reading the correct namespace value, like you've already done within the repository:

kibana/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

Lines 157 to 161 in 6627d7d

    
           const namespace = getDescriptorNamespace( 
        
             this.options.baseTypeRegistry, 
        
             type, 
        
             options?.namespace 
        
           );

x-pack/test/saved_object_api_integration/security_only/apis/bulk_update.ts

x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_update.ts

jportner · 2020-09-02T15:13:20Z

I need to do some manual testing yet, but the only other change I think we need to make is to the encrypted_saved_objects_client_wrapper (sorry I missed this the first time around). Single-namespace ESOs rely on the object's namespace as part of the AAD, and it's currently setup to assume that options?.namespace is the authoritative source of this information. We'll need to tweak this to ensure that we're reading the correct namespace value, like you've already done within the repository:

Hm, so we can't depend on the Spaces plugin to translate 'default' to undefined here, as that should behave the same whether Spaces is enabled or not.
So we have two options:

Duplicate the namespaceStringToId behavior that exists in Core
Export this from Core so it can be consumed by the ESO plugin

I guess we can just duplicate the behavior, agreed?

legrego · 2020-09-02T16:02:59Z

Hm, so we can't depend on the Spaces plugin to translate 'default' to undefined here, as that should behave the same whether Spaces is enabled or not.

ugh. right. I wish I had considered this during my initial review, it could have avoided a lot of churn on your part -- I'm sorry about that.

So now that this is in at least three places, I guess I'm less opposed to exporting these namespace functions from core, so long as @elastic/kibana-platform is also ok with that.

This is simple enough where duplicating wouldn't be the end of the world either.

tl;dr I'm not currently favoring one approach over the other, so I'm happy to defer to you

jportner · 2020-09-02T16:07:43Z

ugh. right. I wish I had considered this during my initial review, it could have avoided a lot of churn on your part -- I'm sorry about that.

No biggie, the partial revert didn't take long!

So now that this is in at least three places, I guess I'm less opposed to exporting these namespace functions from core, so long as @elastic/kibana-platform is also ok with that.

This is simple enough where duplicating wouldn't be the end of the world either.

tl;dr I'm not currently favoring one approach over the other, so I'm happy to defer to you

Sounds good. I don't have a strong opinion to be honest, though duplication does feel a bit off to me. Since Platform is the ultimate owner of this, I'll defer to them.

joshdover · 2020-09-03T21:37:16Z

So now that this is in at least three places, I guess I'm less opposed to exporting these namespace functions from core, so long as @elastic/kibana-platform is also ok with that.

I don't have any problems with it. I would prefer we don't just export a bare namespaceStringToId from src/core/server since that name is not very descriptive by itself. Maybe wrap the two functions up in a SavedObjectsUtils object or something.

…contract" This reverts commit 0b2f62c.

jportner · 2020-09-04T21:01:52Z

I would prefer we don't just export a bare namespaceStringToId from src/core/server since that name is not very descriptive by itself. Maybe wrap the two functions up in a SavedObjectsUtils object or something.

Done in 2c36a31.

jportner · 2020-09-07T03:51:03Z

@elasticmachine merge upstream

jportner · 2020-09-09T14:00:27Z

@elasticmachine merge upstream

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

src/core/server/saved_objects/service/lib/repository.test.js

joshdover · 2020-09-09T20:26:38Z

src/core/server/saved_objects/service/lib/repository.test.js

+        // test again with object namespace string that supersedes the operation's namespace ID
+        await bulkUpdateSuccess([{ ..._obj1, namespace }]);
+        expectClientCallArgsAction([_obj1], { method: 'update', getId });
+        client.bulk.mockClear();
+        await bulkUpdateSuccess([{ ..._obj2, namespace }]);
+        expectClientCallArgsAction([_obj2], { method: 'update', getId, overrides }, 2);


It's not clear to me that this test actually asserts that the per-object namespace was actually used. I think it would be much more clear if some of these parameters & expectations were string literals rather than stored in a reused variable. IMO that makes it much more clear that this is actually working.

That said, I think most of the tests in this giant file have this issue of being incredibly hard to read due to lots of variable use and lookup needed to actually understand what the test is doing.

I agree, and you're not the first person to mention that.

When I rewrote these unit tests earlier this year, they didn't have even close to complete coverage, because so many people had changed and tweaked the repository over the years. I mainly wanted to make sure we had more complete coverage. The only reason I decided to abstract out a lot of the test logic into helper functions and variables in lieu of testing with clearer literals is because I didn't want the test file to balloon to 10k+ lines of code 😛 IMO that's even harder to read and maintain.

In the meantime, I added some comments around the test cases that leverage the getId function argument to clarify it a bit more.

legrego

LGTM, thanks for this Joe!

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts

.../encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts

There was a typo, and I took the opportunity to reword it a bit too

kibanamachine · 2020-09-10T21:30:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f345891

Build metrics

distributable file count

id	value	diff	baseline
default	45523	+1	45522
oss	27218	+1	27217
total		+2

History

💚 Build #73820 succeeded 70793b0
💚 Build #73549 succeeded cf0a0a0
💚 Build #73395 succeeded 0c97bf3
💚 Build #72835 succeeded 0a36aa5
💚 Build #72769 succeeded c2a207a

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

…astic#75478)

…5478) (#77259)

…s-for-710 * 'master' of github.com:elastic/kibana: (65 commits) Separate url forwarding logic and legacy services (elastic#76892) Bump yargs-parser to v13.1.2+ (elastic#77009) [Ingest Manager] Shared Fleet agent policy action (elastic#76013) [Search] Re-add support for aborting when a connection is closed (elastic#76470) [Search] Remove long-running query pop-up (elastic#75385) [Monitoring] Fix UI error when alerting is not available (elastic#77179) do not log plugin id format warning in dist mode (elastic#77134) [ML] Improving client side error handling (elastic#76743) [Alerting][Connectors] Refactor IBM Resilient: Generic Implementation (phase one) (elastic#74357) [Docs] some basic searchsource api docs (elastic#77038) add cGroupOverrides to the legacy config (elastic#77180) Change saved object bulkUpdate to work across multiple namespaces (elastic#75478) [Security Solution][Resolver] Replace Selectable popover with badges (elastic#76997) Removing ml-state index from archive (elastic#77143) [Security Solution] Add unit tests for histograms (elastic#77081) [Lens] Filters aggregation (elastic#75635) [Enterprise Search] Update WS Overview logic to use new config data (elastic#77122) Cleanup type output before building new types (elastic#77211) [Security Solution] Use safe type in resolver backend (elastic#76969) Use proper lodash syntax (elastic#77105) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx

jportner force-pushed the issue-75449-bulk-update-across-spaces branch from 98ee331 to 1059ecc Compare August 19, 2020 23:17

jportner force-pushed the issue-75449-bulk-update-across-spaces branch from 1059ecc to fa6ae1c Compare August 20, 2020 13:16

Change bulkUpdate method to allow per-object namespaces

341052a

Includes docs changes and integration tests.

jportner force-pushed the issue-75449-bulk-update-across-spaces branch from 863811a to 341052a Compare August 20, 2020 15:47

jportner commented Aug 20, 2020

View reviewed changes

jportner requested review from legrego, azasypkin and a team August 20, 2020 16:20

jportner added release_note:enhancement v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Aug 20, 2020

Merge branch 'master' into issue-75449-bulk-update-across-spaces

6c78cd4

legrego reviewed Aug 27, 2020

View reviewed changes

jportner added 6 commits August 31, 2020 12:34

Merge branch 'master' into issue-75449-bulk-update-across-spaces

9d849b6

Add comment per review feedback

2096fc3

Tweak secure saved objects client wrapper authZ checks

440d987

Don't expose core namespace conversion methods in the public contract

0b2f62c

This partially reverts commit fa6ae1c.

Remove @public tag from jsdocs that are no longer exported from core

1228d61

Normalize namespace strings for saved object CRUD operations

46bc77d

Now, options.namespace can use `'default'` interchangeably with `undefined`.

legrego self-requested a review September 1, 2020 16:44

legrego reviewed Sep 1, 2020

View reviewed changes

x-pack/test/saved_object_api_integration/security_only/apis/bulk_update.ts Show resolved Hide resolved

x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_update.ts Outdated Show resolved Hide resolved

jportner added 5 commits September 4, 2020 11:22

Merge branch 'master' of github.com:elastic/kibana

def73c7

Merge branch 'master' into issue-75449-bulk-update-across-spaces

978ccf1

Revert "Don't expose core namespace conversion methods in the public …

ba8c72d

…contract" This reverts commit 0b2f62c.

Wrap namespace utility methods in SavedObjectsUtils class

2c36a31

Change ESO plugin to use SavedObjectsUtils for namespaces

714a14e

Remove unnecessary comment

c2a207a

jportner marked this pull request as ready for review September 7, 2020 03:50

jportner requested a review from a team as a code owner September 7, 2020 03:50

Merge branch 'master' into issue-75449-bulk-update-across-spaces

0a36aa5

Merge branch 'master' into issue-75449-bulk-update-across-spaces

0c97bf3

joshdover approved these changes Sep 9, 2020

View reviewed changes

jportner added 2 commits September 9, 2020 17:44

Merge branch 'master' into issue-75449-bulk-update-across-spaces

65acf6d

Address PR review feedback

cf0a0a0

legrego approved these changes Sep 10, 2020

View reviewed changes

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts Show resolved Hide resolved

.../encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts Outdated Show resolved Hide resolved

jportner added 3 commits September 10, 2020 13:47

Merge branch 'master' into issue-75449-bulk-update-across-spaces

71bf105

Address more PR review feedback

70793b0

Update JSdocs

f345891

There was a typo, and I took the opportunity to reword it a bit too

jportner merged commit 52d044c into elastic:master Sep 11, 2020

jportner deleted the issue-75449-bulk-update-across-spaces branch September 11, 2020 02:09

jportner mentioned this pull request Sep 11, 2020

[7.x] Change saved object bulkUpdate to work across multiple namespaces (#75478) #77259

Merged

jportner added a commit to jportner/kibana that referenced this pull request Sep 11, 2020

Change saved object bulkUpdate to work across multiple namespaces (el…

c3f6b67

…astic#75478)

jportner added a commit that referenced this pull request Sep 11, 2020

Change saved object bulkUpdate to work across multiple namespaces (#7…

c1767fa

…5478) (#77259)

jportner added the backported label Sep 11, 2020

jportner mentioned this pull request Apr 29, 2022

Allow the SavedObjectsClient to work outside of a space #131254

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change saved object bulkUpdate to work across multiple namespaces #75478

Change saved object bulkUpdate to work across multiple namespaces #75478

jportner commented Aug 19, 2020 •

edited

Loading

jportner left a comment •

edited

Loading

jportner Aug 20, 2020

legrego Aug 26, 2020

jportner Aug 20, 2020

legrego Aug 26, 2020

jportner Aug 20, 2020

jportner Aug 20, 2020

jportner commented Aug 26, 2020

legrego commented Aug 26, 2020

legrego Aug 26, 2020

legrego Aug 26, 2020

jportner commented Sep 1, 2020

legrego left a comment

jportner commented Sep 2, 2020

legrego commented Sep 2, 2020

jportner commented Sep 2, 2020

joshdover commented Sep 3, 2020

jportner commented Sep 4, 2020

jportner commented Sep 7, 2020

jportner commented Sep 9, 2020

joshdover Sep 9, 2020

jportner Sep 9, 2020

legrego left a comment

kibanamachine commented Sep 10, 2020

	const namespace = getDescriptorNamespace(
	this.options.baseTypeRegistry,
	type,
	options?.namespace
	);

Change saved object bulkUpdate to work across multiple namespaces #75478

Change saved object bulkUpdate to work across multiple namespaces #75478

Conversation

jportner commented Aug 19, 2020 • edited Loading

Overview

Implementation

jportner left a comment • edited Loading

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

jportner commented Aug 26, 2020

legrego commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner commented Sep 1, 2020

legrego left a comment

Choose a reason for hiding this comment

jportner commented Sep 2, 2020

legrego commented Sep 2, 2020

jportner commented Sep 2, 2020

joshdover commented Sep 3, 2020

jportner commented Sep 4, 2020

jportner commented Sep 7, 2020

jportner commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 10, 2020

💚 Build Succeeded

Build metrics

distributable file count

History

jportner commented Aug 19, 2020 •

edited

Loading

jportner left a comment •

edited

Loading