fix: return error on interface not found and always clean up snat ep (#3226)

* return error on interface not found and always clean up snat ep

* modify snat client to accept interfaces for unit testing

* address linter issues

Author: QxBytes

network/endpoint.go

@@ -151,7 +151,7 @@ type apipaClient interface {
 }
 
 func (epInfo *EndpointInfo) PrettyString() string {
-	return fmt.Sprintf("Id:%s ContainerID:%s NetNsPath:%s IfName:%s IfIndex:%d MacAddr:%s IPAddrs:%v Gateways:%v Data:%+v NICType: %s NetworkContainerID: %s HostIfName: %s NetNs: %s Options: %v",
+	return fmt.Sprintf("EndpointID:%s ContainerID:%s NetNsPath:%s IfName:%s IfIndex:%d MacAddr:%s IPAddrs:%v Gateways:%v Data:%+v NICType: %s NetworkContainerID: %s HostIfName: %s NetNs: %s Options: %v",
 		epInfo.EndpointID, epInfo.ContainerID, epInfo.NetNsPath, epInfo.IfName, epInfo.IfIndex, epInfo.MacAddress.String(), epInfo.IPAddresses,
 		epInfo.Gateways, epInfo.Data, epInfo.NICType, epInfo.NetworkContainerID, epInfo.HostIfName, epInfo.NetNs, epInfo.Options)
 }

network/endpoint_linux.go

@@ -279,7 +279,6 @@ func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform.
 			epInfo := ep.getInfo()
 			if nw.Mode == opModeTransparentVlan {
 				epClient = NewTransparentVlanEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, plc, nsc, iptc)
-
 			} else {
 				epClient = NewOVSEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, ovsctl.NewOvsctl(), plc, iptc)
 			}

network/manager.go

@@ -439,6 +439,7 @@ func (nm *networkManager) UpdateEndpointState(eps []*endpoint) error {
 	logger.Info("Update endpoint API returend ", zap.String("podname: ", response.ReturnCode.String()))
 	return nil
 }
+
 func validateUpdateEndpointState(endpointID string, ifNameToIPInfoMap map[string]*restserver.IPInfo) error {
 	if endpointID == "" {
 		return errors.New("endpoint id empty while validating update endpoint state")

network/ovs_endpoint_snatroute_linux.go

@@ -33,6 +33,7 @@ func (client *OVSEndpointClient) NewSnatClient(snatBridgeIP, localIP string, epI
 			client.netlink,
 			client.plClient,
 			client.iptablesClient,
+			client.netioshim,
 		)
 	}
 }

network/snat/snat_linux.go

