Today RM_Call allows executing any commands you want, even commands that make no sense to call inside RM_Call like MULTI or BLPOP. Though It is true that modules are not like LUA and should be smarter not to do such things, sometimes you want to execute a command base on client input (for example if you create a module that provides a new scripting language like javascript or python). So if a module wants to know if a command can or can not be executed inside RM_Call, the module needs to perform command info, get the command flags, and to decide whether or not to allow it.
Though solvable, I suggest a better API that allows modules to get a command flags. The new API will expose a new RedisModule API function int RM_GetCommandFlag(RedisModuleString **argv, int argc). The API will return the command flags and the module will be able to check for the existence or unexistence of different flags to decide what to do (for example if the module does not want to allow write commands he can verify that the readonly flag is on). The suggested API gets the entire command and not just the command name because after a discussion with @oranagra I understood that in the future, subcommand might influence the command flag, so we might want to pass the entire command so Redis can be smarter about the returned value. In addition, notice that the flags are returned as int (and not as char) this contradicts the way the flags pass on RM_CreateCommand (today Redis get them as cha). I think return int is better so the module will not have to start parsing and comparing strings. I also think we should introduce a new CreateCommand API that allows sending the flags as int (and maybe eventually deprecates the existing RM_CreateCommand).
Of course, everything here is just a suggestion, will be happy to hear your thoughts and ideas.
@oranagra @yossigo @itamarhaber @guybe7
Comment From: itamarhaber
Thanks @MeirShpilraien for summarizing this so neatly. I agree with word for word.
I also think we should introduce a new CreateCommand API that allows sending the flags as int (and maybe eventually deprecates the existing RM_CreateCommand).
In the past Salvatore was against the int bits approach after someone (me?) complained about the strings' error proneness and/or lack of error returned from CreateCommand (I also do typos, I'm human). That being said, I can't recall the reason for the objection although it may had something to do with how the commands table is built.
Comment From: guybe7
Do the command flags provide sufficient information to determine whether or not it makes sense to use the command with RM_Call? What about commands that have a blocking variant like XREADGROUP?
Comment From: MeirShpilraien
@guybe7 you are right, the flags do not show it but you can get it from the command category:
127.0.0.1:6379> COMMAND INFO XREADGROUP
1) 1) "xreadgroup"
2) (integer) -7
3) 1) write
2) movablekeys
4) (integer) 1
5) (integer) 1
6) (integer) 1
7) 1) @write
2) @stream
3) @slow
4) @blocking
Do you think we should add a similar API for that? Also, I notice that modules can not specify the command category, only flags (should we somehow add this also?) @oranagra any inputs about this?
Comment From: itamarhaber
Also, I notice that modules can not specify the command category, only flags (should we somehow add this also?)
The following is an excerpt from the "Redis Chronicles":
Oran Agra (RedisLabs) 6:24 PM another example populateCommandTableParseFlags implicit ACL categories (like readl-only for read, and dangerous for admin, but commandFlagsFromString that handles module commands doesn't. in fact i'm not clear why modules can't declare ACL categories on their commands (at least for modules that are registered before users are connected, i.e. loaded by config file)
Salvatore Sanfilippo (Redis shepherd) 6:24 PM The idea is that module commands should never be added in pre-existing categories this was designed to be like that So a category, in each Redis version, is just a fixed set of commands, this is very important for security. For modules, you have to explicitly add commands manually. However ACLs right now don't implement part of the original plan that was: +%modulename to add all the commands of a given module
Oran Agra (RedisLabs) 6:25 PM so a module that uses the "FLASHDB" module API, and marks it's command as admin will not be added to the dangerous group?
Salvatore Sanfilippo (Redis shepherd) 6:25 PM Nope, but at the same time, it will not be added to the user capability unless "allcommands" was specified otherwise module commands always are out In general I've the feeling that modules are very vertical use cases, and who does the policy, can decide very well what to include about module.s 99.99% of Redis instances don't have modules For ACLs in general I've the feeling that we can start like that, and after user feedbacks, understand what is missing and proceed with ACLv2 Btw, if you send me a PR for +%modulename, I can surely merge it The idea was: There is no -%modulename, just +%modulename. 2. It does not generate an error if "modulename" is not loaded.
Oran Agra (RedisLabs) 6:29 PM but such a modules command will be part of the @admin category, right? just not the @dangerous category..
Salvatore Sanfilippo (Redis shepherd) 6:29 PM That's bad, it should not be part of anything related to ACL categories are you sure it works like that? Don't remember exactly
Oran Agra (RedisLabs) 6:30 PM let me check again
Salvatore Sanfilippo (Redis shepherd) 6:30 PM but I thought that I used completely different tlags Thanks, what I tried to do, is to "copy" the ACL flags so if a command is marked "readonly" I also copy the "ACL readonly" flag but they were not the same AFAIK, but possibly I messed up
Oran Agra (RedisLabs) 6:30 PM you're right, CMD_ADMIN != CMD_CATEGORY_ADMIN (edited)
Salvatore Sanfilippo (Redis shepherd) 6:31 PM Oh good, at least it's not bugged What we could do soon or later, is the ability for the admin to actually add certain module commands in certain categories, in redis.conf but should be explicit (edited)
Oran Agra (RedisLabs) 6:32 PM ok, let's drop modules and ACL categories.. maybe i'll open an issue to track it, but if you rather not deal with this for 6.0, i understand.