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

Fix ACL regression with renamed commands #1651

Merged
merged 6 commits into from
Feb 3, 2025

Conversation

SoftlyRaining
Copy link
Contributor

@SoftlyRaining SoftlyRaining commented Jan 31, 2025

This regression occurred when we converted the command tables from dict to hashtable. For both server.commands and server.orig_commands pointed to the same struct and used the same field as a key. When a command is renamed it changed this key without updating server.orig_commands, causing it to be unable to find any renamed command.

server.orig_commands is used by ACLs which are always configured and manipulated using the original command names. For example, -config should always remove permission to use the config command, even if its been renamed to asdfconfig.

I added a tcl test which passes before the command table conversion, breaks after the conversion, and passes again with this fix.

Changes in acl.c are only for code cleanup and not necessary for the fix:

  • deleted some dead code
  • added static to methods (clarifies scope/use, aids compiler optimization)
  • couldn't resist changing 1 goto to break 🤷🏻‍♀️

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 98.46154% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (12ec3d5) to head (2016747).
Report is 5 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1651      +/-   ##
============================================
+ Coverage     70.98%   71.03%   +0.04%     
============================================
  Files           121      121              
  Lines         65176    65203      +27     
============================================
+ Hits          46264    46315      +51     
+ Misses        18912    18888      -24     
Files with missing lines Coverage Δ
src/acl.c 90.70% <100.00%> (+1.89%) ⬆️
src/config.c 78.40% <100.00%> (+0.01%) ⬆️
src/server.c 87.63% <100.00%> (+0.01%) ⬆️
src/server.h 100.00% <ø> (ø)
src/module.c 9.59% <0.00%> (-0.01%) ⬇️

... and 13 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Great, thanks for finding and fixing this before the release!

tests/unit/acl.tcl Outdated Show resolved Hide resolved
src/acl.c Show resolved Hide resolved
src/acl.c Show resolved Hide resolved
src/server.c Show resolved Hide resolved
src/config.c Outdated Show resolved Hide resolved
Signed-off-by: Rain Valentine <[email protected]>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

OK, LGTM! (I looked at the test cases a bit more this time and added two nits.)

tests/unit/acl.tcl Outdated Show resolved Hide resolved
tests/unit/acl.tcl Outdated Show resolved Hide resolved
Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast merged commit aced268 into valkey-io:unstable Feb 3, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants