Fix APM configuration file delete (#91058) (#91250)

When we launch Elasticsearch with the APM monitoring
agent, we create a temporary configuration file to
securely pass the API key or secret. This temporary
file is cleaned up on Elasticsearch Node creation.

After we renamed the APM module, the delete logic
didn't get updated, which means we never delete the file
anymore.

This commit:
 - fixes the APM module pattern match when we delete
 - adds additional delete safety net on failed node start
 - adds tests for ensuring the naming dependency isn't
   broken again.

Author: grcevski

distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java

@@ -166,13 +166,18 @@ static List<String> apmJvmOptions(Settings settings, @Nullable KeyStoreWrapper k
         final List<String> options = new ArrayList<>();
         // Use an agent argument to specify the config file instead of e.g. `-Delastic.apm.config_file=...`
         // because then the agent won't try to reload the file, and we can remove it after startup.
-        options.add("-javaagent:" + agentJar + "=c=" + tmpProperties);
+        options.add(agentCommandLineOption(agentJar, tmpProperties));
 
         dynamicSettings.forEach((key, value) -> options.add("-Delastic.apm." + key + "=" + value));
 
         return options;
     }
 
+    // package private for testing
+    static String agentCommandLineOption(Path agentJar, Path tmpPropertiesFile) {
+        return "-javaagent:" + agentJar + "=c=" + tmpPropertiesFile;
+    }
+
     private static void extractSecureSettings(KeyStoreWrapper keystore, Map<String, String> propertiesMap) {
         final Set<String> settingNames = keystore.getSettingNames();
         for (String key : List.of("api_key", "secret_token")) {
@@ -225,9 +230,18 @@ private static Map<String, String> extractApmSettings(Settings settings) throws
         return propertiesMap;
     }
 
+    // package private for testing
+    static Path createTemporaryPropertiesFile(Path tmpdir) throws IOException {
+        return Files.createTempFile(tmpdir, ".elstcapm.", ".tmp");
+    }
+
     /**
      * Writes a Java properties file with data from supplied map to a temporary config, and returns
      * the file that was created.
+     * <p>
+     * We expect that the deleteTemporaryApmConfig function in Node will delete this temporary
+     * configuration file, however if we fail to launch the node (because of an error) we might leave the
+     * file behind. Therefore, we register a CLI shutdown hook that will also attempt to delete the file.
      *
      * @param tmpdir        the directory for the file
      * @param propertiesMap the data to write
@@ -238,10 +252,18 @@ private static Path writeApmProperties(Path tmpdir, Map<String, String> properti
         final Properties p = new Properties();
         p.putAll(propertiesMap);
 
-        final Path tmpFile = Files.createTempFile(tmpdir, ".elstcapm.", ".tmp");
+        final Path tmpFile = createTemporaryPropertiesFile(tmpdir);
         try (OutputStream os = Files.newOutputStream(tmpFile)) {
             p.store(os, " Automatically generated by Elasticsearch, do not edit!");
         }
+        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+            try {
+                Files.deleteIfExists(tmpFile);
+            } catch (IOException e) {
+                // ignore
+            }
+        }, "elasticsearch[apmagent-cleanup]"));
+
         return tmpFile;
     }
 
@@ -253,7 +275,12 @@ private static Path writeApmProperties(Path tmpdir, Map<String, String> properti
      */
     @Nullable
     private static Path findAgentJar() throws IOException, UserException {
-        final Path apmModule = Path.of(System.getProperty("user.dir")).resolve("modules/apm");
+        return findAgentJar(System.getProperty("user.dir"));
+    }
+
+    // package private for testing
+    static Path findAgentJar(String installDir) throws IOException, UserException {
+        final Path apmModule = Path.of(installDir).resolve("modules").resolve("apm");
 
         if (Files.notExists(apmModule)) {
             if (Build.CURRENT.isProductionRelease()) {

distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

@@ -0,0 +1,74 @@
+/*
+ * 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.server.cli;
+
+import org.elasticsearch.cli.UserException;
+import org.elasticsearch.monitor.jvm.JvmInfo;
+import org.elasticsearch.node.Node;
+import org.elasticsearch.test.ESTestCase;
+import org.junit.After;
+import org.junit.Before;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
+@ESTestCase.WithoutSecurityManager
+public class APMJvmOptionsTests extends ESTestCase {
+
+    private Path installDir;
+    private Path agentPath;
+
+    @Before
+    public void setup() throws IOException, UserException {
+        installDir = makeFakeAgentJar();
+        agentPath = APMJvmOptions.findAgentJar(installDir.toAbsolutePath().toString());
+    }
+
+    @After
+    public void cleanup() throws IOException {
+        Files.delete(agentPath);
+    }
+
+    public void testFindJar() throws IOException {
+        assertNotNull(agentPath);
+
+        Path anotherPath = Files.createDirectories(installDir.resolve("another"));
+        Path apmPathDir = anotherPath.resolve("modules").resolve("apm");
+        Files.createDirectories(apmPathDir);
+
+        assertTrue(
+            expectThrows(UserException.class, () -> APMJvmOptions.findAgentJar(anotherPath.toAbsolutePath().toString())).getMessage()
+                .contains("Installation is corrupt")
+        );
+    }
+
+    public void testFileDeleteWorks() throws IOException {
+        var tempFile = APMJvmOptions.createTemporaryPropertiesFile(agentPath.getParent());
+        var commandLineOption = APMJvmOptions.agentCommandLineOption(agentPath, tempFile);
+        var jvmInfo = mock(JvmInfo.class);
+        doReturn(new String[] { commandLineOption }).when(jvmInfo).getInputArguments();
+        assertTrue(Files.exists(tempFile));
+        Node.deleteTemporaryApmConfig(jvmInfo, (e, p) -> fail("Shouldn't hit an exception"));
+        assertFalse(Files.exists(tempFile));
+    }
+
+    private Path makeFakeAgentJar() throws IOException {
+        Path tempFile = createTempFile();
+        Path apmPathDir = tempFile.getParent().resolve("modules").resolve("apm");
+        Files.createDirectories(apmPathDir);
+        Path apmAgentFile = apmPathDir.resolve("elastic-apm-agent-0.0.0.jar");
+        Files.move(tempFile, apmAgentFile);
+
+        return tempFile.getParent();
+    }
+}

docs/changelog/91058.yaml

@@ -0,0 +1,6 @@
+pr: 91058
+summary: Fix APM configuration file delete
+area: Infra/Core
+type: bug
+issues:
+ - 89439

server/src/main/java/org/elasticsearch/node/Node.java

@@ -226,6 +226,7 @@
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.function.BiConsumer;
 import java.util.function.Function;
 import java.util.function.LongSupplier;
 import java.util.function.UnaryOperator;
@@ -401,7 +402,10 @@ protected Node(
                 );
             }
 
-            deleteTemporaryApmConfig(jvmInfo);
+            deleteTemporaryApmConfig(
+                jvmInfo,
+                (e, apmConfig) -> logger.error("failed to delete temporary APM config file [{}], reason: [{}]", apmConfig, e.getMessage())
+            );
 
             this.pluginsService = pluginServiceCtor.apply(tmpSettings);
             final Settings settings = mergePluginSettings(pluginsService.pluginMap(), tmpSettings);
@@ -1115,24 +1119,23 @@ protected Node(
      * If the JVM was started with the Elastic APM agent and a config file argument was specified, then
      * delete the config file. The agent only reads it once, when supplied in this fashion, and it
      * may contain a secret token.
+     * <p>
+     * Public for testing only
      */
     @SuppressForbidden(reason = "Cannot guarantee that the temp config path is relative to the environment")
-    private void deleteTemporaryApmConfig(JvmInfo jvmInfo) {
+    public static void deleteTemporaryApmConfig(JvmInfo jvmInfo, BiConsumer<Exception, Path> errorHandler) {
         for (String inputArgument : jvmInfo.getInputArguments()) {
             if (inputArgument.startsWith("-javaagent:")) {
                 final String agentArg = inputArgument.substring(11);
                 final String[] parts = agentArg.split("=", 2);
-                if (parts[0].matches("modules/x-pack-apm-integration/elastic-apm-agent-\\d+\\.\\d+\\.\\d+\\.jar")) {
+                if (parts[0].matches(".*modules/apm/elastic-apm-agent-\\d+\\.\\d+\\.\\d+\\.jar")) {
                     if (parts.length == 2 && parts[1].startsWith("c=")) {
                         final Path apmConfig = PathUtils.get(parts[1].substring(2));
                         if (apmConfig.getFileName().toString().matches("^\\.elstcapm\\..*\\.tmp")) {
                             try {
                                 Files.deleteIfExists(apmConfig);
                             } catch (IOException e) {
-                                logger.error(
-                                    "Failed to delete temporary APM config file [" + apmConfig + "], reason: [" + e.getMessage() + "]",
-                                    e
-                                );
+                                errorHandler.accept(e, apmConfig);
                             }
                         }
                     }