Delete host veth interface on endpoint creation error in transparent-…

…vlan mode (Azure#1892)

* deletion of host veth interface on error in transparent_vlan mode

* fixed a typo character

* Fix lint issues

* Windows lint fix

* Windows lint fix

* add netio import

* Fixed newly added ut

* reverting pre-push change

* fix a typo

network/bridge_endpointclient_linux.go

@@ -210,7 +210,7 @@ func (client *LinuxBridgeEndpointClient) ConfigureContainerInterfacesAndRoutes(e
 	return nil
 }
 
-func (client *LinuxBridgeEndpointClient) DeleteEndpoints(ep *endpoint) error {
+func (client *LinuxBridgeEndpointClient) DeleteEndpoints(ep *endpoint, _ bool) error {
 	log.Printf("[net] Deleting veth pair %v %v.", ep.HostIfName, ep.IfName)
 	err := client.netlink.DeleteLink(ep.HostIfName)
 	if err != nil {

network/endpoint.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"github.com/Azure/azure-container-networking/log"
+	"github.com/Azure/azure-container-networking/netio"
 	"github.com/Azure/azure-container-networking/netlink"
 	"github.com/Azure/azure-container-networking/network/policy"
 	"github.com/Azure/azure-container-networking/platform"
@@ -108,7 +109,13 @@ func (epInfo *EndpointInfo) PrettyString() string {
 }
 
 // NewEndpoint creates a new endpoint in the network.
-func (nw *network) newEndpoint(cli apipaClient, nl netlink.NetlinkInterface, plc platform.ExecClient, epInfo *EndpointInfo) (*endpoint, error) {
+func (nw *network) newEndpoint(
+	apipaCli apipaClient,
+	nl netlink.NetlinkInterface,
+	plc platform.ExecClient,
+	netioCli netio.NetIOInterface,
+	epInfo *EndpointInfo,
+) (*endpoint, error) {
 	var ep *endpoint
 	var err error
 
@@ -119,7 +126,8 @@ func (nw *network) newEndpoint(cli apipaClient, nl netlink.NetlinkInterface, plc
 	}()
 
 	// Call the platform implementation.
-	ep, err = nw.newEndpointImpl(cli, nl, plc, epInfo)
+	// Pass nil for epClient and will be initialized in newendpointImpl
+	ep, err = nw.newEndpointImpl(apipaCli, nl, plc, netioCli, nil, epInfo)
 	if err != nil {
 		return nil, err
 	}
@@ -149,7 +157,8 @@ func (nw *network) deleteEndpoint(nl netlink.NetlinkInterface, plc platform.Exec
 	}
 
 	// Call the platform implementation.
-	err = nw.deleteEndpointImpl(nl, plc, ep)
+	// Pass nil for epClient and will be initialized in deleteEndpointImpl
+	err = nw.deleteEndpointImpl(nl, plc, nil, ep)
 	if err != nil {
 		return err
 	}

network/endpoint_linux.go

@@ -48,15 +48,21 @@ func ConstructEndpointID(containerID string, _ string, ifName string) (string, s
 }
 
 // newEndpointImpl creates a new endpoint in the network.
-func (nw *network) newEndpointImpl(_ apipaClient, nl netlink.NetlinkInterface, plc platform.ExecClient, epInfo *EndpointInfo) (*endpoint, error) {
+func (nw *network) newEndpointImpl(
+	_ apipaClient,
+	nl netlink.NetlinkInterface,
+	plc platform.ExecClient,
+	netioCli netio.NetIOInterface,
+	epClient EndpointClient,
+	epInfo *EndpointInfo,
+) (*endpoint, error) {
 	var containerIf *net.Interface
 	var ns *Namespace
 	var ep *endpoint
 	var err error
 	var hostIfName string
 	var contIfName string
 	var localIP string
-	var epClient EndpointClient
 	var vlanid int = 0
 
 	if nw.Endpoints[epInfo.Id] != nil {
@@ -88,49 +94,52 @@ func (nw *network) newEndpointImpl(_ apipaClient, nl netlink.NetlinkInterface, p
 		contIfName = fmt.Sprintf("%s%s-2", hostVEthInterfacePrefix, epInfo.Id[:7])
 	}
 
-	if vlanid != 0 {
-		if nw.Mode == opModeTransparentVlan {
-			log.Printf("Transparent vlan client")
-			if _, ok := epInfo.Data[SnatBridgeIPKey]; ok {
-				nw.SnatBridgeIP = epInfo.Data[SnatBridgeIPKey].(string)
+	// epClient is non-nil only when the endpoint is created for the unit test.
+	if epClient == nil {
+		//nolint:gocritic
+		if vlanid != 0 {
+			if nw.Mode == opModeTransparentVlan {
+				log.Printf("Transparent vlan client")
+				if _, ok := epInfo.Data[SnatBridgeIPKey]; ok {
+					nw.SnatBridgeIP = epInfo.Data[SnatBridgeIPKey].(string)
+				}
+				epClient = NewTransparentVlanEndpointClient(nw, epInfo, hostIfName, contIfName, vlanid, localIP, nl, plc)
+			} else {
+				log.Printf("OVS client")
+				if _, ok := epInfo.Data[SnatBridgeIPKey]; ok {
+					nw.SnatBridgeIP = epInfo.Data[SnatBridgeIPKey].(string)
+				}
+
+				epClient = NewOVSEndpointClient(
+					nw,
+					epInfo,
+					hostIfName,
+					contIfName,
+					vlanid,
+					localIP,
+					nl,
+					ovsctl.NewOvsctl(),
+					plc)
 			}
-			epClient = NewTransparentVlanEndpointClient(nw, epInfo, hostIfName, contIfName, vlanid, localIP, nl, plc)
+		} else if nw.Mode != opModeTransparent {
+			log.Printf("Bridge client")
+			epClient = NewLinuxBridgeEndpointClient(nw.extIf, hostIfName, contIfName, nw.Mode, nl, plc)
 		} else {
-			log.Printf("OVS client")
-			if _, ok := epInfo.Data[SnatBridgeIPKey]; ok {
-				nw.SnatBridgeIP = epInfo.Data[SnatBridgeIPKey].(string)
-			}
-
-			epClient = NewOVSEndpointClient(
-				nw,
-				epInfo,
-				hostIfName,
-				contIfName,
-				vlanid,
-				localIP,
-				nl,
-				ovsctl.NewOvsctl(),
-				plc)
+			log.Printf("Transparent client")
+			epClient = NewTransparentEndpointClient(nw.extIf, hostIfName, contIfName, nw.Mode, nl, plc)
 		}
-	} else if nw.Mode != opModeTransparent {
-		log.Printf("Bridge client")
-		epClient = NewLinuxBridgeEndpointClient(nw.extIf, hostIfName, contIfName, nw.Mode, nl, plc)
-	} else {
-		log.Printf("Transparent client")
-		epClient = NewTransparentEndpointClient(nw.extIf, hostIfName, contIfName, nw.Mode, nl, plc)
 	}
 
 	// Cleanup on failure.
 	defer func() {
 		if err != nil {
-			log.Printf("CNI error. Delete Endpoint %v and rules that are created.", contIfName)
+			log.Printf("CNI error:%v. Delete Endpoint %v and rules that are created.", err, contIfName)
 			endpt := &endpoint{
 				Id:                       epInfo.Id,
 				IfName:                   contIfName,
 				HostIfName:               hostIfName,
 				LocalIP:                  localIP,
 				IPAddresses:              epInfo.IPAddresses,
-				Gateways:                 []net.IP{nw.extIf.IPv4Gateway},
 				DNS:                      epInfo.DNS,
 				VlanID:                   vlanid,
 				EnableSnatOnHost:         epInfo.EnableSnatOnHost,
@@ -144,15 +153,20 @@ func (nw *network) newEndpointImpl(_ apipaClient, nl netlink.NetlinkInterface, p
 				epClient.DeleteEndpointRules(endpt)
 			}
 
-			epClient.DeleteEndpoints(endpt)
+			if nw.extIf != nil {
+				endpt.Gateways = []net.IP{nw.extIf.IPv4Gateway}
+			}
+			// set deleteHostVeth to true to cleanup host veth interface if created
+			//nolint:errcheck // ignore error
+			epClient.DeleteEndpoints(endpt, true)
 		}
 	}()
 
 	if err = epClient.AddEndpoints(epInfo); err != nil {
 		return nil, err
 	}
 
-	containerIf, err = net.InterfaceByName(contIfName)
+	containerIf, err = netioCli.GetNetworkInterfaceByName(contIfName)
 	if err != nil {
 		return nil, err
 	}
@@ -220,7 +234,6 @@ func (nw *network) newEndpointImpl(_ apipaClient, nl netlink.NetlinkInterface, p
 		InfraVnetIP:              epInfo.InfraVnetIP,
 		LocalIP:                  localIP,
 		IPAddresses:              epInfo.IPAddresses,
-		Gateways:                 []net.IP{nw.extIf.IPv4Gateway},
 		DNS:                      epInfo.DNS,
 		VlanID:                   vlanid,
 		EnableSnatOnHost:         epInfo.EnableSnatOnHost,
@@ -234,34 +247,44 @@ func (nw *network) newEndpointImpl(_ apipaClient, nl netlink.NetlinkInterface, p
 		PODNameSpace:             epInfo.PODNameSpace,
 	}
 
+	if nw.extIf != nil {
+		ep.Gateways = []net.IP{nw.extIf.IPv4Gateway}
+	}
+
 	ep.Routes = append(ep.Routes, epInfo.Routes...)
 	return ep, nil
 }
 
 // deleteEndpointImpl deletes an existing endpoint from the network.
-func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform.ExecClient, ep *endpoint) error {
-	var epClient EndpointClient
-
+func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform.ExecClient, epClient EndpointClient, ep *endpoint) error {
 	// Delete the veth pair by deleting one of the peer interfaces.
 	// Deleting the host interface is more convenient since it does not require
 	// entering the container netns and hence works both for CNI and CNM.
-	if ep.VlanID != 0 {
-		epInfo := ep.getInfo()
-		if nw.Mode == opModeTransparentVlan {
-			log.Printf("Transparent vlan client")
-			epClient = NewTransparentVlanEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, plc)
 
+	// epClient is nil only for unit test.
+	if epClient == nil {
+		//nolint:gocritic
+		if ep.VlanID != 0 {
+			epInfo := ep.getInfo()
+			if nw.Mode == opModeTransparentVlan {
+				log.Printf("Transparent vlan client")
+				epClient = NewTransparentVlanEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, plc)
+
+			} else {
+				epClient = NewOVSEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, ovsctl.NewOvsctl(), plc)
+			}
+		} else if nw.Mode != opModeTransparent {
+			epClient = NewLinuxBridgeEndpointClient(nw.extIf, ep.HostIfName, "", nw.Mode, nl, plc)
 		} else {
-			epClient = NewOVSEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, ovsctl.NewOvsctl(), plc)
+			epClient = NewTransparentEndpointClient(nw.extIf, ep.HostIfName, "", nw.Mode, nl, plc)
 		}
-	} else if nw.Mode != opModeTransparent {
-		epClient = NewLinuxBridgeEndpointClient(nw.extIf, ep.HostIfName, "", nw.Mode, nl, plc)
-	} else {
-		epClient = NewTransparentEndpointClient(nw.extIf, ep.HostIfName, "", nw.Mode, nl, plc)
 	}
 
 	epClient.DeleteEndpointRules(ep)
-	epClient.DeleteEndpoints(ep)
+	// deleteHostVeth set to false not to delete veth as CRI will remove network namespace and
+	// veth will get removed as part of that.
+	//nolint:errcheck // ignore error
+	epClient.DeleteEndpoints(ep, false)
 
 	return nil
 }

network/endpoint_snatroute_linux.go

@@ -57,7 +57,10 @@ func AddSnatEndpointRules(snatClient *snat.Client, hostToNC, ncToHost bool, nl n
 
 func MoveSnatEndpointToContainerNS(snatClient *snat.Client, netnsPath string, nsID uintptr) error {
 	if err := snatClient.MoveSnatEndpointToContainerNS(netnsPath, nsID); err != nil {
-		return errors.Wrap(err, "failed to move snat endpoint to container ns")
+		if delErr := snatClient.DeleteSnatEndpoint(); delErr != nil {
+			log.Errorf("failed to delete snat endpoint on error(moving to container ns): %v", delErr)
+		}
+		return errors.Wrap(err, "failed to move snat endpoint to container ns. deleted snat endpoint")
 	}
 	return nil
 }

network/endpoint_test.go

@@ -4,10 +4,15 @@
 package network
 
 import (
+	"net"
 	"testing"
 
+	"github.com/Azure/azure-container-networking/netio"
+	"github.com/Azure/azure-container-networking/netlink"
+	"github.com/Azure/azure-container-networking/platform"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestEndpoint(t *testing.T) {
@@ -162,14 +167,101 @@ var _ = Describe("Test Endpoint", func() {
 		})
 	})
 
+	Describe("Test endpointImpl", func() {
+		Context("When endpoint add/delete succeed", func() {
+			nw := &network{
+				Endpoints: map[string]*endpoint{},
+			}
+			epInfo := &EndpointInfo{
+				Id:   "768e8deb-eth1",
+				Data: make(map[string]interface{}),
+			}
+			epInfo.Data[VlanIDKey] = 100
+
+			It("Should be added", func() {
+				// Add endpoint with valid id
+				ep, err := nw.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), NewMockEndpointClient(false), epInfo)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(ep).NotTo(BeNil())
+				Expect(ep.Id).To(Equal(epInfo.Id))
+				Expect(ep.Gateways).To(BeEmpty())
+			})
+			It("should have fields set", func() {
+				nw2 := &network{
+					Endpoints: map[string]*endpoint{},
+					extIf:     &externalInterface{IPv4Gateway: net.ParseIP("192.168.0.1")},
+				}
+				ep, err := nw2.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), NewMockEndpointClient(false), epInfo)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(ep).NotTo(BeNil())
+				Expect(ep.Id).To(Equal(epInfo.Id))
+				Expect(ep.Gateways).NotTo(BeNil())
+				Expect(len(ep.Gateways)).To(Equal(1))
+				Expect(ep.Gateways[0].String()).To(Equal("192.168.0.1"))
+				Expect(ep.VlanID).To(Equal(epInfo.Data[VlanIDKey].(int)))
+			})
+			It("Should be not added", func() {
+				// Adding an endpoint with an id.
+				mockCli := NewMockEndpointClient(false)
+				err := mockCli.AddEndpoints(epInfo)
+				Expect(err).ToNot(HaveOccurred())
+				// Adding endpoint with same id should fail and delete should cleanup the state
+				ep2, err := nw.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), mockCli, epInfo)
+				Expect(err).To(HaveOccurred())
+				Expect(ep2).To(BeNil())
+				assert.Contains(GinkgoT(), err.Error(), "Endpoint already exists")
+				Expect(len(mockCli.endpoints)).To(Equal(0))
+			})
+			It("Should be deleted", func() {
+				// Adding an endpoint with an id.
+				mockCli := NewMockEndpointClient(false)
+				ep2, err := nw.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), mockCli, epInfo)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(ep2).ToNot(BeNil())
+				Expect(len(mockCli.endpoints)).To(Equal(1))
+				// Deleting the endpoint
+				//nolint:errcheck // ignore error
+				nw.deleteEndpointImpl(netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false), mockCli, ep2)
+				Expect(len(mockCli.endpoints)).To(Equal(0))
+				// Deleting same endpoint with same id should not fail
+				//nolint:errcheck // ignore error
+				nw.deleteEndpointImpl(netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false), mockCli, ep2)
+				Expect(len(mockCli.endpoints)).To(Equal(0))
+			})
+		})
+		Context("When endpoint add failed", func() {
+			It("Should not be added to the network", func() {
+				nw := &network{
+					Endpoints: map[string]*endpoint{},
+				}
+				epInfo := &EndpointInfo{
+					Id: "768e8deb-eth1",
+				}
+				ep, err := nw.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), NewMockEndpointClient(true), epInfo)
+				Expect(err).To(HaveOccurred())
+				Expect(ep).To(BeNil())
+				ep, err = nw.newEndpointImpl(nil, netlink.NewMockNetlink(false, ""), platform.NewMockExecClient(false),
+					netio.NewMockNetIO(false, 0), NewMockEndpointClient(false), epInfo)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(ep).NotTo(BeNil())
+				Expect(ep.Id).To(Equal(epInfo.Id))
+			})
+		})
+	})
+
 	Describe("Test updateEndpoint", func() {
 		Context("When endpoint not found", func() {
 			It("Should raise errEndpointNotFound", func() {
 				nm := &networkManager{}
 
 				nw := &network{}
 				existingEpInfo := &EndpointInfo{
-					Id: "test",
+					Id: "768e8deb-eth1",
 				}
 				targetEpInfo := &EndpointInfo{}
 				err := nm.updateEndpoint(nw, existingEpInfo, targetEpInfo)