-
Notifications
You must be signed in to change notification settings - Fork 610
Add unit test for protected prebuilt-rules #5242
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
Add unit test for protected prebuilt-rules #5242
Conversation
Enhancement - GuidelinesThese guidelines serve as a reminder set of considerations when addressing adding a feature to the code. Documentation and Context
Code Standards and Practices
Testing
Additional Checks
|
| """ | ||
| Ensure upstream referenced rule IDs and rule names remain unchanged | ||
| """ | ||
| protected_rules = {"9a1a2dae-0b5f-4c3d-8305-a268d404c306": "Endpoint Security (Elastic Defend)"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for posterity, do we expect there to be substantially more protected rules, or generally is this a small list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically for now 1 and may be a growing small list or not.
Initially I had checked to see if we wanna make all the rule_id immutable. But since there are no upstream impacts choose to stick to only rule that is referenced by its ID here https://github.com/elastic/kibana/blob/3f7184698faea27158fb47f397c35fec909a4ce3/x-pack/solutions/security/plugins/security_solution/common/detection_engine/constants.ts#L38
Co-authored-by: Eric Forte <119343520+eric-forte-elastic@users.noreply.github.com>
|
In Peer Review discussions we identified the root issue of ESQL validation not kicking off. @eric-forte-elastic Will shortly raise a fix and I will rebase and merge |
eric-forte-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of a quick lookup that you are using. Looking at an alternative.
protected_rules = {"9a1a2dae-0b5f-4c3d-8305-a268d404c306": "Endpoint Security (Elastic Defend)"}
# map current rules by id and name for quick lookup
filtered_rules_collection = self.all_rules.filter(lambda r: r.id in protected_rules.keys())
failures: list[str] = []
for rule in filtered_rules_collection:
rule_id = rule.contents.data.get("rule_id")
rule_name = rule.contents.data.get("name")
if rule_id in protected_rules.keys():
if rule_name != protected_rules.get(rule_id):
failures.append(
f"Protected rule_id {rule_id} name modified from '{rule_name}' to '{protected_rules.get(rule_id)}' - review upstream impact"
)
else:
failures.append(
f"Protected rule: {rule_name} rule_id: {rule_id} missing/modified - review upstream impact"
)Your current lookup effectively costs O( 2 n ) due to:
- This is O(n):
current_rules = {rule.contents.data.get("rule_id"): rule.contents.data.get("name") for rule in self.all_rules} - This is O(n)
for rule_id, rule_name in protected_rules.items():
where we know n > 1,000 since we have ~1,500 rules
Whereas the above suggestion:
- This is technically O(n) although it may be more optimized
filtered_rules_collection = self.all_rules.filter(lambda r: r.id in protected_rules.keys()) - This is O(m)
for rule in filtered_rules_collection:
where we know m < 10 (in this case 1).
Test for the above code time:
❯ python -m pytest tests/test_all_rules.py::TestIntegrationRules::test_preserve_upstream_protected_rule_id_name
========================================================================================= test session starts ==========================================================================================
platform linux -- Python 3.12.12, pytest-8.4.2, pluggy-1.6.0
rootdir: /tmp/detection-rules
configfile: pyproject.toml
plugins: typeguard-4.4.4
collected 1 item
tests/test_all_rules.py . [100%]
============================================================================ 1 passed in 121.18s (0:02:01) ============================================================================
⏎
Test for current code time:
❯ python -m pytest tests/test_all_rules.py::TestIntegrationRules::test_preserve_upstream_protected_rule_id_name
================================================================================= test session starts =================================================================================
platform linux -- Python 3.12.12, pytest-8.4.2, pluggy-1.6.0
rootdir: /tmp/detection-rules
configfile: pyproject.toml
plugins: typeguard-4.4.4
collected 1 item
tests/test_all_rules.py . [100%]
============================================================================ 1 passed in 104.75s (0:01:44) ============================================================================
As we can see, the O( 2 n) algorithmic approach is notably faster than the theoretically more optimized approach 👍
LGTM 👍
Pull Request
Issue link(s): #4707
Summary - What I changed
How To Test
Protected Rule not modified
Protected Rule ID modified
Protected Rule name modified
Retested After Code Changes All 3 use cases
bug,enhancement,schema,maintenance,Rule: New,Rule: Deprecation,Rule: Tuning,Hunt: New, orHunt: Tuningso guidelines can be generatedmeta:rapid-mergelabel if planning to merge within 24 hoursContributor checklist