search

netsampler / goflow2

High performance sFlow/IPFIX/NetFlow Collector
BSD 3-Clause "New" or "Revised" License
487 stars 112 forks source link

decoder: fix netflow flowsets decoding #218

Closed TCheruy closed 1 year ago

TCheruy commented 1 year ago

Only last Flowset of Netflow packets is currently decoded (in v2), this PR ensure that all Flowsets are decoded

AlexandreYang commented 1 year ago

Hey @lspgn, Do you mind have a look at this possible bug? Looks like related in the recent v2 refactoring.

lspgn commented 1 year ago

Hmm I'll need to dive more into this (as this also changes the signature of a public function). Could this be solved by doing a loop in DecodeMessageNetflow?

TCheruy commented 1 year ago

The for loop in DecodeMessageCommon can indeed be moved to DecodeMessageNetflow and DecodeMessageIPFIX https://github.com/netsampler/goflow2/blob/34a0c1618e6fcf0d509c4553b3236b4aa85c71ca/decoders/netflow/netflow.go#L288 but it will also lead to a signature change (since the size argument won't be needed in DecodeMessageCommon anymore) Also this is common code to NetFlow and IPFIX so it makes sense to me to have the for loop in DecodeMessageCommon, WDYT ?

mscastanho commented 1 year ago

I've recently started experimenting with the project and went straight into the v2 code. Then I just realized I was getting way less flows than expected (looking at the raw IPFIX packets). Even packets containing 3-4 flows were always generating flowsets with a single flow on them.

So looks like this is the problem =)

Haven't tested the fix in this PR yet, though.

lspgn commented 1 year ago

Had a better look at it. Thank you very much for raising this!

I'm really not sure how I badly missed it when splitting the code from the v1... Turns out I was testing with single item flowsets:

https://github.com/netsampler/goflow2/blob/9287e0552896c46181bd9e8204479e92a82de4b8/decoders/netflow/netflow.go#L520-L524

I pushed a commit that split the function (had to change signature indeed)