From da43bfe04b45de6b4bc29fa5085cdf4ef015eead Mon Sep 17 00:00:00 2001 From: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Sat, 22 Mar 2025 11:59:13 +0100 Subject: [PATCH] Fix NPE in rolling over unknown target and return 404 (#125352) Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd453734d205040f8f1d7279961df8b55d23e24) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java --- docs/changelog/125352.yaml | 5 ++++ .../test/indices.rollover/10_basic.yml | 16 +++++++++++++ .../rollover/MetadataRolloverService.java | 15 ++++-------- .../rollover/TransportRolloverAction.java | 2 +- .../indices/RestRolloverIndexAction.java | 4 ++-- .../MetadataRolloverServiceTests.java | 23 ++++++++++--------- 6 files changed, 41 insertions(+), 24 deletions(-) create mode 100644 docs/changelog/125352.yaml diff --git a/docs/changelog/125352.yaml b/docs/changelog/125352.yaml new file mode 100644 index 0000000000000..0895732b0b5bf --- /dev/null +++ b/docs/changelog/125352.yaml @@ -0,0 +1,5 @@ +pr: 125352 +summary: Fix NPE in rolling over unknown target and return 404 +area: Indices APIs +type: bug +issues: [] diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.rollover/10_basic.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.rollover/10_basic.yml index a53365721cf0c..222035174b8bb 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.rollover/10_basic.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.rollover/10_basic.yml @@ -183,3 +183,19 @@ min_age: "0s" min_docs: 1 - match: { error.reason: "Validation Failed: 1: at least one max_* rollover condition must be set when using min_* conditions;" } + +--- +"Rolling over an unknown target should return 404": + - requires: + capabilities: + - method: POST + path: /{index}/_rollover + capabilities: ['return-404-on-missing-target'] + test_runner_features: [capabilities] + reason: Rollover used to return a 400, then it briefly returned a 500 due to an NPE, now it properly returns a 404 + + - do: + catch: missing + indices.rollover: + alias: "non_existent" + - match: {error.reason: "rollover target [non_existent] does not exist"} diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java index 63a5a792db3c9..eecb0ff354ba5 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java @@ -11,6 +11,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.admin.indices.create.CreateIndexClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; import org.elasticsearch.action.datastreams.autosharding.AutoShardingResult; @@ -148,7 +149,7 @@ public RolloverResult rolloverClusterState( @Nullable AutoShardingResult autoShardingResult, boolean isFailureStoreRollover ) throws Exception { - validate(currentState.metadata(), rolloverTarget, newIndexName, createIndexRequest, isFailureStoreRollover); + validate(currentState.metadata(), rolloverTarget, newIndexName, createIndexRequest); final IndexAbstraction indexAbstraction = currentState.metadata().getIndicesLookup().get(rolloverTarget); return switch (indexAbstraction.getType()) { case ALIAS -> rolloverAlias( @@ -190,7 +191,7 @@ public static NameResolution resolveRolloverNames( CreateIndexRequest createIndexRequest, boolean isFailureStoreRollover ) { - validate(currentState.metadata(), rolloverTarget, newIndexName, createIndexRequest, isFailureStoreRollover); + validate(currentState.metadata(), rolloverTarget, newIndexName, createIndexRequest); final IndexAbstraction indexAbstraction = currentState.metadata().getIndicesLookup().get(rolloverTarget); return switch (indexAbstraction.getType()) { case ALIAS -> resolveAliasRolloverNames(currentState.metadata(), indexAbstraction, newIndexName); @@ -627,16 +628,10 @@ static void checkNoDuplicatedAliasInIndexTemplate( } } - static void validate( - Metadata metadata, - String rolloverTarget, - String newIndexName, - CreateIndexRequest request, - boolean isFailureStoreRollover - ) { + static void validate(Metadata metadata, String rolloverTarget, String newIndexName, CreateIndexRequest request) { final IndexAbstraction indexAbstraction = metadata.getIndicesLookup().get(rolloverTarget); if (indexAbstraction == null) { - throw new IllegalArgumentException("rollover target [" + rolloverTarget + "] does not exist"); + throw new ResourceNotFoundException("rollover target [" + rolloverTarget + "] does not exist"); } if (VALID_ROLLOVER_TARGETS.contains(indexAbstraction.getType()) == false) { throw new IllegalArgumentException( diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index e7a3752ca4b32..59feb9b7e0bbb 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -156,7 +156,7 @@ protected ClusterBlockException checkBlock(RolloverRequest request, ClusterState ResolvedExpression resolvedRolloverTarget = SelectorResolver.parseExpression(request.getRolloverTarget(), request.indicesOptions()); final IndexAbstraction indexAbstraction = state.metadata().getIndicesLookup().get(resolvedRolloverTarget.resource()); final String[] indicesToCheck; - if (indexAbstraction.getType().equals(IndexAbstraction.Type.DATA_STREAM)) { + if (indexAbstraction != null && indexAbstraction.getType().equals(IndexAbstraction.Type.DATA_STREAM)) { DataStream dataStream = (DataStream) indexAbstraction; boolean targetFailureStore = resolvedRolloverTarget.selector() != null && resolvedRolloverTarget.selector().shouldIncludeFailures(); diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java index 18f7cc1222d5f..83abc0555b0b8 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java @@ -50,9 +50,9 @@ public String getName() { @Override public Set supportedCapabilities() { if (DataStream.isFailureStoreFeatureFlagEnabled()) { - return Set.of("lazy-rollover-failure-store", "index-expression-selectors"); + return Set.of("return-404-on-missing-target", "lazy-rollover-failure-store", "index-expression-selectors"); } else { - return Set.of(); + return Set.of("return-404-on-missing-target"); } } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java index 57750bb02bb14..729b1734fea22 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.action.admin.indices.rollover; +import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.admin.indices.alias.Alias; import org.elasticsearch.action.admin.indices.create.CreateIndexClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; @@ -203,23 +204,23 @@ public void testAliasValidation() { Metadata metadata = metadataBuilder.build(); CreateIndexRequest req = new CreateIndexRequest(); - IllegalArgumentException exception = expectThrows( + Exception exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, aliasWithNoWriteIndex, randomAlphaOfLength(5), req, false) + () -> MetadataRolloverService.validate(metadata, aliasWithNoWriteIndex, randomAlphaOfLength(5), req) ); assertThat(exception.getMessage(), equalTo("rollover target [" + aliasWithNoWriteIndex + "] does not point to a write index")); exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, randomFrom(index1, index2), randomAlphaOfLength(5), req, false) + () -> MetadataRolloverService.validate(metadata, randomFrom(index1, index2), randomAlphaOfLength(5), req) ); assertThat(exception.getMessage(), equalTo("rollover target is a [concrete index] but one of [alias,data_stream] was expected")); final String aliasName = randomAlphaOfLength(5); exception = expectThrows( - IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, aliasName, randomAlphaOfLength(5), req, false) + ResourceNotFoundException.class, + () -> MetadataRolloverService.validate(metadata, aliasName, randomAlphaOfLength(5), req) ); assertThat(exception.getMessage(), equalTo("rollover target [" + aliasName + "] does not exist")); - MetadataRolloverService.validate(metadata, aliasWithWriteIndex, randomAlphaOfLength(5), req, false); + MetadataRolloverService.validate(metadata, aliasWithWriteIndex, randomAlphaOfLength(5), req); } public void testDataStreamValidation() throws IOException { @@ -232,18 +233,18 @@ public void testDataStreamValidation() throws IOException { Metadata metadata = md.build(); CreateIndexRequest req = new CreateIndexRequest(); - MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, req, false); + MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, req); IllegalArgumentException exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), randomAlphaOfLength(5), req, false) + () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), randomAlphaOfLength(5), req) ); assertThat(exception.getMessage(), equalTo("new index name may not be specified when rolling over a data stream")); CreateIndexRequest aliasReq = new CreateIndexRequest().alias(new Alias("no_aliases_permitted")); exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, aliasReq, false) + () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, aliasReq) ); assertThat( exception.getMessage(), @@ -254,7 +255,7 @@ public void testDataStreamValidation() throws IOException { CreateIndexRequest mappingReq = new CreateIndexRequest().mapping(mapping); exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, mappingReq, false) + () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, mappingReq) ); assertThat( exception.getMessage(), @@ -264,7 +265,7 @@ public void testDataStreamValidation() throws IOException { CreateIndexRequest settingReq = new CreateIndexRequest().settings(Settings.builder().put("foo", "bar")); exception = expectThrows( IllegalArgumentException.class, - () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, settingReq, false) + () -> MetadataRolloverService.validate(metadata, randomDataStream.getName(), null, settingReq) ); assertThat( exception.getMessage(),