fix(inputs/flow): don't drop whole NetFlow v9/IPFIX datagram on unkno… - #796
fix(inputs/flow): don't drop whole NetFlow v9/IPFIX datagram on unkno…#796samiura wants to merge 4 commits into
Conversation
leoparente
left a comment
leoparente
left a comment
There was a problem hiding this comment.
Try to generate a real .pcap file with that case and add it to fixtures. Do not mock bytes only in tests
bfb9a30 to
64dd4e9
Compare
LCOV of commit
|
|
also issue says: Options templates are unimplemented, then a missing-template lookup fails the whole packet. Our code does not implement options template? if so, shouldn't we implement it? |
Very good catch indeed! The ticket's suggested fix lists two things: (1) don't fail the datagram on unknown/options flowsets, skip and continue, This PR implements (1), which the reporter explicitly calls sufficient for the reported symptom (dropped packets / inflated packet_errors). Happy to open a follow-up issue to track that separately, or fold it into this PR now if you insist. |
|
Fold this in this PR. I think it is time to improve the flow input code |
64dd4e9 to
b77008d
Compare
|
Pushed with the previous requests which includes, options-template parsing for both NetFlow v9 (flowset id 1) and IPFIX (set id 3) is now in this PR. Templates are parsed and registered; And once seen, options-data flowsets/sets are recognized and skipped correctly instead of falling through the generic "unknown template" path. That covers the ticket's "optionally parse options templates" suggestion in full. @leoparente one thing I want to confirm regarding, "time to improve the flow input code" — is there more you had in mind beyond options-template parsing for this PR, or is this good to review as it is now? |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b77008d2fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…wn (options) flowset, or on legitimate zero-flow packets Data flowsets/sets referencing a template id we don't have a data template for (because it's actually an options template) no longer abort the whole datagram parse -- just that flowset is skipped, and any other flowsets in the same datagram are still processed. A structurally valid datagram with zero flow records (e.g. a pure template refresh) is also no longer counted as a parse error.
b77008d to
5b492a6
Compare
…ions/data template classification on id reuse, and only skip (not fail) unknown-template data flowsets Addresses three review findings: a FlowSet/Set shorter than its own 4-byte header could never advance the parse cursor, spinning forever; reassigning a template id from options to data (or back) left a stale classification in the other map, silently dropping real flow data forever; and every process_netflow_v9_data/v10_data failure was being treated as skippable, which also swallowed genuine decode failures for known templates instead of failing the datagram.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad57171e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e number A field spec with the enterprise bit set carries an extra 4-byte enterprise number. If a Set was truncated right at that point, offset advanced past len with no check, and the loop would still exit normally -- so this malformed, truncated template got registered anyway, silently erasing a valid template of the other kind (data vs options) for the same id. Both the regular and options template paths had the identical gap; both are now rejected before they can commit.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cd0a8e297
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…, and reject an invalid IPFIX scope field count Two independent codex findings: - NfMapID was only (source_id, template_id), and both are chosen independently by each exporter, not assigned globally. Two different exporters picking the same values collided in the process-global template maps -- and could erase each other's valid templates via the data<->options erase() added earlier in this PR. The exporter's own network-layer address is now part of the key; FlowInputStream now populates NFSample::exporter_ip before parsing (both the pcap-replay and live-UDP paths already had this address available, just not threaded through). - process_netflow_v10_options_template read field_count but never validated scope_field_count against it, even though scope fields are defined as the first N of the total fields (RFC 7011 3.4.2.2) -- so scope_field_count can never legitimately exceed field_count. A template violating that was silently accepted and could likewise erase a valid data template for the same id.
2 participants
Summary
process_netflow_v9/process_netflow_v10currentlyreturn false(droppedpacket, counted in
packet_errors) in two cases that are normal exporterbehavior, not malformed input:
template for, because it's actually an options flowset governed by an
options template we don't parse. Exporters like softflowd and Cisco
interleave options template/data flowsets with real flow data in the same
datagram — today, if the options flowset happens to appear, the entire
datagram (including any real flow records after it) is discarded.
template refresh with no data flowsets at all. This is legitimate exporter
behavior (periodic template announcement) but was being counted as a parse
error.
Both inflate
packet_errorson healthy exporters and silently drop real flowdata whenever it happens to share a datagram with an options flowset.
Fix
flowset/set (
breakout of the switch case) instead of aborting the wholefunction — any other flowsets/sets in the same datagram are still
processed.
cleanly but yields zero flow records (template-only, or every flowset
skipped per above) now returns
true.total_flowsaccumulator (the same informationis available via
sample->flows.size()), since the fix removes the branchthat used to key off of it.
Testing
Added regression tests in
src/inputs/flow/test_flow.cppusing real.pcapfixtures (
tests/fixtures/nf9_options.pcap,tests/fixtures/ipfix_options.pcap),loaded through
FlowInputStreamthe same way the existing sflow/netflowfile tests are:
survive (previously dropped).
the common path.
Verified with a full local build (Conan + CMake) against the
unit-tests-input-flowtarget: all tests pass, no new compiler warnings.
Also validated the exact modified header standalone before the full build —
confirming the fix eliminates the drop and matches the previously reported
behavior exactly.
Files changed
src/inputs/flow/NetflowData.hsrc/inputs/flow/test_flow.cppsrc/handlers/flow/test_flows.cppsrc/tests/fixtures/nf9_options.pcapsrc/tests/fixtures/ipfix_options.pcap