[EDR Workflows] Improve agent downloader #196135

tomsonpl · 2024-10-14T13:10:25Z

This PR introduces the following improvements and new functionalities:

Improved logging to provide clearer insights into the status of Agent downloads, including detailed error messages for failed downloads and retries.
The caching mechanism has been updated to include not just the latest patch version, but also the previous patch version (GA) for more robust caching support.
Added retry functionality for Agent downloads to improve resilience in cases of network issues or temporary failures. The system will now attempt to retry the download in case of errors.

tomsonpl · 2024-10-14T13:12:01Z

/ci

tomsonpl · 2024-10-14T13:19:35Z

/ci

tomsonpl · 2024-10-14T19:43:12Z

/ci

elasticmachine · 2024-10-15T07:49:16Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

paul-tavares

Left feedback for your review.

also, I think there is a way to manually trigger the job/script that actually uses our agent_downloader... have you don that to ensure it is all running as expected?

paul-tavares · 2024-10-16T15:06:37Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

@@ -11,21 +11,63 @@ import type { ToolingLog } from '@kbn/tooling-log';
 import { getAgentDownloadUrl, getAgentFileName } from '../common/fleet_services';
 import { downloadAndStoreAgent } from '../common/agent_downloads_service';

+// Decrement the patch version by 1
+const decrementPatchVersion = (version: string): string => {
+  const [major, minor, patch] = version.split('.').map(Number);


can you see if the semver package can help here. The code here assumes that version is a simple version of three separated values by . which may not be true.

Also - what happens if patch is 0? Example: 8.15.0, 9.0.0, etc...

I'll give semver a try, great suggestion, thank you! 😊 As for the patch being 0, we only invoke decrementPatchVersion when the patch is greater than 0, so that shouldn't be an issue. 👍

paul-tavares · 2024-10-16T15:14:41Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+
+// Generate a list of versions to attempt downloading, including a fallback to the previous patch (GA)
+const getVersionsToDownload = (version: string): string[] => {
+  const patch = parseInt(version.split('.')[2], 10);


Please use semver.patch(version) (may need to ensure that version does not have -SNAPSHOT in it)

Good hint, I'll use semver here, and drop the pre-release tag

paul-tavares · 2024-10-16T15:16:25Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+      log.info(`Successfully downloaded and stored version ${v}`);
+      return; // Exit once successful
+    } catch (error) {
+      log.error(`Failed to download or store version ${v}: ${error.message}`);


Just want to ensure you mean to ONLY log an error here and then keep going in downloading other versions. is that right?

Yes, that's correct. If the first attempt fails, we move on to try downloading the fallback version.

paul-tavares · 2024-10-16T15:17:46Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+    }
+  }
+
+  log.error(`Failed to download agent for any available version: ${versionsToDownload.join(', ')}`);


This is misleading. you could reach have downloaded some successfully but this just reports that it all error'd out.

Hmm, I’m not sure about that. I believe the early return statement prevents us from reaching this point.

paul-tavares · 2024-10-16T15:18:34Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+
+      await downloadAndStoreAgent(url, fileName);
+      log.info(`Successfully downloaded and stored version ${v}`);
+      return; // Exit once successful


So you download just the first one? and then just exit? you are inside of a loop here, so I'm confused by this return here

Great question! The reason we don’t download multiple versions is in most cases, only one version of the agent is needed to complete the task. The getVersionsToDownload function generates a list of potential versions (the exact version requested and a fallback), but the intention is to find the first valid version that can be successfully downloaded. Once that happens, downloading multiple versions would be redundant.
Was my thinking correct? Are you suggesting that we should download both versions?

Ok. So you are still only looking ot download 1 version, even though you get a list of versions for download.

Maybe add this information to the above for...of loop so that that is clear to others that might look at this code in the future.

paul-tavares · 2024-10-16T15:19:03Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+): Promise<void> => {
+  const versionsToDownload = getVersionsToDownload(version);
+
+  for (const v of versionsToDownload) {


(Optional) Don't use single letter variable name.

paul-tavares · 2024-10-16T15:20:22Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+  log.error(`Failed to download agent for any available version: ${versionsToDownload.join(', ')}`);
+};
+
+const isValidVersion = (version: string): boolean => {


please use semver to validate versions

paul-tavares · 2024-10-16T15:25:13Z

x-pack/plugins/security_solution/scripts/endpoint/common/agent_downloads_service.ts

+    const maxAttempts = 2;
+    for (let attempt = 0; attempt < maxAttempts; attempt++) {


I would suggest you use p-retry

Good idea, I forgot we have this utility :) Thanks!

paul-tavares · 2024-10-16T15:31:16Z

x-pack/plugins/security_solution/scripts/endpoint/common/agent_downloads_service.ts

+            const { body } = await nodeFetch(agentDownloadUrl);
+            await finished(body.pipe(outputStream));
+          },
+          () => fs.promises.unlink(newDownloadInfo.fullFilePath) // Clean up on interruption


is this fs.promises. valid? I can't find it in the docs for nodeJS. My understanding was that if you wanted to work with promise versions of methods you had to explicitly import them.

just use unlink since that was imported from the FS promises package

reverted 👍

tomsonpl

@paul-tavares , thank you for the review 👍 I adjusted the code, or answered in comments :)

tomsonpl · 2024-10-18T10:37:59Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

@@ -11,21 +11,63 @@ import type { ToolingLog } from '@kbn/tooling-log';
 import { getAgentDownloadUrl, getAgentFileName } from '../common/fleet_services';
 import { downloadAndStoreAgent } from '../common/agent_downloads_service';

+// Decrement the patch version by 1
+const decrementPatchVersion = (version: string): string => {
+  const [major, minor, patch] = version.split('.').map(Number);


I'll give semver a try, great suggestion, thank you! 😊 As for the patch being 0, we only invoke decrementPatchVersion when the patch is greater than 0, so that shouldn't be an issue. 👍

tomsonpl · 2024-10-18T11:20:36Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+
+// Generate a list of versions to attempt downloading, including a fallback to the previous patch (GA)
+const getVersionsToDownload = (version: string): string[] => {
+  const patch = parseInt(version.split('.')[2], 10);


Good hint, I'll use semver here, and drop the pre-release tag

tomsonpl · 2024-10-18T11:31:55Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+
+      await downloadAndStoreAgent(url, fileName);
+      log.info(`Successfully downloaded and stored version ${v}`);
+      return; // Exit once successful


Great question! The reason we don’t download multiple versions is in most cases, only one version of the agent is needed to complete the task. The getVersionsToDownload function generates a list of potential versions (the exact version requested and a fallback), but the intention is to find the first valid version that can be successfully downloaded. Once that happens, downloading multiple versions would be redundant.
Was my thinking correct? Are you suggesting that we should download both versions?

tomsonpl · 2024-10-18T11:33:01Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+      log.info(`Successfully downloaded and stored version ${v}`);
+      return; // Exit once successful
+    } catch (error) {
+      log.error(`Failed to download or store version ${v}: ${error.message}`);


Yes, that's correct. If the first attempt fails, we move on to try downloading the fallback version.

tomsonpl · 2024-10-18T11:34:37Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+    }
+  }
+
+  log.error(`Failed to download agent for any available version: ${versionsToDownload.join(', ')}`);


Hmm, I’m not sure about that. I believe the early return statement prevents us from reaching this point.

tomsonpl · 2024-10-18T11:44:01Z

x-pack/plugins/security_solution/scripts/endpoint/common/agent_downloads_service.ts

+            const { body } = await nodeFetch(agentDownloadUrl);
+            await finished(body.pipe(outputStream));
+          },
+          () => fs.promises.unlink(newDownloadInfo.fullFilePath) // Clean up on interruption


reverted 👍

tomsonpl · 2024-10-18T11:44:20Z

x-pack/plugins/security_solution/scripts/endpoint/common/agent_downloads_service.ts

+    const maxAttempts = 2;
+    for (let attempt = 0; attempt < maxAttempts; attempt++) {


Good idea, I forgot we have this utility :) Thanks!

…nloader

tomsonpl · 2024-10-18T17:07:17Z

@paul-tavares Could you help me trigger this job to test agent_downloader?

paul-tavares · 2024-10-18T18:22:57Z

@paul-tavares Could you help me trigger this job to test agent_downloader?

@dasansol92 - I think you were the one that worked with someone from the Kibana ops side to use this agent downloader - can you answer this for @tomsonpl ?

paul-tavares · 2024-10-18T18:25:14Z

x-pack/plugins/security_solution/scripts/endpoint/agent_downloader_cli/agent_downloader.ts

+
+      await downloadAndStoreAgent(url, fileName);
+      log.info(`Successfully downloaded and stored version ${v}`);
+      return; // Exit once successful


Ok. So you are still only looking ot download 1 version, even though you get a list of versions for download.

Maybe add this information to the above for...of loop so that that is clear to others that might look at this code in the future.

…nloader

szwarckonrad

LGTM! Thanks for addressing this!

tomsonpl · 2024-10-21T12:04:08Z

@jbudz Hey, could you please link this PR to a buildkite build so we can test if dowloading/caching agent works well?
Thanks!

tomsonpl · 2024-10-22T07:01:49Z

