Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Defend Workflows][E2E]Get file command from response console #156159

Merged
merged 73 commits into from
May 16, 2023

Conversation

szwarckonrad
Copy link
Contributor

@szwarckonrad szwarckonrad commented Apr 28, 2023

This PR tackles get-file command issued from response console. Two tests are included:

  1. x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/response_console.cy.ts mocked data
  2. x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts real endpoint

# Conflicts:
#	x-pack/plugins/security_solution/common/endpoint/index_data.ts
# Conflicts:
#	x-pack/plugins/security_solution/public/management/cypress/support/data_loaders.ts
#	x-pack/plugins/security_solution/public/management/cypress/support/plugin_handlers/endpoint_data_loader.ts
#	x-pack/plugins/security_solution/public/management/cypress/tasks/index_endpoint_hosts.ts
#	x-pack/plugins/security_solution/scripts/endpoint/agent_emulator/services/endpoint_response_actions.ts
# Conflicts:
#	x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list.ts
# Conflicts:
#	x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/isolate.cy.ts
#	x-pack/plugins/security_solution/public/management/cypress/tasks/isolate.ts
@szwarckonrad szwarckonrad marked this pull request as ready for review May 8, 2023 14:13
@szwarckonrad szwarckonrad requested a review from a team as a code owner May 8, 2023 14:13
@szwarckonrad szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 v8.9.0 labels May 8, 2023
@elasticmachine
Copy link
Contributor

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

@szwarckonrad szwarckonrad requested a review from paul-tavares May 8, 2023 14:14
Comment on lines +189 to +196
cy.task('installPackagesOnEndpoint', { hostname: endpointHostname, packages: ['unzip'] });

