acl check api for functions and eval by yoav-steinberg · Pull Request #10220 · redis/redis

yoav-steinberg · 2022-01-31T15:43:01Z

See issue #10091.
This PR:

Adds the redis.acl_check_cmd() api to lua scripts. It can be used to check if the current user has permissions to execute a given command. The new function receives the command to check as an argument exactly like redis.call() receives the command to execute as an argument.
In the PR I unified the code used to convert lua arguments to redis argv arguments from both the new redis.acl_check_cmd() API and the redis.[p]call() API. This cleans up potential duplicate code.
While doing the refactoring in 2 I noticed there's an optimization to reduce allocation calls when parsing lua arguments into an argv array in the redis.[p]call() implementation. These optimizations were introduced years ago in 48c49c4 and 4f68655. It is unclear why this was added. The original commit message claims a 4% performance increase which I couldn't recreate and might not be worth it even if it did recreate. This PR removes that optimization. Following are details of the benchmark I did that couldn't reveal any performance improvements due to this optimization:

benchmark 1: src/redis-benchmark -P 500 -n 10000000 eval 'return redis.call("ping")' 0
benchmark 2: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__")' 0
benchmark 3: src/redis-benchmark -P 500 -r 1000 -n 100000 eval "for i=1,100,1 do redis.call('set','kk'..i,'vv'..__rand_int__) end return redis.call('get','kk5')" 0
benchmark 4: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")'

I ran the benchmark on this branch with and without commit 68b7168
Results in requests per second:

cmd	without optimization	without optimization 2nd run	with original optimization	with original optimization 2nd run
1	461233.34	477395.31	471098.16	469946.91
2	34774.14	35469.8	35149.38	34464.93
3	6390.59	6281.41	6146.28	6464.12
4	28005.71		27965.77

As you can see, different use cases showed identical or negligible performance differences. So finally I decided to chuck the original optimization and simplify the code.

- Misc cleanups. - Remove optimizations done in 48c49c4 and 4f68655: unclear why this was added. Commit message claims a 4% performance increase which I couldn't recreate and might not be worth it even if it did recreate. Updated code is easier to re-user and much simpler. Details of the benchmarks I did are in the PR (redis#10091).

yoav-steinberg · 2022-02-03T14:19:48Z

@MeirShpilraien I'd like you to look specifically and the old optimization code I removed in this PR and verify my logic makes sense. Please read the top comment.

oranagra

mainly reviewed the interfaces.
leaving the Lua binding code for Meir to review.

oranagra · 2022-02-03T19:29:14Z

@redis/core-team please approve

itamarhaber

LGTM

yossigo

LGTM (not full CR).

MeirShpilraien

👍

oranagra · 2022-02-07T06:00:31Z

@yoav-steinberg can you please make a redis-doc PR?

oranagra · 2022-02-07T06:00:59Z

ohh, you already did.. sorry.

oranagra · 2022-12-04T13:08:25Z

