Skip to content

Commit a9b578e

Browse files
ACL Updates, BF.INSERT updates, other misc updates (#38)
Signed-off-by: Karthik Subbarao <[email protected]>
1 parent ab158bb commit a9b578e

15 files changed

+98
-76
lines changed

src/bloom/command_handler.rs

+10-9
Original file line numberDiff line numberDiff line change
@@ -441,15 +441,14 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
441441
/// Function that implements logic to handle the BF.INSERT command.
442442
pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyResult {
443443
let argc = input_args.len();
444-
// At the very least, we need: BF.INSERT <key> ITEMS <item>
445-
if argc < 4 {
444+
// At the very least, we need: BF.INSERT <key>
445+
if argc < 2 {
446446
return Err(ValkeyError::WrongArity);
447447
}
448448
let mut idx = 1;
449449
// Parse the filter name
450450
let filter_name = &input_args[idx];
451451
idx += 1;
452-
let replicated_cmd = ctx.get_flags().contains(ContextFlags::REPLICATED);
453452
let mut fp_rate = *configs::BLOOM_FP_RATE_F64
454453
.lock()
455454
.expect("Unable to get a lock on fp_rate static");
@@ -464,6 +463,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
464463
false => (Some(configs::FIXED_SEED), false),
465464
};
466465
let mut nocreate = false;
466+
let mut items_provided = false;
467467
while idx < argc {
468468
match input_args[idx].to_string_lossy().to_uppercase().as_str() {
469469
"ERROR" => {
@@ -481,7 +481,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
481481
}
482482
};
483483
}
484-
"TIGHTENING" if replicated_cmd => {
484+
"TIGHTENING" => {
485485
// Note: This argument is only supported on replicated commands since primary nodes replicate bloom objects
486486
// deterministically using every global bloom config/property.
487487
if idx >= (argc - 1) {
@@ -520,7 +520,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
520520
}
521521
};
522522
}
523-
"SEED" if replicated_cmd => {
523+
"SEED" => {
524524
// Note: This argument is only supported on replicated commands since primary nodes replicate bloom objects
525525
// deterministically using every global bloom config/property.
526526
if idx >= (argc - 1) {
@@ -555,6 +555,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
555555
}
556556
"ITEMS" => {
557557
idx += 1;
558+
items_provided = true;
558559
break;
559560
}
560561
_ => {
@@ -563,7 +564,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
563564
}
564565
idx += 1;
565566
}
566-
if idx == argc && !replicated_cmd {
567+
if idx == argc && items_provided {
567568
// We expect the ITEMS <item> [<item> ...] argument to be provided on the BF.INSERT command used on primary nodes.
568569
// For replicated commands, this is optional to allow BF.INSERT to be used to replicate bloom object creation
569570
// commands without any items (BF.RESERVE).
@@ -578,7 +579,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
578579
}
579580
};
580581
// Skip bloom filter size validation on replicated cmds.
581-
let validate_size_limit = !replicated_cmd;
582+
let validate_size_limit = !ctx.get_flags().contains(ContextFlags::REPLICATED);
582583
let mut add_succeeded = false;
583584
match value {
584585
Some(bloom) => {
@@ -589,7 +590,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
589590
bloom,
590591
true,
591592
&mut add_succeeded,
592-
!replicated_cmd,
593+
validate_size_limit,
593594
);
594595
let replicate_args = ReplicateArgs {
595596
capacity: bloom.capacity(),
@@ -632,7 +633,7 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
632633
&mut bloom,
633634
true,
634635
&mut add_succeeded,
635-
!replicated_cmd,
636+
validate_size_limit,
636637
);
637638
match filter_key.set_value(&BLOOM_TYPE, bloom) {
638639
Ok(()) => {

src/bloom/data_type.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,6 @@ impl ValkeyDataType for BloomObject {
8383
// Calculate the memory usage of the BloomFilter/s by summing up BloomFilter sizes as they are de-serialized.
8484
let mut filters_memory_usage = 0;
8585
for i in 0..num_filters {
86-
let Ok(bitmap) = raw::load_string_buffer(rdb) else {
87-
return None;
88-
};
8986
let Ok(capacity) = raw::load_unsigned(rdb) else {
9087
return None;
9188
};
@@ -119,6 +116,9 @@ impl ValkeyDataType for BloomObject {
119116
} else {
120117
capacity
121118
};
119+
let Ok(bitmap) = raw::load_string_buffer(rdb) else {
120+
return None;
121+
};
122122
let filter =
123123
BloomFilter::from_existing(bitmap.as_ref(), num_items as i64, capacity as i64);
124124
if !is_seed_random && filter.seed() != configs::FIXED_SEED {

src/bloom/utils.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -824,7 +824,7 @@ mod tests {
824824
add_items_till_capacity(
825825
&mut bf,
826826
initial_capacity + 1,
827-
1,
827+
add_operation_idx + 1,
828828
&rand_prefix,
829829
Some(BloomError::NonScalingFilterFull),
830830
);
@@ -852,10 +852,13 @@ mod tests {
852852
// Validate that the real fp_rate is not much more than the configured fp_rate.
853853
fp_assert(error_count, num_operations, expected_fp_rate, fp_margin);
854854
// Verify restore
855-
let mut restore_bf = BloomObject::create_copy_from(&bf);
856-
assert_eq!(
857-
restore_bf.add_item(b"new_item", true),
858-
Err(BloomError::NonScalingFilterFull)
855+
let restore_bf = BloomObject::create_copy_from(&bf);
856+
add_items_till_capacity(
857+
&mut bf,
858+
initial_capacity + 1,
859+
add_operation_idx + 1,
860+
&rand_prefix,
861+
Some(BloomError::NonScalingFilterFull),
859862
);
860863
verify_restored_items(
861864
&bf,

src/configs.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ pub fn on_string_config_set(
8686
};
8787
match name {
8888
"bloom-fp-rate" => {
89-
if !(BLOOM_FP_RATE_MIN..BLOOM_FP_RATE_MAX).contains(&value) {
89+
if !(value > BLOOM_FP_RATE_MIN && value < BLOOM_FP_RATE_MAX) {
9090
return Err(ValkeyError::Str(utils::ERROR_RATE_RANGE));
9191
}
9292
let mut fp_rate = BLOOM_FP_RATE_F64
@@ -96,7 +96,7 @@ pub fn on_string_config_set(
9696
Ok(())
9797
}
9898
"bloom-tightening-ratio" => {
99-
if !(BLOOM_TIGHTENING_RATIO_MIN..BLOOM_TIGHTENING_RATIO_MAX).contains(&value) {
99+
if !(value > BLOOM_TIGHTENING_RATIO_MIN && value < BLOOM_TIGHTENING_RATIO_MAX) {
100100
return Err(ValkeyError::Str(utils::TIGHTENING_RATIO_RANGE));
101101
}
102102
let mut tightening = BLOOM_TIGHTENING_F64

src/lib.rs

+9-9
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,15 @@ valkey_module! {
9191
"bloom",
9292
]
9393
commands: [
94-
["BF.ADD", bloom_add_command, "write fast deny-oom", 1, 1, 1, "bloom"],
95-
["BF.MADD", bloom_madd_command, "write fast deny-oom", 1, 1, 1, "bloom"],
96-
["BF.EXISTS", bloom_exists_command, "readonly fast", 1, 1, 1, "bloom"],
97-
["BF.MEXISTS", bloom_mexists_command, "readonly fast", 1, 1, 1, "bloom"],
98-
["BF.CARD", bloom_card_command, "readonly fast", 1, 1, 1, "bloom"],
99-
["BF.RESERVE", bloom_reserve_command, "write fast deny-oom", 1, 1, 1, "bloom"],
100-
["BF.INFO", bloom_info_command, "readonly fast", 1, 1, 1, "bloom"],
101-
["BF.INSERT", bloom_insert_command, "write fast deny-oom", 1, 1, 1, "bloom"],
102-
["BF.LOAD", bloom_load_command, "write fast deny-oom", 1, 1, 1, "bloom"]
94+
["BF.ADD", bloom_add_command, "write fast deny-oom", 1, 1, 1, "fast write bloom"],
95+
["BF.MADD", bloom_madd_command, "write fast deny-oom", 1, 1, 1, "fast write bloom"],
96+
["BF.EXISTS", bloom_exists_command, "readonly fast", 1, 1, 1, "fast read bloom"],
97+
["BF.MEXISTS", bloom_mexists_command, "readonly fast", 1, 1, 1, "fast read bloom"],
98+
["BF.CARD", bloom_card_command, "readonly fast", 1, 1, 1, "fast read bloom"],
99+
["BF.RESERVE", bloom_reserve_command, "write fast deny-oom", 1, 1, 1, "fast write bloom"],
100+
["BF.INFO", bloom_info_command, "readonly fast", 1, 1, 1, "fast read bloom"],
101+
["BF.INSERT", bloom_insert_command, "write fast deny-oom", 1, 1, 1, "fast write bloom"],
102+
["BF.LOAD", bloom_load_command, "write deny-oom", 1, 1, 1, "write bloom"]
103103
],
104104
configurations: [
105105
i64: [

src/wrapper/bloom_callback.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,17 @@ pub unsafe extern "C" fn bloom_rdb_save(rdb: *mut raw::RedisModuleIO, value: *mu
3636
let filter_list = v.filters();
3737
let mut filter_list_iter = filter_list.iter().peekable();
3838
while let Some(filter) = filter_list_iter.next() {
39+
raw::save_unsigned(rdb, filter.capacity() as u64);
40+
if filter_list_iter.peek().is_none() {
41+
raw::save_unsigned(rdb, filter.num_items() as u64);
42+
}
3943
let bloom = filter.raw_bloom();
4044
let bitmap = bloom.as_slice();
4145
raw::RedisModule_SaveStringBuffer.unwrap()(
4246
rdb,
4347
bitmap.as_ptr().cast::<c_char>(),
4448
bitmap.len(),
4549
);
46-
raw::save_unsigned(rdb, filter.capacity() as u64);
47-
if filter_list_iter.peek().is_none() {
48-
raw::save_unsigned(rdb, filter.num_items() as u64);
49-
}
5050
}
5151
}
5252

tests/test_bloom_acl_category.py

+53-39
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import pytest
22
from valkeytests.conftest import resource_port_tracker
33
from valkey_bloom_test_case import ValkeyBloomTestCaseBase
4+
from util.waiters import *
45

56
class TestBloomACLCategory(ValkeyBloomTestCaseBase):
67

7-
def test_bloom_acl_category(self):
8+
def test_bloom_acl_category_permissions(self):
89
# List of bloom commands and the expected returns if the command is valid
910
bloom_commands = [
1011
('BF.ADD add_key item', 1),
@@ -19,52 +20,65 @@ def test_bloom_acl_category(self):
1920
client = self.server.get_new_client()
2021
# Get a list of all commands with the acl category bloom
2122
list_of_bloom_commands = client.execute_command("COMMAND LIST FILTERBY ACLCAT bloom")
23+
# Create users with differnt acl permissions
24+
client.execute_command("ACL SETUSER nonbloomuser1 on >bloom_pass -@bloom")
25+
client.execute_command("ACL SETUSER nonbloomuser2 on >bloom_pass -@all")
26+
client.execute_command("ACL SETUSER bloomuser1 on >bloom_pass ~* &* +@all ")
27+
client.execute_command("ACL SETUSER bloomuser2 on >bloom_pass ~* &* -@all +@bloom ")
28+
client.execute_command("ACL SETUSER bloomuser3 on >bloom_pass ~* &* -@all +@write +@read ")
29+
client.execute_command("ACL SETUSER bloomuser4 on >bloom_pass ~* &* -@all +@write +@bloom")
30+
# Switch to the users with no bloom command access and check error occurs as expected
31+
for i in range(1, 3):
32+
client.execute_command(f"AUTH nonbloomuser{i} bloom_pass")
33+
for cmd in bloom_commands:
34+
self.verify_invalid_user_permissions(client, cmd, list_of_bloom_commands)
35+
# Switch to the users with bloom command access and check commands are run as expected
36+
for i in range(1, 5):
37+
client.execute_command(f"AUTH bloomuser{i} bloom_pass")
38+
for cmd in bloom_commands:
39+
self.verify_valid_user_permissions(client, cmd)
40+
self.client.execute_command('FLUSHDB')
41+
wait_for_equal(lambda: self.client.execute_command('DBSIZE'), 0)
2242

23-
# Create two users one with denied access to bloom commands and one with access to bloom commands and all keys
24-
client.execute_command("ACL SETUSER nonbloomuser on >bloom_pass -@bloom")
25-
client.execute_command("ACL SETUSER bloomuser on >bloom_pass +@bloom ~*")
43+
def verify_valid_user_permissions(self, client, cmd):
44+
cmd_name = cmd[0].split()[0]
45+
try:
46+
result = client.execute_command(cmd[0])
47+
if cmd[0].startswith("BF.M"):
48+
assert len(result) == cmd[1]
49+
# The first add in a new bloom object should always return 1. For MEXISTS the first item we check will have been added as well so should exist
50+
assert result[0] == 1
51+
else:
52+
assert result == cmd[1], f"{cmd_name} should work for default user"
53+
except Exception as e:
54+
assert False, f"bloomuser should be able to execute {cmd_name}: {str(e)}"
2655

27-
# Switch to the user with no bloom command access and check error occurs as expected
28-
client.execute_command("AUTH nonbloomuser bloom_pass")
29-
for cmd in bloom_commands:
30-
cmd_name = cmd[0].split()[0]
31-
# Check that each command we try to run appeared in the list of commands with the bloom acl category
32-
assert cmd_name.encode() in list_of_bloom_commands
33-
try:
34-
result = client.execute_command(cmd[0])
35-
assert False, f"User with no bloom category access shouldnt be able to run {cmd_name}"
36-
except Exception as e:
37-
assert str(e) == f"User nonbloomuser has no permissions to run the '{cmd_name}' command"
38-
39-
# Switch to the user with bloom command access and check commands are run as expected
40-
client.execute_command(f"AUTH bloomuser bloom_pass")
41-
for cmd in bloom_commands:
42-
cmd_name = cmd[0].split()[0]
43-
try:
44-
result = client.execute_command(cmd[0])
45-
if cmd[0].startswith("BF.M"):
46-
assert len(result) == cmd[1]
47-
# The first add in a new bloom object should always return 1. For MEXISTS the first item we check will have been added as well so should exist
48-
assert result[0] == 1
49-
else:
50-
assert result == cmd[1], f"{cmd_name} should work for default user"
51-
except Exception as e:
52-
assert False, f"bloomuser should be able to execute {cmd_name}: {str(e)}"
56+
def verify_invalid_user_permissions(self, client, cmd, list_of_bloom_commands):
57+
cmd_name = cmd[0].split()[0]
58+
# Check that each command we try to run appeared in the list of commands with the bloom acl category
59+
assert cmd_name.encode() in list_of_bloom_commands
60+
try:
61+
result = client.execute_command(cmd[0])
62+
assert False, f"User with no bloom category access shouldnt be able to run {cmd_name}"
63+
except Exception as e:
64+
assert f"has no permissions to run the '{cmd_name}' command" in str(e)
5365

5466
def test_bloom_command_acl_categories(self):
5567
# List of bloom commands and their acl categories
5668
bloom_commands = [
57-
('BF.ADD', [b'write' , b'denyoom', b'module', b'fast']),
58-
('BF.EXISTS', [b'readonly', b'module', b'fast']),
59-
('BF.MADD', [b'write', b'denyoom', b'module', b'fast']),
60-
('BF.MEXISTS', [b'readonly', b'module', b'fast']),
61-
('BF.INSERT', [b'write', b'denyoom', b'module', b'fast']),
62-
('BF.INFO', [b'readonly', b'module', b'fast']),
63-
('BF.CARD', [b'readonly', b'module', b'fast']),
64-
('BF.RESERVE', [b'write', b'denyoom', b'module', b'fast']),
69+
('BF.ADD', [b'write' , b'denyoom', b'module', b'fast'], [b'@write', b'@fast', b'@bloom']),
70+
('BF.EXISTS', [b'readonly', b'module', b'fast'], [b'@read', b'@fast', b'@bloom']),
71+
('BF.MADD', [b'write', b'denyoom', b'module', b'fast'], [b'@write', b'@fast', b'@bloom']),
72+
('BF.MEXISTS', [b'readonly', b'module', b'fast'], [b'@read', b'@fast', b'@bloom']),
73+
('BF.INSERT', [b'write', b'denyoom', b'module', b'fast'], [b'@write', b'@fast', b'@bloom']),
74+
('BF.INFO', [b'readonly', b'module', b'fast'], [b'@read', b'@fast', b'@bloom']),
75+
('BF.CARD', [b'readonly', b'module', b'fast'], [b'@read', b'@fast', b'@bloom']),
76+
('BF.RESERVE', [b'write', b'denyoom', b'module', b'fast'], [b'@write', b'@fast', b'@bloom']),
77+
('BF.LOAD', [b'write', b'denyoom', b'module'], [b'@write', b'@bloom']),
6578
]
6679
for cmd in bloom_commands:
6780
# Get the info of the commands and compare the acl categories
6881
cmd_info = self.client.execute_command(f'COMMAND INFO {cmd[0]}')
6982
assert cmd_info[0][2] == cmd[1]
70-
assert cmd_info[0][6] == [b'@bloom']
83+
for category in cmd[2]:
84+
assert category in cmd_info[0][6]
File renamed without changes.
File renamed without changes.

tests/test_bloom_command.py

+8-4
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,16 @@ def test_bloom_command_error(self):
3434
('bf.insert key CAPACITY 10000 ERROR 0.01 EXPANSION 0.99 NOCREATE NONSCALING ITEMS test1 test2 test3', 'bad expansion'),
3535
('BF.INSERT KEY HELLO WORLD', 'unknown argument received'),
3636
('BF.INSERT KEY error 2 ITEMS test1', '(0 < error rate range < 1)'),
37+
('BF.INSERT KEY ERROR err ITEMS test1', 'bad error rate'),
38+
('BF.INSERT KEY TIGHTENING tr ITEMS test1', 'bad tightening ratio'),
39+
('BF.INSERT KEY TIGHTENING 2 ITEMS test1', '(0 < tightening ratio range < 1)'),
3740
('BF.INSERT TEST_LIMIT ERROR 0.99999999999999999 ITEMS ERROR_RATE', '(0 < error rate range < 1)'),
41+
('BF.INSERT TEST_LIMIT TIGHTENING 0.99999999999999999 ITEMS ERROR_RATE', '(0 < tightening ratio range < 1)'),
3842
('BF.INSERT TEST_LIMIT CAPACITY 9223372036854775808 ITEMS CAP', 'bad capacity'),
3943
('BF.INSERT TEST_LIMIT CAPACITY 0 ITEMS CAP0', '(capacity should be larger than 0)'),
4044
('BF.INSERT TEST_LIMIT EXPANSION 4294967299 ITEMS EXPAN', 'bad expansion'),
4145
('BF.INSERT TEST_NOCREATE NOCREATE ITEMS A B', 'not found'),
46+
('BF.INSERT KEY HELLO', 'unknown argument received'),
4247
('BF.RESERVE KEY String 100', 'bad error rate'),
4348
('BF.RESERVE KEY 0.99999999999999999 3000', '(0 < error rate range < 1)'),
4449
('BF.RESERVE KEY 2 100', '(0 < error rate range < 1)'),
@@ -61,9 +66,6 @@ def test_bloom_command_error(self):
6166
('BF.INFO', 'wrong number of arguments for \'BF.INFO\' command'),
6267
('bf.info key capacity size', 'wrong number of arguments for \'BF.INFO\' command'),
6368
('BF.INSERT', 'wrong number of arguments for \'BF.INSERT\' command'),
64-
('BF.INSERT KEY', 'wrong number of arguments for \'BF.INSERT\' command'),
65-
('BF.INSERT KEY HELLO', 'wrong number of arguments for \'BF.INSERT\' command'),
66-
('BF.INSERT MISS_ITEM EXPANSION 2', 'wrong number of arguments for \'BF.INSERT\' command'),
6769
('BF.INSERT MISS_ITEM EXPANSION 2 ITEMS', 'wrong number of arguments for \'BF.INSERT\' command'),
6870
('BF.INSERT MISS_VAL ERROR 0.5 EXPANSION', 'wrong number of arguments for \'BF.INSERT\' command'),
6971
('BF.INSERT MISS_VAL ERROR 0.5 CAPACITY', 'wrong number of arguments for \'BF.INSERT\' command'),
@@ -107,6 +109,8 @@ def test_bloom_command_behavior(self):
107109
('BF.INSERT TEST_EXPANSION EXPANSION 9 ITEMS ITEM', 1),
108110
('BF.INSERT TEST_CAPACITY CAPACITY 2000 ITEMS ITEM', 1),
109111
('BF.INSERT TEST_ITEMS ITEMS 1 2 3 EXPANSION 2', 5),
112+
('BF.INSERT KEY', 0),
113+
('BF.INSERT KEY EXPANSION 2', 0),
110114
('BF.INFO TEST Capacity', 100),
111115
('BF.INFO TEST ITEMS', 5),
112116
('BF.INFO TEST filters', 1),
@@ -147,4 +151,4 @@ def test_bloom_command_behavior(self):
147151
assert bf_info[capacity_index] == self.client.execute_command('BF.INFO BF_INFO CAPACITY') == 2000
148152
assert bf_info[filter_index] == self.client.execute_command('BF.INFO BF_INFO FILTERS') == 1
149153
assert bf_info[item_index] == self.client.execute_command('BF.INFO BF_INFO ITEMS') == 0
150-
assert bf_info[expansion_index] == self.client.execute_command('BF.INFO BF_INFO EXPANSION') == None
154+
assert bf_info[expansion_index] == self.client.execute_command('BF.INFO BF_INFO EXPANSION') == None
File renamed without changes.

0 commit comments

Comments
 (0)