Tweaks

n1v0lg · Dec 18, 2023 · df4763a · df4763a
1 parent f2c4f4f
commit df4763a
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 53 deletions.
diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java
@@ -36,48 +36,22 @@ public RemoteClusterCredentialsManager(Settings settings) {
     public synchronized UpdateRemoteClusterCredentialsResult updateClusterCredentials(Settings settings) {
         final Map<String, SecureString> newClusterCredentials = REMOTE_CLUSTER_CREDENTIALS.getAsMap(settings);
         if (clusterCredentials.isEmpty()) {
-            setCredentialsAndLog(newClusterCredentials);
-            return new UpdateRemoteClusterCredentialsResult(new TreeSet<>(clusterCredentials.keySet()), Collections.emptySortedSet());
+            setClusterCredentialsAndLog(newClusterCredentials);
+            return new UpdateRemoteClusterCredentialsResult(new TreeSet<>(newClusterCredentials.keySet()), Collections.emptySortedSet());
         }
-        final SortedSet<String> aliasesWithAddedCredentials = Sets.sortedDifference(
-            newClusterCredentials.keySet(),
-            clusterCredentials.keySet()
-        );
-        final SortedSet<String> aliasesWithRemovedCredentials = Sets.sortedDifference(
-            clusterCredentials.keySet(),
-            newClusterCredentials.keySet()
-        );
-        setCredentialsAndLog(newClusterCredentials);
-        assert Sets.haveEmptyIntersection(aliasesWithRemovedCredentials, aliasesWithAddedCredentials);
-        return new UpdateRemoteClusterCredentialsResult(aliasesWithAddedCredentials, aliasesWithRemovedCredentials);
-    }
-
-    private void setCredentialsAndLog(Map<String, SecureString> newClusterCredentials) {
-        clusterCredentials = newClusterCredentials;
-        logger.debug(
-            () -> Strings.format(
-                "Updated remote cluster credentials for clusters: [%s]",
-                Strings.collectionToCommaDelimitedString(clusterCredentials.keySet())
-            )
-        );
+        final SortedSet<String> addedClusterAliases = Sets.sortedDifference(newClusterCredentials.keySet(), clusterCredentials.keySet());
+        final SortedSet<String> removedClusterAliases = Sets.sortedDifference(clusterCredentials.keySet(), newClusterCredentials.keySet());
+        setClusterCredentialsAndLog(newClusterCredentials);
+        assert Sets.haveEmptyIntersection(removedClusterAliases, addedClusterAliases);
+        return new UpdateRemoteClusterCredentialsResult(addedClusterAliases, removedClusterAliases);
     }
 
     public record UpdateRemoteClusterCredentialsResult(
-        // Use sorted sets since we will iterate over these, and call a synchronized method. Establishing a deterministic order to prevent
-        // deadlocks
-        SortedSet<String> aliasesWithAddedCredentials,
-        SortedSet<String> aliasesWithRemovedCredentials
-    ) {
-        int totalSize() {
-            return aliasesWithAddedCredentials.size() + aliasesWithRemovedCredentials.size();
-        }
-
-        SortedSet<String> allAliases() {
-            final var set = new TreeSet<>(aliasesWithAddedCredentials);
-            set.addAll(aliasesWithRemovedCredentials);
-            return set;
-        }
-    }
+        // Use sorted sets since we will iterate over these, and call a synchronized method for each.
+        // Sorting establishes a deterministic call order to prevent deadlocks
+        SortedSet<String> addedClusterAliases,
+        SortedSet<String> removedClusterAliases
+    ) {}
 
     @Nullable
     public SecureString resolveCredentials(String clusterAlias) {
@@ -88,5 +62,15 @@ public boolean hasCredentials(String clusterAlias) {
         return clusterCredentials.containsKey(clusterAlias);
     }
 
+    private void setClusterCredentialsAndLog(Map<String, SecureString> newClusterCredentials) {
+        clusterCredentials = newClusterCredentials;
+        logger.debug(
+            () -> Strings.format(
+                "Updated remote cluster credentials for clusters: [%s]",
+                Strings.collectionToCommaDelimitedString(clusterCredentials.keySet())
+            )
+        );
+    }
+
     public static final RemoteClusterCredentialsManager EMPTY = new RemoteClusterCredentialsManager(Settings.EMPTY);
 }
diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java
@@ -313,29 +313,43 @@ public synchronized void updateRemoteClusterCredentials(Supplier<Settings> setti
 
     public synchronized void updateRemoteClusterCredentials(Settings settings, ActionListener<Void> listener) {
         final UpdateRemoteClusterCredentialsResult result = remoteClusterCredentialsManager.updateClusterCredentials(settings);
-        if (result.totalSize() == 0) {
-            logger.debug("No connection rebuilding required after credentials update");
+        // We only need to rebuild connections when a credential was newly added or removed for a cluster alias, not if the credential
+        // value was updated. Therefore, only consider added or removed aliases
+        final int totalConnectionsToRebuild = result.addedClusterAliases().size() + result.removedClusterAliases().size();
+        if (totalConnectionsToRebuild == 0) {
+            logger.debug("no connection rebuilding required after credentials update");
             listener.onResponse(null);
             return;
         }
-        logger.debug("Rebuilding [{}] connections after credentials update", result.totalSize());
+        logger.debug("rebuilding [{}] connections after credentials update", totalConnectionsToRebuild);
         final GroupedActionListener<RemoteClusterConnectionStatus> groupedListener = new GroupedActionListener<>(
-            result.totalSize(),
+            totalConnectionsToRebuild,
             listener.map(remoteClusterConnectionStatuses -> {
-                assert remoteClusterConnectionStatuses.size() == result.totalSize();
-                logger.debug("Remote connections statuses after credential update: [{}]", remoteClusterConnectionStatuses);
+                logger.debug("rebuild complete for [{}] connections after credentials update", remoteClusterConnectionStatuses.size());
                 return null;
             })
         );
-        for (var aliasWithChangedCredentials : result.allAliases()) {
-            logger.debug("Rebuilding connection for remote cluster [{}] because credentials changed", aliasWithChangedCredentials);
-            if (remoteClusters.containsKey(aliasWithChangedCredentials)) {
-                updateRemoteCluster(aliasWithChangedCredentials, settings, true, groupedListener);
-            } else {
-                // A credential was added before a remote connection was configured.
-                // Without an existing connection, there is nothing to rebuild.
-                groupedListener.onResponse(RemoteClusterConnectionStatus.UNCHANGED);
-            }
+        for (var clusterAlias : result.addedClusterAliases()) {
+            maybeRebuildConnection(clusterAlias, settings, groupedListener);
+        }
+        for (var clusterAlias : result.removedClusterAliases()) {
+            maybeRebuildConnection(clusterAlias, settings, groupedListener);
+        }
+    }
+
+    private void maybeRebuildConnection(
+        String clusterAlias,
+        Settings settings,
+        GroupedActionListener<RemoteClusterConnectionStatus> groupedListener
+    ) {
+        if (remoteClusters.containsKey(clusterAlias)) {
+            logger.trace("rebuilding connection for remote cluster [{}] because credentials changed", clusterAlias);
+            updateRemoteCluster(clusterAlias, settings, true, groupedListener);
+        } else {
+            // A credential was added before a remote connection was configured.
+            // Without an existing connection, there is nothing to rebuild.
+            logger.trace("no connection rebuild required for remote cluster [{}]", clusterAlias);
+            groupedListener.onResponse(RemoteClusterConnectionStatus.UNCHANGED);
         }
     }