ICMP Rule with NodeLatencyMonitor #7011

Dhruv-J · 2025-02-20T19:06:00Z

This PR provides the solution to an edge case with NodeLatencyMonitor, where
the feature does not work if ICMP queries are blocked by default. To fix this,
an iptable rule will be added if NodeLatencyMonitor is enabled, such that ICMP
requests via the Antrea gateway will be allowed.

Fixes #6952

Signed-off-by: Dhruv-J [email protected]

antoninbas · 2025-03-04T08:04:48Z

@Dhruv-J you can rebase your branch and follow the same pattern as in #7030 (which has been merged)

This PR provides the solution to an edge case with NodeLatencyMonitor, where the feature does not work if ICMP queries are blocked by default. To fix this, an iptable rule will be added if NodeLatencyMonitor is enabled, such that ICMP requests via the Antrea gateway will be allowed. Fixes issue antrea-io#6952 Signed-off-by: Dhruv Jain <[email protected]>

Dhruv-J · 2025-03-14T07:45:44Z

still updating pipelines to pass, but will be ready for review right after

…r, fixed route_windows.go Signed-off-by: Dhruv Jain <[email protected]>

antoninbas · 2025-03-17T20:53:05Z

pkg/agent/route/route_linux.go

@@ -1245,6 +1258,38 @@ func (c *Client) initWireguard() {
 	}
 }

+func (c *Client) initNodeLatency() {
+	gateway := "antrea-gw0"


this is configurable in theory so you should not hardcode it

the gateway name should be available in the nodeConfig field, please take a look

antoninbas · 2025-03-17T20:53:56Z

pkg/agent/route/route_linux.go

@@ -1245,6 +1258,38 @@ func (c *Client) initWireguard() {
 	}
 }

+func (c *Client) initNodeLatency() {
+	gateway := "antrea-gw0"
+	if c.networkConfig.TrafficEncapMode.String() == "networkPolicyOnly" {


use the IsNetworkPolicyOnly() method on c.networkConfig.TrafficEncapMode

I think there are examples in this file

antoninbas · 2025-03-17T20:54:41Z

pkg/agent/route/route_linux.go

+func (c *Client) initNodeLatency() {
+	gateway := "antrea-gw0"
+	if c.networkConfig.TrafficEncapMode.String() == "networkPolicyOnly" {
+		gateway = "transport"


this is not correct, there is no interface names transport. But there is a TransportIface field in the networkConfig.

antoninbas · 2025-03-17T20:56:02Z

pkg/agent/route/route_linux.go

+	antreaOutputChainRules := []string{
+		iptables.NewRuleBuilder(antreaOutputChain).
+			MatchOutputInterface(gateway).
+			SetComment("Antrea: allow egress packets from NodeLatencyMonitor").


inconsistent with the input rule comment

adjusting both comments to specify ICMP and which type (ingress or egress)

antoninbas · 2025-03-17T20:56:38Z

pkg/agent/route/route_linux.go

+	antreaInputChainRules := []string{
+		iptables.NewRuleBuilder(antreaInputChain).
+			MatchInputInterface(gateway).
+			SetComment("Antrea: allow ICMP packets from NodeLatencyMonitor").
+			SetTarget(iptables.AcceptTarget).
+			Done().
+			GetRule(),
+	}
+	antreaOutputChainRules := []string{


I see no match on "icmp" packets, this rule is way too generic as it allows all traffic

antoninbas · 2025-03-17T20:57:31Z

pkg/agent/route/route_linux.go

@@ -737,10 +748,12 @@ func (c *Client) syncIPTables() error {
 	// for performance reasons.
 	addFilterRulesToChain(iptablesFilterRulesByChainV4, &c.wireguardIPTablesIPv4)
 	addFilterRulesToChain(iptablesFilterRulesByChainV4, &c.nodeNetworkPolicyIPTablesIPv4)
+	addFilterRulesToChain(iptablesFilterRulesByChainV4, &c.nodeLatencyMonitorIPTablesIPv4)


check the comment above:

Install the static rules (WireGuard for now) before the dynamic rules (e.g., NodeNetworkPolicy) for performance reasons

this applies to the NLM rules as well, which are also static

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas · 2025-03-19T00:14:42Z

pkg/agent/route/route_linux.go

+	if c.networkConfig.TrafficEncapMode.IsNetworkPolicyOnly() {
+		gateway = c.networkConfig.TransportIface
+	}
+	icmpZero := int32(0)


maybe use https://pkg.go.dev/golang.org/x/net/ipv4#ICMPType since I notice that we already import / use this package in pkg/agent/monitortool. Same for IPV6.

antoninbas · 2025-03-19T00:16:26Z

pkg/agent/route/route_linux.go

+		antreaInputChainRulesIPv6 := []string{
+			iptables.NewRuleBuilder(antreaInputChain).
+				MatchInputInterface(gateway).
+				MatchICMP(&icmpZero, &icmpZero, iptables.ProtocolIPv6).


It's not the same values for IPv6 and IPv4. See https://pkg.go.dev/golang.org/x/net/ipv6

antoninbas · 2025-03-19T00:17:17Z

pkg/agent/route/route_linux.go

+		antreaInputChainRulesIPv6 := []string{
+			iptables.NewRuleBuilder(antreaInputChain).
+				MatchInputInterface(gateway).
+				MatchICMP(&icmpZero, &icmpZero, iptables.ProtocolIPv6).


Use nil for the code

pkg/agent/route/route_linux.go

antoninbas · 2025-03-19T00:19:33Z

pkg/agent/route/route_linux_test.go

+			},
+		},
+		{
+			name:                      "encap,egress=true,multicastEnabled=true,proxyAll=true,nodeNetworkPolicy=false,nodeLatencyMonitor=true,nodeSNATRandomFully=false",


let's not add this extra test IMO, the test update above is enough

antoninbas · 2025-03-19T00:19:56Z

test/e2e/nodelatencymonitor_test.go

@@ -28,6 +28,7 @@ import (
 	"antrea.io/antrea/pkg/features"
 )

+// go test -timeout=10m -v antrea.io/antrea/test/e2e -run TestNodeLatencyMonitor -provider=kind -deploy-antrea=false


we don't add this generally, so I'd suggest removing it

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas · 2025-03-20T21:41:25Z

pkg/agent/route/route_linux.go

+		if icmpType == 128 {
+			comment = "Antrea: allow ICMP echo request egress packets from NodeLatencyMonitor"
+		} else if icmpType == 8 {


no need to distinguish between the 2, they are both for ICMP echo requests (one for IPv4 and one for IPv6)

personally i think we can use the same comment everywhere, for both request and reply, and avoid the complexity (e.g. Antrea: allow ICMP probes for NodeLatencyMonitor).

antoninbas · 2025-03-20T21:42:51Z

pkg/agent/route/route_linux.go

+		inputRequestRule := buildInputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoRequest))
+		outputRequestRule := buildOutputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoRequest))
+		inputReplyRule := buildInputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoReply))
+		outputReplyRule := buildOutputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoReply))
 		c.nodeLatencyMonitorIPTablesIPv6.Store(antreaInputChain, []string{inputRequestRule, inputReplyRule})
 		c.nodeLatencyMonitorIPTablesIPv6.Store(antreaOutputChain, []string{outputRequestRule, outputReplyRule})


I would avoid fining these extra variables, for readability, I think they are not necessary:

c.nodeLatencyMonitorIPTablesIPv6.Store(antreaInputChain, []string{ buildInputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoRequest)), buildInputRule(iptables.ProtocolIPv6, int32(ipv6.ICMPTypeEchoReply)), })

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas

minor comments, otherwise lgtm

antoninbas · 2025-03-21T20:05:29Z

cmd/antrea-agent/agent.go

