-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Ensure index templates are not applied to system indices #16418
base: main
Are you sure you want to change the base?
Ensure index templates are not applied to system indices #16418
Conversation
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Please advice for backporting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach LGTM.
How can we test this? I've been looking at MetadataCreateIndexTests
and don't see that we ever test this method directly.
server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
Outdated
Show resolved
Hide resolved
We should definitely backport to 2.x. The SystemIndices code is 2.16 though so we can't go further back. |
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
I will add some test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM! Waiting for tests to approve.
❌ Gradle check result for f51c296: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java
Show resolved
Hide resolved
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Added test cases, please review it. Also, modified one of the methods' access from private to public to enable testing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16418 +/- ##
============================================
- Coverage 72.16% 72.11% -0.06%
+ Complexity 65115 65065 -50
============================================
Files 5313 5313
Lines 303362 303375 +13
Branches 43901 43904 +3
============================================
- Hits 218917 218765 -152
- Misses 66489 66653 +164
- Partials 17956 17957 +1 ☔ View full report in Codecov by Sentry. |
@pyek-bot this needs a changelog entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make it package private (no modifier) so that a test in the same package can access it. |
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense to me. Just one comment about the unit test. @dbwiddis is there any way to create an integration test of the scenario you observed that would validate this fix?
metadataCreateIndexService.applyCreateIndexRequest(clusterState, request, false, mockMetadataTransformer); | ||
verify(metadataCreateIndexService).applyCreateIndexRequestWithNoTemplates( | ||
eq(clusterState), | ||
eq(request), | ||
eq(false), | ||
eq(mockMetadataTransformer) | ||
); | ||
|
||
verify(metadataCreateIndexService).applyCreateIndexRequestWithV1Templates( | ||
eq(clusterState), | ||
eq(request), | ||
eq(false), | ||
eq(Collections.emptyList()), | ||
eq(mockMetadataTransformer) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't think tests like these add a lot of value. You're really just testing that you're calling an internal method that you think you're calling. This isn't verifying that the code is actually doing the thing you expect it to be doing (i.e. that index templates are not actually applied to a system index). Furthermore, this becomes brittle to internal refactorings of the class under test even if the refactoring does not change the behavior of the class at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way to create an integration test of the scenario you observed that would validate this fix?
Good call-out. This seems the better approach here. The repro in the original issue should be easy to set up in an integ test, the only hitch here is making sure systemIndices
is populated with whatever test index we're choosing, unless the test is isolated enough that we can use one of the actual indices specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an YAML spec or IT test case would definitely be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, IT test will actually validate this behavior rather than simply check for a method call. Is there a guide to how such a test can be added? I could take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pyek-bot Here is an example test that asserts that deprecation warnings are emitted when a user writes to a system index:
OpenSearch/qa/smoke-test-http/src/test/java/org/opensearch/http/SystemIndexRestIT.java
Line 82 in 4e45e44
public void testSystemIndexAccessBlockedByDefault() throws Exception { |
Description
Prevents index templates from applying to system indices
Related Issues
Resolves #16340
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.