Fixing _list/shards API for closed indices

CHANGELOG.md

@@ -56,6 +56,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix `doc_values` only (`index:false`) IP field searching for masks ([#16628](https://github.com/opensearch-project/OpenSearch/pull/16628))
 - Fix stale cluster state custom file deletion ([#16670](https://github.com/opensearch-project/OpenSearch/pull/16670))
 - [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560))
+- Fix _list/shards API failing when closed indices are present in a cluster ([#16606](https://github.com/opensearch-project/OpenSearch/pull/16606))
 
 ### Security
 

server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/shards/TransportCatShardsActionIT.java

@@ -9,8 +9,11 @@
 package org.opensearch.action.admin.cluster.shards;
 
 import org.opensearch.action.admin.indices.stats.IndicesStatsResponse;
+import org.opensearch.action.pagination.PageParams;
+import org.opensearch.client.Requests;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.cluster.routing.ShardRouting;
+import org.opensearch.common.action.ActionFuture;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.core.action.ActionListener;
@@ -20,8 +23,10 @@
 import org.opensearch.test.OpenSearchIntegTestCase;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 
 import static org.opensearch.cluster.routing.UnassignedInfo.INDEX_DELAYED_NODE_LEFT_TIMEOUT_SETTING;
 import static org.opensearch.common.unit.TimeValue.timeValueMillis;
@@ -125,4 +130,101 @@ public void onFailure(Exception e) {
         latch.await();
     }
 
+    public void testListShardsWithClosedAndHiddenIndices() throws InterruptedException, ExecutionException {
+        final int numIndices = 3;
+        final int numShards = 1;
+        final int numReplicas = 2;
+        final int pageSize = numIndices * numReplicas * (numShards + 1);
+        internalCluster().startClusterManagerOnlyNodes(1);
+        internalCluster().startDataOnlyNodes(3);
+        createIndex(
+            "test",
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numReplicas)
+                .build()
+        );
+        createIndex(
+            "test-2",
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numReplicas)
+                .build()
+        );
+        createIndex(
+            "test-closed-idx",
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numReplicas)
+                .build()
+        );
+        createIndex(
+            "test-hidden-idx",
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, numReplicas)
+                .put(IndexMetadata.SETTING_INDEX_HIDDEN, true)
+                .build()
+        );
+        ensureGreen();
+        // close index "test-closed-idx"
+        client().admin().indices().close(Requests.closeIndexRequest("test-closed-idx")).get();
+
+        // Verifying response for default queries: /_list/shards
+        // all the shards should be part of response, however stats should not be displayed for closed index
+        CatShardsRequest listShardsRequest = new CatShardsRequest();
+        listShardsRequest.setCancelAfterTimeInterval(NO_TIMEOUT);
+        listShardsRequest.setIndices(Strings.EMPTY_ARRAY);
+        listShardsRequest.setPageParams(new PageParams(null, PageParams.PARAM_ASC_SORT_VALUE, pageSize));
+        ActionFuture<CatShardsResponse> listShardsResponse = client().execute(CatShardsAction.INSTANCE, listShardsRequest);
+        assertTrue(listShardsResponse.get().getResponseShards().stream().anyMatch(shard -> shard.getIndexName().equals("test-closed-idx")));
+        assertTrue(listShardsResponse.get().getResponseShards().stream().anyMatch(shard -> shard.getIndexName().equals("test-hidden-idx")));
+        assertFalse(
+            Arrays.stream(listShardsResponse.get().getIndicesStatsResponse().getShards())
+                .anyMatch(shardStats -> shardStats.getShardRouting().getIndexName().equals("test-closed-idx"))
+        );
+
+        // Verifying responses when hidden indices are explicitly queried: /_list/shards/test-hidden-idx
+        // Shards for hidden index should appear in response along with stats
+        listShardsRequest.setIndices(List.of("test-hidden-idx").toArray(new String[0]));
+        listShardsResponse = client().execute(CatShardsAction.INSTANCE, listShardsRequest);
+        assertTrue(listShardsResponse.get().getResponseShards().stream().allMatch(shard -> shard.getIndexName().equals("test-hidden-idx")));
+        assertTrue(
+            Arrays.stream(listShardsResponse.get().getIndicesStatsResponse().getShards())
+                .allMatch(shardStats -> shardStats.getShardRouting().getIndexName().equals("test-hidden-idx"))
+        );
+        assertEquals(
+            listShardsResponse.get().getResponseShards().size(),
+            listShardsResponse.get().getIndicesStatsResponse().getShards().length
+        );
+
+        // Verifying responses when hidden indices are queried with wildcards: /_list/shards/test-hidden-idx*
+        // Shards for hidden index should appear in response without stats
+        listShardsRequest.setIndices(List.of("test-hidden-idx*").toArray(new String[0]));
+        listShardsResponse = client().execute(CatShardsAction.INSTANCE, listShardsRequest);
+        assertTrue(listShardsResponse.get().getResponseShards().stream().allMatch(shard -> shard.getIndexName().equals("test-hidden-idx")));
+        assertTrue(
+            Arrays.stream(listShardsResponse.get().getIndicesStatsResponse().getShards())
+                .allMatch(shardStats -> shardStats.getShardRouting().getIndexName().equals("test-hidden-idx"))
+        );
+        assertEquals(
+            listShardsResponse.get().getResponseShards().size(),
+            listShardsResponse.get().getIndicesStatsResponse().getShards().length
+        );
+
+        // Explicitly querying for closed index: /_list/shards/test-closed-idx
+        // should output closed shards without stats.
+        listShardsRequest.setIndices(List.of("test-closed-idx").toArray(new String[0]));
+        listShardsResponse = client().execute(CatShardsAction.INSTANCE, listShardsRequest);
+        assertTrue(listShardsResponse.get().getResponseShards().stream().allMatch(shard -> shard.getIndexName().equals("test-closed-idx")));
+        assertEquals(0, listShardsResponse.get().getIndicesStatsResponse().getShards().length);
+
+        // Querying for closed index with wildcards: /_list/shards/test-closed-idx*
+        // should output closed shards without stats.
+        listShardsRequest.setIndices(List.of("test-closed-idx*").toArray(new String[0]));
+        listShardsResponse = client().execute(CatShardsAction.INSTANCE, listShardsRequest);
+        assertTrue(listShardsResponse.get().getResponseShards().stream().allMatch(shard -> shard.getIndexName().equals("test-closed-idx")));
+        assertEquals(0, listShardsResponse.get().getIndicesStatsResponse().getShards().length);
+    }
+
 }

server/src/main/java/org/opensearch/action/admin/cluster/shards/TransportCatShardsAction.java

@@ -16,18 +16,24 @@
 import org.opensearch.action.pagination.ShardPaginationStrategy;
 import org.opensearch.action.support.ActionFilters;
 import org.opensearch.action.support.HandledTransportAction;
+import org.opensearch.action.support.IndicesOptions;
 import org.opensearch.action.support.TimeoutTaskCancellationUtility;
 import org.opensearch.client.node.NodeClient;
+import org.opensearch.cluster.ClusterState;
+import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.breaker.ResponseLimitBreachedException;
 import org.opensearch.common.breaker.ResponseLimitSettings;
 import org.opensearch.common.inject.Inject;
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.core.action.NotifyOnceListener;
+import org.opensearch.core.common.Strings;
 import org.opensearch.tasks.CancellableTask;
 import org.opensearch.tasks.Task;
 import org.opensearch.transport.TransportService;
 
+import java.util.List;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 import static org.opensearch.common.breaker.ResponseLimitSettings.LimitEntity.SHARDS;
 
@@ -63,6 +69,7 @@
             clusterStateRequest.clear().nodes(true).routingTable(true).indices(shardsRequest.getIndices());
         } else {
             clusterStateRequest.clear().nodes(true).routingTable(true).indices(shardsRequest.getIndices()).metadata(true);
+            clusterStateRequest.indicesOptions(IndicesOptions.lenientExpandHidden());
         }
         assert parentTask instanceof CancellableTask;
         clusterStateRequest.setParentTask(client.getLocalNodeId(), parentTask.getId());
@@ -98,18 +105,21 @@
                             shardsRequest.getPageParams(),
                             clusterStateResponse
                         );
-                        String[] indices = Objects.isNull(paginationStrategy)
-                            ? shardsRequest.getIndices()
-                            : paginationStrategy.getRequestedIndices().toArray(new String[0]);
                         catShardsResponse.setNodes(clusterStateResponse.getState().getNodes());
                         catShardsResponse.setResponseShards(
                             Objects.isNull(paginationStrategy)
                                 ? clusterStateResponse.getState().routingTable().allShards()
                                 : paginationStrategy.getRequestedEntities()
                         );
                         catShardsResponse.setPageToken(Objects.isNull(paginationStrategy) ? null : paginationStrategy.getResponseToken());
+
+                        String[] indices = Objects.isNull(paginationStrategy)
+                            ? shardsRequest.getIndices()
+                            : filterPaginationResponse(clusterStateResponse.getState(), paginationStrategy.getRequestedIndices()).toArray(
+                                Strings.EMPTY_ARRAY
+                            );
                         // For paginated queries, if strategy outputs no shards to be returned, avoid fetching IndicesStats.
-                        if (shouldSkipIndicesStatsRequest(paginationStrategy)) {
+                        if (shouldSkipIndicesStatsRequest(paginationStrategy, indices)) {
                             catShardsResponse.setIndicesStatsResponse(IndicesStatsResponse.getEmptyResponse());
                             cancellableListener.onResponse(catShardsResponse);
                             return;
@@ -118,6 +128,11 @@
                         indicesStatsRequest.setShouldCancelOnTimeout(true);
                         indicesStatsRequest.all();
                         indicesStatsRequest.indices(indices);
+                        // Since the indices for paginated query are already concrete and have been filtered to
+                        // only consider OPEN indices, invoking IndexNameExpressionResolver should be avoided.
+                        if (paginationStrategy != null) {
+                            indicesStatsRequest.skipIndexNameResolver(true);
+                        }
                         indicesStatsRequest.setParentTask(client.getLocalNodeId(), parentTask.getId());
                         client.admin().indices().stats(indicesStatsRequest, new ActionListener<IndicesStatsResponse>() {
                             @Override
@@ -166,7 +181,19 @@
         }
     }
 
-    private boolean shouldSkipIndicesStatsRequest(ShardPaginationStrategy paginationStrategy) {
-        return Objects.nonNull(paginationStrategy) && paginationStrategy.getRequestedEntities().isEmpty();
+    private boolean shouldSkipIndicesStatsRequest(ShardPaginationStrategy paginationStrategy, String[] indices) {
+        return Objects.nonNull(paginationStrategy) && (indices == null || indices.length == 0);
+    }
+
+    /**
+     * Will be used by paginated query (_list/shards) to filter out closed indices (only consider OPEN) before fetching
+     * IndicesStats. Since pagination strategy always passes concrete indices to TransportIndicesStatsAction,
+     * the default behaviour of StrictExpandOpenAndForbidClosed leads to errors if closed indices are encountered.
+     */
+    private List<String> filterPaginationResponse(ClusterState clusterState, List<String> strategyIndices) {
+        return strategyIndices.stream().filter(index -> {
+            IndexMetadata metadata = clusterState.metadata().indices().get(index);
+            return metadata != null && metadata.getState().equals(IndexMetadata.State.OPEN);
+        }).collect(Collectors.toList());
     }
 }

server/src/main/java/org/opensearch/action/admin/indices/stats/IndicesStatsRequest.java

@@ -57,6 +57,7 @@
 public class IndicesStatsRequest extends BroadcastRequest<IndicesStatsRequest> {
 
     private CommonStatsFlags flags = new CommonStatsFlags();
+    private boolean skipIndexNameResolver = false;
 
     public IndicesStatsRequest() {
         super((String[]) null);
@@ -307,4 +308,13 @@
     public boolean includeDataStreams() {
         return true;
     }
+
+    public boolean skipIndexNameResolver() {
+        return skipIndexNameResolver;
+    }
+
+    public IndicesStatsRequest skipIndexNameResolver(boolean skipIndexNameResolver) {
+        this.skipIndexNameResolver = skipIndexNameResolver;
+        return this;
+    }
 }

server/src/main/java/org/opensearch/action/admin/indices/stats/TransportIndicesStatsAction.java

@@ -153,4 +153,12 @@
         }
         return new ShardStats(indexShard.routingEntry(), indexShard.shardPath(), commonStats, commitStats, seqNoStats, retentionLeaseStats);
     }
+
+    @Override
+    protected String[] resolveConcreteIndexNames(ClusterState clusterState, IndicesStatsRequest request) {
+        if (request.skipIndexNameResolver()) {
+            return request.indices();
+        }
+        return super.resolveConcreteIndexNames(clusterState, request);
+    }
 }