From e35417e9c43dfd6cffa0ce10157b8a95b5ebde69 Mon Sep 17 00:00:00 2001 From: Lan Luo Date: Wed, 28 Jul 2021 17:07:06 +0800 Subject: [PATCH] fix tunnel interface name issue the purpose of this commit is to fix a tunnel interface issue founded during some IPSec PoC verification: 1. when the node name is like 'lan-k8s-0-0', the IPsec tunnel interface name will be like '-k8s-0-2-e8dbe6', then it will failed to run command like `ipsec up '-k8s-0-2-e8dbe6'` with error `/usr/lib/ipsec/stroke: invalid option -- 'k'` due to the first char is '-', ipsec command interrupted it as an option. so changed the `generateInterfaceName` method to use `strings.TrimLeft()` to remove '-' in left. 2. `createIPSecTunnelPort` method can't handle the case when tunnel interface name changed when there is cache matched for the node. it will reuse the existing tunnel name without creating a new one with new name which means any change in `generateInterfaceName` won't take affect. and also add some unit test cases. Signed-off-by: Lan Luo --- .../noderoute/node_route_controller.go | 28 ++- .../noderoute/node_route_controller_test.go | 169 ++++++++++++++++++ pkg/agent/util/net.go | 5 +- 3 files changed, 194 insertions(+), 8 deletions(-) diff --git a/pkg/agent/controller/noderoute/node_route_controller.go b/pkg/agent/controller/noderoute/node_route_controller.go index 7d2dd79dd9a..9d67c6e5b86 100644 --- a/pkg/agent/controller/noderoute/node_route_controller.go +++ b/pkg/agent/controller/noderoute/node_route_controller.go @@ -222,7 +222,9 @@ func (c *Controller) removeStaleTunnelPorts() error { } ifaceID := util.GenerateNodeTunnelInterfaceKey(node.Name) - validConfiguration := interfaceConfig.PSK == c.networkConfig.IPSecPSK && + ifaceName := util.GenerateNodeTunnelInterfaceName(node.Name) + validConfiguration := interfaceConfig.InterfaceName == ifaceName && + interfaceConfig.PSK == c.networkConfig.IPSecPSK && interfaceConfig.RemoteIP.Equal(peerNodeIP) && interfaceConfig.TunnelInterfaceConfig.Type == c.networkConfig.TunnelType if validConfiguration { @@ -529,16 +531,30 @@ func getPodCIDRsOnNode(node *corev1.Node) []string { // createIPSecTunnelPort creates an IPSec tunnel port for the remote Node if the // tunnel does not exist, and returns the ofport number. func (c *Controller) createIPSecTunnelPort(nodeName string, nodeIP net.IP) (int32, error) { + portName := util.GenerateNodeTunnelInterfaceName(nodeName) interfaceConfig, ok := c.interfaceStore.GetNodeTunnelInterface(nodeName) - if ok { - // TODO: check if Node IP, PSK, or tunnel type changes. This can - // happen if removeStaleTunnelPorts fails to remove a "stale" - // tunnel port for which the configuration has changed. + stalePortRemoved := false + // check if Node IP, PSK, or tunnel type changes. This can + // happen if removeStaleTunnelPorts fails to remove a "stale" + // tunnel port for which the configuration has changed. + if ok && (interfaceConfig.InterfaceName != portName || + interfaceConfig.PSK != c.networkConfig.IPSecPSK || + !interfaceConfig.RemoteIP.Equal(nodeIP) || + interfaceConfig.TunnelInterfaceConfig.Type != c.networkConfig.TunnelType) { + klog.Infof("IPSec tunnel interface config mismatched with cached one, remove stale IPSec tunnel port %s", interfaceConfig.InterfaceName) + if err := c.ovsBridgeClient.DeletePort(interfaceConfig.PortUUID); err != nil { + klog.Errorf("fail to remove stale IPSec tunnel port %d for Node %s", interfaceConfig.OFPort, nodeName) + } else { + stalePortRemoved = true + } + } + + if ok && !stalePortRemoved { + klog.V(2).Infof("find cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName) if interfaceConfig.OFPort != 0 { return interfaceConfig.OFPort, nil } } else { - portName := util.GenerateNodeTunnelInterfaceName(nodeName) ovsExternalIDs := map[string]interface{}{ovsExternalIDNodeName: nodeName} portUUID, err := c.ovsBridgeClient.CreateTunnelPortExt( portName, diff --git a/pkg/agent/controller/noderoute/node_route_controller_test.go b/pkg/agent/controller/noderoute/node_route_controller_test.go index 162055cbe7f..a3fc084d23e 100644 --- a/pkg/agent/controller/noderoute/node_route_controller_test.go +++ b/pkg/agent/controller/noderoute/node_route_controller_test.go @@ -23,6 +23,7 @@ import ( "github.com/containernetworking/plugins/pkg/ip" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/informers" @@ -32,6 +33,8 @@ import ( "antrea.io/antrea/pkg/agent/interfacestore" oftest "antrea.io/antrea/pkg/agent/openflow/testing" routetest "antrea.io/antrea/pkg/agent/route/testing" + "antrea.io/antrea/pkg/agent/util" + "antrea.io/antrea/pkg/ovs/ovsconfig" ovsconfigtest "antrea.io/antrea/pkg/ovs/ovsconfig/testing" ) @@ -228,3 +231,169 @@ func TestIPInPodSubnets(t *testing.T) { assert.Equal(t, false, c.Controller.IPInPodSubnets(net.ParseIP("10.10.10.10"))) assert.Equal(t, false, c.Controller.IPInPodSubnets(net.ParseIP("8.8.8.8"))) } + +func TestRemoveStaleTunnelPorts(t *testing.T) { + c, closeFn := newController(t) + defer closeFn() + defer c.queue.ShutDown() + stopCh := make(chan struct{}) + defer close(stopCh) + c.informerFactory.Start(stopCh) + c.informerFactory.WaitForCacheSync(stopCh) + + c.interfaceStore.AddInterface(&interfacestore.InterfaceConfig{ + Type: 2, + InterfaceName: util.GenerateNodeTunnelInterfaceName("xyz-k8s-0-1"), + TunnelInterfaceConfig: &interfacestore.TunnelInterfaceConfig{ + NodeName: "xyz-k8s-0-1", + Type: "vxlan", + PSK: "changeme", + RemoteIP: net.ParseIP("10.10.10.10"), + }, + OVSPortConfig: &interfacestore.OVSPortConfig{ + PortUUID: "123", + }, + }) + c.interfaceStore.AddInterface(&interfacestore.InterfaceConfig{ + Type: 2, + InterfaceName: util.GenerateNodeTunnelInterfaceName("xyz-k8s-0-2"), + TunnelInterfaceConfig: &interfacestore.TunnelInterfaceConfig{ + NodeName: "xyz-k8s-0-2", + Type: "vxlan", + }, + OVSPortConfig: &interfacestore.OVSPortConfig{ + PortUUID: "abc", + }, + }) + + node1 := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "xyz-k8s-0-1", + }, + Spec: corev1.NodeSpec{ + PodCIDR: podCIDR.String(), + PodCIDRs: []string{podCIDR.String()}, + }, + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + { + Type: corev1.NodeInternalIP, + Address: nodeIP1.String(), + }, + }, + }, + } + c.clientset.CoreV1().Nodes().Create(context.TODO(), node1, metav1.CreateOptions{}) + c.ovsClient.EXPECT().DeletePort("abc").Times(1) + tests := []struct { + name string + networkConfig *config.NetworkConfig + wantErr bool + }{ + { + name: "good path with IPsec enable", + networkConfig: &config.NetworkConfig{ + TrafficEncapMode: 0, + TunnelType: ovsconfig.TunnelType("vxlan"), + EnableIPSecTunnel: true, + IPSecPSK: "changeme", + }, + wantErr: false, + }, + } + for _, tt := range tests { + c.networkConfig = tt.networkConfig + t.Run(tt.name, func(t *testing.T) { + if err := c.removeStaleTunnelPorts(); (err != nil) != tt.wantErr { + t.Errorf("Controller.removeStaleTunnelPorts() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestCreateIPSecTunnelPort(t *testing.T) { + c, closeFn := newController(t) + defer closeFn() + defer c.queue.ShutDown() + c.interfaceStore.AddInterface(&interfacestore.InterfaceConfig{ + Type: 2, + InterfaceName: "mismatchedname", + TunnelInterfaceConfig: &interfacestore.TunnelInterfaceConfig{ + NodeName: "xyz-k8s-0-2", + Type: "vxlan", + PSK: "changeme", + RemoteIP: net.ParseIP("10.10.10.10"), + }, + OVSPortConfig: &interfacestore.OVSPortConfig{ + PortUUID: "123", + }, + }) + + stopCh := make(chan struct{}) + defer close(stopCh) + c.informerFactory.Start(stopCh) + c.informerFactory.WaitForCacheSync(stopCh) + + node1PortName := util.GenerateNodeTunnelInterfaceName("xyz-k8s-0-1") + node2PortName := util.GenerateNodeTunnelInterfaceName("xyz-k8s-0-2") + + c.ovsClient.EXPECT().CreateTunnelPortExt( + node1PortName, ovsconfig.TunnelType("vxlan"), int32(0), + false, "", "10.10.10.1", "changeme", + map[string]interface{}{ovsExternalIDNodeName: "xyz-k8s-0-1"}).Times(1) + c.ovsClient.EXPECT().CreateTunnelPortExt( + node2PortName, ovsconfig.TunnelType("vxlan"), int32(0), + false, "", "10.10.10.1", "changeme", + map[string]interface{}{ovsExternalIDNodeName: "xyz-k8s-0-2"}).Times(1) + c.ovsClient.EXPECT().GetOFPort(node1PortName).Return(int32(1), nil) + c.ovsClient.EXPECT().GetOFPort(node2PortName).Return(int32(2), nil) + c.ovsClient.EXPECT().DeletePort("123").Times(1) + + tests := []struct { + name string + networkConfig *config.NetworkConfig + nodeName string + wantErr bool + want int32 + }{ + { + name: "good path", + networkConfig: &config.NetworkConfig{ + TrafficEncapMode: 0, + TunnelType: ovsconfig.TunnelType("vxlan"), + EnableIPSecTunnel: true, + IPSecPSK: "changeme", + }, + nodeName: "xyz-k8s-0-1", + wantErr: false, + want: 1, + }, + { + name: "hit cache but interface name changed for the same node, need remove stale port", + networkConfig: &config.NetworkConfig{ + TrafficEncapMode: 0, + TunnelType: "vxlan", + EnableIPSecTunnel: true, + IPSecPSK: "changeme", + }, + nodeName: "xyz-k8s-0-2", + wantErr: false, + want: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c.networkConfig = tt.networkConfig + got, err := c.createIPSecTunnelPort(tt.nodeName, net.ParseIP("10.10.10.1")) + if (err != nil) != tt.wantErr { + t.Errorf("Controller.createIPSecTunnelPort() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("Controller.createIPSecTunnelPort() got %v,want = %v", got, tt.want) + return + } + }) + } +} diff --git a/pkg/agent/util/net.go b/pkg/agent/util/net.go index 13d15240570..8e51b2e1043 100644 --- a/pkg/agent/util/net.go +++ b/pkg/agent/util/net.go @@ -21,6 +21,7 @@ import ( "fmt" "io" "net" + "strings" ) const ( @@ -39,9 +40,9 @@ func generateInterfaceName(key string, name string, useHead bool) string { prefix := name if len(name) > interfacePrefixLength { if useHead { - prefix = name[:interfacePrefixLength] + prefix = strings.TrimLeft(name[:interfacePrefixLength], "-") } else { - prefix = name[len(name)-interfacePrefixLength:] + prefix = strings.TrimLeft(name[len(name)-interfacePrefixLength:], "-") } } return fmt.Sprintf("%s-%s", prefix, interfaceKey[:interfaceKeyLength])