NETOBSERV-915 skip reinterpret direction for conversations

[jpinsonneau]

Skip reinterpretDirection for conversation events as source and destination can be swapped by TCP flag SYN_ACK interpretation.

Related PRs:

• NETOBSERV-915 add conversation direction network-observability-operator#313
• NETOBSERV-915 add conversation direction network-observability-console-plugin#317

[codecov]

Codecov Report

Merging #430 (3a4e9a8) into main (95395fc) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
+ Coverage   64.74%   64.75%   +0.01%     
==========================================
  Files          94       94              
  Lines        6651     6654       +3     
==========================================
+ Hits         4306     4309       +3     
  Misses       2102     2102              
  Partials      243      243              

Flag	Coverage Δ
unittests	64.75% <100.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
pkg/pipeline/transform/transform_network.go	73.11% <100.00%> (+0.44%)	⬆️

[Amoghrd]

/ok-to-test

[github-actions]

New image: quay.io/netobserv/flowlogs-pipeline:ef3ec70. It will expire after two weeks.

[github-actions]

New image: quay.io/netobserv/flowlogs-pipeline:b5b03e5. It will expire after two weeks.

[Amoghrd]

/ok-to-test

[Amoghrd]

/label qe-approved

[jotak]

Just to clarify: are we safe doing so just because the "Reporter" field is anyway disabled in UI for Conversation views?

I'm asking because, else, it's not very clear to me why we would skip reinterpret for conntrack. It was added because the concept of INGRESS/EGRESS direction, provided be the ebpf agent, is bound to an interface and not to the whole node as expected with our "Reporter" definition (well explained by Mario here: https://issues.redhat.com/browse/NETOBSERV-696?focusedId=21315152&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-21315152 )

So the "reinterpret" is a way to "normalize" what we consider as ingress or egress, and I don't see why it should be different with conntrack events - but if the "Reporter" setting is disabled anyway for conntrack, then I guess that's ok

PS: I wonder if doing the "reinterpret" before processing conntrack wouldn't have also fixed the issues that we had

[jotak]

/lgtm
but still curious about the explanation :)

[jpinsonneau]

Just to clarify: are we safe doing so just because the "Reporter" field is anyway disabled in UI for Conversation views?

The goal is to enable it netobserv/network-observability-operator#313

I'm asking because, else, it's not very clear to me why we would skip reinterpret for conntrack. It was added because the concept of INGRESS/EGRESS direction, provided be the ebpf agent, is bound to an interface and not to the whole node as expected with our "Reporter" definition (well explained by Mario here: https://issues.redhat.com/browse/NETOBSERV-696?focusedId=21315152&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-21315152 )

So the "reinterpret" is a way to "normalize" what we consider as ingress or egress, and I don't see why it should be different with conntrack events - but if the "Reporter" setting is disabled anyway for conntrack, then I guess that's ok

As far as I have seen in my tests, it conflicts with the swapAB capability as it just update src / dst but doesn't affect Direction / IfDirection.
Keeping the original values avoid mixing unconsistent check on srcNode == reporter.
Some alternatives could be:

• disable swapAB
• keep track of swaps to manage this case in reinterpretDirection
• update Direction when swapping (may need hardcoded checks or will improve code complexity / config)

PS: I wonder if doing the "reinterpret" before processing conntrack wouldn't have also fixed the issues that we had
It would; but today everything is mixed inside the enrich stage that must be after connection tracking. Keeping this reinterpret before would force us to add an extra stage at first and the actual enrich. It will impact performances.

Why don't we do that reinterpretation in eBPF agent ?

[jpinsonneau]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpinsonneau

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 [jpinsonneau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment