[Saved Object Aggregation View] Use namespace registry to add tenant filter #1169

cwperks · 2022-10-28T20:14:01Z

Signed-off-by: Craig Perkins [email protected]

Description

Corresponding PR in OSD: opensearch-project/OpenSearch-Dashboards#2656

This PR uses the new namespace filter to add a tenant filter on the Saved Objects screen of OSD if the saved object aggregation view flag is enabled.

This PR also addresses logic on the typeToNamespacesMap to only include type for config when all namespaces are requested or the current tenant is in the list of requested namespaces.

Opening this as a Draft until corresponding PR is merged.

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

The first milestone of opensearch-project/security#1869.

Testing

Manual UI testing with many scenarios:

Aggregation view enabled
Aggregation view disabled
Aggregation view disabled -> Aggregation view enabled -> Aggregation view disabled
Tested without security plugin

Unit Tests

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…nant filter Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2022-10-28T20:16:40Z

public/apps/configuration/utils/tenant-utils.tsx

+      name: tenant,
+    };
+  });
+  namespacesToRegister.push({


This will always include default in the dropdown. default comes into play when OSD deployments with existing saved objects enable the saved object aggregation view feature. These objects do not have the namespace of the saved object populated and will be given the default (or null) namespace.

Signed-off-by: Craig Perkins <[email protected]>

codecov-commenter · 2022-10-28T20:42:52Z

Codecov Report

Merging #1169 (e946d2f) into main (f815e3c) will decrease coverage by 0.02%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
- Coverage   71.80%   71.78%   -0.03%     
==========================================
  Files          88       88              
  Lines        2011     2027      +16     
  Branches      265      274       +9     
==========================================
+ Hits         1444     1455      +11     
- Misses        505      509       +4     
- Partials       62       63       +1

Impacted Files	Coverage Δ
public/plugin.ts	`13.11% <20.00%> (+0.61%)`	⬆️
public/apps/configuration/utils/tenant-utils.tsx	`70.88% <90.90%> (+3.23%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cliu123 · 2022-10-28T21:11:47Z

@cwperks Is this ready for review?

cwperks · 2022-10-28T21:12:42Z

@cliu123 This is ready for review now that the companion PR is merged.

public/plugin.ts

cwperks · 2022-10-31T13:30:46Z

public/apps/configuration/utils/tenant-utils.tsx

@@ -179,6 +179,33 @@ export function transformRoleTenantPermissions(
  }));
 }

+export function getNamespacesToRegister(accountInfo: any) {
+  const tenants = accountInfo.tenants || {};


@cliu123 I noticed when testing this change that the following data structure comes out as:

{ "global_tenant": true, "admin_tenant": true, "admin": true }

Will the values in this map of tenants ever contain false? If so, should this skip over the tenants where the value in this object is false?

@cwperks You might have logged in as admin, then you are expected to have all tenant permissions. If you create a role, grant permission to a specific tenant, create a user, map the role to the user, and log in as the user, then you'll see the difference.

…uest Signed-off-by: Craig Perkins <[email protected]>

…rity-dashboards-plugin into register-namespaces

cliu123

@cwperks The changes in saved_objects_wrapper.ts would break the aggregation functionality. With the changes, the Saved Objects table no longer aggregates object from all namespaces. Please consider the following changes:

      if ('namespaces' in options && !options.namespaces) {
        // Requests coming from "Index Patterns" table have "namespaces: undefined" in the options.
        _.assign(options, { namespaces: availableTenantNames });
      }  else {
        // Requests coming from "Save Objects" table do not have namespaces in the options.
        // Move the logic with `typeToNamespacesMap` to here.
        const typeToNamespacesMap: any = {};
        ...
        ...
      }

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2022-11-01T14:55:48Z

@cliu123 I updated this to:

if (options.namespaces !== undefined) {
  const typeToNamespacesMap: any = {};
  ...

  options.typeToNamespacesMap = new Map(Object.entries(typeToNamespacesMap));
  options.type = '';
  options.namespaces = [];
} else {
  options.namespaces = [namespaceValue];
}

The else in this clause scopes the request not explicitly containing namespaces to use the current namespace (tenant). This is similar to what you have above, but just includes the current namespace.

Signed-off-by: Craig Perkins <[email protected]>

cliu123

@cwperks With the changes in this PR, the Saved Object table does not aggregate saved objects. It only shows the saved objects in the selected tenant.

cwperks · 2022-11-02T02:05:35Z

@cliu123 This PR in OSD Core needs to be merged first: opensearch-project/OpenSearch-Dashboards#2696

The old behavior would send an empty array to signal to the saved_objects_wrapper that it should include all namespaces, but this was inconsistent with the behavior expected of the find API. Since this PR will register namespaces with the new extension point in OSD core, OSD core will ship either all registered namespaces or the list of requested namespaces (if the filter is selected or search text includes namespaces) with each request to find.

This PR has been updated to treat lack of namespaces (undefined, null or empty array of options.namespaces) as a request without namespaces and will only perform the request for the current tenant.

This will help with the issue of determining which page a request comes from. Since this change targets the Saved Objects screen we will expect that requests from that page will provide namespaces and similar find requests coming from other pages will not have namespaces in the request.

Signed-off-by: Chang Liu <[email protected]>

cliu123 · 2022-11-02T20:19:12Z

Removing the backport labels for now and will add them back after backporting #1146

cliu123 · 2022-11-02T20:20:21Z

@cwperks UTs and ITs failed. Could you please check?

Signed-off-by: Chang Liu <[email protected]>

cwperks · 2022-11-02T20:46:36Z

@cliu123 re-ran UT and IT and they are passing now. The failures were on flaky tests.

cliu123

Let's hold off the merge on this PR till merging the other one moving the utils. There could be some conflicts that need to be resolved on this one.

Signed-off-by: Craig Perkins <[email protected]>

…filter (opensearch-project#1169) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Chang Liu <[email protected]> Co-authored-by: Chang Liu <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]> (cherry picked from commit 24807bb)

* Saved Object Aggregation View (#1146) * Move tenant-related utils to common folder (#1184) * [Saved Object Aggregation View] Use namespace registry to add tenant filter (#1169) Signed-off-by: Chang Liu <[email protected]> Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Yan Zeng <[email protected]> Co-authored-by: Ryan Liang <[email protected]> Co-authored-by: Ryan Liang <[email protected]> Co-authored-by: Craig Perkins <[email protected]> Co-authored-by: Yan Zeng <[email protected]>

[Saved Object Aggregation View] Use namespace register to register te…

1978641

…nant filter Signed-off-by: Craig Perkins <[email protected]>

cwperks commented Oct 28, 2022

View reviewed changes

Run eslint --fix

1b81368

Signed-off-by: Craig Perkins <[email protected]>

cwperks marked this pull request as ready for review October 28, 2022 21:12

cwperks requested a review from a team October 28, 2022 21:12

RyanL1997 reviewed Oct 29, 2022

View reviewed changes

public/plugin.ts Show resolved Hide resolved

cliu123 approved these changes Oct 30, 2022

View reviewed changes

cwperks commented Oct 31, 2022

View reviewed changes

RyanL1997 previously approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'main' into register-namespaces

91c999e

cliu123 previously approved these changes Oct 31, 2022

View reviewed changes

RyanL1997 mentioned this pull request Oct 31, 2022

Add test cases for filter of aggregation view feature opensearch-project/opensearch-dashboards-functional-test#357

Merged

1 task

cwperks added 2 commits October 31, 2022 18:37

Populate with [namespaceValue] if namespaces aren't provided with req…

92cf494

…uest Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'register-namespaces' of https://github.com/cwperks/secu…

ee9ead0

…rity-dashboards-plugin into register-namespaces

cwperks dismissed stale reviews from cliu123 and RyanL1997 via ee9ead0 October 31, 2022 22:38

cliu123 requested changes Nov 1, 2022

View reviewed changes

cwperks added 2 commits November 1, 2022 10:08

Use _all convention for requests for all namespaces

056d04c

Signed-off-by: Craig Perkins <[email protected]>

Remove _all convention

6d919d5

Signed-off-by: Craig Perkins <[email protected]>

cliu123 previously approved these changes Nov 1, 2022

View reviewed changes

Check for not undefined and not null

571a130

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed cliu123’s stale review via 571a130 November 1, 2022 17:53

Use not not when checking if namespaces is supplied in the request

b49644b

Signed-off-by: Craig Perkins <[email protected]>

cliu123 requested changes Nov 1, 2022

View reviewed changes

cliu123 approved these changes Nov 2, 2022

View reviewed changes

cwperks added backport 2.x backport to 2.x branch backport 2.4 backport to 2.4 branch labels Nov 2, 2022

DarshitChanpura previously approved these changes Nov 2, 2022

View reviewed changes

cliu123 added 2 commits November 2, 2022 12:46

Move utils to common folder

abfae9a

Signed-off-by: Chang Liu <[email protected]>

Remove the unnecessary export tenantColumn in public/index.js

0e1178b

Signed-off-by: Chang Liu <[email protected]>

cliu123 removed backport 2.x backport to 2.x branch backport 2.4 backport to 2.4 branch labels Nov 2, 2022

Move globalTenantName to common

30fd8f5

Signed-off-by: Chang Liu <[email protected]>

cliu123 requested changes Nov 2, 2022

View reviewed changes

cwperks added 2 commits November 2, 2022 17:33

Merge branch 'move_utils_to_common_folder' into register-namespaces

0993186

import default tenant

c5a3a0f

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed stale reviews from DarshitChanpura and RyanL1997 via c5a3a0f November 2, 2022 21:42

Merge changes from main

e946d2f

Signed-off-by: Craig Perkins <[email protected]>

cliu123 approved these changes Nov 2, 2022

View reviewed changes

This was referenced Nov 2, 2022

Move tenant-related utils to common folder #1184

Merged

[BUG] can not run a release build of this plugin #1175

Closed

RyanL1997 approved these changes Nov 2, 2022

View reviewed changes

cliu123 merged commit 24807bb into opensearch-project:main Nov 2, 2022

cliu123 mentioned this pull request Nov 3, 2022

[Backport 2.x] OSD Saved Object Aggregation View #1186

Merged

cliu123 mentioned this pull request Nov 3, 2022

[Backport 2.4] OSD Saved Object Aggregation View #1187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Saved Object Aggregation View] Use namespace registry to add tenant filter #1169

[Saved Object Aggregation View] Use namespace registry to add tenant filter #1169

cwperks commented Oct 28, 2022

cwperks Oct 28, 2022

codecov-commenter commented Oct 28, 2022 •

edited

Loading

cliu123 commented Oct 28, 2022

cwperks commented Oct 28, 2022

cwperks Oct 31, 2022

cliu123 Oct 31, 2022

cliu123 left a comment •

edited

Loading

cwperks commented Nov 1, 2022

cliu123 left a comment

cwperks commented Nov 2, 2022

cliu123 commented Nov 2, 2022

cliu123 commented Nov 2, 2022

cwperks commented Nov 2, 2022

cliu123 left a comment

[Saved Object Aggregation View] Use namespace registry to add tenant filter #1169

[Saved Object Aggregation View] Use namespace registry to add tenant filter #1169

Conversation

cwperks commented Oct 28, 2022

Description

Category

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

Testing

Check List

cwperks Oct 28, 2022

Choose a reason for hiding this comment

codecov-commenter commented Oct 28, 2022 • edited Loading

Codecov Report

cliu123 commented Oct 28, 2022

cwperks commented Oct 28, 2022

cwperks Oct 31, 2022

Choose a reason for hiding this comment

cliu123 Oct 31, 2022

Choose a reason for hiding this comment

cliu123 left a comment • edited Loading

Choose a reason for hiding this comment

cwperks commented Nov 1, 2022

cliu123 left a comment

Choose a reason for hiding this comment

cwperks commented Nov 2, 2022

cliu123 commented Nov 2, 2022

cliu123 commented Nov 2, 2022

cwperks commented Nov 2, 2022

cliu123 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 28, 2022 •

edited

Loading

cliu123 left a comment •

edited

Loading