Upgrade go-ipfix to v0.13.0 #6998

antoninbas · 2025-02-14T00:24:23Z

As part of this upgrade, we make the following changes:

Switch to buffered IPFIX exporter in the Flow Aggregator. This
exporter has better performance for UDP IPFIX messages, by ensuring
that multiple data records can be batched together in a single
message.
Provide Path MTU (PMTU) when creating the IPFIX exporter in the Flow
Aggregator. The value is used by the new buffered exporter to
determine how many IPFIX records can fit in a single message while
avoiding IP fragmentation. In our case, we "approximate" the Path MTU
by looking up the MTU of the Flow Aggregator Pod's eth0 interface.
Add a MaxMsgSize configuration parameter to the Flow Aggregator as a
way to override the default behavior, which is to use the MTU (minus
header overhead) when the UDP protocol is used.
Add periodic flushing when exporting IPFIX records, which is necessary
after switching to the buffered exporter. In Aggregation mode,
flushing happens after processing a given batch of expired records. In
Proxy mode, flushing happens every second.
Use updated reference IPFIX collector in e2e tests. The updated
collector handles the case where multiple data records are included in
the same IPFIX message more gracefully, which leads to some
simplification in the test code.

luolanzone · 2025-02-20T02:04:03Z

pkg/flowaggregator/exporter/ipfix.go

@@ -144,7 +150,8 @@ func (e *IPFIXExporter) UpdateOptions(opt *options.Options) {
 		e.observationDomainID = genObservationDomainID(e.clusterUUID)
 	}
 	e.templateRefreshTimeout = opt.TemplateRefreshTimeout
-	klog.InfoS("New IPFIXExporter configuration", "collectorAddress", e.externalFlowCollectorAddr, "collectorProtocol", e.externalFlowCollectorProto, "sendJSON", e.sendJSONRecord, "domainID", e.observationDomainID, "templateRefreshTimeout", e.templateRefreshTimeout)
+	e.maxIPFIXMsgSize = int(opt.Config.FlowCollector.MaxIPFIXMsgSize)


Any range for this? should we check if the value provided by the user is a reasonable value?

I added validation code, using the same limits as in the go-ipfix library

pkg/flowaggregator/flowaggregator.go

tnqn · 2025-02-20T14:02:35Z

pkg/flowaggregator/exporter/ipfix.go

 	if klog.V(7).Enabled() {
-		klog.InfoS("Data set sent successfully", "bytes sent", sentBytes)
+		klog.InfoS("Data record added successfully")
 	}


should it just call klog.V(7).InfoS("Data record added successfully"), getting rid of Enabled check?

I assume you are suggesting this because the "cost" is the same in the absence of fields to log?

Right, it's not common to have a dedicated Enabled check when there is no big overhead to serialize the values as it could have been written in one line.

tnqn · 2025-02-20T15:23:00Z

pkg/flowaggregator/exporter/ipfix.go

 	if e.exportingProcess != nil {
 		e.exportingProcess.CloseConnToCollector()


should it flush bufferedExporter first like Stop()?

I would say it's reasonable not to flush in case of error?

This is when user changes the configuration, right? If not flushed, could some records received in the last second be lost. My understanding this is similar to Stop + Start, so I wonder why we flush upon Stop but not here.

you're right actually, I thought you left that comment in a different place. I'll make the change

pkg/flowaggregator/exporter/ipfix.go

As part of this upgrade, we make the following changes: * Switch to buffered IPFIX exporter in the Flow Aggregator. This exporter has better performance for UDP IPFIX messages, by ensuring that multiple data records can be batched together in a single message. * Provide Path MTU (PMTU) when creating the IPFIX exporter in the Flow Aggregator. The value is used by the new buffered exporter to determine how many IPFIX records can fit in a single message while avoiding IP fragmentation. In our case, we "approximate" the Path MTU by looking up the MTU of the Flow Aggregator Pod's eth0 interface. * Add a MaxMsgSize configuration parameter to the Flow Aggregator as a way to override the default behavior, which is to use the MTU (minus header overhead) when the UDP protocol is used. * Add periodic flushing when exporting IPFIX records, which is necessary after switching to the buffered exporter. In Aggregation mode, flushing happens after processing a given batch of expired records. In Proxy mode, flushing happens every second. * Use updated reference IPFIX collector in e2e tests. The updated collector handles the case where multiple data records are included in the same IPFIX message more gracefully, which leads to some simplification in the test code. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

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

luolanzone

LGTM overall, one nit.

luolanzone · 2025-02-21T02:06:33Z

build/charts/flow-aggregator/README.md

@@ -34,6 +34,7 @@ Kubernetes: `>= 1.19.0-0`
 | flowAggregatorAddress | string | `""` | Provide an extra DNS name or IP address of flow aggregator for generating TLS certificate. |
 | flowCollector.address | string | `""` | Provide the flow collector address as string with format <IP>:<port>[:<proto>],  where proto is tcp or udp. If no L4 transport proto is given, we consider tcp as default. |
 | flowCollector.enable | bool | `false` | Determine whether to enable exporting flow records to external flow collector. |
+| flowCollector.maxIPFIXMsgSize | int | `0` | Maximum message size to use for IPFIX records. If set to 0 (recommended), a reasonable default value will be used based on the protocol (tcp or udp) used to connect to the collector. |


Suggested change

| flowCollector.maxIPFIXMsgSize | int | `0` | Maximum message size to use for IPFIX records. If set to 0 (recommended), a reasonable default value will be used based on the protocol (tcp or udp) used to connect to the collector. |

| flowCollector.maxIPFIXMsgSize | int | `0` | Maximum message size to use for IPFIX records. If set to 0 (recommended), a reasonable default value will be used based on the protocol (tcp or udp) used to connect to the collector. Min valid value is 512 and max valid value is 65535. |

antoninbas · 2025-02-21T02:58:14Z

/test-all

antoninbas · 2025-02-21T02:58:31Z

closed by mistake...

antoninbas · 2025-02-21T02:58:35Z

/test-all

tnqn

LGTM, one minor comment about inconsistent error messages

tnqn · 2025-02-21T03:23:35Z

pkg/flowaggregator/options/options.go

+				return nil, fmt.Errorf("maxIPFIXMsgSize cannot be smaller than minimum valid IPFIX mesage size %d", flowaggregatorconfig.MinValidIPFIXMsgSize)
+			}
+			if opt.Config.FlowCollector.MaxIPFIXMsgSize > flowaggregatorconfig.MaxValidIPFIXMsgSize {
+				return nil, fmt.Errorf("maxIPFIXMsgSize cannot be greater than the maximum valid IPFIX mesage size %d", flowaggregatorconfig.MaxValidIPFIXMsgSize)


should the error message have "the" in front of minimum/maximum consistently?

done, thanks

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

antoninbas · 2025-02-21T03:25:58Z

/test-all

tnqn

LGTM

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

antoninbas added the area/flow-visibility Issues or PRs related to flow visibility support in Antrea label Feb 14, 2025

antoninbas force-pushed the upgrade-go-ipfix-to-0.13.0 branch 4 times, most recently from 09b8c16 to 31bbde9 Compare February 19, 2025 20:54

antoninbas marked this pull request as ready for review February 19, 2025 20:54

antoninbas requested a review from luolanzone February 19, 2025 20:55

luolanzone reviewed Feb 20, 2025

View reviewed changes

tnqn reviewed Feb 20, 2025

View reviewed changes

antoninbas added 2 commits February 20, 2025 16:00

Address review comments

8942f97

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

antoninbas force-pushed the upgrade-go-ipfix-to-0.13.0 branch from 31bbde9 to 8942f97 Compare February 21, 2025 00:00

luolanzone reviewed Feb 21, 2025

View reviewed changes

antoninbas closed this Feb 21, 2025

antoninbas reopened this Feb 21, 2025

antoninbas requested review from tnqn and luolanzone February 21, 2025 02:58

antoninbas force-pushed the upgrade-go-ipfix-to-0.13.0 branch from 17e2d9d to cb16327 Compare February 21, 2025 03:19

tnqn previously approved these changes Feb 21, 2025

View reviewed changes

Address review comments

314fd64

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

antoninbas dismissed tnqn’s stale review via 314fd64 February 21, 2025 03:25

antoninbas force-pushed the upgrade-go-ipfix-to-0.13.0 branch from cb16327 to 314fd64 Compare February 21, 2025 03:25

tnqn approved these changes Feb 21, 2025

View reviewed changes

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

antoninbas deleted the upgrade-go-ipfix-to-0.13.0 branch February 21, 2025 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade go-ipfix to v0.13.0 #6998

Upgrade go-ipfix to v0.13.0 #6998

antoninbas commented Feb 14, 2025 •

edited

Loading

luolanzone Feb 20, 2025

antoninbas Feb 21, 2025

tnqn Feb 20, 2025

antoninbas Feb 20, 2025

tnqn Feb 21, 2025

tnqn Feb 20, 2025

antoninbas Feb 21, 2025

tnqn Feb 21, 2025

antoninbas Feb 21, 2025

luolanzone left a comment

luolanzone Feb 21, 2025

antoninbas commented Feb 21, 2025

antoninbas commented Feb 21, 2025

antoninbas commented Feb 21, 2025

tnqn left a comment

tnqn Feb 21, 2025

antoninbas Feb 21, 2025

antoninbas commented Feb 21, 2025

tnqn left a comment

		if e.exportingProcess != nil {
		e.exportingProcess.CloseConnToCollector()

	\| flowCollector.maxIPFIXMsgSize \| int \| `0` \| Maximum message size to use for IPFIX records. If set to 0 (recommended), a reasonable default value will be used based on the protocol (tcp or udp) used to connect to the collector. \|
	\| flowCollector.maxIPFIXMsgSize \| int \| `0` \| Maximum message size to use for IPFIX records. If set to 0 (recommended), a reasonable default value will be used based on the protocol (tcp or udp) used to connect to the collector. Min valid value is 512 and max valid value is 65535. \|

Upgrade go-ipfix to v0.13.0 #6998

Upgrade go-ipfix to v0.13.0 #6998

Conversation

antoninbas commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Feb 21, 2025

antoninbas commented Feb 21, 2025

antoninbas commented Feb 21, 2025

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Feb 21, 2025

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Feb 14, 2025 •

edited

Loading