Fix APM configuration file delete

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

@@ -229,6 +229,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;
@@ -404,7 +405,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);
@@ -1134,24 +1138,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);
                             }
                         }
                     }