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

Netflow input: Improve session reset detection and allow disabling #14904

Merged
merged 5 commits into from
Jan 7, 2020

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Dec 3, 2019

This adds a few fixes to Netflow input:

  • Session reset detection was too sensitive and not resilient against out-of-order UDP packets.
  • Fixes logging (was not thread-safe).

Also this adds a new config option check_sequence_reset to allow disable reset detection completely.

@adriansr adriansr added bug review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. labels Dec 3, 2019
@adriansr adriansr requested a review from a team as a code owner December 3, 2019 11:09
@adriansr adriansr requested a review from a team December 3, 2019 11:09
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.
Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.
This structure wasn't thread safe but it was used by different threads
which can cause a crash.
@adriansr adriansr force-pushed the netflow_seq_improvements branch from 5e30562 to 99e553f Compare December 18, 2019 19:28
@adriansr
Copy link
Contributor Author

@leehinman @andrewkroh I added another fix. Can you review again?

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.

LGTM

detected, record templates for the given exporter will be dropped. This will
cause flow loss until the exporter provides new templates. If set to `false`,
{beatname_uc} will ignore sequence numbers, which can cause some invalid flows
if the exporter process is reset. This option is only applicable to Netflow V9
Copy link
Member

Choose a reason for hiding this comment

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

So an invalid flow could occur if the template changes after the Exporter Process reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, yes. In practice, I understand the exporter will first send the new templates after a reset as it doesn't make any sense to start sending flows before sending templates for them.

@adriansr adriansr merged commit c47b9a2 into elastic:master Jan 7, 2020
@adriansr adriansr added v7.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jan 7, 2020
adriansr added a commit to adriansr/beats that referenced this pull request Jan 7, 2020
…lastic#14904)

- New NetFlow option detect_sequence_reset

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.

- Improve session reset detection in Netflow

Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.

- Make logDebugWrapper thread safe

This structure wasn't thread safe but it was used by different threads
which can cause a crash.

(cherry picked from commit c47b9a2)
adriansr added a commit to adriansr/beats that referenced this pull request Jan 7, 2020
…lastic#14904)

- New NetFlow option detect_sequence_reset

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.

- Improve session reset detection in Netflow

Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.

- Make logDebugWrapper thread safe

This structure wasn't thread safe but it was used by different threads
which can cause a crash.

(cherry picked from commit c47b9a2)
@adriansr adriansr added the v7.5.2 label Jan 7, 2020
adriansr added a commit that referenced this pull request Jan 7, 2020
…14904) (#15350)

- New NetFlow option detect_sequence_reset

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.

- Improve session reset detection in Netflow

Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.

- Make logDebugWrapper thread safe

This structure wasn't thread safe but it was used by different threads
which can cause a crash.

(cherry picked from commit c47b9a2)
adriansr added a commit that referenced this pull request Jan 10, 2020
…tion and allow disabling (#15351)

* Netflow input: Improve session reset detection and allow disabling (#14904)

- New NetFlow option detect_sequence_reset

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.

- Improve session reset detection in Netflow

Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.

- Make logDebugWrapper thread safe

This structure wasn't thread safe but it was used by different threads
which can cause a crash.

(cherry picked from commit c47b9a2)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…t detection and allow disabling (elastic#15351)

* Netflow input: Improve session reset detection and allow disabling (elastic#14904)

- New NetFlow option detect_sequence_reset

Add option to allow disabling sequence number tracking, which can cause
too many lost flows in some environments.

- Improve session reset detection in Netflow

Session reset detection was too sensitive and not resilent against
out-of-order UDP packets.

- Make logDebugWrapper thread safe

This structure wasn't thread safe but it was used by different threads
which can cause a crash.

(cherry picked from commit 2325b77)
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.

3 participants