Support for creating and setting custom (Module based) ACL Categories using existing Module APIs

[KarthikSubbarao]

We have the following ACL Module APIs in the core:

ValkeyModule_AddACLCategory
ValkeyModule_SetCommandACLCategories

Adding support for these existing APIs as wrapper functions (or macros) in the valkeymodule-rs SDK will allow modules to create their own customer ACL category and set this category on the module commands.

Example C Module which uses the APIs above: https://github.com/valkey-io/valkey/blob/4593dc2f059661e1c4eb43bba025f68948344228/tests/modules/aclcheck.c#L305-L313

[zackcam]

Hi - I'd like to start working on this. Can this issue be assigned to me?

There are a few approaches to add this functionality to the SDK. I'll make a follow up comment detailing those ways once I've gone over them a bit more.

[zackcam]

Option 1

High Level: In the modules we will make the developer of that set acl categories explicitly by calling a function that does the AddAclCategory function once. Followed by the setCommandACLCategories for each command. Will create two functions in Context called add_acl_category and set_acl_category. The add_acl_category will take in a string (The acl category name) and call ValkeyModule_AddACLCategory. While the set_acl_category will take in a string of the command name and another string (The acl category name). The command name will then be used in a get command which is then used in the SetCommandACLCategories.

Example Usage:

fn register_commands(ctx: &Context) -> i32 {
    let commands = ["bf.add", "bf.exists"];
    ctx.add_acl_category(ctx, "bloom"); // New
    valkey_command! { ctx.ctx, "bf.add", bloom_add_command, "write fast deny-oom", 1, -1, 1};
    for command in commands {
        ctx.set_acl_category(command, "bloom");
        
    }
}

APIs / Interface:

pub fn add_acl_category(&self, acl_flags: &str) -> i32;

pub fn set_acl_category(&self, command: &str, acl_flags: &str) -> i32;

Pros:

• Has no breaking changes, as we dont change the macros or arguments in macros but instead create two new macros.
• Less review needed in valkeymodule-rs as less and simpler changes are needed.

Cons:

• Will require more work from devs in their own modules to add acl categories
• Seems much easier to make mistakes with setting acl categories as specifying what commands have what acl categories could get complex
• Multiple calls might need to be made for multiple acl categories unless we use lists

Option 2

High Level: Update CommandInfo Struct to include acl_categories as a component, will cause a breaking change for modules as there will now be an incorrect number of args. Will also update the valkey_module macro to include a new field called acl categories where all the custom acl categories can be added, while also adding the field to the commands component of the macro where you can actually decide which commands get the acl categories or not.

Example Usage:

valkey_module! {
    name: MODULE_NAME,
    version: 1,
    allocator: (valkey_module::alloc::ValkeyAlloc, valkey_module::alloc::ValkeyAlloc),
    data_types: [
        BLOOM_FILTER_TYPE,
    ],
    init: initialize,
    deinit: deinitialize,
    acl_categories: [
        "bloom",
    ],   //Adds a new acl category
    commands: [
        ["BF.ADD", bloom_add_command, "write fast deny-oom", "bloom", 1, 1, 1],   //Example of adding "bloom" as an acl category
        ["BF.MADD", bloom_madd_command, "write fast deny-oom", "", 1, 1, 1],    // Example of no acl categories for a command
        ["BF.INSERT", bloom_insert_command, "write fast deny-oom", "", 1, 1, 1],
    ],   
}

APIs / Interface:

#[macro_export]
macro_rules! valkey_module {
    (
        name: $module_name:expr,
        version: $module_version:expr,
        allocator: ($allocator_type:ty, $allocator_init:expr),
        data_types: [
            $($data_type:ident),* $(,)*
        ],
        $(init: $init_func:ident,)* $(,)*
        $(deinit: $deinit_func:ident,)* $(,)*
        $(info: $info_func:ident,)?
        // Below is new. It is an optional list so multiple custom acl categories can be defined by the Module.
        $(acl_categories: [$($acl_categories:ident),* $(,)*])?
        commands: [
            $([
                $name:expr,
                $command:expr,
                $flags:expr,
                $acl_categories:expr,    // New, allows ability to set acl categories for specific commands
                $firstkey:expr,
                $lastkey:expr,
                $keystep:expr
              ]),* $(,)*
        ] $(,)*

Pros:

• Much less work for the developers of modules consuming this as all they need to do is add the field of acl_categories and update commands
• Very easy to see and keep track of which command has what acl categories
• Easier to change if command has a certain acl category or not

Cons:

• Causes a breaking change so any modules already consuming this will stop working
  • Breaking change occurs due to changing the args for commands in the valkey_module macro to include an acl_categories field
• More changes to valkeymodule-rs which could lead to longer time for it to be merged

Overall for both:
Both options provide flexibility on ACL categories where they can be set for certain commands and not for other commands.

[dmitrypol]

So this can also be used to add commands to existing categories such as admin, right? And if developers do not want to take advantage of it they just need to add empty "" where category would be.
commands: [ ["BF.ADD", bloom_add_command, "write fast deny-oom", "admin", 1, 1, 1], ... ],

But this will only work on Valkey or Redis 7.2 or greater. Can you wrap it in feature = "min-redis-compatibility-version-7-2"?

[zackcam]

So this can also be used to add commands to existing categories such as admin
Yes both options would allow developers to add the existing categories to commands. Have tested with admin, write and fast. I can test others if desired.

And if developers do not want to take advantage of it they just need to add empty ""
Yes that's the plan if option two is chosen. For option one the developers wouldn't have to do anything at all if they dont want to use ACL categories.

For compatibility we will check the min compatibility of the APIs and use the correct one for this feature

Do you have a recommendation for either option 1 or 2?