Set constant mac for host veth interface in transparent vlan mode (#1906)

* set constant mac for host veth interface

* fixed a race issue in transparent-vlan where delete can happen after add and removes route add by ADD call

* moved log to place where its executed

* enable proxy arp on bridge to allow public connectivity from apipa interface

* validate newly created namespace is not same as host namespace

* addressed comments and added UTs

* fixed cni delete call for linux multitenancy

* lint fixes

* windows lint fixes

* lint fixes

* fix issues with network namespace creation and vlan interface creation

* Removed deletehostveth flag and delete host veth on delete endpoint trigger

* lint fix

* address comment

Author: tamilmani1989

cni/network/network.go

@@ -466,10 +466,11 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 
 	// iterate ipamAddResults and program the endpoint
 	for i := 0; i < len(ipamAddResults); i++ {
+		var networkID string
 		ipamAddResult = ipamAddResults[i]
 
 		options := make(map[string]any)
-		networkID, err := plugin.getNetworkName(args.Netns, &ipamAddResult, nwCfg)
+		networkID, err = plugin.getNetworkName(args.Netns, &ipamAddResult, nwCfg)
 
 		endpointID := GetEndpointID(args)
 		policies := cni.GetPoliciesFromNwCfg(nwCfg.AdditionalArgs)
@@ -557,7 +558,8 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 			natInfo:          natInfo,
 		}
 
-		epInfo, err := plugin.createEndpointInternal(&createEndpointInternalOpt)
+		var epInfo network.EndpointInfo
+		epInfo, err = plugin.createEndpointInternal(&createEndpointInternalOpt)
 		if err != nil {
 			log.Errorf("Endpoint creation failed:%w", err)
 			return err
@@ -953,7 +955,7 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 	numEndpointsToDelete := 1
 	// only get number of endpoints if it's multitenancy mode
 	if nwCfg.MultiTenancy {
-		numEndpointsToDelete = plugin.nm.GetNumEndpointsInNetNs(args.Netns)
+		numEndpointsToDelete = plugin.nm.GetNumEndpointsByContainerID(args.ContainerID)
 	}
 
 	log.Printf("[cni-net] number of endpoints to be deleted %d", numEndpointsToDelete)
@@ -973,23 +975,21 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 		}
 		// Query the network.
 		if nwInfo, err = plugin.nm.GetNetworkInfo(networkID); err != nil {
-			if !nwCfg.MultiTenancy {
-				log.Printf("[cni-net] Failed to query network:%s: %v", networkID, err)
-				// Log the error but return success if the network is not found.
-				// if cni hits this, mostly state file would be missing and it can be reboot scenario where
-				// container runtime tries to delete and create pods which existed before reboot.
-				err = nil
-				return err
-			}
+			// Log the error but return success if the network is not found.
+			// if cni hits this, mostly state file would be missing and it can be reboot scenario where
+			// container runtime tries to delete and create pods which existed before reboot.
+			log.Printf("[cni-net] Failed to query network:%s: %v", networkID, err)
+			err = nil
+			return err
 		}
 
 		endpointID := GetEndpointID(args)
 		// Query the endpoint.
 		if epInfo, err = plugin.nm.GetEndpointInfo(networkID, endpointID); err != nil {
+			log.Printf("[cni-net] GetEndpoint for endpointID: %s returns: %v", endpointID, err)
 			if !nwCfg.MultiTenancy {
 				// attempt to release address associated with this Endpoint id
 				// This is to ensure clean up is done even in failure cases
-				log.Printf("[cni-net] Failed to query endpoint %s: %v", endpointID, err)
 				logAndSendEvent(plugin, fmt.Sprintf("Release ip by ContainerID (endpoint not found):%v", args.ContainerID))
 				if err = plugin.ipamInvoker.Delete(nil, nwCfg, args, nwInfo.Options); err != nil {
 					return plugin.RetriableError(fmt.Errorf("failed to release address(no endpoint): %w", err))

netio/mocknetio.go

@@ -6,10 +6,13 @@ import (
 	"net"
 )
 
+type getInterfaceValidationFn func(name string) (*net.Interface, error)
+
 type MockNetIO struct {
 	fail           bool
 	failAttempt    int
 	numTimesCalled int
+	getInterfaceFn getInterfaceValidationFn
 }
 
 // ErrMockNetIOFail - mock netio error
@@ -22,13 +25,21 @@ func NewMockNetIO(fail bool, failAttempt int) *MockNetIO {
 	}
 }
 
+func (netshim *MockNetIO) SetGetInterfaceValidatonFn(fn getInterfaceValidationFn) {
+	netshim.getInterfaceFn = fn
+}
+
 func (netshim *MockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, error) {
 	netshim.numTimesCalled++
 
 	if netshim.fail && netshim.failAttempt == netshim.numTimesCalled {
 		return nil, fmt.Errorf("%w:%s", ErrMockNetIOFail, name)
 	}
 
+	if netshim.getInterfaceFn != nil {
+		return netshim.getInterfaceFn(name)
+	}
+
 	hwAddr, _ := net.ParseMAC("ab:cd:ef:12:34:56")
 
 	return &net.Interface{

netlink/mocknetlink.go

@@ -13,9 +13,13 @@ func newErrorMockNetlink(errStr string) error {
 	return fmt.Errorf("%w : %s", ErrorMockNetlink, errStr)
 }
 
+type routeValidateFn func(route *Route) error
+
 type MockNetlink struct {
-	returnError bool
-	errorString string
+	returnError   bool
+	errorString   string
+	deleteRouteFn routeValidateFn
+	addRouteFn    routeValidateFn
 }
 
 func NewMockNetlink(returnError bool, errorString string) *MockNetlink {
@@ -25,6 +29,14 @@ func NewMockNetlink(returnError bool, errorString string) *MockNetlink {
 	}
 }
 
+func (f *MockNetlink) SetDeleteRouteValidationFn(fn routeValidateFn) {
+	f.deleteRouteFn = fn
+}
+
+func (f *MockNetlink) SetAddRouteValidationFn(fn routeValidateFn) {
+	f.addRouteFn = fn
+}
+
 func (f *MockNetlink) error() error {
 	if f.returnError {
 		return newErrorMockNetlink(f.errorString)
@@ -88,10 +100,16 @@ func (f *MockNetlink) GetIPRoute(*Route) ([]*Route, error) {
 	return nil, f.error()
 }
 
-func (f *MockNetlink) AddIPRoute(*Route) error {
+func (f *MockNetlink) AddIPRoute(r *Route) error {
+	if f.addRouteFn != nil {
+		return f.addRouteFn(r)
+	}
 	return f.error()
 }
 
-func (f *MockNetlink) DeleteIPRoute(*Route) error {
+func (f *MockNetlink) DeleteIPRoute(r *Route) error {
+	if f.deleteRouteFn != nil {
+		return f.deleteRouteFn(r)
+	}
 	return f.error()
 }

netns/netns.go

@@ -36,3 +36,11 @@ func (f *Netns) NewNamed(name string) (int, error) {
 func (f *Netns) DeleteNamed(name string) error {
 	return errors.Wrap(netns.DeleteNamed(name), "netns impl")
 }
+
+func (f *Netns) IsNamespaceEqual(fd1, fd2 int) bool {
+	return netns.NsHandle(fd1).Equal(netns.NsHandle(fd2))
+}
+
+func (f *Netns) NamespaceUniqueID(fd int) string {
+	return netns.NsHandle(fd).UniqueId()
+}

network/bridge_endpointclient_linux.go

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

network/endpoint_linux.go

@@ -158,7 +158,7 @@ func (nw *network) newEndpointImpl(
 			}
 			// set deleteHostVeth to true to cleanup host veth interface if created
 			//nolint:errcheck // ignore error
-			epClient.DeleteEndpoints(endpt, true)
+			epClient.DeleteEndpoints(endpt)
 		}
 	}()
 
@@ -284,7 +284,7 @@ func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform.
 	// 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)
+	epClient.DeleteEndpoints(ep)
 
 	return nil
 }
@@ -297,8 +297,6 @@ func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, inte
 	ifIndex := 0
 
 	for _, route := range routes {
-		log.Printf("[net] Adding IP route %+v to link %v.", route, interfaceName)
-
 		if route.DevName != "" {
 			devIf, _ := netioshim.GetNetworkInterfaceByName(route.DevName)
 			ifIndex = devIf.Index
@@ -327,6 +325,7 @@ func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, inte
 			Table:     route.Table,
 		}
 
+		log.Printf("[net] Adding IP route %+v to link %v.", route, interfaceName)
 		if err := nl.AddIPRoute(nlRoute); err != nil {
 			if !strings.Contains(strings.ToLower(err.Error()), "file exists") {
 				return err
@@ -343,8 +342,6 @@ func deleteRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, i
 	ifIndex := 0
 
 	for _, route := range routes {
-		log.Printf("[net] Deleting IP route %+v from link %v.", route, interfaceName)
-
 		if route.DevName != "" {
 			devIf, _ := netioshim.GetNetworkInterfaceByName(route.DevName)
 			if devIf == nil {
@@ -353,15 +350,15 @@ func deleteRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, i
 			}
 
 			ifIndex = devIf.Index
-		} else {
+		} else if interfaceName != "" {
 			interfaceIf, _ := netioshim.GetNetworkInterfaceByName(interfaceName)
 			if interfaceIf == nil {
 				log.Printf("[net] Not deleting route. Interface %v doesn't exist", interfaceName)
 				continue
 			}
-
 			ifIndex = interfaceIf.Index
 		}
+
 		family := netlink.GetIPAddressFamily(route.Gw)
 		if route.Gw == nil {
 			family = netlink.GetIPAddressFamily(route.Dst.IP)
@@ -370,12 +367,13 @@ func deleteRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, i
 		nlRoute := &netlink.Route{
 			Family:    family,
 			Dst:       &route.Dst,
-			Gw:        route.Gw,
 			LinkIndex: ifIndex,
+			Gw:        route.Gw,
 			Protocol:  route.Protocol,
 			Scope:     route.Scope,
 		}
 
+		log.Printf("[net] Deleting IP route %+v from link %v.", route, interfaceName)
 		if err := nl.DeleteIPRoute(nlRoute); err != nil {
 			return err
 		}

network/endpoint_linux_test.go

@@ -0,0 +1,137 @@
+package network
+
+import (
+	"net"
+	"testing"
+
+	"github.com/Azure/azure-container-networking/netio"
+	"github.com/Azure/azure-container-networking/netlink"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
+)
+
+func TestEndpointLinux(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Endpoint Suite")
+}
+
+var _ = Describe("Test TestEndpointLinux", func() {
+	Describe("Test deleteRoutes", func() {
+		_, dst, _ := net.ParseCIDR("192.168.0.0/16")
+
+		It("DeleteRoute with interfacename explicit", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetDeleteRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r.LinkIndex).To(Equal(5))
+				return nil
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal("eth0"))
+				return &net.Interface{
+					Index: 5,
+				}, nil
+			})
+
+			err := deleteRoutes(nlc, netiocl, "eth0", []RouteInfo{{Dst: *dst, DevName: ""}})
+			Expect(err).To(BeNil())
+		})
+		It("DeleteRoute with interfacename set in Route", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetDeleteRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r.LinkIndex).To(Equal(6))
+				return nil
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal("eth1"))
+				return &net.Interface{
+					Index: 6,
+				}, nil
+			})
+
+			err := deleteRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: "eth1"}})
+			Expect(err).To(BeNil())
+		})
+		It("DeleteRoute with no ifindex", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetDeleteRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r.LinkIndex).To(Equal(0))
+				return nil
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal("eth1"))
+				return &net.Interface{
+					Index: 6,
+				}, nil
+			})
+
+			err := deleteRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: ""}})
+			Expect(err).To(BeNil())
+		})
+	})
+	Describe("Test addRoutes", func() {
+		_, dst, _ := net.ParseCIDR("192.168.0.0/16")
+		It("AddRoute with interfacename explicit", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetAddRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r).NotTo(BeNil())
+				Expect(r.LinkIndex).To(Equal(5))
+				return nil
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal("eth0"))
+				return &net.Interface{
+					Index: 5,
+				}, nil
+			})
+
+			err := addRoutes(nlc, netiocl, "eth0", []RouteInfo{{Dst: *dst, DevName: ""}})
+			Expect(err).To(BeNil())
+		})
+		It("AddRoute with interfacename set in route", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetAddRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r.LinkIndex).To(Equal(6))
+				return nil
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal("eth1"))
+				return &net.Interface{
+					Index: 6,
+				}, nil
+			})
+
+			err := addRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: "eth1"}})
+			Expect(err).To(BeNil())
+		})
+		It("AddRoute with interfacename not set should return error", func() {
+			nlc := netlink.NewMockNetlink(false, "")
+			nlc.SetAddRouteValidationFn(func(r *netlink.Route) error {
+				Expect(r.LinkIndex).To(Equal(0))
+				//nolint:goerr113 // for testing
+				return errors.New("Cannot add route")
+			})
+
+			netiocl := netio.NewMockNetIO(false, 0)
+			netiocl.SetGetInterfaceValidatonFn(func(ifName string) (*net.Interface, error) {
+				Expect(ifName).To(Equal(""))
+				return &net.Interface{
+					Index: 0,
+				}, errors.Wrapf(netio.ErrInterfaceNil, "Cannot get interface")
+			})
+
+			err := addRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: ""}})
+			Expect(err).ToNot(BeNil())
+		})
+	})
+})

network/endpoint_windows.go

@@ -157,6 +157,7 @@ func (nw *network) newEndpointImplHnsV1(epInfo *EndpointInfo) (*endpoint, error)
 		VlanID:           vlanid,
 		EnableSnatOnHost: epInfo.EnableSnatOnHost,
 		NetNs:            epInfo.NetNsPath,
+		ContainerID:      epInfo.ContainerID,
 	}
 
 	for _, route := range epInfo.Routes {