@@ -11,6 +11,7 @@ import (
 	"github.com/Azure/azure-container-networking/cni/log"
 	"github.com/Azure/azure-container-networking/ebtables"
 	"github.com/Azure/azure-container-networking/iptables"
+	"github.com/Azure/azure-container-networking/netio"
 	"github.com/Azure/azure-container-networking/netlink"
 	"github.com/Azure/azure-container-networking/network/networkutils"
 	"github.com/Azure/azure-container-networking/platform"
@@ -55,6 +56,7 @@ type Client struct {
 	netlink                netlink.NetlinkInterface
 	plClient               platform.ExecClient
 	ipTablesClient         ipTablesClient
+	netioClient            netio.NetIOInterface
 }
 
 func NewSnatClient(hostIfName string,
@@ -67,6 +69,7 @@ func NewSnatClient(hostIfName string,
 	nl netlink.NetlinkInterface,
 	plClient platform.ExecClient,
 	iptc ipTablesClient,
+	nio netio.NetIOInterface,
 ) Client {
 	snatClient := Client{
 		hostSnatVethName:       hostIfName,
@@ -78,6 +81,7 @@ func NewSnatClient(hostIfName string,
 		netlink:                nl,
 		plClient:               plClient,
 		ipTablesClient:         iptc,
+		netioClient:            nio,
 	}
 
 	snatClient.SkipAddressesFromBlock = append(snatClient.SkipAddressesFromBlock, skipAddressesFromBlock...)
@@ -223,7 +227,11 @@ func (client *Client) AllowInboundFromHostToNC() error {
 		return newErrorSnatClient(err.Error())
 	}
 
-	snatContainerVeth, _ := net.InterfaceByName(client.containerSnatVethName)
+	snatContainerVeth, err := client.netioClient.GetNetworkInterfaceByName(client.containerSnatVethName)
+	if err != nil {
+		logger.Info("Could not find interface", zap.String("containerSnatVethName", client.containerSnatVethName))
+		return errors.Wrap(newErrorSnatClient(err.Error()), "could not find container snat veth name for allow host to nc")
+	}
 
 	// Add static arp entry for localIP to prevent arp going out of VM
 	logger.Info("Adding static arp entry for ip", zap.Any("containerIP", containerIP),
@@ -319,7 +327,11 @@ func (client *Client) AllowInboundFromNCToHost() error {
 		return err
 	}
 
-	snatContainerVeth, _ := net.InterfaceByName(client.containerSnatVethName)
+	snatContainerVeth, err := client.netioClient.GetNetworkInterfaceByName(client.containerSnatVethName)
+	if err != nil {
+		logger.Info("Could not find interface", zap.String("containerSnatVethName", client.containerSnatVethName))
+		return errors.Wrap(newErrorSnatClient(err.Error()), "could not find container snat veth name for allow nc to host")
+	}
 
 	// Add static arp entry for localIP to prevent arp going out of VM
 	logger.Info("Adding static arp entry for ip", zap.Any("containerIP", containerIP), zap.String("HardwareAddr", snatContainerVeth.HardwareAddr.String()))
@@ -416,7 +428,7 @@ func (client *Client) DropArpForSnatBridgeApipaRange(snatBridgeIP, azSnatVethIfN
 
 // This function creates linux bridge which will be used for outbound connectivity by NCs
 func (client *Client) createSnatBridge(snatBridgeIP, hostPrimaryMac string) error {
-	_, err := net.InterfaceByName(SnatBridgeName)
+	_, err := client.netioClient.GetNetworkInterfaceByName(SnatBridgeName)
 	if err == nil {
 		logger.Info("Snat Bridge already exists")
 	} else {

network/snat/snat_linux_test.go

@@ -4,12 +4,30 @@ import (
 	"os"
 	"testing"
 
-	"github.com/Azure/azure-container-networking/iptables"
+	"github.com/Azure/azure-container-networking/netio"
 	"github.com/Azure/azure-container-networking/netlink"
 )
 
 var anyInterface = "dummy"
 
+type mockIPTablesClient struct{}
+
+func (c mockIPTablesClient) InsertIptableRule(_, _, _, _, _ string) error {
+	return nil
+}
+
+func (c mockIPTablesClient) AppendIptableRule(_, _, _, _, _ string) error {
+	return nil
+}
+
+func (c mockIPTablesClient) DeleteIptableRule(_, _, _, _, _ string) error {
+	return nil
+}
+
+func (c mockIPTablesClient) CreateChain(_, _, _ string) error {
+	return nil
+}
+
 func TestMain(m *testing.M) {
 	exitCode := m.Run()
 
@@ -18,16 +36,22 @@ func TestMain(m *testing.M) {
 	os.Exit(exitCode)
 }
 
-func TestAllowInboundFromHostToNC(t *testing.T) {
-	nl := netlink.NewNetlink()
-	iptc := iptables.NewClient()
-	client := &Client{
+func GetTestClient(nl netlink.NetlinkInterface, iptc ipTablesClient, nio netio.NetIOInterface) *Client {
+	return &Client{
 		SnatBridgeIP:          "169.254.0.1/16",
 		localIP:               "169.254.0.4/16",
 		containerSnatVethName: anyInterface,
 		netlink:               nl,
 		ipTablesClient:        iptc,
+		netioClient:           nio,
 	}
+}
+
+func TestAllowInboundFromHostToNC(t *testing.T) {
+	nl := netlink.NewMockNetlink(false, "")
+	iptc := &mockIPTablesClient{}
+	nio := netio.NewMockNetIO(false, 0)
+	client := GetTestClient(nl, iptc, nio)
 
 	if err := nl.AddLink(&netlink.DummyLink{
 		LinkInfo: netlink.LinkInfo{
@@ -65,18 +89,18 @@ func TestAllowInboundFromHostToNC(t *testing.T) {
 	if err := nl.DeleteLink(SnatBridgeName); err != nil {
 		t.Errorf("Error removing snat bridge: %v", err)
 	}
+
+	client.netioClient = netio.NewMockNetIO(true, 1)
+	if err := client.AllowInboundFromHostToNC(); err == nil {
+		t.Errorf("Expected error when interface not found in allow host to nc but got nil")
+	}
 }
 
 func TestAllowInboundFromNCToHost(t *testing.T) {
-	nl := netlink.NewNetlink()
-	iptc := iptables.NewClient()
-	client := &Client{
-		SnatBridgeIP:          "169.254.0.1/16",
-		localIP:               "169.254.0.4/16",
-		containerSnatVethName: anyInterface,
-		netlink:               nl,
-		ipTablesClient:        iptc,
-	}
+	nl := netlink.NewMockNetlink(false, "")
+	iptc := &mockIPTablesClient{}
+	nio := netio.NewMockNetIO(false, 0)
+	client := GetTestClient(nl, iptc, nio)
 
 	if err := nl.AddLink(&netlink.DummyLink{
 		LinkInfo: netlink.LinkInfo{
@@ -114,4 +138,9 @@ func TestAllowInboundFromNCToHost(t *testing.T) {
 	if err := nl.DeleteLink(SnatBridgeName); err != nil {
 		t.Errorf("Error removing snat bridge: %v", err)
 	}
+
+	client.netioClient = netio.NewMockNetIO(true, 1)
+	if err := client.AllowInboundFromNCToHost(); err == nil {
+		t.Errorf("Expected error when interface not found in allow nc to host but got nil")
+	}
 }

network/transparent_vlan_endpoint_snatroute_linux.go

@@ -21,6 +21,7 @@ func (client *TransparentVlanEndpointClient) NewSnatClient(snatBridgeIP, localIP
 			client.netlink,
 			client.plClient,
 			client.iptablesClient,
+			client.netioshim,
 		)
 	}
 }

network/transparent_vlan_endpointclient_linux.go

@@ -635,7 +635,7 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error
 		return client.DeleteEndpointsImpl(ep, getNumRoutesLeft)
 	})
 	if err != nil {
-		return err
+		logger.Warn("could not delete transparent vlan endpoints", zap.String("errorMsg", err.Error()))
 	}
 
 	// VM NS

network/transparent_vlan_endpointclient_linux_test.go

@@ -15,9 +15,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-var errNetnsMock = errors.New("mock netns error")
-var errMockNetIOFail = errors.New("netio fail")
-var errMockNetIONoIfFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: errors.New("no such network interface")}
+var (
+	errNetnsMock         = errors.New("mock netns error")
+	errMockNetIOFail     = errors.New("netio fail")
+	errMockNetIONoIfFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: errors.New("no such network interface")}
+)
 
 func newNetnsErrorMock(errStr string) error {
 	return errors.Wrap(errNetnsMock, errStr)