ROX-13326: Use UUID columns for roles, permissionsets and simpleaccessscopes (PermissionSet and SimpleAccessScope identifiers) in postgres mode. by rhybrillou · Pull Request #3725 · stackrox/stackrox

rhybrillou · 2022-11-07T16:37:04Z

Description

The relation between roles and the associated permission sets as well as access scopes used to be done relying on decorated UUIDs (generated UUIDs prefixed with human-readable strings).
These object are candidate to conversion to UUIDs.

Some of the UUID base work is needed for this change to work (PRs #3679 and #3681 ).

PR stack:
#3802 (server support code)
+- #3725 (data migration)
... +- #3818 (UI code)

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
~~- [ ] Evaluated and added CHANGELOG entry if required~~
~~- [ ] Determined and documented upgrade steps~~
~~- [ ] Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)~~

If any of these don't apply, please comment below.

Testing Performed

CI is a good first step.

Manual testing performed:

deploy central in non-postgres mode.
check the existing roles, permission sets and access scopes.
create a sample permission set, a sample access scopes, and roles covering the possible combinations of default and created permission sets and access scopes.
open the existing roles, permission sets and access scopes and keep trace of their identifiers.
deploy postgres central-db.
switch central to postgres mode.
monitor for migration steps. These should include migration 148 to 149, 157 to 158 and 163 to 164 (currently latest rocks migration sequence number is 111, and the altered migrations are postgres 37 to 38, 46 to 47 and 52 to 53).
open the UI
check the new identifiers. They should all be UUIDs. The identifiers that were prefixed UUIDs before the migration should be the same UUID without prefix.
check that the inter-object relations are kept (roles referencing permission sets and access scopes as well as referenced permission sets and access scopes).
connect to the postgres DB
check the table datamodel (id columns for permission_sets and simple_access_scopes should be UUIDs).

