Skip to content
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.

Sign up for GitHub

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

Add a non breaking feature for module configs to support validation & rejection of config sets #144

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Add a non breaking feature for module configs to support validation & rejection of config sets #144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2046261
Add a non breaking feature for module configs to support validation &…
KarthikSubbarao Dec 4, 2024
809af26
cargo fmt + styling changes
KarthikSubbarao Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 59 additions & 1 deletion examples/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ use valkey_module::alloc::ValkeyAlloc;
use valkey_module::{
configuration::{ConfigurationContext, ConfigurationFlags},
enum_configuration, valkey_module, ConfigurationValue, Context, ValkeyGILGuard, ValkeyResult,
ValkeyString, ValkeyValue,
ValkeyString, ValkeyValue, ValkeyError
};

enum_configuration! {
#[derive(PartialEq)]
enum EnumConfiguration {
Val1 = 1,
Val2 = 2,
Expand All @@ -21,15 +22,21 @@ enum_configuration! {
lazy_static! {
static ref NUM_OF_CONFIGURATION_CHANGES: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_I64: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_REJECT_I64: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_ATOMIC_I64: AtomicI64 = AtomicI64::new(1);
static ref CONFIGURATION_VALKEY_STRING: ValkeyGILGuard<ValkeyString> =
ValkeyGILGuard::new(ValkeyString::create(None, "default"));
static ref CONFIGURATION_REJECT_VALKEY_STRING: ValkeyGILGuard<ValkeyString> =
ValkeyGILGuard::new(ValkeyString::create(None, "default"));
static ref CONFIGURATION_STRING: ValkeyGILGuard<String> = ValkeyGILGuard::new("default".into());
static ref CONFIGURATION_MUTEX_STRING: Mutex<String> = Mutex::new("default".into());
static ref CONFIGURATION_ATOMIC_BOOL: AtomicBool = AtomicBool::default();
static ref CONFIGURATION_BOOL: ValkeyGILGuard<bool> = ValkeyGILGuard::default();
static ref CONFIGURATION_REJECT_BOOL: ValkeyGILGuard<bool> = ValkeyGILGuard::default();
static ref CONFIGURATION_ENUM: ValkeyGILGuard<EnumConfiguration> =
ValkeyGILGuard::new(EnumConfiguration::Val1);
static ref CONFIGURATION_REJECT_ENUM: ValkeyGILGuard<EnumConfiguration> =
ValkeyGILGuard::new(EnumConfiguration::Val1);
static ref CONFIGURATION_MUTEX_ENUM: Mutex<EnumConfiguration> =
Mutex::new(EnumConfiguration::Val1);
}
Expand All @@ -43,6 +50,53 @@ fn on_configuration_changed<G, T: ConfigurationValue<G>>(
*val += 1
}

// Custom on_set handlers to add validation and conditionally
// reject upon config change.
fn on_string_config_set<G, T: ConfigurationValue<ValkeyString>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v.to_string_lossy().contains("reject-value") {
return Err(ValkeyError::Str("Rejected from custom string validation."));
}
Ok(())
}
fn on_i64_config_set<G, T: ConfigurationValue<i64>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == 123 {
return Err(ValkeyError::Str("Rejected from custom i64 validation."));
}
Ok(())
}
fn on_bool_config_set<G, T: ConfigurationValue<bool>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == false {
return Err(ValkeyError::Str("Rejected from custom bool validation."));
}
Ok(())
}
fn on_enum_config_set<G, T: ConfigurationValue<EnumConfiguration>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == EnumConfiguration::Val2 {
return Err(ValkeyError::Str("Rejected from custom enum validation."));
}
Ok(())
}

fn num_changes(ctx: &Context, _: Vec<ValkeyString>) -> ValkeyResult {
let val = NUM_OF_CONFIGURATION_CHANGES.lock(ctx);
Ok(ValkeyValue::Integer(*val))
Expand All @@ -61,19 +115,23 @@ valkey_module! {
configurations: [
i64: [
["i64", &*CONFIGURATION_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject-i64", &*CONFIGURATION_REJECT_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_i64_config_set::<ValkeyString, ValkeyGILGuard<i64>>))],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit: could you stick to snake_case just to be consistent in all 4 examples

["atomic_i64", &*CONFIGURATION_ATOMIC_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
],
string: [
["valkey_string", &*CONFIGURATION_VALKEY_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject-valkey_string", &*CONFIGURATION_REJECT_VALKEY_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_string_config_set::<ValkeyString, ValkeyGILGuard<ValkeyString>>))],
["string", &*CONFIGURATION_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed::<String, _>))],
["mutex_string", &*CONFIGURATION_MUTEX_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed::<String, _>))],
],
bool: [
["atomic_bool", &*CONFIGURATION_ATOMIC_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["bool", &*CONFIGURATION_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject-bool", &*CONFIGURATION_REJECT_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_bool_config_set::<ValkeyString, ValkeyGILGuard<bool>>))],
],
enum: [
["enum", &*CONFIGURATION_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject-enum", &*CONFIGURATION_REJECT_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_enum_config_set::<ValkeyString, ValkeyGILGuard<EnumConfiguration>>))],
["enum_mutex", &*CONFIGURATION_MUTEX_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
],
module_args_as_configuration: true,
Expand Down
25 changes: 24 additions & 1 deletion src/configuration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,12 @@ impl ConfigurationValue<bool> for AtomicBool {

type OnUpdatedCallback<T> = Box<dyn Fn(&ConfigurationContext, &str, &'static T)>;

type OnSetCallback<T> = Box<dyn Fn(&ConfigurationContext, &str, &'static T) -> Result<(), ValkeyError>>;

struct ConfigrationPrivateData<G, T: ConfigurationValue<G> + 'static> {
variable: &'static T,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
phantom: PhantomData<G>,
}

Expand All @@ -191,12 +194,24 @@ impl<G, T: ConfigurationValue<G> + 'static> ConfigrationPrivateData<G, T> {
return raw::REDISMODULE_ERR as i32;
}
let c_str_name = unsafe { CStr::from_ptr(name) };
if let Some(v) = self.on_set.as_ref() {
let result = v(
&configuration_ctx,
c_str_name.to_str().unwrap(),
self.variable,
);
if let Err(e) = result {
let error_msg = ValkeyString::create(None, e.to_string().as_str());
unsafe { *err = error_msg.take() };
return raw::REDISMODULE_ERR as i32;
}
}
if let Some(v) = self.on_changed.as_ref() {
v(
&configuration_ctx,
c_str_name.to_str().unwrap(),
self.variable,
)
);
}
raw::REDISMODULE_OK as i32
}
Expand Down Expand Up @@ -233,11 +248,13 @@ pub fn register_i64_configuration<T: ConfigurationValue<i64>>(
max: i64,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<i64>,
};
unsafe {
Expand Down Expand Up @@ -304,12 +321,14 @@ pub fn register_string_configuration<T: ConfigurationValue<ValkeyString>>(
default: &str,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let default = CString::new(default).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<ValkeyString>,
};
unsafe {
Expand Down Expand Up @@ -359,11 +378,13 @@ pub fn register_bool_configuration<T: ConfigurationValue<bool>>(
default: bool,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<bool>,
};
unsafe {
Expand Down Expand Up @@ -427,6 +448,7 @@ pub fn register_enum_configuration<G: EnumConfigurationValue, T: ConfigurationVa
default: G,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let (names, vals) = default.get_options();
Expand All @@ -438,6 +460,7 @@ pub fn register_enum_configuration<G: EnumConfigurationValue, T: ConfigurationVa
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<G>,
};
unsafe {
Expand Down
44 changes: 36 additions & 8 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,28 +148,28 @@ macro_rules! valkey_module {
$i64_min:expr,
$i64_max:expr,
$i64_flags_options:expr,
$i64_on_changed:expr
$i64_on_changed:expr $(, $i64_on_set:expr)?
]),* $(,)*],)?
$(string:[$([
$string_configuration_name:expr,
$string_configuration_val:expr,
$string_default:expr,
$string_flags_options:expr,
$string_on_changed:expr
$string_on_changed:expr $(, $string_on_set:expr)?
]),* $(,)*],)?
$(bool:[$([
$bool_configuration_name:expr,
$bool_configuration_val:expr,
$bool_default:expr,
$bool_flags_options:expr,
$bool_on_changed:expr
$bool_on_changed:expr $(, $bool_on_set:expr)?
]),* $(,)*],)?
$(enum:[$([
$enum_configuration_name:expr,
$enum_configuration_val:expr,
$enum_default:expr,
$enum_flags_options:expr,
$enum_on_changed:expr
$enum_on_changed:expr $(, $enum_on_set:expr)?
]),* $(,)*],)?
$(module_args_as_configuration:$use_module_args:expr,)?
$(module_config_get:$module_config_get_command:expr,)?
Expand Down Expand Up @@ -290,7 +290,14 @@ macro_rules! valkey_module {
} else {
$i64_default
};
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed, $i64_on_set);
)?
if (use_fallback) {
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed, None);
}
)*
)?
$(
Expand All @@ -306,7 +313,14 @@ macro_rules! valkey_module {
} else {
$string_default
};
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed, $string_on_set);
)?
if (use_fallback) {
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed, None);
}
)*
)?
$(
Expand All @@ -322,7 +336,14 @@ macro_rules! valkey_module {
} else {
$bool_default
};
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed, $bool_on_set);
)?
if (use_fallback) {
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed, None);
}
)*
)?
$(
Expand All @@ -338,7 +359,14 @@ macro_rules! valkey_module {
} else {
$enum_default
};
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default, $enum_flags_options, $enum_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default.clone(), $enum_flags_options, $enum_on_changed, $enum_on_set);
)?
if (use_fallback) {
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default.clone(), $enum_flags_options, $enum_on_changed, None);
}
)*
)?
raw::RedisModule_LoadConfigs.unwrap()(ctx);
Expand Down
33 changes: 31 additions & 2 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ fn test_configuration() -> Result<()> {
let res: String = redis::cmd("config")
.arg(&["set", config, val])
.query(&mut con)
.with_context(|| "failed to run config set")?;
.map_err(|e| anyhow::anyhow!("Failed to run config set: {}", e))?;
assert_eq!(res, "OK");
Ok(())
};
Expand Down Expand Up @@ -520,11 +520,40 @@ fn test_configuration() -> Result<()> {
config_set("configuration.enum_mutex", "Val2")?;
assert_eq!(config_get("configuration.enum_mutex")?, "Val2");

// Validate that configs can be rejected
let value = config_set("configuration.reject-valkey_string", "reject-value");
assert!(value.unwrap_err().to_string().contains("Rejected from custom string validation"));
let value = config_set("configuration.reject-i64", "123");
assert!(value.unwrap_err().to_string().contains("Rejected from custom i64 validation"));
let value = config_set("configuration.reject-bool", "no");
assert!(value.unwrap_err().to_string().contains("Rejected from custom bool validation"));
let value = config_set("configuration.reject-enum", "Val2");
assert!(value.unwrap_err().to_string().contains("Rejected from custom enum validation"));

let mut con = get_valkey_connection(port).with_context(|| FAILED_TO_CONNECT_TO_SERVER)?;
let res: i64 = redis::cmd("configuration.num_changes")
.query(&mut con)
.with_context(|| "failed to run configuration.num_changes")?;
assert_eq!(res, 26); // the first configuration initialisation is counted as well, so we will get 22 changes.

// Validate that configs with logic to reject values can also succeed
assert_eq!(config_get("configuration.reject-valkey_string")?, "default");
config_set("configuration.reject-valkey_string", "valid-value")?;
assert_eq!(config_get("configuration.reject-valkey_string")?, "valid-value");
assert_eq!(config_get("configuration.reject-i64")?, "10");
config_set("configuration.reject-i64", "11")?;
assert_eq!(config_get("configuration.reject-i64")?, "11");
assert_eq!(config_get("configuration.reject-bool")?, "yes");
config_set("configuration.reject-bool", "yes")?;
assert_eq!(config_get("configuration.reject-bool")?, "yes");
assert_eq!(config_get("configuration.reject-enum")?, "Val1");
config_set("configuration.reject-enum", "Val1")?;
assert_eq!(config_get("configuration.reject-enum")?, "Val1");
let mut con = get_valkey_connection(port).with_context(|| FAILED_TO_CONNECT_TO_SERVER)?;
let res: i64 = redis::cmd("configuration.num_changes")
.query(&mut con)
.with_context(|| "failed to run configuration.num_changes")?;
assert_eq!(res, 18); // the first configuration initialisation is counted as well, so we will get 18 changes.
assert_eq!(res, 28);

Ok(())
}
Expand Down