fix: modify cleanup path to always delete link (#3519)

* fix cleanup path to always delete link

* remove error log as we no longer return errors

Author: QxBytes

netlink/mocknetlink.go

@@ -22,6 +22,7 @@ type MockNetlink struct {
 	errorString   string
 	deleteRouteFn routeValidateFn
 	addRouteFn    routeValidateFn
+	DeleteLinkFn  func(name string) error
 }
 
 func NewMockNetlink(returnError bool, errorString string) *MockNetlink {
@@ -55,6 +56,9 @@ func (f *MockNetlink) SetLinkMTU(name string, mtu int) error {
 }
 
 func (f *MockNetlink) DeleteLink(name string) error {
+	if f.DeleteLinkFn != nil {
+		return f.DeleteLinkFn(name)
+	}
 	return f.error()
 }
 

network/transparent_vlan_endpointclient_linux.go

@@ -622,7 +622,7 @@ func (client *TransparentVlanEndpointClient) AddDefaultArp(interfaceName, destMa
 
 func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error {
 	// Vnet NS
-	err := ExecuteInNS(client.nsClient, client.vnetNSName, func() error {
+	_ = ExecuteInNS(client.nsClient, client.vnetNSName, func() error {
 		// Passing in functionality to get number of routes after deletion
 		getNumRoutesLeft := func() (int, error) {
 			routes, err := vishnetlink.RouteList(nil, vishnetlink.FAMILY_V4)
@@ -632,11 +632,9 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error
 			return len(routes), nil
 		}
 
-		return client.DeleteEndpointsImpl(ep, getNumRoutesLeft)
+		client.DeleteEndpointsImpl(ep, getNumRoutesLeft)
+		return nil
 	})
-	if err != nil {
-		logger.Warn("could not delete transparent vlan endpoints", zap.String("errorMsg", err.Error()))
-	}
 
 	// VM NS
 	if err := client.DeleteSnatEndpoint(); err != nil {
@@ -646,16 +644,16 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error
 }
 
 // getNumRoutesLeft is a function which gets the current number of routes in the namespace. Namespace: Vnet
-func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, _ func() (int, error)) error {
+func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, _ func() (int, error)) {
 	routeInfoList := client.GetVnetRoutes(ep.IPAddresses)
 	if err := deleteRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList); err != nil {
-		return errors.Wrap(err, "failed to remove routes")
+		logger.Error("Failed to remove routes", zap.Error(err))
 	}
 
 	logger.Info("Deleting host veth", zap.String("vnetVethName", client.vnetVethName))
 	// Delete Host Veth
 	if err := client.netlink.DeleteLink(client.vnetVethName); err != nil {
-		return errors.Wrapf(err, "deleteLink for %v failed", client.vnetVethName)
+		logger.Error("Failed to delete link", zap.Error(err), zap.String("vnetVethName", client.vnetVethName))
 	}
 
 	// TODO: revist if this require in future.
@@ -670,7 +668,6 @@ func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, _
 			}
 		}
 	*/
-	return nil
 }
 
 // Helper function that allows executing a function in a VM namespace

network/transparent_vlan_endpointclient_linux_test.go

@@ -633,7 +633,6 @@ func TestTransparentVlanDeleteEndpoints(t *testing.T) {
 			routesLeft: func() (int, error) {
 				return numDefaultRoutes, nil
 			},
-			wantErr: false,
 		},
 		{
 			name: "Delete endpoint do not delete vnet ns it is still in use",
@@ -659,7 +658,6 @@ func TestTransparentVlanDeleteEndpoints(t *testing.T) {
 			routesLeft: func() (int, error) {
 				return numDefaultRoutes + 1, nil
 			},
-			wantErr: false,
 		},
 		//nolint gocritic
 		/*		{
@@ -694,15 +692,47 @@ func TestTransparentVlanDeleteEndpoints(t *testing.T) {
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			err := tt.client.DeleteEndpointsImpl(tt.ep, tt.routesLeft)
-			if tt.wantErr {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), tt.wantErrMsg, "Expected:%v actual:%v", tt.wantErrMsg, err.Error())
-			} else {
-				require.NoError(t, err)
-			}
+			tt.client.DeleteEndpointsImpl(tt.ep, tt.routesLeft)
 		})
 	}
+
+	t.Run("Delete endpoint runs even if delete routes fails", func(t *testing.T) {
+		nl := netlink.NewMockNetlink(true, "netlink failure")
+		// count number of times delete and link and set route are called
+		// even if deleting the routes fail, we should still delete the veth pair in the vnet ns
+		deleteLinkFlag := 0
+		nl.DeleteLinkFn = func(_ string) error {
+			deleteLinkFlag++
+			return errors.New("err mock")
+		}
+		errOnDeleteRouteFlag := 0
+		nl.SetDeleteRouteValidationFn(func(_ *netlink.Route) error {
+			errOnDeleteRouteFlag++
+			return errors.New("err mock")
+		})
+
+		client := TransparentVlanEndpointClient{
+			primaryHostIfName: "eth0",
+			vlanIfName:        "eth0.1",
+			vnetVethName:      "A1veth0",
+			containerVethName: "B1veth0",
+			vnetNSName:        "az_ns_1",
+			netnsClient: &mockNetns{
+				deleteNamed: defaultDeleteNamed,
+			},
+			netlink:        nl,
+			plClient:       platform.NewMockExecClient(false),
+			netUtilsClient: networkutils.NewNetworkUtils(nl, plc),
+			netioshim:      netio.NewMockNetIO(false, 0),
+		}
+		ep := &endpoint{
+			IPAddresses: IPAddresses,
+		}
+		client.DeleteEndpointsImpl(ep, func() (int, error) { return 0, nil })
+
+		require.Equal(t, 1, errOnDeleteRouteFlag, "error must occur during delete route path")
+		require.Equal(t, 1, deleteLinkFlag, "delete link must still be called")
+	})
 }
 
 func TestTransparentVlanConfigureContainerInterfacesAndRoutes(t *testing.T) {