Agent was successfully downloaded here: https://buildkite.com/elastic/kibana-pull-request/builds/244398

Reverting the changes (manual trigger of downloader) 👍

elasticmachine · 2024-10-22T08:47:18Z

💚 Build Succeeded

Buildkite Build
Commit: 159c7ad

Metrics [docs]

✅ unchanged

History

💚 Build #244398 succeeded 2b80dc3
💛 Build #244114 was flaky 955f62d
💚 Build #243870 succeeded 3bc96a0
💔 Build #243838 failed 7761c26
💚 Build #242432 succeeded a118cec
💚 Build #242342 succeeded 2b744f9

cc @tomsonpl

kibanamachine · 2024-10-22T08:52:12Z

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11456672120

(cherry picked from commit c5067fd)

kibanamachine · 2024-10-22T08:57:01Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.16`: - [[EDR Workflows] Improve agent downloader (#196135)](#196135)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Tomasz Ciecierski <[email protected]>

# Backport This will backport the following commits from `main` to `8.x`: - [[EDR Workflows] Improve agent downloader (#196135)](#196135)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Tomasz Ciecierski <[email protected]>

improve agent downloader

b02fad8

tomsonpl added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 labels Oct 14, 2024

tomsonpl self-assigned this Oct 14, 2024

params

9dd496a

tomsonpl added the backport:version Backport to applied version labels label Oct 14, 2024

types

2b744f9

fix

a118cec

tomsonpl marked this pull request as ready for review October 15, 2024 07:49

tomsonpl requested a review from a team as a code owner October 15, 2024 07:49

tomsonpl requested review from pzl and szwarckonrad October 15, 2024 07:49

tomsonpl requested review from paul-tavares and removed request for pzl October 15, 2024 07:49

paul-tavares reviewed Oct 16, 2024

View reviewed changes

tomsonpl added the v8.17.0 label Oct 18, 2024

tomsonpl added 2 commits October 18, 2024 13:43

apply comments from review

1ccd0c8

clean up

60ff27e

tomsonpl commented Oct 18, 2024

View reviewed changes

tomsonpl added 3 commits October 18, 2024 13:48

Merge branch 'main' into agent-downloader

7761c26

fixes

024fb38

Merge remote-tracking branch 'origin/agent-downloader' into agent-dow…

3bc96a0

…nloader

tomsonpl requested a review from paul-tavares October 18, 2024 17:05

paul-tavares approved these changes Oct 18, 2024

View reviewed changes

tomsonpl added 3 commits October 21, 2024 07:18

Merge branch 'main' into agent-downloader

588e95e

add comment

a8d551d

Merge remote-tracking branch 'origin/agent-downloader' into agent-dow…

955f62d

…nloader

szwarckonrad approved these changes Oct 21, 2024

View reviewed changes

add agent-downloader

b67dc7d

tomsonpl requested a review from a team as a code owner October 21, 2024 17:21

fix place for agent-downloader

2b80dc3

Ikuni17 approved these changes Oct 21, 2024

View reviewed changes

revert

159c7ad

tomsonpl merged commit c5067fd into elastic:main Oct 22, 2024
43 checks passed

kibanamachine added the v9.0.0 label Oct 22, 2024

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 22, 2024

[EDR Workflows] Improve agent downloader (elastic#196135)

56ff949

(cherry picked from commit c5067fd)

kibanamachine mentioned this pull request Oct 22, 2024

[8.16] [EDR Workflows] Improve agent downloader (#196135) #197183

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 22, 2024

[EDR Workflows] Improve agent downloader (elastic#196135)

0754135

(cherry picked from commit c5067fd)

kibanamachine mentioned this pull request Oct 22, 2024

[8.x] [EDR Workflows] Improve agent downloader (#196135) #197184

Merged

		const maxAttempts = 2;
		for (let attempt = 0; attempt < maxAttempts; attempt++) {

[EDR Workflows] Improve agent downloader #196135

[EDR Workflows] Improve agent downloader #196135

Conversation

tomsonpl commented Oct 14, 2024 • edited by kibanamachine Loading

tomsonpl commented Oct 14, 2024

tomsonpl commented Oct 14, 2024

tomsonpl commented Oct 14, 2024

elasticmachine commented Oct 15, 2024

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomsonpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomsonpl commented Oct 18, 2024

paul-tavares commented Oct 18, 2024

Choose a reason for hiding this comment

szwarckonrad left a comment

Choose a reason for hiding this comment

tomsonpl commented Oct 21, 2024

tomsonpl commented Oct 22, 2024

elasticmachine commented Oct 22, 2024

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

💚 All backports created successfully

Questions ?

tomsonpl commented Oct 14, 2024 •

edited by kibanamachine

Loading