From 114816e4d612f00f9fcf26bcf48728c8955c4a3a Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Mon, 8 Jan 2024 17:44:41 +0100 Subject: [PATCH] No need to sort --- .../RemoteClusterCredentialsManager.java | 13 ++++-------- .../transport/RemoteClusterService.java | 8 +++---- .../transport/RemoteClusterServiceTests.java | 21 ++++++++++--------- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java index 3e6206b31a9f1..bc2e3ade7440f 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java @@ -18,7 +18,7 @@ import java.util.Collections; import java.util.Map; -import java.util.SortedSet; +import java.util.Set; import java.util.TreeSet; import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS; @@ -40,19 +40,14 @@ public final synchronized UpdateRemoteClusterCredentialsResult updateClusterCred return new UpdateRemoteClusterCredentialsResult(new TreeSet<>(newClusterCredentials.keySet()), Collections.emptySortedSet()); } - final SortedSet addedClusterAliases = Sets.sortedDifference(newClusterCredentials.keySet(), clusterCredentials.keySet()); - final SortedSet removedClusterAliases = Sets.sortedDifference(clusterCredentials.keySet(), newClusterCredentials.keySet()); + final Set addedClusterAliases = Sets.difference(newClusterCredentials.keySet(), clusterCredentials.keySet()); + final Set removedClusterAliases = Sets.difference(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 for each. - // Sorting establishes a deterministic call order to prevent deadlocks - SortedSet addedClusterAliases, - SortedSet removedClusterAliases - ) {} + public record UpdateRemoteClusterCredentialsResult(Set addedClusterAliases, Set removedClusterAliases) {} @Nullable public SecureString resolveCredentials(String clusterAlias) { diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java index dbcf0e3ac15b3..0090010e49e2b 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java @@ -308,11 +308,7 @@ private synchronized void updateSkipUnavailable(String clusterAlias, Boolean ski } public synchronized void updateRemoteClusterCredentials(Supplier settingsSupplier, ActionListener listener) { - updateRemoteClusterCredentials(settingsSupplier.get(), listener); - } - - // package-private for testing - synchronized void updateRemoteClusterCredentials(Settings settings, ActionListener listener) { + final Settings settings = settingsSupplier.get(); final UpdateRemoteClusterCredentialsResult result = remoteClusterCredentialsManager.updateClusterCredentials(settings); // 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 @@ -333,6 +329,8 @@ synchronized void updateRemoteClusterCredentials(Settings settings, ActionListen } } + // package-private for testing + private void maybeRebuildConnectionOnCredentialsChange(String clusterAlias, Settings settings, RefCountingRunnable connectionRefs) { if (false == remoteClusters.containsKey(clusterAlias)) { // A credential was added or removed before a remote connection was configured. diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java index 5bc4875b58ae5..29a5d5a34e37f 100644 --- a/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java +++ b/server/src/test/java/org/elasticsearch/transport/RemoteClusterServiceTests.java @@ -1467,10 +1467,8 @@ public void testUpdateRemoteClusterCredentialsRebuildsConnectionWithCorrectProfi final MockSecureSettings secureSettings = new MockSecureSettings(); secureSettings.setString("cluster.remote.cluster_1.credentials", randomAlphaOfLength(10)); final PlainActionFuture listener = new PlainActionFuture<>(); - service.updateRemoteClusterCredentials( - Settings.builder().put(clusterSettings).setSecureSettings(secureSettings).build(), - listener - ); + final Settings settings = Settings.builder().put(clusterSettings).setSecureSettings(secureSettings).build(); + service.updateRemoteClusterCredentials(() -> settings, listener); listener.actionGet(10, TimeUnit.SECONDS); } @@ -1483,7 +1481,7 @@ public void testUpdateRemoteClusterCredentialsRebuildsConnectionWithCorrectProfi final PlainActionFuture listener = new PlainActionFuture<>(); service.updateRemoteClusterCredentials( // Settings without credentials constitute credentials removal - clusterSettings, + () -> clusterSettings, listener ); listener.actionGet(10, TimeUnit.SECONDS); @@ -1568,10 +1566,12 @@ public void testUpdateRemoteClusterCredentialsRebuildsMultipleConnectionsDespite secureSettings.setString("cluster.remote." + goodCluster + ".credentials", randomAlphaOfLength(10)); secureSettings.setString("cluster.remote." + missingCluster + ".credentials", randomAlphaOfLength(10)); final PlainActionFuture listener = new PlainActionFuture<>(); - service.updateRemoteClusterCredentials( - Settings.builder().put(cluster1Settings).put(cluster2Settings).setSecureSettings(secureSettings).build(), - listener - ); + final Settings settings = Settings.builder() + .put(cluster1Settings) + .put(cluster2Settings) + .setSecureSettings(secureSettings) + .build(); + service.updateRemoteClusterCredentials(() -> settings, listener); listener.actionGet(10, TimeUnit.SECONDS); } @@ -1587,9 +1587,10 @@ public void testUpdateRemoteClusterCredentialsRebuildsMultipleConnectionsDespite { final PlainActionFuture listener = new PlainActionFuture<>(); + final Settings settings = Settings.builder().put(cluster1Settings).put(cluster2Settings).build(); service.updateRemoteClusterCredentials( // Settings without credentials constitute credentials removal - Settings.builder().put(cluster1Settings).put(cluster2Settings).build(), + () -> settings, listener ); listener.actionGet(10, TimeUnit.SECONDS);