Skip to content

Commit 958b33f

Browse files
mohityadav766claude
mohityadav766
and
claude
authored
fix(search): scope alias lookups to cluster prefix on shared OpenSearch clusters (#27466)
* fix(search): scope alias and stats lookups to clusterAlias prefix On shared OpenSearch/Elasticsearch clusters where tenant roles only grant indices:admin/aliases/get on their own prefix, the orphaned index cleanup and metrics refresh were failing with 403 Forbidden because listIndicesByPrefix("") and getAllIndexStats() issued unscoped GET /*/_alias and stats("*") requests. Route both through a shared buildScopedPattern() that substitutes {clusterAlias}_* when the caller passes an empty prefix and a cluster alias is configured, so each deployment only reads its own indices. Explicit non-empty prefixes are already cluster-qualified by their callers and are left untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(search): add IT for cluster-scoped orphaned index cleanup Verifies that on a shared search cluster where the app is configured with clusterAlias="openmetadata", the orphaned-index cleanup and index-listing paths only read / touch indices matching {clusterAlias}_*. The test provisions a "foreign tenant" directly against the real OpenSearch/Elasticsearch container by creating indices under a different prefix (foreigntenant_it_orphans_*), then asserts: 1. listIndicesByPrefix("") never returns foreign-prefixed indices 2. getAllIndexStats() never returns foreign-prefixed indices 3. OrphanedIndexCleaner.cleanupOrphanedIndices() only deletes orphans under the configured cluster prefix, leaving foreign tenant indices (both orphaned and live) intact Security plugin is disabled in the IT bootstrap, so the exact 403 cannot be reproduced — but the behavioral guarantee that prevents it (never issuing unscoped GET /*/_alias) is verified here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(search): use IndexMapping.INDEX_NAME_SEPARATOR in scoped pattern Centralize the cluster-prefix separator so the scoped wildcard is built from the same constant used by getIndexName() / getAlias(). Addresses review feedback on #27466. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(search): address review feedback on scoped pattern IT - Use IndexMapping.INDEX_NAME_SEPARATOR in unit test assertions for parity with production code. - Rewrite the IT's cleanup test as a read-only discovery test via findOrphanedRebuildIndices(). cleanupOrphanedIndices() is a globally-scoped destructive op that could race with parallel ITs creating _rebuild_ indices under the same shared openmetadata_* namespace. Discovery-scope is the invariant that produces the 403 prevention; per-index deletion is already covered by unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(search): make scoped cleanup IT setup idempotent @BeforeAll was calling createIndex() directly, which returns 400 if the index already exists from a prior failed run (or a re-run against a reused container). Delete first, then create, so setUp is safe to rerun. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent dbf6410 commit 958b33f

5 files changed

Lines changed: 301 additions & 6 deletions

File tree

‎openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedIndexCleanerScopedCleanupIT.java‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
/*
2+
* Copyright 2024 Collate
3+
* Licensed under the Apache License, Version 2.0 (the "License");
4+
* you may not use this file except in compliance with the License.
5+
* You may obtain a copy of the License at
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*/
8+
9+
package org.openmetadata.it.tests;
10+
11+
import static org.junit.jupiter.api.Assertions.assertFalse;
12+
import static org.junit.jupiter.api.Assertions.assertTrue;
13+
14+
import es.co.elastic.clients.transport.rest5_client.low_level.Request;
15+
import es.co.elastic.clients.transport.rest5_client.low_level.Rest5Client;
16+
import java.util.List;
17+
import java.util.Set;
18+
import java.util.concurrent.TimeUnit;
19+
import org.junit.jupiter.api.AfterAll;
20+
import org.junit.jupiter.api.BeforeAll;
21+
import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
22+
import org.junit.jupiter.api.Order;
23+
import org.junit.jupiter.api.Test;
24+
import org.junit.jupiter.api.TestMethodOrder;
25+
import org.junit.jupiter.api.parallel.Execution;
26+
import org.junit.jupiter.api.parallel.ExecutionMode;
27+
import org.openmetadata.it.bootstrap.TestSuiteBootstrap;
28+
import org.openmetadata.service.Entity;
29+
import org.openmetadata.service.apps.bundles.searchIndex.OrphanedIndexCleaner;
30+
import org.openmetadata.service.search.IndexManagementClient.IndexStats;
31+
import org.openmetadata.service.search.SearchClient;
32+
import org.openmetadata.service.search.SearchRepository;
33+
34+
/**
35+
* Verifies that on a shared search cluster where the app is configured with a non-empty
36+
* {@code clusterAlias}, the orphaned-index cleanup and index-listing paths only read / touch
37+
* indices matching {@code {clusterAlias}_*}.
38+
*
39+
* <p>In production this prevents the {@code indices:admin/aliases/get} 403 reported in
40+
* openmetadata-collate#3557: if we never ask OpenSearch for foreign indices, the tenant role
41+
* never needs permission on them.
42+
*
43+
* <p>The test simulates a "foreign tenant" by creating indices with a different prefix directly
44+
* on the container (security plugin is disabled in the IT bootstrap, so the 403 itself cannot be
45+
* reproduced — but the behavioral guarantee that produces it is verified here).
46+
*/
47+
@Execution(ExecutionMode.SAME_THREAD)
48+
@TestMethodOrder(OrderAnnotation.class)
49+
public class OrphanedIndexCleanerScopedCleanupIT {
50+
51+
private static final String CLUSTER_ALIAS = "openmetadata";
52+
private static final String FOREIGN_PREFIX = "foreigntenant_it_orphans";
53+
private static final String OUR_PREFIX = CLUSTER_ALIAS + "_it_orphans";
54+
55+
private static final long OLD_TIMESTAMP =
56+
System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(45);
57+
58+
private static final String OUR_ORPHAN = OUR_PREFIX + "_table_rebuild_" + OLD_TIMESTAMP;
59+
private static final String FOREIGN_ORPHAN = FOREIGN_PREFIX + "_table_rebuild_" + OLD_TIMESTAMP;
60+
private static final String FOREIGN_LIVE = FOREIGN_PREFIX + "_table_live";
61+
private static final String FOREIGN_LIVE_ALIAS = FOREIGN_PREFIX + "_alias";
62+
63+
private static Rest5Client lowLevelClient;
64+
65+
@BeforeAll
66+
static void setUp() throws Exception {
67+
// Sanity-check: the app under test must have the cluster alias configured, otherwise this
68+
// test is not exercising the scoping behavior at all.
69+
SearchRepository searchRepo = Entity.getSearchRepository();
70+
assertTrue(
71+
CLUSTER_ALIAS.equals(searchRepo.getClusterAlias()),
72+
"Test expects cluster alias '"
73+
+ CLUSTER_ALIAS
74+
+ "' but got '"
75+
+ searchRepo.getClusterAlias()
76+
+ "'");
77+
78+
lowLevelClient = TestSuiteBootstrap.createSearchClient();
79+
80+
// Idempotent: drop any residue from a prior failed run before creating.
81+
for (String index : List.of(OUR_ORPHAN, FOREIGN_ORPHAN, FOREIGN_LIVE)) {
82+
deleteIndexQuietly(index);
83+
}
84+
85+
createIndex(OUR_ORPHAN);
86+
createIndex(FOREIGN_ORPHAN);
87+
createIndex(FOREIGN_LIVE);
88+
addAlias(FOREIGN_LIVE, FOREIGN_LIVE_ALIAS);
89+
}
90+
91+
@AfterAll
92+
static void tearDown() throws Exception {
93+
if (lowLevelClient == null) {
94+
return;
95+
}
96+
// Best-effort cleanup — the cleaner may have already removed OUR_ORPHAN.
97+
for (String index : List.of(OUR_ORPHAN, FOREIGN_ORPHAN, FOREIGN_LIVE)) {
98+
deleteIndexQuietly(index);
99+
}
100+
lowLevelClient.close();
101+
}
102+
103+
@Test
104+
@Order(1)
105+
void listIndicesByPrefixWithEmptyPrefixOnlyReturnsClusterScopedIndices() {
106+
SearchClient client = Entity.getSearchRepository().getSearchClient();
107+
108+
Set<String> indices = client.listIndicesByPrefix("");
109+
110+
assertTrue(
111+
indices.contains(OUR_ORPHAN),
112+
"Expected our-prefix orphan " + OUR_ORPHAN + " to be listed, got " + indices);
113+
assertFalse(
114+
indices.contains(FOREIGN_ORPHAN),
115+
"Foreign orphan " + FOREIGN_ORPHAN + " must not be listed (cross-tenant leak)");
116+
assertFalse(
117+
indices.contains(FOREIGN_LIVE),
118+
"Foreign live index " + FOREIGN_LIVE + " must not be listed (cross-tenant leak)");
119+
for (String name : indices) {
120+
assertTrue(
121+
name.startsWith(CLUSTER_ALIAS + "_"),
122+
"Index " + name + " outside cluster prefix should not be returned");
123+
}
124+
}
125+
126+
@Test
127+
@Order(2)
128+
void getAllIndexStatsOnlyReturnsClusterScopedIndices() throws Exception {
129+
SearchClient client = Entity.getSearchRepository().getSearchClient();
130+
131+
List<IndexStats> stats = client.getAllIndexStats();
132+
133+
for (IndexStats stat : stats) {
134+
assertTrue(
135+
stat.name().startsWith(CLUSTER_ALIAS + "_"),
136+
"Stats for " + stat.name() + " returned from outside cluster prefix");
137+
}
138+
assertTrue(
139+
stats.stream().anyMatch(s -> s.name().equals(OUR_ORPHAN)),
140+
"Expected stats for our-prefix orphan " + OUR_ORPHAN);
141+
assertFalse(
142+
stats.stream().anyMatch(s -> s.name().equals(FOREIGN_ORPHAN)),
143+
"Foreign orphan " + FOREIGN_ORPHAN + " must not appear in stats");
144+
}
145+
146+
/**
147+
* Read-only assertion that orphan discovery only looks at indices under the cluster prefix.
148+
*
149+
* <p>We deliberately avoid calling {@link OrphanedIndexCleaner#cleanupOrphanedIndices} here:
150+
* that is a destructive, globally-scoped operation and would race with other ITs that may
151+
* create temporary {@code _rebuild_} indices under the same shared {@code openmetadata_*}
152+
* namespace. Since cleanup = discovery + per-index delete, verifying discovery is scoped is
153+
* sufficient for the 403-prevention guarantee; per-index deletion is covered by unit tests.
154+
*/
155+
@Test
156+
@Order(3)
157+
void findOrphanedRebuildIndicesOnlyDiscoversClusterScopedOrphans() {
158+
SearchClient client = Entity.getSearchRepository().getSearchClient();
159+
OrphanedIndexCleaner cleaner = new OrphanedIndexCleaner();
160+
161+
List<OrphanedIndexCleaner.OrphanedIndex> orphans = cleaner.findOrphanedRebuildIndices(client);
162+
163+
assertTrue(
164+
orphans.stream().anyMatch(o -> o.indexName().equals(OUR_ORPHAN)),
165+
"Expected our-prefix orphan "
166+
+ OUR_ORPHAN
167+
+ " to be discovered, got "
168+
+ orphans.stream().map(OrphanedIndexCleaner.OrphanedIndex::indexName).toList());
169+
assertFalse(
170+
orphans.stream().anyMatch(o -> o.indexName().equals(FOREIGN_ORPHAN)),
171+
"Foreign orphan " + FOREIGN_ORPHAN + " must not be discovered (cross-tenant leak)");
172+
for (OrphanedIndexCleaner.OrphanedIndex orphan : orphans) {
173+
assertTrue(
174+
orphan.indexName().startsWith(CLUSTER_ALIAS + "_"),
175+
"Discovered orphan " + orphan.indexName() + " is outside cluster prefix");
176+
}
177+
assertTrue(indexExists(FOREIGN_ORPHAN), "Foreign orphan must still exist (never touched)");
178+
assertTrue(indexExists(FOREIGN_LIVE), "Foreign live index must still exist (never touched)");
179+
}
180+
181+
private static void createIndex(String name) throws Exception {
182+
Request request = new Request("PUT", "/" + name);
183+
request.setJsonEntity(
184+
"{\"settings\":{\"index\":{\"number_of_shards\":1,\"number_of_replicas\":0}}}");
185+
lowLevelClient.performRequest(request);
186+
}
187+
188+
private static void addAlias(String index, String alias) throws Exception {
189+
Request request = new Request("POST", "/_aliases");
190+
request.setJsonEntity(
191+
String.format(
192+
"{\"actions\":[{\"add\":{\"index\":\"%s\",\"alias\":\"%s\"}}]}", index, alias));
193+
lowLevelClient.performRequest(request);
194+
}
195+
196+
private static boolean indexExists(String name) {
197+
try {
198+
Request request = new Request("HEAD", "/" + name);
199+
return lowLevelClient.performRequest(request).getStatusCode() == 200;
200+
} catch (Exception e) {
201+
return false;
202+
}
203+
}
204+
205+
private static void deleteIndexQuietly(String name) {
206+
try {
207+
lowLevelClient.performRequest(new Request("DELETE", "/" + name));
208+
} catch (Exception ignored) {
209+
// Best-effort cleanup.
210+
}
211+
}
212+
}

‎openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchIndexManager.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -458,23 +458,36 @@ public Set<String> listIndicesByPrefix(String prefix) {
458458
return indices;
459459
}
460460
try {
461-
String pattern = prefix + "*";
461+
String pattern = buildScopedPattern(prefix);
462462
GetAliasRequest request = GetAliasRequest.of(g -> g.index(pattern));
463463
GetAliasResponse response = client.indices().getAlias(request);
464464

465465
indices.addAll(response.aliases().keySet());
466466

467-
LOG.info("Retrieved {} indices matching prefix '{}': {}", indices.size(), prefix, indices);
467+
LOG.info(
468+
"Retrieved {} indices matching pattern '{}' (prefix='{}'): {}",
469+
indices.size(),
470+
pattern,
471+
prefix,
472+
indices);
468473
} catch (Exception e) {
469474
LOG.error("Failed to list indices by prefix {} due to", prefix, e);
470475
}
471476
return indices;
472477
}
473478

479+
private String buildScopedPattern(String prefix) {
480+
if (prefix != null && !prefix.isEmpty()) {
481+
return prefix + "*";
482+
}
483+
return clusterAlias.isEmpty() ? "*" : clusterAlias + IndexMapping.INDEX_NAME_SEPARATOR + "*";
484+
}
485+
474486
@Override
475487
public List<IndexStats> getAllIndexStats() throws IOException {
476488
List<IndexStats> result = new ArrayList<>();
477-
var statsResponse = client.indices().stats(s -> s.index("*"));
489+
String statsPattern = buildScopedPattern(null);
490+
var statsResponse = client.indices().stats(s -> s.index(statsPattern));
478491
var indices = statsResponse.indices();
479492
for (var entry : indices.entrySet()) {
480493
String indexName = entry.getKey();

‎openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchIndexManager.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -538,23 +538,36 @@ public Set<String> listIndicesByPrefix(String prefix) {
538538
return indices;
539539
}
540540
try {
541-
String pattern = prefix + "*";
541+
String pattern = buildScopedPattern(prefix);
542542
GetAliasRequest request = GetAliasRequest.of(g -> g.index(pattern));
543543
GetAliasResponse response = client.indices().getAlias(request);
544544

545545
indices.addAll(response.result().keySet());
546546

547-
LOG.info("Retrieved {} indices matching prefix '{}': {}", indices.size(), prefix, indices);
547+
LOG.info(
548+
"Retrieved {} indices matching pattern '{}' (prefix='{}'): {}",
549+
indices.size(),
550+
pattern,
551+
prefix,
552+
indices);
548553
} catch (Exception e) {
549554
LOG.error("Failed to list indices by prefix {} due to", prefix, e);
550555
}
551556
return indices;
552557
}
553558

559+
private String buildScopedPattern(String prefix) {
560+
if (prefix != null && !prefix.isEmpty()) {
561+
return prefix + "*";
562+
}
563+
return clusterAlias.isEmpty() ? "*" : clusterAlias + IndexMapping.INDEX_NAME_SEPARATOR + "*";
564+
}
565+
554566
@Override
555567
public List<IndexStats> getAllIndexStats() throws IOException {
556568
List<IndexStats> result = new ArrayList<>();
557-
var statsResponse = client.indices().stats(s -> s.index("*"));
569+
String statsPattern = buildScopedPattern(null);
570+
var statsResponse = client.indices().stats(s -> s.index(statsPattern));
558571
var indices = statsResponse.indices();
559572
for (var entry : indices.entrySet()) {
560573
String indexName = entry.getKey();

‎openmetadata-service/src/test/java/org/openmetadata/service/search/elasticsearch/ElasticSearchIndexManagerTest.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import static org.junit.jupiter.api.Assertions.assertFalse;
66
import static org.junit.jupiter.api.Assertions.assertNotNull;
77
import static org.junit.jupiter.api.Assertions.assertTrue;
8+
import static org.mockito.ArgumentCaptor.forClass;
89
import static org.mockito.ArgumentMatchers.any;
910
import static org.mockito.Mockito.doReturn;
1011
import static org.mockito.Mockito.lenient;
@@ -576,6 +577,34 @@ void testListIndicesByPrefix_HandlesException() throws IOException {
576577
verify(indicesClient).getAlias(any(GetAliasRequest.class));
577578
}
578579

580+
@Test
581+
void testListIndicesByPrefix_EmptyPrefixScopesToClusterAlias() throws IOException {
582+
when(indicesClient.getAlias(any(GetAliasRequest.class))).thenReturn(getAliasResponse);
583+
when(getAliasResponse.aliases()).thenReturn(Map.of());
584+
585+
indexManager.listIndicesByPrefix("");
586+
587+
var captor = forClass(GetAliasRequest.class);
588+
verify(indicesClient).getAlias(captor.capture());
589+
assertEquals(
590+
List.of(CLUSTER_ALIAS + IndexMapping.INDEX_NAME_SEPARATOR + "*"),
591+
captor.getValue().index());
592+
}
593+
594+
@Test
595+
void testListIndicesByPrefix_EmptyPrefixWithoutClusterAliasUsesWildcard() throws IOException {
596+
ElasticSearchIndexManager unscopedManager =
597+
new ElasticSearchIndexManager(elasticsearchClient, "");
598+
when(indicesClient.getAlias(any(GetAliasRequest.class))).thenReturn(getAliasResponse);
599+
when(getAliasResponse.aliases()).thenReturn(Map.of());
600+
601+
unscopedManager.listIndicesByPrefix(null);
602+
603+
var captor = forClass(GetAliasRequest.class);
604+
verify(indicesClient).getAlias(captor.capture());
605+
assertEquals(List.of("*"), captor.getValue().index());
606+
}
607+
579608
@Test
580609
void testSwapAliases_ReturnsTrueWhenAliasesAreEmpty() {
581610
assertTrue(indexManager.swapAliases(Set.of("old_index"), "new_index", Set.of()));

‎openmetadata-service/src/test/java/org/openmetadata/service/search/opensearch/OpenSearchIndexManagerTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import static org.junit.jupiter.api.Assertions.assertFalse;
66
import static org.junit.jupiter.api.Assertions.assertNotNull;
77
import static org.junit.jupiter.api.Assertions.assertTrue;
8+
import static org.mockito.ArgumentCaptor.forClass;
89
import static org.mockito.ArgumentMatchers.any;
910
import static org.mockito.Mockito.doReturn;
1011
import static org.mockito.Mockito.lenient;
@@ -581,6 +582,33 @@ void testListIndicesByPrefix_ClientNotAvailable() {
581582
verifyNoInteractions(indicesClient);
582583
}
583584

585+
@Test
586+
void testListIndicesByPrefix_EmptyPrefixScopesToClusterAlias() throws IOException {
587+
when(indicesClient.getAlias(any(GetAliasRequest.class))).thenReturn(getAliasResponse);
588+
when(getAliasResponse.result()).thenReturn(Map.of());
589+
590+
indexManager.listIndicesByPrefix("");
591+
592+
var captor = forClass(GetAliasRequest.class);
593+
verify(indicesClient).getAlias(captor.capture());
594+
assertEquals(
595+
List.of(CLUSTER_ALIAS + IndexMapping.INDEX_NAME_SEPARATOR + "*"),
596+
captor.getValue().index());
597+
}
598+
599+
@Test
600+
void testListIndicesByPrefix_EmptyPrefixWithoutClusterAliasUsesWildcard() throws IOException {
601+
OpenSearchIndexManager unscopedManager = new OpenSearchIndexManager(openSearchClient, "");
602+
when(indicesClient.getAlias(any(GetAliasRequest.class))).thenReturn(getAliasResponse);
603+
when(getAliasResponse.result()).thenReturn(Map.of());
604+
605+
unscopedManager.listIndicesByPrefix(null);
606+
607+
var captor = forClass(GetAliasRequest.class);
608+
verify(indicesClient).getAlias(captor.capture());
609+
assertEquals(List.of("*"), captor.getValue().index());
610+
}
611+
584612
@Test
585613
void testSwapAliases_ReturnsTrueWhenAliasesAreEmpty() {
586614
assertTrue(indexManager.swapAliases(Set.of("old_index"), "new_index", Set.of()));

0 commit comments

Comments
 (0)