MOD-7613: Add search-oss-global-password by nafraf · Pull Request #5237 · RediSearch/RediSearch

nafraf · 2024-11-20T23:59:58Z

Describe the changes in the pull request

Configure the module parameter: search-oss-global-password which is equivalent to the previous OSS_GLOBAL_PASSWORD module parameter.
Configure the module parameter: search-oss-acl-username which is equivalent to the previous OSS_ACL_USERNAME

Which issues this PR fixes

MOD-7613

Main objects this PR modified

...
...

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

src/coord/config.c

codecov · 2024-12-09T13:01:27Z

Codecov Report

Attention: Patch coverage is 27.27273% with 16 lines in your changes missing coverage. Please review.

Project coverage is 87.24%. Comparing base (e017288) to head (61512b7).
Report is 2 commits behind head on nafraf_config-params.

Files with missing lines	Patch %	Lines
src/coord/config.c	5.88%	16 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           nafraf_config-params    #5237      +/-   ##
========================================================
- Coverage                 87.28%   87.24%   -0.04%     
========================================================
  Files                       196      196              
  Lines                     34986    34997      +11     
========================================================
- Hits                      30538    30534       -4     
- Misses                     4448     4463      +15

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

tests/pytests/test_config.py

src/coord/config.c

oshadmi · 2024-12-12T14:25:06Z

tests/pytests/test_config.py

+        .error().contains('CONFIG SET failed')
+
+
+@skip(cluster=False, redis_less_than='8.1.0')


Should be greater than 8.0.2? (what we have now)

Now it is skipped on less than 8.0, for redis-server 8.0.m02 the version number is v=7.9.225.
Another option is to skip it on less than 7.9.226 to include the test on the 8.0.m03 if it exists.

tests/pytests/test_config.py

src/coord/config.c

oshadmi · 2024-12-16T07:07:18Z

src/coord/config.c

+  if (clusterConfig.type == ClusterType_RedisOSS) {
+    if (RedisModule_RegisterStringConfig (
+          ctx, "search-oss-global-password", "",
+          REDISMODULE_CONFIG_IMMUTABLE | REDISMODULE_CONFIG_UNPREFIXED,


Should we use REDISMODULE_CONFIG_SENSITIVE flag as well to redact it in logging?

Yes, we should. Flag added

oshadmi · 2024-12-17T16:44:26Z

src/coord/config.c

+// acl-username
+RedisModuleString * get_oss_acl_username(const char *name, void *privdata) {
+  char *str = *(char **)privdata;
+  if (str) {


Can null check be dropped? (if initialized properly)

Yes. Check dropped.

oshadmi · 2024-12-17T16:51:09Z

src/coord/config.c

+    if (config_oss_acl_username) {
+      RedisModule_FreeString(NULL, config_oss_acl_username);
+    }


Not need to free previous value since it is immutable. Consider also denoting the setter is for immutable config params.

Changes done and setter function renamed.

tests/pytests/test_config.py

kei-nan · 2024-12-19T11:32:00Z

src/coord/config.c


 CONFIG_SETTER(setGlobalPass) {
+  RedisModule_Log(RSDummyContext, "warning",
+    "OSS_GLOBAL_PASSWORD is deprecated. Use `CONFIG SET search-oss-global-password <password>` instead");


Shouldn't the set be in the config file? since it is immutable
Maybe clarify that in the warning.
Also it isn't clear that the operation was able to set the value inf the warning.

…utable

* 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]>

nafraf added 3 commits November 20, 2024 18:58

WIP: Use oss-global-password from server

d4905c9

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

e93cf16

Register oss-global-password

b18c8f0

nafraf changed the base branch from master to nafraf_config-params November 22, 2024 00:55

nafraf added 3 commits November 22, 2024 09:48

Rename oss-global-password to global-password

461f8e7

Add search.acl-username config

0446525

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

731e3d9

nafraf changed the title ~~Replace OSS_GLOBAL_PASSWORD~~ MOD-7613: Add search-oss-global-password Dec 7, 2024

Config params only for redis > 8.1

9134d8b

nafraf marked this pull request as ready for review December 9, 2024 12:38

oshadmi reviewed Dec 9, 2024

View reviewed changes

src/coord/config.c Outdated Show resolved Hide resolved

oshadmi reviewed Dec 12, 2024

View reviewed changes

tests/pytests/test_config.py Show resolved Hide resolved

oshadmi reviewed Dec 12, 2024

View reviewed changes

src/coord/config.c Show resolved Hide resolved

oshadmi reviewed Dec 12, 2024

View reviewed changes

tests/pytests/test_config.py Show resolved Hide resolved

nafraf added 4 commits December 13, 2024 08:19

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

5e1a942

Refactor cluster configuration setters

d9f5cbf

Fix test skips

d94f26a

Fix test comment

bac9347

oshadmi reviewed Dec 15, 2024

View reviewed changes

src/coord/config.c Outdated Show resolved Hide resolved

oshadmi reviewed Dec 16, 2024

View reviewed changes

oshadmi reviewed Dec 17, 2024

View reviewed changes

tests/pytests/test_config.py Show resolved Hide resolved

nafraf added 2 commits December 17, 2024 23:20

Update cluster string config functions for immutability and sensitivity

67dfb08

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

e4ba053

kei-nan reviewed Dec 19, 2024

View reviewed changes

nafraf added 2 commits January 5, 2025 17:49

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

615d0b3

Remove warning for OSS_GLOBAL_PASSWORD config setter because it's imm…

7790b07

…utable

Merge branch 'nafraf_config-params' into nafraf_oss-global-password

61512b7

oshadmi approved these changes Jan 8, 2025

View reviewed changes

nafraf merged commit b60a09b into nafraf_config-params Jan 8, 2025
7 checks passed

nafraf deleted the nafraf_oss-global-password branch January 8, 2025 18:54

		.error().contains('CONFIG SET failed')


		@skip(cluster=False, redis_less_than='8.1.0')

Conversation

nafraf commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

oshadmi Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

nafraf Dec 13, 2024 • 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!

oshadmi Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

nafraf Dec 18, 2024 • 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!

oshadmi Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

nafraf Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

oshadmi Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

nafraf Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kei-nan Dec 19, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nafraf commented Nov 20, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

nafraf Dec 13, 2024 •

edited

Loading

nafraf Dec 18, 2024 •

edited

Loading

kei-nan Dec 19, 2024 •

edited

Loading