Skip to content

Handle key-spec flags with modules#10237

Merged
oranagra merged 7 commits into
redis:unstablefrom
oranagra:modules_keyspec_flags
Feb 8, 2022
Merged

Handle key-spec flags with modules#10237
oranagra merged 7 commits into
redis:unstablefrom
oranagra:modules_keyspec_flags

Conversation

@oranagra

@oranagra oranagra commented Feb 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Member
  • 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

fix #10189, fix #10144

@oranagra
Handle key-spec flags with modules
7704d26 
- 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
zuiderkwast
zuiderkwast reviewed Feb 5, 2022
View reviewed changes
Comment thread src/module.c Outdated
Comment thread src/module.c Outdated
Comment thread src/server.c Outdated
Comment thread src/db.c
Comment thread src/db.c
Comment thread src/module.c Outdated
Comment thread tests/unit/moduleapi/keyspecs.tcl Outdated
enjoy-binbin
enjoy-binbin reviewed Feb 5, 2022
View reviewed changes

@enjoy-binbin enjoy-binbin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just looking around

Comment thread src/module.c Outdated
Comment thread src/server.c Outdated
oranagra
oranagra commented Feb 5, 2022
View reviewed changes
Comment thread src/commands/command-getkeysandflags.json
guybe7
guybe7 reviewed Feb 5, 2022
View reviewed changes
Comment thread src/module.c Outdated
guybe7
guybe7 reviewed Feb 5, 2022
View reviewed changes
Comment thread src/module.c Outdated
@zuiderkwast zuiderkwast closed this Feb 5, 2022
@enjoy-binbin enjoy-binbin reopened this Feb 6, 2022
@oranagra

oranagra commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

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

@oranagra 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
zuiderkwast approved these changes Feb 7, 2022
View reviewed changes

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

oranagra commented Feb 7, 2022

Copy link
Copy Markdown
Member Author

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
madolson approved these changes Feb 7, 2022
View reviewed changes

@madolson madolson left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All API changes seem reasonable to me.

Comment thread src/commands/command-getkeysandflags.json
Comment thread src/module.c Outdated
Comment thread src/db.c Outdated
@oranagra oranagra mentioned this pull request Feb 8, 2022
Merged
@oranagra

oranagra commented Feb 8, 2022

Copy link
Copy Markdown
Member Author

approved in core-team meeting.

@oranagra oranagra merged commit 66be30f into redis:unstable Feb 8, 2022
@oranagra 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
@oranagra @itamarhaber
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 <itamar@redis.com>
@oranagra oranagra mentioned this pull request Feb 28, 2022
Merged
@hpatro

hpatro commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

@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

oranagra commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

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

hpatro commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yossigo yossigo yossigo left review comments
@guybe7 guybe7 guybe7 left review comments
+3 more reviewers
@enjoy-binbin enjoy-binbin enjoy-binbin left review comments
@zuiderkwast zuiderkwast zuiderkwast approved these changes
@madolson madolson madolson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Projects

Redis 7.0
Archived in project

Milestone

No milestone

7 participants

@oranagra @hpatro @zuiderkwast @yossigo @guybe7 @enjoy-binbin @madolson