fix tunnel interface name issue #2486

luolanzone · 2021-07-28T09:26:54Z

the purpose of this commit is to fix a tunnel interface issue founded during some IPSec PoC verification:

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' inside a Antrea agent 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.
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. so refine it with more checks.

and also add some unit test cases.

Signed-off-by: Lan Luo [email protected]

codecov-commenter · 2021-07-28T10:18:38Z

Codecov Report

Merging #2486 (e35417e) into main (42077cb) will decrease coverage by 14.66%.
The diff coverage is 5.55%.

@@             Coverage Diff             @@
##             main    #2486       +/-   ##
===========================================
- Coverage   61.70%   47.04%   -14.67%     
===========================================
  Files         276      278        +2     
  Lines       21317    21364       +47     
===========================================
- Hits        13154    10051     -3103     
- Misses       6782     9985     +3203     
+ Partials     1381     1328       -53

Flag	Coverage Δ
kind-e2e-tests	`47.04% <5.55%> (-5.88%)`	⬇️
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...gent/controller/noderoute/node_route_controller.go	`40.13% <0.00%> (-6.95%)`	⬇️
pkg/agent/util/net.go	`51.28% <50.00%> (ø)`
pkg/agent/controller/egress/id_allocator.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`2.85% <0.00%> (-88.58%)`	⬇️
pkg/controller/egress/controller.go	`0.00% <0.00%> (-83.88%)`	⬇️
pkg/agent/util/iptables/lock.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/controller/networkpolicy/crd_utils.go	`10.84% <0.00%> (-80.73%)`	⬇️
pkg/apis/controlplane/v1beta1/conversion.go	`1.20% <0.00%> (-79.81%)`	⬇️
pkg/controller/networkpolicy/clustergroup.go	`3.27% <0.00%> (-79.44%)`	⬇️
pkg/cni/client.go	`0.00% <0.00%> (-75.52%)`	⬇️
... and 117 more

antoninbas · 2021-07-28T16:56:38Z

pkg/agent/controller/noderoute/node_route_controller.go

+	if ok && (interfaceConfig.InterfaceName != portName ||
+		interfaceConfig.PSK != c.networkConfig.IPSecPSK ||
+		!interfaceConfig.RemoteIP.Equal(nodeIP) ||
+		interfaceConfig.TunnelInterfaceConfig.Type != c.networkConfig.TunnelType) {


this seems to be duplicated from removeStaleTunnelPorts, can we define a separate function for tunnel config comparison?

sure, will do.

antoninbas · 2021-07-28T16:57:25Z

pkg/agent/controller/noderoute/node_route_controller.go

+		} else {
+			stalePortRemoved = true
+		}


I am curious as to why we don't just return an error here and let the Node be requeued by the controller?

ok, I will leave it to reconcile

pkg/agent/util/net.go

this PR is based on antrea-io#2486 and I verified all tunnel modes with IPSec in K8s Cluster, it all works fine now, so I remove the limitation on our docs and the check in the code. Signed-off-by: Lan Luo <[email protected]>

abhiraut

minor nits

abhiraut · 2021-07-29T22:21:38Z

pkg/agent/controller/noderoute/node_route_controller.go

+		if !c.validateInterfaceConfig(interfaceConfig, nodeIP, portName) {
+			return 0, fmt.Errorf("IPSec tunnel interface config mismatched with cached one,stale IPSec tunnel port %s", interfaceConfig.InterfaceName)
+		} else {
+			klog.V(2).Infof("find cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)


Suggested change

klog.V(2).Infof("find cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)

klog.V(2).Infof("Found cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)

abhiraut · 2021-07-29T22:22:22Z

pkg/agent/controller/noderoute/node_route_controller.go

-			return interfaceConfig.OFPort, nil
+		if !c.validateInterfaceConfig(interfaceConfig, nodeIP, portName) {
+			return 0, fmt.Errorf("IPSec tunnel interface config mismatched with cached one,stale IPSec tunnel port %s", interfaceConfig.InterfaceName)
+		} else {


else block is not needed since you return in the if statement

pkg/agent/util/net.go

luolanzone · 2021-08-04T01:38:08Z

@antoninbas @abhiraut Could you help to take a look again? thanks!

antoninbas · 2021-08-04T01:57:37Z

pkg/agent/controller/noderoute/node_route_controller.go

@@ -258,6 +256,14 @@ func (c *Controller) removeStaleTunnelPorts() error {
 	return nil
 }

+func (c *Controller) validateInterfaceConfig(interfaceConfig *interfacestore.InterfaceConfig,


s/validateInterfaceConfig/compareInterfaceConfig

antoninbas · 2021-08-04T01:59:50Z

pkg/agent/controller/noderoute/node_route_controller.go

 	interfaceConfig, ok := c.interfaceStore.GetNodeTunnelInterface(nodeName)
+	// 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, return error to requeue the node.


s/node/Node

antoninbas · 2021-08-04T02:00:16Z

pkg/agent/controller/noderoute/node_route_controller.go

-		// happen if removeStaleTunnelPorts fails to remove a "stale"
-		// tunnel port for which the configuration has changed.
+		if !c.validateInterfaceConfig(interfaceConfig, nodeIP, portName) {
+			return 0, fmt.Errorf("IPSec tunnel interface config mismatched with cached one, stale IPSec tunnel port %s", interfaceConfig.InterfaceName)


s/mismatched with cached one/doesn't match cached one

antoninbas · 2021-08-04T02:00:34Z

pkg/agent/controller/noderoute/node_route_controller.go

+		if !c.validateInterfaceConfig(interfaceConfig, nodeIP, portName) {
+			return 0, fmt.Errorf("IPSec tunnel interface config mismatched with cached one, stale IPSec tunnel port %s", interfaceConfig.InterfaceName)
+		}
+		klog.V(2).Infof("Found cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)


please use structured logging: klog.V(2).InfoS

antoninbas · 2021-08-04T02:01:06Z

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"
+


we don't typically split these into 2 separate import groups IIRC

antoninbas · 2021-08-04T02:04:00Z

pkg/agent/util/net.go

@@ -38,10 +39,14 @@ func generateInterfaceName(key string, name string, useHead bool) string {
 	interfaceKey := hex.EncodeToString(hash.Sum(nil))
 	prefix := name
 	if len(name) > interfacePrefixLength {
+		// We use Node/Pod name to generate the interface name,
+		// valid chars for Node/Pod name are ASCII letters from a to z,
+		// the digits from 0 to 9, and the hyphen (-), hyphen (-) is the only char


should be a new sentence:

Hyphen (-) is the only char which will impact command-line interpretation if the interface name starts with one, so we remove it here.

updated it.

antoninbas · 2021-08-04T02:41:57Z

pkg/agent/controller/noderoute/node_route_controller_test.go

+	c, closeFn := newController(t, &config.NetworkConfig{
+		TrafficEncapMode:  0,
+		TunnelType:        ovsconfig.TunnelType("vxlan"),
+		EnableIPSecTunnel: true,
+		IPSecPSK:          "changeme",
+	})
+
+	c.interfaceStore.AddInterface(&interfacestore.InterfaceConfig{
+		Type:          interfacestore.TunnelInterface,
+		InterfaceName: util.GenerateNodeTunnelInterfaceName("xyz-k8s-0-1"),
+		TunnelInterfaceConfig: &interfacestore.TunnelInterfaceConfig{
+			NodeName: "xyz-k8s-0-1",
+			Type:     ovsconfig.TunnelType("vxlan"),
+			PSK:      "mismatchpsk",
+			RemoteIP: nodeIP1,
+		},
+		OVSPortConfig: &interfacestore.OVSPortConfig{
+			PortUUID: "123",
+		},
+	})
+
+	defer closeFn()
+	defer c.queue.ShutDown()
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	c.informerFactory.Start(stopCh)
+	c.informerFactory.WaitForCacheSync(stopCh)


maybe unify this set up code in a separate function:

func setup(ifaces ...*interfacestore.InterfaceConfig) cleanupFn { ... }

then in the test:

cleanup := setup(ifaceConfig) defer cleanup()

antoninbas · 2021-08-04T02:43:44Z

pkg/agent/controller/noderoute/node_route_controller_test.go

+	tests := []struct {
+		name    string
+		wantErr bool
+	}{
+		{
+			name:    "good path with IPSec enabled",
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		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)
+			}
+		})


do you think there will be more tests added in the future? maybe since you only have one test right now there is no need to use a table pattern

removed table pattern

antoninbas · 2021-08-04T18:30:42Z

pkg/agent/controller/noderoute/node_route_controller_test.go

+			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
+			}


can we use testify assertions for these tests like we do in other Antrea unit tests? assert assertions will continue the test on error, while require assertions will abort the test on error

antoninbas · 2021-08-04T18:31:57Z

pkg/agent/controller/noderoute/node_route_controller_test.go

+		want       int32
+	}{
+		{
+			name:       "good path",


"good path" is not a great name, maybe "create new port"

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 <[email protected]>

antoninbas · 2021-08-05T22:34:59Z

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

luolanzone · 2021-08-06T01:03:49Z

/test-ipv6-only-e2e

antoninbas · 2021-08-06T20:36:56Z

@luolanzone please cherry-pick to release-1.2

this PR is based on antrea-io#2486 and I verified all tunnel modes with IPSec in K8s Cluster, it all works fine now, so I remove the limitation on our docs and the check in the code. Signed-off-by: Lan Luo <[email protected]>

tnqn

Sorry for late review, I notice this change via another PR and have a question.

tnqn · 2021-08-10T16:42:18Z

pkg/agent/controller/noderoute/node_route_controller.go

 	interfaceConfig, ok := c.interfaceStore.GetNodeTunnelInterface(nodeName)
+	// 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, return error to requeue the Node.


Since you removed the TODO, and according to the commit message, I suppose this is fixed. However, how requeuing the Node helps? I think the next attemp will still get here and requeue again?

Hi @tnqn, I think the TODO is highlighting it doesn't check the configuration change here but return directly. it's an assumption that it's a temporary issue if last remove action failed in removeStaleTunnelPorts, so re-queue will help because removeStaleTunnelPorts also checks configuration difference. but it indeed will go back here if failed again, is there any case in your mind port can't be removed?

My understanding was that we would call c.ovsBridgeClient.DeletePort and c.interfaceStore.DeleteInterface here, and in case of error during port removal, return an error and let the Node be requeued. removeStaleTunnelPorts is only called on controller start up and transient errors are always possible.

So the issue is not really fixed. @luolanzone would you have a follow-up PR to fix it? and maybe have a test covering this scenario.

sure, I will fix this.

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 <[email protected]>

this PR is based on antrea-io#2486 and I verified all tunnel modes with IPSec in K8s Cluster, it all works fine now, so I remove the limitation on our docs and the check in the code. Signed-off-by: Lan Luo <[email protected]>

this PR is based on #2486 and I verified all tunnel modes with IPSec in K8s Cluster, it all works fine now, so I remove the limitation on our docs and the check in the code. Signed-off-by: Lan Luo <[email protected]>

luolanzone force-pushed the tunnel-bug-fix branch from 3a68e51 to e35417e Compare July 28, 2021 14:40

antoninbas reviewed Jul 28, 2021

View reviewed changes

luolanzone force-pushed the tunnel-bug-fix branch 2 times, most recently from 17c50ac to 0e978e4 Compare July 29, 2021 02:56

luolanzone mentioned this pull request Jul 29, 2021

Remove IPSec tunnel limitation #2489

Merged

abhiraut reviewed Jul 29, 2021

View reviewed changes

luolanzone force-pushed the tunnel-bug-fix branch from 0e978e4 to 328af21 Compare July 30, 2021 01:15

abhiraut previously approved these changes Jul 30, 2021

View reviewed changes

luolanzone dismissed abhiraut’s stale review via b734541 July 30, 2021 02:11

luolanzone force-pushed the tunnel-bug-fix branch 6 times, most recently from bb1581d to bfe6f25 Compare July 30, 2021 10:21

antoninbas reviewed Aug 4, 2021

View reviewed changes

luolanzone force-pushed the tunnel-bug-fix branch 2 times, most recently from 16a97ce to 39e711d Compare August 4, 2021 03:38

antoninbas reviewed Aug 4, 2021

View reviewed changes

luolanzone force-pushed the tunnel-bug-fix branch from 39e711d to 74e2a6c Compare August 5, 2021 05:59

antoninbas approved these changes Aug 5, 2021

View reviewed changes

abhiraut approved these changes Aug 6, 2021

View reviewed changes

antoninbas merged commit 09fc82f into antrea-io:main Aug 6, 2021

luolanzone mentioned this pull request Aug 9, 2021

Automated cherry pick of #2486: fix tunnel interface name issue #2560

Merged

luolanzone deleted the tunnel-bug-fix branch August 9, 2021 02:32

tnqn reviewed Aug 10, 2021

View reviewed changes

	klog.V(2).Infof("find cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)
	klog.V(2).Infof("Found cached IPSec tunnel interface %s with port %d for Node %s", interfaceConfig.InterfaceName, interfaceConfig.OFPort, nodeName)

fix tunnel interface name issue #2486

fix tunnel interface name issue #2486

Conversation

luolanzone commented Jul 28, 2021

codecov-commenter commented Jul 28, 2021 • edited Loading

Codecov Report

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

abhiraut 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

luolanzone commented Aug 4, 2021

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

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 commented Aug 5, 2021

luolanzone commented Aug 6, 2021

antoninbas commented Aug 6, 2021

tnqn 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

codecov-commenter commented Jul 28, 2021 •

edited

Loading