@@ -154,6 +154,7 @@ func run(o *Options) error {
 	enableBridgingMode := enableAntreaIPAM && o.config.EnableBridgingMode
 	l7NetworkPolicyEnabled := features.DefaultFeatureGate.Enabled(features.L7NetworkPolicy)
 	nodeNetworkPolicyEnabled := features.DefaultFeatureGate.Enabled(features.NodeNetworkPolicy)
+	nodeLatencyMonitorEnabled := o.config.FeatureGates[string(features.NodeLatencyMonitor)]


why is this different from the other ones?

Instead of using the default featuregate value, I wanted to use the actual value of the config. I noticed that even if I set NodeLatencyMonitor to true in the deployment, it wouldn't set the boolean correctly. Is this expected behavior until I deploy a NodeLatencyMonitor?

Instead of using the default featuregate value, I wanted to use the actual value of the config

This is what all the surrounding lines are doing, otherwise the code would be badly broken.

I set NodeLatencyMonitor to true in the deployment, it wouldn't set the boolean correctly. Is this expected behavior until I deploy a NodeLatencyMonitor?

No, that's not the expected behavior but I would be surprised if this was the actual behavior as well.

Ok, will update, it seemed to just use a constant's value, so I chose to use the config value isntead.

antoninbas · 2025-03-21T20:06:15Z

pkg/agent/route/route_linux.go

@@ -735,10 +748,12 @@ func (c *Client) syncIPTables() error {
 	iptablesFilterRulesByChainV4 := make(map[string][]string)
 	// Install the static rules (WireGuard for now) before the dynamic rules (e.g., NodeNetworkPolicy)


update the comment:
s/WireGuard for now/WireGuard + NodeLatencyMonitor

antoninbas · 2025-03-21T20:08:43Z

pkg/agent/route/route_linux.go

@@ -1245,6 +1260,53 @@ func (c *Client) initWireguard() {
 	}
 }

+func (c *Client) initNodeLatency() {
+	gateway := c.nodeConfig.GatewayConfig.Name


rename the gateway variable to iface as the "gateway" name is not appropriate for policyOnly mode

maybe also add a comment for future readability:

the interface on which ICMP probes are sent / received is the Antrea gateway interface, except in networkPolicyOnly mode, for which it is the Node's transport interface.

Signed-off-by: Dhruv Jain <[email protected]>

tnqn · 2025-03-31T15:18:32Z

cmd/antrea-agent/agent.go

@@ -154,6 +154,7 @@ func run(o *Options) error {
 	enableBridgingMode := enableAntreaIPAM && o.config.EnableBridgingMode
 	l7NetworkPolicyEnabled := features.DefaultFeatureGate.Enabled(features.L7NetworkPolicy)
 	nodeNetworkPolicyEnabled := features.DefaultFeatureGate.Enabled(features.NodeNetworkPolicy)
+	nodeLatencyMonitorEnabled := features.DefaultFeatureGate.Enabled(features.NodeLatencyMonitor) //o.config.FeatureGates[string(features.NodeLatencyMonitor)]


is the comment needed?

tnqn · 2025-03-31T15:25:46Z

pkg/agent/route/route_linux.go

@@ -278,6 +287,10 @@ func (c *Client) Initialize(nodeConfig *config.NodeConfig, done func()) error {
 	if c.networkConfig.TrafficEncryptionMode == config.TrafficEncryptionModeWireGuard {
 		c.initWireguard()
 	}
+	if c.nodeLatencyMonitorEnabled {
+		c.initNodeLatency()


does initNodeLatencyMonitor sound more clear what it's for?

I figured since it wasn't initializing the monitor itself, and just the rules, could initNodeLatencyRules() be a better descriptor?

initNodeLatencyRules sounds good to me

tnqn · 2025-03-31T15:43:02Z

pkg/agent/route/route_linux.go

@@ -278,6 +287,10 @@ func (c *Client) Initialize(nodeConfig *config.NodeConfig, done func()) error {
 	if c.networkConfig.TrafficEncryptionMode == config.TrafficEncryptionModeWireGuard {
 		c.initWireguard()
 	}
+	if c.nodeLatencyMonitorEnabled {


Should it only add the rule when latencyConfig.Enable in pkg/agent/monitortool/monitor.go is true?
If we only check the feature gate, the 4 rules would be installed unconditionally on all clusters even the API is not used, which might add unnecessary overhead to Node-to-Node traffic. This might already be the case of nodeNetworkPolicyEnabled, but with more feature-specific rule added, I'm a bit concerned about the total overhead.

Or we could defer this and add one item to these features' beta graduation criteria: the feature-specific rules should only be installed when the feature is in-use.

what do you think? @antoninbas

I am fine with taking care of this as a follow-up item, and prior to graduating the feature. Maybe @Dhruv-J could open a separate issue to keep track of this.

tnqn · 2025-03-31T16:00:34Z

Please change "Fixes issue #.." to "Fixes #..", otherwise merging the PR wouldn't close the issue.

Signed-off-by: Dhruv Jain <[email protected]>

tnqn

LGTM

tnqn · 2025-04-07T02:22:03Z

/test-all

Dhruv-J · 2025-04-08T20:34:52Z

/test-all

Dhruv-J force-pushed the nlm-icmp branch from 0683805 to 8441520 Compare February 20, 2025 19:10

Dhruv-J force-pushed the nlm-icmp branch 4 times, most recently from d690aed to 52b466b Compare March 13, 2025 22:31

Dhruv-J force-pushed the nlm-icmp branch from 52b466b to 413015f Compare March 13, 2025 22:33

Dhruv-J marked this pull request as ready for review March 14, 2025 07:41

Dhruv-J requested review from wenyingd and antoninbas March 14, 2025 07:43

removed networkPolicyOnlyMode as it was giving an error for the linte…

dbcc0fc

…r, fixed route_windows.go Signed-off-by: Dhruv Jain <[email protected]>

antoninbas reviewed Mar 17, 2025

View reviewed changes

addressed feedback

8e29691

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas reviewed Mar 19, 2025

View reviewed changes

Dhruv Jain added 2 commits March 20, 2025 03:08

updated some feedback, still getting duplicate ipTable rules

f5e5b75

Signed-off-by: Dhruv Jain <[email protected]>

fixed iptables duplicate rule bug

b59cf74

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas reviewed Mar 20, 2025

View reviewed changes

feedback addressed

2445512

Signed-off-by: Dhruv Jain <[email protected]>

antoninbas reviewed Mar 21, 2025

View reviewed changes

antoninbas requested a review from tnqn March 21, 2025 20:09

feedback addressed

9087e03

Signed-off-by: Dhruv Jain <[email protected]>

tnqn reviewed Mar 31, 2025

View reviewed changes

feedback addressed

f6977d8

Signed-off-by: Dhruv Jain <[email protected]>

tnqn approved these changes Apr 7, 2025

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Apr 7, 2025

antoninbas merged commit 05d0cf3 into antrea-io:main Apr 8, 2025
58 of 63 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICMP Rule with NodeLatencyMonitor #7011

ICMP Rule with NodeLatencyMonitor #7011

Dhruv-J commented Feb 20, 2025 •

edited

Loading

antoninbas commented Mar 4, 2025

Dhruv-J commented Mar 14, 2025

antoninbas Mar 17, 2025

antoninbas Mar 17, 2025

antoninbas Mar 17, 2025

antoninbas Mar 17, 2025

Dhruv-J Mar 17, 2025

antoninbas Mar 17, 2025

antoninbas Mar 17, 2025

antoninbas Mar 19, 2025

antoninbas Mar 19, 2025

antoninbas Mar 19, 2025

antoninbas Mar 19, 2025

antoninbas Mar 19, 2025

antoninbas Mar 20, 2025

antoninbas Mar 20, 2025

antoninbas left a comment

antoninbas Mar 21, 2025

Dhruv-J Mar 21, 2025

antoninbas Mar 21, 2025

Dhruv-J Mar 21, 2025

antoninbas Mar 21, 2025

antoninbas Mar 21, 2025

tnqn Mar 31, 2025

tnqn Mar 31, 2025

Dhruv-J Mar 31, 2025 •

edited

Loading

tnqn Apr 1, 2025

tnqn Mar 31, 2025

antoninbas Mar 31, 2025

tnqn commented Mar 31, 2025

tnqn left a comment

tnqn commented Apr 7, 2025

Dhruv-J commented Apr 8, 2025

		@@ -735,10 +748,12 @@ func (c *Client) syncIPTables() error {
		iptablesFilterRulesByChainV4 := make(map[string][]string)
		// Install the static rules (WireGuard for now) before the dynamic rules (e.g., NodeNetworkPolicy)

ICMP Rule with NodeLatencyMonitor #7011

ICMP Rule with NodeLatencyMonitor #7011

Conversation

Dhruv-J commented Feb 20, 2025 • edited Loading

antoninbas commented Mar 4, 2025

Dhruv-J commented Mar 14, 2025

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

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

antoninbas left a 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

Choose a reason for hiding this comment

Dhruv-J Mar 31, 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

tnqn commented Mar 31, 2025

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 7, 2025

Dhruv-J commented Apr 8, 2025

Dhruv-J commented Feb 20, 2025 •

edited

Loading

Dhruv-J Mar 31, 2025 •

edited

Loading