Fix APM configuration file delete #91058

grcevski · 2022-10-20T20:47:07Z

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 PR:

fixes the APM module pattern match when we delete, while adding support for absolute paths
adds additional delete safety net on failed node start
adds tests for ensuring that the naming dependency isn't broken again.

After reviewing the possible ways to pass options to the APM agent, I think the current design is the best way to pass options to the APM Java agent at the moment. Even if we worked around the issues with the Security manager and we attached the agent instead of using the command line, the agent upon attach seems to be writing a configuration file anyway: https://github.com/elastic/apm-agent-java/blob/main/apm-agent-attach/src/main/java/co/elastic/apm/attach/ElasticApmAttacher.java#L115.

Closes #89439

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.

elasticsearchmachine · 2022-10-20T20:47:32Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-10-20T20:47:33Z

Hi @grcevski, I've created a changelog YAML for you.

grcevski · 2022-10-20T20:48:00Z

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

@@ -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");


I did resolve("modules").resolve("apm") to be Windows friendly.

grcevski · 2022-10-20T20:48:46Z

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

        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")) {


I had to add .* to make sure this works with absolute paths.

grcevski · 2022-10-20T20:49:51Z

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


        dynamicSettings.forEach((key, value) -> options.add("-Delastic.apm." + key + "=" + value));

        return options;
    }

+    // package private for testing


These small methods are extracted so that I can prove the tests use whatever is used in the code (given we have the cross file dependency with Node).

elasticsearchmachine · 2022-10-20T20:59:35Z

Hi @grcevski, I've updated the changelog YAML for you.

…ch into fix/apm_file_delete

pugnascotia

LGTM, apologies for the break 🙏

thecoop · 2022-10-24T10:05:45Z

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

+        Node.deleteTemporaryApmConfig(jvmInfo, (e, p) -> fail("Shouldn't hit an exception"));
+        assertFalse(Files.exists(tempFile));
+
+        Files.delete(agentPath);


Should this be done regardless of test status? Or maybe in test class cleanup?

grcevski · 2022-11-01T20:04:30Z

@elasticsearchmachine run CLA

grcevski · 2022-11-01T20:05:25Z

@elasticsearchmachine test this please

elasticsearchmachine · 2022-11-02T13:26:27Z

💚 Backport successful

Status	Branch	Result
✅	8.5

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.

* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java

grcevski added >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team auto-backport-and-merge v8.5.1 v8.6.0 labels Oct 20, 2022

Update docs/changelog/91058.yaml

237fe0f

grcevski commented Oct 20, 2022

View reviewed changes

grcevski requested a review from pugnascotia October 20, 2022 20:51

grcevski and others added 3 commits October 20, 2022 16:59

Update docs/changelog/91058.yaml

6c95c78

Fix test

b3f054c

Merge branch 'fix/apm_file_delete' of github.com:grcevski/elasticsear…

733ddf5

…ch into fix/apm_file_delete

pugnascotia approved these changes Oct 23, 2022

View reviewed changes

thecoop reviewed Oct 24, 2022

View reviewed changes

Adjust test code per feedback

25c962b

Merge branch 'main' into fix/apm_file_delete

a8a5561

grcevski merged commit 691a679 into elastic:main Nov 2, 2022

grcevski deleted the fix/apm_file_delete branch November 2, 2022 13:25

grcevski mentioned this pull request Nov 2, 2022

[8.5] Fix APM configuration file delete (#91058) #91250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix APM configuration file delete #91058

Fix APM configuration file delete #91058

Uh oh!

grcevski commented Oct 20, 2022 •

edited

Loading

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

grcevski Oct 20, 2022

Uh oh!

grcevski Oct 20, 2022

Uh oh!

grcevski Oct 20, 2022

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

pugnascotia left a comment

Uh oh!

thecoop Oct 24, 2022

Uh oh!

grcevski commented Nov 1, 2022

Uh oh!

grcevski commented Nov 1, 2022

Uh oh!

elasticsearchmachine commented Nov 2, 2022

Uh oh!

Uh oh!

Fix APM configuration file delete #91058

Fix APM configuration file delete #91058

Uh oh!

Conversation

grcevski commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

grcevski Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

grcevski Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

grcevski Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 20, 2022

Uh oh!

pugnascotia left a comment

Choose a reason for hiding this comment

Uh oh!

thecoop Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

grcevski commented Nov 1, 2022

Uh oh!

grcevski commented Nov 1, 2022

Uh oh!

elasticsearchmachine commented Nov 2, 2022

💚 Backport successful

Uh oh!

Uh oh!

grcevski commented Oct 20, 2022 •

edited

Loading