Clean up stale IP addresses on Antrea host gateway

When a Node leaves a K8s cluster and joins it again later, it is likely
that it will receive a different Pod CIDR. If we simply add the new
gateway IP (first IP in the CIDR) to the host gateway interface without
removing the previous one, we have observed that it can lead to
connectivity issues. This commit ensures that all stale IPs are removed
when configuring the gateway interface on an Agent restart. Link-local
addresses are left untouched. An integration test is added for both
Linux & Windows.

Fixes #1685

pkg/agent/agent.go

@@ -528,12 +528,11 @@ func (i *Initializer) configureGatewayInterface(gatewayIface *interfacestore.Int
 	}
 
 	i.nodeConfig.GatewayConfig.LinkIndex = gwLinkIdx
-	// Allocate the gateway IP address from the Pod CIDRs if it exists. The gateway IP should be the first address
-	// in the Subnet and configure on the host gateway.
-	for _, podCIDR := range []*net.IPNet{i.nodeConfig.PodIPv4CIDR, i.nodeConfig.PodIPv6CIDR} {
-		if err := i.allocateGatewayAddress(podCIDR, gatewayIface); err != nil {
-			return err
-		}
+	// Allocate the gateway IP address for each Pod CIDR allocated to the Node. For each CIDR,
+	// the first address in the subnet is assigned to the host gateway interface.
+	podCIDRs := []*net.IPNet{i.nodeConfig.PodIPv4CIDR, i.nodeConfig.PodIPv6CIDR}
+	if err := i.allocateGatewayAddresses(podCIDRs, gatewayIface); err != nil {
+		return err
 	}
 
 	return nil
@@ -824,27 +823,38 @@ func (i *Initializer) getNodeMTU(localIntf *net.Interface) (int, error) {
 	return mtu, nil
 }
 
-func (i *Initializer) allocateGatewayAddress(localSubnet *net.IPNet, gatewayIface *interfacestore.InterfaceConfig) error {
-	if localSubnet == nil {
+func (i *Initializer) allocateGatewayAddresses(localSubnets []*net.IPNet, gatewayIface *interfacestore.InterfaceConfig) error {
+	var gwIPs []*net.IPNet
+	for _, localSubnet := range localSubnets {
+		if localSubnet == nil {
+			continue
+		}
+		subnetID := localSubnet.IP.Mask(localSubnet.Mask)
+		gwIP := &net.IPNet{IP: ip.NextIP(subnetID), Mask: localSubnet.Mask}
+		gwIPs = append(gwIPs, gwIP)
+	}
+	if len(gwIPs) == 0 {
 		return nil
 	}
-	subnetID := localSubnet.IP.Mask(localSubnet.Mask)
-	gwIP := &net.IPNet{IP: ip.NextIP(subnetID), Mask: localSubnet.Mask}
 
-	// Check IP address configuration on existing interface first, return if the interface has the desired address.
+	// Check IP address configuration on existing interface first, return if the interface has the desired addresses.
 	// We perform this check unconditionally, even if the OVS port does not exist when this function is called
 	// (i.e. portExists is false). Indeed, it may be possible for the interface to exist even if the OVS bridge does
 	// not exist.
-	// Configure the IP address on the interface if it does not exist.
-	if err := util.ConfigureLinkAddress(i.nodeConfig.GatewayConfig.LinkIndex, gwIP); err != nil {
+	// Configure any missing IP address on the interface. Remove any extra IP address that may exist.
+	if err := util.ConfigureLinkAddresses(i.nodeConfig.GatewayConfig.LinkIndex, gwIPs); err != nil {
 		return err
 	}
-	if gwIP.IP.To4() != nil {
-		i.nodeConfig.GatewayConfig.IPv4 = gwIP.IP
-	} else {
-		i.nodeConfig.GatewayConfig.IPv6 = gwIP.IP
+
+	for _, gwIP := range gwIPs {
+		if gwIP.IP.To4() != nil {
+			i.nodeConfig.GatewayConfig.IPv4 = gwIP.IP
+		} else {
+			i.nodeConfig.GatewayConfig.IPv6 = gwIP.IP
+		}
+
+		gatewayIface.IPs = append(gatewayIface.IPs, gwIP.IP)
 	}
 
-	gatewayIface.IPs = append(gatewayIface.IPs, gwIP.IP)
 	return nil
 }

pkg/agent/util/net_linux.go

@@ -130,36 +130,68 @@ func SetLinkUp(name string) (net.HardwareAddr, int, error) {
 	return mac, index, nil
 }
 
-func ConfigureLinkAddress(idx int, gwIPNet *net.IPNet) error {
+func ConfigureLinkAddresses(idx int, ipNets []*net.IPNet) error {
 	// No need to check the error here, since the link is found in previous steps.
 	link, _ := netlink.LinkByIndex(idx)
-	gwAddr := &netlink.Addr{IPNet: gwIPNet, Label: ""}
-
-	var addrFamily int
-	if gwIPNet.IP.To4() != nil {
-		addrFamily = netlink.FAMILY_V4
-	} else {
-		addrFamily = netlink.FAMILY_V6
-	}
-
-	if addrs, err := netlink.AddrList(link, addrFamily); err != nil {
-		klog.Errorf("Failed to query address list for interface %s: %v", link.Attrs().Name, err)
-		return err
-	} else if addrs != nil {
-		for _, addr := range addrs {
-			klog.V(4).Infof("Found address %s for interface %s", addr.IP.String(), link.Attrs().Name)
-			if addr.IP.Equal(gwAddr.IPNet.IP) {
-				klog.V(2).Infof("Address %s already assigned to interface %s", addr.IP.String(), link.Attrs().Name)
-				return nil
+	ifaceName := link.Attrs().Name
+	var newAddrs []netlink.Addr
+	for _, ipNet := range ipNets {
+		newAddrs = append(newAddrs, netlink.Addr{IPNet: ipNet, Label: ""})
+	}
+
+	allAddrs, err := netlink.AddrList(link, netlink.FAMILY_ALL)
+	if err != nil {
+		return fmt.Errorf("failed to query address list for interface %s: %v", ifaceName, err)
+	}
+	// Remove link-local address from list
+	addrs := make([]netlink.Addr, 0, len(allAddrs))
+	for _, addr := range allAddrs {
+		if !addr.IP.IsLinkLocalUnicast() {
+			addrs = append(addrs, addr)
+		}
+	}
+
+	addrSliceDifference := func(s1, s2 []netlink.Addr) []*netlink.Addr {
+		var diff []*netlink.Addr
+
+		for i, e1 := range s1 {
+			found := false
+			for _, e2 := range s2 {
+				if e1.Equal(e2) {
+					found = true
+					break
+				}
+			}
+			if !found {
+				diff = append(diff, &s1[i])
 			}
 		}
+
+		return diff
 	}
 
-	klog.V(2).Infof("Adding address %v to gateway interface %s", gwAddr, link.Attrs().Name)
-	if err := netlink.AddrAdd(link, gwAddr); err != nil {
-		klog.Errorf("Failed to set gateway interface %s with address %v: %v", link.Attrs().Name, gwAddr, err)
-		return err
+	addrsToAdd := addrSliceDifference(newAddrs, addrs)
+	addrsToRemove := addrSliceDifference(addrs, newAddrs)
+
+	if len(addrsToAdd) == 0 && len(addrsToRemove) == 0 {
+		klog.V(2).Infof("IP configuration for interface %s does not need to change", ifaceName)
+		return nil
+	}
+
+	for _, addr := range addrsToRemove {
+		klog.V(2).Infof("Removing address %v from interface %s", addr, ifaceName)
+		if err := netlink.AddrDel(link, addr); err != nil {
+			return fmt.Errorf("failed to remove address %v from interface %s: %v", addr, ifaceName, err)
+		}
 	}
+
+	for _, addr := range addrsToAdd {
+		klog.V(2).Infof("Adding address %v to interface %s", addr, ifaceName)
+		if err := netlink.AddrAdd(link, addr); err != nil {
+			return fmt.Errorf("failed to add address %v to interface %s: %v", addr, ifaceName, err)
+		}
+	}
+
 	return nil
 }
 

pkg/agent/util/net_windows.go

@@ -95,6 +95,17 @@ func ConfigureInterfaceAddress(ifaceName string, ipConfig *net.IPNet) error {
 	return nil
 }
 
+// RemoveInterfaceAddress removes IPAddress from the specified interface.
+func RemoveInterfaceAddress(ifaceName string, ipAddr net.IP) error {
+	cmd := fmt.Sprintf(`Remove-NetIPAddress -InterfaceAlias "%s" -IPAddress %s -Confirm:$false`, ifaceName, ipAddr.String())
+	err := InvokePSCommand(cmd)
+	// If the address does not exist, ignore the error.
+	if err != nil && !strings.Contains(err.Error(), "No matching") {
+		return err
+	}
+	return nil
+}
+
 // ConfigureInterfaceAddressWithDefaultGateway adds IPAddress on the specified interface and sets the default gateway
 // for the host.
 func ConfigureInterfaceAddressWithDefaultGateway(ifaceName string, ipConfig *net.IPNet, gateway string) error {
@@ -270,34 +281,73 @@ func SetLinkUp(name string) (net.HardwareAddr, int, error) {
 	return mac, index, nil
 }
 
-func ConfigureLinkAddress(idx int, gwIPNet *net.IPNet) error {
-	if gwIPNet.IP.To4() == nil {
-		klog.Warningf("Windows only supports IPv4 addresses. Skip this address %s", gwIPNet.String())
-		return nil
-	}
-
+func ConfigureLinkAddresses(idx int, ipNets []*net.IPNet) error {
 	iface, _ := net.InterfaceByIndex(idx)
-	gwIP := gwIPNet.IP
-	name := iface.Name
-	if addrs, err := iface.Addrs(); err != nil {
-		klog.Errorf("Failed to query IPv4 address list for interface %s: %v", name, err)
-		return err
-	} else if addrs != nil {
-		for _, addr := range addrs {
-			// Check with IPv4 address.
+	ifaceName := iface.Name
+	var addrs []*net.IPNet
+	if ifaceAddrs, err := iface.Addrs(); err != nil {
+		return fmt.Errorf("failed to query IPv4 address list for interface %s: %v", ifaceName, err)
+	} else {
+		for _, addr := range ifaceAddrs {
 			if ipNet, ok := addr.(*net.IPNet); ok {
-				if ipNet.IP.To4() != nil && ipNet.IP.Equal(gwIPNet.IP) {
-					return nil
+				if ipNet.IP.To4() != nil && !ipNet.IP.IsLinkLocalUnicast() {
+					addrs = append(addrs, ipNet)
 				}
 			}
 		}
 	}
 
-	klog.V(2).Infof("Adding address %v to gateway interface %s", gwIP, name)
-	if err := ConfigureInterfaceAddress(iface.Name, gwIPNet); err != nil {
-		klog.Errorf("Failed to set gateway interface %v with address %v: %v", iface, gwIP, err)
-		return err
+	addrEqual := func(addr1, addr2 *net.IPNet) bool {
+		size1, _ := addr1.Mask.Size()
+		size2, _ := addr2.Mask.Size()
+		return addr1.IP.Equal(addr2.IP) && size1 == size2
+	}
+
+	addrSliceDifference := func(s1, s2 []*net.IPNet) []*net.IPNet {
+		var diff []*net.IPNet
+
+		for _, e1 := range s1 {
+			found := false
+			for _, e2 := range s2 {
+				if addrEqual(e1, e2) {
+					found = true
+					break
+				}
+			}
+			if !found {
+				diff = append(diff, e1)
+			}
+		}
+
+		return diff
+	}
+
+	addrsToAdd := addrSliceDifference(ipNets, addrs)
+	addrsToRemove := addrSliceDifference(addrs, ipNets)
+
+	if len(addrsToAdd) == 0 && len(addrsToRemove) == 0 {
+		klog.V(2).Infof("IP configuration for interface %s does not need to change", ifaceName)
+		return nil
+	}
+
+	for _, addr := range addrsToRemove {
+		klog.V(2).Infof("Removing address %v from interface %s", addr, ifaceName)
+		if err := RemoveInterfaceAddress(ifaceName, addr.IP); err != nil {
+			return fmt.Errorf("failed to remove address %v from interface %s: %v", addr, ifaceName, err)
+		}
+	}
+
+	for _, addr := range addrsToAdd {
+		klog.V(2).Infof("Adding address %v to interface %s", addr, ifaceName)
+		if addr.IP.To4() == nil {
+			klog.Warningf("Windows only supports IPv4 addresses, skipping this address %v", addr)
+			return nil
+		}
+		if err := ConfigureInterfaceAddress(ifaceName, addr); err != nil {
+			return fmt.Errorf("failed to add address %v to interface %s: %v", addr, ifaceName, err)
+		}
 	}
+
 	return nil
 }
 

test/integration/agent/net_test.go

@@ -0,0 +1,82 @@
+// Copyright 2021 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package agent
+
+import (
+	"fmt"
+	"math/rand"
+	"net"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/vmware-tanzu/antrea/pkg/agent/util"
+)
+
+func init() {
+	rand.Seed(time.Now().UTC().UnixNano())
+}
+
+func addrEqual(addr1, addr2 *net.IPNet) bool {
+	size1, _ := addr1.Mask.Size()
+	size2, _ := addr2.Mask.Size()
+	return addr1.IP.Equal(addr2.IP) && size1 == size2
+}
+
+func TestConfigureLinkAddresses(t *testing.T) {
+	// #nosec G404: random number generator not used for security purposes
+	suffix := rand.Uint32()
+	ifaceName := fmt.Sprintf("test%x", suffix)
+	createTestInterface(t, ifaceName)
+	defer deleteTestInterface(t, ifaceName)
+	ifaceIdx := setTestInterfaceUp(t, ifaceName)
+
+	addrs := getTestInterfaceAddresses(t, ifaceName)
+	t.Logf("Found the following initial addresses: %v", addrs)
+	nAddrs := len(addrs)
+	// there can be up to one IPv6 link-local address and one IPv4
+	// link-local address (on Windows)
+	assert.LessOrEqual(t, nAddrs, 2)
+
+	_, dummyAddr, _ := net.ParseCIDR("192.0.2.0/24")
+
+	addTestInterfaceAddress(t, ifaceName, dummyAddr)
+	addrs2 := getTestInterfaceAddresses(t, ifaceName)
+	nAddrs2 := len(addrs2)
+	assert.Equal(t, nAddrs+1, nAddrs2)
+
+	_, ipAddr, _ := net.ParseCIDR("192.0.3.0/24")
+	err := util.ConfigureLinkAddresses(ifaceIdx, []*net.IPNet{ipAddr})
+	require.NoError(t, err)
+
+	addrs3 := getTestInterfaceAddresses(t, ifaceName)
+	nAddrs3 := len(addrs3)
+	assert.Equal(t, nAddrs+1, nAddrs3)
+
+	foundDummy := false
+	foundActual := false
+	for _, addr := range addrs3 {
+		switch {
+		case addrEqual(addr, dummyAddr):
+			foundDummy = true
+		case addrEqual(addr, ipAddr):
+			foundActual = true
+		}
+	}
+	assert.True(t, foundActual, "IP address was not assigned to test interface")
+	assert.False(t, foundDummy, "Dummy IP address should have been removed from test interface")
+}