cy.task('createFileOnEndpoint', {
hostname: endpointHostname,
path: filePath,
content: fileContent,
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This really feels overkill... All you are trying to test is that the endpoint can return a file based on a get-file action. Why not just retrieve a log file from the directory where endpoint is installed?

I'm ok if you leave it, but it just feels like we're doing unnecessary work IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I was aiming at is an e2e coverage of user's journey, therefore I want to make sure user can download and extract the exact file they requested. Do you thinks that's going too far? If so then the rest of the changes doesn't make much sense indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can make sure that the user can retrieve a file from the host - goal here should be to ensure Endpoint is sending back the file to ensure we have round trip communication. to do that, you can just get-file and use the path to any file on the VM - example: one of the log files under the Agent's install directory.

I was just really confused as to why you were creatingFileOnEndpoint() and uploadFileToEndpoint() and readZipFileContentOnEndpoint(). Just feels all of these are not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I went this way because I wanted to confirm that user is getting the file they requested and that the archive can be extracted with default password and I wouIdn't be able to do that with a random log file. In the process I found out that unzipping password protected zips in node can be tricky, its not platform agnostic when done locally so the most "stable" way is to do it on the host that runs ubuntu. It is cumbersome for sure but in the end its cheap time wise and covers what I believe is worth checking. It all comes down to the question whether checking the archive content adds value or not. Let me know what you think about it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Its feels like you are testing the Endpoint and not really focusing on the test from Kibana. IMO, this is unnecessary and goes beyond what we should be focusing on. Go ahead an keep it if you feel strongly about this, but remember that we are not testing endpoint - we're testing that we can communicate with the endpoint for different actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@paul-tavares , isn't e2e test's purpose to validate if the user journey is successful? We don't specifically test if just one part of the functionality works.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have different understanding of the level of testing we are validating with these real endpoint e2e tests. You seem to feel strongly that this is the right level so I'm going to retreat 😄 and let others provide feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Since e2e tests are slow to write and run I would argue that we write tests that cover only the essential high-value flow for a user. In other words, we should only test high-value paths in the e2e tests that we can't test in unit/integration tests.

So in this example, it is enough to test that I can fetch an actual file from the endpoint and that I have a downloadable link, any file will do. I am not interested in verifying what the user does with it after it is fetched on the console. In terms of other console actions as well, it is enough to write a test that verifies that the action does what it is supposed to do. Send a request and check the response from the endpoint is what is expected.

I do understand the appeal of writing a bunch of tests to verify the entire click-by-click user interaction, but that would mean duplicating a bunch of tests that already cover that. Considering the effort required to write, maintain, and debug tests, I would argue against adding redundant tests for the whole user interaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

hey all, thanks for the discussion, great to see us thinking deeply about what and how we test.

Here's my thoughts:

  • The main goals of testing with real Agent/Endpoints is to validate that we are integrating correctly with Endpoints and that the data flow is working end to end. In this case, we want to make sure that we can successfully send the Get File command to the Endpoint and it will respond back successfully. That being said, I think it is sufficient to just look at data flow for many of these tests and we don't necessarily need to validate user action on the host itself.
  • However, on the other hand, I certainly see the value to take the user journey flow one step further and validate that downloads, etc are working correctly. Thinking about it in terms of a user journey as @tomsonpl and @szwarckonrad laid out.

Considering the above, we definitely achieve the initial goal of testing stack/Endpoint data flow ✅

I think validating that the user can download the file from Kibana is also something good to do here. Endpoint tests would not cover this particular scenario in the user journey as they are focused on Endpoint functionality itself and not what happens after file upload to ES is complete. So I think the extra coverage would be good here ✅

I think we should merge this test as is because it does automate a scenario that we would otherwise do manually. If I were to test this manually, I would certainly take the extra step to download and inspect the file. As we strive to eliminate manual testing, this seems like something good to do.

I acknowledge that this goes beyond our original thoughts around testing this area, but I see the value in these extra steps. I think it's good to always evolve the way we think about testing, especially early on with a new framework.

# Conflicts:
#	x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts
#	x-pack/plugins/security_solution/public/management/cypress/support/data_loaders.ts
@szwarckonrad szwarckonrad enabled auto-merge (squash) May 16, 2023 14:01
Copy link
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.

LGTM

@szwarckonrad szwarckonrad merged commit cf6f350 into elastic:main May 16, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 400 404 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 480 484 +4
total +6

History

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

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.8 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.8:
- [Defend Workflows][E2E]Endpoint e2e response console (#155605)

Manual backport

To create the backport manually run:

node scripts/backport --pr 156159

Questions ?

Please refer to the Backport tool documentation

jasonrhodes pushed a commit that referenced this pull request May 17, 2023
This PR tackles `get-file` command issued from response console. Two
tests are included:
1.
`x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/response_console.cy.ts`
mocked data
2.
`x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts`
real endpoint

---------

Co-authored-by: Patryk Kopycinski <[email protected]>
@szwarckonrad
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.8

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

Questions ?

Please refer to the Backport tool documentation

ashokaditya added a commit that referenced this pull request May 26, 2023
…156159) (#158542)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Defend Workflows][E2E]Get file command from response console
(#156159)](#156159)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Konrad
Szwarc","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-16T17:55:57Z","message":"[Defend
Workflows][E2E]Get file command from response console (#156159)\n\nThis
PR tackles `get-file` command issued from response console. Two\r\ntests
are
included:\r\n1.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/response_console.cy.ts`\r\nmocked
data\r\n2.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts`\r\nreal
endpoint\r\n\r\n---------\r\n\r\nCo-authored-by: Patryk Kopycinski
<[email protected]>","sha":"cf6f350ed21eb86053e54005a5d59b1ff64a2d0d","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Defend
Workflows","v8.8.0","v8.9.0"],"number":156159,"url":"https://github.com/elastic/kibana/pull/156159","mergeCommit":{"message":"[Defend
Workflows][E2E]Get file command from response console (#156159)\n\nThis
PR tackles `get-file` command issued from response console. Two\r\ntests
are
included:\r\n1.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/response_console.cy.ts`\r\nmocked
data\r\n2.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts`\r\nreal
endpoint\r\n\r\n---------\r\n\r\nCo-authored-by: Patryk Kopycinski
<[email protected]>","sha":"cf6f350ed21eb86053e54005a5d59b1ff64a2d0d"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156159","number":156159,"mergeCommit":{"message":"[Defend
Workflows][E2E]Get file command from response console (#156159)\n\nThis
PR tackles `get-file` command issued from response console. Two\r\ntests
are
included:\r\n1.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/response_console.cy.ts`\r\nmocked
data\r\n2.\r\n`x-pack/plugins/security_solution/public/management/cypress/e2e/endpoint/response_console.cy.ts`\r\nreal
endpoint\r\n\r\n---------\r\n\r\nCo-authored-by: Patryk Kopycinski
<[email protected]>","sha":"cf6f350ed21eb86053e54005a5d59b1ff64a2d0d"}}]}]
BACKPORT-->

Co-authored-by: Ashokaditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 v8.8.1 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants