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.

Sign up for GitHub

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

Jump to bottom

[Bug] New Terms Rule Import Failing #3569

Merged
merged 6 commits into from
Apr 4, 2024
Merged

[Bug] New Terms Rule Import Failing #3569

merged 6 commits into from
Apr 4, 2024

Conversation

eric-forte-elastic
Copy link
Contributor

@eric-forte-elastic eric-forte-elastic commented Apr 3, 2024 •
edited
Loading

Issues

#3542

Summary

This PR is an intermediate fix for addressing an issue where new terms rules do not import successfully. See the above issue for more context.

There are two primary issues addressed in this PR:

  1. The new terms rule schemas do not match what is supplied by Kibana
    • Our intermediate fix is to add a handler in the rule creation/importing loop that makes slight changes to the incoming data to support our schema.
  2. The current schema for rule loading does not support having brackets in the rule name, where Kibana does.
    • Out fix for this is to direct add support for brackets in our schema to better support Kibana.

Testing

To test these updates use the following two example new terms rules and run python -m detection_rules import-rules <filename> --required-only

This first export tests the first issue independently of the second.
rules_export.ndjson.txt

This second export tests both combined.
rules_export_new_terms_test_other.ndjson.txt

@eric-forte-elastic eric-forte-elastic linked an issue Apr 3, 2024 that may be closed by this pull request
[Bug] New Terms Rule Importing Failures #3542
Closed
@eric-forte-elastic eric-forte-elastic self-assigned this Apr 3, 2024
@eric-forte-elastic eric-forte-elastic added bug Something isn't working python Internal python for the repository Area: DED Team: TRADE labels Apr 3, 2024
@eric-forte-elastic eric-forte-elastic marked this pull request as ready for review April 3, 2024 21:12
brokensound77
brokensound77 reviewed Apr 4, 2024
View reviewed changes
detection_rules/schemas/definitions.py
@@ -17,7 +17,7 @@
DATE_PATTERN = r'^\d{4}/\d{2}/\d{2}$'
MATURITY_LEVELS = ['development', 'experimental', 'beta', 'production', 'deprecated']
OS_OPTIONS = ['windows', 'linux', 'macos']
NAME_PATTERN = r'^[a-zA-Z0-9].+?[a-zA-Z0-9()]$'
NAME_PATTERN = r'^[a-zA-Z0-9].+?[a-zA-Z0-9\[\]()]$'
Copy link
Contributor

Choose a reason for hiding this comment

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

Have an example of why this was needed?

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, any case where rules exported from Kibana have a title similar to Rule Name [Duplicate]
Example from the testing block above that has this:

rules_export_new_terms_test_other.ndjson.txt

@botelastic botelastic bot added the schema label Apr 4, 2024
brokensound77
brokensound77 reviewed Apr 4, 2024
View reviewed changes
detection_rules/cli_utils.py
@@ -165,7 +165,19 @@ def rule_prompt(path=None, rule_type=None, required_only=True, save=True, verbos
contents[name] = schema_prompt(name, value=kwargs.pop(name))
continue

result = schema_prompt(name, is_required=name in required_fields, **options.copy())
if name == "new_terms":
Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm if it works 😓

brokensound77
brokensound77 approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@brokensound77 brokensound77 left a comment

Choose a reason for hiding this comment

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

Ideally we need to fix the schema itself and modify the TOML state of the data (which I know we are punting for a bug fix a bit later).

This prompt is in desperate need of refactor, so intermediate patches help, but should be considered the temporary solution.

Nice job on this - LGTM 🚢

@eric-forte-elastic eric-forte-elastic linked an issue Apr 4, 2024 that may be closed by this pull request
[Bug] CLI Command import-rules Prompts Threshold Related Values to be Submitted #3266
Closed
This was unlinked from issues Apr 4, 2024
[Bug] CLI Command import-rules Prompts Threshold Related Values to be Submitted #3266
Closed
[Bug] New Terms Rule Importing Failures #3542
Closed
@Mikaayenson Mikaayenson linked an issue Apr 4, 2024 that may be closed by this pull request
[Bug] New Terms Rule Importing Failures #3542
Closed
Mikaayenson
Mikaayenson approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

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

This LGTM. I do think we don't need this if check on new_terms_fields. The schema identifies that its new_terms, in which case all the fields are required.

detection_rules/cli_utils.py Outdated Show resolved Hide resolved
Mikaayenson and others added 2 commits April 4, 2024 16:07
@eric-forte-elastic eric-forte-elastic merged commit fa75876 into main Apr 4, 2024
14 checks passed
@eric-forte-elastic eric-forte-elastic deleted the 3542-bug-new-terms-rule-importing-failures branch April 4, 2024 21:37
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
c6656e4 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
d53092d 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
3048160 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
c087fcb 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
72450ce 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
bf2e3d3 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
71fba3c 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
0720498 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
645fa59 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
3059931 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
protectionsmachine pushed a commit that referenced this pull request Apr 4, 2024
@eric-forte-elastic @github-actions
[Bug] New Terms Rule Import Failing (#3569)
7080a73 
* initial patch

* Update definitions to allow for brackets in name

* Update to prompt for required fields.

* Update detection_rules/cli_utils.py

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>

(cherry picked from commit fa75876)
@Mikaayenson Mikaayenson mentioned this pull request Jul 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikaayenson Mikaayenson Mikaayenson approved these changes

@brokensound77 brokensound77 brokensound77 approved these changes

@shashank-elastic shashank-elastic Awaiting requested review from shashank-elastic

Assignees

@eric-forte-elastic eric-forte-elastic

Labels
backport: auto bug Something isn't working python Internal python for the repository schema Team: TRADE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] New Terms Rule Importing Failures
3 participants
@eric-forte-elastic @Mikaayenson @brokensound77