Add ALL_DBS flag to CLUSTER FLUSHSLOT for database-level ACL#3806
Merged
Conversation
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates over all databases and removes the slot's keys from every database, just like FLUSHALL command will touch all the databases. And it was missing the ALL_DBS command flag introduced in valkey-io#2309. Add test to cover this case, also do a extra cleanup, replace the manual start_multiple_servers with a single start_cluster call. Signed-off-by: Binbin <binloveplay1314@qq.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR adds ALL_DBS permission requirement to the CLUSTER FLUSHSLOT command. Command metadata is updated in both the compiled command table and JSON definition. Function prototypes are refreshed in the header. ACL tests verify the command correctly enforces all-database permissions. ChangesCLUSTER FLUSHSLOT ALL_DBS permission
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Member
Author
|
CLUSTER FLUSHSLOT may have originally been intended as an internal command, but there seems to not harm in doing it. |
dvkashapov
approved these changes
May 22, 2026
dvkashapov
left a comment
Member
There was a problem hiding this comment.
LGTM, you're right about no harm in this change.
Just for reference I deleted flag here for it because it is internal command #2309 (comment)
zuiderkwast
approved these changes
May 22, 2026
zuiderkwast
left a comment
Contributor
There was a problem hiding this comment.
JFYI I deleted ALL_DBS permissions from CLUSTER FLUSHSLOT because I thought it was available to user, but it is internal so no need for that.
I guess I missed that, or at least I didn't think about it properly.
Even if it's internal (undocumented), clients can access it and they should not be able to escape the ACL rules. Internal just means that we don't promise backward compatibility and such things, so users should not rely on it.
valkeyrie-ops Bot
pushed a commit
that referenced
this pull request
Jun 1, 2026
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates over all databases and removes the slot's keys from every database, just like FLUSHALL command will touch all the databases. And it was missing the ALL_DBS command flag introduced in #2309. Add test to cover this case, also do a extra cleanup, replace the manual start_multiple_servers with a single start_cluster call. Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot
pushed a commit
that referenced
this pull request
Jun 10, 2026
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates over all databases and removes the slot's keys from every database, just like FLUSHALL command will touch all the databases. And it was missing the ALL_DBS command flag introduced in #2309. Add test to cover this case, also do a extra cleanup, replace the manual start_multiple_servers with a single start_cluster call. Signed-off-by: Binbin <binloveplay1314@qq.com>
ranshid
added a commit
that referenced
this pull request
Jun 11, 2026
# Backport sweep for 9.1 Automated cherry-picks from PRs marked "To be backported". ## Applied | Source PR | Title | Detail | |---|---|---| | #3743 | Fix buffered_reply assert in HFE commands with module keyspace notifications | cherry-picked in a prior sweep | | #3766 | Fix flaky block_keyspace_notification test for HGETDEL notify race | cherry-picked in a prior sweep | | #3800 | Fix heap-use-after-free in ACL LOAD when client free is deferred | cherry-picked in a prior sweep | | #3723 | Fix double-finish and RESP reply violation in cluster slot migration | cherry-picked in a prior sweep | | #3872 | Redacting customer information when hide_user_data_from_log is true in rdb.c, networking.c, debug.c and t_hash | cherry-picked in a prior sweep | | #3846 | Fix use-after-free in VM_RegisterClusterMessageReceiver | cherry-picked in a prior sweep | | #3806 | Add ALL_DBS flag to CLUSTER FLUSHSLOT for database-level ACL | cherry-picked in a prior sweep | | #3847 | Harden SENTINEL commands and config rewrite against control-character injection | | | #3801 | Validate every DB clause in COPY against ACL db= permissions | | | #3851 | Replace AUTOMATION_PAT with Valkeyrie Bot GitHub App token | | | #3848 | Fix cluster AUX-field control-character and delimiter injection | | | #3544 | Revert "IO-Threads redesign cleanup work (#3544)" | cherry-picked in a prior sweep | | #3888 | Report exact dbid for COPY in ACL LOG when db= access is denied | conflicts resolved by Claude Code | | #3942 | Fix shard_id format specifier in UPDATE message log | | | #3941 | Avoid random() % 0 undefined behaviour when cluster-node-timeout < 30 | | --- *Generated by valkey-ci-agent using Claude Code.* --------- Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: chx9 <lovelypiska@outlook.com> Signed-off-by: zackcam <zackcam@amazon.com> Signed-off-by: Eran Ifrah <eifrah@amazon.com> Signed-off-by: Jules Lasarte <lasartej@amazon.com> Signed-off-by: jjuleslasarte <jules.lasarte@gmail.com> Signed-off-by: akash kumar <akumdev@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: lovelypiska <lovelypiska@outlook.com> Co-authored-by: zackcam <zackcam@amazon.com> Co-authored-by: eifrah-aws <eifrah@amazon.com> Co-authored-by: jjuleslasarte <jules.lasarte@gmail.com> Co-authored-by: Jules Lasarte <lasartej@amazon.com> Co-authored-by: Akash Kumar <45854686+akashkgit@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates
over all databases and removes the slot's keys from every database,
just like FLUSHALL command will touch all the databases. And it was
missing the ALL_DBS command flag introduced in #2309.
Add test to cover this case, also do a extra cleanup, replace the
manual start_multiple_servers with a single start_cluster call.