catalog: Allow for more builtin roles by jkosh44 · Pull Request #14128 · MaterializeInc/materialize

jkosh44 · 2022-08-08T19:18:33Z

This commit updates role ids to have two separate namespaces. One for
builtin roles and one for user roles. This will allow us to more easily
add builtin roles in the future.

This is a breaking change.

Motivation

This PR refactors existing code.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-protobuf label.
This PR includes the following user-facing behavior changes:
- Changes role ids to be a string, which changes the id column of mz_roles.

This commit updates role ids to have two separate namespaces. One for builtin roles and one for user roles. This will allow us to more easily add builtin roles in the future. This is a breaking change.

madelynnblue · 2022-08-08T21:48:07Z

test/cluster/mzcompose.py

-            Materialized(options=mz_options)
+            # This commit added the missing column rolconnlimit to pg_authid.
+            Materialized(
+                image="materialize/materialized:devel-438ea318093b3a15a924fbdae70e0db6d379a921",


I'm real sad if this needs to happen, because it effectively means this test will never test any code in a PR, but just run exactly the same code over and over.

This test has caught migration bugs from other PRs before, and seems kinda good to have maybe? At least, I'd argue that if we want to hard code this we should maybe just...remove the test because it's doing nothing and replace it with a real migration test.

I deleted it, it had a good run but with a breaking change it will no longer work. I'll open up an issue tonight about making adding a new upgrade test for this feature.

madelynnblue · 2022-08-08T21:50:47Z

src/adapter/src/catalog/storage.rs

 struct RoleKey {
-    #[prost(uint64)]
-    id: u64,
+    #[prost(string)]


Oooh our first protobuf migration. The code here will cause any existing stash to fail on startup. We need to decide if we're ok with breaking the world again or not. I don't know.

As per slack discussion, next release will have another breaking change, so we're going to piggy back off of that.

…e-id-namespace

benesch · 2022-08-10T06:26:38Z

Skimmed the test changes and I love it. Deferring the actual review to @mjibson though.

jkosh44 · 2022-08-10T20:04:16Z

@mjibson In case you already started reviewing, I just changed the name of the added user from "mz_system_external" to "http_default_user" because I thought the old name was really non-descriptive.

jkosh44 · 2022-08-10T21:29:31Z

@mjibson In case you already started reviewing, I just changed the name of the added user from "mz_system_external" to "http_default_user" because I thought the old name was really non-descriptive.

Oops didn't follow my own rule and gave it a bad name without a "mz_" prefix. Just fixed the name of the role.

madelynnblue · 2022-08-10T21:40:49Z

src/adapter/src/catalog/builtin.rs

 });
-pub static BUILTIN_ROLES: Lazy<Vec<BuiltinRole>> = Lazy::new(|| vec![MZ_SYSTEM]);
+pub static BUILTIN_ROLES: Lazy<Vec<BuiltinRole>> =
+    Lazy::new(|| vec![MZ_SYSTEM, MZ_HTTP_DEFAULT_USER]);


Is it required that the order of this doesn't change should we add more? That is, new entries must be added at the end instead of being alphabetically sorted? If so, document that with a comment.

The order for this doesn't matter. It's only used on startup to persist any new roles that haven't already been persisted.

madelynnblue · 2022-08-10T21:41:44Z

src/adapter/src/catalog/storage.rs

            txn.id_allocator.insert(
                IdAllocKey {
-                    name: ROLE_ID_ALLOC_KEY.into(),
+                    name: USER_ROLE_ID_ALLOC_KEY.into(),


If this is a breaking change to which we aren't providing an upgrade path, the cloud team should be flagged, unless there's some other breaking change they already know about.

https://materializeinc.slack.com/archives/CL68GT3AT/p1660169050996079

madelynnblue · 2022-08-10T21:48:36Z

test/cluster/mzcompose.py

    c.sql("CREATE CLUSTER default REPLICAS (default (SIZE '1'))")
-
-
-def workflow_test_builtin_migration(c: Composition) -> None:


madelynnblue · 2022-08-10T21:52:03Z

src/adapter/src/catalog/builtin.rs

-    id: 0,
+pub const MZ_SYSTEM: BuiltinRole = BuiltinRole { name: "mz_system" };
+
+pub const MZ_HTTP_DEFAULT_USER: BuiltinRole = BuiltinRole {


Is this unused? Should it be added in a later PR?

I thought it might be good to test a second builtin role, but you're probably right. I should save this for a later PR.

…e-id-namespace � Conflicts: � src/adapter/src/catalog.rs

jkosh44 force-pushed the role-id-namespace branch 3 times, most recently from 1c11c30 to 10ff954 Compare August 8, 2022 21:13

catalog: Allow for more builtin roles

cb77bb1

This commit updates role ids to have two separate namespaces. One for builtin roles and one for user roles. This will allow us to more easily add builtin roles in the future. This is a breaking change.

jkosh44 force-pushed the role-id-namespace branch from 10ff954 to cb77bb1 Compare August 8, 2022 21:21

jkosh44 marked this pull request as ready for review August 8, 2022 21:37

jkosh44 requested review from benesch and madelynnblue August 8, 2022 21:37

madelynnblue reviewed Aug 8, 2022

View reviewed changes

jkosh44 closed this Aug 9, 2022

jkosh44 reopened this Aug 9, 2022

jkosh44 added the T-proto Theme: `$T ⇔ Proto$T` conversions and `*.proto` files label Aug 9, 2022

Dynamically allocate role ids

0d8fdb6

jkosh44 mentioned this pull request Aug 9, 2022

catalog: dynamically generate role ids #14145

Closed

3 tasks

jkosh44 added 3 commits August 9, 2022 18:15

Remove non-existent test

db31af5

Merge branch 'main' of github.com:MaterializeInc/materialize into rol…

313e183

…e-id-namespace

Fix lint

a84047d

Change name of added role

76c9554

Fix new role name

36020cb

madelynnblue reviewed Aug 10, 2022

View reviewed changes

madelynnblue approved these changes Aug 10, 2022

View reviewed changes

Remove HTTP user

65988bc

jkosh44 enabled auto-merge (squash) August 10, 2022 22:14

Merge branch 'main' of github.com:MaterializeInc/materialize into rol…

f500f55

…e-id-namespace � Conflicts: � src/adapter/src/catalog.rs

jkosh44 merged commit d370474 into MaterializeInc:main Aug 10, 2022

jkosh44 deleted the role-id-namespace branch August 10, 2022 23:06

		c.sql("CREATE CLUSTER default REPLICAS (default (SIZE '1'))")


		def workflow_test_builtin_migration(c: Composition) -> None:

Conversation

jkosh44 commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesch commented Aug 10, 2022

Uh oh!

jkosh44 commented Aug 10, 2022

Uh oh!

jkosh44 commented Aug 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madelynnblue Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkosh44 commented Aug 8, 2022 •

edited

Loading

madelynnblue Aug 10, 2022 •

edited

Loading