Skip to content

MOD-8054: Validate index prefixes#5239

Merged
raz-mon merged 16 commits intomasterfrom
razmon-validate_index_prefixes
Nov 27, 2024
Merged

MOD-8054: Validate index prefixes#5239
raz-mon merged 16 commits intomasterfrom
razmon-validate_index_prefixes

Conversation

@raz-mon
Copy link
Collaborator

@raz-mon raz-mon commented Nov 21, 2024

This PR adds the validation of index prefixes for the FT.AGGREGATE, FT.SEARCH, FT.PROFILE commands between the coordinator and shards.
This fixes a potential in which the shard-index may change to a different index with the same name while a query while already dispatched from the coordinator is awaiting the shard execution. This situation may cause an ACL breach, or just yield irrelevant results.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@raz-mon raz-mon marked this pull request as draft November 21, 2024 12:16
@codecov
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 88.69565% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.69%. Comparing base (6e468f0) to head (84fa9aa).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/module.c 83.33% 8 Missing ⚠️
src/coord/dist_aggregate.c 90.47% 2 Missing ⚠️
src/aggregate/aggregate_exec.c 0.00% 1 Missing ⚠️
src/aggregate/aggregate_request.c 95.45% 1 Missing ⚠️
src/concurrent_ctx.c 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5239      +/-   ##
==========================================
- Coverage   86.70%   86.69%   -0.02%     
==========================================
  Files         193      193              
  Lines       34709    34788      +79     
==========================================
+ Hits        30095    30158      +63     
- Misses       4614     4630      +16     

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

@raz-mon raz-mon marked this pull request as ready for review November 24, 2024 07:20
Copy link
Collaborator

@GuyAv46 GuyAv46 left a comment

Choose a reason for hiding this comment

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

Very nice!
A few comments and suggestions

@raz-mon raz-mon requested review from GuyAv46 and oshadmi November 26, 2024 09:21
oshadmi
oshadmi previously approved these changes Nov 26, 2024
@raz-mon raz-mon requested review from GuyAv46 and oshadmi November 27, 2024 10:08
GuyAv46
GuyAv46 previously approved these changes Nov 27, 2024
Copy link
Collaborator

@GuyAv46 GuyAv46 left a comment

Choose a reason for hiding this comment

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

💯

@raz-mon raz-mon added this pull request to the merge queue Nov 27, 2024
Merged via the queue into master with commit 1aeaaaa Nov 27, 2024
@raz-mon raz-mon deleted the razmon-validate_index_prefixes branch November 27, 2024 13:57
@redisearch-backport-pull-request
Copy link
Contributor

Backport failed for 8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.0
git worktree add -d .worktree/backport-5239-to-8.0 origin/8.0
cd .worktree/backport-5239-to-8.0
git switch --create backport-5239-to-8.0
git cherry-pick -x 1aeaaaa86a53a1319223689e0572582869669bf9

raz-mon added a commit that referenced this pull request Nov 27, 2024
* wip

* Add index prefix validation for FT.SEARCH

* Some fixes

* Add validation for FT.AGGREGATE

* Fix ref releasing

* Fix test

* Fix tests

* Fix tests

* Remove unused context

* Address review 2

* Address review 3

* Address review 4

* Use VarArgs

* Remove flakiness from cluster initialization
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2024
* MOD-8054: Validate index prefixes (#5239)

* wip

* Add index prefix validation for FT.SEARCH

* Some fixes

* Add validation for FT.AGGREGATE

* Fix ref releasing

* Fix test

* Fix tests

* Fix tests

* Remove unused context

* Address review 2

* Address review 3

* Address review 4

* Use VarArgs

* Remove flakiness from cluster initialization

* Fix flaky cluster initialization test
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.

3 participants