From 5c13399ca557901a9c824903c81811842db78e8a Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Thu, 20 Mar 2025 22:20:10 +0100 Subject: [PATCH 1/7] Fix NPE in rolling over unknown target Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. This PR restores that to it's original behavior (which is to return a 400 - illegal argument exception). Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. --- .../rest-api-spec/test/indices.rollover/10_basic.yml | 8 ++++++++ .../admin/indices/rollover/TransportRolloverAction.java | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) 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..5f27b16e620f7 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,11 @@ 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": + - do: + catch: bad_request + 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/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index 432d720cb4e38..a1f41656ddbbb 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 @@ -164,7 +164,7 @@ protected ClusterBlockException checkBlock(RolloverRequest request, ClusterState ResolvedExpression resolvedRolloverTarget = SelectorResolver.parseExpression(request.getRolloverTarget(), request.indicesOptions()); final IndexAbstraction indexAbstraction = projectMetadata.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(); From 91355780d247ef8094fb4d3f63f589506312c4ae Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 21 Mar 2025 08:46:00 +0100 Subject: [PATCH 2/7] Return 404 instead of 400 --- .../resources/rest-api-spec/test/indices.rollover/10_basic.yml | 2 +- .../action/admin/indices/rollover/MetadataRolloverService.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) 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 5f27b16e620f7..a2efc362eee3b 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 @@ -187,7 +187,7 @@ --- "Rolling over an unknown target should return 404": - do: - catch: bad_request + 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 175b13e1b16c7..68ccf77e94084 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; @@ -664,7 +665,7 @@ static void validate( ) { final IndexAbstraction indexAbstraction = project.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( From 24ddc07f6c5d7bbb47e063348280cc5e0286dcf2 Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 21 Mar 2025 08:46:51 +0100 Subject: [PATCH 3/7] Remove unused parameter --- .../rollover/MetadataRolloverService.java | 7 +++---- .../rollover/MetadataRolloverServiceTests.java | 18 +++++++++--------- 2 files changed, 12 insertions(+), 13 deletions(-) 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 68ccf77e94084..f2a2e5032a968 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 @@ -152,7 +152,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( @@ -194,7 +194,7 @@ public static NameResolution resolveRolloverNames( CreateIndexRequest createIndexRequest, boolean isFailureStoreRollover ) { - validate(project, rolloverTarget, newIndexName, createIndexRequest, isFailureStoreRollover); + validate(project, rolloverTarget, newIndexName, createIndexRequest); final IndexAbstraction indexAbstraction = project.getIndicesLookup().get(rolloverTarget); return switch (indexAbstraction.getType()) { case ALIAS -> resolveAliasRolloverNames(project, indexAbstraction, newIndexName); @@ -660,8 +660,7 @@ static void validate( ProjectMetadata project, String rolloverTarget, String newIndexName, - CreateIndexRequest request, - boolean isFailureStoreRollover + CreateIndexRequest request ) { final IndexAbstraction indexAbstraction = project.getIndicesLookup().get(rolloverTarget); if (indexAbstraction == null) { 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 7ec371fff68c9..5e812ee1ddea7 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 @@ -205,21 +205,21 @@ public void testAliasValidation() { IllegalArgumentException 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) + () -> 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 +232,18 @@ public void testDataStreamValidation() throws IOException { ProjectMetadata 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 +254,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 +264,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(), From bf35059d38aae3c9610789532de65d863e1f9154 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Fri, 21 Mar 2025 07:55:14 +0000 Subject: [PATCH 4/7] [CI] Auto commit changes from spotless --- .../admin/indices/rollover/MetadataRolloverService.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) 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 f2a2e5032a968..f4eee1b0ecb65 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 @@ -656,12 +656,7 @@ static void checkNoDuplicatedAliasInIndexTemplate( } } - static void validate( - ProjectMetadata project, - String rolloverTarget, - String newIndexName, - CreateIndexRequest request - ) { + static void validate(ProjectMetadata project, String rolloverTarget, String newIndexName, CreateIndexRequest request) { final IndexAbstraction indexAbstraction = project.getIndicesLookup().get(rolloverTarget); if (indexAbstraction == null) { throw new ResourceNotFoundException("rollover target [" + rolloverTarget + "] does not exist"); From 99e9ada82357270311353c1aefeb0d43d1925d3f Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 21 Mar 2025 09:00:14 +0100 Subject: [PATCH 5/7] Add test capability --- .../rest-api-spec/test/indices.rollover/10_basic.yml | 8 ++++++++ .../action/admin/indices/RestRolloverIndexAction.java | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) 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 a2efc362eee3b..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 @@ -186,6 +186,14 @@ --- "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: 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 25cefb428c4e7..d957ad3c0153d 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 @@ -44,9 +44,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"); } } From 767b8945d8c77e9e3d8699c75d1a61fb9b675668 Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Fri, 21 Mar 2025 09:31:48 +0100 Subject: [PATCH 6/7] Fix test --- .../admin/indices/rollover/MetadataRolloverServiceTests.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 5e812ee1ddea7..26fd4635b7185 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,7 +204,7 @@ public void testAliasValidation() { ProjectMetadata metadata = metadataBuilder.build(); CreateIndexRequest req = new CreateIndexRequest(); - IllegalArgumentException exception = expectThrows( + Exception exception = expectThrows( IllegalArgumentException.class, () -> MetadataRolloverService.validate(metadata, aliasWithNoWriteIndex, randomAlphaOfLength(5), req) ); @@ -215,7 +216,7 @@ public void testAliasValidation() { 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, + ResourceNotFoundException.class, () -> MetadataRolloverService.validate(metadata, aliasName, randomAlphaOfLength(5), req) ); assertThat(exception.getMessage(), equalTo("rollover target [" + aliasName + "] does not exist")); From 08d520f7908f1ac29a1837025b50ac10026864c4 Mon Sep 17 00:00:00 2001 From: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Sat, 22 Mar 2025 10:56:18 +0100 Subject: [PATCH 7/7] Update docs/changelog/125352.yaml --- docs/changelog/125352.yaml | 5 +++++ 1 file changed, 5 insertions(+) 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: []