[FlowExporter] More efficient IP checks by antoninbas · Pull Request #6960 · antrea-io/antrea

antoninbas · 2025-01-30T01:09:48Z

The FlowExporter in the Agent queries the NodeRouteController to determine whether the source / destination IPs are Pod IPs (NodeIPAM only). Prior to this change, these checks were expensive, involving an indexer lookup and conversions between different IP formats. The new implementation is about 10x faster, and performs no memory allocations.

The new implementation introduces a new set in the NodeRouteController, dedicated to storing all the PodCIDRs in the cluster. While I considered removing the dependency of the FlowExporter on the NodeRouteController altogether, it would have been a much bigger change. Additionally, in the long term, we could consider removing these checks from the FlowExporter altogether, and pushing the logic to the FlowAggregator.

We also make a few additional changes to the FlowExporter:

more consistently ignore connections where the source / destination IP is a gateway IP
classify Pod-to-Service traffic where the destination IP is not a Pod IP as Pod-to-External
log a warning if FlowExporter is enabled alongside AntreaIPAM

pkg/agent/flowexporter/exporter/exporter.go

luolanzone · 2025-02-11T09:36:52Z

pkg/agent/flowexporter/exporter/exporter.go


 func (exp *FlowExporter) exportConn(conn *flowexporter.Connection) error {
 	conn.FlowType = exp.findFlowType(*conn)
+	if conn.FlowType == flowTypeUnsupported {


So we will export the flowTypeUnknown flow anyway? or it's actually not possible when the FlowExporter is running? It seems the nodeRouteController will be nil only when it's running as a VM agent, Do we support FlowExporter when Antrea is running as a VM agent?

I preserved the existing behavior, we can revisit it later.

antoninbas · 2025-02-12T21:21:22Z

pkg/agent/flowexporter/exporter/exporter.go

+		return flowTypeUnsupported
 	}
-	if exp.nodeRouteController.IPInPodSubnets(conn.FlowKey.SourceAddress.AsSlice()) {
-		if conn.Mark&openflow.ServiceCTMark.GetRange().ToNXRange().ToUint32Mask() == openflow.ServiceCTMark.GetValue() || exp.nodeRouteController.IPInPodSubnets(conn.FlowKey.DestinationAddress.AsSlice()) {


@heanlan @yuntanghsu could you take a look at this change?
I tried to look through the Git history, but could not find the rationale for this check (conn.Mark&openflow.ServiceCTMark.GetRange().ToNXRange().ToUint32Mask() == openflow.ServiceCTMark.GetValue()) and there was no comment in the code. Why are we checking if the flow is a "Service flow"? it feels to me like the VIP does not matter when classifying the flow. Only the actual destination matters (is it a Pod and if yes, is it a local Pod or not?).

I think this one might be copied and paste from deny_connections where we used this condition to determine whether we need to fill the service information?
I cannot remember it clearly.
But if it is a service flow, will LookupIPInPodSubnets return true for isPod? Because I think if a flow is a service flow then it must be FlowTypeInterNode or FlowTypeIntraNode?

For Service traffic, conn.FlowKey.DestinationAddress will always be the resolved Endpoint IP, not the ClusterIP, even at the source Node. This is ensured by using conn.TupleReply.IP.SourceAddress:

antrea/pkg/agent/flowexporter/connections/conntrack_linux.go

Lines 136 to 142 in b3c40b0

FlowKey: flowexporter.Tuple{

SourceAddress: conn.TupleOrig.IP.SourceAddress,

DestinationAddress: conn.TupleReply.IP.SourceAddress,

Protocol: conn.TupleOrig.Proto.Protocol,

SourcePort: conn.TupleOrig.Proto.SourcePort,

DestinationPort: conn.TupleReply.Proto.SourcePort,

},

So yes LookupIPInPodSubnets will return true for Pod-to-Service traffic, both at the source Node and destination Node (incl. if these 2 Nodes are the same).

All the tests are passing, so I have to assume that the new logic is valid...

luolanzone

LGTM overall, two nits.

pkg/agent/controller/noderoute/node_route_controller.go

The FlowExporter in the Agent queries the NodeRouteController to determine whether the source / destination IPs are Pod IPs (NodeIPAM only). Prior to this change, these checks were expensive, involving an indexer lookup and conversions between different IP formats. The new implementation is about 10x faster, and peforms no memory allocations. The new implementation introduces a new set in the NodeRouteController, dedicated to storing all the PodCIDRs in the cluster. While I considered removing the dependency of the FlowExporter on the NodeRouteController altogether, it would have been a much bigger change. Additionally, in the long term, we could consider removing these checks from the FlowExporter altogether, and pushing the logic to the FlowAggregator. We also make a few additional changes to the FlowExporter: * more consistently ignore connections where the source / destination IP is a gateway IP * classify Pod-to-Service traffic where the destination IP is not a Pod IP as Pod-to-External * log a warning if FlowExporter is enabled alongside AntreaIPAM Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

antoninbas · 2025-02-14T21:29:17Z

/test-all

luolanzone

LGTM

yuntanghsu · 2025-02-20T19:24:06Z

pkg/agent/flowexporter/exporter/exporter.go

+		return flowTypeUnsupported
 	}
-	if exp.nodeRouteController.IPInPodSubnets(conn.FlowKey.SourceAddress.AsSlice()) {
-		if conn.Mark&openflow.ServiceCTMark.GetRange().ToNXRange().ToUint32Mask() == openflow.ServiceCTMark.GetValue() || exp.nodeRouteController.IPInPodSubnets(conn.FlowKey.DestinationAddress.AsSlice()) {


I think this one might be copied and paste from deny_connections where we used this condition to determine whether we need to fill the service information?
I cannot remember it clearly.
But if it is a service flow, will LookupIPInPodSubnets return true for isPod? Because I think if a flow is a service flow then it must be FlowTypeInterNode or FlowTypeIntraNode?

yuntanghsu · 2025-02-20T19:25:36Z

pkg/agent/flowexporter/exporter/exporter.go

-			}
-			return ipfixregistry.FlowTypeIntraNode
+	if !srcIsPod {
+		if klog.V(5).Enabled() {


So we haven't supported external to pod flow, right?

No, we have never supported this case, and we still don't.

yuntanghsu · 2025-02-20T19:43:05Z

pkg/agent/flowexporter/exporter/exporter.go

 		}
+		return flowTypeUnsupported
+	}
+	if !dstIsPod {


Please see the comment above. I think the destination can be a service ip and it would be an inter/intra Node flow?

I answered the comment above. conn.FlowKey.DestinationAddress will never be a Service VIP.
At the source Node (and only at the source Node), conn.OriginalDestinationAddress will store the Service VIP.

antoninbas force-pushed the improve-pod-ip-checks-in-flowexporter branch 2 times, most recently from 1d444b8 to 6b4b8e2 Compare January 30, 2025 06:08

antoninbas added this to the Antrea v2.3 release milestone Feb 7, 2025

luolanzone reviewed Feb 11, 2025

View reviewed changes

antoninbas force-pushed the improve-pod-ip-checks-in-flowexporter branch from dbcf705 to e1dac0b Compare February 12, 2025 21:15

antoninbas requested review from heanlan and yuntanghsu February 12, 2025 21:16

antoninbas marked this pull request as ready for review February 12, 2025 21:16

antoninbas commented Feb 12, 2025

View reviewed changes

antoninbas requested a review from luolanzone February 12, 2025 22:04

luolanzone reviewed Feb 14, 2025

View reviewed changes

pkg/agent/controller/noderoute/node_route_controller.go Outdated Show resolved Hide resolved

pkg/agent/controller/noderoute/node_route_controller.go Outdated Show resolved Hide resolved

antoninbas added 2 commits February 14, 2025 13:27

Address review comments

970411f

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

antoninbas force-pushed the improve-pod-ip-checks-in-flowexporter branch from e1dac0b to 970411f Compare February 14, 2025 21:29

antoninbas requested a review from luolanzone February 14, 2025 21:29

luolanzone approved these changes Feb 15, 2025

View reviewed changes

yuntanghsu reviewed Feb 20, 2025

View reviewed changes

antoninbas merged commit f74eee0 into antrea-io:main Feb 21, 2025
58 of 63 checks passed

antoninbas deleted the improve-pod-ip-checks-in-flowexporter branch February 21, 2025 04:47

	FlowKey: flowexporter.Tuple{
	SourceAddress: conn.TupleOrig.IP.SourceAddress,
	DestinationAddress: conn.TupleReply.IP.SourceAddress,
	Protocol: conn.TupleOrig.Proto.Protocol,
	SourcePort: conn.TupleOrig.Proto.SourcePort,
	DestinationPort: conn.TupleReply.Proto.SourcePort,
	},

Conversation

antoninbas commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luolanzone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antoninbas commented Feb 14, 2025

Uh oh!

luolanzone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

antoninbas commented Jan 30, 2025 •

edited

Loading