Skip to content

Commit fd3ad03

Browse files
TCeasonTCeason
authored
feat(query): support create or replace role (#18500)
* feat(query): support create or replace role * fix conversation * fix ci: Handle ExistsError * delete 306 307 add 311 rbac compat test In ths pr add replace role, and it's based on PB. In 306/307 role is serde_json not support PB. Add 311 rbac read write compat test. 311 is the first version that ownership and role serialize to pb. https://github.com/databendlabs/databend/releases/tag/v1.2.311-nightly
1 parent fbd8f78 commit fd3ad03

File tree

16 files changed

+148
-59
lines changed

16 files changed

+148
-59
lines changed

‎.github/actions/test_compat_fuse/action.yml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,11 @@ runs:
1313
run: |
1414
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.46 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path base
1515
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.241 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path revoke
16-
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.306 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path rbac
17-
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.307 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path rbac
16+
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.311 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path rbac
1817
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.318 --reader-version current --meta-versions 1.2.527 1.2.677 --logictest-path rbac
1918
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version 1.2.680 --reader-version current --meta-versions 1.2.527 1.2.680 --logictest-path udf
2019
21-
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version current --reader-version 1.2.307 --meta-versions 1.2.677 --logictest-path rbac
20+
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version current --reader-version 1.2.311 --meta-versions 1.2.677 --logictest-path rbac
2221
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version current --reader-version 1.2.318 --meta-versions 1.2.677 --logictest-path rbac
2322
bash ./tests/compat_fuse/test_compat_fuse.sh --writer-version current --reader-version 1.2.680 --meta-versions 1.2.680 --logictest-path udf
2423
- name: Upload failure

‎src/meta/app/src/principal/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ pub use procedure_id_ident::ProcedureId;
8383
pub use procedure_id_to_name::ProcedureIdToNameIdent;
8484
pub use procedure_identity::ProcedureIdentity;
8585
pub use procedure_name_ident::ProcedureNameIdent;
86+
pub use role_ident::Resource;
8687
pub use role_ident::RoleIdent;
8788
pub use role_ident::RoleIdentRaw;
8889
pub use role_info::RoleInfo;

‎src/query/ast/src/ast/statements/statement.rs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ pub enum Statement {
244244
show_options: Option<ShowOptions>,
245245
},
246246
CreateRole {
247-
if_not_exists: bool,
247+
create_option: CreateOption,
248248
role_name: String,
249249
},
250250
DropRole {
@@ -875,11 +875,15 @@ impl Display for Statement {
875875
write!(f, " {}", user)?;
876876
}
877877
Statement::CreateRole {
878-
if_not_exists,
878+
create_option,
879879
role_name: role,
880880
} => {
881-
write!(f, "CREATE ROLE")?;
882-
if *if_not_exists {
881+
write!(f, "CREATE")?;
882+
if let CreateOption::CreateOrReplace = create_option {
883+
write!(f, " OR REPLACE")?;
884+
}
885+
write!(f, " ROLE")?;
886+
if let CreateOption::CreateIfNotExists = create_option {
883887
write!(f, " IF NOT EXISTS")?;
884888
}
885889
write!(f, " {}", QuotedString(role, '\''))?;

‎src/query/ast/src/parser/statement.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1643,13 +1643,17 @@ pub fn statement_body(i: Input) -> IResult<Statement> {
16431643
},
16441644
|(_, _, show_options)| Statement::ShowRoles { show_options },
16451645
);
1646-
let create_role = map(
1646+
let create_role = map_res(
16471647
rule! {
1648-
CREATE ~ ROLE ~ ( IF ~ ^NOT ~ ^EXISTS )? ~ #role_name
1648+
CREATE ~ ( OR ~ ^REPLACE )? ~ ROLE ~ ( IF ~ ^NOT ~ ^EXISTS )? ~ #role_name
16491649
},
1650-
|(_, _, opt_if_not_exists, role_name)| Statement::CreateRole {
1651-
if_not_exists: opt_if_not_exists.is_some(),
1652-
role_name,
1650+
|(_, opt_or_replace, _, opt_if_not_exists, role_name)| {
1651+
let create_option =
1652+
parse_create_option(opt_or_replace.is_some(), opt_if_not_exists.is_some())?;
1653+
Ok(Statement::CreateRole {
1654+
create_option,
1655+
role_name,
1656+
})
16531657
},
16541658
);
16551659
let drop_role = map(

‎src/query/ast/tests/it/testdata/stmt.txt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13055,7 +13055,7 @@ create role test
1305513055
CREATE ROLE 'test'
1305613056
---------- AST ------------
1305713057
CreateRole {
13058-
if_not_exists: false,
13058+
create_option: Create,
1305913059
role_name: "test",
1306013060
}
1306113061

@@ -13066,7 +13066,7 @@ create role 'test'
1306613066
CREATE ROLE 'test'
1306713067
---------- AST ------------
1306813068
CreateRole {
13069-
if_not_exists: false,
13069+
create_option: Create,
1307013070
role_name: "test",
1307113071
}
1307213072

‎src/query/management/src/role/role_api.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,23 @@
1313
// limitations under the License.
1414

1515
use databend_common_exception::Result;
16+
use databend_common_meta_app::principal::role_ident;
1617
use databend_common_meta_app::principal::OwnershipInfo;
1718
use databend_common_meta_app::principal::OwnershipObject;
1819
use databend_common_meta_app::principal::RoleInfo;
20+
use databend_common_meta_app::tenant_key::errors::ExistError;
1921
use databend_common_meta_kvapi::kvapi::ListKVReply;
2022
use databend_common_meta_types::MatchSeq;
23+
use databend_common_meta_types::MetaError;
2124
use databend_common_meta_types::SeqV;
2225

2326
#[async_trait::async_trait]
2427
pub trait RoleApi: Sync + Send {
25-
async fn add_role(&self, role_info: RoleInfo) -> Result<u64>;
28+
async fn add_role(
29+
&self,
30+
role_info: RoleInfo,
31+
can_replace: bool,
32+
) -> std::result::Result<std::result::Result<(), ExistError<role_ident::Resource>>, MetaError>;
2633

2734
#[allow(clippy::ptr_arg)]
2835
async fn get_role(&self, role: &String, seq: MatchSeq) -> Result<SeqV<RoleInfo>>;

‎src/query/management/src/role/role_mgr.rs

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@ use std::sync::Arc;
1717
use databend_common_base::base::tokio::sync::Mutex;
1818
use databend_common_exception::ErrorCode;
1919
use databend_common_meta_api::kv_pb_api::KVPbApi;
20+
use databend_common_meta_api::kv_pb_api::UpsertPB;
2021
use databend_common_meta_api::reply::unpack_txn_reply;
2122
use databend_common_meta_api::txn_backoff::txn_backoff;
2223
use databend_common_meta_api::txn_cond_seq;
2324
use databend_common_meta_api::txn_op_del;
2425
use databend_common_meta_api::txn_op_put;
2526
use databend_common_meta_app::app_error::AppError;
2627
use databend_common_meta_app::app_error::TxnRetryMaxTimes;
28+
use databend_common_meta_app::principal::role_ident;
2729
use databend_common_meta_app::principal::GrantObject;
2830
use databend_common_meta_app::principal::OwnershipInfo;
2931
use databend_common_meta_app::principal::OwnershipObject;
@@ -32,6 +34,7 @@ use databend_common_meta_app::principal::RoleInfo;
3234
use databend_common_meta_app::principal::TenantOwnershipObjectIdent;
3335
use databend_common_meta_app::principal::UserPrivilegeType;
3436
use databend_common_meta_app::tenant::Tenant;
37+
use databend_common_meta_app::tenant_key::errors::ExistError;
3538
use databend_common_meta_app::KeyWithTenant;
3639
use databend_common_meta_cache::Cache;
3740
use databend_common_meta_client::ClientHandle;
@@ -48,6 +51,7 @@ use databend_common_meta_types::Operation;
4851
use databend_common_meta_types::SeqV;
4952
use databend_common_meta_types::TxnRequest;
5053
use databend_common_meta_types::UpsertKV;
54+
use databend_common_meta_types::With;
5155
use enumflags2::make_bitflags;
5256
use fastrace::func_name;
5357
use futures::TryStreamExt;
@@ -175,23 +179,28 @@ impl RoleMgr {
175179
impl RoleApi for RoleMgr {
176180
#[async_backtrace::framed]
177181
#[fastrace::trace]
178-
async fn add_role(&self, role_info: RoleInfo) -> databend_common_exception::Result<u64> {
179-
let match_seq = MatchSeq::Exact(0);
180-
let key = self.role_ident(role_info.identity()).to_string_key();
181-
let value = serialize_struct(&role_info, ErrorCode::IllegalUserInfoFormat, || "")?;
182+
async fn add_role(
183+
&self,
184+
info: RoleInfo,
185+
can_replace: bool,
186+
) -> Result<Result<(), ExistError<role_ident::Resource>>, MetaError> {
187+
let seq = if can_replace {
188+
MatchSeq::GE(0)
189+
} else {
190+
MatchSeq::Exact(0)
191+
};
182192

183-
let upsert_kv = self.kv_api.upsert_kv(UpsertKV::new(
184-
&key,
185-
match_seq,
186-
Operation::Update(value),
187-
None,
188-
));
193+
let key = RoleIdent::new(&self.tenant, &info.name);
194+
let req = UpsertPB::insert(key, info.clone()).with(seq);
195+
let res = self.kv_api.upsert_pb(&req).await?;
189196

190-
let res_seq = upsert_kv.await?.added_seq_or_else(|_v| {
191-
ErrorCode::RoleAlreadyExists(format!("Role '{}' already exists.", role_info.name))
192-
})?;
197+
if !can_replace && res.prev.is_some() {
198+
return Ok(Err(
199+
RoleIdent::new(&self.tenant, &info.name).exist_error(func_name!())
200+
));
201+
}
193202

194-
Ok(res_seq)
203+
Ok(Ok(()))
195204
}
196205

197206
#[async_backtrace::framed]

‎src/query/service/src/interpreters/interpreter_role_create.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ impl Interpreter for CreateRoleInterpreter {
7070
let tenant = self.ctx.get_tenant();
7171
let user_mgr = UserApiProvider::instance();
7272
user_mgr
73-
.add_role(&tenant, RoleInfo::new(&role_name), plan.if_not_exists)
73+
.add_role(&tenant, RoleInfo::new(&role_name), &plan.create_option)
7474
.await?;
7575
RoleCacheManager::instance().force_reload(&tenant).await?;
7676
Ok(PipelineBuildResult::create())

‎src/query/service/tests/it/storages/system.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ use databend_common_meta_app::principal::AuthType;
2525
use databend_common_meta_app::principal::RoleInfo;
2626
use databend_common_meta_app::principal::UserInfo;
2727
use databend_common_meta_app::schema::CreateOption;
28+
use databend_common_meta_app::schema::CreateOption::Create;
29+
use databend_common_meta_app::schema::CreateOption::CreateIfNotExists;
30+
use databend_common_meta_app::schema::CreateOption::CreateOrReplace;
2831
use databend_common_meta_app::storage::StorageFsConfig;
2932
use databend_common_meta_app::storage::StorageParams;
3033
use databend_common_meta_app::storage::StorageS3Config;
@@ -381,17 +384,26 @@ async fn test_roles_table() -> Result<()> {
381384
{
382385
let role_info = RoleInfo::new("test");
383386
UserApiProvider::instance()
384-
.add_role(&tenant, role_info, false)
387+
.add_role(&tenant, role_info, &CreateOrReplace)
385388
.await?;
386389
}
387390

388391
{
389392
let mut role_info = RoleInfo::new("test1");
390393
role_info.grants.grant_role("test".to_string());
391394
UserApiProvider::instance()
392-
.add_role(&tenant, role_info, false)
395+
.add_role(&tenant, role_info, &Create)
393396
.await?;
394397
}
398+
399+
{
400+
let mut role_info = RoleInfo::new("test1");
401+
role_info.grants.grant_role("t2".to_string());
402+
UserApiProvider::instance()
403+
.add_role(&tenant, role_info, &CreateIfNotExists)
404+
.await?;
405+
}
406+
395407
let table = RolesTable::create(1);
396408
let source_plan = table
397409
.read_plan(ctx.clone(), None, None, false, true)

‎src/query/sql/src/planner/binder/binder.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ impl Binder {
388388
self.bind_show_roles(bind_context, show_options).await?
389389
}
390390
Statement::CreateRole {
391-
if_not_exists,
391+
create_option,
392392
role_name,
393393
} => {
394394
if illegal_ident_name(role_name) {
@@ -397,7 +397,7 @@ impl Binder {
397397
));
398398
}
399399
Plan::CreateRole(Box::new(CreateRolePlan {
400-
if_not_exists: *if_not_exists,
400+
create_option: create_option.clone().into(),
401401
role_name: role_name.to_string(),
402402
}))
403403
}

0 commit comments

Comments
 (0)