Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

By default prevent cross slot operations in functions and scripts with # #10615

Merged
merged 6 commits into from
Apr 26, 2022

Conversation

madolson
Copy link
Contributor

@madolson madolson commented Apr 21, 2022

Adds the allow-cross-slot-keys flag to Eval scripts and Functions to allow scripts to access keys from multiple slots.
The default behavior is now that they are not allowed to do that (unlike before).
This is a breaking change for 7.0 release candidates (to be part of 7.0.0), but not for previous redis releases since EVAL without shebang isn't doing this check.

Note that the check is done on both the keys declared by the EVAL / FCALL command arguments, and also the ones used by the script when making a redis.call.

A note about the implementation, there seems to have been some confusion about allowing access to non local keys. I thought I missed something in our wider conversation, but Redis scripts do block access to non-local keys. So the issue was just about cross slots being accessed.

to do:

Resolves, #10503.

@madolson madolson changed the title By default prevent cross slot operations in functions By default prevent cross slot operations in functions and scripts with # Apr 21, 2022
@madolson madolson added state:major-decision Requires core team consensus release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged labels Apr 23, 2022
src/server.c Show resolved Hide resolved
tests/test_helper.tcl Outdated Show resolved Hide resolved
tests/unit/scripting.tcl Outdated Show resolved Hide resolved
tests/unit/scripting.tcl Outdated Show resolved Hide resolved
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to the lack of time, i'm approving this ahead of time, so you can merge if if you think it's ready.
i suppose out of all the open discussions, the only ones that need a change is the relocation some test code to different files.

@oranagra oranagra added the state:needs-doc-pr requires a PR to redis-doc repository label Apr 25, 2022
@madolson
Copy link
Contributor Author

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved in core-team meeting.

@oranagra oranagra merged commit efcd1bf into redis:unstable Apr 26, 2022
oranagra added a commit to redis/redis-doc that referenced this pull request Apr 27, 2022
Adds documentation for a new lua flag, allow-cross-slot-keys which is implemented here: redis/redis#10615

Co-authored-by: Oran Agra <oran@redislabs.com>
@oranagra oranagra mentioned this pull request Apr 27, 2022
@zuiderkwast
Copy link
Contributor

Out of curiosity, who uses cross-slot keys in scripts?

If keys are in different shards, the script doesn't work anyway.

Is it only in single-shard clusters (does anyone use that?) or do some users make sure that certain slots reside on the same shard?

@madolson
Copy link
Contributor Author

madolson commented Jul 3, 2023

@zuiderkwast The only "legitimate" cases I am aware of are people who will do a bunch of incremental scan work in a script. Since we don't really expose a cursor local to a slot, you might need to do cross slot operations since the scan will return multiple items from the same bucket (so to be atomic you might want to process them together). The script would then return the cursor back, so the caller could send it again. Since they are never doing real cross slot commands, since each individual command will only ever operate on same slot keys, and the data is definitely in the given node, since they were returned from scan, it's pretty safe. There might be others, but since I know of at least one I didn't feel comfortable introducing a backwards breaking change.

I'm sure there are plenty of other esoteric use cases where people know what they are doing (carefully constructing the slot space so data is together) so that it works. I don't think this is too common, and even if it was we would want to disallow that. I think we probably want to have some flag like "allowed-script-flags" or attach it to the ACLs to help enable better control there though.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Jul 3, 2023

Thanks! With CSCAN (or SLOTSCAN) we can obsolete this use case. 👍

[Edit] Maybe not completely. It might still be better to scan multiple slots in one script to avoid too many round trips.

enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…h # (redis#10615)

Adds the `allow-cross-slot-keys` flag to Eval scripts and Functions to allow
scripts to access keys from multiple slots.
The default behavior is now that they are not allowed to do that (unlike before).
This is a breaking change for 7.0 release candidates (to be part of 7.0.0), but
not for previous redis releases since EVAL without shebang isn't doing this check.

Note that the check is done on both the keys declared by the EVAL / FCALL command
arguments, and also the ones used by the script when making a `redis.call`.

A note about the implementation, there seems to have been some confusion
about allowing access to non local keys. I thought I missed something in our
wider conversation, but Redis scripts do block access to non-local keys.
So the issue was just about cross slots being accessed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval-needed Waiting for core team approval to be merged 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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants