Treat subcommands as commands #9504

guybe7 · 2021-09-15T10:17:40Z

Intro

The purpose is to allow having different flags/ACL categories for
subcommands (Example: CONFIG GET is ok-loading but CONFIG SET isn't)

We create a small command table for every command that has subcommands
and each subcommand has its own flags, etc. (same as a "regular" command)

This commit also unites the Redis and the Sentinel command tables

Affected commands

CONFIG
Used to have "admin ok-loading ok-stale no-script"
Changes:

Dropped "ok-loading" in all except GET (this doesn't change behavior since there were checks in the code doing that)

XINFO
Used to have "read-only random"
Changes:

Dropped "random" in all except CONSUMERS

XGROUP
Used to have "write use-memory"
Changes:

Dropped "use-memory" in all except CREATE and CREATECONSUMER

COMMAND
No changes.

MEMORY
Used to have "random read-only"
Changes:

Dropped "random" in PURGE and USAGE

ACL
Used to have "admin no-script ok-loading ok-stale"
Changes:

Dropped "admin" in WHOAMI, GENPASS, and CAT

LATENCY
No changes.

MODULE
No changes.

SLOWLOG
Used to have "admin random ok-loading ok-stale"
Changes:

Dropped "random" in RESET

OBJECT
Used to have "read-only random"
Changes:

Dropped "random" in ENCODING and REFCOUNT

SCRIPT
Used to have "may-replicate no-script"
Changes:

Dropped "may-replicate" in all except FLUSH and LOAD

CLIENT
Used to have "admin no-script random ok-loading ok-stale"
Changes:

Dropped "random" in all except INFO and LIST
Dropped "admin" in ID, TRACKING, TRACKINGINFO, CACHING, GETREDIR, INFO, SETNAME, GETNAME, and REPLY

STRALGO
No changes.

PUBSUB
No changes.

CLUSTER
Changes:

Dropped "admin in countkeysinslots, getkeysinslot, info, nodes, keyslot, myid, and slots

SENTINEL
No changes.

(note that DEBUG also fits, but we decided not to convert it since it's for
debugging and anyway undocumented)

New sub-command

This commit adds another element to the per-command output of COMMAND,
describing the list of subcommands, if any (in the same structure as "regular" commands)
Also, it adds a new subcommand:

COMMAND LIST [FILTERBY (MODULE <module-name>|ACLCAT <cat>|PATTERN <pattern>)]

which returns a set of all commands (unless filters), but excluding subcommands.

Module API

A new module API, RM_CreateSubcommand, was added, in order to allow
module writer to define subcommands

ACL changes:

Now, that each subcommand is actually a command, each has its own ACL id.
The old mechanism of allowed_subcommands is redundant
(blocking/allowing a subcommand is the same as blocking/allowing a regular command),
but we had to keep it, to support the widespread usage of allowed_subcommands
to block commands with certain args, that aren't subcommands (e.g. "-select +select|0").
I have renamed allowed_subcommands to allowed_firstargs to emphasize the difference.
Because subcommands are commands in ACL too, you can now use "-" to block subcommands
(e.g. "+client -client|kill"), which wasn't possible in the past.
It is also possible to use the allowed_firstargs mechanism with subcommand.
For example: +config -config|set +config|set|loglevel will block all CONFIG SET except
for setting the log level.
NOTE: this capability was later removed in Improved handling of subcommands (don't allow ACL on first-arg of a sub-command) #10147
All of the ACL changes above required some amount of refactoring.

Misc

There are two approaches: Either each subcommand has its own function or all
subcommands use the same function, determining what to do according to argv[0].
For now, I took the former approaches only with CONFIG and COMMAND,
while other commands use the latter approach (for smaller blamelog diff).
Deleted memoryGetKeys: It is no longer needed because MEMORY USAGE now uses the "range" key spec.
Bugfix: GETNAME was missing from CLIENT's help message.
Sentinel and Redis now use the same table, with the same function pointer.
Some commands have a different implementation in Sentinel, so we redirect
them (these are ROLE, PUBLISH, and INFO).
Command stats now show the stats per subcommand (e.g. instead of stats just
for "config" you will have stats for "config|set", "config|get", etc.)
It is now possible to use COMMAND directly on subcommands:
COMMAND INFO CONFIG|GET (The pipeline syntax was inspired from ACL, and
can be used in functions lookupCommandBySds and lookupCommandByCString)
STRALGO is now a container command (has "help")

Breaking changes:

Command stats now show the stats per subcommand (see (5) above)

TODO

need to update the acl topic in redis.io (both the categories and the sub-commands) - with some note about redis 7 and up

The purpose is to allow having different flags/ACL categories for subcommands (Example: CONFIG GET is ok-loading but CONFIG SET isn't) We create a small command table for every command that has subcommands and each subcommand has its own flags, etc. (same as a "regular" command) This commit also unites the Redis and the Sentinel command tables This commit adds a new command COMMANDS in order to preserve backward-compatibility of COMMAND and also in the intention of deprecating it.

guybe7 · 2021-09-15T10:20:28Z

modules: maybe we want something like RM_CreateContainerCommand + RM_CreateSubcommand ?

guybe7 · 2021-09-15T10:24:16Z

@oranagra @madolson please go over all subcommands' flags: I've put some thought in some (CONFIG's subcommand) but in others, I just copied the flags from the parent command (CLUSTER)

oranagra

part one of the review.
reviewed everything except the command table itself.

src/config.c

src/db.c

src/module.c

src/networking.c

src/server.c

soloestoy · 2021-09-16T07:29:33Z

I didn't go deep into this PR, the subcommand idea is good, but a problem that came to my mind is about module, how to let module to register subcommands?

guybe7 · 2021-09-16T07:33:48Z

@soloestoy I guess we should introduce a new API RM_CreateSubcommand which is identical to RM_CreateCommand except that it gets const char* container_name (and maybe also drop the old [first,last,step] scheme, not sure)

guybe7 · 2021-09-16T15:24:09Z

TODO:

module API
COMMANDS LIST [FILTERBY (MODULE <module-name>|ACLCAT <cat>|GROUP <group>|PATTERN <pattern>)]
different ACL command-id per subcommand. pay special attention to backward-compatibility.

@oranagra please ack

madolson

Some comments

src/server.h

src/server.c

src/config.c

src/t_string.c

src/server.c

tests/unit/acl.tcl

src/acl.c

madolson

I think the ACL code is correct, but adding tests for various cases like -@all +config -config|get work would probably be good.

src/acl.c

yossigo

LGTM, only skimmed through the code - but concept and API make sense and look great.

src/acl.c

Introduced via typo in #9504. Also adds a sanity test for coverage.

overlooked in #9504

Improve code doc for allowed_firstargs (used to be allowed_commands before #9504. I don't think the text in the code needs to refer to the history (it's not there just for backwards compatibility). instead it should just describe what it does.

src/server.c

The following error commands will crash redis-server: ``` > get| Error: Server closed the connection > get|set Error: Server closed the connection > get|other ``` The reason is in redis#9504, we use `lookupCommandBySds` for find the container command. And it split the command (argv[0]) with `|`. If we input something like `get|other`, after the split, `get` will become a valid command name, pass the `ERR unknown command` check, and finally crash in `addReplySubcommandSyntaxError` In this case we do not need to split the command name with `|` and just look in the commands dict to find if `argv[0]` is a container command. So this commit introduce a new function call `isContainerCommandBySds` that it will return true if a command name is a container command. Also with the old code, there is a incorrect error message: ``` > config|get set (error) ERR Unknown subcommand or wrong number of arguments for 'set'. Try CONFIG|GET HELP. ``` The crash was reported in redis#10070.

The following error commands will crash redis-server: ``` > get| Error: Server closed the connection > get|set Error: Server closed the connection > get|other ``` The reason is in #9504, we use `lookupCommandBySds` for find the container command. And it split the command (argv[0]) with `|`. If we input something like `get|other`, after the split, `get` will become a valid command name, pass the `ERR unknown command` check, and finally crash in `addReplySubcommandSyntaxError` In this case we do not need to split the command name with `|` and just look in the commands dict to find if `argv[0]` is a container command. So this commit introduce a new function call `isContainerCommandBySds` that it will return true if a command name is a container command. Also with the old code, there is a incorrect error message: ``` > config|get set (error) ERR Unknown subcommand or wrong number of arguments for 'set'. Try CONFIG|GET HELP. ``` The crash was reported in #10070.

enjoy-binbin · 2022-01-14T02:47:57Z

src/server.c

+        parent->subcommands_dict = dictCreate(&commandTableDictType);
+
+    subcommand->parent = parent; /* Assign the parent command */
+    sds fullname = getFullCommandName(subcommand);


can we just set the subcommand->name = fullname? (or add a fullname field....)
it seems to me now that the subcommand name are generally unusable, we must get the fullname to use it.
so i suppose if we can just set the name = fullname, when we need it, we can just use it...

like here, it looks like the subcommand won't get the full name (unfamiliar with modules)

redis/src/module.c

Lines 10198 to 10204 in 56a8020

/* Return the name of the command currently running */

const char *RM_GetCurrentCommandName(RedisModuleCtx *ctx) {

if (!ctx || !ctx->client || !ctx->client->cmd)

return NULL;

return (const char*)ctx->client->cmd->name;

}

interesting..
in theory changing that (past 7.0) could be a breaking change.
but currently (before 7.0 is released), modules can't register sub-commands.

@enjoy-binbin did you conduct some search to see who's still using the simple cmd->name (on something that could be a sub-command) to find that one?
if that's the only one, i tend to agree we wanna make name the full name by default and drop all the complication of getFullCommandName

did you conduct some search to see who's still using the simple cmd->name (on something that could be a sub-command) to find that one?

actually no, i happen to see it (reviewing / studying some module codes.)

i also tend to use the full name by default, so that we don't have to deal with getFullCommandName.
i do some search, here are what i found:

latencyAllCommandsFillCDF, show sub-commands INFO latencystats and LATENCY HISTOGRAM #10103

afterErrorReply (we need to take care of it, it could be a subcommand i think):

const char *cmdname = c->lastcmd ? c->lastcmd->name : "<unknown>"; serverLog(LL_WARNING,"== CRITICAL == This %s is sending an error " "to its %s: '%.*s' after processing the command " "'%s'", from, to, (int)len, s, cmdname);

ACL CAT? maybe we need to also print the subcommand in future? (for now, it only works on parentcommand)

COMMAND LIST? same as above, only print the parent command, but others one, like command docs config|get, can work on subcommand

@guybe7 considering sub-commands are now first class citizens, i suppose we should list them in ACL CAT and COMMAND LIST, WDYT?

and IIRC from @enjoy-binbin findings, we might be better off just deleting getFullCommandName and put the full command name in cmd->name, which will also fix:

RM_GetCurrentCommandName module API

the CRITICAL log warning in afterErrorReply

replies in COMMAND LIST and ACL CAT once we change them to include sub-commands.

@enjoy-binbin Guy didn't respond yet, but i'm paranoid about forgetting this (discussion in a closed PR), and i think we must do that in 7.0 for completeness. wanna make a PR and we can continue the discussion there if needed?

@oranagra @enjoy-binbin I agree. we should always store the fullname, and eliminate all uses getFullCommandName, except for the initial assignment of the name.
we should rename redisCommand.name to something like tmp_cmdname so that the compiler will make sure we didn't miss anything. after that, we can call it name again. another option is to change it to fullname permanently in order to cause conflicts in open PRs such as #10103

i 'd like go the fullname option.
at the same time we should also care about subcommands_dict, now the key is the subcommand short name.
we can treat name as the short name(or rename it in some function arg name), fullname as the full name

i think that the command name is always the full name (also in the ubcommand_dict)
to clarify, the word "rewrite" from CONFIG REWRITE should never appear anywhere by itself, it should always be saved as "config|rewrite"

suggestion: we need to make sure that all callers to commandAddSubcommand already pass the subcommand with the fullname

@enjoy-binbin maybe while you're working on it, you can also handle #10124 and see if we missed anything.

… stores fullname (#10127) Summary of changes: 1. Rename `redisCommand->name` to `redisCommand->declared_name`, it is a const char * for native commands and SDS for module commands. 2. Store the [sub]command fullname in `redisCommand->fullname` (sds). 3. List subcommands in `ACL CAT` 4. List subcommands in `COMMAND LIST` 5. `moduleUnregisterCommands` now will also free the module subcommands. 6. RM_GetCurrentCommandName returns full command name Other changes: 1. Add `addReplyErrorArity` and `addReplyErrorExpireTime` 2. Remove `getFullCommandName` function that now is useless. 3. Some cleanups about `fullname` since now it is SDS. 4. Delete `populateSingleCommand` function from server.h that is useless. 5. Added tests to cover this change. 6. Add some module unload tests and fix the leaks 7. Make error messages uniform, make sure they always contain the full command name and that it's quoted. 7. Fixes some typos see the history in #9504, fixes #10124 Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: guybe7 <guy.benoish@redislabs.com>

guybe7 requested review from madolson and oranagra September 15, 2021 10:24

oranagra reviewed Sep 15, 2021

View reviewed changes

guybe7 added 4 commits September 15, 2021 08:14

CR fixes

dbf500b

split COMMANDS

9389b76

COMMANDS INFO output is now a map

f285acd

drop container flag

34e53b2

guybe7 mentioned this pull request Sep 15, 2021

Redis as the SSOT of its commands #9359

Closed

oranagra mentioned this pull request Sep 16, 2021

add module list modulename to list commands in a module #3211

Closed

madolson reviewed Sep 17, 2021

View reviewed changes

src/server.h Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/config.c Show resolved Hide resolved

src/t_string.c Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository labels Sep 19, 2021

oranagra mentioned this pull request Sep 20, 2021

[NEW] Redis module configuration #9437

Closed

guybe7 added 2 commits September 21, 2021 01:01

support subcmds in acl + minor bugfixes + minor CR fixes

4a3325e

Merge remote-tracking branch 'origin/unstable' into promote_subcommands

dcc19f6

guybe7 force-pushed the promote_subcommands branch from 27b21f9 to dcc19f6 Compare September 21, 2021 08:06

guybe7 added 2 commits September 21, 2021 01:29

rename old allowed_subcommands

919f940

typo

b0c55d8

oranagra reviewed Sep 21, 2021

View reviewed changes

tests/unit/acl.tcl Show resolved Hide resolved

tests/unit/acl.tcl Show resolved Hide resolved

tests/unit/acl.tcl Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

guybe7 added 2 commits September 21, 2021 08:17

CR fixes

769f7b0

typo

5914939

madolson reviewed Sep 21, 2021

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Show resolved Hide resolved

yossigo approved these changes Oct 19, 2021

View reviewed changes

src/acl.c Show resolved Hide resolved

cr fixes

daa88cf

oranagra merged commit 43e736f into redis:unstable Oct 20, 2021

This was referenced Oct 20, 2021

Update COMMAND and ACL docs redis/redis-doc#1669

Closed

Fix sentinel commands, ACL dictIter leak #9661

Merged

itamarhaber mentioned this pull request Oct 23, 2021

Fixes CLUSTER COUNTKEYSINSLOT #9672

Merged

This was referenced Oct 24, 2021

Improve code doc of allowed_firstargs following #9504 #9674

Merged

Removes admin acl category from CLIENT TRACKINGINFO #9662

Merged

oranagra pushed a commit that referenced this pull request Oct 24, 2021

Fixes CLUSTER COUNTKEYSINSLOT (#9672)

48e4d77

Introduced via typo in #9504. Also adds a sanity test for coverage.

oranagra pushed a commit that referenced this pull request Oct 25, 2021

Removes admin acl category from CLIENT TRACKINGINFO (#9662)

00362f2

overlooked in #9504

oranagra mentioned this pull request Nov 2, 2021

[QUESTION]bitfield get cannot execute on slave node? #9724

Closed

enjoy-binbin reviewed Dec 6, 2021

View reviewed changes

src/server.c Show resolved Hide resolved

enjoy-binbin mentioned this pull request Dec 7, 2021

Fix SENTINEL subcommands's arity #9909

Merged

enjoy-binbin mentioned this pull request Jan 8, 2022

[CRASH] SIGSEGV in addReplySubcommandSyntaxError at networking.c:966 #10070

Closed

enjoy-binbin mentioned this pull request Jan 9, 2022

Fix crash when error [sub]command name contains | #10082

Merged

oranagra mentioned this pull request Jan 11, 2022

Move doc metadata from COMMAND to COMMAND DOCS #10056

Merged

enjoy-binbin reviewed Jan 14, 2022

View reviewed changes

guybe7 mentioned this pull request Jan 17, 2022

Iterating all the commands #10124

Closed

enjoy-binbin mentioned this pull request Jan 17, 2022

sub-command support for ACL CAT and COMMAND LIST. redisCommand always stores fullname #10127

Merged

soloestoy mentioned this pull request Jan 18, 2022

Lookup subcommand more strictly and consistently #10132

Closed

oranagra mentioned this pull request Jul 7, 2022

Improvement of commands.json regarding return types and argument sets #10948

Open

madolson mentioned this pull request Oct 7, 2022

[BUG] Missing command stat metrics at parent command level #11363

Closed

hwware mentioned this pull request Apr 13, 2023

[BUG] COMMAND GETKEYS and COMMAND GETKEYSANDFLAG returns error for CLUSTER subcommand KEYSLOT #12041

Closed

XA21X mentioned this pull request May 25, 2023

Jedis 4.4.1 fails on CLIENT SETINFO for Redis 6.x if the user does not have all +CLIENT permissions redis/jedis#3438

Closed

	/* Return the name of the command currently running */
	const char RM_GetCurrentCommandName(RedisModuleCtx ctx) {
	if (!ctx \|\| !ctx->client \|\| !ctx->client->cmd)
	return NULL;

	return (const char*)ctx->client->cmd->name;
	}

Treat subcommands as commands #9504

Treat subcommands as commands #9504

Uh oh!

Conversation

guybe7 commented Sep 15, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intro

Affected commands

New sub-command

Module API

ACL changes:

Misc

Breaking changes:

TODO

Uh oh!

guybe7 commented Sep 15, 2021

Uh oh!

guybe7 commented Sep 15, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soloestoy commented Sep 16, 2021

Uh oh!

guybe7 commented Sep 16, 2021

Uh oh!

guybe7 commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

enjoy-binbin Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

oranagra Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybe7 commented Sep 15, 2021 •

edited by oranagra

Loading

guybe7 commented Sep 16, 2021 •

edited

Loading

oranagra Jan 14, 2022 •

edited

Loading

oranagra Jan 16, 2022 •

edited

Loading

guybe7 Jan 17, 2022 •

edited

Loading

guybe7 Jan 17, 2022 •

edited

Loading