Add new field to ipfix exporter for flowRtt

bhale commented 7 months ago

Description

I am working on implementing a fix for https://github.com/netobserv/flowlogs-pipeline/issues/544

There are many new fields added to the pipeline but not yet written via IPFIX. I attempted to start with a single field and get feedback before I added several fields using a bad approach.

Dependencies

None

Checklist

I am not familiar with the processes

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

[ ] Will this change affect NetObserv / Network Observability operator? If not, you can ignore the rest of this checklist.
[ ] Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
[ ] Does this PR require product documentation?
- [ ] If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
[ ] Does this PR require a product release notes entry?
- [ ] If so, fill in "Release Note Text" in the JIRA.
[ ] Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
- [ ] If so, make sure it is described in the JIRA ticket.
QE requirements (check 1 from the list):
- [ ] Standard QE validation, with pre-merge tests unless stated otherwise.
- [ ] Regression tests only (e.g. refactoring with no user-facing change).
- [ ] No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

openshift-ci[bot] commented 7 months ago

Hi @bhale. Thanks for your PR.

I'm waiting for a netobserv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

bhale commented 7 months ago

I think this pattern is going badly, partly because there isn't a logical place to put the new fields... the previous Kubernetes enrichment section added strings that don't change or contain values, like source namespace. With the new data being produced upstream in the ebpf agent, there are new fields we want to export that are not static data / enrichment, or data that already exists in an IANA data model - such as timeFlowRttNs. Should we be looking to create a third set of fields, or am I missing a better solution?

jotak commented 7 months ago

Hi @bhale , thanks for the contrib!

I am not super familiar with the ipfix code, perhaps if @praveingk is around he can better answer that me. I'm not sure to understand what you mean when saying "the previous Kubernetes enrichment section added strings that don't change or contain values" ... kube enrichment should set values and vary according to the src/dst IPs found in the flows, this is not supposed to be static ... or I'm misunderstanding something.

That said, it seems indeed wrong to tie agent-specific features such as RTT to the K8s model, as this isn't related to k8s. I'm not sure how to do that (as said I'm not very familiar with this code) but I can take a look and try

praveingk commented 7 months ago

@bhale Thanks for addressing this. I had previously added enrichEnterpriseID to support fields that are not supported by IANA registry and a custom registry. In this case, RTT isn't supported by IANA, and it makes sense to add a separate function and not adding as part of K8s enrichment.

jotak commented 7 months ago

@bhale my PR https://github.com/netobserv/flowlogs-pipeline/pull/606 adds a test with ipfix that should be helpful here There will be a small conflict with your PR, as you'll then need to add any additional field managed in a global list here: https://github.com/netobserv/flowlogs-pipeline/pull/606/files#diff-9f1ae1c930f06df67c0493ad41f8774ae4fa31832f12f7a8d4f38f0ffa534b74R79-R81

bhale commented 7 months ago

@bhale my PR #606 adds a test with ipfix that should be helpful here There will be a small conflict with your PR, as you'll then need to add any additional field managed in a global list here: https://github.com/netobserv/flowlogs-pipeline/pull/606/files#diff-9f1ae1c930f06df67c0493ad41f8774ae4fa31832f12f7a8d4f38f0ffa534b74R79-R81

This is a huge help thank you! I will rebase on these changes tomorrow, it looks like it will simplify things.

bhale commented 7 months ago

Thanks for all your help and advice, @praveingk and @jotak! I incorporated the refactoring and tests from jotak, integrated my changes to export timeFlowRttNs, and added a test for it. The test passes, and I can see valid data for RTT in IBM SevOne for the new RTT field.

Regarding the comment on using field id 7739 - my colleague has access to a database at IBM that tracks the fields for our enterpriseId. Someone had already claimed 7739, so I skipped it.

jotak commented 7 months ago

Thanks for all your help and advice, @praveingk and @jotak! I incorporated the refactoring and tests from jotak, integrated my changes to export timeFlowRttNs, and added a test for it. The test passes, and I can see valid data for RTT in IBM SevOne for the new RTT field.

Nice 👍

Regarding the comment on using field id 7739 - my colleague has access to a database at IBM that tracks the fields for our enterpriseId. Someone had already claimed 7739, so I skipped it.

ok .. I don't know why exactly it starts at 7733, it would probably have been better if we had a range that wouldn't overlap with other's using the same enterprise id (if someone not from IBM wants to add a new field, I guess we'll need to coordinate with you ; or change the way it works to allow custom field ids)

jotak commented 7 months ago

btw I've merged my PR, this one should be rebased

bhale commented 7 months ago

btw I've merged my PR, this one should be rebased

Rebase good to go!

rupertgregoryibm commented 7 months ago

@jotak FYI, yes, between myself & Dale Bowie, we track the field ID allocations. The 7xxx starting number was partly looking for a range that doesn't overlap with other IBM products. We should probably switch it from private to public git at some point.

github-actions[bot] commented 7 months ago

New image: quay.io/netobserv/flowlogs-pipeline:71f5ae6

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=71f5ae6 make set-flp-image

jotak commented 7 months ago

/lgtm /approve Thank you @bhale !

openshift-ci[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/netobserv/flowlogs-pipeline/blob/main/OWNERS)~~ [jotak] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 66.95%. Comparing base (ba7ad1f) to head (6dd5537). Report is 3 commits behind head on main.

Files	Patch %	Lines
pkg/pipeline/write/write_ipfix.go	0.00%	11 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #603 +/- ## ========================================== - Coverage 67.03% 66.95% -0.08% ========================================== Files 103 103 Lines 7401 7412 +11 ========================================== + Hits 4961 4963 +2 - Misses 2147 2157 +10 + Partials 293 292 -1 ``` | [Flag](https://app.codecov.io/gh/netobserv/flowlogs-pipeline/pull/603/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=netobserv) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/netobserv/flowlogs-pipeline/pull/603/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=netobserv) | `66.95% <0.00%> (-0.08%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=netobserv#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

netobserv / flowlogs-pipeline