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

Hive alerter update #142

Merged
merged 10 commits into from
May 10, 2021
Merged

Hive alerter update #142

merged 10 commits into from
May 10, 2021

Conversation

ferozsalam
Copy link
Collaborator

The organisation where I work makes extensive use of the integration between TheHive and ElastAlert, and we have noticed several issues with the current alerter. These are:

  • Undocumented functionality, such as the functionality around customFields
  • Unexpected behaviour with aggregations (Aggregation issues with TheHive alerting Yelp/elastalert#2560)
  • The entire functionality of the alerter being defined in a single function, making it hard to reason about
  • The use of behaviour that is not standard to ElastAlert, such as the syntax for populating the alert_text
  • The use of older Python styles (although this applies to other alerters as well)

I have refactored the alerter extensively and am opening this PR to propose integrating the refactored alerter upstream.

As part of this change I have:

  • Converted the alerter to use newer Python functionality, such as f-strings and type hints
  • Documented the various features that were previously missed, and explained their working
  • Refactored the monolithic alert function into several smaller functions that are easier to reason about
  • Converted the alerter to use behaviour standard to other ElastAlert alerters, including for aggregated alerts (which should resolve Aggregation issues with TheHive alerting Yelp/elastalert#2560)

This is a breaking change, and some TheHive rules already written will need (minor) refactoring to continue working. Wherever possible, I have attempted to minimise such changes. I do believe that the refactored alerter will be easier to work with over the long-term than the previous version.

The refactored alerter has been tested in a local environment against multiple rule types.

@nsano-rururu
Copy link
Collaborator

HiveAlerter doesn't have any test code, so I think you need to add it to tests/alerts_test.py.

@ferozsalam
Copy link
Collaborator Author

@nsano-rururu I have added a test that covers the main functionality of the alerter.

Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

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

Thanks!

@jertel jertel merged commit ec6160b into jertel:master May 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aggregation issues with TheHive alerting
3 participants