Do not delete IPv6 link-local route in reconciler #5483

wenyingd · 2023-09-13T09:57:10Z

In the existing code, the IPv6 link-local route on antrea-gw0 is deleted in
route reconcile, which results in the IPv6 Neighbor Solicitation sent from Pod's
link-local address is dropped on the Node by kenel reverse path filtering, and
Pod would mark the antrea-gw0 as a "FAILED" neighbor. Then the Pod's accross
Node traffic or the Pod-to-external traffic does not work as expected.

This change includes,

Do not delete IPv6 link-local routes in the reconcile function,
Restore IPv6 link-local route in syncRoute function.

Fix: #5482

tnqn · 2023-09-13T11:13:52Z

pkg/agent/route/route_linux.go

+		// A route destined to an IPv6 link-local CIDR is always system auto-generated along with a link-local
+		// address, which is not configured by antrea and is supposed to be ignored in the "deletion" list.
+		// Such routes are helpful in some case, e.g., IPv6 NDP.
+		if route.Dst.IP != nil && route.Dst.IP.IsLinkLocalUnicast() && route.Dst.IP.To4() == nil {


IPNet won't have a nil IP, the first check can be removed. L902 never check it as well.

Sure, updated.

tnqn · 2023-09-13T11:23:42Z

pkg/wfp/interfaces.go

@@ -0,0 +1,62 @@
+package wfp


remove the file

tnqn · 2023-09-13T11:27:25Z

To prevent regression in the future, can we improve testReconcileGatewayRoutesOnStartup to cover this validation of this route?

wenyingd · 2023-09-14T06:25:57Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

tnqn · 2023-09-14T08:06:41Z

pkg/agent/route/route_linux.go

+		// A route destined to an IPv6 link-local CIDR is always system auto-generated along with a link-local
+		// address, which is not configured by antrea and is supposed to be ignored in the "deletion" list.


I think we should also add the route to

antrea/pkg/agent/route/route_linux.go

Lines 280 to 285 in 99a5f25

gwAutoconfRoutes = append(gwAutoconfRoutes, &netlink.Route{

LinkIndex: c.nodeConfig.GatewayConfig.LinkIndex,

Dst: c.nodeConfig.PodIPv6CIDR,

Src: c.nodeConfig.GatewayConfig.IPv6,

Scope: netlink.SCOPE_LINK,

})

given we already have logic to recover required routes and to make bumping up release can fix broken clusters. Otherwise the cluster would not recover until Nodes are rebooted.

wenyingd · 2023-09-14T11:19:52Z

/test-ipv6-all
/test-ipv6-only-all
/test-all

wenyingd · 2023-09-15T02:34:26Z

/test-conformance
/test-ipv6-only-e2e
/test-ipv6-only-conformance

wenyingd · 2023-09-15T05:59:54Z

/test-ipv6-only-e2e
/test-ipv6-only-conformance

tnqn

LGTM, one question

pkg/agent/route/route_linux.go

tnqn · 2023-09-15T08:43:51Z

Please update the commit message to reflect the actual change: it will restore the link-local route if it doesn't exist.

wenyingd · 2023-09-18T06:26:04Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

pkg/agent/route/route_linux.go

wenyingd · 2023-09-18T09:55:18Z

/test-all

wenyingd · 2023-09-18T09:56:15Z

The IPv6 e2e failure is not related with this change, using another issue #5492 to track it.

luolanzone

LGTM overall, one nit.

pkg/agent/route/route_linux.go

tnqn · 2023-09-19T09:15:25Z

pkg/agent/route/route_linux.go

+	// The system auto-generated IPv6 link-local route always uses "fe80::/64"
+	// as the destination regardless of the interface's global address's mask.
+	llRouteDstv6 := "fe80::/64"
+	_, llrCIDR, _ := net.ParseCIDR(llRouteDstv6)
+	routeKey := func(r *netlink.Route) string {
+		if r.Dst.String() == llRouteDstv6 {
+			// Use "$Dst_$linkIndex" as the key of IPv6 link-local route because
+			// such route entry is auto-configured on each net interface.
+			return fmt.Sprintf("%s_%d", r.Dst.String(), r.LinkIndex)
+		}
+		return r.Dst.String()
+	}


We can make it more generic, instead of making a map with different kinds of keys and later compare the values, the routes could be saved in a set, item of which contains only the fields we care about:

type routeKey struct { LinkIndex int Dst string Gw string } routeKeys := sets.New[routeKey]() for _, route := range routeList { routeKeys.Insert(routeKey{ LinkIndex: route.LinkIndex, Dst: route.Dst.String(), Gw: route.Gw.String(), }) } restoreRoute := func(route *netlink.Route) bool { if routeKeys.Has(routeKey{ LinkIndex: route.LinkIndex, Dst: route.Dst.String(), Gw: route.Gw.String(), }) { return true } if err := c.netlink.RouteReplace(route); err != nil { klog.ErrorS(err, "Failed to sync route", "Route", route) return false } return true }

tnqn · 2023-09-19T13:06:12Z

pkg/agent/route/route_linux.go

+	// The system auto-generated IPv6 link-local route always uses "fe80::/64"
+	// as the destination regardless of the interface's global address's mask.
+	llRouteDstv6 := "fe80::/64"
+	_, llrCIDR, _ := net.ParseCIDR(llRouteDstv6)


Static value could be declared as global variable to avoid repeated parse, following

antrea/pkg/agent/route/route_linux.go

Line 88 in 99b539d

globalVMAC, _ = net.ParseMAC("aa:bb:cc:dd:ee:ff")

In the existing code, the IPv6 link-local route on antrea-gw0 is deleted in route reconcile, which results in the IPv6 Neighbor Solicitation sent from Pod's link-local address is dropped on the Node by kenel reverse path filtering, and Pod would mark the antrea-gw0 as a "FAILED" neighbor. Then the Pod's accross Node traffic or the Pod-to-external traffic does not work as expected. This change includes, 1. Do not delete IPv6 link-local routes in the reconcile function, 2. Restore IPv6 link-local route in syncRoute function. Signed-off-by: wenyingd <[email protected]>

tnqn

LGTM

tnqn · 2023-09-20T04:38:00Z

/test-all
/test-ipv6-all

wenyingd · 2023-09-20T07:17:40Z

/test-ipv6-e2e

tnqn reviewed Sep 13, 2023

View reviewed changes

wenyingd force-pushed the issue_5482 branch from b46b3f2 to 1375b18 Compare September 13, 2023 11:17

tnqn reviewed Sep 13, 2023

View reviewed changes

pkg/wfp/interfaces.go Outdated

@@ -0,0 +1,62 @@

package wfp

Copy link

Member

tnqn Sep 13, 2023

Choose a reason for hiding this comment

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

remove the file

tnqn added area/transit/ipv6 Issues or PRs related to IPv6. action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Sep 13, 2023

wenyingd force-pushed the issue_5482 branch from 1375b18 to d6d97e6 Compare September 14, 2023 04:03

tnqn reviewed Sep 14, 2023

View reviewed changes

wenyingd force-pushed the issue_5482 branch from d6d97e6 to df41b48 Compare September 14, 2023 09:41

tnqn previously approved these changes Sep 15, 2023

View reviewed changes

pkg/agent/route/route_linux.go Show resolved Hide resolved

wenyingd force-pushed the issue_5482 branch from df41b48 to 521ea55 Compare September 15, 2023 09:12

wenyingd dismissed tnqn’s stale review via 8c037f6 September 15, 2023 15:50

wenyingd force-pushed the issue_5482 branch 2 times, most recently from 8c037f6 to 006fb1e Compare September 18, 2023 02:29

luolanzone reviewed Sep 18, 2023

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

wenyingd force-pushed the issue_5482 branch from 006fb1e to a081673 Compare September 18, 2023 09:43

wenyingd requested review from luolanzone and tnqn September 18, 2023 09:47

luolanzone reviewed Sep 19, 2023

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

wenyingd force-pushed the issue_5482 branch from a081673 to b5f844e Compare September 19, 2023 03:44

luolanzone previously approved these changes Sep 19, 2023

View reviewed changes

tnqn reviewed Sep 19, 2023

View reviewed changes

wenyingd dismissed luolanzone’s stale review via bd0ba05 September 19, 2023 09:34

wenyingd force-pushed the issue_5482 branch from b5f844e to bd0ba05 Compare September 19, 2023 09:34

tnqn reviewed Sep 19, 2023

View reviewed changes

wenyingd force-pushed the issue_5482 branch from bd0ba05 to f23830a Compare September 20, 2023 01:59

tnqn approved these changes Sep 20, 2023

View reviewed changes

tnqn merged commit 99d9514 into antrea-io:main Sep 20, 2023
42 of 44 checks passed

wenyingd deleted the issue_5482 branch April 3, 2024 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not delete IPv6 link-local route in reconciler #5483

Do not delete IPv6 link-local route in reconciler #5483

wenyingd commented Sep 13, 2023 •

edited

Loading

tnqn Sep 13, 2023

wenyingd Sep 13, 2023

tnqn Sep 13, 2023

tnqn commented Sep 13, 2023

wenyingd commented Sep 14, 2023

tnqn Sep 14, 2023

wenyingd Sep 14, 2023

wenyingd commented Sep 14, 2023

wenyingd commented Sep 15, 2023

wenyingd commented Sep 15, 2023

tnqn left a comment

tnqn commented Sep 15, 2023

wenyingd commented Sep 18, 2023

wenyingd commented Sep 18, 2023

wenyingd commented Sep 18, 2023

luolanzone left a comment •

edited

Loading

tnqn Sep 19, 2023

tnqn Sep 19, 2023

wenyingd Sep 20, 2023

tnqn left a comment

tnqn commented Sep 20, 2023

wenyingd commented Sep 20, 2023

		// A route destined to an IPv6 link-local CIDR is always system auto-generated along with a link-local
		// address, which is not configured by antrea and is supposed to be ignored in the "deletion" list.

	gwAutoconfRoutes = append(gwAutoconfRoutes, &netlink.Route{
	LinkIndex: c.nodeConfig.GatewayConfig.LinkIndex,
	Dst: c.nodeConfig.PodIPv6CIDR,
	Src: c.nodeConfig.GatewayConfig.IPv6,
	Scope: netlink.SCOPE_LINK,
	})

Do not delete IPv6 link-local route in reconciler #5483

Do not delete IPv6 link-local route in reconciler #5483

Conversation

wenyingd commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Sep 13, 2023

wenyingd commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Sep 14, 2023

wenyingd commented Sep 15, 2023

wenyingd commented Sep 15, 2023

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Sep 15, 2023

wenyingd commented Sep 18, 2023

wenyingd commented Sep 18, 2023

wenyingd commented Sep 18, 2023

luolanzone left a comment • 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 left a comment

Choose a reason for hiding this comment

tnqn commented Sep 20, 2023

wenyingd commented Sep 20, 2023

wenyingd commented Sep 13, 2023 •

edited

Loading

luolanzone left a comment •

edited

Loading