MOD-7465 MOD-7466: Add ACL support for RediSearch, phase 1 by raz-mon · Pull Request #4957 · RediSearch/RediSearch

raz-mon · 2024-08-25T11:16:57Z

Describe the changes in the pull request

This PR adds support for the Redis ACL feature in RediSearch, by creating a new search ACL command category, and registering the module commands to their appropriate command categories.

This PR also bumps the versions we use with Valgrind and Sanitizer to 7.4.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

codecov · 2024-08-25T11:40:19Z

Codecov Report

Attention: Patch coverage is 90.35088% with 11 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (9e1f2da) to head (43d6f71).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/module.c	88.15%	9 Missing ⚠️
src/coord/module.c	94.73%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4957      +/-   ##
==========================================
- Coverage   86.16%   86.14%   -0.02%     
==========================================
  Files         192      192              
  Lines       34235    34256      +21     
==========================================
+ Hits        29497    29509      +12     
- Misses       4738     4747       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

raz-mon · 2024-08-29T06:33:31Z

FYI some of the new tests are failing since they depend on the fix we merged to Redis, which was not released yet (pass against unstable), and will be introduced in v7.4.1.

src/coord/module.c

kei-nan · 2024-08-29T14:49:15Z

src/module.c


-  RM_TRY(RedisModule_CreateCommand, ctx, RS_INDEX_LIST_CMD, IndexList, "readonly", 0, 0, 0);
+  // Create the `search` ACL command category
+  RedisModuleString* cat_name = RedisModule_CreateString(ctx, "search", strlen("search"));


Maybe wrap this in a function that accepts a const char*?

What do you mean by "this"? The registration of the new category?

move lines 1012-1018 into a function.
basically yes, the creation of a new acl category

Why?
We do this only once, here, and under no circumstances will this code be called elsewhere - that is not aloud.
If it's to save 4 lines of code from this function I prefer not to.

src/module.c

src/module.h

src/coord/module.c

tests/pytests/test_acl.py

src/coord/module.c

tests/pytests/test_acl.py

* Support ACL for RediSearch commands - wip * Register RediSearch commands to redis ACL categories * Add tests * Small fix * Add coordinator commands registration * Register underscored commands to ACL as well * Remove dangerous from INFO * Skip test conditioned on redis version * Fix categories according to new evaluation * Remove comments * Fix * Address review * Remove TODOs * Replace macro with function * Wrap with RM_TRY * Fix unit-tests * Touchup * Address review * Address review * Address review * Bump sanitizer and Valgrind to use 7.4 * Address review * Touchups * Remove TODOs * Fix test * Address review * Move hack to src

