-
Notifications
You must be signed in to change notification settings - Fork 466
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
catalog: Allow for more builtin roles #14128
Conversation
1c11c30
to
10ff954
Compare
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.
10ff954
to
cb77bb1
Compare
test/cluster/mzcompose.py
Outdated
Materialized(options=mz_options) | ||
# This commit added the missing column rolconnlimit to pg_authid. | ||
Materialized( | ||
image="materialize/materialized:devel-438ea318093b3a15a924fbdae70e0db6d379a921", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -1601,8 +1609,8 @@ impl_codec!(ItemValue); | |||
|
|||
#[derive(Clone, Message, PartialOrd, PartialEq, Eq, Ord, Hash)] | |||
struct RoleKey { | |||
#[prost(uint64)] | |||
id: u64, | |||
#[prost(string)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per slack discussion, next release will have another breaking change, so we're going to piggy back off of that.
Skimmed the test changes and I love it. Deferring the actual review to @mjibson though. |
@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. |
src/adapter/src/catalog/builtin.rs
Outdated
@@ -2288,11 +2293,13 @@ pub static BUILTINS_STATIC: Lazy<Vec<Builtin<NameReference>>> = Lazy::new(|| { | |||
|
|||
builtins | |||
}); | |||
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order for this doesn't matter. It's only used on startup to persist any new roles that haven't already been persisted.
@@ -112,12 +114,18 @@ async fn migrate<S: Append>( | |||
)?; | |||
txn.id_allocator.insert( | |||
IdAllocKey { | |||
name: ROLE_ID_ALLOC_KEY.into(), | |||
name: USER_ROLE_ID_ALLOC_KEY.into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -234,86 +232,3 @@ def workflow_test_drop_default_cluster(c: Composition) -> None: | |||
|
|||
c.sql("DROP CLUSTER default CASCADE") | |||
c.sql("CREATE CLUSTER default REPLICAS (default (SIZE '1'))") | |||
|
|||
|
|||
def workflow_test_builtin_migration(c: Composition) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rip
src/adapter/src/catalog/builtin.rs
Outdated
id: 0, | ||
pub const MZ_SYSTEM: BuiltinRole = BuiltinRole { name: "mz_system" }; | ||
|
||
pub const MZ_HTTP_DEFAULT_USER: BuiltinRole = BuiltinRole { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this unused? Should it be added in a later PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 aT-protobuf
label.This PR includes the following user-facing behavior changes:
id
column ofmz_roles
.