Fix/collision of indexpatterns

[ThyTran1402]

Description

• Root cause: the previous implementation used a full Lucene automaton intersection (Operations.intersection) to decide whether two pattern sets conflict. This is correct since the string app-test--some_other--some- does technically satisfy both patterns simultaneously, but such a string would never appear as a real index name. Because * absorbs any characters (including - and _), wildcards in one pattern can "swap roles" to cover literal segments of the other, producing false conflicts for any two multi-wildcard patterns that share a common literal prefix before the first *

• Fix: Replace the automaton intersection with a minimum-string heuristic. For each pattern, every * is substituted with "" to produce its minimum matching string. If that string matches any pattern in the other set (using the existing Regex.simpleMatch), the two sets are considered conflicting. This restricts conflict detection to cases where naturally named index names would match both templates.

• The heuristic still catches genuine conflicts like baz vs baz*, or logs-* vs logs-prod-* while correctly accepting patterns that are distinguishable by literal segments placed after wildcards.

Related Issues

Resolves #837

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2f4111c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Overlap Detection The minimum-string heuristic only replaces * with "" to produce a minimum matching string, but this is not symmetric in all cases. For example, patterns like foo-*-bar and foo-*-bar-* would not be detected as overlapping by this heuristic (since foo--bar does not match foo-*-bar-* and foo--bar- does not match foo-*-bar), yet an index named foo-x-bar-y would match both. This could lead to silent conflicts at the same priority that go undetected. static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { return true; } } return false; } Asymmetric Heuristic The heuristic checks if the minimum string of each pattern in set1 matches any pattern in set2, and vice versa. However, it does not check if the minimum string of a pattern in set1 matches the minimum string of a pattern in set2. Two patterns like foo-*-bar and foo-*-bar* could both produce minimum strings that match each other only when tested against the original patterns, but edge cases may exist where neither direction catches a real overlap. static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { return true; } } return false; } Missing Test Cases The test testPatternsActuallyOverlap does not cover cases where one pattern is a pure wildcard * (which should overlap with everything), or cases with ? wildcards. Additionally, there is no test for patterns like foo-*-bar vs foo-*-bar-* which may be a genuine overlap missed by the heuristic. public void testPatternsActuallyOverlap() { // Patterns that share a genuine conflict: baz matches baz* assertTrue(MetadataIndexTemplateService.patternsActuallyOverlap(Arrays.asList("egg*", "baz"), Arrays.asList("abc", "baz*"))); // Patterns with multi-wildcards where literal segments after the first wildcard distinguish them assertFalse( MetadataIndexTemplateService.patternsActuallyOverlap( Arrays.asList("app-test-*-some-*"), Arrays.asList("app-test-*-some_other-*") ) ); // Patterns with a common prefix and different literal suffixes after a wildcard assertFalse(MetadataIndexTemplateService.patternsActuallyOverlap(Arrays.asList("foo-*-bar"), Arrays.asList("foo-*-baz"))); // Patterns where one is a superset of the other (logs-* matches any logs-prod-* index name) assertTrue(MetadataIndexTemplateService.patternsActuallyOverlap(Arrays.asList("logs-*"), Arrays.asList("logs-prod-*"))); // Completely disjoint literal prefixes do not overlap assertFalse(MetadataIndexTemplateService.patternsActuallyOverlap(Arrays.asList("foo-*"), Arrays.asList("bar-*"))); // Identical patterns overlap with themselves assertTrue(MetadataIndexTemplateService.patternsActuallyOverlap(Arrays.asList("app-*"), Arrays.asList("app-*"))); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 2f4111c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle single-character wildcard in heuristic Replacing with an empty string is a simplistic heuristic that can produce false negatives for patterns with ? wildcards or multiple characters in complex arrangements. For example, foo--bar- replaced becomes foo--bar-, which may not correctly match against foo-*-bar-baz. Consider also replacing ? with an empty string (or a representative character) to handle single-character wildcards consistently, since Regex.simpleMatch supports ? as well. server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java [886-896] for (String p1 : patterns1) { - if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { + if (Regex.simpleMatch(p2Array, p1.replace("*", "").replace("?", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { - if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { + if (Regex.simpleMatch(p1Array, p2.replace("*", "").replace("?", ""))) { return true; } } Suggestion importance[1-10]: 4 __ Why: The suggestion to also replace ? with an empty string is a valid improvement for consistency, since Regex.simpleMatch supports ? as a single-character wildcard and the current heuristic only handles *. However, ? wildcards are rarely used in index template patterns, making this a minor edge case improvement.	Low
Possible issue	Add exact pattern equality check The current heuristic only checks if the minimum string (wildcard replaced with empty string) matches the other set's patterns. However, it misses cases where two exact (non-wildcard) patterns are identical, since "foo".replace("*", "") equals "foo" and Regex.simpleMatch with an exact pattern would match — but more critically, it also misses cases where two literal patterns are equal but neither contains a wildcard. For example, patternsActuallyOverlap(["foo"], ["foo"]) should return true. Verify that Regex.simpleMatch handles exact string equality correctly (i.e., Regex.simpleMatch("foo", "foo") returns true), and add a direct equality check or a test case to confirm this edge case is handled. server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java [884-898] static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { + // Direct equality check for exact (non-wildcard) patterns + for (String p1 : patterns1) { + for (String p2 : patterns2) { + if (p1.equals(p2)) { + return true; + } + } + } String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { return true; } } return false; } Suggestion importance[1-10]: 2 __ Why: The concern about exact pattern equality is actually already handled by the existing heuristic: Regex.simpleMatch("foo", "foo") returns true since "foo".replace("*", "") equals "foo" and an exact pattern in simpleMatch matches itself. The suggestion adds redundant code and the test case patternsActuallyOverlap(["app-*"], ["app-*"]) already passes with the current implementation.	Low

---

Previous suggestions

Suggestions up to commit 91dc415

Category	Suggestion	Impact
General	Handle single-character wildcard in overlap heuristic The heuristic only replaces * with an empty string, but index patterns can also contain ? (single-character wildcard). A pattern like foo-?-bar would produce the minimum string foo--bar, which may incorrectly match or fail to match patterns. Consider also stripping ? characters (or replacing them with a representative character) when computing the minimum string to keep the heuristic consistent. server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java [884-898] static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { - if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { + if (Regex.simpleMatch(p2Array, p1.replace("*", "").replace("?", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { - if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { + if (Regex.simpleMatch(p1Array, p2.replace("*", "").replace("?", ""))) { return true; } } return false; } Suggestion importance[1-10]: 5 __ Why: The ? wildcard is a valid character in index patterns and stripping it alongside * would make the heuristic more consistent. However, this is a minor edge case since ? is rarely used in index template patterns, and the improvement is moderate in impact.	Low
	Guard against null or empty pattern lists Two literal (non-wildcard) patterns that are identical will correctly return true, but two identical wildcard patterns like ["foo-"] vs ["foo-"] will also return true because foo- matches foo-. However, two identical literal patterns like ["foo"] vs ["foo"] will also work. The real gap is when both patterns are purely a wildcard : p1.replace("", "") produces an empty string "", and Regex.simpleMatch([""], "") should return true, so this case is handled. But consider the edge case where patterns1 or patterns2 is empty — the method would return false silently, which is correct, but it's worth adding a null/empty guard to make the contract explicit and avoid potential NPEs if null is passed. server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java [884-898] static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { + if (patterns1 == null || patterns2 == null || patterns1.isEmpty() || patterns2.isEmpty()) { + return false; + } String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { return true; } } String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { return true; } } return false; } Suggestion importance[1-10]: 3 __ Why: Adding null/empty guards is a minor defensive improvement. The callers (findConflictingV1Templates and findConflictingV2Templates) pass template.patterns() and template.indexPatterns() which are unlikely to be null in practice, making this a low-impact suggestion.	Low

Suggestions up to commit f004a33

Category	Suggestion	Impact
General	Optimize array conversion efficiency The method converts patterns2 to an array twice (once per direction check) when checking bidirectional overlap. This is inefficient and can be optimized by converting both lists to arrays once before the loops, reducing redundant array allocations. server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java [884-898] static boolean patternsActuallyOverlap(List<String> patterns1, List<String> patterns2) { + String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); String[] p2Array = patterns2.toArray(Strings.EMPTY_ARRAY); for (String p1 : patterns1) { if (Regex.simpleMatch(p2Array, p1.replace("*", ""))) { return true; } } - String[] p1Array = patterns1.toArray(Strings.EMPTY_ARRAY); for (String p2 : patterns2) { if (Regex.simpleMatch(p1Array, p2.replace("*", ""))) { return true; } } return false; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that p1Array can be created earlier to avoid redundant work. However, the performance impact is minimal since the method returns early on first match, and the optimization only saves one array allocation in the worst case where no overlap is found.	Low

[github-actions]

Persistent review updated to latest commit 1eeb3f7

[github-actions]

PR Code Suggestions ✨

No code suggestions found for the PR.

[github-actions]

❌ Gradle check result for 1eeb3f7: 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?

[msfroh]

@ThyTran1402, can you please run ./gradlew :server:spotlessApply, add the changed files, commit them, and push them to your PR branch?

We're getting the following build failure:

* What went wrong:
Execution failed for task ':server:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
          @@ -1318,9 +1318,7 @@
           
           ····public·void·testPatternsActuallyOverlap()·{
           ········//·Patterns·that·share·a·genuine·conflict:·baz·matches·baz*
          -········assertTrue(
          ...
          -············0L,
          -············null,
      ... (7 more lines that didn't fit)
  Run './gradlew spotlessApply' to fix all violations.

[github-actions]

Persistent review updated to latest commit 5e0ac20

[ThyTran1402]

@ThyTran1402, can you please run ./gradlew :server:spotlessApply, add the changed files, commit them, and push them to your PR branch?

We're getting the following build failure:

* What went wrong:
Execution failed for task ':server:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/opensearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
          @@ -1318,9 +1318,7 @@
           
           ····public·void·testPatternsActuallyOverlap()·{
           ········//·Patterns·that·share·a·genuine·conflict:·baz·matches·baz*
          -········assertTrue(
          ...
          -············0L,
          -············null,
      ... (7 more lines that didn't fit)
  Run './gradlew spotlessApply' to fix all violations.

Hi @msfroh

I just pushed the changes. Can you review it again please?

Thank you.

[github-actions]

❌ Gradle check result for 5e0ac20: 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?

[msfroh]

Thanks, @ThyTran1402 -- I think I'm starting to understand.

The build is still failing, but that's just because it tries to check backward compatibility by running every test in a mixed-version cluster. In this case, because the change is going into OpenSearch 3.6.0, it doesn't work when mixed with OpenSearch 3.5 nodes or OpenSearch 2.19 nodes.

In your YamlRestTest, you just need to add a skip entry for versions less than 3.6.0 like:

---
"Put distinguishable multi-wildcard templates at the same priority":
  - skip:
      features: allowed_warnings
      version: " - 3.5.99"
      reason: "template overlap check was relaxed in 3.6"

[github-actions]

Persistent review updated to latest commit f0cd178

[ThyTran1402]

Thanks, @ThyTran1402 -- I think I'm starting to understand.

The build is still failing, but that's just because it tries to check backward compatibility by running every test in a mixed-version cluster. In this case, because the change is going into OpenSearch 3.6.0, it doesn't work when mixed with OpenSearch 3.5 nodes or OpenSearch 2.19 nodes.

In your YamlRestTest, you just need to add a skip entry for versions less than 3.6.0 like:

---
"Put distinguishable multi-wildcard templates at the same priority":
  - skip:
      features: allowed_warnings
      version: " - 3.5.99"
      reason: "template overlap check was relaxed in 3.6"

Yup. Thank you for guiding me through this test checks.
I just pushed the fix.

[github-actions]

❌ Gradle check result for f0cd178: 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?

[ThyTran1402]

Hi @msfroh

Can you please review it?

Thank you.

[github-actions]

Persistent review updated to latest commit 91dc415

[github-actions]

❌ Gradle check result for 91dc415: 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?

[ThyTran1402]

Hi @msfroh

Do you know why it still failed some of the gradle check for jenkins? 😭

[msfroh]

Hi @msfroh

Do you know why it still failed some of the gradle check for jenkins? 😭

Let me look into this today. I got working on some other stuff and somehow convinced myself that this had already been merged.

[ThyTran1402]

Hi @msfroh
Do you know why it still failed some of the gradle check for jenkins? 😭

Let me look into this today. I got working on some other stuff and somehow convinced myself that this had already been merged.

Please let me know if there're other test files or docs that I can look at to fix the tests 😭 .

[github-actions]

Persistent review updated to latest commit 2f4111c

[github-actions]

❌ Gradle check result for 2f4111c: 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?

[github-actions]

❌ Gradle check result for 2f4111c: 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?

[ThyTran1402]

It still did not pass jenkins. Is there anything that I should do from my side to pass the gradle jenkins tests? @msfroh
Since it seems like flaky test 😢

[msfroh]

@ThyTran1402 Nothing needed from your part, I think. We do have a lot of flaky tests. I'll keep retrying after confirming that the test failures are not related to your PR. (They probably aren't, but it helps to check.)

[github-actions]

❌ Gradle check result for 2f4111c: 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?

[ThyTran1402]

@ThyTran1402 Nothing needed from your part, I think. We do have a lot of flaky tests. I'll keep retrying after confirming that the test failures are not related to your PR. (They probably aren't, but it helps to check.)

Is there a way that I can take a look or we can reduce the flaky tests? @msfroh

[msfroh]

Is there a way that I can take a look or we can reduce the flaky tests?

So, @andrross has actually just recently created a new dashboard for the flakiest of tests: https://metrics.opensearch.org/_dashboards/app/dashboards#/view/87737cf0-1409-11f1-a611-eb8fce42240d

He's been picking them off one-by-one, starting with the worst offenders.

[github-actions]

❕ Gradle check result for 2f4111c: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.29%. Comparing base (dcbdefa) to head (2f4111c).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
...cluster/metadata/MetadataIndexTemplateService.java	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20702      +/-   ##
============================================
+ Coverage     73.25%   73.29%   +0.04%     
- Complexity    72164    72210      +46     
============================================
  Files          5794     5794              
  Lines        329946   329953       +7     
  Branches      47620    47624       +4     
============================================
+ Hits         241693   241838     +145     
+ Misses        68855    68681     -174     
- Partials      19398    19434      +36     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ThyTran1402]

Is there a way that I can take a look or we can reduce the flaky tests?

So, @andrross has actually just recently created a new dashboard for the flakiest of tests: https://metrics.opensearch.org/_dashboards/app/dashboards#/view/87737cf0-1409-11f1-a611-eb8fce42240d

He's been picking them off one-by-one, starting with the worst offenders.

Oh this is awesome 💯 . I have not checked or seen this new dashboard before. I think it helped a lot for catching flaky tests. Thank you for your suggestion.

[msfroh]

Oh -- looks like this finally passed on Friday evening. I spent the weekend sleeping off a nasty cold and didn't notice it.

Thank you @ThyTran1402 for fixing this issue! And a big thank you to @andrross for cleaning up so many of the flaky tests recently.