Fix NPE in rolling over unknown target and return 404 (elastic#125352)

Since elastic#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.

Author: nielsbauman

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: []

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"}

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;
@@ -151,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(
@@ -193,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);
@@ -655,16 +656,10 @@ static void checkNoDuplicatedAliasInIndexTemplate(
         }
     }
 
-    static void validate(
-        ProjectMetadata project,
-        String rolloverTarget,
-        String newIndexName,
-        CreateIndexRequest request,
-        boolean isFailureStoreRollover
-    ) {
+    static void validate(ProjectMetadata project, String rolloverTarget, String newIndexName, CreateIndexRequest request) {
         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(

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();

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestRolloverIndexAction.java

@@ -44,9 +44,9 @@ public String getName() {
     @Override
     public Set<String> 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");
         }
     }
 

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() {
         ProjectMetadata 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 {
         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 +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(),