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

IPFIX Export support for Kubernetes transformations #338

Merged
merged 27 commits into from
Jan 30, 2023

Conversation

praveingk
Copy link
Contributor

@praveingk praveingk commented Dec 6, 2022

This PR adds IPFIX export support for FLP o/p:

Way to enable this in config, for e.g.:

    write:
      type: ipfix
      ipfix:
        targetHost: 127.0.0.1
        targetPort: 9998
        transport: tcp
        enterpriseID: 27207

enterpriseID is a custom registry ID for kubernetes transformations, and other non-standard entries.

Main Changes in the following files:

  1. pkg/api/write_ipfix.go
  2. pkg/api/api.go
  3. pkg/config/config.go
  4. pkg/pipeline/pipeline_builder.go
  5. pkg/pipeline/write/write_ipfix.go
  6. go.mod

@KalmanMeth
Copy link
Collaborator

What is the purpose of the files added in the hack directory? testing?
Please add unit and/or end-to-end tests so they are run automatically with the rest of the flp tests.

pkg/pipeline/write/write_ipfix.go Outdated Show resolved Hide resolved
pkg/pipeline/pipeline_builder.go Show resolved Hide resolved
pkg/api/write_ipfix.go Outdated Show resolved Hide resolved
pkg/api/write_ipfix.go Outdated Show resolved Hide resolved
pkg/pipeline/write/write_ipfix.go Outdated Show resolved Hide resolved
@praveingk
Copy link
Contributor Author

What is the purpose of the files added in the hack directory? testing?
Please add unit and/or end-to-end tests so they are run automatically with the rest of the flp tests.

It was an example config for reference. Not intended for testing. I dint add a test, since I was unsure how ipfix export can be combined with the existing tests.

@praveingk
Copy link
Contributor Author

What is the purpose of the files added in the hack directory? testing?
Please add unit and/or end-to-end tests so they are run automatically with the rest of the flp tests.

It was an example config for reference. Not intended for testing. I dint add a test, since I was unsure how ipfix export can be combined with the existing tests.

I have removed the deployment manifest files

pkg/pipeline/write/write_ipfix.go Outdated Show resolved Hide resolved
hack/examples/ipfix-collector/ipfix-dump.go Outdated Show resolved Hide resolved
hack/examples/ipfix-collector/ipfix-dump.go Show resolved Hide resolved
hack/examples/ipfix-collector/ipfix-dump.go Outdated Show resolved Hide resolved
pkg/api/write_ipfix.go Outdated Show resolved Hide resolved
pkg/api/write_ipfix.go Outdated Show resolved Hide resolved
pkg/api/write_ipfix.go Show resolved Hide resolved
pkg/pipeline/write/write_ipfix.go Show resolved Hide resolved
pkg/pipeline/write/write_ipfix.go Outdated Show resolved Hide resolved
@praveingk
Copy link
Contributor Author

@KalmanMeth @ronensc Thanks for the detailed reviews. I think I have now addressed all the comments

praveingk and others added 3 commits January 19, 2023 09:56
This reverts commit 40bd9fc.
Co-authored-by: Ronen Schaffer <ronen.schaffer@ibm.com>
Copy link
Collaborator

@ronensc ronensc left a comment

Choose a reason for hiding this comment

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

LGTM
@praveingk Thank you for your contribution!

@praveingk
Copy link
Contributor Author

@ronensc @KalmanMeth @jotak Can this be merged if all tests are ok?

@codecov-commenter
Copy link

Codecov Report

Merging #338 (ce66d6c) into main (30d42fd) will decrease coverage by 7.49%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
- Coverage   69.23%   61.74%   -7.49%     
==========================================
  Files          88       91       +3     
  Lines        5204     5835     +631     
==========================================
  Hits         3603     3603              
- Misses       1382     2013     +631     
  Partials      219      219              
Flag Coverage Δ
unittests 61.74% <0.00%> (-7.49%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hack/examples/ipfix-collector/ipfix-dump.go 0.00% <0.00%> (ø)
pkg/api/write_ipfix.go 0.00% <0.00%> (ø)
pkg/config/config.go 63.33% <ø> (ø)
pkg/pipeline/pipeline_builder.go 71.06% <0.00%> (-0.46%) ⬇️
pkg/pipeline/write/write_ipfix.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jotak
Copy link
Member

jotak commented Jan 23, 2023

Thanks @praveingk
Code looks good to me, but I agree with @KalmanMeth that some tests would be appreciated, though not sure what the best/easiest approach is:

  • Listen on the target address to decode & check the received IPFIX data
  • Or mock the exporter and verify data to be sent

Another thing, perhaps for a follow-up: I think the templates need to be sent again periodically. Not sure if this is part of the spec or not, but I think it's a common thing to do, for resiliency (e.g. if listener restarts, or if it starts after the emitter)

@jotak
Copy link
Member

jotak commented Jan 23, 2023

Looking at the code, I think mocking the exporter would be a simpler approach (and the point is not to test the underlying go-ipfix lib). It seems like there's just 2 functions to mock, NewTemplateID and SendSet, and the latter can be used to verify the content sent

@praveingk
Copy link
Contributor Author

Thanks @praveingk Code looks good to me, but I agree with @KalmanMeth that some tests would be appreciated, though not sure what the best/easiest approach is:

  • Listen on the target address to decode & check the received IPFIX data
  • Or mock the exporter and verify data to be sent

Another thing, perhaps for a follow-up: I think the templates need to be sent again periodically. Not sure if this is part of the spec or not, but I think it's a common thing to do, for resiliency (e.g. if listener restarts, or if it starts after the emitter)

Thanks @jotak, IPFIX library by itself sends the templates periodically every 1 sec.

Looking at the code, I think mocking the exporter would be a simpler approach (and the point is not to test the underlying go-ipfix lib). It seems like there's just 2 functions to mock, NewTemplateID and SendSet, and the latter can be used to verify the content sent

Sure @jotak, I will check the exporter for reference, Do you think it should be part of this PR, or a separate PR for that would be better?

@KalmanMeth
Copy link
Collaborator

@praveingk cc: @jotak I think we can proceed to merge this PR and prepare a new PR with the tests.

@praveingk
Copy link
Contributor Author

Thanks @KalmanMeth

@jotak
Copy link
Member

jotak commented Jan 30, 2023

@KalmanMeth @praveingk sure, thanks for the PR!

@jotak jotak merged commit 3633f12 into netobserv:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants