Skip to content

Commit e498ad1

Browse files
joegallojoegallo
authored
Fix geoip databases index access after system feature migration (again) (elastic#122938) (elastic#123161)
1 parent 1a76203 commit e498ad1

File tree

4 files changed

+118
-21
lines changed

4 files changed

+118
-21
lines changed

‎docs/changelog/122938.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 122938
2+
summary: Fix geoip databases index access after system feature migration (again)
3+
area: Ingest Node
4+
type: bug
5+
issues: []

‎modules/ingest-geoip/qa/full-cluster-restart/src/javaRestTest/java/org/elasticsearch/ingest/geoip/FullClusterRestartIT.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,19 @@ public void testGeoIpSystemFeaturesMigration() throws Exception {
121121

122122
// as should a normal get *
123123
assertBusy(() -> testGetStar(List.of("my-index-00001"), List.of()));
124+
125+
// and getting data streams
126+
assertBusy(() -> testGetDatastreams());
124127
} else {
125128
// after the upgrade, but before the migration, Kibana should work
126129
assertBusy(() -> testGetStarAsKibana(List.of("my-index-00001"), maybeSecurityIndex));
127130

128131
// as should a normal get *
129132
assertBusy(() -> testGetStar(List.of("my-index-00001"), maybeSecurityIndex));
130133

134+
// and getting data streams
135+
assertBusy(() -> testGetDatastreams());
136+
131137
// migrate the system features and give the cluster a moment to settle
132138
Request migrateSystemFeatures = new Request("POST", "/_migration/system_features");
133139
assertOK(client().performRequest(migrateSystemFeatures));
@@ -142,6 +148,9 @@ public void testGeoIpSystemFeaturesMigration() throws Exception {
142148
// as should a normal get *
143149
assertBusy(() -> testGetStar(List.of("my-index-00001"), maybeSecurityIndex));
144150

151+
// and getting data streams
152+
assertBusy(() -> testGetDatastreams());
153+
145154
Request disableDownloader = new Request("PUT", "/_cluster/settings");
146155
disableDownloader.setJsonEntity("""
147156
{"persistent": {"ingest.geoip.downloader.enabled": false}}
@@ -255,4 +264,15 @@ private void testGetStarAsKibana(List<String> indexNames, @Nullable List<String>
255264
Map<String, Object> map = responseAsMap(response);
256265
assertThat(map.keySet(), is(new HashSet<>(indexNames)));
257266
}
267+
268+
private void testGetDatastreams() throws IOException {
269+
Request getStar = new Request("GET", "_data_stream");
270+
getStar.setOptions(
271+
RequestOptions.DEFAULT.toBuilder().setWarningsHandler(WarningsHandler.PERMISSIVE) // we don't care about warnings, just errors
272+
);
273+
Response response = client().performRequest(getStar);
274+
assertOK(response);
275+
276+
// note: we don't actually care about the response, just that there was one and that it didn't error out on us
277+
}
258278
}

‎server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.elasticsearch.common.regex.Regex;
1515
import org.elasticsearch.core.Nullable;
1616
import org.elasticsearch.core.Tuple;
17+
import org.elasticsearch.index.Index;
1718
import org.elasticsearch.index.IndexNotFoundException;
1819
import org.elasticsearch.indices.SystemIndices.SystemIndexAccessLevel;
1920

@@ -158,7 +159,26 @@ public static boolean isIndexVisible(
158159
if (indexAbstraction.isSystem()) {
159160
// check if it is net new
160161
if (resolver.getNetNewSystemIndexPredicate().test(indexAbstraction.getName())) {
161-
return isSystemIndexVisible(resolver, indexAbstraction);
162+
// don't give this code any particular credit for being *correct*. it's just trying to resolve a combination of
163+
// issues in a way that happens to *work*. there's probably a better way of writing things such that this won't
164+
// be necessary, but for the moment, it happens to be expedient to write things this way.
165+
166+
// unwrap the alias and re-run the function on the write index of the alias -- that is, the alias is visible if
167+
// the concrete index that it refers to is visible
168+
Index writeIndex = indexAbstraction.getWriteIndex();
169+
if (writeIndex == null) {
170+
return false;
171+
} else {
172+
return isIndexVisible(
173+
expression,
174+
selectorString,
175+
writeIndex.getName(),
176+
indicesOptions,
177+
metadata,
178+
resolver,
179+
includeDataStreams
180+
);
181+
}
162182
}
163183
}
164184

‎server/src/test/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolverTests.java‎

Lines changed: 72 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.util.function.Supplier;
3030

3131
import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
32+
import static org.elasticsearch.indices.SystemIndices.EXTERNAL_SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY;
3233
import static org.elasticsearch.indices.SystemIndices.SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY;
3334
import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
3435
import static org.hamcrest.Matchers.contains;
@@ -218,18 +219,6 @@ public void testIsIndexVisible() {
218219
assertThat(isIndexVisible("data-stream1", "failures"), is(true));
219220
}
220221

221-
private boolean isIndexVisible(String index, String selector) {
222-
return IndexAbstractionResolver.isIndexVisible(
223-
"*",
224-
selector,
225-
index,
226-
IndicesOptions.strictExpandHidden(),
227-
metadata,
228-
indexNameExpressionResolver,
229-
true
230-
);
231-
}
232-
233222
public void testIsNetNewSystemIndexVisible() {
234223
final Settings settings = Settings.builder()
235224
.put("index.number_of_replicas", 0)
@@ -271,16 +260,71 @@ public void testIsNetNewSystemIndexVisible() {
271260
List.of(new SystemIndices.Feature("name", "description", List.of(fooDescriptor, barDescriptor)))
272261
);
273262

274-
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
275-
threadContext.putHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, "false");
276-
indexNameExpressionResolver = new IndexNameExpressionResolver(threadContext, systemIndices);
277-
indexAbstractionResolver = new IndexAbstractionResolver(indexNameExpressionResolver);
278-
279263
metadata = Metadata.builder().put(foo, true).put(barReindexed, true).put(other, true).build();
280264

281-
assertThat(isIndexVisible("other", "*"), is(true));
282-
assertThat(isIndexVisible(".foo", "*"), is(false));
283-
assertThat(isIndexVisible(".bar", "*"), is(false));
265+
// these indices options are for the GET _data_streams case
266+
final IndicesOptions noHiddenNoAliases = IndicesOptions.builder()
267+
.wildcardOptions(
268+
IndicesOptions.WildcardOptions.builder()
269+
.matchOpen(true)
270+
.matchClosed(true)
271+
.includeHidden(false)
272+
.resolveAliases(false)
273+
.build()
274+
)
275+
.build();
276+
277+
{
278+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
279+
threadContext.putHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, "true");
280+
indexNameExpressionResolver = new IndexNameExpressionResolver(threadContext, systemIndices);
281+
indexAbstractionResolver = new IndexAbstractionResolver(indexNameExpressionResolver);
282+
283+
// this covers the GET * case -- with system access, you can see everything
284+
assertThat(isIndexVisible("other", "*"), is(true));
285+
assertThat(isIndexVisible(".foo", "*"), is(true));
286+
assertThat(isIndexVisible(".bar", "*"), is(true));
287+
288+
// but if you don't ask for hidden and aliases, you won't see hidden indices or aliases, naturally
289+
assertThat(isIndexVisible("other", "*", noHiddenNoAliases), is(true));
290+
assertThat(isIndexVisible(".foo", "*", noHiddenNoAliases), is(false));
291+
assertThat(isIndexVisible(".bar", "*", noHiddenNoAliases), is(false));
292+
}
293+
294+
{
295+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
296+
threadContext.putHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, "false");
297+
indexNameExpressionResolver = new IndexNameExpressionResolver(threadContext, systemIndices);
298+
indexAbstractionResolver = new IndexAbstractionResolver(indexNameExpressionResolver);
299+
300+
// this covers the GET * case -- without system access, you can't see everything
301+
assertThat(isIndexVisible("other", "*"), is(true));
302+
assertThat(isIndexVisible(".foo", "*"), is(false));
303+
assertThat(isIndexVisible(".bar", "*"), is(false));
304+
305+
// no difference here in the datastream case, you can't see these then, either
306+
assertThat(isIndexVisible("other", "*", noHiddenNoAliases), is(true));
307+
assertThat(isIndexVisible(".foo", "*", noHiddenNoAliases), is(false));
308+
assertThat(isIndexVisible(".bar", "*", noHiddenNoAliases), is(false));
309+
}
310+
311+
{
312+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
313+
threadContext.putHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, "true");
314+
threadContext.putHeader(EXTERNAL_SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, "some-elastic-product");
315+
indexNameExpressionResolver = new IndexNameExpressionResolver(threadContext, systemIndices);
316+
indexAbstractionResolver = new IndexAbstractionResolver(indexNameExpressionResolver);
317+
318+
// this covers the GET * case -- with product (only) access, you can't see everything
319+
assertThat(isIndexVisible("other", "*"), is(true));
320+
assertThat(isIndexVisible(".foo", "*"), is(false));
321+
assertThat(isIndexVisible(".bar", "*"), is(false));
322+
323+
// no difference here in the datastream case, you can't see these then, either
324+
assertThat(isIndexVisible("other", "*", noHiddenNoAliases), is(true));
325+
assertThat(isIndexVisible(".foo", "*", noHiddenNoAliases), is(false));
326+
assertThat(isIndexVisible(".bar", "*", noHiddenNoAliases), is(false));
327+
}
284328
}
285329

286330
private static XContentBuilder mappings() {
@@ -308,4 +352,12 @@ private List<String> resolveAbstractionsSelectorAllowed(List<String> expressions
308352
private List<String> resolveAbstractions(List<String> expressions, IndicesOptions indicesOptions, Supplier<Set<String>> mask) {
309353
return indexAbstractionResolver.resolveIndexAbstractions(expressions, indicesOptions, metadata, mask, (idx) -> true, true);
310354
}
355+
356+
private boolean isIndexVisible(String index, String selector) {
357+
return isIndexVisible(index, selector, IndicesOptions.strictExpandHidden());
358+
}
359+
360+
private boolean isIndexVisible(String index, String selector, IndicesOptions indicesOptions) {
361+
return IndexAbstractionResolver.isIndexVisible("*", selector, index, indicesOptions, metadata, indexNameExpressionResolver, true);
362+
}
311363
}

0 commit comments

Comments
 (0)