* Move coord src into general module's src * remove internal src dir in coord * remove internal src dir in coord - now with cmake * WIP * single build path - wip * WIP * remove split * make things work - WIP - clean makefile, move pack * fix pack, move coord unit tests wip: resolve conflicts, fix tests, libuv? * fix conflicts, split test * remove build test step from CI * fixes * remove COORD=0 for coverage * revert extract module symbols * use foreach loop for coordinator unit tests * Addressing Dvir's CR - simplifying test deps and removing CMAKE_FILES var that seems useless * fix CMAKE_COORD var * MOD-7596: Add mirroring support (#4937) * * initial commit * * Use RediSearch branch mirroring fork * * Code Review - Round #1 * Change maxPending requests when changing the number of connections (#4972) change the number of maxPending requests when changing the number of connections * Remove RS_COORDINATOR flag and its associated code, fix bug in makefile for lite and static * fix to allow building as LITE * fix path in unit-tests to community * MOD-7695: Decide Ubuntu Runner Using Variable (#4975) * * initial commit * * code review * Enable MT in comunity eddition by default - [MOD-7160] (#4974) * remove compilation flag (wip) * finish removing compilation flag * remove from tests * split build for lite * MOD-7706 fix ramp-packer version (#4980) ramp-packer 2.5.10 * HNSW vector index - handle inplace deletion after async [MOD-7643] (#4964) * Fix inplace deletion in tiered index in vecsim * remove excessive thread iteration log, finalize test * update vecsim version * update vecsim version - fixing bug found in test vecsim with json multi * remove print from test * better logging * test - print warning always * test2 - print warning DEBUG AND CI ENV VARS * use force for env vars * try print instead * print for GHA annotation * remove light build flag, keep lite as an option fro backward compatibility * Fix warning * use llvm 18 * try to include in cmake cpp test dir * try build without special includes for apple * set apple test * Set APPLE flag for MACOS * debug print * MOD-7737: Add relaxation flags to Rocky9 installations (#4989) Add relaxing installation flags to Rocky platform * MOD-7646 cmake fetch boost if not exists (#4984) cmake fetch boost if not exists * MOD-7465 MOD-7466: Add ACL support for RediSearch, phase 1 (#4957) * Support ACL for RediSearch commands - wip * Register RediSearch commands to redis ACL categories * Add tests * Small fix * Add coordinator commands registration * Register underscored commands to ACL as well * Remove dangerous from INFO * Skip test conditioned on redis version * Fix categories according to new evaluation * Remove comments * Fix * Address review * Remove TODOs * Replace macro with function * Wrap with RM_TRY * Fix unit-tests * Touchup * Address review * Address review * Address review * Bump sanitizer and Valgrind to use 7.4 * Address review * Touchups * Remove TODOs * Fix test * Address review * Move hack to src * try removing visibility * try remove visibility for coord only * apply changes as patch after rebasing from new master * revert visibility hidden * revert visibility hidden * revert * add verbosity for debug * add init-rm to cpp tests * Copy init-rm in cpptests * clean * avoid -executable in mac * move REDISMODULE_MAIN to module-init.c * try removing init-rm from cpp tests * update vecsim * unify module.c files from coord so that "onload" is on the module not coord * remove init-rm * bring back sdsfree in unit test, link hiredis/sds with extern c * remove coord_module.h, align vecsim deps * remove sds from rmutil * remove RMUTIL_NO_SDS totally --------- Co-authored-by: kei-nan <[email protected]> Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: DvirDukhan <[email protected]> Co-authored-by: Raz Monsonego <[email protected]>

* WIP: Config parameters * Refactor config register * MOD-7596: Add mirroring support (#4937) * * initial commit * * Use RediSearch branch mirroring fork * * Code Review - Round #1 * Change maxPending requests when changing the number of connections (#4972) change the number of maxPending requests when changing the number of connections * Test boolean parameters * Fix boolean params default values * MOD-7695: Decide Ubuntu Runner Using Variable (#4975) * * initial commit * * code review * Enable MT in comunity eddition by default - [MOD-7160] (#4974) * remove compilation flag (wip) * finish removing compilation flag * remove from tests * MOD-7706 fix ramp-packer version (#4980) ramp-packer 2.5.10 * HNSW vector index - handle inplace deletion after async [MOD-7643] (#4964) * Fix inplace deletion in tiered index in vecsim * remove excessive thread iteration log, finalize test * update vecsim version * update vecsim version - fixing bug found in test vecsim with json multi * remove print from test * better logging * test - print warning always * test2 - print warning DEBUG AND CI ENV VARS * use force for env vars * try print instead * print for GHA annotation * Add two immutable params * Fix max-doctablesize name * MOD-7737: Add relaxation flags to Rocky9 installations (#4989) Add relaxing installation flags to Rocky platform * MOD-7646 cmake fetch boost if not exists (#4984) cmake fetch boost if not exists * MOD-7465 MOD-7466: Add ACL support for RediSearch, phase 1 (#4957) * Support ACL for RediSearch commands - wip * Register RediSearch commands to redis ACL categories * Add tests * Small fix * Add coordinator commands registration * Register underscored commands to ACL as well * Remove dangerous from INFO * Skip test conditioned on redis version * Fix categories according to new evaluation * Remove comments * Fix * Address review * Remove TODOs * Replace macro with function * Wrap with RM_TRY * Fix unit-tests * Touchup * Address review * Address review * Address review * Bump sanitizer and Valgrind to use 7.4 * Address review * Touchups * Remove TODOs * Fix test * Address review * Move hack to src * WIP: Register numeric params * WIP: Config numeric params * WIP: CONFIG value precedence * Complete boolean parameters * Test search.partial-indexed-docs * Fix string configs * Add index-cursor-limit config * Fix unit tests * Create cluster configuration params * Fix cluster configuration params * Test MODULE LOADEX numeric config * WIP: Test MODULE LOADEX bool config * Fix set_no_gc() * Fix string config parameters * Test MODULE LOADEX enum config * Fix set_workers() * Fix numeric parameter limit * Test boolean parameters with config file * Test with config file: numeric, enum, string * Add max-aggregate-results and max-search-results * tidy up * Add more tests * Use REDISMODULE_CONFIG_UNPREFIXED flag * Revert "Use REDISMODULE_CONFIG_UNPREFIXED flag" This reverts commit b1985fc. * Use common setfn, getfn for numeric params * Fix mem leak 1 * Fix mem leak 2 - string config * Disable immutable limits tests * Use RLTest >= 0.7.14 * Check version before loading config * Add testConfigAPILoadTimeNumericParams() * Fix unit-tests * Refactor config API function parameters for consistency * Use privdata in numeric config params * Refactor boolean config registration and clean up unused macros * Refactor configuration setter functions to improve consistency * Refactor coord config functions * Refactor config functions for improved clarity and consistency * search-gc-scan-size is Load-Time immutable * Fixes from code review * Remove parameter modified validation * Fix search-ext-load and search-friso-ini tests * Remove global config vars * Remove str validation from get_string_config() * Update default configuration to initialize extLoad and frisoIni as empty strings * MOD-7613: Add search-oss-global-password (#5237) 1. Configure the module parameter search-oss-global-password which is equivalent to the previous OSS_GLOBAL_PASSWORD module parameter. 2. Configure the module parameter search-oss-acl-username which is equivalent to the previous OSS_ACL_USERNAME --------- Co-authored-by: kei-nan <[email protected]> Co-authored-by: GuyAv46 <[email protected]> Co-authored-by: DvirDukhan <[email protected]> Co-authored-by: alonre24 <[email protected]> Co-authored-by: Raz Monsonego <[email protected]> Co-authored-by: oshadmi <[email protected]>

raz-mon added 3 commits August 25, 2024 11:27

Support ACL for RediSearch commands - wip

eb7d9dc

Register RediSearch commands to redis ACL categories

ca0656f

Add tests

ce76e85

raz-mon added 2 commits August 25, 2024 17:04

Small fix

5e0fc53

Add coordinator commands registration

aab7bf5

raz-mon requested review from kei-nan and oshadmi August 28, 2024 16:21

Register underscored commands to ACL as well

7652106

raz-mon added 3 commits August 29, 2024 10:32

Remove dangerous from INFO

6aeaf4a

Skip test conditioned on redis version

fb0c187

Fix categories according to new evaluation

9b5692f

raz-mon requested a review from nafraf August 29, 2024 14:45