From 2be6ee2c6155d912987a5ce905043b9494d26696 Mon Sep 17 00:00:00 2001 From: Hongliang Liu Date: Tue, 13 Dec 2022 14:18:55 +0800 Subject: [PATCH] Fix that Service routes may get lost when starting on Windows Fix #4467 Signed-off-by: Hongliang Liu --- .../noderoute/node_route_controller.go | 25 ++----------------- pkg/agent/route/interfaces.go | 2 +- pkg/agent/route/route_linux.go | 2 +- pkg/agent/route/route_linux_test.go | 2 +- pkg/agent/route/route_windows.go | 16 +++++++++--- pkg/agent/route/route_windows_test.go | 6 ++--- pkg/agent/route/testing/mock_route.go | 10 ++++---- test/integration/agent/route_test.go | 6 +---- 8 files changed, 26 insertions(+), 43 deletions(-) diff --git a/pkg/agent/controller/noderoute/node_route_controller.go b/pkg/agent/controller/noderoute/node_route_controller.go index 7c40d933613..fd5555bcfc8 100644 --- a/pkg/agent/controller/noderoute/node_route_controller.go +++ b/pkg/agent/controller/noderoute/node_route_controller.go @@ -42,7 +42,6 @@ import ( "antrea.io/antrea/pkg/ovs/ovsconfig" utilip "antrea.io/antrea/pkg/util/ip" "antrea.io/antrea/pkg/util/k8s" - "antrea.io/antrea/pkg/util/runtime" ) const ( @@ -72,7 +71,6 @@ type Controller struct { nodeInformer coreinformers.NodeInformer nodeLister corelisters.NodeLister nodeListerSynced cache.InformerSynced - svcLister corelisters.ServiceLister queue workqueue.RateLimitingInterface // installedNodes records routes and flows installation states of Nodes. // The key is the host name of the Node, the value is the nodeRouteInfo of the Node. @@ -102,7 +100,6 @@ func NewNodeRouteController( ipsecCertificateManager ipseccertificate.Manager, ) *Controller { nodeInformer := informerFactory.Core().V1().Nodes() - svcLister := informerFactory.Core().V1().Services() controller := &Controller{ kubeClient: kubeClient, ovsBridgeClient: ovsBridgeClient, @@ -114,7 +111,6 @@ func NewNodeRouteController( nodeInformer: nodeInformer, nodeLister: nodeInformer.Lister(), nodeListerSynced: nodeInformer.Informer().HasSynced, - svcLister: svcLister.Lister(), queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "noderoute"), installedNodes: cache.NewIndexer(nodeRouteInfoKeyFunc, cache.Indexers{nodeRouteInfoPodCIDRIndexName: nodeRouteInfoPodCIDRIndexFunc}), wireGuardClient: wireguardClient, @@ -203,27 +199,10 @@ func (c *Controller) removeStaleGatewayRoutes() error { desiredPodCIDRs = append(desiredPodCIDRs, podCIDRs...) } - // TODO: This is not the best place to keep the ClusterIP Service routes. - desiredClusterIPSvcIPs := map[string]bool{} - if c.proxyAll && runtime.IsWindowsPlatform() { - // The route for virtual IP -> antrea-gw0 should be always kept. - desiredClusterIPSvcIPs[config.VirtualServiceIPv4.String()] = true - - svcs, err := c.svcLister.List(labels.Everything()) - for _, svc := range svcs { - for _, ip := range svc.Spec.ClusterIPs { - desiredClusterIPSvcIPs[ip] = true - } - } - if err != nil { - return fmt.Errorf("error when listing ClusterIP Service IPs: %v", err) - } - } - // routeClient will remove orphaned routes whose destinations are not in desiredPodCIDRs. // If proxyAll enabled, it will also remove routes that are for Windows ClusterIP Services - // which no longer exist. - if err := c.routeClient.Reconcile(desiredPodCIDRs, desiredClusterIPSvcIPs); err != nil { + // which no longer exists. + if err := c.routeClient.Reconcile(desiredPodCIDRs); err != nil { return err } return nil diff --git a/pkg/agent/route/interfaces.go b/pkg/agent/route/interfaces.go index f0e1fc04a57..630c0dc57c8 100644 --- a/pkg/agent/route/interfaces.go +++ b/pkg/agent/route/interfaces.go @@ -29,7 +29,7 @@ type Interface interface { // Reconcile should remove orphaned routes and related configuration based on the desired podCIDRs and Service IPs. // If IPv6 is enabled in the cluster, Reconcile should also remove the orphaned IPv6 neighbors. - Reconcile(podCIDRs []string, svcIPs map[string]bool) error + Reconcile(podCIDRs []string) error // AddRoutes should add routes to the provided podCIDR. // It should override the routes if they already exist, without error. diff --git a/pkg/agent/route/route_linux.go b/pkg/agent/route/route_linux.go index 3ad4f5c7ba0..55f7da2b716 100644 --- a/pkg/agent/route/route_linux.go +++ b/pkg/agent/route/route_linux.go @@ -843,7 +843,7 @@ func (c *Client) initServiceIPRoutes() error { // Reconcile removes orphaned podCIDRs from ipset and removes routes to orphaned podCIDRs // based on the desired podCIDRs. svcIPs are used for Windows only. -func (c *Client) Reconcile(podCIDRs []string, svcIPs map[string]bool) error { +func (c *Client) Reconcile(podCIDRs []string) error { desiredPodCIDRs := sets.NewString(podCIDRs...) // Get the peer IPv6 gateways from pod CIDRs desiredIPv6GWs := getIPv6Gateways(podCIDRs) diff --git a/pkg/agent/route/route_linux_test.go b/pkg/agent/route/route_linux_test.go index b8c7503bc90..c2cafd105cf 100644 --- a/pkg/agent/route/route_linux_test.go +++ b/pkg/agent/route/route_linux_test.go @@ -622,7 +622,7 @@ func TestReconcile(t *testing.T) { {IP: net.ParseIP("2001:ab03:cd04:55ee:100b::1")}, // non-existing podCIDR, should be deleted. }, nil) mockNetlink.EXPECT().NeighDel(&netlink.Neigh{IP: net.ParseIP("2001:ab03:cd04:55ee:100b::1")}) - assert.NoError(t, c.Reconcile(podCIDRs, nil)) + assert.NoError(t, c.Reconcile(podCIDRs)) } func TestAddRoutes(t *testing.T) { diff --git a/pkg/agent/route/route_windows.go b/pkg/agent/route/route_windows.go index 3e58e1556bd..6f5cbd28e6a 100644 --- a/pkg/agent/route/route_windows.go +++ b/pkg/agent/route/route_windows.go @@ -122,7 +122,7 @@ func (c *Client) initServiceIPRoutes() error { // Reconcile removes the orphaned routes and related configuration based on the desired podCIDRs and Service IPs. Only // the route entries on the host gateway interface are stored in the cache. -func (c *Client) Reconcile(podCIDRs []string, svcIPs map[string]bool) error { +func (c *Client) Reconcile(podCIDRs []string) error { desiredPodCIDRs := sets.NewString(podCIDRs...) routes, err := c.listRoutes() if err != nil { @@ -133,8 +133,8 @@ func (c *Client) Reconcile(podCIDRs []string, svcIPs map[string]bool) error { c.hostRoutes.Store(dst, rt) continue } - if _, ok := svcIPs[dst]; ok { - c.hostRoutes.Store(dst, rt) + // Don't delete the routes which are added by AntreaProxy. + if c.isServiceRoute(rt) { continue } err := util.RemoveNetRoute(rt) @@ -260,8 +260,8 @@ func (c *Client) addVirtualServiceIPRoute(isIPv6 bool) error { // TODO: Follow the code style in Linux that maintains one Service CIDR. func (c *Client) addServiceRoute(svcIP net.IP) error { - obj, found := c.hostRoutes.Load(svcIP.String()) svcIPNet := util.NewIPNet(svcIP) + obj, found := c.hostRoutes.Load(svcIPNet.String()) // Route: Service IP -> VirtualServiceIPv4 (169.254.0.253) route := &util.Route{ @@ -332,6 +332,14 @@ func (c *Client) UnMigrateRoutesFromGw(route *net.IPNet, linkName string) error func (c *Client) Run(stopCh <-chan struct{}) { } +func (c *Client) isServiceRoute(route *util.Route) bool { + // If the destination IP or gateway IP is virtual Service IP , then it is a route which is added by AntreaProxy. + if route.DestinationSubnet.IP.Equal(config.VirtualServiceIPv4) || route.GatewayAddress.Equal(config.VirtualServiceIPv4) { + return true + } + return false +} + func (c *Client) listRoutes() (map[string]*util.Route, error) { routes, err := util.GetNetRoutesAll() if err != nil { diff --git a/pkg/agent/route/route_windows_test.go b/pkg/agent/route/route_windows_test.go index bfec5a310f7..4f2c437c13a 100644 --- a/pkg/agent/route/route_windows_test.go +++ b/pkg/agent/route/route_windows_test.go @@ -96,7 +96,7 @@ func TestRouteOperation(t *testing.T) { require.Nil(t, err) assert.Equal(t, 1, len(route4)) - err = client.Reconcile([]string{dest2}, map[string]bool{svcIPNet1.String(): true}) + err = client.Reconcile([]string{dest2}) require.Nil(t, err) routes5, err := util.GetNetRoutes(gwLink, destCIDR1) @@ -105,7 +105,7 @@ func TestRouteOperation(t *testing.T) { routes6, err := util.GetNetRoutes(gwLink, svcIPNet2) require.Nil(t, err) - assert.Equal(t, 0, len(routes6)) + assert.Equal(t, 1, len(routes6)) err = client.DeleteRoutes(destCIDR2) require.Nil(t, err) @@ -117,5 +117,5 @@ func TestRouteOperation(t *testing.T) { require.Nil(t, err) routes8, err := util.GetNetRoutes(gwLink, svcIPNet1) require.Nil(t, err) - assert.Equal(t, 0, len(routes8)) + assert.Equal(t, 1, len(routes8)) } diff --git a/pkg/agent/route/testing/mock_route.go b/pkg/agent/route/testing/mock_route.go index 67ee9270b63..2f23c07ca38 100644 --- a/pkg/agent/route/testing/mock_route.go +++ b/pkg/agent/route/testing/mock_route.go @@ -1,4 +1,4 @@ -// Copyright 2021 Antrea Authors +// Copyright 2022 Antrea Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -247,17 +247,17 @@ func (mr *MockInterfaceMockRecorder) MigrateRoutesToGw(arg0 interface{}) *gomock } // Reconcile mocks base method -func (m *MockInterface) Reconcile(arg0 []string, arg1 map[string]bool) error { +func (m *MockInterface) Reconcile(arg0 []string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Reconcile", arg0, arg1) + ret := m.ctrl.Call(m, "Reconcile", arg0) ret0, _ := ret[0].(error) return ret0 } // Reconcile indicates an expected call of Reconcile -func (mr *MockInterfaceMockRecorder) Reconcile(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockInterfaceMockRecorder) Reconcile(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockInterface)(nil).Reconcile), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reconcile", reflect.TypeOf((*MockInterface)(nil).Reconcile), arg0) } // Run mocks base method diff --git a/test/integration/agent/route_test.go b/test/integration/agent/route_test.go index 66d75911334..1c901a30d88 100644 --- a/test/integration/agent/route_test.go +++ b/test/integration/agent/route_test.go @@ -517,7 +517,6 @@ func TestReconcile(t *testing.T) { addedRoutes []peer desiredPeerCIDRs []string desiredNodeIPs []string - desiredServices map[string]bool // expectations expRoutes map[string]netlink.Link }{ @@ -530,7 +529,6 @@ func TestReconcile(t *testing.T) { }, desiredPeerCIDRs: []string{"10.10.20.0/24"}, desiredNodeIPs: []string{remotePeerIP.String()}, - desiredServices: map[string]bool{"200.200.10.10": true}, expRoutes: map[string]netlink.Link{"10.10.20.0/24": gwLink, "10.10.30.0/24": nil}, }, { @@ -542,7 +540,6 @@ func TestReconcile(t *testing.T) { }, desiredPeerCIDRs: []string{"10.10.20.0/24"}, desiredNodeIPs: []string{localPeerIP.String()}, - desiredServices: map[string]bool{"200.200.10.10": true}, expRoutes: map[string]netlink.Link{"10.10.20.0/24": nodeLink, "10.10.30.0/24": nil}, }, { @@ -556,7 +553,6 @@ func TestReconcile(t *testing.T) { }, desiredPeerCIDRs: []string{"10.10.20.0/24", "10.10.40.0/24"}, desiredNodeIPs: []string{localPeerIP.String(), remotePeerIP.String()}, - desiredServices: map[string]bool{"200.200.10.10": true}, expRoutes: map[string]netlink.Link{"10.10.20.0/24": nodeLink, "10.10.30.0/24": nil, "10.10.40.0/24": gwLink, "10.10.50.0/24": nil}, }, } @@ -574,7 +570,7 @@ func TestReconcile(t *testing.T) { assert.NoError(t, routeClient.AddRoutes(peerNet, tc.nodeName, route.peerIP, peerGwIP), "adding routes failed") } - assert.NoError(t, routeClient.Reconcile(tc.desiredPeerCIDRs, tc.desiredServices), "reconcile failed") + assert.NoError(t, routeClient.Reconcile(tc.desiredPeerCIDRs), "reconcile failed") for dst, uplink := range tc.expRoutes { expNum := 0