From fa396cbac68d31df24c34df0c3390f37d0db7c72 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 20 Oct 2022 16:44:54 -0400 Subject: [PATCH 1/5] Fix APM configuration file delete 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. --- .../server/cli/APMJvmOptions.java | 33 +++++++++- .../server/cli/APMJvmOptionsTests.java | 61 +++++++++++++++++++ .../java/org/elasticsearch/node/Node.java | 17 +++--- 3 files changed, 101 insertions(+), 10 deletions(-) create mode 100644 distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java 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..25cdec2acd324 --- /dev/null +++ b/distribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java @@ -0,0 +1,61 @@ +/* + * 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 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 { + + public void testFindJar() throws IOException, UserException { + Path installDir = makeFakeAgentJar(); + var agentPath = APMJvmOptions.findAgentJar(installDir.toAbsolutePath().toString()); + 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, UserException { + var agentPath = APMJvmOptions.findAgentJar(makeFakeAgentJar().toAbsolutePath().toString()); + 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/server/src/main/java/org/elasticsearch/node/Node.java b/server/src/main/java/org/elasticsearch/node/Node.java index 6f7f20e60a291..d5f76af4cd2e4 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); @@ -1135,24 +1139,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); } } } From 237fe0f09eb7e0dd8196ec201a1aefc6b65c6476 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Thu, 20 Oct 2022 16:47:33 -0400 Subject: [PATCH 2/5] Update docs/changelog/91058.yaml --- docs/changelog/91058.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/91058.yaml diff --git a/docs/changelog/91058.yaml b/docs/changelog/91058.yaml new file mode 100644 index 0000000000000..83fdf2c9771d4 --- /dev/null +++ b/docs/changelog/91058.yaml @@ -0,0 +1,5 @@ +pr: 91058 +summary: Fix APM configuration file delete +area: Infra/Core +type: bug +issues: [] From 6c95c784fcf0b894ee0faf6c104235559119b704 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Thu, 20 Oct 2022 16:59:35 -0400 Subject: [PATCH 3/5] Update docs/changelog/91058.yaml --- docs/changelog/91058.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/changelog/91058.yaml b/docs/changelog/91058.yaml index 83fdf2c9771d4..09341799c9f86 100644 --- a/docs/changelog/91058.yaml +++ b/docs/changelog/91058.yaml @@ -2,4 +2,5 @@ pr: 91058 summary: Fix APM configuration file delete area: Infra/Core type: bug -issues: [] +issues: + - 89439 From b3f054c8d422068acb112953c4b7aa7d16fb278a Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 20 Oct 2022 19:07:46 -0400 Subject: [PATCH 4/5] Fix test --- .../java/org/elasticsearch/server/cli/APMJvmOptionsTests.java | 4 ++++ 1 file changed, 4 insertions(+) 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 index 25cdec2acd324..4f8e84f47ee98 100644 --- 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 @@ -36,6 +36,8 @@ public void testFindJar() throws IOException, UserException { expectThrows(UserException.class, () -> APMJvmOptions.findAgentJar(anotherPath.toAbsolutePath().toString())).getMessage() .contains("Installation is corrupt") ); + + Files.delete(agentPath); } public void testFileDeleteWorks() throws IOException, UserException { @@ -47,6 +49,8 @@ public void testFileDeleteWorks() throws IOException, UserException { assertTrue(Files.exists(tempFile)); Node.deleteTemporaryApmConfig(jvmInfo, (e, p) -> fail("Shouldn't hit an exception")); assertFalse(Files.exists(tempFile)); + + Files.delete(agentPath); } private Path makeFakeAgentJar() throws IOException { From 25c962b4888654d0e500f60813e1add803afc795 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 1 Nov 2022 14:55:15 -0400 Subject: [PATCH 5/5] Adjust test code per feedback --- .../server/cli/APMJvmOptionsTests.java | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) 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 index 4f8e84f47ee98..f9ca26ad29505 100644 --- 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 @@ -12,6 +12,8 @@ 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; @@ -23,9 +25,21 @@ @ESTestCase.WithoutSecurityManager public class APMJvmOptionsTests extends ESTestCase { - public void testFindJar() throws IOException, UserException { - Path installDir = makeFakeAgentJar(); - var agentPath = APMJvmOptions.findAgentJar(installDir.toAbsolutePath().toString()); + 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")); @@ -36,12 +50,9 @@ public void testFindJar() throws IOException, UserException { expectThrows(UserException.class, () -> APMJvmOptions.findAgentJar(anotherPath.toAbsolutePath().toString())).getMessage() .contains("Installation is corrupt") ); - - Files.delete(agentPath); } - public void testFileDeleteWorks() throws IOException, UserException { - var agentPath = APMJvmOptions.findAgentJar(makeFakeAgentJar().toAbsolutePath().toString()); + public void testFileDeleteWorks() throws IOException { var tempFile = APMJvmOptions.createTemporaryPropertiesFile(agentPath.getParent()); var commandLineOption = APMJvmOptions.agentCommandLineOption(agentPath, tempFile); var jvmInfo = mock(JvmInfo.class); @@ -49,8 +60,6 @@ public void testFileDeleteWorks() throws IOException, UserException { assertTrue(Files.exists(tempFile)); Node.deleteTemporaryApmConfig(jvmInfo, (e, p) -> fail("Shouldn't hit an exception")); assertFalse(Files.exists(tempFile)); - - Files.delete(agentPath); } private Path makeFakeAgentJar() throws IOException {