Release password from NodesReloadSecureSettingsRequest sooner (#103757)

Today the coordinating node retains the password until the request is
complete. This commit integrates the password's lifecycle with the
lifecycle of the corresponding transport requests so that it can be
released as soon as it's no longer needed.

It also removes some unnecessary junk from the node-level messages.

server/src/internalClusterTest/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

@@ -65,9 +65,14 @@ private static void executeReloadSecureSettings(
         SecureString password,
         ActionListener<NodesReloadSecureSettingsResponse> listener
     ) {
-        final var request = new NodesReloadSecureSettingsRequest(nodeIds);
-        request.setSecureStorePassword(password);
-        client().execute(TransportNodesReloadSecureSettingsAction.TYPE, request, listener);
+        final var request = new NodesReloadSecureSettingsRequest();
+        try {
+            request.nodesIds(nodeIds);
+            request.setSecureStorePassword(password);
+            client().execute(TransportNodesReloadSecureSettingsAction.TYPE, request, listener);
+        } finally {
+            request.decRef();
+        }
     }
 
     private static SecureString emptyPassword() {

server/src/main/java/org/elasticsearch/TransportVersions.java

@@ -176,6 +176,7 @@ static TransportVersion def(int id) {
     public static final TransportVersion ESQL_STATUS_INCLUDE_LUCENE_QUERIES = def(8_564_00_0);
     public static final TransportVersion ESQL_CLUSTER_ALIAS = def(8_565_00_0);
     public static final TransportVersion SNAPSHOTS_IN_PROGRESS_TRACKING_REMOVING_NODES_ADDED = def(8_566_00_0);
+    public static final TransportVersion SMALLER_RELOAD_SECURE_SETTINGS_REQUEST = def(8_567_00_0);
 
     /*
      * STOP! READ THIS FIRST! No, really,

server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

@@ -8,26 +8,32 @@
 
 package org.elasticsearch.action.admin.cluster.node.reload;
 
+import org.elasticsearch.TransportVersions;
+import org.elasticsearch.action.support.TransportAction;
 import org.elasticsearch.action.support.nodes.BaseNodesRequest;
+import org.elasticsearch.cluster.node.DiscoveryNode;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.SecureString;
+import org.elasticsearch.core.AbstractRefCounted;
 import org.elasticsearch.core.CharArrays;
 import org.elasticsearch.core.Nullable;
-import org.elasticsearch.core.Releasable;
+import org.elasticsearch.core.RefCounted;
+import org.elasticsearch.core.Releasables;
+import org.elasticsearch.tasks.TaskId;
+import org.elasticsearch.transport.LeakTracker;
 import org.elasticsearch.transport.TransportRequest;
 
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Request for a reload secure settings action
  */
-public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> implements Releasable {
+public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> {
 
     /**
      * The password is used to re-read and decrypt the contents
@@ -37,39 +43,12 @@ public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesRelo
     @Nullable
     private SecureString secureSettingsPassword;
 
+    private final RefCounted refs = LeakTracker.wrap(AbstractRefCounted.of(() -> Releasables.close(secureSettingsPassword)));
+
     public NodesReloadSecureSettingsRequest() {
         super((String[]) null);
     }
 
-    public NodesReloadSecureSettingsRequest(StreamInput in) throws IOException {
-        super(in);
-        final BytesReference bytesRef = in.readOptionalBytesReference();
-        if (bytesRef != null) {
-            byte[] bytes = BytesReference.toBytes(bytesRef);
-            try {
-                this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(bytes));
-            } finally {
-                Arrays.fill(bytes, (byte) 0);
-            }
-        } else {
-            this.secureSettingsPassword = null;
-        }
-    }
-
-    /**
-     * Reload secure settings only on certain nodes, based on the nodes ids
-     * specified. If none are passed, secure settings will be reloaded on all the
-     * nodes.
-     */
-    public NodesReloadSecureSettingsRequest(String... nodesIds) {
-        super(nodesIds);
-    }
-
-    @Nullable
-    public SecureString getSecureSettingsPassword() {
-        return secureSettingsPassword;
-    }
-
     public void setSecureStorePassword(SecureString secureStorePassword) {
         this.secureSettingsPassword = secureStorePassword;
     }
@@ -80,64 +59,126 @@ boolean hasPassword() {
 
     @Override
     public void writeTo(StreamOutput out) throws IOException {
-        super.writeTo(out);
-        if (this.secureSettingsPassword == null) {
-            out.writeOptionalBytesReference(null);
-        } else {
-            final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars());
-            try {
-                out.writeOptionalBytesReference(new BytesArray(passwordBytes));
-            } finally {
-                Arrays.fill(passwordBytes, (byte) 0);
-            }
-        }
+        TransportAction.localOnly();
     }
 
-    // This field is intentionally not part of serialization
-    private final Set<NodeRequest> nodeRequests = ConcurrentHashMap.newKeySet();
+    @Override
+    public void incRef() {
+        refs.incRef();
+    }
 
-    NodeRequest newNodeRequest() {
-        final NodesReloadSecureSettingsRequest clone = new NodesReloadSecureSettingsRequest(nodesIds());
-        if (hasPassword()) {
-            clone.setSecureStorePassword(getSecureSettingsPassword().clone());
-        }
-        final NodeRequest nodeRequest = new NodeRequest(clone);
-        nodeRequests.add(nodeRequest);
-        return nodeRequest;
+    @Override
+    public boolean tryIncRef() {
+        return refs.tryIncRef();
     }
 
     @Override
-    public void close() {
-        if (this.secureSettingsPassword != null) {
-            this.secureSettingsPassword.close();
-        }
-        nodeRequests.forEach(NodeRequest::close);
+    public boolean decRef() {
+        return refs.decRef();
+    }
+
+    @Override
+    public boolean hasReferences() {
+        return refs.hasReferences();
+    }
+
+    NodeRequest newNodeRequest() {
+        refs.mustIncRef();
+        return new NodeRequest(secureSettingsPassword, refs);
     }
 
-    public static class NodeRequest extends TransportRequest implements Releasable {
+    public static class NodeRequest extends TransportRequest {
+
+        @Nullable
+        private final SecureString secureSettingsPassword;
 
-        // TODO don't wrap the whole top-level request, it contains heavy and irrelevant DiscoveryNode things; see #100878
-        NodesReloadSecureSettingsRequest request;
+        private final RefCounted refs;
 
         NodeRequest(StreamInput in) throws IOException {
             super(in);
-            request = new NodesReloadSecureSettingsRequest(in);
+
+            if (in.getTransportVersion().before(TransportVersions.SMALLER_RELOAD_SECURE_SETTINGS_REQUEST)) {
+                TaskId.readFromStream(in);
+                in.readStringArray();
+                in.readOptionalArray(DiscoveryNode::new, DiscoveryNode[]::new);
+                in.readOptionalTimeValue();
+            }
+
+            final BytesReference bytesRef = in.readOptionalBytesReference();
+            if (bytesRef != null) {
+                byte[] bytes = BytesReference.toBytes(bytesRef);
+                try {
+                    this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(bytes));
+                    this.refs = LeakTracker.wrap(AbstractRefCounted.of(() -> Releasables.close(this.secureSettingsPassword)));
+                } finally {
+                    Arrays.fill(bytes, (byte) 0);
+                }
+            } else {
+                this.secureSettingsPassword = null;
+                this.refs = LeakTracker.wrap(AbstractRefCounted.of(() -> {}));
+            }
         }
 
-        NodeRequest(NodesReloadSecureSettingsRequest request) {
-            this.request = request;
+        NodeRequest(@Nullable SecureString secureSettingsPassword, RefCounted refs) {
+            assert secureSettingsPassword == null || secureSettingsPassword.getChars() != null; // ensures it's not closed
+            assert refs.hasReferences();
+            this.secureSettingsPassword = secureSettingsPassword;
+            this.refs = refs;
         }
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
+            assert hasReferences();
             super.writeTo(out);
-            request.writeTo(out);
+
+            if (out.getTransportVersion().before(TransportVersions.SMALLER_RELOAD_SECURE_SETTINGS_REQUEST)) {
+                TaskId.EMPTY_TASK_ID.writeTo(out);
+                out.writeStringArray(Strings.EMPTY_ARRAY);
+                out.writeOptionalArray(StreamOutput::writeWriteable, null);
+                out.writeOptionalTimeValue(null);
+            }
+
+            if (this.secureSettingsPassword == null) {
+                out.writeOptionalBytesReference(null);
+            } else {
+                final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars());
+                try {
+                    out.writeOptionalBytesReference(new BytesArray(passwordBytes));
+                } finally {
+                    Arrays.fill(passwordBytes, (byte) 0);
+                }
+            }
+        }
+
+        boolean hasPassword() {
+            assert hasReferences();
+            return this.secureSettingsPassword != null && this.secureSettingsPassword.length() > 0;
+        }
+
+        @Nullable
+        public SecureString getSecureSettingsPassword() {
+            assert hasReferences();
+            return secureSettingsPassword;
+        }
+
+        @Override
+        public void incRef() {
+            refs.incRef();
+        }
+
+        @Override
+        public boolean tryIncRef() {
+            return refs.tryIncRef();
+        }
+
+        @Override
+        public boolean decRef() {
+            return refs.decRef();
         }
 
         @Override
-        public void close() {
-            assert request.nodeRequests.isEmpty() : "potential circular reference";
-            request.close();
+        public boolean hasReferences() {
+            return refs.hasReferences();
         }
     }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -96,24 +96,22 @@ protected void doExecute(
         ActionListener<NodesReloadSecureSettingsResponse> listener
     ) {
         if (request.hasPassword() && isNodeLocal(request) == false && isNodeTransportTLSEnabled() == false) {
-            request.close();
             listener.onFailure(
                 new ElasticsearchException(
                     "Secure settings cannot be updated cluster wide when TLS for the transport layer"
                         + " is not enabled. Enable TLS or use the API with a `_local` filter on each node."
                 )
             );
         } else {
-            super.doExecute(task, request, ActionListener.runBefore(listener, request::close));
+            super.doExecute(task, request, listener);
         }
     }
 
     @Override
     protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(
-        NodesReloadSecureSettingsRequest.NodeRequest nodeReloadRequest,
+        NodesReloadSecureSettingsRequest.NodeRequest request,
         Task task
     ) {
-        final NodesReloadSecureSettingsRequest request = nodeReloadRequest.request;
         // We default to using an empty string as the keystore password so that we mimic pre 7.3 API behavior
         try (KeyStoreWrapper keystore = KeyStoreWrapper.load(environment.configFile())) {
             // reread keystore from config file
@@ -143,8 +141,6 @@ protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(
             return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), null);
         } catch (final Exception e) {
             return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), e);
-        } finally {
-            request.close();
         }
     }
 

server/src/main/java/org/elasticsearch/common/settings/SecureString.java

@@ -8,14 +8,15 @@
 
 package org.elasticsearch.common.settings;
 
-import java.io.Closeable;
+import org.elasticsearch.core.Releasable;
+
 import java.util.Arrays;
 import java.util.Objects;
 
 /**
  * A String implementations which allows clearing the underlying char array.
  */
-public final class SecureString implements CharSequence, Closeable {
+public final class SecureString implements CharSequence, Releasable {
 
     private char[] chars;