Skip to content

Implemented module getchannels api and renamed channel keyspec#10299

Merged
oranagra merged 11 commits intoredis:unstablefrom
madolson:module-and-channels
Feb 22, 2022
Merged

Implemented module getchannels api and renamed channel keyspec#10299
oranagra merged 11 commits intoredis:unstablefrom
madolson:module-and-channels

Conversation

@madolson
Copy link
Contributor

@madolson madolson commented Feb 14, 2022

Resolves #10156.

This implements the following main pieces of functionality:

  • Renames key spec "CHANNEL" to be "NOT_KEY", and update the documentation to indicate it's for cluster routing and not for any other key related purpose.
  • Add the getchannels-api, so that modules can now define commands that are subject to ACL channel permission checks.
  • Add 4 new flags that describe how a module interacts with a command (SUBSCRIBE, PUBLISH, UNSUBSCRIBE, and PATTERN). They are all technically composable, however not sure how a command could both subscribe and unsubscribe from a command at once, but didn't see a reason to add explicit validation there.
  • Add two new module apis RM_ChannelAtPosWithFlags and RM_IsChannelsPositionRequest to duplicate the functionality provided by the keys position APIs.
  • The RM_ACLCheckChannelPermissions (only released in 7.0 RC1) was changed to take flags rather than a boolean literal.
  • The RM_ACLCheckKeyPermissions (only released in 7.0 RC1) was changed to take flags corresponding to keyspecs instead of custom permission flags. These keyspec flags mimic the flags for ACLCheckChannelPermissions.

NOTE: I considered renaming all of the keyReference/keyResult -> argReference/argResult, but decided against and just added a small comment.

To Do:

@madolson madolson added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus 7.0-must-have labels Feb 14, 2022
@oranagra
Copy link
Member

Note 2: There was a comment about changing the API for RM_ACLCheckChannelPermissions to take flags, but the module is the one doing the checking and knows what the arguments for the command represents, so not sure what the new flags would be for.

wasn't that needed so that ACL can decide if the request should be rejected or not?
i.e. we had some logic about always allowing unsubscribe (similar to the check we currently have in ACLDoesCommandHaveChannels).
The alternative is to expect modules to know not to call this API for unsubscribe (even if the command take channel arguments)
p.s. we already have a literal boolean, so that's overlapping with a PATTERN flag.

Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

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

LGTM.

@oranagra
Copy link
Member

@redis/core-team please approve (note that two APIs released in RC are changed)

@oranagra
Copy link
Member

approved in a core-team meeting.

@oranagra oranagra merged commit 71204f9 into redis:unstable Feb 22, 2022
oranagra pushed a commit to redis/redis-doc that referenced this pull request Feb 27, 2022
@oranagra oranagra mentioned this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Sort out the mess of modules, channels, keyspecs, and ACLs

4 participants