From 8ade21e4669e0a2cc100615247705feacdf76c5b Mon Sep 17 00:00:00 2001 From: codedump Date: Sat, 15 Apr 2023 16:09:42 +0800 Subject: [PATCH] fix: cannot share a db which is created from share, and test cases (#11081) * chore: disable share statefull test and check bugs * chore: enable share statefull test and fix read share table bug * chore: enable share statefull test and fix read share table bug * fix: cannot share a db which is created from share, and test cases --- src/common/exception/src/exception_code.rs | 1 + src/meta/api/src/share_api_impl.rs | 18 ++++++++-- src/meta/api/src/share_api_test_suite.rs | 33 +++++++++++++++++++ src/meta/app/src/app_error.rs | 33 +++++++++++++++++++ .../src/accessor/share_spec_accessor.rs | 3 +- .../src/accessor/share_table_accessor.rs | 11 ++----- src/query/storages/share/src/lib.rs | 2 +- src/query/storages/share/src/share.rs | 10 ++++-- .../01_sharing/01_0000_basic_sharing.result | 2 ++ .../01_sharing/01_0000_basic_sharing.sh | 7 ++++ 10 files changed, 104 insertions(+), 16 deletions(-) diff --git a/src/common/exception/src/exception_code.rs b/src/common/exception/src/exception_code.rs index f1208b3e15346..ac13149ef9db8 100644 --- a/src/common/exception/src/exception_code.rs +++ b/src/common/exception/src/exception_code.rs @@ -253,6 +253,7 @@ build_exceptions! { UnknownShareEndpoint(2715), UnknownShareEndpointId(2716), UnknownShareTable(2717), + CannotShareDatabaseCreatedFromShare(2718), // Variable error codes. UnknownVariable(2801), diff --git a/src/meta/api/src/share_api_impl.rs b/src/meta/api/src/share_api_impl.rs index 04084da958208..1d2ab7e659caa 100644 --- a/src/meta/api/src/share_api_impl.rs +++ b/src/meta/api/src/share_api_impl.rs @@ -15,6 +15,7 @@ use chrono::DateTime; use chrono::Utc; use common_meta_app::app_error::AppError; +use common_meta_app::app_error::CannotShareDatabaseCreatedFromShare; use common_meta_app::app_error::ShareAccountsAlreadyExists; use common_meta_app::app_error::ShareAlreadyExists; use common_meta_app::app_error::ShareEndpointAlreadyExists; @@ -555,7 +556,8 @@ impl> ShareApi for KV { }; let seq_and_id = - get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant).await?; + get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant, true) + .await?; check_share_object(&share_meta.database, &seq_and_id, &req.object)?; @@ -676,7 +678,8 @@ impl> ShareApi for KV { }; let seq_and_id = - get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant).await?; + get_share_object_seq_and_id(self, &req.object, &share_name_key.tenant, false) + .await?; check_share_object(&share_meta.database, &seq_and_id, &req.object)?; @@ -1503,6 +1506,7 @@ async fn get_share_object_seq_and_id( kv_api: &(impl kvapi::KVApi + ?Sized), obj_name: &ShareGrantObjectName, tenant: &str, + grant: bool, ) -> Result { match obj_name { ShareGrantObjectName::Database(db_name) => { @@ -1517,6 +1521,16 @@ async fn get_share_object_seq_and_id( ) .await?; + if grant && db_meta.from_share.is_some() { + return Err(KVAppError::AppError( + AppError::CannotShareDatabaseCreatedFromShare( + CannotShareDatabaseCreatedFromShare::new( + db_name, + format!("get_share_object_seq_and_id: {}", name_key), + ), + ), + )); + } Ok(ShareGrantObjectSeqAndId::Database( db_meta_seq, db_id, diff --git a/src/meta/api/src/share_api_test_suite.rs b/src/meta/api/src/share_api_test_suite.rs index 817edeff9662a..7251ddd8f669f 100644 --- a/src/meta/api/src/share_api_test_suite.rs +++ b/src/meta/api/src/share_api_test_suite.rs @@ -1243,6 +1243,7 @@ impl ShareApiTestSuite { let tenant1 = "tenant1"; let share1 = "share1"; let share2 = "share2"; + let share3 = "share3"; let db_name = "db1"; let tbl_name = "table1"; let share_id; @@ -1255,6 +1256,10 @@ impl ShareApiTestSuite { tenant: tenant1.to_string(), share_name: share2.to_string(), }; + let share_name3 = ShareNameIdent { + tenant: tenant1.to_string(), + share_name: share3.to_string(), + }; info!("--- get unknown object"); { @@ -1435,6 +1440,34 @@ impl ShareApiTestSuite { info!("create database res: {:?}", res); assert!(res.is_ok()); + // cannot share a database created from a share + { + let req = CreateShareReq { + if_not_exists: false, + share_name: share_name3.clone(), + comment: None, + create_on, + }; + + let res = mt.create_share(req).await; + assert!(res.is_ok()); + + let req = GrantShareObjectReq { + share_name: share_name3.clone(), + object: ShareGrantObjectName::Database(db2.to_string()), + grant_on, + privilege: ShareGrantObjectPrivilege::Usage, + }; + + let res = mt.grant_share_object(req).await; + assert!(res.is_err()); + let err = res.unwrap_err(); + assert_eq!( + ErrorCode::CannotShareDatabaseCreatedFromShare("").code(), + ErrorCode::from(err).code() + ); + } + let req = DropShareReq { if_exists: true, share_name: share_name1.clone(), diff --git a/src/meta/app/src/app_error.rs b/src/meta/app/src/app_error.rs index 4dbec691b40e5..da9555376dbb9 100644 --- a/src/meta/app/src/app_error.rs +++ b/src/meta/app/src/app_error.rs @@ -502,6 +502,24 @@ impl UnknownShareEndpointId { } } +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, thiserror::Error)] +#[error( + "CannotShareDatabaseCreatedFromShare: cannot share database {database_name} which created from share while {context}" +)] +pub struct CannotShareDatabaseCreatedFromShare { + database_name: String, + context: String, +} + +impl CannotShareDatabaseCreatedFromShare { + pub fn new(database_name: impl Into, context: impl Into) -> Self { + Self { + database_name: database_name.into(), + context: context.into(), + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, thiserror::Error)] #[error("TxnRetryMaxTimes: Txn {op} has retry {max_retry} times, abort.")] pub struct TxnRetryMaxTimes { @@ -616,6 +634,9 @@ pub enum AppError { #[error(transparent)] UnknownShareEndpointId(#[from] UnknownShareEndpointId), + + #[error(transparent)] + CannotShareDatabaseCreatedFromShare(#[from] CannotShareDatabaseCreatedFromShare), } impl AppErrorMessage for UnknownDatabase { @@ -775,6 +796,15 @@ impl AppErrorMessage for UnknownShareEndpointId { } } +impl AppErrorMessage for CannotShareDatabaseCreatedFromShare { + fn message(&self) -> String { + format!( + "Cannot share database '{}' which created from share", + self.database_name + ) + } +} + impl AppErrorMessage for TxnRetryMaxTimes { fn message(&self) -> String { format!( @@ -864,6 +894,9 @@ impl From for ErrorCode { AppError::UnknownShareEndpointId(err) => { ErrorCode::UnknownShareEndpointId(err.message()) } + AppError::CannotShareDatabaseCreatedFromShare(err) => { + ErrorCode::CannotShareDatabaseCreatedFromShare(err.message()) + } AppError::TxnRetryMaxTimes(err) => ErrorCode::TxnRetryMaxTimes(err.message()), AppError::DuplicatedUpsertFiles(err) => ErrorCode::DuplicatedUpsertFiles(err.message()), } diff --git a/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs b/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs index ef46b5dcc282c..a37edcee47652 100644 --- a/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs +++ b/src/query/sharing-endpoint/src/accessor/share_spec_accessor.rs @@ -13,6 +13,7 @@ // limitations under the License. use common_exception::Result; +use common_storages_share::get_share_spec_location; use crate::accessor::SharingAccessor; use crate::models; @@ -23,7 +24,7 @@ impl SharingAccessor { #[async_backtrace::framed] pub async fn get_share_spec(tenant: &String) -> Result> { let sharing_accessor = Self::instance(); - let path = sharing_accessor.get_share_spec_location(); + let path = get_share_spec_location(&sharing_accessor.config.tenant); let data = sharing_accessor.op.read(&path).await?; let share_specs: models::SharingConfig = serde_json::from_slice(data.as_slice())?; let mut share_spec_vec = vec![]; diff --git a/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs b/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs index 1d3c5b139abab..70f997609b151 100644 --- a/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs +++ b/src/query/sharing-endpoint/src/accessor/share_table_accessor.rs @@ -15,7 +15,7 @@ use std::time::Duration; use common_exception::Result; -use common_storages_share::SHARE_CONFIG_PREFIX; +use common_storages_share::get_share_spec_location; use crate::accessor::truncate_root; use crate::accessor::SharingAccessor; @@ -35,7 +35,7 @@ impl SharingAccessor { input: &models::LambdaInput, ) -> Result> { let sharing_accessor = Self::instance(); - let path = sharing_accessor.get_share_spec_location(); + let path = get_share_spec_location(&sharing_accessor.config.tenant); let data = sharing_accessor.op.read(&path).await?; let share_specs: models::SharingConfig = serde_json::from_slice(data.as_slice())?; share_specs.get_tables(input) @@ -76,13 +76,6 @@ impl SharingAccessor { Ok(PresignFileResponse::new(&s, input.file_name.clone())) } - pub fn get_share_spec_location(&self) -> String { - format!( - "{}/{}/share_specs.json", - self.config.tenant, SHARE_CONFIG_PREFIX - ) - } - #[async_backtrace::framed] pub async fn get_share_table_spec_presigned_files( input: &models::LambdaInput, diff --git a/src/query/storages/share/src/lib.rs b/src/query/storages/share/src/lib.rs index 22013c8b0c9ca..a5fb8b17f151f 100644 --- a/src/query/storages/share/src/lib.rs +++ b/src/query/storages/share/src/lib.rs @@ -16,7 +16,7 @@ mod share; +pub use share::get_share_spec_location; pub use share::save_share_spec; pub use share::save_share_table_info; pub use share::share_table_info_location; -pub use share::SHARE_CONFIG_PREFIX; diff --git a/src/query/storages/share/src/share.rs b/src/query/storages/share/src/share.rs index 1a455652a25b5..ee2a599746482 100644 --- a/src/query/storages/share/src/share.rs +++ b/src/query/storages/share/src/share.rs @@ -23,17 +23,21 @@ use common_meta_app::share::ShareTableInfoMap; use common_meta_app::share::ShareTableSpec; use opendal::Operator; -pub const SHARE_CONFIG_PREFIX: &str = "_share_config"; +const SHARE_CONFIG_PREFIX: &str = "_share_config"; #[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Default, Eq, PartialEq)] pub struct ShareSpecVec { share_specs: BTreeMap, } +pub fn get_share_spec_location(tenant: &String) -> String { + format!("{}/{}/share_specs.json", SHARE_CONFIG_PREFIX, tenant,) +} + pub fn share_table_info_location(tenant: &str, share_name: &str) -> String { format!( "{}/{}/{}_table_info.json", - tenant, SHARE_CONFIG_PREFIX, share_name + SHARE_CONFIG_PREFIX, tenant, share_name ) } @@ -69,7 +73,7 @@ pub async fn save_share_spec( share_table_info: Option>, ) -> Result<()> { if let Some(share_spec) = spec_vec { - let location = format!("{}/{}/share_specs.json", tenant, SHARE_CONFIG_PREFIX); + let location = get_share_spec_location(tenant); let mut share_spec_vec = ShareSpecVec::default(); for spec in share_spec { let share_name = spec.name.clone(); diff --git a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result index c883476d867e7..552bd0d5e4b42 100644 --- a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result +++ b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.result @@ -18,6 +18,8 @@ INBOUND test_share test_database shared_tenant to_tenant 4 5 6 +cannot grant a database which create from share +ERROR 1105 (HY000) at line 1: Code: 2718, Text = Cannot share database 'shared_db' which created from share. alter table add column and query table data again 1,1 2,1 diff --git a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh index a422c8fd1e6aa..d7dcf8e261694 100755 --- a/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh +++ b/tests/suites/3_stateful_sharing/01_sharing/01_0000_basic_sharing.sh @@ -5,6 +5,7 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) echo "drop test share and database" echo "drop share if exists test_share" | $MYSQL_CLIENT_SHARE_1_CONNECT +echo "drop share if exists my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT echo "drop database if exists test_database" | $MYSQL_CLIENT_SHARE_1_CONNECT echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_2_CONNECT echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_3_CONNECT @@ -39,6 +40,11 @@ echo "CREATE DATABASE if not exists shared_db FROM SHARE shared_tenant.test_shar echo "SELECT * FROM shared_db.t1" | $MYSQL_CLIENT_SHARE_2_CONNECT echo "SELECT * FROM shared_db.t2" | $MYSQL_CLIENT_SHARE_2_CONNECT +# cannot grant a database which create from share +echo "cannot grant a database which create from share" +echo "CREATE SHARE my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT +echo "GRANT USAGE ON DATABASE shared_db TO SHARE my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT + # alter table and query table data again echo "alter table add column and query table data again" echo "ALTER table test_database.t1 add column b UInt64 default 1" | $MYSQL_CLIENT_SHARE_1_CONNECT @@ -66,6 +72,7 @@ echo "SELECT * FROM shared_db.t2" | $MYSQL_CLIENT_SHARE_3_CONNECT ## Drop database and share. echo "all is good" echo "drop share if exists test_share" | $MYSQL_CLIENT_SHARE_1_CONNECT +echo "drop share if exists my_share" | $MYSQL_CLIENT_SHARE_2_CONNECT echo "drop database if exists test_database" | $MYSQL_CLIENT_SHARE_1_CONNECT echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_2_CONNECT echo "drop database if exists shared_db" | $MYSQL_CLIENT_SHARE_3_CONNECT