Skip to content

Support internal commands #56

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

Merged
merged 4 commits into from
Feb 5, 2025
Merged

Support internal commands #56

merged 4 commits into from
Feb 5, 2025

Conversation

MeirShpilraien
Copy link
Contributor

@MeirShpilraien MeirShpilraien commented Jan 22, 2025

The PR makes changes to use the internal command capabilities introduced on: redis/redis#13740.
The main idea is to use the new redismodule API that provides internal secret for internal shard communication.

This means that the following is no longer needed:

  1. The new user configuration.
  2. The internal ACL category

All the commands are marked with the internal flag (if we see that the internal commands feature is supported on the current Redis version we run on). In addition, the connection between the shards uses INTERNALAUTH with the given internal secret so it can run all the internal commands. Regular user can not run any internal command.

The PR require the following PR's to be merge:

  1. Add internal connection and command mechanism redis/redis#13740
  2. Added RM_GetInternalSecret to redismodule.h RedisLabsModules/redismodule-rs#407

The tests was modified to avoid giving password to the test module in case it detects that the internal commands feature exists. In such case, avoiding giving the password to the module, make sure that LibMR actually using the INTERNALAUTH command with the internal secret to authenticate.

todo:

@MeirShpilraien MeirShpilraien requested a review from iddm January 22, 2025 19:57
@MeirShpilraien
Copy link
Contributor Author

unstable CI is expected to pass once the relevant PR's will be merge.

Copy link
Contributor

@ephraimfeldblum ephraimfeldblum left a comment

Choose a reason for hiding this comment

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

LGTM

@MeirShpilraien MeirShpilraien merged commit 3506693 into master Feb 5, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants