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

Update winlogbeat-options.asciidoc #2919

Merged
merged 3 commits into from
Nov 4, 2016
Merged

Update winlogbeat-options.asciidoc #2919

merged 3 commits into from
Nov 4, 2016

Conversation

gmoskovicz
Copy link

Fix for #1491.

@tsg
Copy link
Contributor

tsg commented Nov 2, 2016

LGTM, waiting for reviews from @andrewkroh and @dedemorton.

@tsg tsg added docs Winlogbeat needs_backport PR is waiting to be backported to other branches. review labels Nov 2, 2016

More than 22 event id's should be handled with multiple monitors to the same name:

--------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add [source,yaml] on this to get syntax highlighting.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -149,6 +149,24 @@ winlogbeat.event_logs:
event_id: 4624, 4625, 4700-4800, -4735
--------------------------------------------------------------------------------

[WARNING]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the usual way to create warnings, but it might be difficult in this case because you have code section in it. I'll wait for @dedemorton to suggest something.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is valid asciidoc tagging. You could add [source,yaml] above the dashed line that starts the example to format the code correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Keeping [WARNING] and added [source,yaml]

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I think we should add this note about the limit of 22 event IDs. But the suggested work-around could have some unintended consequences with respect to state persistence. It would work, but I would be hesitant to recommend it.

The two event log consumers defined for the "Security" log will both persist their state in the registry file under "Security" key. The record ID of the event that is read last will be persisted. This could cause a problem if the read offsets for the consumers varied because at startup they will both resume of the last persisted state potentially causing some events to be missed.

We could probably implement a workaround in the query builder to split the query after 22 "event sources". (The term "event sources" is vague so I would need to do some API testing to figure out how conditionals are factored into the limit of 22.) Then only a single consumer would be started for the "Security" log.

=======================================
There is a limit on the query depth to prevent the possibility of a stack overflow from deep recursive queries. This limit is actually set to 22 filters.

More than 22 event id's should be handled with multiple monitors to the same name:
Copy link
Member

Choose a reason for hiding this comment

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

s/id/ID/

Copy link
Author

Choose a reason for hiding this comment

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

FIXED

@@ -149,6 +149,24 @@ winlogbeat.event_logs:
event_id: 4624, 4625, 4700-4800, -4735
--------------------------------------------------------------------------------

[WARNING]
=======================================
There is a limit on the query depth to prevent the possibility of a stack overflow from deep recursive queries. This limit is actually set to 22 filters.
Copy link
Member

Choose a reason for hiding this comment

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

actually could be dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to avoid repeating content from other websites verbatim (the content is likely copyrighted). This sentence is taken directly from the Microsoft website: "There is a limit on the query depth to prevent the possibility of a stack overflow from deep recursive queries."

It would be better anyhow to state this limitation in terms of what the user specifies in the Winlogbeat config. How about something like this:

You can specify up to 22 event IDs. Specifying more than 22 event IDs may produce deep recursive queries that could result in a stack overflow. For more information, see https://support.microsoft.com/en-us/kb/970453.

I didn't suggest edits to the text about the workaround because it sounds like you are still discussing the best approach to solving this issue.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@gmoskovicz
Copy link
Author

gmoskovicz commented Nov 2, 2016

@andrewkroh

I am happy to remove the workaround, but if we add this in the documentation, then it means that we need to suggest a workaround. Do you want me to remove the workaround? What other workaround do you see here? If we do what you propose , do we still need this to be added in the documentation?

@andrewkroh
Copy link
Member

@gmoskovicz, I think another possible workaround would be to use a processor to do the filtering.

I can spend some time on investigating the change to the query builder. If I can implement that then we don't need this in the documentation. But since it's a problem now for 5.0.0 we should merge this PR with a workaround that uses a processor. Can you see if you can setup a processor to do the desired filtering?

@gmoskovicz
Copy link
Author

I'll work with this and get back here.

@gmoskovicz
Copy link
Author

@andrewkroh

Works, but the configuration gets larger since you need a huge OR with equal conditions:

processors:
 - drop_event:
     when:
       equals:
         event_id: 50037

Copy link
Author

@gmoskovicz gmoskovicz left a comment

Choose a reason for hiding this comment

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

I've changed all the suggested text.

@gmoskovicz
Copy link
Author

@andrewkroh @tsg Can i have final review of this? Given that there is no consensus around the workaround, i've suggested one using multiple processors and/or multiple IDs at the input level.

- name: Security
event_id: 4624, 4625, 4700-4800, -4735 ....

processors:
Copy link
Member

Choose a reason for hiding this comment

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

Here's what I'm thinking on this topic.

If you specify more that 22 event IDs to include or 22 event IDs to exclude,
Windows will prevent Winlogbeat from reading the event log because it limits the
number of conditions that can be used in an event log query. If this occurs a similar
warning as shown below will be logged by Winlogbeat, and it will continue
processing data from other event logs. For more information, see
https://support.microsoft.com/en-us/kb/970453. 

`WARN EventLog[Application] Open() error. No events will be read from this
source. The specified query is invalid.`

If you have more than 22 event IDs, you can workaround this Windows limitation
by using a drop_event[drop-event] processor to do the filtering after
Winlogbeat has received the events from Windows. The filter shown below is
equivalent to `event_id: 903, 1024, 4624` but can be expanded beyond 22
event IDs.

[source,yaml]
--------------------------------------------------------------------------------
processors:                                                                                                                                                 
- drop_event.when.and:                                                                                                                                      
    - equals.log_name: Security                                                                                                                             
    - not.or:                                                                                                                                               
        - equals.event_id: 903                                                                                                                              
        - equals.event_id: 1024                                                                                                                             
        - equals.event_id: 4624
--------------------------------------------------------------------------------


[source,yaml]
--------------------------------------------------------------------------------
winlogbeat.event_logs:
Copy link
Member

Choose a reason for hiding this comment

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

I investigated automatically splitting up the event IDs across two queries. It gets complicated due to the interactions with the suppress filters (e.g. -1, -2). I think we should wait until #1491 (comment) is implemented and in the meantime use the drop_event processor as a workaround to either include or exclude events.

I don't want to recommend creating duplicate Security event log consumers because it can potentially result in data loss in some conditions. And in the future we may want to disallow duplicate names to prevent the aforementioned condition (but that's TBD).

@gmoskovicz
Copy link
Author

@andrewkroh sounds good for me! I've updated it to the latest version

@andrewkroh andrewkroh merged commit 1f86779 into 5.0 Nov 4, 2016
@gmoskovicz gmoskovicz deleted the doc-event-id-filter-fix branch November 4, 2016 13:52
@tsg
Copy link
Contributor

tsg commented Nov 4, 2016

Belated LGTM :)

dedemorton pushed a commit to dedemorton/beats that referenced this pull request Dec 21, 2016
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for elastic#1491 using filters.
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Dec 21, 2016
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for elastic#1491 using filters.
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Dec 21, 2016
ruflin pushed a commit that referenced this pull request Dec 21, 2016
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for #1491 using filters.
andrewkroh pushed a commit that referenced this pull request Dec 21, 2016
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for #1491 using filters.
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for elastic#1491 using filters.
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Add a note and workaround to the Winlogbeat docs about the limit of 22 event IDs in a query. And provide a workaround for elastic#1491 using filters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants