Ensure index templates are not applied to system indices #16418

pyek-bot · 2024-10-22T02:41:55Z

Description

Prevents index templates from applying to system indices

Related Issues

Resolves #16340

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

pyek-bot · 2024-10-22T02:43:42Z

Please advice for backporting

dbwiddis

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

dbwiddis · 2024-10-22T03:09:23Z

Please advice for backporting

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>

pyek-bot · 2024-10-22T03:42:02Z

Approach LGTM.

How can we test this? I've been looking at MetadataCreateIndexTests and don't see that we ever test this method directly.

I will add some test cases

dbwiddis

Code changes LGTM! Waiting for tests to approve.

github-actions · 2024-10-22T04:21:39Z

❌ 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

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

pyek-bot · 2024-10-22T20:33:23Z

Code changes LGTM! Waiting for tests to approve.

Added test cases, please review it. Also, modified one of the methods' access from private to public to enable testing.

github-actions · 2024-10-22T21:39:26Z

✅ Gradle check result for d838ec2: SUCCESS

codecov · 2024-10-22T21:40:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.11%. Comparing base (5941a7e) to head (09a27c9).
Report is 4 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

dbwiddis · 2024-10-23T03:18:34Z

@pyek-bot this needs a changelog entry

dbwiddis

LGTM pending changelog entry. Would appreciate review of @reta or @dblock or @andrross here. I'm confident in the code, but we're fundamentally changing (undocumented and poor) behavior (in what I think is a very good way) so would like more eyes on.

dbwiddis · 2024-10-23T04:27:15Z

Also, modified one of the methods' access from private to public to enable testing.

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>

github-actions · 2024-10-23T17:36:35Z

✅ Gradle check result for 09a27c9: SUCCESS

dbwiddis

LGTM!

andrross

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?

andrross · 2024-10-23T19:21:54Z

server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java

+        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)
+        );


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.

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.

Having an YAML spec or IT test case would definitely be helpful

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.

@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 {

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java

fix: ensure system indices are processed without templates

9d80a9d

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

github-actions bot added bug Something isn't working Indexing Indexing, Bulk Indexing and anything related to indexing labels Oct 22, 2024

pyek-bot mentioned this pull request Oct 22, 2024

[BUG] Composable index template mapping can prevent creating an index if mappings conflict #16340

Open

pyek-bot changed the title ~~[Bug fix] Ensure index templates are not applied to system indices~~ Ensure index templates are not applied to system indices Oct 22, 2024

dbwiddis reviewed Oct 22, 2024

View reviewed changes

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

dbwiddis added the backport 2.x Backport to 2.x branch label Oct 22, 2024

refactor: overloaded method for creating without templates

f51c296

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

dbwiddis reviewed Oct 22, 2024

View reviewed changes

opensearch-ci-bot mentioned this pull request Oct 22, 2024

[AUTOCUT] Gradle Check Flaky Test Report for ApproximatePointRangeQueryTests #15807

Open

cwperks reviewed Oct 22, 2024

View reviewed changes

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java Show resolved Hide resolved

test: adding test to check call for notemplates on system index

d838ec2

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

dbwiddis approved these changes Oct 23, 2024

View reviewed changes

pyek-bot added 2 commits October 23, 2024 09:27

refactor: cchange modifier to package private and add entry in changelog

e1090f0

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

fix conflicts and merge main

09a27c9

Signed-off-by: Pavan Yekbote <mail2pavanyekbote@gmail.com>

dbwiddis approved these changes Oct 23, 2024

View reviewed changes

andrross reviewed Oct 23, 2024

View reviewed changes

reta reviewed Oct 23, 2024

View reviewed changes

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java Show resolved Hide resolved

reta added v3.0.0 Issues and PRs related to version 3.0.0 v2.19.0 Issues and PRs related to version 2.19.0 labels Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure index templates are not applied to system indices #16418

Ensure index templates are not applied to system indices #16418

pyek-bot commented Oct 22, 2024 •

edited

Loading

pyek-bot commented Oct 22, 2024

dbwiddis left a comment

dbwiddis commented Oct 22, 2024

pyek-bot commented Oct 22, 2024

dbwiddis left a comment

github-actions bot commented Oct 22, 2024

pyek-bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

dbwiddis commented Oct 23, 2024

dbwiddis left a comment •

edited

Loading

dbwiddis commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024

dbwiddis left a comment

andrross left a comment

andrross Oct 23, 2024

dbwiddis Oct 23, 2024

reta Oct 23, 2024 •

edited

Loading

pyek-bot Oct 23, 2024

andrross Oct 23, 2024

Ensure index templates are not applied to system indices #16418

Are you sure you want to change the base?

Ensure index templates are not applied to system indices #16418

Conversation

pyek-bot commented Oct 22, 2024 • edited Loading

Description

Related Issues

Check List

pyek-bot commented Oct 22, 2024

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis commented Oct 22, 2024

pyek-bot commented Oct 22, 2024

dbwiddis left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024

pyek-bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

dbwiddis commented Oct 23, 2024

dbwiddis left a comment • edited Loading

Choose a reason for hiding this comment

dbwiddis commented Oct 23, 2024 • edited Loading

github-actions bot commented Oct 23, 2024

dbwiddis left a comment

Choose a reason for hiding this comment

andrross left a comment

Choose a reason for hiding this comment

andrross Oct 23, 2024

Choose a reason for hiding this comment

dbwiddis Oct 23, 2024

Choose a reason for hiding this comment

reta Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

pyek-bot Oct 23, 2024

Choose a reason for hiding this comment

andrross Oct 23, 2024

Choose a reason for hiding this comment

pyek-bot commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

dbwiddis left a comment •

edited

Loading

dbwiddis commented Oct 23, 2024 •

edited

Loading

reta Oct 23, 2024 •

edited

Loading