From 9db4cb4f98ec3a94243a0d2898a8b32df6429f67 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 8 Dec 2023 18:51:02 +0100 Subject: [PATCH 01/91] Revert "Hot-reloadable remote cluster credentials (#102798)" (#103211) Reverts elastic/elasticsearch#102798 --- docs/changelog/102798.yaml | 5 - .../transport/ProxyConnectionStrategy.java | 2 +- .../transport/RemoteClusterConnection.java | 25 +- .../RemoteClusterCredentialsManager.java | 52 --- .../transport/RemoteClusterService.java | 27 +- .../transport/RemoteConnectionManager.java | 61 +--- .../transport/SniffConnectionStrategy.java | 6 +- .../ProxyConnectionStrategyTests.java | 54 +-- .../RemoteClusterConnectionTests.java | 69 +--- .../RemoteClusterCredentialsManagerTests.java | 43 --- .../RemoteConnectionManagerTests.java | 32 +- .../RemoteConnectionStrategyTests.java | 18 +- .../SniffConnectionStrategyTests.java | 66 +--- .../ReloadRemoteClusterCredentialsAction.java | 50 --- .../authz/privilege/SystemPrivilege.java | 4 +- .../xpack/security/operator/Constants.java | 1 - .../ReloadRemoteClusterCredentialsIT.java | 314 ------------------ .../xpack/security/Security.java | 104 ++---- ...tReloadRemoteClusterCredentialsAction.java | 54 --- .../RemoteClusterCredentialsResolver.java | 51 +++ .../SecurityServerTransportInterceptor.java | 45 ++- .../xpack/security/LocalStateSecurity.java | 14 +- .../xpack/security/SecurityTests.java | 102 ------ ...RemoteClusterCredentialsResolverTests.java | 38 +++ ...curityServerTransportInterceptorTests.java | 77 +++-- 25 files changed, 260 insertions(+), 1054 deletions(-) delete mode 100644 docs/changelog/102798.yaml delete mode 100644 server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java delete mode 100644 server/src/test/java/org/elasticsearch/transport/RemoteClusterCredentialsManagerTests.java delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/ReloadRemoteClusterCredentialsAction.java delete mode 100644 x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/ReloadRemoteClusterCredentialsIT.java delete mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportReloadRemoteClusterCredentialsAction.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolver.java create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolverTests.java diff --git a/docs/changelog/102798.yaml b/docs/changelog/102798.yaml deleted file mode 100644 index 986ad99f96a19..0000000000000 --- a/docs/changelog/102798.yaml +++ /dev/null @@ -1,5 +0,0 @@ -pr: 102798 -summary: Hot-reloadable remote cluster credentials -area: Security -type: enhancement -issues: [] diff --git a/server/src/main/java/org/elasticsearch/transport/ProxyConnectionStrategy.java b/server/src/main/java/org/elasticsearch/transport/ProxyConnectionStrategy.java index cfb6f872ce748..320b9cfdbf7e6 100644 --- a/server/src/main/java/org/elasticsearch/transport/ProxyConnectionStrategy.java +++ b/server/src/main/java/org/elasticsearch/transport/ProxyConnectionStrategy.java @@ -179,7 +179,7 @@ public class ProxyConnectionStrategy extends RemoteConnectionStrategy { RemoteConnectionManager.wrapConnectionWithRemoteClusterInfo( newConnection, clusterAlias, - connectionManager.getCredentialsManager() + actualProfile.getTransportProfile() ), actualProfile.getHandshakeTimeout(), cn -> true, diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterConnection.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterConnection.java index 3c74e46851504..a055e4122257f 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterConnection.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterConnection.java @@ -57,28 +57,15 @@ final class RemoteClusterConnection implements Closeable { * @param settings the nodes settings object * @param clusterAlias the configured alias of the cluster to connect to * @param transportService the local nodes transport service - * @param credentialsManager object to lookup remote cluster credentials by cluster alias. If a cluster is protected by a credential, - * i.e. it has a credential configured via secure setting. - * This means the remote cluster uses the advances RCS model (as opposed to the basic model). + * @param credentialsProtected Whether the remote cluster is protected by a credentials, i.e. it has a credentials configured + * via secure setting. This means the remote cluster uses the new configurable access RCS model + * (as opposed to the basic model). */ - RemoteClusterConnection( - Settings settings, - String clusterAlias, - TransportService transportService, - RemoteClusterCredentialsManager credentialsManager - ) { + RemoteClusterConnection(Settings settings, String clusterAlias, TransportService transportService, boolean credentialsProtected) { this.transportService = transportService; this.clusterAlias = clusterAlias; - ConnectionProfile profile = RemoteConnectionStrategy.buildConnectionProfile( - clusterAlias, - settings, - credentialsManager.hasCredentials(clusterAlias) - ); - this.remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - credentialsManager, - createConnectionManager(profile, transportService) - ); + ConnectionProfile profile = RemoteConnectionStrategy.buildConnectionProfile(clusterAlias, settings, credentialsProtected); + this.remoteConnectionManager = new RemoteConnectionManager(clusterAlias, createConnectionManager(profile, transportService)); this.connectionStrategy = RemoteConnectionStrategy.buildStrategy(clusterAlias, transportService, remoteConnectionManager, settings); // we register the transport service here as a listener to make sure we notify handlers on disconnect etc. this.remoteConnectionManager.addListener(transportService); diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java deleted file mode 100644 index 32a5e196c3a0b..0000000000000 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterCredentialsManager.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * 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.transport; - -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.Strings; -import org.elasticsearch.common.settings.SecureString; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.core.Nullable; - -import java.util.Map; - -import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS; - -public class RemoteClusterCredentialsManager { - - private static final Logger logger = LogManager.getLogger(RemoteClusterCredentialsManager.class); - - private volatile Map clusterCredentials; - - public RemoteClusterCredentialsManager(Settings settings) { - updateClusterCredentials(settings); - } - - public void updateClusterCredentials(Settings settings) { - clusterCredentials = REMOTE_CLUSTER_CREDENTIALS.getAsMap(settings); - logger.debug( - () -> Strings.format( - "Updated remote cluster credentials for clusters: [%s]", - Strings.collectionToCommaDelimitedString(clusterCredentials.keySet()) - ) - ); - } - - @Nullable - public SecureString resolveCredentials(String clusterAlias) { - return clusterCredentials.get(clusterAlias); - } - - public boolean hasCredentials(String clusterAlias) { - return clusterCredentials.containsKey(clusterAlias); - } - - 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 index 6bfbb95cbcfe9..c38f4b26c665f 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java @@ -147,14 +147,15 @@ public boolean isRemoteClusterServerEnabled() { private final TransportService transportService; private final Map remoteClusters = ConcurrentCollections.newConcurrentMap(); - private final RemoteClusterCredentialsManager remoteClusterCredentialsManager; + private final Set credentialsProtectedRemoteClusters; RemoteClusterService(Settings settings, TransportService transportService) { super(settings); this.enabled = DiscoveryNode.isRemoteClusterClient(settings); this.remoteClusterServerEnabled = REMOTE_CLUSTER_SERVER_ENABLED.get(settings); this.transportService = transportService; - this.remoteClusterCredentialsManager = new RemoteClusterCredentialsManager(settings); + this.credentialsProtectedRemoteClusters = REMOTE_CLUSTER_CREDENTIALS.getAsMap(settings).keySet(); + if (remoteClusterServerEnabled) { registerRemoteClusterHandshakeRequestHandler(transportService); } @@ -304,14 +305,6 @@ private synchronized void updateSkipUnavailable(String clusterAlias, Boolean ski } } - public void updateRemoteClusterCredentials(Settings settings) { - remoteClusterCredentialsManager.updateClusterCredentials(settings); - } - - public RemoteClusterCredentialsManager getRemoteClusterCredentialsManager() { - return remoteClusterCredentialsManager; - } - @Override protected void updateRemoteCluster(String clusterAlias, Settings settings) { CountDownLatch latch = new CountDownLatch(1); @@ -370,7 +363,12 @@ synchronized void updateRemoteCluster( if (remote == null) { // this is a new cluster we have to add a new representation Settings finalSettings = Settings.builder().put(this.settings, false).put(newSettings, false).build(); - remote = new RemoteClusterConnection(finalSettings, clusterAlias, transportService, remoteClusterCredentialsManager); + remote = new RemoteClusterConnection( + finalSettings, + clusterAlias, + transportService, + credentialsProtectedRemoteClusters.contains(clusterAlias) + ); remoteClusters.put(clusterAlias, remote); remote.ensureConnected(listener.map(ignored -> RemoteClusterConnectionStatus.CONNECTED)); } else if (remote.shouldRebuildConnection(newSettings)) { @@ -382,7 +380,12 @@ synchronized void updateRemoteCluster( } remoteClusters.remove(clusterAlias); Settings finalSettings = Settings.builder().put(this.settings, false).put(newSettings, false).build(); - remote = new RemoteClusterConnection(finalSettings, clusterAlias, transportService, remoteClusterCredentialsManager); + remote = new RemoteClusterConnection( + finalSettings, + clusterAlias, + transportService, + credentialsProtectedRemoteClusters.contains(clusterAlias) + ); remoteClusters.put(clusterAlias, remote); remote.ensureConnected(listener.map(ignored -> RemoteClusterConnectionStatus.RECONNECTED)); } else { diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java b/server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java index 3b531d54fb033..b16734b273376 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java @@ -12,7 +12,6 @@ import org.elasticsearch.TransportVersion; import org.elasticsearch.action.ActionListener; import org.elasticsearch.cluster.node.DiscoveryNode; -import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.core.Nullable; import org.elasticsearch.core.Releasable; @@ -26,19 +25,18 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicLong; +import static org.elasticsearch.transport.RemoteClusterPortSettings.REMOTE_CLUSTER_PROFILE; import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_HANDSHAKE_ACTION_NAME; public class RemoteConnectionManager implements ConnectionManager { private final String clusterAlias; - private final RemoteClusterCredentialsManager credentialsManager; private final ConnectionManager delegate; private final AtomicLong counter = new AtomicLong(); private volatile List connectedNodes = Collections.emptyList(); - RemoteConnectionManager(String clusterAlias, RemoteClusterCredentialsManager credentialsManager, ConnectionManager delegate) { + RemoteConnectionManager(String clusterAlias, ConnectionManager delegate) { this.clusterAlias = clusterAlias; - this.credentialsManager = credentialsManager; this.delegate = delegate; this.delegate.addListener(new TransportConnectionListener() { @Override @@ -53,10 +51,6 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti }); } - public RemoteClusterCredentialsManager getCredentialsManager() { - return credentialsManager; - } - /** * Remote cluster connections have a different lifecycle from intra-cluster connections. Use {@link #connectToRemoteClusterNode} * instead of this method. @@ -101,7 +95,13 @@ public void openConnection(DiscoveryNode node, @Nullable ConnectionProfile profi node, profile, listener.delegateFailureAndWrap( - (l, connection) -> l.onResponse(wrapConnectionWithRemoteClusterInfo(connection, clusterAlias, credentialsManager)) + (l, connection) -> l.onResponse( + new InternalRemoteConnection( + connection, + clusterAlias, + profile != null ? profile.getTransportProfile() : getConnectionProfile().getTransportProfile() + ) + ) ) ); } @@ -182,35 +182,16 @@ public void closeNoBlock() { * @return a cluster alias if the connection target a node in the remote cluster, otherwise an empty result */ public static Optional resolveRemoteClusterAlias(Transport.Connection connection) { - return resolveRemoteClusterAliasWithCredentials(connection).map(RemoteClusterAliasWithCredentials::clusterAlias); - } - - public record RemoteClusterAliasWithCredentials(String clusterAlias, @Nullable SecureString credentials) { - @Override - public String toString() { - return "RemoteClusterAliasWithCredentials{clusterAlias='" + clusterAlias + "', credentials='::es_redacted::'}"; - } - } - - /** - * This method returns information (alias and credentials) for remote cluster for the given transport connection. - * Either or both of alias and credentials can be null depending on the connection. - * - * @param connection the transport connection for which to resolve a remote cluster alias - */ - public static Optional resolveRemoteClusterAliasWithCredentials(Transport.Connection connection) { Transport.Connection unwrapped = TransportService.unwrapConnection(connection); if (unwrapped instanceof InternalRemoteConnection remoteConnection) { - return Optional.of( - new RemoteClusterAliasWithCredentials(remoteConnection.getClusterAlias(), remoteConnection.getClusterCredentials()) - ); + return Optional.of(remoteConnection.getClusterAlias()); } return Optional.empty(); } private Transport.Connection getConnectionInternal(DiscoveryNode node) throws NodeNotConnectedException { Transport.Connection connection = delegate.getConnection(node); - return wrapConnectionWithRemoteClusterInfo(connection, clusterAlias, credentialsManager); + return new InternalRemoteConnection(connection, clusterAlias, getConnectionProfile().getTransportProfile()); } private synchronized void addConnectedNode(DiscoveryNode addedNode) { @@ -316,27 +297,21 @@ private static final class InternalRemoteConnection implements Transport.Connect private static final Logger logger = LogManager.getLogger(InternalRemoteConnection.class); private final Transport.Connection connection; private final String clusterAlias; - @Nullable - private final SecureString clusterCredentials; + private final boolean isRemoteClusterProfile; - private InternalRemoteConnection(Transport.Connection connection, String clusterAlias, @Nullable SecureString clusterCredentials) { + InternalRemoteConnection(Transport.Connection connection, String clusterAlias, String transportProfile) { assert false == connection instanceof InternalRemoteConnection : "should not double wrap"; assert false == connection instanceof ProxyConnection : "proxy connection should wrap internal remote connection, not the other way around"; - this.connection = Objects.requireNonNull(connection); this.clusterAlias = Objects.requireNonNull(clusterAlias); - this.clusterCredentials = clusterCredentials; + this.connection = Objects.requireNonNull(connection); + this.isRemoteClusterProfile = REMOTE_CLUSTER_PROFILE.equals(Objects.requireNonNull(transportProfile)); } public String getClusterAlias() { return clusterAlias; } - @Nullable - public SecureString getClusterCredentials() { - return clusterCredentials; - } - @Override public DiscoveryNode getNode() { return connection.getNode(); @@ -346,7 +321,7 @@ public DiscoveryNode getNode() { public void sendRequest(long requestId, String action, TransportRequest request, TransportRequestOptions options) throws IOException, TransportException { final String effectiveAction; - if (clusterCredentials != null && TransportService.HANDSHAKE_ACTION_NAME.equals(action)) { + if (isRemoteClusterProfile && TransportService.HANDSHAKE_ACTION_NAME.equals(action)) { logger.trace("sending remote cluster specific handshake to node [{}] of remote cluster [{}]", getNode(), clusterAlias); effectiveAction = REMOTE_CLUSTER_HANDSHAKE_ACTION_NAME; } else { @@ -414,8 +389,8 @@ public boolean hasReferences() { static InternalRemoteConnection wrapConnectionWithRemoteClusterInfo( Transport.Connection connection, String clusterAlias, - RemoteClusterCredentialsManager credentialsManager + String transportProfile ) { - return new InternalRemoteConnection(connection, clusterAlias, credentialsManager.resolveCredentials(clusterAlias)); + return new InternalRemoteConnection(connection, clusterAlias, transportProfile); } } diff --git a/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java b/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java index 0f68a58faf463..0dcad9cf6864c 100644 --- a/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java +++ b/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java @@ -357,11 +357,7 @@ private ConnectionManager.ConnectionValidator getConnectionValidator(DiscoveryNo : "transport profile must be consistent between the connection manager and the actual profile"; transportService.connectionValidator(node) .validate( - RemoteConnectionManager.wrapConnectionWithRemoteClusterInfo( - connection, - clusterAlias, - connectionManager.getCredentialsManager() - ), + RemoteConnectionManager.wrapConnectionWithRemoteClusterInfo(connection, clusterAlias, profile.getTransportProfile()), profile, listener ); diff --git a/server/src/test/java/org/elasticsearch/transport/ProxyConnectionStrategyTests.java b/server/src/test/java/org/elasticsearch/transport/ProxyConnectionStrategyTests.java index b3c7c5adac95d..ead43d0bac05e 100644 --- a/server/src/test/java/org/elasticsearch/transport/ProxyConnectionStrategyTests.java +++ b/server/src/test/java/org/elasticsearch/transport/ProxyConnectionStrategyTests.java @@ -130,11 +130,7 @@ public void testProxyStrategyWillOpenExpectedNumberOfConnectionsToAddress() { ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -192,11 +188,7 @@ public void testProxyStrategyWillOpenNewConnectionsOnDisconnect() throws Excepti AtomicBoolean useAddress1 = new AtomicBoolean(true); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -271,11 +263,7 @@ public void testConnectFailsWithIncompatibleNodes() { ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -340,11 +328,7 @@ public void testConnectFailsWithNonRetryableException() { ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -404,11 +388,7 @@ public void testClusterNameValidationPreventConnectingToDifferentClusters() thro AtomicBoolean useAddress1 = new AtomicBoolean(true); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -479,11 +459,7 @@ public void testProxyStrategyWillResolveAddressesEachConnect() throws Exception ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -535,11 +511,7 @@ public void onNodeConnected(DiscoveryNode node, Transport.Connection connection) }); try ( - var remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + var remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); var strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -582,11 +554,7 @@ public void testProxyStrategyWillNeedToBeRebuiltIfNumOfSocketsOrAddressesOrServe ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, @@ -704,11 +672,7 @@ public void testServerNameAttributes() { ); int numOfConnections = randomIntBetween(4, 8); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); ProxyConnectionStrategy strategy = new ProxyConnectionStrategy( clusterAlias, localService, diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java index cbe15cc9664f4..d4f03f1027838 100644 --- a/server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java +++ b/server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java @@ -62,7 +62,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; @@ -253,14 +252,7 @@ public void run() { AtomicReference exceptionReference = new AtomicReference<>(); String clusterAlias = "test-cluster"; Settings settings = buildRandomSettings(clusterAlias, addresses(seedNode)); - try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - randomFrom(RemoteClusterCredentialsManager.EMPTY, buildCredentialsManager(clusterAlias)) - ) - ) { + try (RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, randomBoolean())) { ActionListener listener = ActionListener.wrap(x -> { listenerCalled.countDown(); fail("expected exception"); @@ -330,14 +322,7 @@ public void testCloseWhileConcurrentlyConnecting() throws IOException, Interrupt service.acceptIncomingRequests(); String clusterAlias = "test-cluster"; Settings settings = buildRandomSettings(clusterAlias, seedNodes); - try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - RemoteClusterCredentialsManager.EMPTY - ) - ) { + try (RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, false)) { int numThreads = randomIntBetween(4, 10); Thread[] threads = new Thread[numThreads]; CyclicBarrier barrier = new CyclicBarrier(numThreads + 1); @@ -485,12 +470,7 @@ private void doTestGetConnectionInfo(boolean hasClusterCredentials) throws Excep settings = Settings.builder().put(settings).setSecureSettings(secureSettings).build(); } try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - hasClusterCredentials ? buildCredentialsManager(clusterAlias) : RemoteClusterCredentialsManager.EMPTY - ) + RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, hasClusterCredentials) ) { // test no nodes connected RemoteConnectionInfo remoteConnectionInfo = assertSerialization(connection.getConnectionInfo()); @@ -682,12 +662,7 @@ private void doTestCollectNodes(boolean hasClusterCredentials) throws Exception } try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - hasClusterCredentials ? buildCredentialsManager(clusterAlias) : RemoteClusterCredentialsManager.EMPTY - ) + RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, hasClusterCredentials) ) { CountDownLatch responseLatch = new CountDownLatch(1); AtomicReference> reference = new AtomicReference<>(); @@ -738,14 +713,7 @@ public void testNoChannelsExceptREG() throws Exception { String clusterAlias = "test-cluster"; Settings settings = buildRandomSettings(clusterAlias, addresses(seedNode)); - try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - RemoteClusterCredentialsManager.EMPTY - ) - ) { + try (RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, false)) { PlainActionFuture plainActionFuture = new PlainActionFuture<>(); connection.ensureConnected(plainActionFuture); plainActionFuture.get(10, TimeUnit.SECONDS); @@ -811,14 +779,7 @@ public void testConnectedNodesConcurrentAccess() throws IOException, Interrupted String clusterAlias = "test-cluster"; Settings settings = buildRandomSettings(clusterAlias, seedNodes); - try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - randomFrom(RemoteClusterCredentialsManager.EMPTY, buildCredentialsManager(clusterAlias)) - ) - ) { + try (RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, randomBoolean())) { final int numGetThreads = randomIntBetween(4, 10); final Thread[] getThreads = new Thread[numGetThreads]; final int numModifyingThreads = randomIntBetween(4, 10); @@ -912,14 +873,7 @@ public void testGetConnection() throws Exception { service.acceptIncomingRequests(); String clusterAlias = "test-cluster"; Settings settings = buildRandomSettings(clusterAlias, addresses(seedNode)); - try ( - RemoteClusterConnection connection = new RemoteClusterConnection( - settings, - clusterAlias, - service, - RemoteClusterCredentialsManager.EMPTY - ) - ) { + try (RemoteClusterConnection connection = new RemoteClusterConnection(settings, clusterAlias, service, false)) { PlainActionFuture.get(fut -> connection.ensureConnected(fut.map(x -> null))); for (int i = 0; i < 10; i++) { // always a direct connection as the remote node is already connected @@ -967,13 +921,4 @@ private static Settings buildSniffSettings(String clusterAlias, List see ); return builder.build(); } - - private static RemoteClusterCredentialsManager buildCredentialsManager(String clusterAlias) { - Objects.requireNonNull(clusterAlias); - final Settings.Builder builder = Settings.builder(); - final MockSecureSettings secureSettings = new MockSecureSettings(); - secureSettings.setString("cluster.remote." + clusterAlias + ".credentials", randomAlphaOfLength(20)); - builder.setSecureSettings(secureSettings); - return new RemoteClusterCredentialsManager(builder.build()); - } } diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteClusterCredentialsManagerTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteClusterCredentialsManagerTests.java deleted file mode 100644 index f02148a40e47e..0000000000000 --- a/server/src/test/java/org/elasticsearch/transport/RemoteClusterCredentialsManagerTests.java +++ /dev/null @@ -1,43 +0,0 @@ -/* - * 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.transport; - -import org.elasticsearch.common.settings.MockSecureSettings; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.test.ESTestCase; - -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; - -public class RemoteClusterCredentialsManagerTests extends ESTestCase { - public void testResolveRemoteClusterCredentials() { - final String clusterAlias = randomAlphaOfLength(9); - final String otherClusterAlias = randomAlphaOfLength(10); - - final String secret = randomAlphaOfLength(20); - final Settings settings = buildSettingsWithCredentials(clusterAlias, secret); - RemoteClusterCredentialsManager credentialsManager = new RemoteClusterCredentialsManager(settings); - assertThat(credentialsManager.resolveCredentials(clusterAlias).toString(), equalTo(secret)); - assertThat(credentialsManager.hasCredentials(otherClusterAlias), is(false)); - - final String updatedSecret = randomAlphaOfLength(21); - credentialsManager.updateClusterCredentials(buildSettingsWithCredentials(clusterAlias, updatedSecret)); - assertThat(credentialsManager.resolveCredentials(clusterAlias).toString(), equalTo(updatedSecret)); - - credentialsManager.updateClusterCredentials(Settings.EMPTY); - assertThat(credentialsManager.hasCredentials(clusterAlias), is(false)); - } - - private Settings buildSettingsWithCredentials(String clusterAlias, String secret) { - final Settings.Builder builder = Settings.builder(); - final MockSecureSettings secureSettings = new MockSecureSettings(); - secureSettings.setString("cluster.remote." + clusterAlias + ".credentials", secret); - return builder.setSecureSettings(secureSettings).build(); - } -} diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteConnectionManagerTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteConnectionManagerTests.java index b1ffda669e6a1..839138d3c7c34 100644 --- a/server/src/test/java/org/elasticsearch/transport/RemoteConnectionManagerTests.java +++ b/server/src/test/java/org/elasticsearch/transport/RemoteConnectionManagerTests.java @@ -13,7 +13,6 @@ import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeUtils; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -24,20 +23,17 @@ import java.io.IOException; import java.net.InetAddress; import java.util.HashSet; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutionException; import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_HANDSHAKE_ACTION_NAME; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; public class RemoteConnectionManagerTests extends ESTestCase { @@ -53,7 +49,6 @@ public void setUp() throws Exception { transport = mock(Transport.class); remoteConnectionManager = new RemoteConnectionManager( "remote-cluster", - RemoteClusterCredentialsManager.EMPTY, new ClusterConnectionManager(Settings.EMPTY, transport, new ThreadContext(Settings.EMPTY)) ); @@ -125,13 +120,10 @@ public void testResolveRemoteClusterAlias() throws ExecutionException, Interrupt public void testRewriteHandshakeAction() throws IOException { final Transport.Connection connection = mock(Transport.Connection.class); - final String clusterAlias = randomAlphaOfLengthBetween(3, 8); - final RemoteClusterCredentialsManager credentialsResolver = mock(RemoteClusterCredentialsManager.class); - when(credentialsResolver.resolveCredentials(clusterAlias)).thenReturn(new SecureString(randomAlphaOfLength(42))); final Transport.Connection wrappedConnection = RemoteConnectionManager.wrapConnectionWithRemoteClusterInfo( connection, - clusterAlias, - credentialsResolver + randomAlphaOfLengthBetween(3, 8), + RemoteClusterPortSettings.REMOTE_CLUSTER_PROFILE ); final long requestId = randomLong(); final TransportRequest request = mock(TransportRequest.class); @@ -150,26 +142,6 @@ public void testRewriteHandshakeAction() throws IOException { verify(connection).sendRequest(requestId, anotherAction, request, options); } - public void testWrapAndResolveConnectionRoundTrip() { - final Transport.Connection connection = mock(Transport.Connection.class); - final String clusterAlias = randomAlphaOfLengthBetween(3, 8); - final RemoteClusterCredentialsManager credentialsResolver = mock(RemoteClusterCredentialsManager.class); - final SecureString credentials = new SecureString(randomAlphaOfLength(42)); - // second credential will never be resolved - when(credentialsResolver.resolveCredentials(clusterAlias)).thenReturn(credentials, (SecureString) null); - final Transport.Connection wrappedConnection = RemoteConnectionManager.wrapConnectionWithRemoteClusterInfo( - connection, - clusterAlias, - credentialsResolver - ); - - final Optional actual = RemoteConnectionManager - .resolveRemoteClusterAliasWithCredentials(wrappedConnection); - - assertThat(actual.isPresent(), is(true)); - assertThat(actual.get(), equalTo(new RemoteConnectionManager.RemoteClusterAliasWithCredentials(clusterAlias, credentials))); - } - private static class TestRemoteConnection extends CloseableConnection { private final DiscoveryNode node; diff --git a/server/src/test/java/org/elasticsearch/transport/RemoteConnectionStrategyTests.java b/server/src/test/java/org/elasticsearch/transport/RemoteConnectionStrategyTests.java index ca9986ba5eb1f..5d461e906a266 100644 --- a/server/src/test/java/org/elasticsearch/transport/RemoteConnectionStrategyTests.java +++ b/server/src/test/java/org/elasticsearch/transport/RemoteConnectionStrategyTests.java @@ -26,11 +26,7 @@ public void testStrategyChangeMeansThatStrategyMustBeRebuilt() { mock(Transport.class), threadContext ); - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - "cluster-alias", - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager("cluster-alias", connectionManager); FakeConnectionStrategy first = new FakeConnectionStrategy( "cluster-alias", mock(TransportService.class), @@ -50,11 +46,7 @@ public void testSameStrategyChangeMeansThatStrategyDoesNotNeedToBeRebuilt() { mock(Transport.class), threadContext ); - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - "cluster-alias", - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager("cluster-alias", connectionManager); FakeConnectionStrategy first = new FakeConnectionStrategy( "cluster-alias", mock(TransportService.class), @@ -77,11 +69,7 @@ public void testChangeInConnectionProfileMeansTheStrategyMustBeRebuilt() { assertEquals(TimeValue.MINUS_ONE, connectionManager.getConnectionProfile().getPingInterval()); assertEquals(Compression.Enabled.INDEXING_DATA, connectionManager.getConnectionProfile().getCompressionEnabled()); assertEquals(Compression.Scheme.LZ4, connectionManager.getConnectionProfile().getCompressionScheme()); - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - "cluster-alias", - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager("cluster-alias", connectionManager); FakeConnectionStrategy first = new FakeConnectionStrategy( "cluster-alias", mock(TransportService.class), diff --git a/server/src/test/java/org/elasticsearch/transport/SniffConnectionStrategyTests.java b/server/src/test/java/org/elasticsearch/transport/SniffConnectionStrategyTests.java index ddee1ff4d690a..3c955258d45c8 100644 --- a/server/src/test/java/org/elasticsearch/transport/SniffConnectionStrategyTests.java +++ b/server/src/test/java/org/elasticsearch/transport/SniffConnectionStrategyTests.java @@ -192,11 +192,7 @@ public void testSniffStrategyWillConnectToAndDiscoverNodes() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - hasClusterCredentials ? new RemoteClusterCredentialsManager(clientSettings) : RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -266,11 +262,7 @@ public void testSniffStrategyWillResolveDiscoveryNodesEachConnect() throws Excep threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -344,11 +336,7 @@ public void testSniffStrategyWillConnectToMaxAllowedNodesAndOpenNewConnectionsOn threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -436,11 +424,7 @@ public void testDiscoverWithSingleIncompatibleSeedNode() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -502,11 +486,7 @@ public void testConnectFailsWithIncompatibleNodes() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -569,11 +549,7 @@ public void testFilterNodesWithNodePredicate() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -641,11 +617,7 @@ public void testConnectFailsIfNoConnectionsOpened() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -722,11 +694,7 @@ public void testClusterNameValidationPreventConnectingToDifferentClusters() thro threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -815,11 +783,7 @@ public void testMultipleCallsToConnectEnsuresConnection() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -931,11 +895,7 @@ public void testConfiguredProxyAddressModeWillReplaceNodeAddress() { threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, @@ -1004,11 +964,7 @@ public void testSniffStrategyWillNeedToBeRebuiltIfNumOfConnectionsOrSeedsOrProxy threadPool.getThreadContext() ); try ( - RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager( - clusterAlias, - RemoteClusterCredentialsManager.EMPTY, - connectionManager - ); + RemoteConnectionManager remoteConnectionManager = new RemoteConnectionManager(clusterAlias, connectionManager); SniffConnectionStrategy strategy = new SniffConnectionStrategy( clusterAlias, localService, diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/ReloadRemoteClusterCredentialsAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/ReloadRemoteClusterCredentialsAction.java deleted file mode 100644 index 27b9460dd67cb..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/settings/ReloadRemoteClusterCredentialsAction.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * 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; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.core.security.action.settings; - -import org.elasticsearch.action.ActionRequest; -import org.elasticsearch.action.ActionRequestValidationException; -import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.ActionType; -import org.elasticsearch.action.support.TransportAction; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.settings.Settings; - -import java.io.IOException; - -public class ReloadRemoteClusterCredentialsAction extends ActionType { - public static final String NAME = "cluster:admin/xpack/security/remote_cluster_credentials/reload"; - public static final ReloadRemoteClusterCredentialsAction INSTANCE = new ReloadRemoteClusterCredentialsAction(); - - private ReloadRemoteClusterCredentialsAction() { - super(NAME, Writeable.Reader.localOnly()); - } - - public static class Request extends ActionRequest { - private final Settings settings; - - public Request(Settings settings) { - this.settings = settings; - } - - @Override - public ActionRequestValidationException validate() { - return null; - } - - public Settings getSettings() { - return settings; - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - TransportAction.localOnly(); - } - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java index 4d24a757537e2..bc42632507256 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java @@ -12,7 +12,6 @@ import org.elasticsearch.index.seqno.RetentionLeaseSyncAction; import org.elasticsearch.persistent.CompletionPersistentTaskAction; import org.elasticsearch.transport.TransportActionProxy; -import org.elasticsearch.xpack.core.security.action.settings.ReloadRemoteClusterCredentialsAction; import org.elasticsearch.xpack.core.security.support.StringMatcher; import java.util.Collections; @@ -44,8 +43,7 @@ public final class SystemPrivilege extends Privilege { "indices:data/read/*", // needed for SystemIndexMigrator "indices:admin/refresh", // needed for SystemIndexMigrator "indices:admin/aliases", // needed for SystemIndexMigrator - TransportSearchShardsAction.TYPE.name(), // added so this API can be called with the system user by other APIs - ReloadRemoteClusterCredentialsAction.NAME // needed for Security plugin reload of remote cluster credentials + TransportSearchShardsAction.TYPE.name() // added so this API can be called with the system user by other APIs ); private static final Predicate PREDICATE = (action) -> { diff --git a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java index ea27eb9406d09..6e78eb2fb5b83 100644 --- a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java +++ b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java @@ -255,7 +255,6 @@ public class Constants { "cluster:admin/xpack/security/profile/suggest", "cluster:admin/xpack/security/profile/set_enabled", "cluster:admin/xpack/security/realm/cache/clear", - "cluster:admin/xpack/security/remote_cluster_credentials/reload", "cluster:admin/xpack/security/role/delete", "cluster:admin/xpack/security/role/get", "cluster:admin/xpack/security/role/put", diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/ReloadRemoteClusterCredentialsIT.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/ReloadRemoteClusterCredentialsIT.java deleted file mode 100644 index 6042d0072270d..0000000000000 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/ReloadRemoteClusterCredentialsIT.java +++ /dev/null @@ -1,314 +0,0 @@ -/* - * 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; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.security; - -import org.apache.lucene.search.TotalHits; -import org.elasticsearch.TransportVersion; -import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse; -import org.elasticsearch.action.admin.cluster.remote.RemoteClusterNodesAction; -import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; -import org.elasticsearch.action.admin.cluster.state.ClusterStateAction; -import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; -import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; -import org.elasticsearch.action.search.SearchRequest; -import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.action.search.SearchShardsRequest; -import org.elasticsearch.action.search.SearchShardsResponse; -import org.elasticsearch.action.search.ShardSearchFailure; -import org.elasticsearch.action.search.TransportSearchAction; -import org.elasticsearch.action.search.TransportSearchShardsAction; -import org.elasticsearch.cluster.ClusterName; -import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.node.VersionInformation; -import org.elasticsearch.common.Strings; -import org.elasticsearch.common.settings.KeyStoreWrapper; -import org.elasticsearch.common.settings.SecureString; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.transport.TransportAddress; -import org.elasticsearch.common.util.concurrent.ConcurrentCollections; -import org.elasticsearch.common.util.concurrent.EsExecutors; -import org.elasticsearch.env.Environment; -import org.elasticsearch.search.SearchHit; -import org.elasticsearch.search.SearchHits; -import org.elasticsearch.search.aggregations.InternalAggregations; -import org.elasticsearch.search.internal.InternalSearchResponse; -import org.elasticsearch.test.SecuritySingleNodeTestCase; -import org.elasticsearch.test.transport.MockTransportService; -import org.elasticsearch.threadpool.TestThreadPool; -import org.elasticsearch.threadpool.ThreadPool; -import org.elasticsearch.transport.RemoteClusterCredentialsManager; -import org.elasticsearch.transport.TransportService; -import org.elasticsearch.xpack.security.authc.ApiKeyService; -import org.elasticsearch.xpack.security.authc.CrossClusterAccessHeaders; -import org.junit.BeforeClass; - -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.concurrent.BlockingQueue; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; - -import static org.hamcrest.Matchers.empty; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasKey; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; - -public class ReloadRemoteClusterCredentialsIT extends SecuritySingleNodeTestCase { - private static final String CLUSTER_ALIAS = "my_remote_cluster"; - - @BeforeClass - public static void disableInFips() { - assumeFalse( - "Cannot run in FIPS mode since the keystore will be password protected and sending a password in the reload" - + "settings api call, require TLS to be configured for the transport layer", - inFipsJvm() - ); - } - - @Override - public String configRoles() { - return org.elasticsearch.core.Strings.format(""" - user: - cluster: [ "ALL" ] - indices: - - names: '*' - privileges: [ "ALL" ] - remote_indices: - - names: '*' - privileges: [ "ALL" ] - clusters: ["*"] - """); - } - - @Override - public void tearDown() throws Exception { - try { - clearRemoteCluster(); - super.tearDown(); - } finally { - ThreadPool.terminate(threadPool, 10, TimeUnit.SECONDS); - } - } - - private final ThreadPool threadPool = new TestThreadPool(getClass().getName()); - - public void testReloadRemoteClusterCredentials() throws Exception { - final String credentials = randomAlphaOfLength(42); - writeCredentialsToKeyStore(credentials); - final RemoteClusterCredentialsManager clusterCredentialsManager = getInstanceFromNode(TransportService.class) - .getRemoteClusterService() - .getRemoteClusterCredentialsManager(); - // Until we reload, credentials written to keystore are not loaded into the credentials manager - assertThat(clusterCredentialsManager.hasCredentials(CLUSTER_ALIAS), is(false)); - reloadSecureSettings(); - assertThat(clusterCredentialsManager.resolveCredentials(CLUSTER_ALIAS), equalTo(credentials)); - - // Check that credentials get used for a remote connection, once we configure it - final BlockingQueue> capturedHeaders = ConcurrentCollections.newBlockingQueue(); - try (MockTransportService remoteTransport = startTransport("remoteNodeA", threadPool, capturedHeaders)) { - final TransportAddress remoteAddress = remoteTransport.getOriginalTransport() - .profileBoundAddresses() - .get("_remote_cluster") - .publishAddress(); - - configureRemoteCluster(remoteAddress); - - // Run search to trigger header capturing on the receiving side - client().search(new SearchRequest(CLUSTER_ALIAS + ":index-a")).get(); - - assertHeadersContainCredentialsThenClear(credentials, capturedHeaders); - - // Update credentials and ensure they are used - final String updatedCredentials = randomAlphaOfLength(41); - writeCredentialsToKeyStore(updatedCredentials); - reloadSecureSettings(); - - client().search(new SearchRequest(CLUSTER_ALIAS + ":index-a")).get(); - - assertHeadersContainCredentialsThenClear(updatedCredentials, capturedHeaders); - } - } - - private void assertHeadersContainCredentialsThenClear(String credentials, BlockingQueue> capturedHeaders) { - assertThat(capturedHeaders, is(not(empty()))); - for (Map actualHeaders : capturedHeaders) { - assertThat(actualHeaders, hasKey(CrossClusterAccessHeaders.CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY)); - assertThat( - actualHeaders.get(CrossClusterAccessHeaders.CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY), - equalTo(ApiKeyService.withApiKeyPrefix(credentials)) - ); - } - capturedHeaders.clear(); - assertThat(capturedHeaders, is(empty())); - } - - private void clearRemoteCluster() throws InterruptedException, ExecutionException { - final var builder = Settings.builder() - .putNull("cluster.remote." + CLUSTER_ALIAS + ".mode") - .putNull("cluster.remote." + CLUSTER_ALIAS + ".seeds") - .putNull("cluster.remote." + CLUSTER_ALIAS + ".proxy_address"); - clusterAdmin().updateSettings(new ClusterUpdateSettingsRequest().persistentSettings(builder)).get(); - } - - @Override - protected Settings nodeSettings() { - return Settings.builder().put(super.nodeSettings()).put("xpack.security.remote_cluster_client.ssl.enabled", false).build(); - } - - private void configureRemoteCluster(TransportAddress remoteAddress) throws InterruptedException, ExecutionException { - final Settings.Builder builder = Settings.builder(); - if (randomBoolean()) { - builder.put("cluster.remote." + CLUSTER_ALIAS + ".mode", "sniff") - .put("cluster.remote." + CLUSTER_ALIAS + ".seeds", remoteAddress.toString()) - .putNull("cluster.remote." + CLUSTER_ALIAS + ".proxy_address"); - } else { - builder.put("cluster.remote." + CLUSTER_ALIAS + ".mode", "proxy") - .put("cluster.remote." + CLUSTER_ALIAS + ".proxy_address", remoteAddress.toString()) - .putNull("cluster.remote." + CLUSTER_ALIAS + ".seeds"); - } - clusterAdmin().updateSettings(new ClusterUpdateSettingsRequest().persistentSettings(builder)).get(); - } - - private void writeCredentialsToKeyStore(String credentials) throws Exception { - final Environment environment = getInstanceFromNode(Environment.class); - final KeyStoreWrapper keyStoreWrapper = KeyStoreWrapper.create(); - keyStoreWrapper.setString("cluster.remote." + CLUSTER_ALIAS + ".credentials", credentials.toCharArray()); - keyStoreWrapper.save(environment.configFile(), new char[0], false); - } - - public static MockTransportService startTransport( - final String nodeName, - final ThreadPool threadPool, - final BlockingQueue> capturedHeaders - ) { - boolean success = false; - final Settings settings = Settings.builder() - .put("node.name", nodeName) - .put("remote_cluster_server.enabled", "true") - .put("remote_cluster.port", "0") - .put("xpack.security.remote_cluster_server.ssl.enabled", "false") - .build(); - final MockTransportService service = MockTransportService.createNewService( - settings, - VersionInformation.CURRENT, - TransportVersion.current(), - threadPool, - null - ); - try { - service.registerRequestHandler( - ClusterStateAction.NAME, - EsExecutors.DIRECT_EXECUTOR_SERVICE, - ClusterStateRequest::new, - (request, channel, task) -> { - capturedHeaders.add(Map.copyOf(threadPool.getThreadContext().getHeaders())); - channel.sendResponse( - new ClusterStateResponse(ClusterName.DEFAULT, ClusterState.builder(ClusterName.DEFAULT).build(), false) - ); - } - ); - service.registerRequestHandler( - RemoteClusterNodesAction.TYPE.name(), - EsExecutors.DIRECT_EXECUTOR_SERVICE, - RemoteClusterNodesAction.Request::new, - (request, channel, task) -> { - capturedHeaders.add(Map.copyOf(threadPool.getThreadContext().getHeaders())); - channel.sendResponse(new RemoteClusterNodesAction.Response(List.of())); - } - ); - service.registerRequestHandler( - TransportSearchShardsAction.TYPE.name(), - EsExecutors.DIRECT_EXECUTOR_SERVICE, - SearchShardsRequest::new, - (request, channel, task) -> { - capturedHeaders.add(Map.copyOf(threadPool.getThreadContext().getHeaders())); - channel.sendResponse(new SearchShardsResponse(List.of(), List.of(), Collections.emptyMap())); - } - ); - service.registerRequestHandler( - TransportSearchAction.TYPE.name(), - EsExecutors.DIRECT_EXECUTOR_SERVICE, - SearchRequest::new, - (request, channel, task) -> { - capturedHeaders.add(Map.copyOf(threadPool.getThreadContext().getHeaders())); - channel.sendResponse( - new SearchResponse( - new InternalSearchResponse( - new SearchHits(new SearchHit[0], new TotalHits(0, TotalHits.Relation.EQUAL_TO), Float.NaN), - InternalAggregations.EMPTY, - null, - null, - false, - null, - 1 - ), - null, - 1, - 1, - 0, - 100, - ShardSearchFailure.EMPTY_ARRAY, - SearchResponse.Clusters.EMPTY - ) - ); - } - ); - service.start(); - service.acceptIncomingRequests(); - success = true; - return service; - } finally { - if (success == false) { - service.close(); - } - } - } - - private void reloadSecureSettings() throws InterruptedException { - final AtomicReference reloadSettingsError = new AtomicReference<>(); - final CountDownLatch latch = new CountDownLatch(1); - final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; - clusterAdmin().prepareReloadSecureSettings() - .setSecureStorePassword(emptyPassword) - .setNodesIds(Strings.EMPTY_ARRAY) - .execute(new ActionListener<>() { - @Override - public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { - try { - assertThat(nodesReloadResponse, notNullValue()); - final Map nodesMap = nodesReloadResponse.getNodesMap(); - assertThat(nodesMap.size(), equalTo(1)); - for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) { - assertThat(nodeResponse.reloadException(), nullValue()); - } - } catch (final AssertionError e) { - reloadSettingsError.set(e); - } finally { - latch.countDown(); - } - } - - @Override - public void onFailure(Exception e) { - reloadSettingsError.set(new AssertionError("Nodes request failed", e)); - latch.countDown(); - } - }); - latch.await(); - if (reloadSettingsError.get() != null) { - throw reloadSettingsError.get(); - } - } -} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 349cebe7f705f..6d7f6fcd3822b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -13,7 +13,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.util.SetOnce; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.TransportVersion; @@ -157,7 +156,6 @@ import org.elasticsearch.xpack.core.security.action.service.GetServiceAccountCredentialsAction; import org.elasticsearch.xpack.core.security.action.service.GetServiceAccountNodesCredentialsAction; import org.elasticsearch.xpack.core.security.action.settings.GetSecuritySettingsAction; -import org.elasticsearch.xpack.core.security.action.settings.ReloadRemoteClusterCredentialsAction; import org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction; import org.elasticsearch.xpack.core.security.action.token.CreateTokenAction; import org.elasticsearch.xpack.core.security.action.token.InvalidateTokenAction; @@ -249,7 +247,6 @@ import org.elasticsearch.xpack.security.action.service.TransportGetServiceAccountCredentialsAction; import org.elasticsearch.xpack.security.action.service.TransportGetServiceAccountNodesCredentialsAction; import org.elasticsearch.xpack.security.action.settings.TransportGetSecuritySettingsAction; -import org.elasticsearch.xpack.security.action.settings.TransportReloadRemoteClusterCredentialsAction; import org.elasticsearch.xpack.security.action.settings.TransportUpdateSecuritySettingsAction; import org.elasticsearch.xpack.security.action.token.TransportCreateTokenAction; import org.elasticsearch.xpack.security.action.token.TransportInvalidateTokenAction; @@ -370,6 +367,7 @@ import org.elasticsearch.xpack.security.support.CacheInvalidatorRegistry; import org.elasticsearch.xpack.security.support.ExtensionComponents; import org.elasticsearch.xpack.security.support.SecuritySystemIndices; +import org.elasticsearch.xpack.security.transport.RemoteClusterCredentialsResolver; import org.elasticsearch.xpack.security.transport.SecurityHttpSettings; import org.elasticsearch.xpack.security.transport.SecurityServerTransportInterceptor; import org.elasticsearch.xpack.security.transport.filter.IPFilter; @@ -559,7 +557,6 @@ public class Security extends Plugin private final SetOnce reservedRoleMappingAction = new SetOnce<>(); private final SetOnce workflowService = new SetOnce<>(); private final SetOnce realms = new SetOnce<>(); - private final SetOnce client = new SetOnce<>(); public Security(Settings settings) { this(settings, Collections.emptyList()); @@ -579,30 +576,25 @@ public Security(Settings settings) { runStartupChecks(settings); Automatons.updateConfiguration(settings); } else { - ensureNoRemoteClusterCredentialsOnDisabledSecurity(settings); + final List remoteClusterCredentialsSettingKeys = RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS.getAllConcreteSettings( + settings + ).map(Setting::getKey).sorted().toList(); + if (false == remoteClusterCredentialsSettingKeys.isEmpty()) { + throw new IllegalArgumentException( + format( + "Found [%s] remote clusters with credentials [%s]. Security [%s] must be enabled to connect to them. " + + "Please either enable security or remove these settings from the keystore.", + remoteClusterCredentialsSettingKeys.size(), + Strings.collectionToCommaDelimitedString(remoteClusterCredentialsSettingKeys), + XPackSettings.SECURITY_ENABLED.getKey() + ) + ); + } this.bootstrapChecks.set(Collections.emptyList()); } this.securityExtensions.addAll(extensions); } - private void ensureNoRemoteClusterCredentialsOnDisabledSecurity(Settings settings) { - assert false == enabled; - final List remoteClusterCredentialsSettingKeys = RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS.getAllConcreteSettings( - settings - ).map(Setting::getKey).sorted().toList(); - if (false == remoteClusterCredentialsSettingKeys.isEmpty()) { - throw new IllegalArgumentException( - format( - "Found [%s] remote clusters with credentials [%s]. Security [%s] must be enabled to connect to them. " - + "Please either enable security or remove these settings from the keystore.", - remoteClusterCredentialsSettingKeys.size(), - Strings.collectionToCommaDelimitedString(remoteClusterCredentialsSettingKeys), - XPackSettings.SECURITY_ENABLED.getKey() - ) - ); - } - } - private static void runStartupChecks(Settings settings) { validateRealmSettings(settings); if (XPackSettings.FIPS_MODE_ENABLED.get(settings)) { @@ -627,14 +619,6 @@ protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); } - protected Client getClient() { - return client.get(); - } - - protected Realms getRealms() { - return realms.get(); - } - @Override public Collection createComponents(PluginServices services) { try { @@ -673,8 +657,6 @@ Collection createComponents( return Collections.singletonList(new SecurityUsageServices(null, null, null, null, null, null)); } - this.client.set(client); - // The settings in `environment` may have additional values over what was provided during construction // See Plugin#additionalSettings() this.settings = environment.settings(); @@ -1001,6 +983,8 @@ Collection createComponents( ipFilter.set(new IPFilter(settings, auditTrailService, clusterService.getClusterSettings(), getLicenseState())); components.add(ipFilter.get()); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = new RemoteClusterCredentialsResolver(settings); + DestructiveOperations destructiveOperations = new DestructiveOperations(settings, clusterService.getClusterSettings()); crossClusterAccessAuthcService.set(new CrossClusterAccessAuthenticationService(clusterService, apiKeyService, authcService.get())); components.add(crossClusterAccessAuthcService.get()); @@ -1014,6 +998,7 @@ Collection createComponents( securityContext.get(), destructiveOperations, crossClusterAccessAuthcService.get(), + remoteClusterCredentialsResolver, getLicenseState() ) ); @@ -1366,7 +1351,6 @@ public void onIndexModule(IndexModule module) { new ActionHandler<>(SetProfileEnabledAction.INSTANCE, TransportSetProfileEnabledAction.class), new ActionHandler<>(GetSecuritySettingsAction.INSTANCE, TransportGetSecuritySettingsAction.class), new ActionHandler<>(UpdateSecuritySettingsAction.INSTANCE, TransportUpdateSecuritySettingsAction.class), - new ActionHandler<>(ReloadRemoteClusterCredentialsAction.INSTANCE, TransportReloadRemoteClusterCredentialsAction.class), usageAction, infoAction ).filter(Objects::nonNull).toList(); @@ -1906,54 +1890,16 @@ public BiConsumer getJoinValidator() { @Override public void reload(Settings settings) throws Exception { if (enabled) { - final List reloadExceptions = new ArrayList<>(); - try { - reloadRemoteClusterCredentials(settings); - } catch (Exception ex) { - reloadExceptions.add(ex); - } - - try { - reloadSharedSecretsForJwtRealms(settings); - } catch (Exception ex) { - reloadExceptions.add(ex); - } - - if (false == reloadExceptions.isEmpty()) { - final var combinedException = new ElasticsearchException( - "secure settings reload failed for one or more security components" - ); - reloadExceptions.forEach(combinedException::addSuppressed); - throw combinedException; - } - } else { - ensureNoRemoteClusterCredentialsOnDisabledSecurity(settings); + realms.get().stream().filter(r -> JwtRealmSettings.TYPE.equals(r.realmRef().getType())).forEach(realm -> { + if (realm instanceof JwtRealm jwtRealm) { + jwtRealm.rotateClientSecret( + CLIENT_AUTHENTICATION_SHARED_SECRET.getConcreteSettingForNamespace(realm.realmRef().getName()).get(settings) + ); + } + }); } } - private void reloadSharedSecretsForJwtRealms(Settings settingsWithKeystore) { - getRealms().stream().filter(r -> JwtRealmSettings.TYPE.equals(r.realmRef().getType())).forEach(realm -> { - if (realm instanceof JwtRealm jwtRealm) { - jwtRealm.rotateClientSecret( - CLIENT_AUTHENTICATION_SHARED_SECRET.getConcreteSettingForNamespace(realm.realmRef().getName()).get(settingsWithKeystore) - ); - } - }); - } - - /** - * This method uses a transport action internally to access classes that are injectable but not part of the plugin contract. - * See {@link TransportReloadRemoteClusterCredentialsAction} for more context. - */ - private void reloadRemoteClusterCredentials(Settings settingsWithKeystore) { - // Accepting a blocking call here since the underlying action is local-only and only performs fast in-memory ops - // (extracts a subset of passed in `settingsWithKeystore` and stores them in a map) - getClient().execute( - ReloadRemoteClusterCredentialsAction.INSTANCE, - new ReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore) - ).actionGet(); - } - static final class ValidateLicenseForFIPS implements BiConsumer { private final boolean inFipsMode; private final LicenseService licenseService; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportReloadRemoteClusterCredentialsAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportReloadRemoteClusterCredentialsAction.java deleted file mode 100644 index de696a3e0353f..0000000000000 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/settings/TransportReloadRemoteClusterCredentialsAction.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * 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; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.security.action.settings; - -import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.support.ActionFilters; -import org.elasticsearch.action.support.TransportAction; -import org.elasticsearch.common.inject.Inject; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.tasks.Task; -import org.elasticsearch.transport.RemoteClusterService; -import org.elasticsearch.transport.TransportService; -import org.elasticsearch.xpack.core.security.action.settings.ReloadRemoteClusterCredentialsAction; -import org.elasticsearch.xpack.security.Security; - -/** - * This is a local-only action which updates remote cluster credentials for remote cluster connections, from keystore settings reloaded via - * a call to {@link org.elasticsearch.rest.action.admin.cluster.RestReloadSecureSettingsAction}. - * - * It's invoked as part of the {@link Security#reload(Settings)} call. - * - * This action is largely an implementation detail to work around the fact that Security is a plugin without direct access to many core - * classes, including the {@link RemoteClusterService} which is required for credentials update. A transport action gives us access to - * the {@link RemoteClusterService} which is injectable but not part of the plugin contract. - */ -public class TransportReloadRemoteClusterCredentialsAction extends TransportAction< - ReloadRemoteClusterCredentialsAction.Request, - ActionResponse.Empty> { - - private final RemoteClusterService remoteClusterService; - - @Inject - public TransportReloadRemoteClusterCredentialsAction(TransportService transportService, ActionFilters actionFilters) { - super(ReloadRemoteClusterCredentialsAction.NAME, actionFilters, transportService.getTaskManager()); - this.remoteClusterService = transportService.getRemoteClusterService(); - } - - @Override - protected void doExecute( - Task task, - ReloadRemoteClusterCredentialsAction.Request request, - ActionListener listener - ) { - // We avoid stashing and marking context as system to keep the action as minimal as possible (i.e., avoid copying context) - remoteClusterService.updateRemoteClusterCredentials(request.getSettings()); - listener.onResponse(ActionResponse.Empty.INSTANCE); - } -} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolver.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolver.java new file mode 100644 index 0000000000000..93735a700bf92 --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolver.java @@ -0,0 +1,51 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.security.transport; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.xpack.security.authc.ApiKeyService; + +import java.util.Map; +import java.util.Optional; + +import static org.elasticsearch.transport.RemoteClusterService.REMOTE_CLUSTER_CREDENTIALS; + +public class RemoteClusterCredentialsResolver { + + private static final Logger logger = LogManager.getLogger(RemoteClusterCredentialsResolver.class); + + private final Map clusterCredentials; + + public RemoteClusterCredentialsResolver(final Settings settings) { + this.clusterCredentials = REMOTE_CLUSTER_CREDENTIALS.getAsMap(settings); + logger.debug( + "Read cluster credentials for remote clusters [{}]", + Strings.collectionToCommaDelimitedString(clusterCredentials.keySet()) + ); + } + + public Optional resolve(final String clusterAlias) { + final SecureString apiKey = clusterCredentials.get(clusterAlias); + if (apiKey == null) { + return Optional.empty(); + } else { + return Optional.of(new RemoteClusterCredentials(clusterAlias, ApiKeyService.withApiKeyPrefix(apiKey.toString()))); + } + } + + record RemoteClusterCredentials(String clusterAlias, String credentials) { + @Override + public String toString() { + return "RemoteClusterCredentials{clusterAlias='" + clusterAlias + "', credentials='::es_redacted::'}"; + } + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java index 162cabf5297ce..53dd31fe46793 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java @@ -12,7 +12,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.cluster.state.ClusterStateAction; import org.elasticsearch.action.support.DestructiveOperations; -import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.ssl.SslConfiguration; import org.elasticsearch.common.util.Maps; @@ -25,7 +24,6 @@ import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.RemoteConnectionManager; -import org.elasticsearch.transport.RemoteConnectionManager.RemoteClusterAliasWithCredentials; import org.elasticsearch.transport.SendRequestTransportException; import org.elasticsearch.transport.Transport; import org.elasticsearch.transport.TransportChannel; @@ -48,7 +46,6 @@ import org.elasticsearch.xpack.core.ssl.SSLService; import org.elasticsearch.xpack.security.Security; import org.elasticsearch.xpack.security.audit.AuditUtil; -import org.elasticsearch.xpack.security.authc.ApiKeyService; import org.elasticsearch.xpack.security.authc.AuthenticationService; import org.elasticsearch.xpack.security.authc.CrossClusterAccessAuthenticationService; import org.elasticsearch.xpack.security.authc.CrossClusterAccessHeaders; @@ -66,6 +63,7 @@ import static org.elasticsearch.transport.RemoteClusterPortSettings.REMOTE_CLUSTER_PROFILE; import static org.elasticsearch.transport.RemoteClusterPortSettings.REMOTE_CLUSTER_SERVER_ENABLED; import static org.elasticsearch.transport.RemoteClusterPortSettings.TRANSPORT_VERSION_ADVANCED_REMOTE_CLUSTER_SECURITY; +import static org.elasticsearch.xpack.security.transport.RemoteClusterCredentialsResolver.RemoteClusterCredentials; public class SecurityServerTransportInterceptor implements TransportInterceptor { @@ -87,7 +85,8 @@ public class SecurityServerTransportInterceptor implements TransportInterceptor private final Settings settings; private final SecurityContext securityContext; private final CrossClusterAccessAuthenticationService crossClusterAccessAuthcService; - private final Function> remoteClusterCredentialsResolver; + private final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver; + private final Function> remoteClusterAliasResolver; private final XPackLicenseState licenseState; public SecurityServerTransportInterceptor( @@ -99,6 +98,7 @@ public SecurityServerTransportInterceptor( SecurityContext securityContext, DestructiveOperations destructiveOperations, CrossClusterAccessAuthenticationService crossClusterAccessAuthcService, + RemoteClusterCredentialsResolver remoteClusterCredentialsResolver, XPackLicenseState licenseState ) { this( @@ -110,8 +110,9 @@ public SecurityServerTransportInterceptor( securityContext, destructiveOperations, crossClusterAccessAuthcService, + remoteClusterCredentialsResolver, licenseState, - RemoteConnectionManager::resolveRemoteClusterAliasWithCredentials + RemoteConnectionManager::resolveRemoteClusterAlias ); } @@ -124,9 +125,10 @@ public SecurityServerTransportInterceptor( SecurityContext securityContext, DestructiveOperations destructiveOperations, CrossClusterAccessAuthenticationService crossClusterAccessAuthcService, + RemoteClusterCredentialsResolver remoteClusterCredentialsResolver, XPackLicenseState licenseState, // Inject for simplified testing - Function> remoteClusterCredentialsResolver + Function> remoteClusterAliasResolver ) { this.settings = settings; this.threadPool = threadPool; @@ -137,6 +139,7 @@ public SecurityServerTransportInterceptor( this.crossClusterAccessAuthcService = crossClusterAccessAuthcService; this.licenseState = licenseState; this.remoteClusterCredentialsResolver = remoteClusterCredentialsResolver; + this.remoteClusterAliasResolver = remoteClusterAliasResolver; this.profileFilters = initializeProfileFilters(destructiveOperations); } @@ -156,8 +159,7 @@ public void sendRequest( TransportResponseHandler handler ) { assertNoCrossClusterAccessHeadersInContext(); - final Optional remoteClusterAlias = remoteClusterCredentialsResolver.apply(connection) - .map(RemoteClusterAliasWithCredentials::clusterAlias); + final Optional remoteClusterAlias = remoteClusterAliasResolver.apply(connection); if (PreAuthorizationUtils.shouldRemoveParentAuthorizationFromThreadContext(remoteClusterAlias, action, securityContext)) { securityContext.executeAfterRemovingParentAuthorization(original -> { sendRequestInner( @@ -276,23 +278,22 @@ public void sendRequest( * Returns cluster credentials if the connection is remote, and cluster credentials are set up for the target cluster. */ private Optional getRemoteClusterCredentials(Transport.Connection connection) { - final Optional remoteClusterAliasWithCredentials = remoteClusterCredentialsResolver - .apply(connection); - if (remoteClusterAliasWithCredentials.isEmpty()) { + final Optional optionalRemoteClusterAlias = remoteClusterAliasResolver.apply(connection); + if (optionalRemoteClusterAlias.isEmpty()) { logger.trace("Connection is not remote"); return Optional.empty(); } - final String remoteClusterAlias = remoteClusterAliasWithCredentials.get().clusterAlias(); - final SecureString remoteClusterCredentials = remoteClusterAliasWithCredentials.get().credentials(); - if (remoteClusterCredentials == null) { + final String remoteClusterAlias = optionalRemoteClusterAlias.get(); + final Optional remoteClusterCredentials = remoteClusterCredentialsResolver.resolve( + remoteClusterAlias + ); + if (remoteClusterCredentials.isEmpty()) { logger.trace("No cluster credentials are configured for remote cluster [{}]", remoteClusterAlias); return Optional.empty(); } - return Optional.of( - new RemoteClusterCredentials(remoteClusterAlias, ApiKeyService.withApiKeyPrefix(remoteClusterCredentials.toString())) - ); + return remoteClusterCredentials; } private void sendWithCrossClusterAccessHeaders( @@ -441,7 +442,7 @@ private void sendWithUser( throw new IllegalStateException("there should always be a user when sending a message for action [" + action + "]"); } - assert securityContext.getParentAuthorization() == null || remoteClusterCredentialsResolver.apply(connection).isEmpty() + assert securityContext.getParentAuthorization() == null || remoteClusterAliasResolver.apply(connection).isPresent() == false : "parent authorization header should not be set for remote cluster requests"; try { @@ -662,12 +663,4 @@ public void onFailure(Exception e) { } } } - - record RemoteClusterCredentials(String clusterAlias, String credentials) { - - @Override - public String toString() { - return "RemoteClusterCredentials{clusterAlias='" + clusterAlias + "', credentials='::es_redacted::'}"; - } - } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/LocalStateSecurity.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/LocalStateSecurity.java index a2aa04e0f56c3..d44e7c27d760e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/LocalStateSecurity.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/LocalStateSecurity.java @@ -16,7 +16,6 @@ import org.elasticsearch.license.LicenseService; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.plugins.ReloadablePlugin; import org.elasticsearch.protocol.xpack.XPackInfoRequest; import org.elasticsearch.protocol.xpack.XPackInfoResponse; import org.elasticsearch.protocol.xpack.XPackUsageRequest; @@ -37,7 +36,7 @@ import java.util.Collections; import java.util.List; -public class LocalStateSecurity extends LocalStateCompositeXPackPlugin implements ReloadablePlugin { +public class LocalStateSecurity extends LocalStateCompositeXPackPlugin { public static class SecurityTransportXPackUsageAction extends TransportXPackUsageAction { @Inject @@ -131,15 +130,4 @@ protected Class> public List plugins() { return plugins; } - - @Override - public void reload(Settings settings) throws Exception { - plugins.stream().filter(p -> p instanceof ReloadablePlugin).forEach(p -> { - try { - ((ReloadablePlugin) p).reload(settings); - } catch (Exception e) { - throw new RuntimeException(e); - } - }); - } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 75e134691225d..6773da137ac96 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -9,13 +9,10 @@ import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionModule; -import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; @@ -75,7 +72,6 @@ import org.elasticsearch.xpack.core.security.SecurityContext; import org.elasticsearch.xpack.core.security.SecurityExtension; import org.elasticsearch.xpack.core.security.SecurityField; -import org.elasticsearch.xpack.core.security.action.settings.ReloadRemoteClusterCredentialsAction; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper; import org.elasticsearch.xpack.core.security.authc.Realm; @@ -120,7 +116,6 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; -import java.util.stream.Stream; import static java.util.Collections.emptyMap; import static org.elasticsearch.xpack.core.security.authc.RealmSettings.getFullSettingKey; @@ -138,9 +133,7 @@ import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class SecurityTests extends ESTestCase { @@ -884,23 +877,6 @@ public void testSecurityMustBeEnableToConnectRemoteClusterWithCredentials() { + "Please either enable security or remove these settings from the keystore." ) ); - - // Security off, remote cluster with credentials on reload call - final MockSecureSettings secureSettings5 = new MockSecureSettings(); - secureSettings5.setString("cluster.remote.my1.credentials", randomAlphaOfLength(20)); - secureSettings5.setString("cluster.remote.my2.credentials", randomAlphaOfLength(20)); - final Settings.Builder builder5 = Settings.builder().setSecureSettings(secureSettings5); - // Use builder with security disabled to construct valid Security instance - final var security = new Security(builder2.build()); - final IllegalArgumentException e5 = expectThrows(IllegalArgumentException.class, () -> security.reload(builder5.build())); - assertThat( - e5.getMessage(), - containsString( - "Found [2] remote clusters with credentials [cluster.remote.my1.credentials,cluster.remote.my2.credentials]. " - + "Security [xpack.security.enabled] must be enabled to connect to them. " - + "Please either enable security or remove these settings from the keystore." - ) - ); } public void testLoadExtensions() throws Exception { @@ -929,84 +905,6 @@ public List loadExtensions(Class extensionPointType) { assertThat(registry, instanceOf(DummyOperatorOnlyRegistry.class)); } - public void testReload() throws Exception { - final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build(); - - final PlainActionFuture value = new PlainActionFuture<>(); - value.onResponse(ActionResponse.Empty.INSTANCE); - final Client mockedClient = mock(Client.class); - - final Realms mockedRealms = mock(Realms.class); - when(mockedRealms.stream()).thenReturn(Stream.of()); - when(mockedClient.execute(eq(ReloadRemoteClusterCredentialsAction.INSTANCE), any())).thenReturn(value); - security = new Security(settings, Collections.emptyList()) { - @Override - protected Client getClient() { - return mockedClient; - } - - @Override - protected Realms getRealms() { - return mockedRealms; - } - }; - - final Settings inputSettings = Settings.EMPTY; - security.reload(inputSettings); - - verify(mockedClient).execute(eq(ReloadRemoteClusterCredentialsAction.INSTANCE), any()); - verify(mockedRealms).stream(); - } - - public void testReloadWithFailures() { - final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build(); - - final boolean failRemoteClusterCredentialsReload = randomBoolean(); - final PlainActionFuture value = new PlainActionFuture<>(); - if (failRemoteClusterCredentialsReload) { - value.onFailure(new RuntimeException("failed remote cluster credentials reload")); - } else { - value.onResponse(ActionResponse.Empty.INSTANCE); - } - final Client mockedClient = mock(Client.class); - when(mockedClient.execute(eq(ReloadRemoteClusterCredentialsAction.INSTANCE), any())).thenReturn(value); - - final Realms mockedRealms = mock(Realms.class); - final boolean failRealmsReload = (false == failRemoteClusterCredentialsReload) || randomBoolean(); - if (failRealmsReload) { - when(mockedRealms.stream()).thenThrow(new RuntimeException("failed jwt realms reload")); - } else { - when(mockedRealms.stream()).thenReturn(Stream.of()); - } - security = new Security(settings, Collections.emptyList()) { - @Override - protected Client getClient() { - return mockedClient; - } - - @Override - protected Realms getRealms() { - return mockedRealms; - } - }; - - final Settings inputSettings = Settings.EMPTY; - final var exception = expectThrows(ElasticsearchException.class, () -> security.reload(inputSettings)); - - assertThat(exception.getMessage(), containsString("secure settings reload failed for one or more security component")); - if (failRemoteClusterCredentialsReload) { - assertThat(exception.getSuppressed()[0].getMessage(), containsString("failed remote cluster credentials reload")); - if (failRealmsReload) { - assertThat(exception.getSuppressed()[1].getMessage(), containsString("failed jwt realms reload")); - } - } else { - assertThat(exception.getSuppressed()[0].getMessage(), containsString("failed jwt realms reload")); - } - // Verify both called despite failure - verify(mockedClient).execute(eq(ReloadRemoteClusterCredentialsAction.INSTANCE), any()); - verify(mockedRealms).stream(); - } - public void testLoadNoExtensions() throws Exception { Settings settings = Settings.builder() .put("xpack.security.enabled", true) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolverTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolverTests.java new file mode 100644 index 0000000000000..debb50384e217 --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/RemoteClusterCredentialsResolverTests.java @@ -0,0 +1,38 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ +package org.elasticsearch.xpack.security.transport; + +import org.elasticsearch.common.settings.MockSecureSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.security.authc.ApiKeyService; + +import java.util.Optional; + +import static org.elasticsearch.xpack.security.transport.RemoteClusterCredentialsResolver.RemoteClusterCredentials; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +public class RemoteClusterCredentialsResolverTests extends ESTestCase { + + public void testResolveRemoteClusterCredentials() { + final String clusterNameA = "clusterA"; + final String clusterDoesNotExist = randomAlphaOfLength(10); + final Settings.Builder builder = Settings.builder(); + + final String secret = randomAlphaOfLength(20); + final MockSecureSettings secureSettings = new MockSecureSettings(); + secureSettings.setString("cluster.remote." + clusterNameA + ".credentials", secret); + final Settings settings = builder.setSecureSettings(secureSettings).build(); + RemoteClusterCredentialsResolver remoteClusterAuthorizationResolver = new RemoteClusterCredentialsResolver(settings); + final Optional remoteClusterCredentials = remoteClusterAuthorizationResolver.resolve(clusterNameA); + assertThat(remoteClusterCredentials.isPresent(), is(true)); + assertThat(remoteClusterCredentials.get().clusterAlias(), equalTo(clusterNameA)); + assertThat(remoteClusterCredentials.get().credentials(), equalTo(ApiKeyService.withApiKeyPrefix(secret))); + assertThat(remoteClusterAuthorizationResolver.resolve(clusterDoesNotExist), is(Optional.empty())); + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptorTests.java index 822c04be4363f..9bd5d416940d3 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptorTests.java @@ -18,7 +18,6 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.settings.ClusterSettings; -import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.ssl.SslClientAuthenticationMode; import org.elasticsearch.common.ssl.SslConfiguration; @@ -34,7 +33,6 @@ import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.RemoteClusterPortSettings; -import org.elasticsearch.transport.RemoteConnectionManager.RemoteClusterAliasWithCredentials; import org.elasticsearch.transport.SendRequestTransportException; import org.elasticsearch.transport.Transport; import org.elasticsearch.transport.Transport.Connection; @@ -79,7 +77,6 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; -import java.util.function.Function; import static org.elasticsearch.cluster.metadata.DataStreamLifecycle.DATA_STREAM_LIFECYCLE_ORIGIN; import static org.elasticsearch.test.ActionListenerUtils.anyActionListener; @@ -90,6 +87,7 @@ import static org.elasticsearch.xpack.core.security.authc.CrossClusterAccessSubjectInfo.CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY; import static org.elasticsearch.xpack.core.security.authz.RoleDescriptorTests.randomUniquelyNamedRoleDescriptors; import static org.elasticsearch.xpack.security.authc.CrossClusterAccessHeaders.CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY; +import static org.elasticsearch.xpack.security.transport.RemoteClusterCredentialsResolver.RemoteClusterCredentials; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; @@ -155,6 +153,7 @@ public void testSendAsync() throws Exception { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); ClusterServiceUtils.setState(clusterService, clusterService.state()); // force state update to trigger listener @@ -206,6 +205,7 @@ public void testSendAsyncSwitchToSystem() throws Exception { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); ClusterServiceUtils.setState(clusterService, clusterService.state()); // force state update to trigger listener @@ -250,6 +250,7 @@ public void testSendWithoutUser() throws Exception { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ) { @Override @@ -312,6 +313,7 @@ public void testSendToNewerVersionSetsCorrectVersion() throws Exception { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); ClusterServiceUtils.setState(clusterService, clusterService.state()); // force state update to trigger listener @@ -380,6 +382,7 @@ public void testSendToOlderVersionSetsCorrectVersion() throws Exception { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); ClusterServiceUtils.setState(clusterService, clusterService.state()); // force state update to trigger listener @@ -446,6 +449,7 @@ public void testSetUserBasedOnActionOrigin() { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); @@ -600,6 +604,7 @@ public void testSendWithCrossClusterAccessHeadersWithUnsupportedLicense() throws AuthenticationTestHelper.builder().build().writeToContext(threadContext); final String remoteClusterAlias = randomAlphaOfLengthBetween(5, 10); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mockRemoteClusterCredentialsResolver(remoteClusterAlias); final SecurityServerTransportInterceptor interceptor = new SecurityServerTransportInterceptor( settings, @@ -613,8 +618,9 @@ public void testSendWithCrossClusterAccessHeadersWithUnsupportedLicense() throws new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + remoteClusterCredentialsResolver, unsupportedLicenseState, - mockRemoteClusterCredentialsResolver(remoteClusterAlias) + ignored -> Optional.of(remoteClusterAlias) ); final AsyncSender sender = interceptor.interceptSender(mock(AsyncSender.class, ignored -> { @@ -655,16 +661,18 @@ public TransportResponse read(StreamInput in) { actualException.get().getCause().getMessage(), equalTo("current license is non-compliant for [" + Security.ADVANCED_REMOTE_CLUSTER_SECURITY_FEATURE.getName() + "]") ); + verify(remoteClusterCredentialsResolver, times(1)).resolve(eq(remoteClusterAlias)); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY), nullValue()); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY), nullValue()); } - private Function> mockRemoteClusterCredentialsResolver( - String remoteClusterAlias - ) { - return connection -> Optional.of( - new RemoteClusterAliasWithCredentials(remoteClusterAlias, new SecureString(randomAlphaOfLengthBetween(10, 42).toCharArray())) + private RemoteClusterCredentialsResolver mockRemoteClusterCredentialsResolver(String remoteClusterAlias) { + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mock(RemoteClusterCredentialsResolver.class); + final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(randomAlphaOfLengthBetween(10, 42)); + when(remoteClusterCredentialsResolver.resolve(any())).thenReturn( + Optional.of(new RemoteClusterCredentials(remoteClusterAlias, remoteClusterCredential)) ); + return remoteClusterCredentialsResolver; } public void testSendWithCrossClusterAccessHeadersForSystemUserRegularAction() throws Exception { @@ -728,9 +736,12 @@ private void doTestSendWithCrossClusterAccessHeaders( ) throws IOException { authentication.writeToContext(threadContext); final String expectedRequestId = AuditUtil.getOrGenerateRequestId(threadContext); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mock(RemoteClusterCredentialsResolver.class); final String remoteClusterAlias = randomAlphaOfLengthBetween(5, 10); - final String encodedApiKey = randomAlphaOfLengthBetween(10, 42); - final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(encodedApiKey); + final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(randomAlphaOfLengthBetween(10, 42)); + when(remoteClusterCredentialsResolver.resolve(any())).thenReturn( + Optional.of(new RemoteClusterCredentials(remoteClusterAlias, remoteClusterCredential)) + ); final AuthorizationService authzService = mock(AuthorizationService.class); // We capture the listener so that we can complete the full flow, by calling onResponse further down @SuppressWarnings("unchecked") @@ -749,8 +760,9 @@ private void doTestSendWithCrossClusterAccessHeaders( new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + remoteClusterCredentialsResolver, mockLicenseState, - ignored -> Optional.of(new RemoteClusterAliasWithCredentials(remoteClusterAlias, new SecureString(encodedApiKey.toCharArray()))) + ignored -> Optional.of(remoteClusterAlias) ); final AtomicBoolean calledWrappedSender = new AtomicBoolean(false); @@ -849,6 +861,7 @@ public TransportResponse read(StreamInput in) { } assertThat(sentCredential.get(), equalTo(remoteClusterCredential)); verify(securityContext, never()).executeAsInternalUser(any(), any(), anyConsumer()); + verify(remoteClusterCredentialsResolver, times(1)).resolve(eq(remoteClusterAlias)); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY), nullValue()); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY), nullValue()); assertThat(AuditUtil.extractRequestId(securityContext.getThreadContext()), equalTo(expectedRequestId)); @@ -861,9 +874,15 @@ public void testSendWithUserIfCrossClusterAccessHeadersConditionNotMet() throws if (false == (notRemoteConnection || noCredential)) { noCredential = true; } - final boolean finalNoCredential = noCredential; final String remoteClusterAlias = randomAlphaOfLengthBetween(5, 10); - final String encodedApiKey = randomAlphaOfLengthBetween(10, 42); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mock(RemoteClusterCredentialsResolver.class); + when(remoteClusterCredentialsResolver.resolve(any())).thenReturn( + noCredential + ? Optional.empty() + : Optional.of( + new RemoteClusterCredentials(remoteClusterAlias, ApiKeyService.withApiKeyPrefix(randomAlphaOfLengthBetween(10, 42))) + ) + ); final AuthenticationTestHelper.AuthenticationTestBuilder builder = AuthenticationTestHelper.builder(); final Authentication authentication = randomFrom( builder.apiKey().build(), @@ -885,12 +904,9 @@ public void testSendWithUserIfCrossClusterAccessHeadersConditionNotMet() throws new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + remoteClusterCredentialsResolver, mockLicenseState, - ignored -> notRemoteConnection - ? Optional.empty() - : (finalNoCredential - ? Optional.of(new RemoteClusterAliasWithCredentials(remoteClusterAlias, null)) - : Optional.of(new RemoteClusterAliasWithCredentials(remoteClusterAlias, new SecureString(encodedApiKey.toCharArray())))) + ignored -> notRemoteConnection ? Optional.empty() : Optional.of(remoteClusterAlias) ); final AtomicBoolean calledWrappedSender = new AtomicBoolean(false); @@ -928,9 +944,12 @@ public void testSendWithCrossClusterAccessHeadersThrowsOnOldConnection() throws .realm() .build(); authentication.writeToContext(threadContext); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mock(RemoteClusterCredentialsResolver.class); final String remoteClusterAlias = randomAlphaOfLengthBetween(5, 10); - final String encodedApiKey = randomAlphaOfLengthBetween(10, 42); - final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(encodedApiKey); + final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(randomAlphaOfLengthBetween(10, 42)); + when(remoteClusterCredentialsResolver.resolve(any())).thenReturn( + Optional.of(new RemoteClusterCredentials(remoteClusterAlias, remoteClusterCredential)) + ); final SecurityServerTransportInterceptor interceptor = new SecurityServerTransportInterceptor( settings, @@ -944,8 +963,9 @@ public void testSendWithCrossClusterAccessHeadersThrowsOnOldConnection() throws new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + remoteClusterCredentialsResolver, mockLicenseState, - ignored -> Optional.of(new RemoteClusterAliasWithCredentials(remoteClusterAlias, new SecureString(encodedApiKey.toCharArray()))) + ignored -> Optional.of(remoteClusterAlias) ); final AsyncSender sender = interceptor.interceptSender(new AsyncSender() { @@ -1009,6 +1029,7 @@ public TransportResponse read(StreamInput in) { + "] does not support receiving them" ) ); + verify(remoteClusterCredentialsResolver, times(1)).resolve(eq(remoteClusterAlias)); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_SUBJECT_INFO_HEADER_KEY), nullValue()); assertThat(securityContext.getThreadContext().getHeader(CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY), nullValue()); } @@ -1019,9 +1040,12 @@ public void testSendRemoteRequestFailsIfUserHasNoRemoteIndicesPrivileges() throw .realm() .build(); authentication.writeToContext(threadContext); + final RemoteClusterCredentialsResolver remoteClusterCredentialsResolver = mock(RemoteClusterCredentialsResolver.class); final String remoteClusterAlias = randomAlphaOfLengthBetween(5, 10); - final String encodedApiKey = randomAlphaOfLengthBetween(10, 42); - final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(encodedApiKey); + final String remoteClusterCredential = ApiKeyService.withApiKeyPrefix(randomAlphaOfLengthBetween(10, 42)); + when(remoteClusterCredentialsResolver.resolve(any())).thenReturn( + Optional.of(new RemoteClusterCredentials(remoteClusterAlias, remoteClusterCredential)) + ); final AuthorizationService authzService = mock(AuthorizationService.class); doAnswer(invocation -> { @@ -1043,8 +1067,9 @@ public void testSendRemoteRequestFailsIfUserHasNoRemoteIndicesPrivileges() throw new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + remoteClusterCredentialsResolver, mockLicenseState, - ignored -> Optional.of(new RemoteClusterAliasWithCredentials(remoteClusterAlias, new SecureString(encodedApiKey.toCharArray()))) + ignored -> Optional.of(remoteClusterAlias) ); final AsyncSender sender = interceptor.interceptSender(new AsyncSender() { @@ -1146,6 +1171,7 @@ public void testProfileFiltersCreatedDifferentlyForDifferentTransportAndRemoteCl new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); @@ -1199,6 +1225,7 @@ public void testNoProfileFilterForRemoteClusterWhenTheFeatureIsDisabled() { new ClusterSettings(Settings.EMPTY, Collections.singleton(DestructiveOperations.REQUIRES_NAME_SETTING)) ), mock(CrossClusterAccessAuthenticationService.class), + new RemoteClusterCredentialsResolver(settings), mockLicenseState ); From cf7941f23bcb367ff9676953fce4d7209ed3dc38 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 8 Dec 2023 13:24:57 -0500 Subject: [PATCH 02/91] [ES|QL] tests auto decode unsigned longs (#103164) This extracts the type handling sugar around unsigned longs I added in #102830. Turns out, fixing that was a little bigger than intended, and I wanted to break it out into a distinct PR. The strategy here is to let the framework convert from the encoded unsigned long into a normal BigInteger, so test case writers can just phrase their expected values as normal BigIntegers rather than having to think about the encoding. Feedback welcome, happy to discuss if folks don't think this is a good direction for the tests. Co-authored-by: Bogdan Pintea --- .../function/AbstractFunctionTestCase.java | 25 ++++++++++++++++--- .../expression/function/TestCaseSupplier.java | 9 +++++++ .../scalar/convert/ToUnsignedLongTests.java | 20 ++++++--------- .../function/scalar/math/AbsTests.java | 19 +++++++------- .../function/scalar/math/CeilTests.java | 24 +++++++++++------- .../function/scalar/math/FloorTests.java | 3 +-- .../scalar/multivalue/MvMaxTests.java | 8 +----- .../scalar/multivalue/MvMedianTests.java | 5 ++-- .../scalar/multivalue/MvMinTests.java | 8 +----- .../xpack/ql/util/NumericUtilsTests.java | 6 +++++ 10 files changed, 74 insertions(+), 53 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java index 8b4a126d8a0c7..d6f3b61cb19dd 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java @@ -49,6 +49,7 @@ import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; import org.elasticsearch.xpack.ql.type.EsField; +import org.elasticsearch.xpack.ql.util.NumericUtils; import org.elasticsearch.xpack.ql.util.StringUtils; import org.elasticsearch.xpack.versionfield.Version; import org.hamcrest.Matcher; @@ -92,6 +93,7 @@ import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO; import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance; @@ -241,7 +243,7 @@ private void testEvaluate(boolean readFloating) { Object result; try (ExpressionEvaluator evaluator = evaluator(expression).get(driverContext())) { try (Block block = evaluator.eval(row(testCase.getDataValues()))) { - result = toJavaObject(block, 0); + result = toJavaObjectUnsignedLongAware(block, 0); } } assertThat(result, not(equalTo(Double.NaN))); @@ -255,6 +257,16 @@ private void testEvaluate(boolean readFloating) { } } + private Object toJavaObjectUnsignedLongAware(Block block, int position) { + Object result; + result = toJavaObject(block, position); + if (result != null && testCase.expectedType == DataTypes.UNSIGNED_LONG) { + assertThat(result, instanceOf(Long.class)); + result = NumericUtils.unsignedLongAsBigInteger((Long) result); + } + return result; + } + /** * Evaluates a {@link Block} of values, all copied from the input pattern, read directly from the page. *

@@ -398,7 +410,7 @@ private void testEvaluateBlock(BlockFactory inputBlockFactory, DriverContext con assertThat(toJavaObject(block, p), allNullsMatcher()); continue; } - assertThat(toJavaObject(block, p), testCase.getMatcher()); + assertThat(toJavaObjectUnsignedLongAware(block, p), testCase.getMatcher()); } assertThat( "evaluates to tracked block", @@ -472,7 +484,7 @@ public final void testEvaluateInManyThreads() throws ExecutionException, Interru try (EvalOperator.ExpressionEvaluator eval = evalSupplier.get(driverContext())) { for (int c = 0; c < count; c++) { try (Block block = eval.eval(page)) { - assertThat(toJavaObject(block, 0), testCase.getMatcher()); + assertThat(toJavaObjectUnsignedLongAware(block, 0), testCase.getMatcher()); } } } @@ -513,7 +525,12 @@ public final void testFold() { expression = new FoldNull().rule(expression); assertThat(expression.dataType(), equalTo(testCase.expectedType)); assertTrue(expression.foldable()); - assertThat(expression.fold(), testCase.getMatcher()); + Object result = expression.fold(); + // Decode unsigned longs into BigIntegers + if (testCase.expectedType == DataTypes.UNSIGNED_LONG && result != null) { + result = NumericUtils.unsignedLongAsBigInteger((Long) result); + } + assertThat(result, testCase.getMatcher()); if (testCase.getExpectedWarnings() != null) { assertWarnings(testCase.getExpectedWarnings()); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java index faf10d499127a..9eab8cbef5fed 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java @@ -10,6 +10,8 @@ import org.apache.lucene.document.InetAddressPoint; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.network.InetAddresses; +import org.elasticsearch.logging.LogManager; +import org.elasticsearch.logging.Logger; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.esql.expression.function.scalar.convert.AbstractConvertFunction; import org.elasticsearch.xpack.esql.type.EsqlDataTypes; @@ -48,6 +50,8 @@ public record TestCaseSupplier(String name, List types, Supplier supplier) implements Supplier { + + private static Logger logger = LogManager.getLogger(TestCaseSupplier.class); /** * Build a test case without types. * @@ -530,6 +534,8 @@ public static void unary( supplier.type(), "value" ); + logger.info("Value is " + value + " of type " + value.getClass()); + logger.info("expectedValue is " + expectedValue.apply(value)); TestCase testCase = new TestCase( List.of(typed), expectedEvaluatorToString, @@ -949,6 +955,9 @@ public TypedData(Object data, String name) { @Override public String toString() { + if (type == DataTypes.UNSIGNED_LONG && data instanceof Long longData) { + return type.toString() + "(" + NumericUtils.unsignedLongAsBigInteger(longData).toString() + ")"; + } return type.toString() + "(" + (data == null ? "null" : data.toString()) + ")"; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToUnsignedLongTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToUnsignedLongTests.java index 080424602703d..8d5ee002a8f78 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToUnsignedLongTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToUnsignedLongTests.java @@ -16,7 +16,6 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; -import org.elasticsearch.xpack.ql.util.NumericUtils; import java.math.BigDecimal; import java.math.BigInteger; @@ -26,10 +25,7 @@ import java.util.function.Supplier; import static org.elasticsearch.xpack.ql.type.DataTypeConverter.safeToUnsignedLong; -import static org.elasticsearch.xpack.ql.util.NumericUtils.ONE_AS_UNSIGNED_LONG; import static org.elasticsearch.xpack.ql.util.NumericUtils.UNSIGNED_LONG_MAX_AS_DOUBLE; -import static org.elasticsearch.xpack.ql.util.NumericUtils.ZERO_AS_UNSIGNED_LONG; -import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned; public class ToUnsignedLongTests extends AbstractFunctionTestCase { public ToUnsignedLongTests(@Name("TestCase") Supplier testCaseSupplier) { @@ -47,7 +43,7 @@ public static Iterable parameters() { suppliers, read, DataTypes.UNSIGNED_LONG, - NumericUtils::asLongUnsigned, + n -> n, BigInteger.ZERO, UNSIGNED_LONG_MAX, List.of() @@ -57,7 +53,7 @@ public static Iterable parameters() { suppliers, evaluatorName.apply("Boolean"), DataTypes.UNSIGNED_LONG, - b -> b ? ONE_AS_UNSIGNED_LONG : ZERO_AS_UNSIGNED_LONG, + b -> b ? BigInteger.ONE : BigInteger.ZERO, List.of() ); @@ -66,7 +62,7 @@ public static Iterable parameters() { suppliers, evaluatorName.apply("Long"), DataTypes.UNSIGNED_LONG, - instant -> asLongUnsigned(instant.toEpochMilli()), + instant -> BigInteger.valueOf(instant.toEpochMilli()), List.of() ); // random strings that don't look like an unsigned_long @@ -85,7 +81,7 @@ public static Iterable parameters() { suppliers, evaluatorName.apply("Double"), DataTypes.UNSIGNED_LONG, - d -> asLongUnsigned(BigDecimal.valueOf(d).toBigInteger()), // note: not: new BigDecimal(d).toBigInteger + d -> BigDecimal.valueOf(d).toBigInteger(), // note: not: new BigDecimal(d).toBigInteger 0d, UNSIGNED_LONG_MAX_AS_DOUBLE, List.of() @@ -122,7 +118,7 @@ public static Iterable parameters() { suppliers, evaluatorName.apply("Long"), DataTypes.UNSIGNED_LONG, - NumericUtils::asLongUnsigned, + BigInteger::valueOf, 0L, Long.MAX_VALUE, List.of() @@ -146,7 +142,7 @@ public static Iterable parameters() { suppliers, evaluatorName.apply("Int"), DataTypes.UNSIGNED_LONG, - NumericUtils::asLongUnsigned, + BigInteger::valueOf, 0, Integer.MAX_VALUE, List.of() @@ -180,7 +176,7 @@ public static Iterable parameters() { ) .toList(), DataTypes.UNSIGNED_LONG, - bytesRef -> asLongUnsigned(safeToUnsignedLong(((BytesRef) bytesRef).utf8ToString())), + bytesRef -> safeToUnsignedLong(((BytesRef) bytesRef).utf8ToString()), List.of() ); // strings of random doubles within unsigned_long's range @@ -198,7 +194,7 @@ public static Iterable parameters() { ) .toList(), DataTypes.UNSIGNED_LONG, - bytesRef -> asLongUnsigned(safeToUnsignedLong(((BytesRef) bytesRef).utf8ToString())), + bytesRef -> safeToUnsignedLong(((BytesRef) bytesRef).utf8ToString()), List.of() ); // strings of random doubles outside unsigned_long's range, negative diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java index e6621fdf78408..491680d537f37 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; +import java.math.BigInteger; import java.util.ArrayList; import java.util.List; import java.util.function.Supplier; @@ -36,15 +37,15 @@ public static Iterable parameters() { equalTo(Math.abs(arg)) ); })); - suppliers.add(new TestCaseSupplier(List.of(DataTypes.UNSIGNED_LONG), () -> { - long arg = randomLong(); - return new TestCaseSupplier.TestCase( - List.of(new TestCaseSupplier.TypedData(arg, DataTypes.UNSIGNED_LONG, "arg")), - "Attribute[channel=0]", - DataTypes.UNSIGNED_LONG, - equalTo(arg) - ); - })); + TestCaseSupplier.forUnaryUnsignedLong( + suppliers, + "Attribute[channel=0]", + DataTypes.UNSIGNED_LONG, + (n) -> n, + BigInteger.ZERO, + UNSIGNED_LONG_MAX, + List.of() + ); suppliers.add(new TestCaseSupplier(List.of(DataTypes.LONG), () -> { long arg = randomLong(); return new TestCaseSupplier.TestCase( diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java index f4e03de146c54..cbc7e99bf6c09 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java @@ -17,6 +17,8 @@ import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; +import java.math.BigInteger; +import java.util.ArrayList; import java.util.List; import java.util.function.Supplier; @@ -29,7 +31,8 @@ public CeilTests(@Name("TestCase") Supplier testCaseS @ParametersFactory public static Iterable parameters() { - return parameterSuppliersFromTypedData(List.of(new TestCaseSupplier("large double value", () -> { + List suppliers = new ArrayList<>(); + suppliers.addAll(List.of(new TestCaseSupplier("large double value", () -> { double arg = 1 / randomDouble(); return new TestCaseSupplier.TestCase( List.of(new TestCaseSupplier.TypedData(arg, DataTypes.DOUBLE, "arg")), @@ -53,15 +56,18 @@ public static Iterable parameters() { DataTypes.LONG, equalTo(arg) ); - }), new TestCaseSupplier("unsigned long value", () -> { - long arg = randomLong(); - return new TestCaseSupplier.TestCase( - List.of(new TestCaseSupplier.TypedData(arg, DataTypes.UNSIGNED_LONG, "arg")), - "Attribute[channel=0]", - DataTypes.UNSIGNED_LONG, - equalTo(arg) - ); }))); + + TestCaseSupplier.forUnaryUnsignedLong( + suppliers, + "Attribute[channel=0]", + DataTypes.UNSIGNED_LONG, + (n) -> n, + BigInteger.ZERO, + UNSIGNED_LONG_MAX, + List.of() + ); + return parameterSuppliersFromTypedData(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/FloorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/FloorTests.java index f41b7c5de38ad..9a185172c9972 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/FloorTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/FloorTests.java @@ -15,7 +15,6 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; -import org.elasticsearch.xpack.ql.util.NumericUtils; import java.math.BigInteger; import java.util.ArrayList; @@ -37,7 +36,7 @@ public static Iterable parameters() { suppliers, read, DataTypes.UNSIGNED_LONG, - ul -> NumericUtils.asLongUnsigned(ul), + ul -> ul, BigInteger.ZERO, UNSIGNED_LONG_MAX, List.of() diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java index 25764a9029bfd..c477cad17904d 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMaxTests.java @@ -14,7 +14,6 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; -import org.elasticsearch.xpack.ql.util.NumericUtils; import java.math.BigInteger; import java.util.ArrayList; @@ -37,12 +36,7 @@ public static Iterable parameters() { doubles(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsDouble())); ints(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsInt())); longs(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsLong())); - unsignedLongs( - cases, - "mv_max", - "MvMax", - (size, values) -> equalTo(NumericUtils.asLongUnsigned(values.reduce(BigInteger::max).get())) - ); + unsignedLongs(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.reduce(BigInteger::max).get())); dateTimes(cases, "mv_max", "MvMax", (size, values) -> equalTo(values.max().getAsLong())); return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(false, cases))); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMedianTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMedianTests.java index ce83c4bb8f786..43e8467147279 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMedianTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMedianTests.java @@ -15,7 +15,6 @@ import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; -import org.elasticsearch.xpack.ql.util.NumericUtils; import java.math.BigInteger; import java.util.ArrayList; @@ -62,12 +61,12 @@ public static Iterable parameters() { unsignedLongs(cases, "mv_median", "MvMedian", (size, values) -> { int middle = size / 2; if (size % 2 == 1) { - return equalTo(NumericUtils.asLongUnsigned(values.sorted().skip(middle).findFirst().get())); + return equalTo(values.sorted().skip(middle).findFirst().get()); } var s = values.sorted().skip(middle - 1).limit(2).iterator(); BigInteger a = s.next(); BigInteger b = s.next(); - return equalTo(NumericUtils.asLongUnsigned(a.add(b.subtract(a).divide(BigInteger.valueOf(2))))); + return equalTo(a.add(b.subtract(a).divide(BigInteger.valueOf(2)))); }); cases.add( diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java index 5556755cfe125..2e47f7c24bb54 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvMinTests.java @@ -14,7 +14,6 @@ import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; -import org.elasticsearch.xpack.ql.util.NumericUtils; import java.math.BigInteger; import java.util.ArrayList; @@ -37,12 +36,7 @@ public static Iterable parameters() { doubles(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsDouble())); ints(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsInt())); longs(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsLong())); - unsignedLongs( - cases, - "mv_min", - "MvMin", - (size, values) -> equalTo(NumericUtils.asLongUnsigned(values.reduce(BigInteger::min).get())) - ); + unsignedLongs(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.reduce(BigInteger::min).get())); dateTimes(cases, "mv_min", "MvMin", (size, values) -> equalTo(values.min().getAsLong())); return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(false, cases))); } diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/util/NumericUtilsTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/util/NumericUtilsTests.java index 45ab0e732305c..80be578ed2647 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/util/NumericUtilsTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/util/NumericUtilsTests.java @@ -13,6 +13,7 @@ import java.util.function.BiFunction; import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned; +import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsBigInteger; import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber; import static org.elasticsearch.xpack.ql.util.StringUtils.parseIntegral; import static org.hamcrest.Matchers.equalTo; @@ -115,6 +116,11 @@ public void testUnsignedLongMultiplyExact() { assertThat(mulExact("0", "0"), equalTo("0")); } + public void testRoundTripConversion() { + BigInteger b = randomUnsignedLongBetween(BigInteger.ZERO, UNSIGNED_LONG_MAX); + assertThat(b, equalTo(unsignedLongAsBigInteger(asLongUnsigned(b)))); + } + private static String addExact(String x, String y) { return exactOperation(x, y, NumericUtils::unsignedLongAddExact); } From 690a8a2ef8f0b686c75f0e9094b814fc5ecd3051 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 8 Dec 2023 14:27:22 -0500 Subject: [PATCH 03/91] [ES|QL] Trig & hyperbolic function finishing pass (#103205) Relates to #100558 These functions were mostly done already. I added descriptions and argument descriptions for most of them, otherwise not much work. --- docs/reference/esql/functions/cos.asciidoc | 2 +- docs/reference/esql/functions/sin.asciidoc | 2 +- docs/reference/esql/functions/tan.asciidoc | 2 +- .../qa/testFixtures/src/main/resources/show.csv-spec | 12 ++++++------ .../esql/expression/function/scalar/math/Cos.java | 7 +++++-- .../esql/expression/function/scalar/math/Cosh.java | 11 +++++++++-- .../esql/expression/function/scalar/math/Sinh.java | 11 +++++++++-- .../esql/expression/function/scalar/math/Tan.java | 7 +++++-- .../esql/expression/function/scalar/math/Tanh.java | 11 +++++++++-- .../expression/function/scalar/math/CoshTests.java | 3 +-- .../expression/function/scalar/math/SinhTests.java | 3 +-- 11 files changed, 48 insertions(+), 23 deletions(-) diff --git a/docs/reference/esql/functions/cos.asciidoc b/docs/reference/esql/functions/cos.asciidoc index 5dcbb7bea37f4..7227f57e28120 100644 --- a/docs/reference/esql/functions/cos.asciidoc +++ b/docs/reference/esql/functions/cos.asciidoc @@ -4,7 +4,7 @@ [.text-center] image::esql/functions/signature/cos.svg[Embedded,opts=inline] -https://en.wikipedia.org/wiki/Sine_and_cosine[Cosine] trigonometric function. +https://en.wikipedia.org/wiki/Sine_and_cosine[Cosine] trigonometric function. Input expected in radians. [source.merge.styled,esql] ---- diff --git a/docs/reference/esql/functions/sin.asciidoc b/docs/reference/esql/functions/sin.asciidoc index 5fa33a315392d..d948bf2ec39a3 100644 --- a/docs/reference/esql/functions/sin.asciidoc +++ b/docs/reference/esql/functions/sin.asciidoc @@ -4,7 +4,7 @@ [.text-center] image::esql/functions/signature/sin.svg[Embedded,opts=inline] -https://en.wikipedia.org/wiki/Sine_and_cosine[Sine] trigonometric function. +https://en.wikipedia.org/wiki/Sine_and_cosine[Sine] trigonometric function. Input expected in radians. [source.merge.styled,esql] ---- diff --git a/docs/reference/esql/functions/tan.asciidoc b/docs/reference/esql/functions/tan.asciidoc index 1c66562eada7a..03e450ff23b0e 100644 --- a/docs/reference/esql/functions/tan.asciidoc +++ b/docs/reference/esql/functions/tan.asciidoc @@ -4,7 +4,7 @@ [.text-center] image::esql/functions/signature/tan.svg[Embedded,opts=inline] -https://en.wikipedia.org/wiki/Sine_and_cosine[Tangent] trigonometric function. +https://en.wikipedia.org/wiki/Sine_and_cosine[Tangent] trigonometric function. Input expected in radians. [source.merge.styled,esql] ---- diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/show.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/show.csv-spec index ffad468790998..083bd1eaf8417 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/show.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/show.csv-spec @@ -6,7 +6,7 @@ v:long ; # TODO: switch this test to ``&format=csv&delimiter=|` output -showFunctions#[skip:-8.11.99] +showFunctions#[skip:-8.12.99] show functions; name:keyword | synopsis:keyword | argNames:keyword | argTypes:keyword | argDescriptions:keyword |returnType:keyword | description:keyword | optionalArgs:boolean | variadic:boolean @@ -22,8 +22,8 @@ ceil |"? ceil(n:integer|long|double|unsigned_long)" |n cidr_match |? cidr_match(arg1:?, arg2...:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | true coalesce |? coalesce(arg1:?, arg2...:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | true concat |? concat(arg1:?, arg2...:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | true -cos |"double cos(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "" |double | "" | false | false -cosh |"double cosh(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "" |double | "" | false | false +cos |"double cos(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "An angle, in radians" |double | "Returns the trigonometric cosine of an angle" | false | false +cosh |"double cosh(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "The number who's hyperbolic cosine is to be returned" |double | "Returns the hyperbolic cosine of a number" | false | false count |? count(arg1:?) |arg1 |? | "" |? | "" | false | false count_distinct |? count_distinct(arg1:?, arg2:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | false date_extract |? date_extract(arg1:?, arg2:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | false @@ -63,14 +63,14 @@ right |"? right(string:keyword, length:integer)" |[string round |? round(arg1:?, arg2:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | false rtrim |"keyword|text rtrim(str:keyword|text)" |str |"keyword|text" | "" |"keyword|text" |Removes trailing whitespaces from a string.| false | false sin |"double sin(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" |An angle, in radians |double |Returns the trigonometric sine of an angle | false | false -sinh |"double sinh(n:integer|long|double|unsigned_long)"|n |"integer|long|double|unsigned_long" | "" |double | "" | false | false +sinh |"double sinh(n:integer|long|double|unsigned_long)"|n |"integer|long|double|unsigned_long" | "The number to return the hyperbolic sine of" |double | "Returns the hyperbolic sine of a number" | false | false split |? split(arg1:?, arg2:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | false sqrt |"? sqrt(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "" |? | "" | false | false starts_with |? starts_with(arg1:?, arg2:?) |[arg1, arg2] |[?, ?] |["", ""] |? | "" | [false, false] | false substring |? substring(arg1:?, arg2:?, arg3:?) |[arg1, arg2, arg3] |[?, ?, ?] |["", "", ""] |? | "" | [false, false, false]| false sum |? sum(arg1:?) |arg1 |? | "" |? | "" | false | false -tan |"double tan(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "" |double | "" | false | false -tanh |"double tanh(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "" |double | "" | false | false +tan |"double tan(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "An angle, in radians" |double | "Returns the trigonometric tangent of an angle" | false | false +tanh |"double tanh(n:integer|long|double|unsigned_long)" |n |"integer|long|double|unsigned_long" | "The number to return the hyperbolic tangent of" |double | "Returns the hyperbolic tangent of a number" | false | false tau |? tau() | null | null | null |? | "" | null | false to_bool |"boolean to_bool(v:boolean|keyword|text|double|long|unsigned_long|integer)" |v |"boolean|keyword|text|double|long|unsigned_long|integer" | |boolean | |false |false to_boolean |"boolean to_boolean(v:boolean|keyword|text|double|long|unsigned_long|integer)" |v |"boolean|keyword|text|double|long|unsigned_long|integer" | |boolean | |false |false diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cos.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cos.java index c5edc0e0fd6c7..5f8661bb0ae7d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cos.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cos.java @@ -21,8 +21,11 @@ * Cosine trigonometric function. */ public class Cos extends AbstractTrigonometricFunction { - @FunctionInfo(returnType = "double") - public Cos(Source source, @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }) Expression n) { + @FunctionInfo(returnType = "double", description = "Returns the trigonometric cosine of an angle") + public Cos( + Source source, + @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }, description = "An angle, in radians") Expression n + ) { super(source, n); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cosh.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cosh.java index cc315c3e9569c..6cc49cec0c32d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cosh.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Cosh.java @@ -21,8 +21,15 @@ * Cosine hyperbolic function. */ public class Cosh extends AbstractTrigonometricFunction { - @FunctionInfo(returnType = "double") - public Cosh(Source source, @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }) Expression n) { + @FunctionInfo(returnType = "double", description = "Returns the hyperbolic cosine of a number") + public Cosh( + Source source, + @Param( + name = "n", + type = { "integer", "long", "double", "unsigned_long" }, + description = "The number who's hyperbolic cosine is to be returned" + ) Expression n + ) { super(source, n); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Sinh.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Sinh.java index 07228dd1743dd..4b2adef5a2d6f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Sinh.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Sinh.java @@ -21,8 +21,15 @@ * Sine hyperbolic function. */ public class Sinh extends AbstractTrigonometricFunction { - @FunctionInfo(returnType = "double") - public Sinh(Source source, @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }) Expression n) { + @FunctionInfo(returnType = "double", description = "Returns the hyperbolic sine of a number") + public Sinh( + Source source, + @Param( + name = "n", + type = { "integer", "long", "double", "unsigned_long" }, + description = "The number to return the hyperbolic sine of" + ) Expression n + ) { super(source, n); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tan.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tan.java index e6c7bd8c6e530..5596c9098c034 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tan.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tan.java @@ -21,8 +21,11 @@ * Tangent trigonometric function. */ public class Tan extends AbstractTrigonometricFunction { - @FunctionInfo(returnType = "double") - public Tan(Source source, @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }) Expression n) { + @FunctionInfo(returnType = "double", description = "Returns the trigonometric tangent of an angle") + public Tan( + Source source, + @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }, description = "An angle, in radians") Expression n + ) { super(source, n); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tanh.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tanh.java index a9720d6d65c11..ce59cec50bcca 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tanh.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Tanh.java @@ -21,8 +21,15 @@ * Tangent hyperbolic function. */ public class Tanh extends AbstractTrigonometricFunction { - @FunctionInfo(returnType = "double") - public Tanh(Source source, @Param(name = "n", type = { "integer", "long", "double", "unsigned_long" }) Expression n) { + @FunctionInfo(returnType = "double", description = "Returns the hyperbolic tangent of a number") + public Tanh( + Source source, + @Param( + name = "n", + type = { "integer", "long", "double", "unsigned_long" }, + description = "The number to return the hyperbolic tangent of" + ) Expression n + ) { super(source, n); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CoshTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CoshTests.java index fe9578aea6480..be7e8e47a0754 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CoshTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CoshTests.java @@ -33,7 +33,6 @@ public static Iterable parameters() { 710d, // Hyperbolic Cosine grows extremely fast. Values outside this range return Double.POSITIVE_INFINITY List.of() ); - suppliers = anyNullIsNull(true, suppliers); // Out of range cases suppliers.addAll( @@ -62,7 +61,7 @@ public static Iterable parameters() { ) ) ); - return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(suppliers)); + return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(true, suppliers))); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/SinhTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/SinhTests.java index ede33838903de..465879b071542 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/SinhTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/SinhTests.java @@ -33,7 +33,6 @@ public static Iterable parameters() { 710d, // Hyperbolic sine grows extremely fast. Values outside this range return Double.POSITIVE_INFINITY List.of() ); - suppliers = anyNullIsNull(true, suppliers); // Out of range cases suppliers.addAll( @@ -62,7 +61,7 @@ public static Iterable parameters() { ) ) ); - return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(suppliers)); + return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(true, suppliers))); } @Override From 47b57537aec5d49a829e84bf11b310f0f31310be Mon Sep 17 00:00:00 2001 From: Benjamin Trent Date: Fri, 8 Dec 2023 14:39:18 -0500 Subject: [PATCH 04/91] Add docs for the include_named_queries_score param (#103155) The only docs for this _search param were mentioned in the bool query docs. While it makes contextual sense to have it there, we should also add it as a _search parameter in the search API docs. It was introduced in 8.8. --- docs/reference/search/search.asciidoc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/reference/search/search.asciidoc b/docs/reference/search/search.asciidoc index 68d286b3f267b..96e3ae43f98dc 100644 --- a/docs/reference/search/search.asciidoc +++ b/docs/reference/search/search.asciidoc @@ -111,6 +111,13 @@ By default, you cannot page through more than 10,000 hits using the `from` and include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=ignore_throttled] +`include_named_queries_score`:: +(Optional, Boolean) If `true`, includes the score contribution from any named queries. +This functionality reruns each named query on every hit in a search +response. Typically, this adds a small overhead to a request. However, using +computationally expensive named queries on a large number of hits may add +significant overhead. Defaults to `false`. + include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=index-ignore-unavailable] include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=lenient] From bbbc58f37a073880ec7923f31ad599efd94f64bc Mon Sep 17 00:00:00 2001 From: Benjamin Trent Date: Fri, 8 Dec 2023 15:34:07 -0500 Subject: [PATCH 05/91] Fix NPE & empty result handling in CountOnlyQueryPhaseResultConsumer (#103203) Query results can be "null" in that they are a null instance, containing no information from the shard. Additionally, if we see no results, its an empty reduce phase. This code was introduced into 8.12, which has yet to be released, but flagging as a bug for clarity. --- docs/changelog/103203.yaml | 5 + .../CountOnlyQueryPhaseResultConsumer.java | 9 +- .../search/query/QuerySearchResult.java | 6 + ...ountOnlyQueryPhaseResultConsumerTests.java | 133 ++++++++++++++++++ 4 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 docs/changelog/103203.yaml create mode 100644 server/src/test/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumerTests.java diff --git a/docs/changelog/103203.yaml b/docs/changelog/103203.yaml new file mode 100644 index 0000000000000..d2aa3e9961c6a --- /dev/null +++ b/docs/changelog/103203.yaml @@ -0,0 +1,5 @@ +pr: 103203 +summary: Fix NPE & empty result handling in `CountOnlyQueryPhaseResultConsumer` +area: Search +type: bug +issues: [] diff --git a/server/src/main/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumer.java b/server/src/main/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumer.java index 1e67522f6a671..13972ea2bf64a 100644 --- a/server/src/main/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumer.java +++ b/server/src/main/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumer.java @@ -49,12 +49,17 @@ Stream getSuccessfulResults() { public void consumeResult(SearchPhaseResult result, Runnable next) { assert results.contains(result.getShardIndex()) == false : "shardIndex: " + result.getShardIndex() + " is already set"; results.add(result.getShardIndex()); + progressListener.notifyQueryResult(result.getShardIndex(), result.queryResult()); + // We have an empty result, track that we saw it for this shard and continue; + if (result.queryResult().isNull()) { + next.run(); + return; + } // set the relation to the first non-equal relation relationAtomicReference.compareAndSet(TotalHits.Relation.EQUAL_TO, result.queryResult().getTotalHits().relation); totalHits.add(result.queryResult().getTotalHits().value); terminatedEarly.compareAndSet(false, (result.queryResult().terminatedEarly() != null && result.queryResult().terminatedEarly())); timedOut.compareAndSet(false, result.queryResult().searchTimedOut()); - progressListener.notifyQueryResult(result.getShardIndex(), result.queryResult()); next.run(); } @@ -80,7 +85,7 @@ public SearchPhaseController.ReducedQueryPhase reduce() throws Exception { 1, 0, 0, - false + results.isEmpty() ); if (progressListener != SearchProgressListener.NOOP) { progressListener.notifyFinalReduce( diff --git a/server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java b/server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java index 40d4e37045016..7bcafe7005047 100644 --- a/server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java @@ -16,6 +16,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.lucene.search.TopDocsAndMaxScore; import org.elasticsearch.core.AbstractRefCounted; +import org.elasticsearch.core.Nullable; import org.elasticsearch.core.RefCounted; import org.elasticsearch.core.Releasable; import org.elasticsearch.core.Releasables; @@ -149,6 +150,7 @@ public void terminatedEarly(boolean terminatedEarly) { this.terminatedEarly = terminatedEarly; } + @Nullable public Boolean terminatedEarly() { return this.terminatedEarly; } @@ -204,10 +206,12 @@ public void setRankShardResult(RankShardResult rankShardResult) { this.rankShardResult = rankShardResult; } + @Nullable public RankShardResult getRankShardResult() { return rankShardResult; } + @Nullable public DocValueFormat[] sortValueFormats() { return sortValueFormats; } @@ -252,6 +256,7 @@ public void aggregations(InternalAggregations aggregations) { hasAggs = aggregations != null; } + @Nullable public DelayableWriteable aggregations() { return aggregations; } @@ -455,6 +460,7 @@ public void writeToNoId(StreamOutput out) throws IOException { } } + @Nullable public TotalHits getTotalHits() { return totalHits; } diff --git a/server/src/test/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumerTests.java b/server/src/test/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumerTests.java new file mode 100644 index 0000000000000..33e6096bab763 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/search/CountOnlyQueryPhaseResultConsumerTests.java @@ -0,0 +1,133 @@ +/* + * 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.action.search; + +import org.apache.lucene.search.ScoreDoc; +import org.apache.lucene.search.TopDocs; +import org.apache.lucene.search.TotalHits; +import org.elasticsearch.common.lucene.search.TopDocsAndMaxScore; +import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.SearchShardTarget; +import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.query.QuerySearchResult; +import org.elasticsearch.test.ESTestCase; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; + +public class CountOnlyQueryPhaseResultConsumerTests extends ESTestCase { + + public void testProgressListenerExceptionsAreCaught() throws Exception { + ThrowingSearchProgressListener searchProgressListener = new ThrowingSearchProgressListener(); + + List searchShards = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + searchShards.add(new SearchShard(null, new ShardId("index", "uuid", i))); + } + long timestamp = randomLongBetween(1000, Long.MAX_VALUE - 1000); + TransportSearchAction.SearchTimeProvider timeProvider = new TransportSearchAction.SearchTimeProvider( + timestamp, + timestamp, + () -> timestamp + 1000 + ); + searchProgressListener.notifyListShards(searchShards, Collections.emptyList(), SearchResponse.Clusters.EMPTY, false, timeProvider); + + CountOnlyQueryPhaseResultConsumer queryPhaseResultConsumer = new CountOnlyQueryPhaseResultConsumer(searchProgressListener, 10); + try { + AtomicInteger nextCounter = new AtomicInteger(0); + for (int i = 0; i < 10; i++) { + SearchShardTarget searchShardTarget = new SearchShardTarget("node", new ShardId("index", "uuid", i), null); + QuerySearchResult querySearchResult = new QuerySearchResult(); + TopDocs topDocs = new TopDocs(new TotalHits(0, TotalHits.Relation.EQUAL_TO), new ScoreDoc[0]); + querySearchResult.topDocs(new TopDocsAndMaxScore(topDocs, Float.NaN), new DocValueFormat[0]); + querySearchResult.setSearchShardTarget(searchShardTarget); + querySearchResult.setShardIndex(i); + queryPhaseResultConsumer.consumeResult(querySearchResult, nextCounter::incrementAndGet); + } + + assertEquals(10, searchProgressListener.onQueryResult.get()); + queryPhaseResultConsumer.reduce(); + assertEquals(1, searchProgressListener.onFinalReduce.get()); + assertEquals(10, nextCounter.get()); + } finally { + queryPhaseResultConsumer.decRef(); + } + } + + public void testNullShardResultHandling() throws Exception { + CountOnlyQueryPhaseResultConsumer queryPhaseResultConsumer = new CountOnlyQueryPhaseResultConsumer(SearchProgressListener.NOOP, 10); + try { + AtomicInteger nextCounter = new AtomicInteger(0); + for (int i = 0; i < 10; i++) { + SearchShardTarget searchShardTarget = new SearchShardTarget("node", new ShardId("index", "uuid", i), null); + QuerySearchResult querySearchResult = QuerySearchResult.nullInstance(); + querySearchResult.setSearchShardTarget(searchShardTarget); + querySearchResult.setShardIndex(i); + queryPhaseResultConsumer.consumeResult(querySearchResult, nextCounter::incrementAndGet); + } + var reducePhase = queryPhaseResultConsumer.reduce(); + assertEquals(0, reducePhase.totalHits().value); + assertEquals(TotalHits.Relation.EQUAL_TO, reducePhase.totalHits().relation); + assertFalse(reducePhase.isEmptyResult()); + assertEquals(10, nextCounter.get()); + } finally { + queryPhaseResultConsumer.decRef(); + } + } + + public void testEmptyResults() throws Exception { + CountOnlyQueryPhaseResultConsumer queryPhaseResultConsumer = new CountOnlyQueryPhaseResultConsumer(SearchProgressListener.NOOP, 10); + try { + var reducePhase = queryPhaseResultConsumer.reduce(); + assertEquals(0, reducePhase.totalHits().value); + assertEquals(TotalHits.Relation.EQUAL_TO, reducePhase.totalHits().relation); + assertTrue(reducePhase.isEmptyResult()); + } finally { + queryPhaseResultConsumer.decRef(); + } + } + + private static class ThrowingSearchProgressListener extends SearchProgressListener { + private final AtomicInteger onQueryResult = new AtomicInteger(0); + private final AtomicInteger onPartialReduce = new AtomicInteger(0); + private final AtomicInteger onFinalReduce = new AtomicInteger(0); + + @Override + protected void onListShards( + List shards, + List skippedShards, + SearchResponse.Clusters clusters, + boolean fetchPhase, + TransportSearchAction.SearchTimeProvider timeProvider + ) { + throw new UnsupportedOperationException(); + } + + @Override + protected void onQueryResult(int shardIndex, QuerySearchResult queryResult) { + onQueryResult.incrementAndGet(); + throw new UnsupportedOperationException(); + } + + @Override + protected void onPartialReduce(List shards, TotalHits totalHits, InternalAggregations aggs, int reducePhase) { + onPartialReduce.incrementAndGet(); + throw new UnsupportedOperationException(); + } + + @Override + protected void onFinalReduce(List shards, TotalHits totalHits, InternalAggregations aggs, int reducePhase) { + onFinalReduce.incrementAndGet(); + throw new UnsupportedOperationException(); + } + } +} From f74d18d39acb3bc17b2015104fc26ec5c44fb36b Mon Sep 17 00:00:00 2001 From: sabi0 <2sabio@gmail.com> Date: Fri, 8 Dec 2023 22:21:57 +0100 Subject: [PATCH 06/91] Fix typos in CoordinationDiagnosticsService (#103184) --- .../coordination/CoordinationDiagnosticsService.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationDiagnosticsService.java b/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationDiagnosticsService.java index fb5c9f2fea7de..229c34ecc1a14 100644 --- a/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationDiagnosticsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/coordination/CoordinationDiagnosticsService.java @@ -68,7 +68,7 @@ * this will report GREEN. * If we have had a master within the last 30 seconds, but that master has changed more than 3 times in the last 30 minutes (and that is * confirmed by checking with the last-known master), then this will report YELLOW. - * If we have not had a master within the last 30 seconds, then this will will report RED with one exception. That exception is when: + * If we have not had a master within the last 30 seconds, then this will report RED with one exception. That exception is when: * (1) no node is elected master, (2) this node is not master eligible, (3) some node is master eligible, (4) we ask a master-eligible node * to run this service, and (5) it comes back with a result that is not RED. * Since this service needs to be able to run when there is no master at all, it does not depend on the dedicated health node (which @@ -99,7 +99,7 @@ public class CoordinationDiagnosticsService implements ClusterStateListener { /* * This is a Map of tasks that are periodically reaching out to other master eligible nodes to get their ClusterFormationStates for - * diagnosis. The key is the DisoveryNode for the master eligible node being polled, and the value is a Cancellable. + * diagnosis. The key is the DiscoveryNode for the master eligible node being polled, and the value is a Cancellable. * The field is accessed (reads/writes) from multiple threads, but the reference itself is only ever changed on the cluster change * event thread. */ @@ -121,7 +121,7 @@ public class CoordinationDiagnosticsService implements ClusterStateListener { volatile AtomicReference remoteCoordinationDiagnosisTask = null; /* * This field holds the result of the task in the remoteCoordinationDiagnosisTask field above. The field is accessed - * (reads/writes) from multiple threads, but is only ever reassigned on a the initialization thread and the cluster change event thread. + * (reads/writes) from multiple threads, but is only ever reassigned on the initialization thread and the cluster change event thread. */ volatile AtomicReference remoteCoordinationDiagnosisResult = null; @@ -294,7 +294,7 @@ private static CoordinationDiagnosticsDetails getDetails( /** * Returns the health result when we have detected locally that the master has changed to null repeatedly (by default more than 3 times - * in the last 30 minutes). This method attemtps to use the master history from a remote node to confirm what we are seeing locally. + * in the last 30 minutes). This method attempts to use the master history from a remote node to confirm what we are seeing locally. * If the information from the remote node confirms that the master history has been unstable, a YELLOW status is returned. If the * information from the remote node shows that the master history has been stable, then we assume that the problem is with this node * and a GREEN status is returned (the problems with this node will be covered in a separate health indicator). If there had been @@ -1133,7 +1133,7 @@ private Scheduler.Cancellable sendTransportRequest ); responseConsumer.accept(responseTransformationFunction.apply(response, null)); }, e -> { - logger.warn("Exception in remote request to master" + masterEligibleNode, e); + logger.warn("Exception in remote request to master " + masterEligibleNode, e); responseConsumer.accept(responseTransformationFunction.apply(null, e)); })); @@ -1143,7 +1143,7 @@ public void run() { if (masterEligibleNode == null) { /* * This node's PeerFinder hasn't yet discovered the master-eligible nodes. By notifying the responseConsumer with a null - * value we effectively do nothing, and allow this request to be recheduled. + * value we effectively do nothing, and allow this request to be rescheduled. */ responseConsumer.accept(null); } else { From e46c83f6e431557b0ddae991a87a1866f06cd62e Mon Sep 17 00:00:00 2001 From: Keith Massey Date: Fri, 8 Dec 2023 15:44:50 -0600 Subject: [PATCH 07/91] fixing TransportReplicationAction --- .../support/replication/TransportReplicationAction.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java b/server/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java index 61a8be48d23a7..98fb778b2b1df 100644 --- a/server/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java @@ -692,6 +692,7 @@ public void onFailure(Exception e) { e ); replicaRequest.getRequest().onRetry(); + replicaRequest.incRef(); observer.waitForNextChange(new ClusterStateObserver.Listener() { @Override public void onNewClusterState(ClusterState state) { @@ -702,7 +703,7 @@ public void onNewClusterState(ClusterState state) { transportReplicaAction, replicaRequest, new ActionListenerResponseHandler<>( - onCompletionListener, + ActionListener.runAfter(onCompletionListener, replicaRequest::decRef), ReplicaResponse::new, TransportResponseHandler.TRANSPORT_WORKER ) @@ -711,7 +712,11 @@ public void onNewClusterState(ClusterState state) { @Override public void onClusterServiceClose() { - responseWithFailure(new NodeClosedException(clusterService.localNode())); + try { + responseWithFailure(new NodeClosedException(clusterService.localNode())); + } finally { + replicaRequest.decRef(); + } } @Override From 6fac62eb1433be2ec4e54d4afa66c4f79bdf26d0 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 8 Dec 2023 17:00:38 -0800 Subject: [PATCH 08/91] Update Lucene to remove terms enum optimzation (#103229) This commit updates Lucene to a patched version of 9.9.0 without https://github.com/apache/lucene/pull/12699. See https://github.com/apache/lucene/issues/12895. --- build-tools-internal/version.properties | 2 +- gradle/verification-metadata.xml | 120 ++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/build-tools-internal/version.properties b/build-tools-internal/version.properties index adf33dd070a22..c34bdc95046b3 100644 --- a/build-tools-internal/version.properties +++ b/build-tools-internal/version.properties @@ -1,5 +1,5 @@ elasticsearch = 8.13.0 -lucene = 9.9.0 +lucene = 9.9.0-snapshot-bb4fec631e6 bundled_jdk_vendor = openjdk bundled_jdk = 21.0.1+12@415e3f918a1f4062a0074a2794853d0d diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml index 7f672ece21f66..263602c9841a8 100644 --- a/gradle/verification-metadata.xml +++ b/gradle/verification-metadata.xml @@ -2664,121 +2664,241 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 3a1b0dc6c5732a7c2675f0a50837c8ca41cf19de Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 8 Dec 2023 22:03:28 -0800 Subject: [PATCH 09/91] Wrap painless explain error (#103151) In https://github.com/elastic/elasticsearch/pull/100872 Painless errors were wrapped so as to avoid throwing Errors outside scripting. However, one case was missed: PainlessExplainError which is used by Debug.explain. This commit adds the explain error to those that painless wraps. closes #103018 --- docs/changelog/103151.yaml | 6 ++++++ .../painless/ErrorCauseWrapper.java | 1 + .../org/elasticsearch/painless/DebugTests.java | 16 ++++++++++------ 3 files changed, 17 insertions(+), 6 deletions(-) create mode 100644 docs/changelog/103151.yaml diff --git a/docs/changelog/103151.yaml b/docs/changelog/103151.yaml new file mode 100644 index 0000000000000..bd9eea97cac6d --- /dev/null +++ b/docs/changelog/103151.yaml @@ -0,0 +1,6 @@ +pr: 103151 +summary: Wrap painless explain error +area: Infra/Scripting +type: bug +issues: + - 103018 diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java index aeaf44bfd014c..308d6223c666e 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java @@ -23,6 +23,7 @@ class ErrorCauseWrapper extends ElasticsearchException { private static final List> wrappedErrors = List.of( PainlessError.class, + PainlessExplainError.class, OutOfMemoryError.class, StackOverflowError.class, LinkageError.class diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DebugTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DebugTests.java index 87b199cd1b43f..48da785e801d3 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/DebugTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/DebugTests.java @@ -20,6 +20,7 @@ import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.not; @@ -30,29 +31,32 @@ public class DebugTests extends ScriptTestCase { public void testExplain() { // Debug.explain can explain an object Object dummy = new Object(); - PainlessExplainError e = expectScriptThrows( - PainlessExplainError.class, - () -> exec("Debug.explain(params.a)", singletonMap("a", dummy), true) - ); + var wrapper = expectScriptThrows(ErrorCauseWrapper.class, () -> exec("Debug.explain(params.a)", singletonMap("a", dummy), true)); + assertThat(wrapper.realCause.getClass(), equalTo(PainlessExplainError.class)); + var e = (PainlessExplainError) wrapper.realCause; assertSame(dummy, e.getObjectToExplain()); assertThat(e.getHeaders(painlessLookup), hasEntry("es.to_string", singletonList(dummy.toString()))); assertThat(e.getHeaders(painlessLookup), hasEntry("es.java_class", singletonList("java.lang.Object"))); assertThat(e.getHeaders(painlessLookup), hasEntry("es.painless_class", singletonList("java.lang.Object"))); // Null should be ok - e = expectScriptThrows(PainlessExplainError.class, () -> exec("Debug.explain(null)")); + wrapper = expectScriptThrows(ErrorCauseWrapper.class, () -> exec("Debug.explain(null)")); + assertThat(wrapper.realCause.getClass(), equalTo(PainlessExplainError.class)); + e = (PainlessExplainError) wrapper.realCause; assertNull(e.getObjectToExplain()); assertThat(e.getHeaders(painlessLookup), hasEntry("es.to_string", singletonList("null"))); assertThat(e.getHeaders(painlessLookup), not(hasKey("es.java_class"))); assertThat(e.getHeaders(painlessLookup), not(hasKey("es.painless_class"))); // You can't catch the explain exception - e = expectScriptThrows(PainlessExplainError.class, () -> exec(""" + wrapper = expectScriptThrows(ErrorCauseWrapper.class, () -> exec(""" try { Debug.explain(params.a) } catch (Exception e) { return 1 }""", singletonMap("a", dummy), true)); + assertThat(wrapper.realCause.getClass(), equalTo(PainlessExplainError.class)); + e = (PainlessExplainError) wrapper.realCause; assertSame(dummy, e.getObjectToExplain()); } From 80b222c3957942c9e11fd1043bf98ef773d862d0 Mon Sep 17 00:00:00 2001 From: John Verwolf Date: Sat, 9 Dec 2023 15:09:02 -0800 Subject: [PATCH 10/91] Update elasticsearch.modules.parent-join.internalClusterTest (#102189) Part of the broader work covered in https://github.com/elastic/elasticsearch/issues/102030 Updates tests in: - ChildQuerySearchIT - TokenCountFieldMapperIntegrationIT --- .../join/query/ChildQuerySearchIT.java | 27 ++++----- .../AggregationsIntegrationIT.java | 43 ++++++-------- .../elasticsearch/test/ESIntegTestCase.java | 2 +- .../hamcrest/ElasticsearchAssertions.java | 58 ++++++++++++++++++- 4 files changed, 83 insertions(+), 47 deletions(-) diff --git a/modules/parent-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java b/modules/parent-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java index ae1adf4160c2a..02776eb277020 100644 --- a/modules/parent-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java +++ b/modules/parent-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java @@ -12,7 +12,6 @@ import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchRequestBuilder; -import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchType; import org.elasticsearch.action.support.WriteRequest.RefreshPolicy; import org.elasticsearch.common.lucene.search.function.CombineFunction; @@ -69,6 +68,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailuresAndResponse; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertScrollResponsesAndHitCount; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHit; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasId; @@ -1403,22 +1403,15 @@ public void testParentChildQueriesViaScrollApi() throws Exception { boolQuery().must(matchAllQuery()).filter(hasParentQuery("parent", matchAllQuery(), false)) }; for (QueryBuilder query : queries) { - SearchResponse scrollResponse = prepareSearch("test").setScroll(TimeValue.timeValueSeconds(30)) - .setSize(1) - .addStoredField("_id") - .setQuery(query) - .get(); - - assertNoFailures(scrollResponse); - assertThat(scrollResponse.getHits().getTotalHits().value, equalTo(10L)); - int scannedDocs = 0; - do { - assertThat(scrollResponse.getHits().getTotalHits().value, equalTo(10L)); - scannedDocs += scrollResponse.getHits().getHits().length; - scrollResponse = client().prepareSearchScroll(scrollResponse.getScrollId()).setScroll(TimeValue.timeValueSeconds(30)).get(); - } while (scrollResponse.getHits().getHits().length > 0); - clearScroll(scrollResponse.getScrollId()); - assertThat(scannedDocs, equalTo(10)); + assertScrollResponsesAndHitCount( + TimeValue.timeValueSeconds(60), + prepareSearch("test").setScroll(TimeValue.timeValueSeconds(30)).setSize(1).addStoredField("_id").setQuery(query), + 10, + (respNum, response) -> { + assertNoFailures(response); + assertThat(response.getHits().getTotalHits().value, equalTo(10L)); + } + ); } } diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/AggregationsIntegrationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/AggregationsIntegrationIT.java index df8f3825a5ea6..a856ee36aadc2 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/AggregationsIntegrationIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/aggregations/AggregationsIntegrationIT.java @@ -18,7 +18,8 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.terms; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailuresAndResponse; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertScrollResponsesAndHitCount; @ESIntegTestCase.SuiteScopeTestCase public class AggregationsIntegrationIT extends ESIntegTestCase { @@ -38,32 +39,22 @@ public void setupSuiteScopeCluster() throws Exception { public void testScroll() { final int size = randomIntBetween(1, 4); - final String[] scroll = new String[1]; - final int[] total = new int[1]; - assertNoFailuresAndResponse( - prepareSearch("index").setSize(size).setScroll(TimeValue.timeValueMinutes(1)).addAggregation(terms("f").field("f")), - response -> { - Aggregations aggregations = response.getAggregations(); - assertNotNull(aggregations); - Terms terms = aggregations.get("f"); - assertEquals(Math.min(numDocs, 3L), terms.getBucketByKey("0").getDocCount()); - scroll[0] = response.getScrollId(); - total[0] = response.getHits().getHits().length; + assertScrollResponsesAndHitCount( + TimeValue.timeValueSeconds(60), + prepareSearch("index").setSize(size).addAggregation(terms("f").field("f")), + numDocs, + (respNum, response) -> { + assertNoFailures(response); + + if (respNum == 1) { // initial response. + Aggregations aggregations = response.getAggregations(); + assertNotNull(aggregations); + Terms terms = aggregations.get("f"); + assertEquals(Math.min(numDocs, 3L), terms.getBucketByKey("0").getDocCount()); + } else { + assertNull(response.getAggregations()); + } } ); - int currentTotal = 0; - while (total[0] - currentTotal > 0) { - currentTotal = total[0]; - assertNoFailuresAndResponse( - client().prepareSearchScroll(scroll[0]).setScroll(TimeValue.timeValueMinutes(1)), - scrollResponse -> { - assertNull(scrollResponse.getAggregations()); - total[0] += scrollResponse.getHits().getHits().length; - scroll[0] = scrollResponse.getScrollId(); - } - ); - } - clearScroll(scroll[0]); - assertEquals(numDocs, total[0]); } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index e0083d5570baa..779d846f4eac2 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -1881,7 +1881,7 @@ protected void addError(Exception e) { /** * Clears the given scroll Ids */ - public void clearScroll(String... scrollIds) { + public static void clearScroll(String... scrollIds) { ClearScrollResponse clearResponse = client().prepareClearScroll().setScrollIds(Arrays.asList(scrollIds)).get(); assertThat(clearResponse.isSucceeded(), equalTo(true)); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java index bba7a6e19deea..0d7ab26faecf9 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java +++ b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java @@ -25,6 +25,7 @@ import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchRequestBuilder; import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.action.search.SearchScrollRequestBuilder; import org.elasticsearch.action.search.ShardSearchFailure; import org.elasticsearch.action.support.DefaultShardOperationFailedException; import org.elasticsearch.action.support.broadcast.BaseBroadcastResponse; @@ -51,6 +52,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -60,10 +62,13 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.function.BiConsumer; import java.util.function.Consumer; import static org.apache.lucene.tests.util.LuceneTestCase.expectThrows; import static org.apache.lucene.tests.util.LuceneTestCase.expectThrowsAnyOf; +import static org.elasticsearch.test.ESIntegTestCase.clearScroll; +import static org.elasticsearch.test.ESIntegTestCase.client; import static org.elasticsearch.test.LambdaMatchers.transformedArrayItemsMatch; import static org.elasticsearch.test.LambdaMatchers.transformedItemsMatch; import static org.elasticsearch.test.LambdaMatchers.transformedMatch; @@ -73,6 +78,7 @@ import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.emptyArray; import static org.hamcrest.Matchers.greaterThan; @@ -369,6 +375,48 @@ public static void assertRes } } + /** + * A helper to enable the testing of scroll requests with ref-counting. + * + * @param keepAlive The TTL for the scroll context. + * @param searchRequestBuilder The initial search request. + * @param expectedTotalHitCount The number of hits that are expected to be retrieved. + * @param responseConsumer (respNum, response) -> {your assertions here}. + * respNum starts at 1, which contains the resp from the initial request. + */ + public static void assertScrollResponsesAndHitCount( + TimeValue keepAlive, + SearchRequestBuilder searchRequestBuilder, + int expectedTotalHitCount, + BiConsumer responseConsumer + ) { + searchRequestBuilder.setScroll(keepAlive); + List responses = new ArrayList<>(); + var scrollResponse = searchRequestBuilder.get(); + responses.add(scrollResponse); + int retrievedDocsCount = 0; + try { + assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) expectedTotalHitCount)); + retrievedDocsCount += scrollResponse.getHits().getHits().length; + responseConsumer.accept(responses.size(), scrollResponse); + while (scrollResponse.getHits().getHits().length > 0) { + scrollResponse = prepareScrollSearch(scrollResponse.getScrollId(), keepAlive).get(); + responses.add(scrollResponse); + assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) expectedTotalHitCount)); + retrievedDocsCount += scrollResponse.getHits().getHits().length; + responseConsumer.accept(responses.size(), scrollResponse); + } + } finally { + clearScroll(scrollResponse.getScrollId()); + responses.forEach(SearchResponse::decRef); + } + assertThat(retrievedDocsCount, equalTo(expectedTotalHitCount)); + } + + public static SearchScrollRequestBuilder prepareScrollSearch(String scrollId, TimeValue timeout) { + return client().prepareSearchScroll(scrollId).setScroll(timeout); + } + public static void assertResponse(ActionFuture responseFuture, Consumer consumer) throws ExecutionException, InterruptedException { var res = responseFuture.get(); @@ -442,6 +490,10 @@ public static void assertFailures(SearchRequestBuilder searchRequestBuilder, Res } } + public static void assertFailures(SearchRequestBuilder searchRequestBuilder, RestStatus restStatus) { + assertFailures(searchRequestBuilder, restStatus, containsString("")); + } + public static void assertNoFailures(BaseBroadcastResponse response) { if (response.getFailedShards() != 0) { final AssertionError assertionError = new AssertionError("[" + response.getFailedShards() + "] shard failures"); @@ -791,9 +843,9 @@ public static void assertToXContentEquivalent(BytesReference expected, BytesRefe * Often latches are called as assertTrue(latch.await(1, TimeUnit.SECONDS)); * In case of a failure this will just throw an assertion error without any further message * - * @param latch The latch to wait for - * @param timeout The value of the timeout - * @param unit The unit of the timeout + * @param latch The latch to wait for + * @param timeout The value of the timeout + * @param unit The unit of the timeout * @throws InterruptedException An exception if the waiting is interrupted */ public static void awaitLatch(CountDownLatch latch, long timeout, TimeUnit unit) throws InterruptedException { From 64818df6e8bee30944e34cde9aaa14a69a719ce2 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 10 Dec 2023 20:40:39 +0100 Subject: [PATCH 11/91] Fix SearchResponse leaks in searchable snapshot test and production code (#103118) It's in the title. Fix all the tests like we have everywhere else. Fix the production use of SearchResponse in BlobStoreCacheMaintenanceService by properly ref-counting the response it holds on to. --- .../search/SearchResponseUtils.java | 9 +- .../BaseSearchableSnapshotsIntegTestCase.java | 9 +- .../RetrySearchIntegTests.java | 42 +-- ...pshotsCanMatchOnCoordinatorIntegTests.java | 83 +++--- .../SearchableSnapshotsIntegTests.java | 58 ++-- ...napshotsRecoverFromSnapshotIntegTests.java | 10 +- ...archableSnapshotsRepositoryIntegTests.java | 18 +- ...ableSnapshotsBlobStoreCacheIntegTests.java | 9 +- ...tsBlobStoreCacheMaintenanceIntegTests.java | 17 +- .../shared/NodesCachesStatsIntegTests.java | 2 +- .../BlobStoreCacheMaintenanceService.java | 278 ++++++++++-------- 11 files changed, 270 insertions(+), 265 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java b/test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java index e61b89fcff42c..589bc76c55a3d 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java +++ b/test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java @@ -7,17 +7,22 @@ */ package org.elasticsearch.search; +import org.apache.lucene.search.TotalHits; import org.elasticsearch.action.search.SearchRequestBuilder; public enum SearchResponseUtils { ; - public static long getTotalHitsValue(SearchRequestBuilder request) { + public static TotalHits getTotalHits(SearchRequestBuilder request) { var resp = request.get(); try { - return resp.getHits().getTotalHits().value; + return resp.getHits().getTotalHits(); } finally { resp.decRef(); } } + + public static long getTotalHitsValue(SearchRequestBuilder request) { + return getTotalHits(request).value; + } } diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java index d3bb435dc03ab..b7dc212fe12ad 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java @@ -64,6 +64,7 @@ import static org.elasticsearch.license.LicenseSettings.SELF_GENERATED_LICENSE_TYPE; import static org.elasticsearch.test.NodeRoles.addRoles; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse; import static org.elasticsearch.xpack.searchablesnapshots.cache.common.TestUtils.pageAligned; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; @@ -289,10 +290,10 @@ protected void assertTotalHits(String indexName, TotalHits originalAllHits, Tota } catch (InterruptedException e) { throw new RuntimeException(e); } - allHits.set(t, prepareSearch(indexName).setTrackTotalHits(true).get().getHits().getTotalHits()); - barHits.set( - t, - prepareSearch(indexName).setTrackTotalHits(true).setQuery(matchQuery("foo", "bar")).get().getHits().getTotalHits() + assertResponse(prepareSearch(indexName).setTrackTotalHits(true), resp -> allHits.set(t, resp.getHits().getTotalHits())); + assertResponse( + prepareSearch(indexName).setTrackTotalHits(true).setQuery(matchQuery("foo", "bar")), + resp -> barHits.set(t, resp.getHits().getTotalHits()) ); }); threads[i].start(); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/RetrySearchIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/RetrySearchIntegTests.java index 0551ac3007f10..c80cf3c3d62e3 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/RetrySearchIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/RetrySearchIntegTests.java @@ -9,7 +9,6 @@ import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.ClosePointInTimeRequest; import org.elasticsearch.action.search.OpenPointInTimeRequest; -import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchType; import org.elasticsearch.action.search.TransportClosePointInTimeAction; import org.elasticsearch.action.search.TransportOpenPointInTimeAction; @@ -32,7 +31,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailuresAndResponse; import static org.hamcrest.Matchers.equalTo; public class RetrySearchIntegTests extends BaseSearchableSnapshotsIntegTestCase { @@ -144,30 +143,31 @@ public void testRetryPointInTime() throws Exception { ).keepAlive(TimeValue.timeValueMinutes(2)); final String pitId = client().execute(TransportOpenPointInTimeAction.TYPE, openRequest).actionGet().getPointInTimeId(); try { - SearchResponse resp = prepareSearch().setIndices(indexName) - .setPreference(null) - .setPointInTime(new PointInTimeBuilder(pitId)) - .get(); - assertNoFailures(resp); - assertThat(resp.pointInTimeId(), equalTo(pitId)); - assertHitCount(resp, docCount); - + assertNoFailuresAndResponse( + prepareSearch().setIndices(indexName).setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)), + resp -> { + assertThat(resp.pointInTimeId(), equalTo(pitId)); + assertHitCount(resp, docCount); + } + ); final Set allocatedNodes = internalCluster().nodesInclude(indexName); for (String allocatedNode : allocatedNodes) { internalCluster().restartNode(allocatedNode); } ensureGreen(indexName); - resp = prepareSearch().setIndices(indexName) - .setQuery(new RangeQueryBuilder("created_date").gte("2011-01-01").lte("2011-12-12")) - .setSearchType(SearchType.QUERY_THEN_FETCH) - .setPreference(null) - .setPreFilterShardSize(between(1, 10)) - .setAllowPartialSearchResults(true) - .setPointInTime(new PointInTimeBuilder(pitId)) - .get(); - assertNoFailures(resp); - assertThat(resp.pointInTimeId(), equalTo(pitId)); - assertHitCount(resp, docCount); + assertNoFailuresAndResponse( + prepareSearch().setIndices(indexName) + .setQuery(new RangeQueryBuilder("created_date").gte("2011-01-01").lte("2011-12-12")) + .setSearchType(SearchType.QUERY_THEN_FETCH) + .setPreference(null) + .setPreFilterShardSize(between(1, 10)) + .setAllowPartialSearchResults(true) + .setPointInTime(new PointInTimeBuilder(pitId)), + resp -> { + assertThat(resp.pointInTimeId(), equalTo(pitId)); + assertHitCount(resp, docCount); + } + ); } finally { client().execute(TransportClosePointInTimeAction.TYPE, new ClosePointInTimeRequest(pitId)).actionGet(); } diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java index 844e6099460b2..a7a3b8e461604 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java @@ -52,6 +52,7 @@ import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_ROUTING_REQUIRE_GROUP_SETTING; import static org.elasticsearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.lessThanOrEqualTo; @@ -182,14 +183,14 @@ public void testSearchableSnapshotShardsAreSkippedBySearchRequestWithoutQuerying .source(new SearchSourceBuilder().query(rangeQuery)); if (includeIndexCoveringSearchRangeInSearchRequest) { - SearchResponse searchResponse = client().search(request).actionGet(); - - // All the regular index searches succeeded - assertThat(searchResponse.getSuccessfulShards(), equalTo(indexWithinSearchRangeShardCount)); - // All the searchable snapshots shard search failed - assertThat(searchResponse.getFailedShards(), equalTo(indexOutsideSearchRangeShardCount)); - assertThat(searchResponse.getSkippedShards(), equalTo(0)); - assertThat(searchResponse.getTotalShards(), equalTo(totalShards)); + assertResponse(client().search(request), searchResponse -> { + // All the regular index searches succeeded + assertThat(searchResponse.getSuccessfulShards(), equalTo(indexWithinSearchRangeShardCount)); + // All the searchable snapshots shard search failed + assertThat(searchResponse.getFailedShards(), equalTo(indexOutsideSearchRangeShardCount)); + assertThat(searchResponse.getSkippedShards(), equalTo(0)); + assertThat(searchResponse.getTotalShards(), equalTo(totalShards)); + }); } else { // All shards failed, since all shards are unassigned and the IndexMetadata min/max timestamp // is not available yet @@ -271,13 +272,13 @@ public void testSearchableSnapshotShardsAreSkippedBySearchRequestWithoutQuerying waitUntilAllShardsAreUnassigned(updatedIndexMetadata.getIndex()); if (includeIndexCoveringSearchRangeInSearchRequest) { - SearchResponse newSearchResponse = client().search(request).actionGet(); - - assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount)); - assertThat(newSearchResponse.getSuccessfulShards(), equalTo(totalShards)); - assertThat(newSearchResponse.getFailedShards(), equalTo(0)); - assertThat(newSearchResponse.getTotalShards(), equalTo(totalShards)); - assertThat(newSearchResponse.getHits().getTotalHits().value, equalTo((long) numDocsWithinRange)); + assertResponse(client().search(request), newSearchResponse -> { + assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount)); + assertThat(newSearchResponse.getSuccessfulShards(), equalTo(totalShards)); + assertThat(newSearchResponse.getFailedShards(), equalTo(0)); + assertThat(newSearchResponse.getTotalShards(), equalTo(totalShards)); + assertThat(newSearchResponse.getHits().getTotalHits().value, equalTo((long) numDocsWithinRange)); + }); // test with SearchShardsAPI { @@ -338,13 +339,14 @@ public void testSearchableSnapshotShardsAreSkippedBySearchRequestWithoutQuerying } } } else { - SearchResponse newSearchResponse = client().search(request).actionGet(); - // When all shards are skipped, at least one of them should be queried in order to - // provide a proper search response. - assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount - 1)); - assertThat(newSearchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount - 1)); - assertThat(newSearchResponse.getFailedShards(), equalTo(1)); - assertThat(newSearchResponse.getTotalShards(), equalTo(indexOutsideSearchRangeShardCount)); + assertResponse(client().search(request), newSearchResponse -> { + // When all shards are skipped, at least one of them should be queried in order to + // provide a proper search response. + assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount - 1)); + assertThat(newSearchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount - 1)); + assertThat(newSearchResponse.getFailedShards(), equalTo(1)); + assertThat(newSearchResponse.getTotalShards(), equalTo(indexOutsideSearchRangeShardCount)); + }); // test with SearchShardsAPI { @@ -449,14 +451,15 @@ public void testQueryPhaseIsExecutedInAnAvailableNodeWhenAllShardsCanBeSkipped() // test with Search API { - SearchResponse searchResponse = client().search(request).actionGet(); - // All the regular index searches succeeded - assertThat(searchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount)); - // All the searchable snapshots shard search failed - assertThat(searchResponse.getFailedShards(), equalTo(indexOutsideSearchRangeShardCount)); - assertThat(searchResponse.getSkippedShards(), equalTo(searchableSnapshotShardCount)); - assertThat(searchResponse.getTotalShards(), equalTo(totalShards)); - assertThat(searchResponse.getHits().getTotalHits().value, equalTo(0L)); + assertResponse(client().search(request), searchResponse -> { + // All the regular index searches succeeded + assertThat(searchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount)); + // All the searchable snapshots shard search failed + assertThat(searchResponse.getFailedShards(), equalTo(indexOutsideSearchRangeShardCount)); + assertThat(searchResponse.getSkippedShards(), equalTo(searchableSnapshotShardCount)); + assertThat(searchResponse.getTotalShards(), equalTo(totalShards)); + assertThat(searchResponse.getHits().getTotalHits().value, equalTo(0L)); + }); } // test with SearchShards API @@ -513,16 +516,16 @@ public void testQueryPhaseIsExecutedInAnAvailableNodeWhenAllShardsCanBeSkipped() // busy assert since computing the time stamp field from the cluster state happens off of the CS applier thread and thus can be // slightly delayed assertBusy(() -> { - SearchResponse newSearchResponse = client().search(request).actionGet(); - - // All the regular index searches succeeded - assertThat(newSearchResponse.getSuccessfulShards(), equalTo(totalShards)); - assertThat(newSearchResponse.getFailedShards(), equalTo(0)); - // We have to query at least one node to construct a valid response, and we pick - // a shard that's available in order to construct the search response - assertThat(newSearchResponse.getSkippedShards(), equalTo(totalShards - 1)); - assertThat(newSearchResponse.getTotalShards(), equalTo(totalShards)); - assertThat(newSearchResponse.getHits().getTotalHits().value, equalTo(0L)); + assertResponse(client().search(request), newSearchResponse -> { + // All the regular index searches succeeded + assertThat(newSearchResponse.getSuccessfulShards(), equalTo(totalShards)); + assertThat(newSearchResponse.getFailedShards(), equalTo(0)); + // We have to query at least one node to construct a valid response, and we pick + // a shard that's available in order to construct the search response + assertThat(newSearchResponse.getSkippedShards(), equalTo(totalShards - 1)); + assertThat(newSearchResponse.getTotalShards(), equalTo(totalShards)); + assertThat(newSearchResponse.getHits().getTotalHits().value, equalTo(0L)); + }); }); // test with SearchShards API diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java index c3f5e44ae32a0..876ff9ebdb86f 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java @@ -46,6 +46,7 @@ import org.elasticsearch.indices.IndicesService; import org.elasticsearch.repositories.RepositoryData; import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.search.SearchResponseUtils; import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.snapshots.SnapshotInfo; import org.elasticsearch.snapshots.SnapshotsService; @@ -117,19 +118,12 @@ public void testCreateAndRestoreSearchableSnapshot() throws Exception { populateIndex(indexName, 10_000); - final TotalHits originalAllHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .get() - .getHits() - .getTotalHits(); - final TotalHits originalBarHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .setQuery(matchQuery("foo", "bar")) - .get() - .getHits() - .getTotalHits(); + final TotalHits originalAllHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true) + ); + final TotalHits originalBarHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true).setQuery(matchQuery("foo", "bar")) + ); logger.info("--> [{}] in total, of which [{}] match the query", originalAllHits, originalBarHits); expectThrows( @@ -765,19 +759,12 @@ public void testSnapshotOfSearchableSnapshotIncludesNoDataButCanBeRestored() thr Settings.builder().put(INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1).put(INDEX_SOFT_DELETES_SETTING.getKey(), true) ); - final TotalHits originalAllHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .get() - .getHits() - .getTotalHits(); - final TotalHits originalBarHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .setQuery(matchQuery("foo", "bar")) - .get() - .getHits() - .getTotalHits(); + final TotalHits originalAllHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true) + ); + final TotalHits originalBarHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true).setQuery(matchQuery("foo", "bar")) + ); logger.info("--> [{}] in total, of which [{}] match the query", originalAllHits, originalBarHits); // The repository that contains the actual data @@ -936,19 +923,12 @@ public void testSnapshotOfSearchableSnapshotCanBeRestoredBeforeRepositoryRegiste Settings.builder().put(INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1).put(INDEX_SOFT_DELETES_SETTING.getKey(), true) ); - final TotalHits originalAllHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .get() - .getHits() - .getTotalHits(); - final TotalHits originalBarHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .setQuery(matchQuery("foo", "bar")) - .get() - .getHits() - .getTotalHits(); + final TotalHits originalAllHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true) + ); + final TotalHits originalBarHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true).setQuery(matchQuery("foo", "bar")) + ); logger.info("--> [{}] in total, of which [{}] match the query", originalAllHits, originalBarHits); // Take snapshot containing the actual data to one repository diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRecoverFromSnapshotIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRecoverFromSnapshotIntegTests.java index 6f71f7c33bf06..894d3af8d75b8 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRecoverFromSnapshotIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRecoverFromSnapshotIntegTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.indices.recovery.plan.ShardSnapshotsService; import org.elasticsearch.repositories.blobstore.BlobStoreRepository; import org.elasticsearch.repositories.fs.FsRepository; +import org.elasticsearch.search.SearchResponseUtils; import org.elasticsearch.test.MockLogAppender; import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest; @@ -43,12 +44,9 @@ public void testSearchableSnapshotRelocationDoNotUseSnapshotBasedRecoveries() th .put(INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0) ); - final TotalHits totalHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .get() - .getHits() - .getTotalHits(); + final TotalHits totalHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true) + ); final var snapshotName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT); createSnapshot(repositoryName, snapshotName, List.of(indexName)); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRepositoryIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRepositoryIntegTests.java index cb6cf45b641c6..f97151f9ae330 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRepositoryIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsRepositoryIntegTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.core.Nullable; import org.elasticsearch.repositories.RepositoryConflictException; import org.elasticsearch.repositories.fs.FsRepository; +import org.elasticsearch.search.SearchResponseUtils; import org.elasticsearch.snapshots.SnapshotRestoreException; import java.util.Arrays; @@ -49,12 +50,9 @@ public void testRepositoryUsedBySearchableSnapshotCanBeUpdatedButNotUnregistered Settings.builder().put(INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1).put(INDEX_SOFT_DELETES_SETTING.getKey(), true) ); - final TotalHits totalHits = internalCluster().client() - .prepareSearch(indexName) - .setTrackTotalHits(true) - .get() - .getHits() - .getTotalHits(); + final TotalHits totalHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(indexName).setTrackTotalHits(true) + ); final String snapshotName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT); createSnapshot(repositoryName, snapshotName, List.of(indexName)); @@ -164,7 +162,9 @@ public void testMountIndexWithDifferentDeletionOfSnapshot() throws Exception { final String index = "index"; createAndPopulateIndex(index, Settings.builder().put(INDEX_SOFT_DELETES_SETTING.getKey(), true)); - final TotalHits totalHits = internalCluster().client().prepareSearch(index).setTrackTotalHits(true).get().getHits().getTotalHits(); + final TotalHits totalHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(index).setTrackTotalHits(true) + ); final String snapshot = "snapshot"; createSnapshot(repository, snapshot, List.of(index)); @@ -220,7 +220,9 @@ public void testDeletionOfSnapshotSettingCannotBeUpdated() throws Exception { final String index = "index"; createAndPopulateIndex(index, Settings.builder().put(INDEX_SOFT_DELETES_SETTING.getKey(), true)); - final TotalHits totalHits = internalCluster().client().prepareSearch(index).setTrackTotalHits(true).get().getHits().getTotalHits(); + final TotalHits totalHits = SearchResponseUtils.getTotalHits( + internalCluster().client().prepareSearch(index).setTrackTotalHits(true) + ); final String snapshot = "snapshot"; createSnapshot(repository, snapshot, List.of(index)); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheIntegTests.java index b5ebf1104a195..37b3ecfd36959 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheIntegTests.java @@ -37,6 +37,7 @@ import org.elasticsearch.plugins.ClusterPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.reindex.ReindexPlugin; +import org.elasticsearch.search.SearchResponseUtils; import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.test.InternalTestCluster; import org.elasticsearch.xcontent.XContentBuilder; @@ -207,11 +208,9 @@ public void testBlobStoreCache() throws Exception { refreshSystemIndex(); - final long numberOfCachedBlobs = systemClient().prepareSearch(SNAPSHOT_BLOB_CACHE_INDEX) - .setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN) - .get() - .getHits() - .getTotalHits().value; + final long numberOfCachedBlobs = SearchResponseUtils.getTotalHitsValue( + systemClient().prepareSearch(SNAPSHOT_BLOB_CACHE_INDEX).setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN) + ); IndexingStats indexingStats = systemClient().admin() .indices() diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheMaintenanceIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheMaintenanceIntegTests.java index 04233e47b7bcc..981ffe2832e66 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheMaintenanceIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheMaintenanceIntegTests.java @@ -29,6 +29,7 @@ import org.elasticsearch.reindex.ReindexPlugin; import org.elasticsearch.repositories.IndexId; import org.elasticsearch.repositories.fs.FsRepository; +import org.elasticsearch.search.SearchResponseUtils; import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; @@ -318,16 +319,12 @@ private Client systemClient() { } private long numberOfEntriesInCache() { - var res = systemClient().prepareSearch(SNAPSHOT_BLOB_CACHE_INDEX) - .setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN) - .setTrackTotalHits(true) - .setSize(0) - .get(); - try { - return res.getHits().getTotalHits().value; - } finally { - res.decRef(); - } + return SearchResponseUtils.getTotalHitsValue( + systemClient().prepareSearch(SNAPSHOT_BLOB_CACHE_INDEX) + .setIndicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN) + .setTrackTotalHits(true) + .setSize(0) + ); } private void refreshSystemIndex(boolean failIfNotExist) { diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/NodesCachesStatsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/NodesCachesStatsIntegTests.java index 3858b087f4d3a..42ac63579b6c6 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/NodesCachesStatsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/NodesCachesStatsIntegTests.java @@ -112,7 +112,7 @@ public void testNodesCachesStats() throws Exception { randomBoolean() ? QueryBuilders.rangeQuery("id").gte(randomIntBetween(0, 1000)) : QueryBuilders.termQuery("test", "value" + randomIntBetween(0, 1000)) - ).setSize(randomIntBetween(0, 1000)).get(); + ).setSize(randomIntBetween(0, 1000)).get().decRef(); } assertExecutorIsIdle(SearchableSnapshots.CACHE_FETCH_ASYNC_THREAD_POOL_NAME); diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/BlobStoreCacheMaintenanceService.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/BlobStoreCacheMaintenanceService.java index 64508e1d49959..89cab65765bf9 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/BlobStoreCacheMaintenanceService.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/blob/BlobStoreCacheMaintenanceService.java @@ -417,7 +417,7 @@ private class PeriodicMaintenanceTask implements Runnable, Releasable { private volatile Map> existingSnapshots; private volatile Set existingRepositories; - private volatile SearchResponse searchResponse; + private final AtomicReference searchResponse = new AtomicReference<>(); private volatile Instant expirationTime; private volatile String pointIntTimeId; private volatile Object[] searchAfter; @@ -458,146 +458,155 @@ public void onFailure(Exception e) { final String pitId = pointIntTimeId; assert Strings.hasLength(pitId); - if (searchResponse == null) { - final SearchSourceBuilder searchSource = new SearchSourceBuilder(); - searchSource.fetchField(new FieldAndFormat(CachedBlob.CREATION_TIME_FIELD, "epoch_millis")); - searchSource.fetchSource(false); - searchSource.trackScores(false); - searchSource.sort(ShardDocSortField.NAME); - searchSource.size(batchSize); - if (searchAfter != null) { - searchSource.searchAfter(searchAfter); - searchSource.trackTotalHits(false); - } else { - searchSource.trackTotalHits(true); + SearchResponse searchResponseRef; + do { + searchResponseRef = searchResponse.get(); + if (searchResponseRef == null) { + handleMissingSearchResponse(pitId); + return; } - final PointInTimeBuilder pointInTime = new PointInTimeBuilder(pitId); - pointInTime.setKeepAlive(keepAlive); - searchSource.pointInTimeBuilder(pointInTime); - final SearchRequest searchRequest = new SearchRequest(); - searchRequest.source(searchSource); - clientWithOrigin.execute(TransportSearchAction.TYPE, searchRequest, new ActionListener<>() { - @Override - public void onResponse(SearchResponse response) { - if (searchAfter == null) { - assert PeriodicMaintenanceTask.this.total.get() == 0L; - PeriodicMaintenanceTask.this.total.set(response.getHits().getTotalHits().value); - } - PeriodicMaintenanceTask.this.searchResponse = response; - PeriodicMaintenanceTask.this.searchAfter = null; - executeNext(PeriodicMaintenanceTask.this); - } - - @Override - public void onFailure(Exception e) { - complete(e); - } - }); - return; + } while (searchResponseRef.tryIncRef() == false); + try { + var searchHits = searchResponseRef.getHits().getHits(); + if (searchHits != null && searchHits.length > 0) { + updateWithSearchHits(searchHits); + return; + } + } finally { + searchResponseRef.decRef(); } + // we're done, complete the task + complete(null); + } catch (Exception e) { + complete(e); + } + } - final SearchHit[] searchHits = searchResponse.getHits().getHits(); - if (searchHits != null && searchHits.length > 0) { - if (expirationTime == null) { - final TimeValue retention = periodicTaskRetention; - expirationTime = Instant.ofEpochMilli(threadPool.absoluteTimeInMillis()) - .minus(retention.duration(), retention.timeUnit().toChronoUnit()); - - final ClusterState state = clusterService.state(); - // compute the list of existing searchable snapshots and repositories once - existingSnapshots = listSearchableSnapshots(state); - existingRepositories = RepositoriesMetadata.get(state) - .repositories() - .stream() - .map(RepositoryMetadata::name) - .collect(Collectors.toSet()); + private void handleMissingSearchResponse(String pitId) { + final SearchSourceBuilder searchSource = new SearchSourceBuilder(); + searchSource.fetchField(new FieldAndFormat(CachedBlob.CREATION_TIME_FIELD, "epoch_millis")); + searchSource.fetchSource(false); + searchSource.trackScores(false); + searchSource.sort(ShardDocSortField.NAME); + searchSource.size(batchSize); + if (searchAfter != null) { + searchSource.searchAfter(searchAfter); + searchSource.trackTotalHits(false); + } else { + searchSource.trackTotalHits(true); + } + final PointInTimeBuilder pointInTime = new PointInTimeBuilder(pitId); + pointInTime.setKeepAlive(keepAlive); + searchSource.pointInTimeBuilder(pointInTime); + final SearchRequest searchRequest = new SearchRequest(); + searchRequest.source(searchSource); + clientWithOrigin.execute(TransportSearchAction.TYPE, searchRequest, new ActionListener<>() { + @Override + public void onResponse(SearchResponse response) { + if (searchAfter == null) { + assert PeriodicMaintenanceTask.this.total.get() == 0L; + PeriodicMaintenanceTask.this.total.set(response.getHits().getTotalHits().value); } + PeriodicMaintenanceTask.this.setCurrentResponse(response); + PeriodicMaintenanceTask.this.searchAfter = null; + executeNext(PeriodicMaintenanceTask.this); + } - final BulkRequest bulkRequest = new BulkRequest(); - final Map> knownSnapshots = existingSnapshots; - assert knownSnapshots != null; - final Set knownRepositories = existingRepositories; - assert knownRepositories != null; - final Instant expirationTimeCopy = this.expirationTime; - assert expirationTimeCopy != null; - - Object[] lastSortValues = null; - for (SearchHit searchHit : searchHits) { - lastSortValues = searchHit.getSortValues(); - assert searchHit.getId() != null; - try { - boolean delete = false; - - // See {@link BlobStoreCacheService#generateId} - // doc id = {repository name}/{snapshot id}/{snapshot index id}/{shard id}/{file name}/@{file offset} - final String[] parts = Objects.requireNonNull(searchHit.getId()).split("/"); - assert parts.length == 6 : Arrays.toString(parts) + " vs " + searchHit.getId(); - - final String repositoryName = parts[0]; - if (knownRepositories.contains(repositoryName) == false) { - logger.trace("deleting blob store cache entry with id [{}]: repository does not exist", searchHit.getId()); - delete = true; - } else { - final Set knownIndexIds = knownSnapshots.get(parts[1]); - if (knownIndexIds == null || knownIndexIds.contains(parts[2]) == false) { - logger.trace("deleting blob store cache entry with id [{}]: not used", searchHit.getId()); - delete = true; - } - } - if (delete) { - final Instant creationTime = getCreationTime(searchHit); - if (creationTime.isAfter(expirationTimeCopy)) { - logger.trace( - "blob store cache entry with id [{}] was created recently, skipping deletion", - searchHit.getId() - ); - continue; - } - bulkRequest.add(new DeleteRequest().index(searchHit.getIndex()).id(searchHit.getId())); - } - } catch (Exception e) { - logger.warn( - () -> format("exception when parsing blob store cache entry with id [%s], skipping", searchHit.getId()), - e - ); + @Override + public void onFailure(Exception e) { + complete(e); + } + }); + } + + private void updateWithSearchHits(SearchHit[] searchHits) { + if (expirationTime == null) { + final TimeValue retention = periodicTaskRetention; + expirationTime = Instant.ofEpochMilli(threadPool.absoluteTimeInMillis()) + .minus(retention.duration(), retention.timeUnit().toChronoUnit()); + + final ClusterState state = clusterService.state(); + // compute the list of existing searchable snapshots and repositories once + existingSnapshots = listSearchableSnapshots(state); + existingRepositories = RepositoriesMetadata.get(state) + .repositories() + .stream() + .map(RepositoryMetadata::name) + .collect(Collectors.toSet()); + } + + final BulkRequest bulkRequest = new BulkRequest(); + final Map> knownSnapshots = existingSnapshots; + assert knownSnapshots != null; + final Set knownRepositories = existingRepositories; + assert knownRepositories != null; + final Instant expirationTimeCopy = this.expirationTime; + assert expirationTimeCopy != null; + + Object[] lastSortValues = null; + for (SearchHit searchHit : searchHits) { + lastSortValues = searchHit.getSortValues(); + assert searchHit.getId() != null; + try { + boolean delete = false; + + // See {@link BlobStoreCacheService#generateId} + // doc id = {repository name}/{snapshot id}/{snapshot index id}/{shard id}/{file name}/@{file offset} + final String[] parts = Objects.requireNonNull(searchHit.getId()).split("/"); + assert parts.length == 6 : Arrays.toString(parts) + " vs " + searchHit.getId(); + + final String repositoryName = parts[0]; + if (knownRepositories.contains(repositoryName) == false) { + logger.trace("deleting blob store cache entry with id [{}]: repository does not exist", searchHit.getId()); + delete = true; + } else { + final Set knownIndexIds = knownSnapshots.get(parts[1]); + if (knownIndexIds == null || knownIndexIds.contains(parts[2]) == false) { + logger.trace("deleting blob store cache entry with id [{}]: not used", searchHit.getId()); + delete = true; } } - - assert lastSortValues != null; - if (bulkRequest.numberOfActions() == 0) { - this.searchResponse = null; - this.searchAfter = lastSortValues; - executeNext(this); - return; + if (delete) { + final Instant creationTime = getCreationTime(searchHit); + if (creationTime.isAfter(expirationTimeCopy)) { + logger.trace("blob store cache entry with id [{}] was created recently, skipping deletion", searchHit.getId()); + continue; + } + bulkRequest.add(new DeleteRequest().index(searchHit.getIndex()).id(searchHit.getId())); } + } catch (Exception e) { + logger.warn(() -> format("exception when parsing blob store cache entry with id [%s], skipping", searchHit.getId()), e); + } + } - final Object[] finalSearchAfter = lastSortValues; - clientWithOrigin.execute(BulkAction.INSTANCE, bulkRequest, new ActionListener<>() { - @Override - public void onResponse(BulkResponse response) { - for (BulkItemResponse itemResponse : response.getItems()) { - if (itemResponse.isFailed() == false) { - assert itemResponse.getResponse() instanceof DeleteResponse; - PeriodicMaintenanceTask.this.deletes.incrementAndGet(); - } - } - PeriodicMaintenanceTask.this.searchResponse = null; - PeriodicMaintenanceTask.this.searchAfter = finalSearchAfter; - executeNext(PeriodicMaintenanceTask.this); - } + assert lastSortValues != null; + if (bulkRequest.numberOfActions() == 0) { + setCurrentResponse(null); + this.searchAfter = lastSortValues; + executeNext(this); + return; + } - @Override - public void onFailure(Exception e) { - complete(e); + final Object[] finalSearchAfter = lastSortValues; + clientWithOrigin.execute(BulkAction.INSTANCE, bulkRequest, new ActionListener<>() { + @Override + public void onResponse(BulkResponse response) { + for (BulkItemResponse itemResponse : response.getItems()) { + if (itemResponse.isFailed() == false) { + assert itemResponse.getResponse() instanceof DeleteResponse; + deletes.incrementAndGet(); } - }); - return; + } + PeriodicMaintenanceTask.this.setCurrentResponse(null); + PeriodicMaintenanceTask.this.searchAfter = finalSearchAfter; + executeNext(PeriodicMaintenanceTask.this); } - // we're done, complete the task - complete(null); - } catch (Exception e) { - complete(e); - } + + @Override + public void onFailure(Exception e) { + complete(e); + } + }); } public boolean isClosed() { @@ -614,6 +623,7 @@ private void ensureOpen() { @Override public void close() { if (closed.compareAndSet(false, true)) { + setCurrentResponse(null); final Exception e = error.get(); if (e != null) { logger.warn( @@ -679,6 +689,16 @@ public void onFailure(Exception e) { } } } + + private void setCurrentResponse(SearchResponse response) { + if (response != null) { + response.mustIncRef(); + } + var previous = searchResponse.getAndSet(response); + if (previous != null) { + previous.decRef(); + } + } } private void executeNext(PeriodicMaintenanceTask maintenanceTask) { From 50d541869a95f6cef74f0f8ce47528a696abe17a Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 10 Dec 2023 22:27:58 +0100 Subject: [PATCH 12/91] Remove redundant try-catch from some listeners in TransportStartDatafeedAction (#103246) Found this during some listener logic cleanup: We use ActionListener.wrap here already so any exception will make it to onFailure. The way this is set up throwing in onFailure will lead to another call to onFailure, which seems like it could cause or at least hide bugs. --- .../action/TransportStartDatafeedAction.java | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java index b02f6339e49c0..65ef493f664f9 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java @@ -269,28 +269,20 @@ public void onFailure(Exception e) { }; ActionListener jobListener = ActionListener.wrap(jobBuilder -> { - try { - Job job = jobBuilder.build(); - validate(job, datafeedConfigHolder.get(), tasks, xContentRegistry); - auditDeprecations(datafeedConfigHolder.get(), job, auditor, xContentRegistry); - createDataExtractor.accept(job); - } catch (Exception e) { - listener.onFailure(e); - } + Job job = jobBuilder.build(); + validate(job, datafeedConfigHolder.get(), tasks, xContentRegistry); + auditDeprecations(datafeedConfigHolder.get(), job, auditor, xContentRegistry); + createDataExtractor.accept(job); }, listener::onFailure); ActionListener datafeedListener = ActionListener.wrap(datafeedBuilder -> { - try { - DatafeedConfig datafeedConfig = datafeedBuilder.build(); - params.setDatafeedIndices(datafeedConfig.getIndices()); - params.setJobId(datafeedConfig.getJobId()); - params.setIndicesOptions(datafeedConfig.getIndicesOptions()); - datafeedConfigHolder.set(datafeedConfig); - - jobConfigProvider.getJob(datafeedConfig.getJobId(), null, jobListener); - } catch (Exception e) { - listener.onFailure(e); - } + DatafeedConfig datafeedConfig = datafeedBuilder.build(); + params.setDatafeedIndices(datafeedConfig.getIndices()); + params.setJobId(datafeedConfig.getJobId()); + params.setIndicesOptions(datafeedConfig.getIndicesOptions()); + datafeedConfigHolder.set(datafeedConfig); + + jobConfigProvider.getJob(datafeedConfig.getJobId(), null, jobListener); }, listener::onFailure); datafeedConfigProvider.getDatafeedConfig(params.getDatafeedId(), null, datafeedListener); From 86933ad2f3255002365616897f476808b68d1d97 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 10 Dec 2023 22:31:34 +0100 Subject: [PATCH 13/91] Remove deprecated + unused ActionListener#wrap variant (#103243) This isn't used anymore. --- .../java/org/elasticsearch/action/ActionListener.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ActionListener.java b/server/src/main/java/org/elasticsearch/action/ActionListener.java index 5017f0af0007c..ae5835686425d 100644 --- a/server/src/main/java/org/elasticsearch/action/ActionListener.java +++ b/server/src/main/java/org/elasticsearch/action/ActionListener.java @@ -145,15 +145,6 @@ public String toString() { }; } - /** - * @deprecated in favour of {@link #running(Runnable)} because this implementation doesn't "wrap" exceptions from {@link #onResponse} - * into {@link #onFailure}. - */ - @Deprecated(forRemoval = true) - static ActionListener wrap(Runnable runnable) { - return running(runnable); - } - /** * Creates a listener that executes the appropriate consumer when the response (or failure) is received. This listener is "wrapped" in * the sense that an exception from the {@code onResponse} consumer is passed into the {@code onFailure} consumer. From 394444cd27f2a355959c9c302fedbda53626315b Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 11 Dec 2023 16:01:29 +1100 Subject: [PATCH 14/91] [Docs] Custom S3 CA must be reinstalled on upgrade (#103168) This commit updates the docs to call out that custom certificate authorities for S3 repositories will need to be reinstalled every time ES is upgraded, is the node is using the bundled JDK --- docs/reference/snapshot-restore/repository-s3.asciidoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/reference/snapshot-restore/repository-s3.asciidoc b/docs/reference/snapshot-restore/repository-s3.asciidoc index 032d4f47bf678..e204061c28458 100644 --- a/docs/reference/snapshot-restore/repository-s3.asciidoc +++ b/docs/reference/snapshot-restore/repository-s3.asciidoc @@ -123,7 +123,9 @@ settings belong in the `elasticsearch.yml` file. `https`. Defaults to `https`. When using HTTPS, this repository type validates the repository's certificate chain using the JVM-wide truststore. Ensure that the root certificate authority is in this truststore using the JVM's - `keytool` tool. + `keytool` tool. If you have a custom certificate authority for your S3 repository + and you use the {es} <>, then you will need to reinstall your + CA certificate every time you upgrade {es}. `proxy.host`:: From da231d9623bab65dd110f5a477081a7edd34a09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lorenzo=20Dematt=C3=A9?= Date: Mon, 11 Dec 2023 09:28:52 +0100 Subject: [PATCH 15/91] Fix: Watcher API watcher_get_settings clears product header (#103003) Current code do stashing and un-stashing of the context to ignore warning headers about accessing a system index. However, this ignores all header, omitting the X-elastic-product header from the response. This PR removes the stashing/un-stashing and instead calls the concreteIndexNames variant that allows system index access (which does not generate the warning), concreteIndexNamesWithSystemIndexAccess --- docs/changelog/103003.yaml | 6 +++ .../TransportGetWatcherSettingsAction.java | 19 +++---- .../TransportUpdateWatcherSettingsAction.java | 52 +++++++++++-------- 3 files changed, 43 insertions(+), 34 deletions(-) create mode 100644 docs/changelog/103003.yaml diff --git a/docs/changelog/103003.yaml b/docs/changelog/103003.yaml new file mode 100644 index 0000000000000..accacc2b62416 --- /dev/null +++ b/docs/changelog/103003.yaml @@ -0,0 +1,6 @@ +pr: 103003 +summary: "Fix: Watcher REST API `GET /_watcher/settings` now includes product header" +area: "Watcher" +type: bug +issues: + - 102928 diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportGetWatcherSettingsAction.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportGetWatcherSettingsAction.java index eecbc21ad0475..0c52057100860 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportGetWatcherSettingsAction.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportGetWatcherSettingsAction.java @@ -9,7 +9,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; -import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.master.TransportMasterNodeAction; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlockException; @@ -20,7 +19,6 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.EsExecutors; -import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; @@ -28,6 +26,7 @@ import org.elasticsearch.xpack.core.watcher.transport.actions.put.UpdateWatcherSettingsAction; import static org.elasticsearch.xpack.watcher.transport.actions.TransportUpdateWatcherSettingsAction.WATCHER_INDEX_NAME; +import static org.elasticsearch.xpack.watcher.transport.actions.TransportUpdateWatcherSettingsAction.WATCHER_INDEX_REQUEST; public class TransportGetWatcherSettingsAction extends TransportMasterNodeAction< GetWatcherSettingsAction.Request, @@ -61,15 +60,11 @@ protected void masterOperation( ClusterState state, ActionListener listener ) { - final ThreadContext threadContext = threadPool.getThreadContext(); - // Stashing and un-stashing the context allows warning headers about accessing a system index to be ignored. - try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { - IndexMetadata metadata = state.metadata().index(WATCHER_INDEX_NAME); - if (metadata == null) { - listener.onResponse(new GetWatcherSettingsAction.Response(Settings.EMPTY)); - } else { - listener.onResponse(new GetWatcherSettingsAction.Response(filterSettableSettings(metadata.getSettings()))); - } + IndexMetadata metadata = state.metadata().index(WATCHER_INDEX_NAME); + if (metadata == null) { + listener.onResponse(new GetWatcherSettingsAction.Response(Settings.EMPTY)); + } else { + listener.onResponse(new GetWatcherSettingsAction.Response(filterSettableSettings(metadata.getSettings()))); } } @@ -95,7 +90,7 @@ protected ClusterBlockException checkBlock(GetWatcherSettingsAction.Request requ return state.blocks() .indicesBlockedException( ClusterBlockLevel.METADATA_READ, - indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.LENIENT_EXPAND_OPEN, WATCHER_INDEX_NAME) + indexNameExpressionResolver.concreteIndexNamesWithSystemIndexAccess(state, WATCHER_INDEX_REQUEST) ); } } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java index 124cb6de5fd40..b9895b2319599 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transport/actions/TransportUpdateWatcherSettingsAction.java @@ -9,6 +9,7 @@ import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.IndicesRequest; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsClusterStateUpdateRequest; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.IndicesOptions; @@ -24,7 +25,6 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.EsExecutors; -import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.index.Index; import org.elasticsearch.logging.LogManager; import org.elasticsearch.logging.Logger; @@ -37,7 +37,19 @@ public class TransportUpdateWatcherSettingsAction extends TransportMasterNodeAct UpdateWatcherSettingsAction.Request, AcknowledgedResponse> { - public static final String WATCHER_INDEX_NAME = ".watches"; + static final String WATCHER_INDEX_NAME = ".watches"; + + static final IndicesRequest WATCHER_INDEX_REQUEST = new IndicesRequest() { + @Override + public String[] indices() { + return new String[] { WATCHER_INDEX_NAME }; + } + + @Override + public IndicesOptions indicesOptions() { + return IndicesOptions.LENIENT_EXPAND_OPEN; + } + }; private static final Logger logger = LogManager.getLogger(TransportUpdateWatcherSettingsAction.class); private final MetadataUpdateSettingsService updateSettingsService; @@ -83,27 +95,23 @@ protected void masterOperation( new Index[] { watcherIndexMd.getIndex() } ).settings(newSettings).ackTimeout(request.timeout()).masterNodeTimeout(request.masterNodeTimeout()); - final ThreadContext threadContext = threadPool.getThreadContext(); - // Stashing and un-stashing the context allows warning headers about accessing a system index to be ignored. - try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { - updateSettingsService.updateSettings(clusterStateUpdateRequest, new ActionListener<>() { - @Override - public void onResponse(AcknowledgedResponse acknowledgedResponse) { - if (acknowledgedResponse.isAcknowledged()) { - logger.info("successfully updated Watcher service settings to {}", request.settings()); - } else { - logger.warn("updating Watcher service settings to {} was not acknowledged", request.settings()); - } - listener.onResponse(acknowledgedResponse); + updateSettingsService.updateSettings(clusterStateUpdateRequest, new ActionListener<>() { + @Override + public void onResponse(AcknowledgedResponse acknowledgedResponse) { + if (acknowledgedResponse.isAcknowledged()) { + logger.info("successfully updated Watcher service settings to {}", request.settings()); + } else { + logger.warn("updating Watcher service settings to {} was not acknowledged", request.settings()); } + listener.onResponse(acknowledgedResponse); + } - @Override - public void onFailure(Exception e) { - logger.debug(() -> "failed to update settings for Watcher service", e); - listener.onFailure(e); - } - }); - } + @Override + public void onFailure(Exception e) { + logger.debug(() -> "failed to update settings for Watcher service", e); + listener.onFailure(e); + } + }); } @Override @@ -115,7 +123,7 @@ protected ClusterBlockException checkBlock(UpdateWatcherSettingsAction.Request r return state.blocks() .indicesBlockedException( ClusterBlockLevel.METADATA_WRITE, - indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.LENIENT_EXPAND_OPEN, WATCHER_INDEX_NAME) + indexNameExpressionResolver.concreteIndexNamesWithSystemIndexAccess(state, WATCHER_INDEX_REQUEST) ); } } From cd2e2946fa7da88ba5095e418be82d5f100b9f18 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 11 Dec 2023 08:37:40 +0000 Subject: [PATCH 16/91] Fix format string in OldLuceneVersions (#103185) The message introduced in #92986 used `{}` instead of `%s` for the placeholders, which doesn't work for `Strings.format`. This commit fixes the problem. --- docs/changelog/103185.yaml | 5 +++++ .../elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 docs/changelog/103185.yaml diff --git a/docs/changelog/103185.yaml b/docs/changelog/103185.yaml new file mode 100644 index 0000000000000..3a1a4960ba98c --- /dev/null +++ b/docs/changelog/103185.yaml @@ -0,0 +1,5 @@ +pr: 103185 +summary: Fix format string in `OldLuceneVersions` +area: Search +type: bug +issues: [] diff --git a/x-pack/plugin/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java b/x-pack/plugin/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java index 406ea50315de0..42fe09691d249 100644 --- a/x-pack/plugin/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java +++ b/x-pack/plugin/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java @@ -170,8 +170,8 @@ private static void convertToNewFormat(IndexShard indexShard) { throw new UncheckedIOException( Strings.format( """ - Elasticsearch version [{}] has limited support for indices created with version [{}] but this index could not be \ - read. It may be using an unsupported feature, or it may be damaged or corrupt. See {} for further information.""", + Elasticsearch version [%s] has limited support for indices created with version [%s] but this index could not be \ + read. It may be using an unsupported feature, or it may be damaged or corrupt. See %s for further information.""", Build.current().version(), IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(indexShard.indexSettings().getSettings()), ReferenceDocs.ARCHIVE_INDICES From dd0d9eca48504a082507f31fb3350752f456998a Mon Sep 17 00:00:00 2001 From: Tim Grein Date: Mon, 11 Dec 2023 10:16:15 +0100 Subject: [PATCH 17/91] [Connectors API] Check connector sync job stats after they were updated using another get request Assert that the connector sync job stats were updated in integration tests --- .../460_connector_sync_job_update_stats.yml | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/460_connector_sync_job_update_stats.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/460_connector_sync_job_update_stats.yml index 0e69866ce8b6c..0c7300bd2b436 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/460_connector_sync_job_update_stats.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/460_connector_sync_job_update_stats.yml @@ -2,6 +2,7 @@ setup: - skip: version: " - 8.11.99" reason: Introduced in 8.12.0 + - do: connector.put: connector_id: test-connector @@ -20,7 +21,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -31,6 +34,13 @@ setup: - match: { acknowledged: true } + - do: + connector_sync_job.get: + connector_sync_job_id: $id + + - match: { deleted_document_count: 10 } + - match: { indexed_document_count: 20 } + - match: { indexed_document_volume: 1000 } --- "Update the ingestion stats for a connector sync job - negative deleted document count error": @@ -40,7 +50,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -50,7 +62,6 @@ setup: indexed_document_volume: 1000 catch: bad_request - --- "Update the ingestion stats for a connector sync job - negative indexed document count error": - do: @@ -59,7 +70,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -69,7 +82,6 @@ setup: indexed_document_volume: 1000 catch: bad_request - --- "Update the ingestion stats for a connector sync job - negative indexed document volume error": - do: @@ -78,7 +90,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -96,7 +110,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -115,7 +131,9 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -127,6 +145,14 @@ setup: - match: { acknowledged: true } + - do: + connector_sync_job.get: + connector_sync_job_id: $id + + - match: { deleted_document_count: 10 } + - match: { indexed_document_count: 20 } + - match: { indexed_document_volume: 1000 } + - match: { total_document_count: 20 } --- "Update the ingestion stats for a connector sync job - with optional last_seen": @@ -137,6 +163,7 @@ setup: job_type: full trigger_method: on_demand - set: { id: id } + - do: connector_sync_job.update_stats: connector_sync_job_id: $id @@ -148,6 +175,15 @@ setup: - match: { acknowledged: true } + - do: + connector_sync_job.get: + connector_sync_job_id: $id + + - match: { deleted_document_count: 10 } + - match: { indexed_document_count: 20 } + - match: { indexed_document_volume: 1000 } + - match: { last_seen: 2023-12-04T08:45:50.567149Z } + --- "Update the ingestion stats for a Connector Sync Job - Connector Sync Job does not exist": - do: From ce9e91937e256598d2b861b63953800c2a723d15 Mon Sep 17 00:00:00 2001 From: Tim Grein Date: Mon, 11 Dec 2023 10:19:00 +0100 Subject: [PATCH 18/91] [Connectors API] Verify that last_seen was updated in the check-in integration tests (#103194) Extend the check-in integration tests to verify that last_seen was updated correctly after a check-in. --- .../entsearch/420_connector_sync_job_check_in.yml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/420_connector_sync_job_check_in.yml b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/420_connector_sync_job_check_in.yml index 9ef37f4a9fe60..d08f7f6a51c91 100644 --- a/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/420_connector_sync_job_check_in.yml +++ b/x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/420_connector_sync_job_check_in.yml @@ -2,6 +2,7 @@ setup: - skip: version: " - 8.11.99" reason: Introduced in 8.12.0 + features: is_after - do: connector.put: connector_id: test-connector @@ -20,13 +21,26 @@ setup: id: test-connector job_type: full trigger_method: on_demand + - set: { id: sync-job-id-to-check-in } + + - do: + connector_sync_job.get: + connector_sync_job_id: $sync-job-id-to-check-in + + - set: { last_seen: last_seen_before_check_in } + - do: connector_sync_job.check_in: connector_sync_job_id: $sync-job-id-to-check-in - match: { acknowledged: true } + - do: + connector_sync_job.get: + connector_sync_job_id: $sync-job-id-to-check-in + + - is_after: { last_seen: $last_seen_before_check_in } --- "Check in a Connector Sync Job - Connector Sync Job does not exist": From 784c49b2b809d33bde6aac922f218dd98173594a Mon Sep 17 00:00:00 2001 From: Tim Grein Date: Mon, 11 Dec 2023 11:18:59 +0100 Subject: [PATCH 19/91] [Connectors API] Fix: Handle nullable fields correctly in the ConnectorSyncJob parser (#103183) Declare nullable fields correctly in the ConnectorSyncJob parser. --- docs/changelog/103183.yaml | 6 ++ .../connector/syncjob/ConnectorSyncJob.java | 31 +++++-- .../syncjob/ConnectorSyncJobTests.java | 92 ++++++++++++++++++- 3 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 docs/changelog/103183.yaml diff --git a/docs/changelog/103183.yaml b/docs/changelog/103183.yaml new file mode 100644 index 0000000000000..cb28033cff6a7 --- /dev/null +++ b/docs/changelog/103183.yaml @@ -0,0 +1,6 @@ +pr: 103183 +summary: "[Connectors API] Handle nullable fields correctly in the `ConnectorSyncJob`\ + \ parser" +area: Application +type: bug +issues: [] diff --git a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJob.java b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJob.java index f14d0fa52b1c7..c63cb1921adc6 100644 --- a/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJob.java +++ b/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJob.java @@ -263,12 +263,22 @@ public ConnectorSyncJob(StreamInput in) throws IOException { static { PARSER.declareField( optionalConstructorArg(), - (p, c) -> Instant.parse(p.text()), + (p, c) -> parseNullableInstant(p), CANCELATION_REQUESTED_AT_FIELD, ObjectParser.ValueType.STRING_OR_NULL ); - PARSER.declareField(optionalConstructorArg(), (p, c) -> Instant.parse(p.text()), CANCELED_AT_FIELD, ObjectParser.ValueType.STRING); - PARSER.declareField(optionalConstructorArg(), (p, c) -> Instant.parse(p.text()), COMPLETED_AT_FIELD, ObjectParser.ValueType.STRING); + PARSER.declareField( + optionalConstructorArg(), + (p, c) -> parseNullableInstant(p), + CANCELED_AT_FIELD, + ObjectParser.ValueType.STRING_OR_NULL + ); + PARSER.declareField( + optionalConstructorArg(), + (p, c) -> parseNullableInstant(p), + COMPLETED_AT_FIELD, + ObjectParser.ValueType.STRING_OR_NULL + ); PARSER.declareField( constructorArg(), (p, c) -> ConnectorSyncJob.syncJobConnectorFromXContent(p), @@ -287,9 +297,14 @@ public ConnectorSyncJob(StreamInput in) throws IOException { JOB_TYPE_FIELD, ObjectParser.ValueType.STRING ); - PARSER.declareField(constructorArg(), (p, c) -> Instant.parse(p.text()), LAST_SEEN_FIELD, ObjectParser.ValueType.STRING); + PARSER.declareField(constructorArg(), (p, c) -> parseNullableInstant(p), LAST_SEEN_FIELD, ObjectParser.ValueType.STRING_OR_NULL); PARSER.declareField(constructorArg(), (p, c) -> p.map(), METADATA_FIELD, ObjectParser.ValueType.OBJECT); - PARSER.declareField(optionalConstructorArg(), (p, c) -> Instant.parse(p.text()), STARTED_AT_FIELD, ObjectParser.ValueType.STRING); + PARSER.declareField( + optionalConstructorArg(), + (p, c) -> parseNullableInstant(p), + STARTED_AT_FIELD, + ObjectParser.ValueType.STRING_OR_NULL + ); PARSER.declareField( constructorArg(), (p, c) -> ConnectorSyncStatus.fromString(p.text()), @@ -303,7 +318,11 @@ public ConnectorSyncJob(StreamInput in) throws IOException { TRIGGER_METHOD_FIELD, ObjectParser.ValueType.STRING ); - PARSER.declareString(optionalConstructorArg(), WORKER_HOSTNAME_FIELD); + PARSER.declareStringOrNull(optionalConstructorArg(), WORKER_HOSTNAME_FIELD); + } + + private static Instant parseNullableInstant(XContentParser p) throws IOException { + return p.currentToken() == XContentParser.Token.VALUE_NULL ? null : Instant.parse(p.text()); } @SuppressWarnings("unchecked") diff --git a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJobTests.java b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJobTests.java index 49a3f0c4ad043..04629b6ee9751 100644 --- a/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJobTests.java +++ b/x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/connector/syncjob/ConnectorSyncJobTests.java @@ -158,7 +158,7 @@ public void testFromXContent_WithAllFields_AllSet() throws IOException { assertThat(syncJob.getWorkerHostname(), equalTo("worker-hostname")); } - public void testFromXContent_WithAllNonOptionalFieldsSet_DoesNotThrow() throws IOException { + public void testFromXContent_WithOnlyNonNullableFieldsSet_DoesNotThrow() throws IOException { String content = XContentHelper.stripWhitespace(""" { "connector": { @@ -242,6 +242,96 @@ public void testFromXContent_WithAllNonOptionalFieldsSet_DoesNotThrow() throws I ConnectorSyncJob.fromXContentBytes(new BytesArray(content), XContentType.JSON); } + public void testFromXContent_WithAllNullableFieldsSetToNull_DoesNotThrow() throws IOException { + String content = XContentHelper.stripWhitespace(""" + { + "cancelation_requested_at": null, + "canceled_at": null, + "completed_at": null, + "connector": { + "id": "connector-id", + "filtering": [ + { + "active": { + "advanced_snippet": { + "created_at": "2023-12-01T14:18:37.397819Z", + "updated_at": "2023-12-01T14:18:37.397819Z", + "value": {} + }, + "rules": [ + { + "created_at": "2023-12-01T14:18:37.397819Z", + "field": "_", + "id": "DEFAULT", + "order": 0, + "policy": "include", + "rule": "regex", + "updated_at": "2023-12-01T14:18:37.397819Z", + "value": ".*" + } + ], + "validation": { + "errors": [], + "state": "valid" + } + }, + "domain": "DEFAULT", + "draft": { + "advanced_snippet": { + "created_at": "2023-12-01T14:18:37.397819Z", + "updated_at": "2023-12-01T14:18:37.397819Z", + "value": {} + }, + "rules": [ + { + "created_at": "2023-12-01T14:18:37.397819Z", + "field": "_", + "id": "DEFAULT", + "order": 0, + "policy": "include", + "rule": "regex", + "updated_at": "2023-12-01T14:18:37.397819Z", + "value": ".*" + } + ], + "validation": { + "errors": [], + "state": "valid" + } + } + } + ], + "index_name": "search-connector", + "language": "english", + "pipeline": { + "extract_binary_content": true, + "name": "ent-search-generic-ingestion", + "reduce_whitespace": true, + "run_ml_inference": false + }, + "service_type": "service type", + "configuration": {} + }, + "created_at": "2023-12-01T14:18:43.07693Z", + "deleted_document_count": 10, + "error": null, + "id": "HIC-JYwB9RqKhB7x_hIE", + "indexed_document_count": 10, + "indexed_document_volume": 10, + "job_type": "full", + "last_seen": null, + "metadata": {}, + "started_at": null, + "status": "canceling", + "total_document_count": 0, + "trigger_method": "scheduled", + "worker_hostname": null + } + """); + + ConnectorSyncJob.fromXContentBytes(new BytesArray(content), XContentType.JSON); + } + private void assertTransportSerialization(ConnectorSyncJob testInstance) throws IOException { ConnectorSyncJob deserializedInstance = copyInstance(testInstance); assertNotSame(testInstance, deserializedInstance); From 7ddf450d1961296445315c185c84b2dffb6873a7 Mon Sep 17 00:00:00 2001 From: Abdon Pijpelink Date: Mon, 11 Dec 2023 11:23:27 +0100 Subject: [PATCH 20/91] [DOCS] Empty keys for ES|QL DISSECT (#102632) * [DOCS] Empty keys for ES|QL DISSECT * Update test --- ...sql-process-data-with-dissect-grok.asciidoc | 18 ++++++------------ .../src/main/resources/docs.csv-spec | 1 - 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/docs/reference/esql/esql-process-data-with-dissect-grok.asciidoc b/docs/reference/esql/esql-process-data-with-dissect-grok.asciidoc index a13633a9f8d92..87748fee4f202 100644 --- a/docs/reference/esql/esql-process-data-with-dissect-grok.asciidoc +++ b/docs/reference/esql/esql-process-data-with-dissect-grok.asciidoc @@ -62,9 +62,8 @@ clientip:keyword | @timestamp:keyword | status:keyword include::../ingest/processors/dissect.asciidoc[tag=intro-example-explanation] -A <> can be used to match values, but -exclude the value from the output. -// TODO: Change back to original text when https://github.com/elastic/elasticsearch/pull/102580 is merged +An empty key (`%{}`) or <> can be used to +match values, but exclude the value from the output. All matched values are output as keyword string data types. Use the <> to convert to another data type. @@ -137,8 +136,6 @@ include::{esql-specs}/docs.csv-spec[tag=dissectRightPaddingModifier] include::{esql-specs}/docs.csv-spec[tag=dissectRightPaddingModifier-result] |=== -//// -// TODO: Re-enable when https://github.com/elastic/elasticsearch/pull/102580 is merged include::../ingest/processors/dissect.asciidoc[tag=dissect-modifier-empty-right-padding] For example: @@ -150,7 +147,6 @@ include::{esql-specs}/docs.csv-spec[tag=dissectEmptyRightPaddingModifier] |=== include::{esql-specs}/docs.csv-spec[tag=dissectEmptyRightPaddingModifier-result] |=== -//// [[esql-append-modifier]] ====== Append modifier (`+`) @@ -180,11 +176,9 @@ include::{esql-specs}/docs.csv-spec[tag=dissectAppendWithOrderModifier-result] [[esql-named-skip-key]] ====== Named skip key (`?`) -// include::../ingest/processors/dissect.asciidoc[tag=named-skip-key] -// TODO: Re-enable when https://github.com/elastic/elasticsearch/pull/102580 is merged - -Dissect supports ignoring matches in the final result. This can be done with a -named skip key using the `{?name}` syntax: +include::../ingest/processors/dissect.asciidoc[tag=named-skip-key] +This can be done with a named skip key using the `{?name}` syntax. In the +following query, `ident` and `auth` are not added to the output table: [source.merge.styled,esql] ---- @@ -199,7 +193,7 @@ include::{esql-specs}/docs.csv-spec[tag=dissectNamedSkipKey-result] ===== Limitations // tag::dissect-limitations[] -The `DISSECT` command does not support reference keys and empty keys. +The `DISSECT` command does not support reference keys. // end::dissect-limitations[] [[esql-process-data-with-grok]] diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-spec index a754194739992..ef3e43aa6d8ab 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/docs.csv-spec @@ -574,7 +574,6 @@ dissectEmptyRightPaddingModifier ROW message="[1998-08-10T17:15:42] [WARN]" | DISSECT message "[%{ts}]%{->}[%{level}]" // end::dissectEmptyRightPaddingModifier[] -| KEEP message, ts, level ; // tag::dissectEmptyRightPaddingModifier-result[] From df450348f7e02782415fba33551c867bada9521a Mon Sep 17 00:00:00 2001 From: Simon Cooper Date: Mon, 11 Dec 2023 10:41:59 +0000 Subject: [PATCH 21/91] Close rather than stop HttpServerTransport on shutdown (#102759) Also add a clearer exception message when a lifecycle object is starting when the ES process is shutting down --- docs/changelog/102759.yaml | 6 +++++ .../bootstrap/Elasticsearch.java | 2 ++ .../bootstrap/ElasticsearchProcess.java | 24 +++++++++++++++++++ .../common/component/Lifecycle.java | 8 ++++++- .../java/org/elasticsearch/node/Node.java | 5 +--- .../org/elasticsearch/node/NodeTests.java | 7 ++++++ 6 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 docs/changelog/102759.yaml create mode 100644 server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchProcess.java diff --git a/docs/changelog/102759.yaml b/docs/changelog/102759.yaml new file mode 100644 index 0000000000000..1c002ef2b678e --- /dev/null +++ b/docs/changelog/102759.yaml @@ -0,0 +1,6 @@ +pr: 102759 +summary: Close rather than stop `HttpServerTransport` on shutdown +area: Infra/Node Lifecycle +type: bug +issues: + - 102501 diff --git a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java index eb2c2b7f6738e..dd60a2085acc1 100644 --- a/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java +++ b/server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java @@ -460,6 +460,8 @@ private void start() throws NodeValidationException { } private static void shutdown() { + ElasticsearchProcess.markStopping(); + if (INSTANCE == null) { return; // never got far enough } diff --git a/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchProcess.java b/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchProcess.java new file mode 100644 index 0000000000000..7397bb98322f5 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/bootstrap/ElasticsearchProcess.java @@ -0,0 +1,24 @@ +/* + * 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.bootstrap; + +/** + * Helper class to determine if the ES process is shutting down + */ +public class ElasticsearchProcess { + private static volatile boolean stopping; + + static void markStopping() { + stopping = true; + } + + public static boolean isStopping() { + return stopping; + } +} diff --git a/server/src/main/java/org/elasticsearch/common/component/Lifecycle.java b/server/src/main/java/org/elasticsearch/common/component/Lifecycle.java index 793975048f846..1488963ab2644 100644 --- a/server/src/main/java/org/elasticsearch/common/component/Lifecycle.java +++ b/server/src/main/java/org/elasticsearch/common/component/Lifecycle.java @@ -8,6 +8,8 @@ package org.elasticsearch.common.component; +import org.elasticsearch.bootstrap.ElasticsearchProcess; + /** * Lifecycle state. Allows the following transitions: *