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

[Security Solution][API testing] Move and restructures Rule execution logic #170765

Merged
merged 44 commits into from
Nov 16, 2023

Conversation

WafaaNasr
Copy link
Contributor

@WafaaNasr WafaaNasr commented Nov 7, 2023

Summary

Following the initial work in this #166755

  • Addresses part of [Security Solution] Restructure API integration tests into meaningful groups that run in parallel on CI #151902 for rule execution logic

  • Moved the utility files associated with rule execution logic to the new directory security_solution_api_integration. Files not actively used in the previous folder were moved, while duplicate files remained in their original positions.

  • Updated the CodeOwner file for the newly moved tests

  • Old/new group details, decisions, and execution time are mentioned in thisdocument

  • Added new Alert archive for version 8.8.0

  • Resolved the issue with the query.ts test where the execution logic is executed last, encompassing the "query" test because it was unloading the alerts document and led to failures in subsequent tests.

  • For Alert As Data in Serverless the alert ancestor will be a data-stream however in ESS will be .internal.alerts-security.alerts-default-000001'

Action File New Path if moved
Delete security_and_spaces/rule_execution_logic -
Delete security_and_spaces/group5 -
Move detection_engine_api_integration/security_and_spaces/group5 detections_response/default_license/rule_execution_logic/keyword_family
Move detection_engine_api_integration/security_and_spaces/rule_execution_logic detections_response/default_license/rule_execution_logic/execution_logic
Move detection_engine_api_integration/security_and_spaces/group1/ignore_fields detections_response/default_license/rule_execution_logic/ignore_fields.ts
Move detection_engine_api_integration/security_and_spaces/group1/runtime detections_response/default_license/rule_execution_logic/runtime.ts
Move detection_engine_api_integration/security_and_spaces/group1/timestamps detections_response/default_license/rule_execution_logic/timestamps.ts

@WafaaNasr WafaaNasr added release_note:skip Skip the PR/issue when compiling release notes FTR labels Nov 7, 2023
@WafaaNasr WafaaNasr self-assigned this Nov 7, 2023
WafaaNasr and others added 25 commits November 8, 2023 15:24
@WafaaNasr WafaaNasr marked this pull request as ready for review November 15, 2023 13:35
@WafaaNasr WafaaNasr requested review from a team as code owners November 15, 2023 13:35
Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for doing this! Left a few comments

@@ -20,7 +20,6 @@
},
"settings": {
"index": {
"refresh_interval": "1s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why did we remove refresh_interval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! The refresh_interval is not a valid config in Serverless, and We found out that we don't even need for ESS either since out tests don't valid against that fields

return {
...svlSharedConfig.getAll(),
services: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it the same as just services,?

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 true, I added it in case we want to pass additional services for the Security

const config = getService('config');
const isServerless = config.get('serverless');
const dataPathBuilder = new EsArchivePathBuilder(isServerless);
const path = dataPathBuilder.getPath('auditbeat/hosts');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename path to auditbeatPath? I think it will be more descriptive this way.

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, sure

expect(previewAlerts.length).eql(maxSignals * 3);
const shellSignals = previewAlerts.filter((alert) => alert._source?.[ALERT_DEPTH] === 2);
expect(previewAlerts.length).eql(maxAlerts * 3);
const shellalerts = previewAlerts.filter((alert) => alert._source?.[ALERT_DEPTH] === 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

shellAlerts?

const config = getService('config');
const isServerless = config.get('serverless');
const dataPathBuilder = new EsArchivePathBuilder(isServerless);
const path = dataPathBuilder.getPath('auditbeat/hosts');
Copy link
Contributor

Choose a reason for hiding this comment

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

also here, maybe auditbeatPath will be more descriptive

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

cc @WafaaNasr

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

ftr_configs.yml

@WafaaNasr WafaaNasr merged commit 2b136a2 into elastic:main Nov 16, 2023
30 checks passed
@WafaaNasr WafaaNasr deleted the move-structure-rule_execution_test branch November 16, 2023 15:14
@kibanamachine kibanamachine added v8.12.0 backport:skip This commit does not require backporting labels Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting FTR release_note:skip Skip the PR/issue when compiling release notes v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants