consolidate logic and improve uts

Author: QxBytes

network/transparent_vlan_endpointclient_linux.go

@@ -110,6 +110,19 @@ func NewTransparentVlanEndpointClient(
 	return client
 }
 
+// cleanupInterfaceIfExists checks if an interface exists and deletes it if found
+// Returns an error if the deletion fails
+func (client *TransparentVlanEndpointClient) cleanupInterfaceIfExists(interfaceName string) error {
+	_, ifExists := client.netioshim.GetNetworkInterfaceByName(interfaceName)
+	if ifExists == nil {
+		logger.Info("Interface exists, deleting", zap.String("interfaceName", interfaceName))
+		if err := client.netlink.DeleteLink(interfaceName); err != nil {
+			return errors.Wrapf(err, "failed to clean up %s", interfaceName)
+		}
+	}
+	return nil
+}
+
 // Adds interfaces to the vnet (created if not existing) and vm namespace
 func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) error {
 	// VM Namespace
@@ -151,13 +164,9 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error {
 		}
 	}
 	// Delete the vlan interface in the VM namespace if it exists
-	_, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName)
-	if vlanIfInVMErr == nil {
-		// The vlan interface exists in the VM ns because it failed to move into the network ns previously and needs to be cleaned up
-		logger.Info("vlan interface exists on the VM namespace, deleting", zap.String("vlanIfName", client.vlanIfName))
-		if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil {
-			return errors.Wrap(delErr, "failed to clean up vlan interface")
-		}
+	// The vlan interface exists in the VM ns because it failed to move into the network ns previously and needs to be cleaned up
+	if delErr := client.cleanupInterfaceIfExists(client.vlanIfName); delErr != nil {
+		return errors.Wrap(delErr, "failed to clean up vlan interface")
 	}
 	return nil
 }
@@ -188,7 +197,7 @@ func (client *TransparentVlanEndpointClient) setLinkNetNSAndConfirm(name string,
 	logger.Info("Move link to NS", zap.String("ifName", name), zap.Any("NSFileDescriptor", fd))
 	err := client.netlink.SetLinkNetNs(name, fd)
 	if err != nil {
-		return errors.Wrapf(err, "failed to set %v inside namespace %v", name, fd)
+		return errors.Wrapf(err, "failed to set %v inside namespace %v (%s)", name, fd, nsName)
 	}
 
 	// confirm veth was moved successfully
@@ -200,7 +209,7 @@ func (client *TransparentVlanEndpointClient) setLinkNetNSAndConfirm(name string,
 		})
 	}, numRetries, sleepInMs)
 	if err != nil {
-		return errors.Wrapf(err, "failed to detect %v inside namespace %v", name, fd)
+		return errors.Wrapf(err, "failed to detect %v inside namespace %v (%s)", name, fd, nsName)
 	}
 	return nil
 }
@@ -311,13 +320,11 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er
 	}
 
 	// Proactively clean up any leftover veth interfaces before creating new ones
-	if err = client.netlink.DeleteLink(client.vnetVethName); err != nil {
-		logger.Info("Could not proactively clean up vnet veth",
-			zap.String("vnetVethName", client.vnetVethName), zap.Error(err))
+	if vnetDelErr := client.cleanupInterfaceIfExists(client.vnetVethName); vnetDelErr != nil {
+		logger.Info("Could not proactively clean up vnet veth", zap.String("vnetVethName", client.vnetVethName), zap.Error(vnetDelErr))
 	}
-	if err = client.netlink.DeleteLink(client.containerVethName); err != nil {
-		logger.Info("Could not proactively clean up container veth",
-			zap.String("containerVethName", client.containerVethName), zap.Error(err))
+	if containerDelErr := client.cleanupInterfaceIfExists(client.containerVethName); containerDelErr != nil {
+		logger.Info("Could not proactively clean up container veth", zap.String("containerVethName", client.containerVethName), zap.Error(containerDelErr))
 	}
 
 	// Create veth pair

network/transparent_vlan_endpointclient_linux_test.go

@@ -121,12 +121,14 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 	nl := netlink.NewMockNetlink(false, "")
 	plc := platform.NewMockExecClient(false)
 
-	tests := []struct {
-		name       string
-		client     *TransparentVlanEndpointClient
-		epInfo     *EndpointInfo
-		wantErr    bool
-		wantErrMsg string
+	setLinkNetNSTests := []struct {
+		name          string
+		client        *TransparentVlanEndpointClient
+		epInfo        *EndpointInfo
+		moveInterface string
+		moveNS        string
+		wantErr       bool
+		wantErrMsg    string
 	}{
 		// Set the link network namespace and confirm that it was moved inside
 		{
@@ -143,8 +145,10 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 				netioshim:         netio.NewMockNetIO(false, 0),
 				nsClient:          NewMockNamespaceClient(),
 			},
-			epInfo:  &EndpointInfo{},
-			wantErr: false,
+			moveInterface: "eth0.1",
+			moveNS:        "az_ns_1",
+			epInfo:        &EndpointInfo{},
+			wantErr:       false,
 		},
 		{
 			name: "Set link netns fail to set",
@@ -160,9 +164,11 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 				netioshim:         netio.NewMockNetIO(false, 0),
 				nsClient:          NewMockNamespaceClient(),
 			},
-			epInfo:     &EndpointInfo{},
-			wantErr:    true,
-			wantErrMsg: "failed to set eth0.1",
+			moveInterface: "A1veth0",
+			moveNS:        "az_ns_2",
+			epInfo:        &EndpointInfo{},
+			wantErr:       true,
+			wantErrMsg:    "failed to set A1veth0 inside namespace 1 (az_ns_2)",
 		},
 		{
 			name: "Set link netns fail to detect",
@@ -181,15 +187,17 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 				},
 				nsClient: NewMockNamespaceClient(),
 			},
-			epInfo:     &EndpointInfo{},
-			wantErr:    true,
-			wantErrMsg: "failed to detect eth0.1",
+			moveInterface: "eth0.1",
+			moveNS:        "az_ns_1",
+			epInfo:        &EndpointInfo{},
+			wantErr:       true,
+			wantErrMsg:    "failed to detect eth0.1 inside namespace 1 (az_ns_1)",
 		},
 	}
-	for _, tt := range tests {
+	for _, tt := range setLinkNetNSTests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			err := tt.client.setLinkNetNSAndConfirm(tt.client.vlanIfName, 1, tt.client.vnetNSName)
+			err := tt.client.setLinkNetNSAndConfirm(tt.moveInterface, 1, tt.moveNS)
 			if tt.wantErr {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), tt.wantErrMsg, "Expected:%v actual:%v", tt.wantErrMsg, err.Error())
@@ -199,7 +207,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 		})
 	}
 
-	tests = []struct {
+	tests := []struct {
 		name       string
 		client     *TransparentVlanEndpointClient
 		epInfo     *EndpointInfo
@@ -256,7 +264,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 			},
 			epInfo:     &EndpointInfo{},
 			wantErr:    true,
-			wantErrMsg: "failed to cleanup/delete ns after noticing vlan veth does not exist: netns failure: " + errNetnsMock.Error(),
+			wantErrMsg: "failed to cleanup/delete ns after noticing vlan interface does not exist: netns failure: " + errNetnsMock.Error(),
 		},
 		{
 			name: "Ensure clean populate VM cleanup straggling vlan if in vm ns",
@@ -380,7 +388,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 			},
 			epInfo:     &EndpointInfo{},
 			wantErr:    true,
-			wantErrMsg: "failed to move or detect vnetVethName in vnet ns, deleting: failed to set A1veth0 inside namespace 1: " + netlink.ErrorMockNetlink.Error() + " : netlink fail",
+			wantErrMsg: "failed to move or detect vnetVethName in vnet ns, deleting: failed to set A1veth0 inside namespace 1 (az_ns_1): " + netlink.ErrorMockNetlink.Error() + " : netlink fail",
 		},
 		{
 			name: "Add endpoints get interface fail for primary interface (eth0)",
@@ -528,7 +536,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "Add endpoints fail check vlan veth exists",
+			name: "Add endpoints fail check vlan interface exists",
 			client: &TransparentVlanEndpointClient{
 				primaryHostIfName: "eth0",
 				vlanIfName:        "eth0.1",
@@ -542,7 +550,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) {
 			},
 			epInfo:     &EndpointInfo{},
 			wantErr:    true,
-			wantErrMsg: "vlan veth doesn't exist: " + netio.ErrMockNetIOFail.Error() + ":eth0.1",
+			wantErrMsg: "vlan interface doesn't exist: " + netio.ErrMockNetIOFail.Error() + ":eth0.1",
 		},
 		{
 			name: "Add endpoints fail check vnet veth exists",