Skip to content

[Security Solution][Endpoint] New enroll endpoint host function CI specific for Cypress tests to use cached agent files#171399

Merged
dasansol92 merged 12 commits intoelastic:mainfrom
dasansol92:feat/olm-new_function_for_CI_to_create_endpoint_host_using_cached_agent_installers
Nov 28, 2023
Merged

[Security Solution][Endpoint] New enroll endpoint host function CI specific for Cypress tests to use cached agent files#171399
dasansol92 merged 12 commits intoelastic:mainfrom
dasansol92:feat/olm-new_function_for_CI_to_create_endpoint_host_using_cached_agent_installers

Conversation

@dasansol92
Copy link
Copy Markdown
Contributor

Summary

In order to avoid downloading the elastic agent installer file on each Cypress test, we have introduced a new method CI specific that will cache elastic agent files and reuse it across all tests.

Old code about if CI conditions will be removed in a follow up pr.

It also introduces a CLI script to download a specific version of elastic agent using the existing methods in place.

@dasansol92 dasansol92 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution skip-ci v8.12.0 labels Nov 16, 2023
Copy link
Copy Markdown
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it out and it works nicely! I've few suggestions and nits but this is good to 🚢 !

// Check if agent file is already on disk before downloading it again
agentDownload = isAgentDownloadFromDiskAvailable(agentFileName);

// If it has not been already dowloaded, it has to be donwnloaded.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
// If it has not been already dowloaded, it has to be donwnloaded.
// If it has not been already downloaded, it should be downloaded.

log,
kbnClient,
})
: await createAndEnrollEndpointHost({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now remove the CI check and dependent code in createAndEnrollEndpointHost that also has a vagrant VM creation logic

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added a note in the description about doing that in a follow up pr.

export const downloadAndStoreAgent = async (
agentDownloadUrl: string
agentDownloadUrl: string,
agentFileName?: string
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please also add the new param to JSDoc above

`UNSUPPORTED_ARCHITECTURE_${process.arch}`;
const fileNameNoExtension = `elastic-agent-${agentVersion}-linux-${downloadArch}`;

const fileNameNoExtension = getAgentFileName(agentVersion);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
const fileNameNoExtension = getAgentFileName(agentVersion);
const fileNameWithoutExtension = getAgentFileName(agentVersion);

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented Nov 16, 2023

packer_cache.sh build didn't have any issues.

The new image can be tested by replacing n2-4-spot with n2-4-spot-test or n2-4-virt with n2-4-virt-test in the pipeline configuration for any tests you're interested in and re-triggering CI. We should revert this change before merging.

@dasansol92 dasansol92 marked this pull request as ready for review November 27, 2023 17:19
@dasansol92 dasansol92 requested review from a team as code owners November 27, 2023 17:19
@dasansol92 dasansol92 requested a review from parkiino November 27, 2023 17:19
@elasticmachine
Copy link
Copy Markdown
Contributor

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

Comment thread .buildkite/pipelines/pull_request/base.yml
Copy link
Copy Markdown
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@dasansol92
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, interesting changes, thanks for investing time and efforts into fixing this issue 👍

I left a few comments, but in general lgtm 👍

node scripts/es snapshot --download-only --base-path "$ES_CACHE_DIR" --version "$version"
done

for version in $(cat versions.json | jq -r '.versions[].version'); do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think combining two for loops into one would be possible? What are the beenfits of doing it separately?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think should be okay to download ES snapshot and agent in the same loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for that path in order to not block the existing code path if something fails with the elastic agent download

log.info(`Creating endpoint host, attempt ${retryAttempt}`);
const newHost = process.env.CI
? await createAndEnrollEndpointHostCI({
useClosestVersionMatch: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I understand correctly options can contain useClosestVersionMatch , do we want to let the hardcoded one be overwritten by options? What is the reason for this order, asking because this looks confusing to me :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even more confusing since we set useClosestVersionMatch to default to true in the function params

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a typo in the order introduced here. 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, this was a copy/paste of the existing code here. I'm ok removing this since the default value is true in a follow up pr if that looks good to you

@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests on Serverless #2 / Response console Execute operations: "execute --command" - should execute a command "execute --command" - should execute a command

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 464 465 +1

Total ESLint disabled count

id before after diff
securitySolution 534 535 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dasansol92 dasansol92 requested a review from jbudz November 28, 2023 13:28
@dasansol92 dasansol92 merged commit 1823d94 into elastic:main Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants