From a368a5ba150f5f9715256b8495fa7dbd9d00e5d1 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 14:50:34 +0800 Subject: [PATCH 1/7] add license key newtype Signed-off-by: Bugen Zhao --- Cargo.lock | 1 + src/common/src/config.rs | 9 +- src/common/src/system_param/common.rs | 2 +- src/common/src/system_param/diff.rs | 4 +- src/common/src/system_param/mod.rs | 49 +++++----- src/common/src/system_param/reader.rs | 9 +- src/license/src/lib.rs | 90 +++++++++++++++++++ src/license/src/manager.rs | 8 +- src/meta/Cargo.toml | 1 + src/meta/src/controller/system_param.rs | 2 +- .../src/task/task_pubsub_emu_ready_check.rs | 11 ++- 11 files changed, 145 insertions(+), 41 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 220d030807e14..c2454e00fc86c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11041,6 +11041,7 @@ dependencies = [ "risingwave_common_heap_profiling", "risingwave_connector", "risingwave_hummock_sdk", + "risingwave_license", "risingwave_meta_dashboard", "risingwave_meta_model_migration", "risingwave_meta_model_v2", diff --git a/src/common/src/config.rs b/src/common/src/config.rs index eeb393edb4ab9..d4b9501d6e0d4 100644 --- a/src/common/src/config.rs +++ b/src/common/src/config.rs @@ -1249,7 +1249,7 @@ impl SystemConfig { macro_rules! fields { ($({ $field:ident, $($rest:tt)* },)*) => { SystemParams { - $($field: self.$field,)* + $($field: self.$field.map(Into::into),)* ..Default::default() // deprecated fields } }; @@ -2382,10 +2382,9 @@ mod tests { use super::*; fn default_config_for_docs() -> RwConfig { - let mut config = RwConfig::default(); - // Set `license_key` to empty to avoid showing the test-only license key in the docs. - config.system.license_key = Some("".to_owned()); - config + // Although we have `license_key` default to a test-only value in the code, + // it gets redacted during serialization, so we don't need to do anything special here. + RwConfig::default() } /// This test ensures that `config/example.toml` is up-to-date with the default values specified diff --git a/src/common/src/system_param/common.rs b/src/common/src/system_param/common.rs index 7aec7d14f012b..832e07aee7888 100644 --- a/src/common/src/system_param/common.rs +++ b/src/common/src/system_param/common.rs @@ -38,7 +38,7 @@ impl CommonHandler { } // Refresh the license key. if let Some(key) = diff.license_key.as_ref() { - LicenseManager::get().refresh(key); + LicenseManager::get().refresh(key.as_ref()); } } } diff --git a/src/common/src/system_param/diff.rs b/src/common/src/system_param/diff.rs index 243b3e247bec8..c6705e70a047e 100644 --- a/src/common/src/system_param/diff.rs +++ b/src/common/src/system_param/diff.rs @@ -40,7 +40,7 @@ impl SystemParamsDiff { ($({ $field:ident, $($rest:tt)* },)*) => { $( if curr.$field() != prev.$field() { - diff.$field = Some(curr.$field().to_owned()); + diff.$field = Some(curr.$field().into()); } )* }; @@ -57,7 +57,7 @@ impl SystemParamsDiff { ($({ $field:ident, $($rest:tt)* },)*) => { Self { $( - $field: Some(initial.$field().to_owned()), + $field: Some(initial.$field().into()), )* } }; diff --git a/src/common/src/system_param/mod.rs b/src/common/src/system_param/mod.rs index f4f9e931fa1a0..c97e062136d83 100644 --- a/src/common/src/system_param/mod.rs +++ b/src/common/src/system_param/mod.rs @@ -30,7 +30,7 @@ use std::ops::RangeBounds; use std::str::FromStr; use paste::paste; -use risingwave_license::TEST_PAID_LICENSE_KEY; +use risingwave_license::{LicenseKey, LicenseKeyRef, TEST_PAID_LICENSE_KEY}; use risingwave_pb::meta::PbSystemParams; use self::diff::SystemParamsDiff; @@ -60,14 +60,15 @@ impl_param_value!(u32); impl_param_value!(u64); impl_param_value!(f64); impl_param_value!(String => &'a str); +impl_param_value!(LicenseKey => LicenseKeyRef<'a>); /// Set the default value of `license_key` to [`TEST_PAID_LICENSE_KEY`] in debug mode. -fn default_license_key() -> String { +fn default_license_key() -> LicenseKey { if cfg!(debug_assertions) { TEST_PAID_LICENSE_KEY.to_owned() } else { "".to_owned() - } + }.into() } /// Define all system parameters here. @@ -83,23 +84,23 @@ fn default_license_key() -> String { macro_rules! for_all_params { ($macro:ident) => { $macro! { - // name type default value mut? doc - { barrier_interval_ms, u32, Some(1000_u32), true, "The interval of periodic barrier.", }, - { checkpoint_frequency, u64, Some(1_u64), true, "There will be a checkpoint for every n barriers.", }, - { sstable_size_mb, u32, Some(256_u32), false, "Target size of the Sstable.", }, - { parallel_compact_size_mb, u32, Some(512_u32), false, "The size of parallel task for one compact/flush job.", }, - { block_size_kb, u32, Some(64_u32), false, "Size of each block in bytes in SST.", }, - { bloom_false_positive, f64, Some(0.001_f64), false, "False positive probability of bloom filter.", }, - { state_store, String, None, false, "URL for the state store", }, - { data_directory, String, None, false, "Remote directory for storing data and metadata objects.", }, - { backup_storage_url, String, None, true, "Remote storage url for storing snapshots.", }, - { backup_storage_directory, String, None, true, "Remote directory for storing snapshots.", }, - { max_concurrent_creating_streaming_jobs, u32, Some(1_u32), true, "Max number of concurrent creating streaming jobs.", }, - { pause_on_next_bootstrap, bool, Some(false), true, "Whether to pause all data sources on next bootstrap.", }, - { enable_tracing, bool, Some(false), true, "Whether to enable distributed tracing.", }, - { use_new_object_prefix_strategy, bool, None, false, "Whether to split object prefix.", }, - { license_key, String, Some(default_license_key()), true, "The license key to activate enterprise features.", }, - { time_travel_retention_ms, u64, Some(0_u64), true, "The data retention period for time travel, where 0 indicates that it's disabled.", }, + // name type default value mut? doc + { barrier_interval_ms, u32, Some(1000_u32), true, "The interval of periodic barrier.", }, + { checkpoint_frequency, u64, Some(1_u64), true, "There will be a checkpoint for every n barriers.", }, + { sstable_size_mb, u32, Some(256_u32), false, "Target size of the Sstable.", }, + { parallel_compact_size_mb, u32, Some(512_u32), false, "The size of parallel task for one compact/flush job.", }, + { block_size_kb, u32, Some(64_u32), false, "Size of each block in bytes in SST.", }, + { bloom_false_positive, f64, Some(0.001_f64), false, "False positive probability of bloom filter.", }, + { state_store, String, None, false, "URL for the state store", }, + { data_directory, String, None, false, "Remote directory for storing data and metadata objects.", }, + { backup_storage_url, String, None, true, "Remote storage url for storing snapshots.", }, + { backup_storage_directory, String, None, true, "Remote directory for storing snapshots.", }, + { max_concurrent_creating_streaming_jobs, u32, Some(1_u32), true, "Max number of concurrent creating streaming jobs.", }, + { pause_on_next_bootstrap, bool, Some(false), true, "Whether to pause all data sources on next bootstrap.", }, + { enable_tracing, bool, Some(false), true, "Whether to enable distributed tracing.", }, + { use_new_object_prefix_strategy, bool, None, false, "Whether to split object prefix.", }, + { license_key, risingwave_license::LicenseKey, Some(default_license_key()), true, "The license key to activate enterprise features.", }, + { time_travel_retention_ms, u64, Some(0_u64), true, "The data retention period for time travel, where 0 indicates that it's disabled.", }, } }; } @@ -203,7 +204,7 @@ macro_rules! impl_derive_missing_fields { pub fn derive_missing_fields(params: &mut PbSystemParams) { $( if params.$field.is_none() && let Some(v) = OverrideFromParams::$field(params) { - params.$field = Some(v); + params.$field = Some(v.into()); } )* } @@ -331,7 +332,7 @@ macro_rules! impl_set_system_param { match key { $( key_of!($field) => { - let v = if let Some(v) = value { + let v: $type = if let Some(v) = value { #[allow(rw::format_error)] v.as_ref().parse().map_err(|e| format!("cannot parse parameter value: {e}"))? } else { @@ -346,7 +347,7 @@ macro_rules! impl_set_system_param { $field: Some(v.to_owned()), ..Default::default() }; - params.$field = Some(v); + params.$field = Some(v.into()); Ok(Some((new_value, diff))) } else { Ok(None) @@ -383,7 +384,7 @@ macro_rules! impl_system_params_for_test { pub fn system_params_for_test() -> PbSystemParams { let mut ret = PbSystemParams { $( - $field: $default, + $field: ($default as Option<$type>).map(Into::into), )* ..Default::default() // `None` for deprecated params }; diff --git a/src/common/src/system_param/reader.rs b/src/common/src/system_param/reader.rs index 505e36693f1d5..dbedbcc8d0e3b 100644 --- a/src/common/src/system_param/reader.rs +++ b/src/common/src/system_param/reader.rs @@ -14,6 +14,7 @@ use std::borrow::Borrow; +use risingwave_license::LicenseKeyRef; use risingwave_pb::meta::PbSystemParams; use super::{default, ParamValue}; @@ -169,8 +170,12 @@ where .unwrap_or_else(default::enable_tracing) } - fn license_key(&self) -> &str { - self.inner().license_key.as_deref().unwrap_or_default() + fn license_key(&self) -> LicenseKeyRef<'_> { + self.inner() + .license_key + .as_deref() + .unwrap_or_default() + .into() } fn time_travel_retention_ms(&self) -> u64 { diff --git a/src/license/src/lib.rs b/src/license/src/lib.rs index 7f2b25d8f3fb4..f9a9abb6f2bc5 100644 --- a/src/license/src/lib.rs +++ b/src/license/src/lib.rs @@ -16,4 +16,94 @@ mod feature; mod manager; pub use feature::*; +pub use key::*; pub use manager::*; + +mod key { + use std::convert::Infallible; + use std::str::FromStr; + + use serde::{Deserialize, Serialize}; + + #[derive(Clone, Copy, Deserialize)] + #[serde(transparent)] + pub struct LicenseKey(pub(crate) T); + + pub type LicenseKeyRef<'a> = LicenseKey<&'a str>; + + impl From for LicenseKey { + fn from(t: T) -> Self { + Self(t) + } + } + + impl PartialEq> for LicenseKey + where + A: AsRef, + B: AsRef, + { + fn eq(&self, other: &LicenseKey) -> bool { + self.0.as_ref() == other.0.as_ref() + } + } + + impl> Eq for LicenseKey {} + + impl> LicenseKey { + fn redact_str(&self) -> &str { + let s = self.0.as_ref(); + if s.is_empty() { + "" + } else { + "[REDACTED]" + } + } + } + + impl> std::fmt::Debug for LicenseKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.redact_str()) + } + } + + impl> std::fmt::Display for LicenseKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(self, f) + } + } + + impl> Serialize for LicenseKey { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + self.redact_str().serialize(serializer) + } + } + + impl FromStr for LicenseKey { + type Err = Infallible; + + fn from_str(s: &str) -> Result { + Ok(Self(s.to_owned())) + } + } + + impl> Into for LicenseKey { + fn into(self) -> String { + self.0.as_ref().to_owned() + } + } + + impl<'a> From> for LicenseKey { + fn from(t: LicenseKeyRef<'a>) -> Self { + Self(t.0.to_owned()) + } + } + + impl LicenseKey { + pub fn as_ref(&self) -> LicenseKeyRef<'_> { + LicenseKey(self.0.as_ref()) + } + } +} diff --git a/src/license/src/manager.rs b/src/license/src/manager.rs index f815a145ba01d..2702490e02276 100644 --- a/src/license/src/manager.rs +++ b/src/license/src/manager.rs @@ -19,6 +19,8 @@ use serde::Deserialize; use thiserror::Error; use thiserror_ext::AsReport; +use crate::LicenseKeyRef; + /// License tier. /// /// Each enterprise [`Feature`](super::Feature) is available for a specific tier and above. @@ -130,7 +132,8 @@ impl LicenseManager { } /// Refresh the license with the given license key. - pub fn refresh(&self, license_key: &str) { + pub fn refresh(&self, license_key: LicenseKeyRef<'_>) { + let license_key = license_key.0; let mut inner = self.inner.write().unwrap(); // Empty license key means unset. Use the default one here. @@ -190,10 +193,11 @@ mod tests { use expect_test::expect; use super::*; + use crate::LicenseKey; fn do_test(key: &str, expect: expect_test::Expect) { let manager = LicenseManager::new(); - manager.refresh(key); + manager.refresh(LicenseKey(key)); match manager.license() { Ok(license) => expect.assert_debug_eq(&license), diff --git a/src/meta/Cargo.toml b/src/meta/Cargo.toml index 9e35e94896e5a..b35ce9e73d96b 100644 --- a/src/meta/Cargo.toml +++ b/src/meta/Cargo.toml @@ -55,6 +55,7 @@ risingwave_common = { workspace = true } risingwave_common_heap_profiling = { workspace = true } risingwave_connector = { workspace = true } risingwave_hummock_sdk = { workspace = true } +risingwave_license = { workspace = true } risingwave_meta_dashboard = { workspace = true } risingwave_meta_model_migration = { workspace = true } risingwave_meta_model_v2 = { workspace = true } diff --git a/src/meta/src/controller/system_param.rs b/src/meta/src/controller/system_param.rs index 75b1e22cb1f9f..a1da3f2c6ea40 100644 --- a/src/meta/src/controller/system_param.rs +++ b/src/meta/src/controller/system_param.rs @@ -60,7 +60,7 @@ macro_rules! impl_system_params_from_db { match model.name.as_str() { $( key_of!($field) => { - params.$field = Some(model.value.parse::<$type>().unwrap()); + params.$field = Some(model.value.parse::<$type>().unwrap().into()); false } )* diff --git a/src/risedevtool/src/task/task_pubsub_emu_ready_check.rs b/src/risedevtool/src/task/task_pubsub_emu_ready_check.rs index f7ba1abacefab..8a3779568ece4 100644 --- a/src/risedevtool/src/task/task_pubsub_emu_ready_check.rs +++ b/src/risedevtool/src/task/task_pubsub_emu_ready_check.rs @@ -36,10 +36,13 @@ impl Task for PubsubReadyTaskCheck { ctx.pb.set_message("waiting for online..."); // environment variables to use the pubsub emulator - std::env::set_var( - "PUBSUB_EMULATOR_HOST", - format!("{}:{}", self.config.address, self.config.port), - ); + // SAFETY: RiseDev is for development purposes only. + unsafe { + std::env::set_var( + "PUBSUB_EMULATOR_HOST", + format!("{}:{}", self.config.address, self.config.port), + ); + } thread::sleep(Duration::from_secs(5)); let async_runtime = tokio::runtime::Builder::new_current_thread() From e97fda4a040cec2f50cbd840bfa7c54a744edc58 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 15:05:31 +0800 Subject: [PATCH 2/7] refactor code Signed-off-by: Bugen Zhao --- src/common/src/system_param/mod.rs | 15 +--- src/license/src/key.rs | 127 +++++++++++++++++++++++++++++ src/license/src/lib.rs | 90 +------------------- src/license/src/manager.rs | 10 +-- 4 files changed, 132 insertions(+), 110 deletions(-) create mode 100644 src/license/src/key.rs diff --git a/src/common/src/system_param/mod.rs b/src/common/src/system_param/mod.rs index c97e062136d83..e98ec1021487a 100644 --- a/src/common/src/system_param/mod.rs +++ b/src/common/src/system_param/mod.rs @@ -30,7 +30,7 @@ use std::ops::RangeBounds; use std::str::FromStr; use paste::paste; -use risingwave_license::{LicenseKey, LicenseKeyRef, TEST_PAID_LICENSE_KEY}; +use risingwave_license::{LicenseKey, LicenseKeyRef}; use risingwave_pb::meta::PbSystemParams; use self::diff::SystemParamsDiff; @@ -62,15 +62,6 @@ impl_param_value!(f64); impl_param_value!(String => &'a str); impl_param_value!(LicenseKey => LicenseKeyRef<'a>); -/// Set the default value of `license_key` to [`TEST_PAID_LICENSE_KEY`] in debug mode. -fn default_license_key() -> LicenseKey { - if cfg!(debug_assertions) { - TEST_PAID_LICENSE_KEY.to_owned() - } else { - "".to_owned() - }.into() -} - /// Define all system parameters here. /// /// To match all these information, write the match arm as follows: @@ -99,7 +90,7 @@ macro_rules! for_all_params { { pause_on_next_bootstrap, bool, Some(false), true, "Whether to pause all data sources on next bootstrap.", }, { enable_tracing, bool, Some(false), true, "Whether to enable distributed tracing.", }, { use_new_object_prefix_strategy, bool, None, false, "Whether to split object prefix.", }, - { license_key, risingwave_license::LicenseKey, Some(default_license_key()), true, "The license key to activate enterprise features.", }, + { license_key, risingwave_license::LicenseKey, Some(Default::default()), true, "The license key to activate enterprise features.", }, { time_travel_retention_ms, u64, Some(0_u64), true, "The data retention period for time travel, where 0 indicates that it's disabled.", }, } }; @@ -161,8 +152,6 @@ macro_rules! def_default { pub mod default { use std::sync::LazyLock; - use super::*; - for_all_params!(def_default_opt); for_all_params!(def_default); } diff --git a/src/license/src/key.rs b/src/license/src/key.rs new file mode 100644 index 0000000000000..01d8ac313f791 --- /dev/null +++ b/src/license/src/key.rs @@ -0,0 +1,127 @@ +// Copyright 2024 RisingWave Labs +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::convert::Infallible; +use std::str::FromStr; + +use serde::{Deserialize, Serialize}; + +/// A license key with the paid tier that only works in tests. +pub(crate) const TEST_PAID_LICENSE_KEY_CONTENT: &str = + "eyJhbGciOiJSUzUxMiIsInR5cCI6IkpXVCJ9.\ + eyJzdWIiOiJydy10ZXN0IiwidGllciI6InBhaWQiLCJpc3MiOiJ0ZXN0LnJpc2luZ3dhdmUuY29tIiwiZXhwIjo5OTk5OTk5OTk5fQ.\ + c6Gmb6xh3dBDYX_4cOnHUbwRXJbUCM7W3mrJA77nLC5FkoOLpGstzvQ7qfnPVBu412MFtKRDvh-Lk8JwG7pVa0WLw16DeHTtVHxZukMTZ1Q_ciZ1xKeUx_pwUldkVzv6c9j99gNqPSyTjzOXTdKlidBRLer2zP0v3Lf-ZxnMG0tEcIbTinTb3BNCtAQ8bwBSRP-X48cVTWafjaZxv_zGiJT28uV3bR6jwrorjVB4VGvqhsJi6Fd074XOmUlnOleoAtyzKvjmGC5_FvnL0ztIe_I0z_pyCMfWpyJ_J4C7rCP1aVWUImyoowLmVDA-IKjclzOW5Fvi0wjXsc6OckOc_A"; + +/// A newtype wrapping `String` or `&str` for the license key. +/// +/// - The default value is set to [`TEST_PAID_LICENSE_KEY_CONTENT`] in debug mode, and empty otherwise. +/// - The content will be redacted when printed or serialized. +#[derive(Clone, Copy, Deserialize)] +#[serde(transparent)] +pub struct LicenseKey(pub(crate) T); + +/// Alias for [`LicenseKey<&str>`]. +pub type LicenseKeyRef<'a> = LicenseKey<&'a str>; + +impl> Default for LicenseKey { + fn default() -> Self { + Self( + if cfg!(debug_assertions) { + TEST_PAID_LICENSE_KEY_CONTENT + } else { + "" + } + .into(), + ) + } +} + +impl From for LicenseKey { + fn from(t: T) -> Self { + Self(t) + } +} + +impl PartialEq> for LicenseKey +where + A: AsRef, + B: AsRef, +{ + fn eq(&self, other: &LicenseKey) -> bool { + self.0.as_ref() == other.0.as_ref() + } +} + +impl> Eq for LicenseKey {} + +impl> LicenseKey { + fn redact_str(&self) -> &str { + let s = self.0.as_ref(); + if s.is_empty() { + "" + } else if self == &LicenseKeyRef::default() { + "" + } else { + "" + } + } +} + +impl> std::fmt::Debug for LicenseKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.redact_str()) + } +} + +impl> std::fmt::Display for LicenseKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(self, f) + } +} + +impl> Serialize for LicenseKey { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + self.redact_str().serialize(serializer) + } +} + +impl FromStr for LicenseKey { + type Err = Infallible; + + fn from_str(s: &str) -> Result { + Ok(Self(s.to_owned())) + } +} + +impl> Into for LicenseKey { + fn into(self) -> String { + self.0.as_ref().to_owned() + } +} + +impl<'a> From> for LicenseKey { + fn from(t: LicenseKeyRef<'a>) -> Self { + Self(t.0.to_owned()) + } +} + +impl LicenseKey { + /// Convert to a reference. + pub fn as_ref(&self) -> LicenseKeyRef<'_> { + LicenseKey(self.0.as_ref()) + } +} diff --git a/src/license/src/lib.rs b/src/license/src/lib.rs index f9a9abb6f2bc5..bdcac90441043 100644 --- a/src/license/src/lib.rs +++ b/src/license/src/lib.rs @@ -13,97 +13,9 @@ // limitations under the License. mod feature; +mod key; mod manager; pub use feature::*; pub use key::*; pub use manager::*; - -mod key { - use std::convert::Infallible; - use std::str::FromStr; - - use serde::{Deserialize, Serialize}; - - #[derive(Clone, Copy, Deserialize)] - #[serde(transparent)] - pub struct LicenseKey(pub(crate) T); - - pub type LicenseKeyRef<'a> = LicenseKey<&'a str>; - - impl From for LicenseKey { - fn from(t: T) -> Self { - Self(t) - } - } - - impl PartialEq> for LicenseKey - where - A: AsRef, - B: AsRef, - { - fn eq(&self, other: &LicenseKey) -> bool { - self.0.as_ref() == other.0.as_ref() - } - } - - impl> Eq for LicenseKey {} - - impl> LicenseKey { - fn redact_str(&self) -> &str { - let s = self.0.as_ref(); - if s.is_empty() { - "" - } else { - "[REDACTED]" - } - } - } - - impl> std::fmt::Debug for LicenseKey { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.redact_str()) - } - } - - impl> std::fmt::Display for LicenseKey { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - std::fmt::Debug::fmt(self, f) - } - } - - impl> Serialize for LicenseKey { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.redact_str().serialize(serializer) - } - } - - impl FromStr for LicenseKey { - type Err = Infallible; - - fn from_str(s: &str) -> Result { - Ok(Self(s.to_owned())) - } - } - - impl> Into for LicenseKey { - fn into(self) -> String { - self.0.as_ref().to_owned() - } - } - - impl<'a> From> for LicenseKey { - fn from(t: LicenseKeyRef<'a>) -> Self { - Self(t.0.to_owned()) - } - } - - impl LicenseKey { - pub fn as_ref(&self) -> LicenseKeyRef<'_> { - LicenseKey(self.0.as_ref()) - } - } -} diff --git a/src/license/src/manager.rs b/src/license/src/manager.rs index 2702490e02276..cac51105358ae 100644 --- a/src/license/src/manager.rs +++ b/src/license/src/manager.rs @@ -180,12 +180,6 @@ impl LicenseManager { } } -/// A license key with the paid tier that only works in tests. -pub const TEST_PAID_LICENSE_KEY: &str = - "eyJhbGciOiJSUzUxMiIsInR5cCI6IkpXVCJ9.\ - eyJzdWIiOiJydy10ZXN0IiwidGllciI6InBhaWQiLCJpc3MiOiJ0ZXN0LnJpc2luZ3dhdmUuY29tIiwiZXhwIjo5OTk5OTk5OTk5fQ.\ - c6Gmb6xh3dBDYX_4cOnHUbwRXJbUCM7W3mrJA77nLC5FkoOLpGstzvQ7qfnPVBu412MFtKRDvh-Lk8JwG7pVa0WLw16DeHTtVHxZukMTZ1Q_ciZ1xKeUx_pwUldkVzv6c9j99gNqPSyTjzOXTdKlidBRLer2zP0v3Lf-ZxnMG0tEcIbTinTb3BNCtAQ8bwBSRP-X48cVTWafjaZxv_zGiJT28uV3bR6jwrorjVB4VGvqhsJi6Fd074XOmUlnOleoAtyzKvjmGC5_FvnL0ztIe_I0z_pyCMfWpyJ_J4C7rCP1aVWUImyoowLmVDA-IKjclzOW5Fvi0wjXsc6OckOc_A"; - // Tests below only work in debug mode. #[cfg(debug_assertions)] #[cfg(test)] @@ -193,7 +187,7 @@ mod tests { use expect_test::expect; use super::*; - use crate::LicenseKey; + use crate::{LicenseKey, TEST_PAID_LICENSE_KEY_CONTENT}; fn do_test(key: &str, expect: expect_test::Expect) { let manager = LicenseManager::new(); @@ -208,7 +202,7 @@ mod tests { #[test] fn test_paid_license_key() { do_test( - TEST_PAID_LICENSE_KEY, + TEST_PAID_LICENSE_KEY_CONTENT, expect![[r#" License { sub: "rw-test", From e6411a38a14aa497b6cc2f73f0361cae35304c5b Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 15:17:44 +0800 Subject: [PATCH 3/7] add test Signed-off-by: Bugen Zhao --- e2e_test/error_ui/simple/license.slt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/e2e_test/error_ui/simple/license.slt b/e2e_test/error_ui/simple/license.slt index 22bd4c60cf689..fa04699efe380 100644 --- a/e2e_test/error_ui/simple/license.slt +++ b/e2e_test/error_ui/simple/license.slt @@ -2,6 +2,12 @@ statement ok ALTER SYSTEM SET license_key TO 'eyJhbGciOiJSUzUxMiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJydy10ZXN0IiwidGllciI6ImZyZWUiLCJpc3MiOiJ0ZXN0LnJpc2luZ3dhdmUuY29tIiwiZXhwIjo5OTk5OTk5OTk5fQ.ALC3Kc9LI6u0S-jeMB1YTxg1k8Azxwvc750ihuSZgjA_e1OJC9moxMvpLrHdLZDzCXHjBYi0XJ_1lowmuO_0iPEuPqN5AFpDV1ywmzJvGmMCMtw3A2wuN7hhem9OsWbwe6lzdwrefZLipyo4GZtIkg5ZdwGuHzm33zsM-X5gl_Ns4P6axHKiorNSR6nTAyA6B32YVET_FAM2YJQrXqpwA61wn1XLfarZqpdIQyJ5cgyiC33BFBlUL3lcRXLMLeYe6TjYGeV4K63qARCjM9yeOlsRbbW5ViWeGtR2Yf18pN8ysPXdbaXm_P_IVhl3jCTDJt9ctPh6pUCbkt36FZqO9A'; +# The key should be redacted when trying to retrieve it. +query T +SELECT setting FROM pg_settings WHERE name = 'license_key'; +---- + + query error SELECT rw_test_paid_tier(); ---- @@ -36,6 +42,12 @@ Caused by these errors (recent errors listed first): statement ok ALTER SYSTEM SET license_key TO ''; +# Not showing `` for license key empty (unset). +query T +SELECT setting FROM pg_settings WHERE name = 'license_key'; +---- +(empty) + query error SELECT rw_test_paid_tier(); ---- @@ -53,6 +65,12 @@ Hint: You may want to set a license key with `ALTER SYSTEM SET license_key = '.. statement ok ALTER SYSTEM SET license_key TO DEFAULT; +# Show `` if the license key is set to default. +query T +SELECT setting FROM pg_settings WHERE name = 'license_key'; +---- + + query T SELECT rw_test_paid_tier(); ---- From 0d8e2e3546c3d5791077219dfd53d5a7a57edbdb Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 15:22:15 +0800 Subject: [PATCH 4/7] add more docs Signed-off-by: Bugen Zhao --- src/common/src/system_param/reader.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/common/src/system_param/reader.rs b/src/common/src/system_param/reader.rs index dbedbcc8d0e3b..4c869f7465ce6 100644 --- a/src/common/src/system_param/reader.rs +++ b/src/common/src/system_param/reader.rs @@ -21,9 +21,15 @@ use super::{default, ParamValue}; use crate::for_all_params; /// Information about a system parameter. +/// +/// Used to display to users through `pg_settings` system table and `SHOW PARAMETERS` command. pub struct ParameterInfo { pub name: &'static str, pub mutable: bool, + /// The [`ToString`] representation of the parameter value. + /// + /// Certain parameters, such as `license_key`, may be sensitive, and redaction is applied to them in their newtypes. + /// As a result, we get the redacted value here for display. pub value: String, pub description: &'static str, } From bff1ca89c8a99081b37da0807cb99fc63797fbf6 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 15:33:43 +0800 Subject: [PATCH 5/7] leave it blank in docs Signed-off-by: Bugen Zhao --- src/common/src/config.rs | 9 ++++++--- src/license/src/key.rs | 5 +++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/common/src/config.rs b/src/common/src/config.rs index d4b9501d6e0d4..083a8b77b4f33 100644 --- a/src/common/src/config.rs +++ b/src/common/src/config.rs @@ -2379,12 +2379,15 @@ pub struct CompactionConfig { #[cfg(test)] mod tests { + use risingwave_license::LicenseKey; + use super::*; fn default_config_for_docs() -> RwConfig { - // Although we have `license_key` default to a test-only value in the code, - // it gets redacted during serialization, so we don't need to do anything special here. - RwConfig::default() + let mut config = RwConfig::default(); + // Set `license_key` to empty in the docs to avoid any confusion. + config.system.license_key = Some(LicenseKey::empty()); + config } /// This test ensures that `config/example.toml` is up-to-date with the default values specified diff --git a/src/license/src/key.rs b/src/license/src/key.rs index 01d8ac313f791..65e726c6e8388 100644 --- a/src/license/src/key.rs +++ b/src/license/src/key.rs @@ -120,6 +120,11 @@ impl<'a> From> for LicenseKey { } impl LicenseKey { + /// Create an empty license key, which means no license key is set. + pub fn empty() -> Self { + Self(String::new()) + } + /// Convert to a reference. pub fn as_ref(&self) -> LicenseKeyRef<'_> { LicenseKey(self.0.as_ref()) From db8b32f69e144714421072527f3b0cf489469755 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 16:08:12 +0800 Subject: [PATCH 6/7] fix clippy Signed-off-by: Bugen Zhao --- src/license/src/key.rs | 6 +++--- src/meta/src/barrier/mod.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/license/src/key.rs b/src/license/src/key.rs index 65e726c6e8388..f27ad22719e8b 100644 --- a/src/license/src/key.rs +++ b/src/license/src/key.rs @@ -107,9 +107,9 @@ impl FromStr for LicenseKey { } } -impl> Into for LicenseKey { - fn into(self) -> String { - self.0.as_ref().to_owned() +impl> From> for String { + fn from(val: LicenseKey) -> Self { + val.0.as_ref().to_owned() } } diff --git a/src/meta/src/barrier/mod.rs b/src/meta/src/barrier/mod.rs index e2a1b24794c6b..cde11303397cd 100644 --- a/src/meta/src/barrier/mod.rs +++ b/src/meta/src/barrier/mod.rs @@ -756,7 +756,7 @@ impl GlobalBarrierManager { } self.failure_recovery(err).await; } else { - warn!(e = ?e.as_report(), worker_id, "no barrier to collect from worker, ignore err"); + warn!(e = %e.as_report(), worker_id, "no barrier to collect from worker, ignore err"); } } } From 343cdaecdd739415018ac0440a583bfff0c6d421 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Tue, 6 Aug 2024 16:41:19 +0800 Subject: [PATCH 7/7] use reader for logs Signed-off-by: Bugen Zhao --- src/common/src/system_param/reader.rs | 27 ++++++++++++++++++++++-- src/meta/src/controller/system_param.rs | 2 +- src/meta/src/manager/system_param/mod.rs | 2 +- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/common/src/system_param/reader.rs b/src/common/src/system_param/reader.rs index 4c869f7465ce6..e4eb9806e2a5c 100644 --- a/src/common/src/system_param/reader.rs +++ b/src/common/src/system_param/reader.rs @@ -38,10 +38,11 @@ macro_rules! define_system_params_read_trait { ($({ $field:ident, $type:ty, $default:expr, $is_mutable:expr, $doc:literal, $($rest:tt)* },)*) => { /// The trait delegating reads on [`risingwave_pb::meta::SystemParams`]. /// - /// For 2 purposes: + /// Purposes: /// - Avoid misuse of deprecated fields by hiding their getters. /// - Abstract fallback logic for fields that might not be provided by meta service due to backward /// compatibility. + /// - Redact sensitive fields by returning a newtype around the original value. pub trait SystemParamsRead { $( #[doc = $doc] @@ -70,11 +71,33 @@ for_all_params!(define_system_params_read_trait); /// The wrapper delegating reads on [`risingwave_pb::meta::SystemParams`]. /// /// See [`SystemParamsRead`] for more details. -#[derive(Clone, Debug, PartialEq)] +// TODO: should we manually impl `PartialEq` by comparing each field with read functions? +#[derive(Clone, PartialEq)] pub struct SystemParamsReader { inner: I, } +// TODO: should ban `Debug` for inner `SystemParams`. +// https://github.com/risingwavelabs/risingwave/pull/17906#discussion_r1705056943 +macro_rules! impl_system_params_reader_debug { + ($({ $field:ident, $($rest:tt)* },)*) => { + impl std::fmt::Debug for SystemParamsReader + where + I: Borrow, + { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SystemParamsReader") + $( + .field(stringify!($field), &self.$field()) + )* + .finish() + } + } + }; +} + +for_all_params!(impl_system_params_reader_debug); + impl From for SystemParamsReader where I: Borrow, diff --git a/src/meta/src/controller/system_param.rs b/src/meta/src/controller/system_param.rs index a1da3f2c6ea40..a4b5ff239c074 100644 --- a/src/meta/src/controller/system_param.rs +++ b/src/meta/src/controller/system_param.rs @@ -143,7 +143,7 @@ impl SystemParamsController { let params = SystemParameter::find().all(&db).await?; let params = merge_params(system_params_from_db(params)?, init_params); - info!("system parameters: {:?}", params); + info!(initial_params = ?SystemParamsReader::new(¶ms), "initialize system parameters"); check_missing_params(¶ms).map_err(|e| anyhow!(e))?; let ctl = Self { diff --git a/src/meta/src/manager/system_param/mod.rs b/src/meta/src/manager/system_param/mod.rs index dbc4a743fa72c..d77be8e5d03d4 100644 --- a/src/meta/src/manager/system_param/mod.rs +++ b/src/meta/src/manager/system_param/mod.rs @@ -77,7 +77,7 @@ impl SystemParamsManager { return Err(require_sql_meta_store_err().into()); } - info!("system parameters: {:?}", params); + info!(initial_params = ?SystemParamsReader::new(¶ms), "initialize system parameters"); check_missing_params(¶ms).map_err(|e| anyhow!(e))?; Ok(Self {