One observation during the manual test was that the Unrestricted access scope page failed to open. A pull request has been opened in order to fix the UI part as a consequence (#3818).

openshift-ci · 2022-11-07T16:37:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

rhybrillou · 2022-11-07T16:41:03Z

/test all

rhybrillou · 2022-11-07T17:17:46Z

/test all

ghost · 2022-11-07T17:47:23Z

Images are ready for the commit at 1d580ea.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-621-g1d580ea402.

rhybrillou · 2022-11-07T17:55:02Z

/test all

rhybrillou · 2022-11-07T18:24:02Z

/test all

central/role/datastore/singleton.go

migrator/migrations/n_52_to_n_53_postgres_simple_access_scopes/migration.go

pkg/postgres/pgutils/utils.go

dhaus67 · 2022-11-07T18:37:11Z

pkg/search/postgres/common.go

 		for _, pk := range q.Schema.PrimaryKeys() {
-			primaryKeyPaths = append(primaryKeyPaths, qualifyColumn(pk.Schema.Table, pk.ColumnName))
+			var cast string
+			if pk.SQLType == "uuid" {


How about re-using postgres/walker/types.go here instead of using the string representation?
I haven't looked too deep into it, but I saw that you already import postgres/walker, so I would assume that we do not end up in a circular dependency.
However, if it's not possible, we should create an unexported const for this within this file.

dhaus67 · 2022-11-07T18:39:03Z

pkg/search/postgres/common.go

 		switch subBQ := q.GetBaseQuery().Query.(type) {
 		case *v1.BaseQuery_DocIdQuery:
+			cast := "::text[]"
+			if schema.ID().SQLType == "uuid" {


Why are we not mapping it to text[] here in case of uuid?

dhaus67 · 2022-11-07T18:41:09Z

pkg/search/postgres/query/uuid_query.go

+	if queryModifiers[0] == pkgSearch.AtLeastOne {
+		panic("I dont think this is used")
+	}


Should we handle this more graceful instead of this panic and return an error here?

I'm not sure whether this is a pattern within the postgres code, I certainly know it's not for the rest of the stackrox code 😛

dhaus67 · 2022-11-07T18:44:32Z

pkg/search/postgres/query/uuid_query.go

+	err := fmt.Errorf("unknown query modifier: %s", queryModifiers[0])
+	utils.Should(err)


Since we already use the switch case, let's use this within a default case.

Suggested change

err := fmt.Errorf("unknown query modifier: %s", queryModifiers[0])

utils.Should(err)

default:

err := fmt.Errorf("unknown query modifier: %s", queryModifiers[0])

utils.Should(err)

return WhereClause{}, err

I recently learned that we should use utils.Should only in case or programatic errors that are not tied to user input. In this case, I'm not sure we should use it (since its bound on the query modifier parameter from the consumer of the function) - so it might be just sufficient enough to log within the default case and return an error here instead of using utils.Should.

rhybrillou · 2022-11-08T09:01:36Z

/test all

rhybrillou · 2022-11-08T09:26:10Z

/test all

…sscopes (PermissionSet and SimpleAccessScope identifiers) in postgres mode.

…ers do not get regenerated

rhybrillou · 2022-11-15T16:11:05Z

/retest

rhybrillou · 2022-11-15T17:14:08Z

/test gke-postgres-nongroovy-e2e-tests

rhybrillou · 2022-11-15T20:28:54Z

/test gke-postgres-qa-e2e-tests

dashrews78 · 2022-11-17T15:04:51Z

central/role/accessscope_ids.go

+// The values are UUIDs taken in descending order from ffffffff-ffff-fff4-f5ff-ffffffffffff
+// Next ID: ffffffff-ffff-fff4-f5ff-fffffffffffd
 const (
 	denyAllAccessScopeID      = "ffffffff-ffff-fff4-f5ff-fffffffffffe"


nit: would it make sense to order these in descending order like was done with permission sets?

…sscopes (PermissionSet and SimpleAccessScope identifiers) in postgres mode. (#3725)

rhybrillou requested a review from a team November 7, 2022 16:37

openshift-ci bot added the do-not-merge/work-in-progress label Nov 7, 2022

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch from c696624 to caab356 Compare November 7, 2022 16:40

github-actions bot added area/central area/postgres labels Nov 7, 2022

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch from 5e750b0 to ce4d71b Compare November 7, 2022 17:51

github-actions bot added area/auth area/helm area/operator area/roxctl area/sensor area/ui ci-all-qa-tests Tells CI to run all API tests (not just BAT). labels Nov 7, 2022

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch from ce4d71b to 51d2029 Compare November 7, 2022 17:53

dhaus67 self-requested a review November 7, 2022 18:05

dhaus67 reviewed Nov 7, 2022

View reviewed changes

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch from f025c70 to 05cff82 Compare November 8, 2022 08:57

rhybrillou marked this pull request as ready for review November 8, 2022 13:38

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 8, 2022

janisz requested review from dashrews78, janisz and mtodor November 8, 2022 16:13

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch 4 times, most recently from 6a1cbb9 to 1da346c Compare November 14, 2022 23:32

dashrews78 and others added 5 commits November 15, 2022 02:16

ROX-13326: Use UUID columns for roles, permissionsets and simpleacces…

d9abd0a

…sscopes (PermissionSet and SimpleAccessScope identifiers) in postgres mode.

Add full migration test

53ded04

Style fixes

ae6397c

Extend test to validate UUID access scope and permission set identifi…

c8d861e

…ers do not get regenerated

Fix migrator build issues

85c6d50

rhybrillou force-pushed the yann/ROX-13326-role_ids_as_uuids branch from 534473b to 85c6d50 Compare November 15, 2022 01:16

rhybrillou added 3 commits November 15, 2022 10:00

Refine migrator test

1b76b83

Type management fixes

49588ae

Fix test

1d580ea

rhybrillou changed the base branch from master to yann/ROX-13326-use_uuids_for_role_internals November 16, 2022 08:49

rhybrillou mentioned this pull request Nov 16, 2022

ROX-13326: Access control UUID support in the UI #3818

Merged

1 task

rhybrillou requested a review from janisz November 17, 2022 14:31

janisz requested review from dashrews78, dhaus67 and mtodor November 17, 2022 14:31

janisz approved these changes Nov 17, 2022

View reviewed changes

dashrews78 reviewed Nov 17, 2022

View reviewed changes

dashrews78 approved these changes Nov 17, 2022

View reviewed changes

rhybrillou and others added 2 commits November 17, 2022 17:59

ROX-13326: Access control UUID support in the UI (#3818)

a4c6ba5

Address review comment

78f731f

rhybrillou merged commit 4bcd8f0 into yann/ROX-13326-use_uuids_for_role_internals Nov 17, 2022

rhybrillou deleted the yann/ROX-13326-role_ids_as_uuids branch November 17, 2022 17:08

rhybrillou added a commit that referenced this pull request Nov 18, 2022

ROX-13326: Use UUID columns for roles, permissionsets and simpleacces…

500a3c9

…sscopes (PermissionSet and SimpleAccessScope identifiers) in postgres mode. (#3725)

		err := fmt.Errorf("unknown query modifier: %s", queryModifiers[0])
		utils.Should(err)

Conversation

rhybrillou commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented Nov 7, 2022

Uh oh!

rhybrillou commented Nov 7, 2022

Uh oh!

rhybrillou commented Nov 7, 2022

Uh oh!

ghost commented Nov 7, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhybrillou commented Nov 7, 2022

Uh oh!

rhybrillou commented Nov 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhaus67 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

dhaus67 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

dhaus67 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

dhaus67 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

dhaus67 Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

rhybrillou commented Nov 8, 2022

Uh oh!

rhybrillou commented Nov 8, 2022

Uh oh!

rhybrillou commented Nov 15, 2022

Uh oh!

rhybrillou commented Nov 15, 2022

Uh oh!

rhybrillou commented Nov 15, 2022

Uh oh!

dashrews78 Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rhybrillou commented Nov 7, 2022 •

edited

Loading

ghost commented Nov 7, 2022 •

edited by ghost

Loading