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

NETOBSERV-759: decorate flows with agent IP #78

Merged
merged 2 commits into from
Dec 20, 2022
Merged

NETOBSERV-759: decorate flows with agent IP #78

merged 2 commits into from
Dec 20, 2022

Conversation

mariomac
Copy link

@mariomac mariomac commented Dec 19, 2022

Only for Protocol Buffers encoding. IPFIX is still missing.

This PR creates a new "Decorator" instance and moves there also the interface naming process.

At least for OVN-K, the default external IP parsing coincides with the Node IP without any kind of tuning from the NOO:
image

only for Protocol Buffers encoding. IPFIX is still missing.
@codecov-commenter
Copy link

Codecov Report

Merging #78 (94114f0) into main (0e3f5ea) will increase coverage by 1.31%.
The diff coverage is 76.29%.

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   36.71%   38.02%   +1.31%     
==========================================
  Files          26       28       +2     
  Lines        1841     1959     +118     
==========================================
+ Hits          676      745      +69     
- Misses       1134     1176      +42     
- Partials       31       38       +7     
Flag Coverage Δ
unittests 38.02% <76.29%> (+1.31%) ⬆️

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

Impacted Files Coverage Δ
pkg/exporter/ipfix.go 0.00% <ø> (ø)
pkg/flow/record.go 82.85% <ø> (-0.48%) ⬇️
pkg/agent/ip.go 70.40% <70.40%> (ø)
pkg/agent/agent.go 39.06% <88.46%> (-4.12%) ⬇️
pkg/flow/account.go 82.35% <100.00%> (-0.34%) ⬇️
pkg/flow/decorator.go 100.00% <100.00%> (ø)
pkg/flow/tracer_map.go 80.51% <100.00%> (-0.50%) ⬇️
pkg/test/export_fake.go 82.35% <100.00%> (+1.10%) ⬆️

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

@eranra
Copy link
Collaborator

eranra commented Dec 19, 2022

@mariomac can you provide some motivation for this enrichment :-) --- we have the network Interface ID --- can't we get that data from k8s and not send that field over and over for each flow?

@mariomac
Copy link
Author

@eranra We want to provide the IP of the reporter to help us deciding whether a flow is going outside the node or comes from another node (see: netobserv/flowlogs-pipeline#350)

Also, flows can be also captured from virtual interfaces or physical interfaces that do no lead to external traffic.

@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 20, 2022
@github-actions
Copy link

New image: ["quay.io/netobserv/netobserv-ebpf-agent:a05f544"]. It will expire after two weeks.

@jotak
Copy link
Member

jotak commented Dec 20, 2022

@eranra do you have strong concerns about this?
It's really solving a lot of issues that we have, regarding the interpretation of FlowDirection (what we assume on frontend side != what it is at the low level, and knowing the Agent IP was the missing piece between them)

@jotak
Copy link
Member

jotak commented Dec 20, 2022

tested and works as expected
@praveingk do you want to review? You know better the code than me :)

@@ -12,6 +12,9 @@ import (

var ilog = logrus.WithField("component", "exporter/IPFIXProto")

// TODO: encode also the equivalent of the Protobuf's AgentIP field in a format that is binary-
Copy link
Member

Choose a reason for hiding this comment

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

For information, in goflow's IPFIX this was called "SamplerAddress": https://github.com/cloudflare/goflow#output-format

In IPFIX spec (https://www.iana.org/assignments/ipfix/ipfix.xhtml) this is exporterIPv4Address / exporterIPv6Address

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it what you already have in exporter *ipfixExporter.ExportingProcess ?
@praveingk

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jotak the ExportingProcess has a net.Conn, which could be used to get the info. Yes, exporterIPv4(/6)Address seems right though

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, the ExportingProcess could be used only if the AgentIPIface is set as "External"

type InterfaceNamer func(ifIndex int) string

// Decorate adds to the flows extra metadata fields that are not directly fetched by eBPF:
// - The interface name (corresponding to the interface index in the flow).
Copy link
Member

Choose a reason for hiding this comment

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

@mariomac the interface name was already in the flows, is it just that you refactored at the same time the way it is added, right? Or did you change the flow content wrt interface?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it was added in an earlier stage in the flow creation, but the constructor required to inject an external component (interfaceNamer) and I think that moving the extra decoration to another stage makes the code cleaner.

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

thanks!

@mariomac
Copy link
Author

Merging to unblock other PRs/tests and, if @praveingk has any concern about the current PR, we can address them into another PR.

@mariomac mariomac merged commit aa7838d into netobserv:main Dec 20, 2022
@mariomac mariomac deleted the agent-ip branch December 20, 2022 15:55
@eranra
Copy link
Collaborator

eranra commented Dec 21, 2022

@eranra do you have strong concerns about this? It's really solving a lot of issues that we have, regarding the interpretation of FlowDirection (what we assume on frontend side != what it is at the low level, and knowing the Agent IP was the missing piece between them)

@jotak I am actually on vacation and not in front of the computer ( maybe a meeting could be helpful on that) --- and I understand the value of knowing which agent reported about a flow record --- I think that this might be concluded differently from the interface ID that we already reported, so this makes this field redundant. I am not super opinionated on this addition, I think this adds data over and over in each flow record we report, so it makes sense to think if we need that.

shach33 pushed a commit to shach33/netobserv-ebpf-agent that referenced this pull request Jan 10, 2023
* NETOBSERV-759: decorate flows with agent IP

only for Protocol Buffers encoding. IPFIX is still missing.

* Fix GRPC export + test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants