Make feature reset API response more informative (#71240)

Previously, the ResetFeatureStateStatus object captured its status in a
String, which meant that if we wanted to know if something succeeded or
failed, we'd have to parse information out of the string. This isn't a
good way of doing things.

I've introduced a SUCCESS/FAILURE enum for status constants, and added a
check for failures in the transport action. We return a 207 if some but not all
reset actions fail, and for every failure, we also return information about the
exception or error that caused it.

Co-authored-by: Jay Modi <[email protected]>

Author: williamrandolph

client/rest-high-level/src/main/java/org/elasticsearch/client/feature/ResetFeaturesResponse.java

@@ -8,21 +8,30 @@
 
 package org.elasticsearch.client.feature;
 
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.XContentParser;
 
 import java.util.List;
+import java.util.Objects;
 
+/**
+ * This class represents the response of the Feature State Reset API. It is a
+ * list containing the response of every feature whose state can be reset. The
+ * response from each feature will indicate success or failure. In the case of a
+ * failure, the cause will be returned as well.
+ */
 public class ResetFeaturesResponse {
     private final List<ResetFeatureStateStatus> features;
 
     private static final ParseField FEATURES = new ParseField("features");
 
     @SuppressWarnings("unchecked")
     private static final ConstructingObjectParser<ResetFeaturesResponse, Void> PARSER = new ConstructingObjectParser<>(
-        "snapshottable_features_response", true,
+        "features_reset_status_response", true,
         (a, ctx) -> new ResetFeaturesResponse((List<ResetFeatureStateStatus>) a[0])
     );
 
@@ -32,51 +41,93 @@ public class ResetFeaturesResponse {
             ResetFeaturesResponse.ResetFeatureStateStatus::parse, FEATURES);
     }
 
+    /**
+     * Create a new ResetFeaturesResponse
+     * @param features A full list of status responses from individual feature reset operations.
+     */
     public ResetFeaturesResponse(List<ResetFeatureStateStatus> features) {
         this.features = features;
     }
 
-    public List<ResetFeatureStateStatus> getFeatures() {
+    /**
+     * @return List containing a reset status for each feature that we have tried to reset.
+     */
+    public List<ResetFeatureStateStatus> getFeatureResetStatuses() {
         return features;
     }
 
     public static ResetFeaturesResponse parse(XContentParser parser) {
         return PARSER.apply(parser, null);
     }
 
+    /**
+     * A class representing the status of an attempt to reset a feature's state.
+     * The attempt to reset either succeeds and we return the name of the
+     * feature and a success flag; or it fails and we return the name of the feature,
+     * a status flag, and the exception thrown during the attempt to reset the feature.
+     */
     public static class ResetFeatureStateStatus {
         private final String featureName;
         private final String status;
+        private final Exception exception;
 
         private static final ParseField FEATURE_NAME = new ParseField("feature_name");
         private static final ParseField STATUS = new ParseField("status");
+        private static final ParseField EXCEPTION = new ParseField("exception");
 
-        private static final ConstructingObjectParser<ResetFeatureStateStatus, Void> PARSER =  new ConstructingObjectParser<>(
-            "features", true, (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1])
+        private static final ConstructingObjectParser<ResetFeatureStateStatus, Void> PARSER = new ConstructingObjectParser<>(
+            "feature_state_reset_stats", true,
+            (a, ctx) -> new ResetFeatureStateStatus((String) a[0], (String) a[1], (ElasticsearchException) a[2])
         );
 
         static {
             PARSER.declareField(ConstructingObjectParser.constructorArg(),
                 (p, c) -> p.text(), FEATURE_NAME, ObjectParser.ValueType.STRING);
             PARSER.declareField(ConstructingObjectParser.constructorArg(),
                 (p, c) -> p.text(), STATUS, ObjectParser.ValueType.STRING);
+            PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(),
+                (p, c) -> ElasticsearchException.fromXContent(p), EXCEPTION);
         }
 
-        ResetFeatureStateStatus(String featureName, String status) {
+        /**
+         * Create a ResetFeatureStateStatus.
+         * @param featureName Name of the feature whose status has been reset.
+         * @param status Whether the reset attempt succeeded or failed.
+         * @param exception If the reset attempt failed, the exception that caused the
+         *                  failure. Must be null when status is "SUCCESS".
+         */
+        ResetFeatureStateStatus(String featureName, String status, @Nullable Exception exception) {
             this.featureName = featureName;
+            assert "SUCCESS".equals(status) || "FAILURE".equals(status);
             this.status = status;
+            assert "FAILURE".equals(status) ? Objects.nonNull(exception) : Objects.isNull(exception);
+            this.exception = exception;
         }
 
         public static ResetFeatureStateStatus parse(XContentParser parser, Void ctx) {
             return PARSER.apply(parser, ctx);
         }
 
+        /**
+         * @return Name of the feature that we tried to reset
+         */
         public String getFeatureName() {
             return featureName;
         }
 
+        /**
+         * @return "SUCCESS" if the reset attempt succeeded, "FAILURE" otherwise.
+         */
         public String getStatus() {
             return status;
         }
+
+        /**
+         * @return The exception that caused the reset attempt to fail.
+         */
+        @Nullable
+        public Exception getException() {
+            return exception;
+        }
     }
 }

client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java

@@ -12,9 +12,15 @@
 import org.elasticsearch.client.feature.GetFeaturesResponse;
 import org.elasticsearch.client.feature.ResetFeaturesRequest;
 import org.elasticsearch.client.feature.ResetFeaturesResponse;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.search.SearchModule;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.stream.Collectors;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.notNullValue;
 
@@ -34,13 +40,25 @@ public void testGetFeatures() throws IOException {
     public void testResetFeatures() throws IOException {
         ResetFeaturesRequest request = new ResetFeaturesRequest();
 
+        // need superuser privileges to execute the reset
+        RestHighLevelClient adminHighLevelClient = new RestHighLevelClient(
+            adminClient(),
+            (client) -> {},
+            new SearchModule(Settings.EMPTY, Collections.emptyList()).getNamedXContents());
         ResetFeaturesResponse response = execute(request,
-            highLevelClient().features()::resetFeatures, highLevelClient().features()::resetFeaturesAsync);
+            adminHighLevelClient.features()::resetFeatures,
+            adminHighLevelClient.features()::resetFeaturesAsync);
 
         assertThat(response, notNullValue());
-        assertThat(response.getFeatures(), notNullValue());
-        assertThat(response.getFeatures().size(), greaterThan(1));
-        assertTrue(response.getFeatures().stream().anyMatch(
+        assertThat(response.getFeatureResetStatuses(), notNullValue());
+        assertThat(response.getFeatureResetStatuses().size(), greaterThan(1));
+        assertTrue(response.getFeatureResetStatuses().stream().anyMatch(
             feature -> "tasks".equals(feature.getFeatureName()) && "SUCCESS".equals(feature.getStatus())));
+
+        Set<String> statuses = response.getFeatureResetStatuses().stream()
+            .map(ResetFeaturesResponse.ResetFeatureStateStatus::getStatus)
+            .collect(Collectors.toSet());
+
+        assertThat(statuses, contains("SUCCESS"));
     }
 }

client/rest-high-level/src/test/java/org/elasticsearch/client/snapshots/ResetFeaturesResponseTests.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.client.snapshots;
+
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse;
+import org.elasticsearch.client.AbstractResponseTestCase;
+import org.elasticsearch.client.feature.ResetFeaturesResponse;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.Matchers.everyItem;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.in;
+import static org.hamcrest.Matchers.is;
+
+public class ResetFeaturesResponseTests extends AbstractResponseTestCase<ResetFeatureStateResponse, ResetFeaturesResponse> {
+
+    @Override
+    protected ResetFeatureStateResponse createServerTestInstance(
+        XContentType xContentType) {
+        return new org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse(
+            randomList(
+                10,
+                () -> randomBoolean()
+                    ? ResetFeatureStateResponse.ResetFeatureStateStatus.success(randomAlphaOfLengthBetween(6, 10))
+                    : ResetFeatureStateResponse.ResetFeatureStateStatus.failure(
+                        randomAlphaOfLengthBetween(6, 10), new ElasticsearchException("something went wrong"))
+            )
+        );
+    }
+
+    @Override
+    protected ResetFeaturesResponse doParseToClientInstance(XContentParser parser) throws IOException {
+        return ResetFeaturesResponse.parse(parser);
+    }
+
+    @Override
+    protected void assertInstances(ResetFeatureStateResponse serverTestInstance, ResetFeaturesResponse clientInstance) {
+
+        assertNotNull(serverTestInstance.getFeatureStateResetStatuses());
+        assertNotNull(clientInstance.getFeatureResetStatuses());
+
+        assertThat(clientInstance.getFeatureResetStatuses(), hasSize(serverTestInstance.getFeatureStateResetStatuses().size()));
+
+        Map<String, String> clientFeatures = clientInstance.getFeatureResetStatuses()
+            .stream()
+            .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus()));
+        Map<String, String> serverFeatures = serverTestInstance.getFeatureStateResetStatuses()
+            .stream()
+            .collect(Collectors.toMap(f -> f.getFeatureName(), f -> f.getStatus().toString()));
+
+        assertThat(clientFeatures.entrySet(), everyItem(is(in(serverFeatures.entrySet()))));
+    }
+}

docs/reference/features/apis/reset-features-api.asciidoc

@@ -8,7 +8,7 @@ experimental::[]
 
 Clears all of the the state information stored in system indices by {es} features, including the security and machine learning indices.
 
-WARNING: Intended for development and testing use only. Do not reset features on a production cluster.  
+WARNING: Intended for development and testing use only. Do not reset features on a production cluster.
 
 [source,console]
 -----------------------------------
@@ -26,9 +26,11 @@ POST /_features/_reset
 
 Return a cluster to the same state as a new installation by resetting the feature state for all {es} features. This deletes all state information stored in system indices.
 
-Note that select features might provide a way to reset particular system indices. Using this API resets _all_ features, both those that are built-in and implemented as plugins. 
+The response code is `HTTP 200` if state is successfully reset for all features, `HTTP 207` if there is a mixture of successes and failures, and `HTTP 500` if the reset operation fails for all features.
 
-To list the features that will be affected, use the <<get-features-api,get features API>>. 
+Note that select features might provide a way to reset particular system indices. Using this API resets _all_ features, both those that are built-in and implemented as plugins.
+
+To list the features that will be affected, use the <<get-features-api,get features API>>.
 
 IMPORTANT: The features installed on the node you submit this request to are the features that will be reset. Run on the master node if you have any doubts about which plugins are installed on individual nodes.
 

server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java

@@ -8,10 +8,14 @@
 
 package org.elasticsearch.snapshots;
 
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse;
 import org.elasticsearch.action.admin.indices.get.GetIndexResponse;
+import org.elasticsearch.client.Client;
+import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.indices.SystemIndexDescriptor;
@@ -23,10 +27,13 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import static org.hamcrest.Matchers.arrayContaining;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.notNullValue;
 
 public class FeatureStateResetApiIT extends ESIntegTestCase {
 
@@ -35,6 +42,7 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
         List<Class<? extends Plugin>> plugins = new ArrayList<>(super.nodePlugins());
         plugins.add(SystemIndexTestPlugin.class);
         plugins.add(SecondSystemIndexTestPlugin.class);
+        plugins.add(EvilSystemIndexTestPlugin.class);
         return plugins;
     }
 
@@ -62,10 +70,11 @@ public void testResetSystemIndices() throws Exception {
 
         // call the reset API
         ResetFeatureStateResponse apiResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get();
-        assertThat(apiResponse.getItemList(), containsInAnyOrder(
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("SystemIndexTestPlugin", "SUCCESS"),
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("SecondSystemIndexTestPlugin", "SUCCESS"),
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("tasks", "SUCCESS")
+        assertThat(apiResponse.getFeatureStateResetStatuses(), containsInAnyOrder(
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("SystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("SecondSystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("EvilSystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("tasks")
         ));
 
         // verify that both indices are gone
@@ -94,6 +103,31 @@ public void testResetSystemIndices() throws Exception {
         assertThat(response.getIndices(), arrayContaining("my_index"));
     }
 
+    /**
+     * Evil test - test that when a feature fails to reset, we get a response object
+     * indicating the failure
+     */
+    public void testFeatureResetFailure() throws Exception {
+        try {
+            EvilSystemIndexTestPlugin.setBeEvil(true);
+            ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE,
+                new ResetFeatureStateRequest()).get();
+
+            List<String> failedFeatures = resetFeatureStateResponse.getFeatureStateResetStatuses().stream()
+                .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE)
+                .peek(status -> assertThat(status.getException(), notNullValue()))
+                .map(status -> {
+                    // all failed statuses should have exceptions
+                    assertThat(status.getException(), notNullValue());
+                    return status.getFeatureName();
+                })
+                .collect(Collectors.toList());
+            assertThat(failedFeatures, contains("EvilSystemIndexTestPlugin"));
+        } finally {
+            EvilSystemIndexTestPlugin.setBeEvil(false);
+        }
+    }
+
     /**
      * A test plugin with patterns for system indices and associated indices.
      */
@@ -145,4 +179,43 @@ public String getFeatureDescription() {
             return "A second test plugin";
         }
     }
+
+    /**
+     * An evil test plugin to test failure cases.
+     */
+    public static class EvilSystemIndexTestPlugin extends Plugin implements SystemIndexPlugin {
+
+        private static boolean beEvil = false;
+
+        @Override
+        public String getFeatureName() {
+            return "EvilSystemIndexTestPlugin";
+        }
+
+        @Override
+        public String getFeatureDescription() {
+            return "a plugin that can be very bad";
+        }
+
+        public static synchronized void setBeEvil(boolean evil) {
+            beEvil = evil;
+        }
+
+        public static synchronized boolean isEvil() {
+            return beEvil;
+        }
+
+        @Override
+        public void cleanUpFeature(
+            ClusterService clusterService,
+            Client client,
+            ActionListener<ResetFeatureStateResponse.ResetFeatureStateStatus> listener) {
+            if (isEvil()) {
+                listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(),
+                    new ElasticsearchException("problem!")));
+            } else {
+                listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName()));
+            }
+        }
+    }
 }