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

ElasticSearch Queries in Rule Types #883

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ElasticSearch Queries in Rule Types #883

wants to merge 3 commits into from

Conversation

Mraoul
Copy link

@Mraoul Mraoul commented Jan 25, 2017

This mod allows rule types to designate their own queries to be used to limit the data returned from ES. Updated the blacklist and whitelist rule types to use this. Basically the blacklist type creates a query with a bool should query, while the whitelist type creates a bool must not query. To ensure backwards compatibility, a new option called 'key_indexed' has been added to both which defaults to False if not present, which maintains currently functionality. Since only the filter part of the filtered query is used in the existing code (afaict), this code uses the query part of the filtered query so it should not cause any dsl syntax errors.

To allow the rule test to be able to access the new function, had to add a call to the load_modules function in test_file to ensure the class instance was loaded.

…ies to potentially optimize searches. Both the blacklist and whitelist rule types have been updated to take advantage of this enabling them to only receive data that should match their given criteria.
…call to mock setup which returns None. Might want to add test that checks return syntax ...
@Qmando
Copy link
Member

Qmando commented Feb 2, 2017

To be honest, whitelist and blacklist rules are totally useless, because they can be recreated with type: any and the proper filters, which is exactly what you are also doing here.

This is a very good idea, but I'm thinking about doing away with those types entirely, or maybe converting them for backwards compatibly reasons.

@Mraoul
Copy link
Author

Mraoul commented Feb 10, 2017

Frankly, I'm of the opposite mindset, I'd rather have more abstraction from the DSL, so users/analysts need less spin-up time to maintain this (and so they don't have to ask me to double check their work). Further, ES tends to change the DSL occasionally, e.g., filtered queries are apparently deprecated in 5 (ran into this very recently with something that no longer worked with 5), so I'd prefer to abstract that so rules don't have to be re-written when a new version of ES comes out.

But speaking of filtered queries, this pr made use of that which might complicate your ES 5 transition, so might want to table this until ES 5 support is fully integrated.

@alesnav
Copy link
Contributor

alesnav commented Sep 12, 2017

I think that this would be awesome!

@Mraoul , can you show an example using this modified blacklist/whitelist type?

@Qmando , this improvement makes sense because you can move the lists to an elasticsearch index instead of having them in plain text files (replicas, snapshots, availability, ...). Another improvement using this new method would be that you don't need to restart the elastalert service to refresh the lists.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants