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

Add normalization for IPFIX events #9616

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

somedatapacket
Copy link

Uses IPFIX information elements to normalize IPFIX records.

Uses IPFIX information elements to normalize IPFIX records.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@karmi
Copy link

karmi commented May 18, 2018

Hi @somedatapacket, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@somedatapacket
Copy link
Author

@karmi sorry about that, done.

@andrewvc
Copy link
Contributor

Jenkins, please test this

@somedatapacket
Copy link
Author

I added two additional translations/normalizations. There are some things that require more complicated logic, though, for this to be feature complete.

@andrewvc
Copy link
Contributor

Thanks @somedatapacket . @jsvd who would be a good person to review this?

@andrewvc
Copy link
Contributor

@somedatapacket what things are missing to complete the picture?

@somedatapacket
Copy link
Author

@andrewvc hopefully not much! but in my case I can't validate implementation of ipVersion normalization or flow direction, as these attributes are not included in the flow template I'm testing with.

@jsvd jsvd requested a review from colinsurprenant May 23, 2018 15:56
@colinsurprenant colinsurprenant self-assigned this May 23, 2018
@colinsurprenant colinsurprenant requested review from colinsurprenant and removed request for colinsurprenant May 23, 2018 20:17
@colinsurprenant
Copy link
Contributor

I am trying to figure how we could practically test this. The problem is that we do not have any tests which can validate the configuration file and prevent regressions especially for the filters logic. This is obviously a larger & separate issue than this PR purpose here but I'd like to see if we could come up with something practical that could help us validate these changes and help us move forward with figuring the way to add tests.

One idea that came up while discussin this internally was to use a tool like nfdump to capture and replay netflow data.

We could generate a config that preserves the filters and replace the input with a file or stdin input and similarly replace the output with a file or stdout output plus json codec. That way we could isolate the filter testing by replaying netflow data by sending it to stdin and validate the json output with what is expected, or something like that.

As a first step @somedatapacket would you be open in manually trying this methodology? If we could come up with a set of netflow data which tests the new parsing that way, this would validate the idea and then we could use that to bootstrap the testing framework in another issue.

WDYT?

@somedatapacket
Copy link
Author

@colinsurprenant internally, I have been testing this code with data from routers in our lab environment, but my methodology is probably insufficiently rigorous. Would something like this have utility? https://github.com/logstash-plugins/logstash-codec-netflow/blob/master/spec/codecs/ipfix_stress.py

@colinsurprenant
Copy link
Contributor

@somedatapacket good question - let's ping @jorritfolmer to see what his thoughts are on that since in essence anything done in that direction for the module will be usable for the codec testing too I guess.

@jorritfolmer
Copy link

@colinsurprenant @somedatapacket The logstash-input-udp rspec script would be a good place start. It actually can send a message through the udp socket as a client, and verifies the resulting Logstash event. Replace this message with one of the samples from our netflow packet library, and verify the result to be the expected normalised fields.

On a separate note: Isn't this file based on the upstream Elastiflow? It seems support for normalising IPFIX fields has already been added some time ago: https://github.com/robcowart/elastiflow/commits/master/logstash/elastiflow/conf.d/20_filter_30_ipfix.logstash.conf. It seems to me accepting PRs here would lead to duplication of effort that would be much better focussed on improving the state of art over there.

@somedatapacket
Copy link
Author

It sure looks similar! Elastiflow is new to me.

@somedatapacket
Copy link
Author

@colinsurprenant is this native plugin based on elastiflow? sorry for my ignorance here if so...

@jsvd
Copy link
Member

jsvd commented Jun 5, 2018

is this native plugin based on elastiflow?

It is indeed! #7715

Until recently both were in sync but now Elastiflow changed from the apache 2 license. We want to continue to provide an out of the box netflow experience through the netflow module that logstash ships with, but it is normal that these two projects will continue to deviate.

@somedatapacket
Copy link
Author

@jsvd @colinsurprenant what next steps could we take on this?

@colinsurprenant
Copy link
Contributor

@somedatapacket since adding automated tests is a project in itself I'd be ok to move forward with this PR given your own experience with it and create a separate issue for the testing framework.

@jsvd WDYT?

@somedatapacket
Copy link
Author

@colinsurprenant @jsvd i'm definitely in favor!

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.

8 participants