diff --git a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java index d9ad620ffaee5..1b43c865544c4 100644 --- a/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java +++ b/distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java @@ -166,13 +166,18 @@ static List apmJvmOptions(Settings settings, @Nullable KeyStoreWrapper k final List 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 propertiesMap) { final Set settingNames = keystore.getSettingNames(); for (String key : List.of("api_key", "secret_token")) { @@ -225,9 +230,18 @@ private static Map 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. + *

+ * 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 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 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()) { diff --git a/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java new file mode 100644 index 0000000000000..f9ca26ad29505 --- /dev/null +++ b/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(); + } +} diff --git a/docs/changelog/91058.yaml b/docs/changelog/91058.yaml new file mode 100644 index 0000000000000..09341799c9f86 --- /dev/null +++ b/docs/changelog/91058.yaml @@ -0,0 +1,6 @@ +pr: 91058 +summary: Fix APM configuration file delete +area: Infra/Core +type: bug +issues: + - 89439 diff --git a/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 228889aea900f..05d5971f458b1 100644 --- a/server/src/main/java/org/elasticsearch/node/Node.java +++ b/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. + *

+ * 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 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); } } }