Address review comments

Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
antrea-io · Feb 21, 2025 · 8942f97 · 8942f97
1 parent 47e3b8c
commit 8942f97
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 5 deletions.
diff --git a/build/charts/flow-aggregator/conf/flow-aggregator.conf b/build/charts/flow-aggregator/conf/flow-aggregator.conf
@@ -77,6 +77,7 @@ flowCollector:
 
   # 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.
   maxIPFIXMsgSize: {{ .Values.flowCollector.maxIPFIXMsgSize }}
 
 # clickHouse contains ClickHouse related configuration options.

diff --git a/build/charts/flow-aggregator/values.yaml b/build/charts/flow-aggregator/values.yaml
@@ -54,6 +54,7 @@ flowCollector:
   templateRefreshTimeout: "600s"
   # -- 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.
   maxIPFIXMsgSize: 0
 # clickHouse contains ClickHouse related configuration options.
 clickHouse:

diff --git a/build/yamls/flow-aggregator.yml b/build/yamls/flow-aggregator.yml
@@ -229,6 +229,7 @@ data:
 
       # 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.
       maxIPFIXMsgSize: 0
 
     # clickHouse contains ClickHouse related configuration options.
@@ -405,7 +406,7 @@ spec:
   template:
     metadata:
       annotations:
-        checksum/config: 96acfb574fbfb758e6388d677cbc8359c0375031fd68875d4ec2d03f34d2e49c
+        checksum/config: 13cccea100703f4180b6b432d44ec50b97c0508f9bdf06dd1ef4fbad08b4bed6
       labels:
         app: flow-aggregator
     spec:

diff --git a/pkg/config/flowaggregator/config.go b/pkg/config/flowaggregator/config.go
@@ -107,6 +107,7 @@ type FlowCollectorConfig struct {
 	TemplateRefreshTimeout string `yaml:"templateRefreshTimeout,omitempty"`
 	// 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.
 	MaxIPFIXMsgSize int32 `yaml:"maxIPFIXMsgSize,omitempty"`
 }
 

diff --git a/pkg/config/flowaggregator/default.go b/pkg/config/flowaggregator/default.go
@@ -30,6 +30,8 @@ const (
 	DefaultAggregatorTransportProtocol    = "TLS"
 	DefaultRecordFormat                   = "IPFIX"
 	DefaultTemplateRefreshTimeout         = "600s"
+	MinValidIPFIXMsgSize                  = 512
+	MaxValidIPFIXMsgSize                  = 65535
 
 	DefaultClickHouseDatabase       = "default"
 	DefaultClickHouseCommitInterval = "8s"

diff --git a/pkg/flowaggregator/exporter/ipfix.go b/pkg/flowaggregator/exporter/ipfix.go
@@ -169,9 +169,7 @@ func (e *IPFIXExporter) sendRecord(record ipfixentities.Record, isRecordIPv6 boo
 	if err := e.bufferedExporter.AddRecord(record); err != nil {
 		return err
 	}
-	if klog.V(7).Enabled() {
-		klog.InfoS("Data record added successfully")
-	}
+	klog.V(7).InfoS("Data record added successfully")
 	return nil
 }
 

diff --git a/pkg/flowaggregator/options/options.go b/pkg/flowaggregator/options/options.go
@@ -89,7 +89,7 @@ func LoadConfig(configBytes []byte) (*Options, error) {
 		return nil, err
 	}
 	// Validate flow collector specific parameters
-	if opt.Config.FlowCollector.Enable && len(opt.Config.FlowCollector.Address) > 0 {
+	if opt.Config.FlowCollector.Enable {
 		host, port, proto, err := flowexport.ParseFlowCollectorAddr(
 			opt.Config.FlowCollector.Address, flowaggregatorconfig.DefaultExternalFlowCollectorPort,
 			flowaggregatorconfig.DefaultExternalFlowCollectorTransport)
@@ -110,6 +110,18 @@ func LoadConfig(configBytes []byte) (*Options, error) {
 		if opt.TemplateRefreshTimeout < 0 {
 			return nil, fmt.Errorf("templateRefreshTimeout cannot be a negative duration")
 		}
+
+		if opt.Config.FlowCollector.MaxIPFIXMsgSize < 0 {
+			return nil, fmt.Errorf("maxIPFIXMsgSize cannot be negative")
+		}
+		if opt.Config.FlowCollector.MaxIPFIXMsgSize > 0 {
+			if opt.Config.FlowCollector.MaxIPFIXMsgSize < flowaggregatorconfig.MinValidIPFIXMsgSize {
+				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)
+			}
+		}
 	}
 	// Validate clickhouse specific parameters
 	if opt.Config.ClickHouse.Enable {