@MeirShpilraien FYI

I believe we decided we're okay merging this. Going to pick this so I can merge it into my redaction PR.

@madolson @nmvk i now realize we removed the read-only flag (see this #8820 (comment))
so doesn't that beat the purpose for which this command was created? how do we plan clients to know they can forward it to replicas?

@oranagra The check for whether or not a command can get executed on a replica is if it has the 'write' flag is present, not if the 'read-only' flag is present. Clients can also know that a command "accesses" keys by checking keyspecs I suppose, and seeing that the command accesses at least one key.

The same question also sort of exists for EVAL, which isn't marked as a write command.

right, both EVAL and EVAL_RO don't have either the read-only or write flags, and whether or not they access keys is beside the point.
i was under the impression that some clients look at the read-only flag, but now that i see it was dropped, and the two commands have the same flag, i wonder what's the point of this commend?
unless some clients will implement hard coded logic to recognize it by its name, but in that case, they can also simply implement a wrapper function like redis.eval_ro that is routed differently but is executing the same EVAL command.

The important point is not whether they access keys, it's how they access them. EVAL_RO has keys marked as RO and EVAL has keys marked as RW.

If we want to use read-only as the authoritative flag to indicate a command to be sent on the replica, that seems reasonable but we probably want to refactor the tracking code as was originally called out.

ok, so you're saying that the difference is in the key-spec (which is a new thing that's not widely used yet).
and the grantee that the command cannot and will not do any writes.

i was under the impression that there are some clients that already use the read-only flag for command routing, and if that's the case, i think we need to do the extra effort to add that flag and fix the problems with tracking.

if that's not the case, maybe it's just that the value of this new command is lower than i thought.

Some clients:

1. Redis-py is derpy, and has a hard coded list of "read" commands, https://github.com/redis/redis-py/blob/883fca7199a7ab7adc583b9ee324a9e44d0909eb/redis/cluster.py#L155.
2. Jedis apparently doesn't support routing to replicas at all.
3. redis-rb got close, and wrote a helper function called "should_send_to_slave?", that checks the read-only flag, but it's never used and only "should_send_to_master?" is ever used, which checks if it's a write command. (So all scripts are always sent to slaves when possible, seems like a bug)
4. go-redis seems to do it the way you described.

So, not very well adopted, but I agree it seems like the right approach. @oranagra Any thoughts about whether or not we should include EVAL_RO inside the ACL read-only category? I was thinking probably not, given how often lua vulnerabilities exist.

i don't see the relation between ACL read-only and Lua vulnerabilities.
if we consider Lua to be dangerous we can put it in the dangerous category, or disabled by default like the MODULE and DEBUG command. 8-)

The ACL readonly category is the collection of all commands that have the readonly flag, which would include the lua script. I was suggesting that lua should still be excluded from this category even if we add the flag. I don't want it to be implicitly added, I feel like you need to 'opt-in' to allowing scripts.

if you allow a certain user a read-only access, what you care about is that they won't modify the dataset, and EVAL_RO is a valid command, since it doesn't allow modifying the dataset (unless we have bugs, but that's not what we design features by).

LCS is also dangerous (we had some overflows in it i the past), should we set it as non-read-only too?
i don't think these are related. if we have some command that's potentially dangerous and needs to be selectively enabled, that's what the dangerous is for.

@yossigo please comment.

anyway, putting the ACL argument aside for a moment, we do agree that considering the purpose of this command, and at least the implementation of some (one) client libraries, it should have the read-only flag in COMMAND command output.
so can you see what needs to be done for client side caching and either make a PR or post some instructions for someone else (i didn't dig much in that area)

Yeah, I do agree about marking the flag that way.

if you allow a certain user a read-only access, what you care about is that they won't modify the dataset, and EVAL_RO is a valid command, since it doesn't allow modifying the dataset (unless we have bugs, but that's not what we design features by).

A counter argument to this is that you have to explicitly allow @scripting for EVAL. If yossi has a comment here, please do, otherwise let me just go implement the PR and we can move the discussion there.

EDIT: Scripting commands aren't marked as dangerous? That seems shocking to me.

@madolson I agree, read-only makes sense. Why do you think scripts are dangerous? They are not destructive by definition (unlike FLUSHALL).

@madolson i understand that you're concerned that if someone makes an ACL that blocks all commands and grants access only to +readonly category, then you don't want it to include any Lua capability.
this is clear in the test at #10728.
and i suppose you don't care about the other case of someone allowing all commands and removing the -readonly category (seems like a useless approach).

on the other hand, i'm not sure if the first one is that useful, arguably if a user wants to only allow reads, a better approach is the block -write.

@yossigo i'm not entirely sure what you meant by I agree, read-only makes sense. can you be more explicit.

@oranagra That doesn't really work well with ACLs, since +@all -@write is usually not what most people want when they want to create a "read" user. (They need to do +@all -@write -@admin -@scripting) If we had a "keys" command category (keyspace is only the non-type specific commands), something like +@keys -@write might make sense. -@all +@read is usually easier to reason about, although less "technically correct".

+@ALL is also considered less optimal than -@ALL since it allows ALL future module commands, if any exist, which could be read or write. <- this is also something we may want to fix

we discussed this in a core-team meeting and decided to go with it, i.e. these commands will have the read-only command flag, but will be excluded from the @read ACL category (there's nowhere that documents they're related to each other).

Also, we realized that we kinda regret adding the EVAL_RO command (was done before shabang flags existed), and we now realize that can can probably split client libraries into 3 categories:

1. ones that let the user explicitly define how to route commands (e.g. also, strongly consistent reads vs weak ones), in which case the client library can expose an .eval() function with some arguments or other way to influence it, and it doesn't need to rely on a special server side command and its flags.
2. client libraries that implicitly do routing of commands based their flags
3. client libraries that completely luck support for such smart routing features (or are more low-level)

arguably option [2] is insufficient anyway, and we don't need to promote it.