better handling for clients with incorrect configured secrets

Author: ywangd

modules/repository-gcs/src/main/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientsManager.java

@@ -14,11 +14,11 @@
 import org.elasticsearch.cluster.metadata.ProjectId;
 import org.elasticsearch.cluster.metadata.ProjectMetadata;
 import org.elasticsearch.common.CheckedBiFunction;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.settings.ProjectSecrets;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
+import org.elasticsearch.common.util.set.Sets;
 import org.elasticsearch.logging.LogManager;
 import org.elasticsearch.logging.Logger;
 
@@ -89,20 +89,25 @@ public void applyClusterState(ClusterChangedEvent event) {
                 .put(nodeGcsSettings)
                 .setSecureSettings(projectSecrets.getSettings())
                 .build();
-            final Map<String, GoogleCloudStorageClientSettings> clientSettings = GoogleCloudStorageClientSettings.load(currentSettings)
-                .entrySet()
+
+            final var allClientSettings = GoogleCloudStorageClientSettings.load(currentSettings);
+            assert allClientSettings.isEmpty() == false;
+            // Skip project clients that have no credentials configured. This should not happen in serverless.
+            // But it is safer to skip them and is also a more consistent behaviour with the cases when
+            // project secrets are not present.
+            final var clientSettings = allClientSettings.entrySet()
                 .stream()
-                // Skip project clients that have no credentials configured. This should not happen in serverless.
-                // But it is safer to skip them and is also a more consistent behaviour with the cases when
-                // project secrets are not present.
                 .filter(entry -> entry.getValue().getCredential() != null)
                 .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue));
 
-            if (clientSettings.isEmpty()) {
-                // clientSettings should not be empty, i.e. there should be at least one client configured.
-                // But if it does somehow happen, log a warning and continue. The project will not have usable client but that is ok.
-                logger.warn("Skipping project [{}] with no client settings", project.id());
-                continue;
+            if (allClientSettings.size() != clientSettings.size()) {
+                logger.warn(
+                    "Project [{}] has [{}] GCS client settings, but [{}] is usable due to missing credentials for clients {}",
+                    project.id(),
+                    allClientSettings.size(),
+                    clientSettings.size(),
+                    Sets.difference(allClientSettings.keySet(), clientSettings.keySet())
+                );
             }
 
             // TODO: If performance is an issue, we may consider comparing just the relevant project secrets for new or updated clients
@@ -221,10 +226,7 @@ MeteredStorage client(final String clientName, final String repositoryName, fina
 
                 if (settings == null) {
                     throw new IllegalArgumentException(
-                        "Unknown client name ["
-                            + clientName
-                            + "]. Existing client configs: "
-                            + Strings.collectionToDelimitedString(allClientSettings().keySet(), ",")
+                        "Unknown client name [" + clientName + "]. Existing client configs: " + allClientSettings().keySet()
                     );
                 }
 

modules/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageClientsManagerTests.java

@@ -37,7 +37,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 import java.util.stream.IntStream;
+import java.util.stream.Stream;
 
 import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.CREDENTIALS_FILE_SETTING;
 import static org.hamcrest.Matchers.anEmptyMap;
@@ -69,7 +71,8 @@ public void setUp() throws Exception {
         super.setUp();
         privateKeyIdGenerators = ConcurrentCollections.newConcurrentMap();
         statsCollector = new GcsRepositoryStatsCollector();
-        clientNames = IntStream.range(0, between(2, 5)).mapToObj(i -> randomIdentifier() + "_" + i).toList();
+        clientNames = Stream.concat(Stream.of("default"), IntStream.range(0, between(1, 4)).mapToObj(i -> randomIdentifier() + "_" + i))
+            .toList();
 
         final Settings.Builder builder = Settings.builder();
         final var mockSecureSettings = new MockSecureSettings();
@@ -182,6 +185,32 @@ public void testClientsLifeCycleForSingleProject() throws Exception {
         assertThat(gcsClientsManager.getPerProjectClientsHolders(), not(hasKey(projectId)));
     }
 
+    public void testClientsWithNoCredentialsAreFilteredOut() throws IOException {
+        final ProjectId projectId = randomUniqueProjectId();
+        updateProjectInClusterState(projectId, newProjectClientsSecrets(projectId, clientNames.toArray(String[]::new)));
+        for (var clientName : clientNames) {
+            assertNotNull(getClientFromManager(projectId, clientName));
+        }
+
+        final List<String> clientsWithIncorrectSecretsConfig = randomNonEmptySubsetOf(clientNames);
+
+        updateProjectInClusterState(projectId, clientNames.stream().collect(Collectors.toUnmodifiableMap(clientName -> {
+            if (clientsWithIncorrectSecretsConfig.contains(clientName)) {
+                return "gcs.client." + clientName + ".some_non_existing_setting";
+            } else {
+                return CREDENTIALS_FILE_SETTING.getConcreteSettingForNamespace(clientName).getKey();
+            }
+        }, clientName -> TestUtils.createServiceAccount(random(), projectClientPrivateKeyId(projectId, clientName)))));
+
+        for (var clientName : clientNames) {
+            if (clientsWithIncorrectSecretsConfig.contains(clientName)) {
+                assertClientNotFound(projectId, clientName);
+            } else {
+                assertNotNull(getClientFromManager(projectId, clientName));
+            }
+        }
+    }
+
     public void testClientsForMultipleProjects() throws InterruptedException {
         final List<ProjectId> projectIds = randomList(2, 8, ESTestCase::randomUniqueProjectId);