for the record, i tested this specific commit (on unstable at the time it was merged)
with both (taken from #11541)

memtier_benchmark --command="eval \"redis.call('hset', 'h1', 'k', 'v');redis.call('hset', 'h2', 'k', 'v');return redis.call('ping')\" 0"  - --hide-histogram --test-time 60 --pipeline 10 -x 1

and (taken from the top comment of this one)

src/redis-benchmark -P 500 -r 1000 -n 100000 eval "for i=1,100,1 do redis.call('set','kk'..i,'vv'..__rand_int__) end return redis.call('get','kk5')" 0

both showed around 5% performance degradation (same as what we see in #11541 when adding it back on top of recent unstable)

…7.0 (#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in #10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: sundb <sundbcn@gmail.com>

…7.0 (#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in #10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: sundb <sundbcn@gmail.com> (cherry picked from commit 2d80cd7)

…7.0 (redis#11541) This mechanism aims to reduce calls to malloc and free when preparing the arguments the script sends to redis commands. This is a mechanism was originally implemented in 48c49c4 and 4f68655, and was removed in redis#10220 (thinking it's not needed and that it has no impact), but it now turns out it was wrong, and it indeed provides some 5% performance improvement. The implementation is a little bit too simplistic, it assumes consecutive calls use the same size in the same arg index, but that's arguably sufficient since it's only aimed at caching very small things. We could even consider always pre-allocating args to the full LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument, that would increase the chance they'll be able to be re-used. But in some way this is already happening since we're using sdsalloc, which in turn uses s_malloc_usable and takes ownership of the full side of the allocation, so we are padded to the allocator bucket size. Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: sundb <sundbcn@gmail.com>

wip: acl check api for functions and eval

a72b794

yoav-steinberg changed the title ~~wip: acl check api for functions and eval~~ acl check api for functions and eval Jan 31, 2022

yoav-steinberg added 3 commits February 2, 2022 10:22

sane return values

e0a497d

scripting redis.acl_check_cmd_permissions() tests

b89a30a

yoav-steinberg marked this pull request as ready for review February 3, 2022 14:01

yoav-steinberg added the state:needs-doc-pr requires a PR to redis-doc repository label Feb 3, 2022

oranagra linked an issue Feb 3, 2022 that may be closed by this pull request

Add api for functions / eval for ACL check #10091

Closed

oranagra added the 7.0-must-have label Feb 3, 2022

oranagra reviewed Feb 3, 2022

View reviewed changes

Comment thread tests/unit/scripting.tcl Outdated

Comment thread tests/unit/scripting.tcl Outdated

Comment thread src/script_lua.c Outdated

Comment thread src/script_lua.c

Comment thread src/script_lua.c

Comment thread src/script_lua.c Outdated

oranagra requested a review from MeirShpilraien February 3, 2022 15:12

yoav-steinberg added 4 commits February 3, 2022 17:29

cr typo

2767f40

cr cleanup

4a07a5a

CR

ed8e473

cr cleanups

19ee939

oranagra approved these changes Feb 3, 2022

View reviewed changes

oranagra added 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 labels Feb 3, 2022

itamarhaber approved these changes Feb 4, 2022

View reviewed changes

yossigo approved these changes Feb 5, 2022

View reviewed changes

MeirShpilraien approved these changes Feb 5, 2022

View reviewed changes

yoav-steinberg mentioned this pull request Feb 5, 2022

Added redis.acl_check_cmd() documentation. redis/redis-doc#1777

Merged

oranagra merged commit 9dfeda5 into redis:unstable Feb 7, 2022

oranagra mentioned this pull request Feb 28, 2022

Release 7.0 RC2 #10355

Merged

sundb mentioned this pull request Nov 13, 2022

Performance degrade 7.0.3 vs 6.2.7 #10981

Closed

oranagra mentioned this pull request Dec 4, 2022

Reintroduce lua argument cache in luaRedisGenericCommand removed in v7.0 #11541

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

acl check api for functions and eval#10220

acl check api for functions and eval#10220
oranagra merged 8 commits into
redis:unstablefrom
yoav-steinberg:functions_eval_acl_check

yoav-steinberg commented Jan 31, 2022 •

edited by oranagra

Loading

Uh oh!

yoav-steinberg commented Feb 3, 2022

Uh oh!

oranagra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 3, 2022

Uh oh!

itamarhaber left a comment

Uh oh!

yossigo left a comment

Uh oh!

MeirShpilraien left a comment

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

oranagra commented Dec 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

yoav-steinberg commented Jan 31, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoav-steinberg commented Feb 3, 2022

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!

oranagra commented Feb 3, 2022

Uh oh!

itamarhaber left a comment

Choose a reason for hiding this comment

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

MeirShpilraien left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

oranagra commented Dec 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yoav-steinberg commented Jan 31, 2022 •

edited by oranagra

Loading