Handle key-spec flags with modules by oranagra · Pull Request #10237 · redis/redis

oranagra · 2022-02-04T19:45:32Z

add COMMAND GETKEYSANDFLAGS sub-command
add RM_KeyAtPosWithFlags and GetCommandKeysWithFlags
RM_KeyAtPos and RM_CreateCommand set flags requiring full access for keys
RM_CreateCommand set VARIABLE_FLAGS
expose variable_flags flag in COMMAND INFO key-specs
getKeysFromCommandWithSpecs prefers key-specs over getkeys-api
add tests for all of these

- add COMMAND GETKEYSANDFLAGS sub-command - add RM_KeyAtPosWithFlags and GetCommandKeysWithFlags - RM_KeyAtPos and RM_CreateCommand use flags requiring full access - expose `variable_flags` flag in COMMAND INFO key-specs - getKeysFromCommandWithSpecs prefers key-specs over getkeys-api - add tests for all of these

src/module.c

src/server.c

src/db.c

src/module.c

tests/unit/moduleapi/keyspecs.tcl

enjoy-binbin

just looking around

src/module.c

src/server.c

src/commands/command-getkeysandflags.json

src/module.c

oranagra · 2022-02-07T05:55:58Z

@redis/core-team please approve the new module APIs, and command (COMMAND GETKEYSANDFLAGS)

zuiderkwast

It won't hurt to check the GETKEYS (or GETKEYSANDFLAGS) output for the commands in keyspecs.tcl. That even serves as examples to easier understand the keyspecs in each test case. (I wrote these earlier.)

     test "Module key specs: Two ranges" {
         ...
+        assert_equal [r command getkeys kspec.tworanges foo bar baz quux] {foo bar}
     }

     test "Module key specs: Keyword-only spec clears the legacy triple" {
         ...
+        assert_equal [r command getkeys kspec.keyword foo KEYS bar baz] {bar baz}
     }
 
     test "Module key specs: Complex specs, case 1" {
         ...
+        assert_equal [r command getkeys kspec.complex1 foo dummy KEYS 1 bar baz STORE quux] {foo quux bar}
     }
 
     test "Module key specs: Complex specs, case 2" {
         ...
+        assert_equal [r command getkeys kspec.complex2 foo bar 2 baz quux banana STORE dst dummy MOREKEYS hey ho] {dst foo bar baz quux hey ho}
     }

oranagra · 2022-02-07T09:07:09Z

i think that test-wise, that's kinda already covered by COMMAND GETKEYSANDFLAGS correctly reports module key-spec without flags and COMMAND GETKEYSANDFLAGS correctly reports module key-spec flags.
but it surely can't hurt and can make the example maybe clearer, so i'll add these (using plain GETKEYS)

madolson

All API changes seem reasonable to me.

src/commands/command-getkeysandflags.json

src/module.c

src/db.c

…lags

oranagra · 2022-02-08T08:00:40Z

approved in core-team meeting.

…ges (#1784) doc changes for redis/redis#10237 Co-authored-by: Itamar Haber <[email protected]>

hpatro · 2023-11-08T23:02:16Z

@oranagra I had a question about this decision

RM_KeyAtPos and RM_CreateCommand set flags requiring full access for keys

If a module command declares it's appropriate categories while creating a command, why do we still set the flag to be requiring full access? This decision makes the ACL key permissions to be ineffective on module commands.

oranagra · 2023-11-09T07:08:37Z

If a module command declares it's appropriate categories while creating a command

what do you mean? the write and readonly flags? (about command flags, not (ACL) categories)?
these should not be confused for key-spec access flags.
I don't think we should draw any ACL decision based on them, but they're also insufficient (ACL key-spec flags require more details such ACCESS and DELETE.
for a module to properly work with more restrictive ACLv2 selectors, it has to provide explicit key-spec flags.

hpatro · 2023-11-09T16:27:19Z

If a module command declares it's appropriate categories while creating a command

what do you mean? the write and readonly flags? (about command flags, not (ACL) categories)?

these should not be confused for key-spec access flags.

I don't think we should draw any ACL decision based on them, but they're also insufficient (ACL key-spec flags require more details such ACCESS and DELETE.

for a module to properly work with more restrictive ACLv2 selectors, it has to provide explicit key-spec flags.

Yes,Thanks 👍

oranagra requested review from guybe7, madolson and zuiderkwast February 4, 2022 19:45

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

fix test leak

ccd94b7

zuiderkwast reviewed Feb 5, 2022

View reviewed changes

enjoy-binbin reviewed Feb 5, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

oranagra commented Feb 5, 2022

View reviewed changes

src/commands/command-getkeysandflags.json Show resolved Hide resolved

guybe7 reviewed Feb 5, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

guybe7 reviewed Feb 5, 2022

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

zuiderkwast closed this Feb 5, 2022

enjoy-binbin reopened this Feb 6, 2022

oranagra added 2 commits February 6, 2022 14:58

CR comments

0ccbfa3

fix

74a839f

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 7, 2022

zuiderkwast approved these changes Feb 7, 2022

View reviewed changes

improve tests

c1c8b7e

zuiderkwast approved these changes Feb 7, 2022

View reviewed changes

madolson approved these changes Feb 7, 2022

View reviewed changes

src/commands/command-getkeysandflags.json Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

oranagra added 2 commits February 8, 2022 09:34

Merge remote-tracking branch 'origin/unstable' into modules_keyspec_f…

323a313

…lags

CR comments

7c78a46

oranagra mentioned this pull request Feb 8, 2022

GETKEYSANDFLAGS sub-command, new module apis, and key-spec flags changes redis/redis-doc#1784

Merged

oranagra merged commit 66be30f into redis:unstable Feb 8, 2022

oranagra deleted the modules_keyspec_flags branch February 8, 2022 08:01

oranagra added a commit to redis/redis-doc that referenced this pull request Feb 8, 2022

GETKEYSANDFLAGS sub-command, new module apis, and key-spec flags chan…

f5ca307

…ges (#1784) doc changes for redis/redis#10237 Co-authored-by: Itamar Haber <[email protected]>

oranagra mentioned this pull request Feb 28, 2022

Release 7.0 RC2 #10355

Merged

Conversation

oranagra commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

madolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 8, 2022

Uh oh!

hpatro commented Nov 8, 2023

Uh oh!

oranagra commented Nov 9, 2023

Uh oh!

hpatro commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

oranagra commented Feb 4, 2022 •

edited

Loading

madolson left a comment •

edited

Loading