From 206170175b592e0714e28ec5dbc8231e72e7ea6a Mon Sep 17 00:00:00 2001 From: tamanoha Date: Mon, 10 Apr 2023 10:27:00 -0700 Subject: [PATCH 01/14] set constant mac for host veth interface --- network/transparent_vlan_endpointclient_linux.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 57b09e2597..f627644a1f 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -191,7 +191,14 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } client.vnetNSFileDescriptor = vnetNS - if err = client.netUtilsClient.CreateEndpoint(client.vnetVethName, client.containerVethName, nil); err != nil { + // Get the default constant host veth mac + mac, err := net.ParseMAC(defaultHostVethHwAddr) + if err != nil { + log.Printf("[net] Failed to parse the mac addrress %v", defaultHostVethHwAddr) + } + + // Create veth pair + if err = client.netUtilsClient.CreateEndpoint(client.vnetVethName, client.containerVethName, mac); err != nil { return errors.Wrap(err, "failed to create veth pair") } // Disable RA for veth pair, and delete if any failure From 62ab3b109faf2233b4e3fd7e8952b03572517730 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Mon, 10 Apr 2023 14:15:05 -0700 Subject: [PATCH 02/14] fixed a race issue in transparent-vlan where delete can happen after add and removes route add by ADD call --- network/bridge_endpointclient_linux.go | 4 ++-- network/endpoint_linux.go | 20 +++++++++++++++++-- network/network_linux.go | 6 +++--- network/ovs_endpointclient_linux.go | 2 +- network/transparent_endpointclient_linux.go | 8 ++++---- .../transparent_vlan_endpointclient_linux.go | 14 ++++++------- 6 files changed, 35 insertions(+), 19 deletions(-) diff --git a/network/bridge_endpointclient_linux.go b/network/bridge_endpointclient_linux.go index 4dc591db61..1cb3f4437f 100644 --- a/network/bridge_endpointclient_linux.go +++ b/network/bridge_endpointclient_linux.go @@ -195,7 +195,7 @@ func (client *LinuxBridgeEndpointClient) ConfigureContainerInterfacesAndRoutes(e return err } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes, false); err != nil { return err } @@ -288,7 +288,7 @@ func (client *LinuxBridgeEndpointClient) setupIPV6Routes(epInfo *EndpointInfo) e routes = append(routes, defaultV6Route) log.Printf("[net] Adding ipv6 routes in container %+v", routes) - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, routes); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, routes, false); err != nil { return nil } } diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index 92400ec8af..6dc2aa487a 100644 --- a/network/endpoint_linux.go +++ b/network/endpoint_linux.go @@ -270,7 +270,7 @@ func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform. func (ep *endpoint) getInfoImpl(epInfo *EndpointInfo) { } -func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, interfaceName string, routes []RouteInfo) error { +func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, interfaceName string, routes []RouteInfo, removeRouteBeforeAdd bool) error { ifIndex := 0 for _, route := range routes { @@ -304,6 +304,22 @@ func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, inte Table: route.Table, } + if removeRouteBeforeAdd { + delNlRoute := &netlink.Route{ + Family: family, + Dst: &route.Dst, + Gw: route.Gw, + Priority: route.Priority, + Protocol: route.Protocol, + Scope: route.Scope, + Table: route.Table, + } + log.Printf("[net] Deleting old IP route before add:%+v.", route) + if err := nl.DeleteIPRoute(delNlRoute); err != nil { + log.Printf("[net] Try to remove route if exists before add returns: %v", err) + } + } + if err := nl.AddIPRoute(nlRoute); err != nil { if !strings.Contains(strings.ToLower(err.Error()), "file exists") { return err @@ -483,7 +499,7 @@ func (nm *networkManager) updateRoutes(existingEp *EndpointInfo, targetEp *Endpo return err } - err = addRoutes(nm.netlink, &netio.NetIO{}, existingEp.IfName, tobeAddedRoutes) + err = addRoutes(nm.netlink, &netio.NetIO{}, existingEp.IfName, tobeAddedRoutes, false) if err != nil { return err } diff --git a/network/network_linux.go b/network/network_linux.go index 50070e66ad..24599c0310 100644 --- a/network/network_linux.go +++ b/network/network_linux.go @@ -118,7 +118,7 @@ func (nm *networkManager) newNetworkImpl(nwInfo *NetworkInfo, extIf *externalInt func (nm *networkManager) handleCommonOptions(ifName string, nwInfo *NetworkInfo) error { var err error if routes, exists := nwInfo.Options[RoutesKey]; exists { - err = addRoutes(nm.netlink, nm.netio, ifName, routes.([]RouteInfo)) + err = addRoutes(nm.netlink, nm.netio, ifName, routes.([]RouteInfo), false) if err != nil { return err } @@ -152,7 +152,7 @@ func (nm *networkManager) deleteNetworkImpl(nw *network) error { return nil } -// SaveIPConfig saves the IP configuration of an interface. +// SaveIPConfig saves the IP configuration of an interface. func (nm *networkManager) saveIPConfig(hostIf *net.Interface, extIf *externalInterface) error { // Save the default routes on the interface. routes, err := nm.netlink.GetIPRoute(&netlink.Route{Dst: &net.IPNet{}, LinkIndex: hostIf.Index}) @@ -649,7 +649,7 @@ func AddStaticRoute(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, gwIP := net.ParseIP("0.0.0.0") route := RouteInfo{Dst: *ipNet, Gw: gwIP} routes = append(routes, route) - if err := addRoutes(nl, netioshim, interfaceName, routes); err != nil { + if err := addRoutes(nl, netioshim, interfaceName, routes, false); err != nil { if err != nil && !strings.Contains(strings.ToLower(err.Error()), "file exists") { log.Printf("addroutes failed with error %v", err) return err diff --git a/network/ovs_endpointclient_linux.go b/network/ovs_endpointclient_linux.go index a3d0874ef6..305d0e8c61 100644 --- a/network/ovs_endpointclient_linux.go +++ b/network/ovs_endpointclient_linux.go @@ -230,7 +230,7 @@ func (client *OVSEndpointClient) ConfigureContainerInterfacesAndRoutes(epInfo *E return err } - return addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes) + return addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes, false) } func (client *OVSEndpointClient) DeleteEndpoints(ep *endpoint) error { diff --git a/network/transparent_endpointclient_linux.go b/network/transparent_endpointclient_linux.go index 82fe9bf51a..da47f48889 100644 --- a/network/transparent_endpointclient_linux.go +++ b/network/transparent_endpointclient_linux.go @@ -152,7 +152,7 @@ func (client *TransparentEndpointClient) AddEndpointRules(epInfo *EndpointInfo) log.Printf("[net] Adding route for the ip %v", ipNet.String()) routeInfo.Dst = ipNet routeInfoList = append(routeInfoList, routeInfo) - if err := addRoutes(client.netlink, client.netioshim, client.hostVethName, routeInfoList); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.hostVethName, routeInfoList, false); err != nil { return newErrorTransparentEndpointClient(err.Error()) } } @@ -234,7 +234,7 @@ func (client *TransparentEndpointClient) ConfigureContainerInterfacesAndRoutes(e Dst: *virtualGwNet, Scope: netlink.RT_SCOPE_LINK, } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}, false); err != nil { return newErrorTransparentEndpointClient(err.Error()) } @@ -245,7 +245,7 @@ func (client *TransparentEndpointClient) ConfigureContainerInterfacesAndRoutes(e Dst: dstIP, Gw: virtualGwIP, } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}, false); err != nil { return err } @@ -294,7 +294,7 @@ func (client *TransparentEndpointClient) setupIPV6Routes() error { Gw: virtualGwIP, } - return addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{gwRoute, defaultRoute}) + return addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{gwRoute, defaultRoute}, false) } func (client *TransparentEndpointClient) setIPV6NeighEntry() error { diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index f627644a1f..cd33653226 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -368,7 +368,7 @@ func (client *TransparentVlanEndpointClient) ConfigureContainerInterfacesAndRout } } - if err := client.AddDefaultRoutes(client.containerVethName, 0); err != nil { + if err := client.addDefaultRoutes(client.containerVethName, 0); err != nil { return errors.Wrap(err, "failed container ns add default routes") } if err := client.AddDefaultArp(client.containerVethName, client.vnetMac.String()); err != nil { @@ -386,16 +386,16 @@ func (client *TransparentVlanEndpointClient) ConfigureVnetInterfacesAndRoutesImp // Add route specifying which device the pod ip(s) are on routeInfoList := client.GetVnetRoutes(epInfo.IPAddresses) - if err = client.AddDefaultRoutes(client.vlanIfName, 0); err != nil { + if err = client.addDefaultRoutes(client.vlanIfName, 0); err != nil { return errors.Wrap(err, "failed vnet ns add default/gateway routes (idempotent)") } if err = client.AddDefaultArp(client.vlanIfName, azureMac); err != nil { return errors.Wrap(err, "failed vnet ns add default arp entry (idempotent)") } - if err = addRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList); err != nil { + if err = addRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList, true); err != nil { return errors.Wrap(err, "failed adding routes to vnet specific to this container") } - if err = client.AddDefaultRoutes(client.vlanIfName, tunnelingTable); err != nil { + if err = client.addDefaultRoutes(client.vlanIfName, tunnelingTable); err != nil { return errors.Wrap(err, "failed vnet ns add outbound routing table routes for tunneling (idempotent)") } // Return to ConfigureContainerInterfacesAndRoutes @@ -430,7 +430,7 @@ func (client *TransparentVlanEndpointClient) GetVnetRoutes(ipAddresses []net.IPN // to the virtual gateway ip on linkToName device interface // Route 1: 169.254.1.1 dev // Route 2: default via 169.254.1.1 dev -func (client *TransparentVlanEndpointClient) AddDefaultRoutes(linkToName string, table int) error { +func (client *TransparentVlanEndpointClient) addDefaultRoutes(linkToName string, table int) error { // Add route for virtualgwip (ip route add 169.254.1.1/32 dev eth0) virtualGwIP, virtualGwNet, _ := net.ParseCIDR(virtualGwIPString) routeInfo := RouteInfo{ @@ -439,7 +439,7 @@ func (client *TransparentVlanEndpointClient) AddDefaultRoutes(linkToName string, Table: table, } // Difference between interface name in addRoutes and DevName: in RouteInfo? - if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}); err != nil { + if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}, false); err != nil { return err } @@ -452,7 +452,7 @@ func (client *TransparentVlanEndpointClient) AddDefaultRoutes(linkToName string, Table: table, } - if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}); err != nil { + if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}, false); err != nil { return err } return nil From 7a0d8cd12b73ad96a0ebe42fdd9b3860b68ddba2 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Mon, 10 Apr 2023 14:16:43 -0700 Subject: [PATCH 03/14] moved log to place where its executed --- network/endpoint_linux.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index 6dc2aa487a..f35e51fd77 100644 --- a/network/endpoint_linux.go +++ b/network/endpoint_linux.go @@ -274,8 +274,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 @@ -320,6 +318,7 @@ func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, inte } } + 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 From 3ed02ea3beee051ac55e631f00711c1157635e13 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Mon, 10 Apr 2023 19:27:22 -0700 Subject: [PATCH 04/14] enable proxy arp on bridge to allow public connectivity from apipa interface --- network/networkutils/networkutils_linux.go | 6 ++ network/ovs_endpoint_snatroute_linux.go | 1 + network/snat/snat_linux.go | 91 +++++++++++++------ ...ansparent_vlan_endpoint_snatroute_linux.go | 1 + 4 files changed, 72 insertions(+), 27 deletions(-) diff --git a/network/networkutils/networkutils_linux.go b/network/networkutils/networkutils_linux.go index a135ee0b0c..181d9514be 100644 --- a/network/networkutils/networkutils_linux.go +++ b/network/networkutils/networkutils_linux.go @@ -267,6 +267,12 @@ func (nu NetworkUtils) DisableRAForInterface(ifName string) error { return err } +func (nu NetworkUtils) SetProxyArp(ifName string) error { + cmd := fmt.Sprintf("echo 1 > /proc/sys/net/ipv4/conf/%v/proxy_arp", ifName) + _, err := nu.plClient.ExecuteCommand(cmd) + return err +} + func getPrivateIPSpace() []string { privateIPAddresses := []string{"10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "169.254.0.0/16"} return privateIPAddresses diff --git a/network/ovs_endpoint_snatroute_linux.go b/network/ovs_endpoint_snatroute_linux.go index 9f43b2a6a2..b1625c92b1 100644 --- a/network/ovs_endpoint_snatroute_linux.go +++ b/network/ovs_endpoint_snatroute_linux.go @@ -29,6 +29,7 @@ func (client *OVSEndpointClient) NewSnatClient(snatBridgeIP, localIP string, epI snatBridgeIP, client.hostPrimaryMac, epInfo.DNS.Servers, + false, client.netlink, client.plClient, ) diff --git a/network/snat/snat_linux.go b/network/snat/snat_linux.go index e10e2d7471..fb95510cc2 100644 --- a/network/snat/snat_linux.go +++ b/network/snat/snat_linux.go @@ -40,9 +40,9 @@ type Client struct { localIP string SnatBridgeIP string SkipAddressesFromBlock []string + enableProxyArpOnBridge bool netlink netlink.NetlinkInterface - - plClient platform.ExecClient + plClient platform.ExecClient } func NewSnatClient(hostIfName string, @@ -51,20 +51,20 @@ func NewSnatClient(hostIfName string, snatBridgeIP string, hostPrimaryMac string, skipAddressesFromBlock []string, + enableProxyArpOnBridge bool, nl netlink.NetlinkInterface, - plClient platform.ExecClient, ) Client { log.Printf("Initialize new snat client") snatClient := Client{ - hostSnatVethName: hostIfName, - containerSnatVethName: contIfName, - localIP: localIP, - SnatBridgeIP: snatBridgeIP, - hostPrimaryMac: hostPrimaryMac, - netlink: nl, - - plClient: plClient, + hostSnatVethName: hostIfName, + containerSnatVethName: contIfName, + localIP: localIP, + SnatBridgeIP: snatBridgeIP, + hostPrimaryMac: hostPrimaryMac, + enableProxyArpOnBridge: enableProxyArpOnBridge, + netlink: nl, + plClient: plClient, } snatClient.SkipAddressesFromBlock = append(snatClient.SkipAddressesFromBlock, skipAddressesFromBlock...) @@ -81,6 +81,16 @@ func (client *Client) CreateSnatEndpoint() error { return err } + nuc := networkutils.NewNetworkUtils(client.netlink, client.plClient) + // Enabling proxy arp on bridge allows bridge to respond to arp requests it receives with its own mac otherwise arp requests are not getting forwarded and responded. + if client.enableProxyArpOnBridge { + // Enable proxy arp on bridge + if err := nuc.SetProxyArp(SnatBridgeName); err != nil { + log.Printf("Enabling proxy arp failed with error %v", err) + return err + } + } + // SNAT Rule to masquerade packets destined to non-vnet ip if err := client.addMasqueradeRule(client.SnatBridgeIP); err != nil { log.Printf("Adding snat rule failed with error %v", err) @@ -93,9 +103,8 @@ func (client *Client) CreateSnatEndpoint() error { return err } - epc := networkutils.NewNetworkUtils(client.netlink, client.plClient) // Create veth pair to tie one end to container and other end to linux bridge - if err := epc.CreateEndpoint(client.hostSnatVethName, client.containerSnatVethName, nil); err != nil { + if err := nuc.CreateEndpoint(client.hostSnatVethName, client.containerSnatVethName, nil); err != nil { log.Printf("Creating Snat Endpoint failed with error %v", err) return newErrorSnatClient(err.Error()) } @@ -127,9 +136,13 @@ func (client *Client) BlockIPAddressesOnSnatBridge() error { return nil } -/** +/* +* + Move container veth inside container network namespace -**/ + +* +*/ func (client *Client) MoveSnatEndpointToContainerNS(netnsPath string, nsID uintptr) error { log.Printf("[snat] Setting link %v netns %v.", client.containerSnatVethName, netnsPath) err := client.netlink.SetLinkNetNs(client.containerSnatVethName, nsID) @@ -139,9 +152,13 @@ func (client *Client) MoveSnatEndpointToContainerNS(netnsPath string, nsID uintp return nil } -/** +/* +* + Configure Routes and setup name for container veth -**/ + +* +*/ func (client *Client) SetupSnatContainerInterface() error { epc := networkutils.NewNetworkUtils(client.netlink, client.plClient) if err := epc.SetupContainerInterface(client.containerSnatVethName, azureSnatIfName); err != nil { @@ -159,9 +176,13 @@ func getNCLocalAndGatewayIP(client *Client) (brIP, contIP net.IP) { return bridgeIP, containerIP } -/** +/* +* + This function adds iptables rules that allows only host to NC communication and not the other way -**/ + +* +*/ func (client *Client) AllowInboundFromHostToNC() error { bridgeIP, containerIP := getNCLocalAndGatewayIP(client) @@ -250,9 +271,13 @@ func (client *Client) DeleteInboundFromHostToNC() error { return err } -/** +/* +* + This function adds iptables rules that allows only NC to Host communication and not the other way -**/ + +* +*/ func (client *Client) AllowInboundFromNCToHost() error { bridgeIP, containerIP := getNCLocalAndGatewayIP(client) @@ -388,9 +413,13 @@ func (client *Client) DropArpForSnatBridgeApipaRange(snatBridgeIP, azSnatVethIfN return err } -/** +/* +* + 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) if err == nil { @@ -437,18 +466,26 @@ func (client *Client) createSnatBridge(snatBridgeIP, hostPrimaryMac string) erro return nil } -/** +/* +* + This function adds iptable rules that will snat all traffic that has source ip in apipa range and coming via linux bridge -**/ + +* +*/ func (client *Client) addMasqueradeRule(snatBridgeIPWithPrefix string) error { _, ipNet, _ := net.ParseCIDR(snatBridgeIPWithPrefix) matchCondition := fmt.Sprintf("-s %s", ipNet.String()) return iptables.InsertIptableRule(iptables.V4, iptables.Nat, iptables.Postrouting, matchCondition, iptables.Masquerade) } -/** +/* +* + Drop all vlan traffic on linux bridge -**/ + +* +*/ func (client *Client) addVlanDropRule() error { out, err := client.plClient.ExecuteCommand(l2PreroutingEntries) if err != nil { diff --git a/network/transparent_vlan_endpoint_snatroute_linux.go b/network/transparent_vlan_endpoint_snatroute_linux.go index 109f699b35..609ec137df 100644 --- a/network/transparent_vlan_endpoint_snatroute_linux.go +++ b/network/transparent_vlan_endpoint_snatroute_linux.go @@ -17,6 +17,7 @@ func (client *TransparentVlanEndpointClient) NewSnatClient(snatBridgeIP, localIP snatBridgeIP, client.hostPrimaryMac.String(), epInfo.DNS.Servers, + true, client.netlink, client.plClient, ) From 68a1c1d5cdddc497f58bc5ccc25d6e8fa9278c78 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Tue, 11 Apr 2023 10:23:10 -0700 Subject: [PATCH 05/14] validate newly created namespace is not same as host namespace --- netns/netns.go | 8 ++++++++ network/transparent_vlan_endpointclient_linux.go | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/netns/netns.go b/netns/netns.go index dba8f30f80..48b04aea19 100644 --- a/netns/netns.go +++ b/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() +} diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index cd33653226..c0e80729bd 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -32,6 +32,8 @@ type netnsClient interface { Set(fileDescriptor int) (err error) NewNamed(name string) (fileDescriptor int, err error) DeleteNamed(name string) (err error) + IsNamespaceEqual(fd1, fd2 int) bool + NamespaceUniqueID(fd int) string } type TransparentVlanEndpointClient struct { primaryHostIfName string // So like eth0 @@ -130,6 +132,8 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er if err != nil { return errors.Wrap(err, "failed to create vnet ns") } + log.Printf("Vnet Namespace created: %s", client.netnsClient.NamespaceUniqueID(vnetNS)) + client.vnetNSFileDescriptor = vnetNS deleteNSIfNotNilErr := client.netnsClient.Set(vmNS) // Any failure will trigger removing the namespace created @@ -146,6 +150,12 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er return errors.Wrap(deleteNSIfNotNilErr, "failed to set current ns to vm") } + if client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { + deleteNSIfNotNilErr = fmt.Errorf("vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) + log.Errorf(deleteNSIfNotNilErr.Error()) + return deleteNSIfNotNilErr + } + // Now create vlan veth log.Printf("[transparent vlan] Create the host vlan link after getting eth0: %s", client.primaryHostIfName) // Get parent interface index. Index is consistent across libraries. From 90e4fad2cc735b13eb6020648bcdd2507cb2ea62 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Tue, 11 Apr 2023 14:57:08 -0700 Subject: [PATCH 06/14] addressed comments and added UTs --- netio/mocknetio.go | 11 ++ netlink/mocknetlink.go | 26 +++- network/bridge_endpointclient_linux.go | 4 +- network/endpoint_linux.go | 29 +---- network/endpoint_test.go | 120 ++++++++++++++++++ network/network_linux.go | 4 +- network/ovs_endpointclient_linux.go | 2 +- network/snat/snat_linux.go | 61 ++------- network/transparent_endpointclient_linux.go | 8 +- .../transparent_vlan_endpointclient_linux.go | 14 +- ...nsparent_vlan_endpointclient_linux_test.go | 8 ++ 11 files changed, 195 insertions(+), 92 deletions(-) diff --git a/netio/mocknetio.go b/netio/mocknetio.go index dd3c077692..b002e4dde4 100644 --- a/netio/mocknetio.go +++ b/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,6 +25,10 @@ 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++ @@ -29,6 +36,10 @@ func (netshim *MockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface 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{ diff --git a/netlink/mocknetlink.go b/netlink/mocknetlink.go index fb8471d59e..0b0b4f48db 100644 --- a/netlink/mocknetlink.go +++ b/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 func(route *Route) error + addRouteFn func(route *Route) error } 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() } diff --git a/network/bridge_endpointclient_linux.go b/network/bridge_endpointclient_linux.go index 1cb3f4437f..4dc591db61 100644 --- a/network/bridge_endpointclient_linux.go +++ b/network/bridge_endpointclient_linux.go @@ -195,7 +195,7 @@ func (client *LinuxBridgeEndpointClient) ConfigureContainerInterfacesAndRoutes(e return err } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes); err != nil { return err } @@ -288,7 +288,7 @@ func (client *LinuxBridgeEndpointClient) setupIPV6Routes(epInfo *EndpointInfo) e routes = append(routes, defaultV6Route) log.Printf("[net] Adding ipv6 routes in container %+v", routes) - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, routes, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, routes); err != nil { return nil } } diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index f35e51fd77..b4329e288c 100644 --- a/network/endpoint_linux.go +++ b/network/endpoint_linux.go @@ -270,7 +270,7 @@ func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform. func (ep *endpoint) getInfoImpl(epInfo *EndpointInfo) { } -func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, interfaceName string, routes []RouteInfo, removeRouteBeforeAdd bool) error { +func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, interfaceName string, routes []RouteInfo) error { ifIndex := 0 for _, route := range routes { @@ -302,22 +302,6 @@ func addRoutes(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, inte Table: route.Table, } - if removeRouteBeforeAdd { - delNlRoute := &netlink.Route{ - Family: family, - Dst: &route.Dst, - Gw: route.Gw, - Priority: route.Priority, - Protocol: route.Protocol, - Scope: route.Scope, - Table: route.Table, - } - log.Printf("[net] Deleting old IP route before add:%+v.", route) - if err := nl.DeleteIPRoute(delNlRoute); err != nil { - log.Printf("[net] Try to remove route if exists before add returns: %v", err) - } - } - 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") { @@ -335,8 +319,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 { @@ -345,15 +327,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) @@ -362,12 +344,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 } @@ -498,7 +481,7 @@ func (nm *networkManager) updateRoutes(existingEp *EndpointInfo, targetEp *Endpo return err } - err = addRoutes(nm.netlink, &netio.NetIO{}, existingEp.IfName, tobeAddedRoutes, false) + err = addRoutes(nm.netlink, &netio.NetIO{}, existingEp.IfName, tobeAddedRoutes) if err != nil { return err } diff --git a/network/endpoint_test.go b/network/endpoint_test.go index 2f758910d4..99caaeffa6 100644 --- a/network/endpoint_test.go +++ b/network/endpoint_test.go @@ -4,8 +4,12 @@ package network import ( + "errors" + "net" "testing" + "github.com/Azure/azure-container-networking/netio" + "github.com/Azure/azure-container-networking/netlink" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -193,4 +197,120 @@ var _ = Describe("Test Endpoint", 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)) + 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.New("interface not found") + }) + + err := addRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: ""}}) + Expect(err).ToNot(BeNil()) + }) + }) }) diff --git a/network/network_linux.go b/network/network_linux.go index 24599c0310..3374fb64a2 100644 --- a/network/network_linux.go +++ b/network/network_linux.go @@ -118,7 +118,7 @@ func (nm *networkManager) newNetworkImpl(nwInfo *NetworkInfo, extIf *externalInt func (nm *networkManager) handleCommonOptions(ifName string, nwInfo *NetworkInfo) error { var err error if routes, exists := nwInfo.Options[RoutesKey]; exists { - err = addRoutes(nm.netlink, nm.netio, ifName, routes.([]RouteInfo), false) + err = addRoutes(nm.netlink, nm.netio, ifName, routes.([]RouteInfo)) if err != nil { return err } @@ -649,7 +649,7 @@ func AddStaticRoute(nl netlink.NetlinkInterface, netioshim netio.NetIOInterface, gwIP := net.ParseIP("0.0.0.0") route := RouteInfo{Dst: *ipNet, Gw: gwIP} routes = append(routes, route) - if err := addRoutes(nl, netioshim, interfaceName, routes, false); err != nil { + if err := addRoutes(nl, netioshim, interfaceName, routes); err != nil { if err != nil && !strings.Contains(strings.ToLower(err.Error()), "file exists") { log.Printf("addroutes failed with error %v", err) return err diff --git a/network/ovs_endpointclient_linux.go b/network/ovs_endpointclient_linux.go index 305d0e8c61..a3d0874ef6 100644 --- a/network/ovs_endpointclient_linux.go +++ b/network/ovs_endpointclient_linux.go @@ -230,7 +230,7 @@ func (client *OVSEndpointClient) ConfigureContainerInterfacesAndRoutes(epInfo *E return err } - return addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes, false) + return addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes) } func (client *OVSEndpointClient) DeleteEndpoints(ep *endpoint) error { diff --git a/network/snat/snat_linux.go b/network/snat/snat_linux.go index fb95510cc2..c6d77bd8b3 100644 --- a/network/snat/snat_linux.go +++ b/network/snat/snat_linux.go @@ -136,13 +136,7 @@ func (client *Client) BlockIPAddressesOnSnatBridge() error { return nil } -/* -* - - Move container veth inside container network namespace - -* -*/ +// Move container veth inside container network namespace func (client *Client) MoveSnatEndpointToContainerNS(netnsPath string, nsID uintptr) error { log.Printf("[snat] Setting link %v netns %v.", client.containerSnatVethName, netnsPath) err := client.netlink.SetLinkNetNs(client.containerSnatVethName, nsID) @@ -152,13 +146,7 @@ func (client *Client) MoveSnatEndpointToContainerNS(netnsPath string, nsID uintp return nil } -/* -* - - Configure Routes and setup name for container veth - -* -*/ +// Configure Routes and setup name for container veth func (client *Client) SetupSnatContainerInterface() error { epc := networkutils.NewNetworkUtils(client.netlink, client.plClient) if err := epc.SetupContainerInterface(client.containerSnatVethName, azureSnatIfName); err != nil { @@ -176,13 +164,7 @@ func getNCLocalAndGatewayIP(client *Client) (brIP, contIP net.IP) { return bridgeIP, containerIP } -/* -* - - This function adds iptables rules that allows only host to NC communication and not the other way - -* -*/ +// This function adds iptables rules that allows only host to NC communication and not the other way func (client *Client) AllowInboundFromHostToNC() error { bridgeIP, containerIP := getNCLocalAndGatewayIP(client) @@ -271,13 +253,7 @@ func (client *Client) DeleteInboundFromHostToNC() error { return err } -/* -* - - This function adds iptables rules that allows only NC to Host communication and not the other way - -* -*/ +// This function adds iptables rules that allows only NC to Host communication and not the other way func (client *Client) AllowInboundFromNCToHost() error { bridgeIP, containerIP := getNCLocalAndGatewayIP(client) @@ -365,10 +341,7 @@ func (client *Client) DeleteInboundFromNCToHost() error { return err } -/** - Configures Local IP Address for container Veth -**/ - +// Configures Local IP Address for container Veth func (client *Client) ConfigureSnatContainerInterface() error { log.Printf("[snat] Adding IP address %v to link %v.", client.localIP, client.containerSnatVethName) ip, intIpAddr, _ := net.ParseCIDR(client.localIP) @@ -413,13 +386,7 @@ func (client *Client) DropArpForSnatBridgeApipaRange(snatBridgeIP, azSnatVethIfN return err } -/* -* - - This function creates linux bridge which will be used for outbound connectivity by NCs - -* -*/ +// 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) if err == nil { @@ -466,26 +433,14 @@ func (client *Client) createSnatBridge(snatBridgeIP, hostPrimaryMac string) erro return nil } -/* -* - - This function adds iptable rules that will snat all traffic that has source ip in apipa range and coming via linux bridge - -* -*/ +// This function adds iptable rules that will snat all traffic that has source ip in apipa range and coming via linux bridge func (client *Client) addMasqueradeRule(snatBridgeIPWithPrefix string) error { _, ipNet, _ := net.ParseCIDR(snatBridgeIPWithPrefix) matchCondition := fmt.Sprintf("-s %s", ipNet.String()) return iptables.InsertIptableRule(iptables.V4, iptables.Nat, iptables.Postrouting, matchCondition, iptables.Masquerade) } -/* -* - - Drop all vlan traffic on linux bridge - -* -*/ +// Drop all vlan traffic on linux bridge func (client *Client) addVlanDropRule() error { out, err := client.plClient.ExecuteCommand(l2PreroutingEntries) if err != nil { diff --git a/network/transparent_endpointclient_linux.go b/network/transparent_endpointclient_linux.go index da47f48889..82fe9bf51a 100644 --- a/network/transparent_endpointclient_linux.go +++ b/network/transparent_endpointclient_linux.go @@ -152,7 +152,7 @@ func (client *TransparentEndpointClient) AddEndpointRules(epInfo *EndpointInfo) log.Printf("[net] Adding route for the ip %v", ipNet.String()) routeInfo.Dst = ipNet routeInfoList = append(routeInfoList, routeInfo) - if err := addRoutes(client.netlink, client.netioshim, client.hostVethName, routeInfoList, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.hostVethName, routeInfoList); err != nil { return newErrorTransparentEndpointClient(err.Error()) } } @@ -234,7 +234,7 @@ func (client *TransparentEndpointClient) ConfigureContainerInterfacesAndRoutes(e Dst: *virtualGwNet, Scope: netlink.RT_SCOPE_LINK, } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}); err != nil { return newErrorTransparentEndpointClient(err.Error()) } @@ -245,7 +245,7 @@ func (client *TransparentEndpointClient) ConfigureContainerInterfacesAndRoutes(e Dst: dstIP, Gw: virtualGwIP, } - if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{routeInfo}); err != nil { return err } @@ -294,7 +294,7 @@ func (client *TransparentEndpointClient) setupIPV6Routes() error { Gw: virtualGwIP, } - return addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{gwRoute, defaultRoute}, false) + return addRoutes(client.netlink, client.netioshim, client.containerVethName, []RouteInfo{gwRoute, defaultRoute}) } func (client *TransparentEndpointClient) setIPV6NeighEntry() error { diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index c0e80729bd..981d77e03f 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -5,6 +5,7 @@ import ( "net" "strings" + "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/iptables" "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/netio" @@ -402,7 +403,14 @@ func (client *TransparentVlanEndpointClient) ConfigureVnetInterfacesAndRoutesImp if err = client.AddDefaultArp(client.vlanIfName, azureMac); err != nil { return errors.Wrap(err, "failed vnet ns add default arp entry (idempotent)") } - if err = addRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList, true); err != nil { + + // Delete old route if any for this IP + logger.Printf("[transparent-vlan] Deleting old route if any.") + if err = deleteRoutes(client.netlink, client.netioshim, "", routeInfoList); err != nil { + return errors.Wrap(err, "failed deleting routes to vnet specific to this container") + } + + if err = addRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList); err != nil { return errors.Wrap(err, "failed adding routes to vnet specific to this container") } if err = client.addDefaultRoutes(client.vlanIfName, tunnelingTable); err != nil { @@ -449,7 +457,7 @@ func (client *TransparentVlanEndpointClient) addDefaultRoutes(linkToName string, Table: table, } // Difference between interface name in addRoutes and DevName: in RouteInfo? - if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}); err != nil { return err } @@ -462,7 +470,7 @@ func (client *TransparentVlanEndpointClient) addDefaultRoutes(linkToName string, Table: table, } - if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}, false); err != nil { + if err := addRoutes(client.netlink, client.netioshim, linkToName, []RouteInfo{routeInfo}); err != nil { return err } return nil diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 5d0c42acf3..51fd291517 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -49,6 +49,14 @@ func (netns *mockNetns) DeleteNamed(name string) (err error) { return netns.deleteNamed(name) } +func (netns *mockNetns) IsNamespaceEqual(fd1, fd2 int) bool { + return false +} + +func (netns *mockNetns) NamespaceUniqueID(fd int) string { + return "nsid" +} + func defaultGet() (int, error) { return 1, nil } From b2f0a293af64cddb240e2037cd17e36e162e27df Mon Sep 17 00:00:00 2001 From: tamanoha Date: Wed, 12 Apr 2023 17:07:24 -0700 Subject: [PATCH 07/14] fixed cni delete call for linux multitenancy --- cni/network/network.go | 18 ++++++------- network/endpoint_windows.go | 1 + network/manager.go | 2 +- network/network.go | 6 ++--- network/network_test.go | 27 +++++++++---------- .../transparent_vlan_endpointclient_linux.go | 7 ++--- 6 files changed, 28 insertions(+), 33 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index e6aeeae3a9..d7ce953fb5 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -953,7 +953,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 +973,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)) diff --git a/network/endpoint_windows.go b/network/endpoint_windows.go index 0889e59d1d..7e19e186a3 100644 --- a/network/endpoint_windows.go +++ b/network/endpoint_windows.go @@ -156,6 +156,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 { diff --git a/network/manager.go b/network/manager.go index 0fbeb2d0a9..ec569a469a 100644 --- a/network/manager.go +++ b/network/manager.go @@ -76,7 +76,7 @@ type NetworkManager interface { GetNetworkInfo(networkID string) (NetworkInfo, error) // FindNetworkIDFromNetNs returns the network name that contains an endpoint created for this netNS, errNetworkNotFound if no network is found FindNetworkIDFromNetNs(netNs string) (string, error) - GetNumEndpointsInNetNs(netNs string) int + GetNumEndpointsByContainerID(containerID string) int CreateEndpoint(client apipaClient, networkID string, epInfo *EndpointInfo) error DeleteEndpoint(networkID string, endpointID string) error diff --git a/network/network.go b/network/network.go index 9a9dfac69a..f09725b027 100644 --- a/network/network.go +++ b/network/network.go @@ -276,7 +276,7 @@ func (nm *networkManager) FindNetworkIDFromNetNs(netNs string) (string, error) { } // GetNumEndpointsInNetNs returns number of endpoints -func (nm *networkManager) GetNumEndpointsInNetNs(netNs string) int { +func (nm *networkManager) GetNumEndpointsByContainerID(containerID string) int { numEndpoints := 0 // Look through the external interfaces for _, iface := range nm.ExternalInterfaces { @@ -285,8 +285,8 @@ func (nm *networkManager) GetNumEndpointsInNetNs(netNs string) int { // Network may have multiple endpoints, so look through all of them for _, endpoint := range network.Endpoints { // If the netNs matches for this endpoint, return the network ID (which is the name) - if endpoint.NetNs == netNs { - log.Printf("Found endpoint [%s] for NetNS [%s]", endpoint.Id, netNs) + if endpoint.ContainerID == containerID { + log.Printf("Found endpoint [%s] for containerID [%s]", endpoint.Id, containerID) numEndpoints++ } } diff --git a/network/network_test.go b/network/network_test.go index c804aa2f2f..7bd353577c 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -258,10 +258,10 @@ var _ = Describe("Test Network", func() { }) }) - Describe("Test GetNumEndpointsInNetNs", func() { + Describe("Test GetNumEndpointsByContainerID", func() { Context("When one network has one endpoint and another network has two endpoints", func() { It("Should return three endpoints", func() { - netNs := "989c079b-45a6-485f-8f9e-88b05d6c55c5" + containerID := "989c079b-45a6-485f-8f9e-88b05d6c55c5" networkOneID := "byovnetbridge-vlan1-10-128-8-0_23" networkTwoID := "byovnetbridge-vlan2-20-128-8-0_23" @@ -274,15 +274,14 @@ var _ = Describe("Test Network", func() { Id: "byovnetbridge-vlan1-10-128-8-0_23", Endpoints: map[string]*endpoint{ "a591be2a-eth0": { - Id: "a591be2a-eth0", - NetNs: netNs, + Id: "a591be2a-eth0", + ContainerID: containerID, }, "a691be2b-eth0": { - Id: "a691be2b-eth0", - NetNs: netNs, + Id: "a691be2b-eth0", + ContainerID: containerID, }, }, - NetNs: "aaac079b-45a6-485f-8f9e-88b05d6c55c5", }, }, }, @@ -293,30 +292,30 @@ var _ = Describe("Test Network", func() { Id: "byovnetbridge-vlan2-20-128-8-0_23", Endpoints: map[string]*endpoint{ "a591be2b-eth0": { - Id: "a591be2b-eth0", - NetNs: netNs, + Id: "a591be2b-eth0", + ContainerID: containerID, }, }, - NetNs: "aaac079b-45a6-485f-8f9e-88b05d6c55c5", + NetNs: "", }, }, }, }, } - got := nm.GetNumEndpointsInNetNs(netNs) - Expect(got).To(Equal(3)) + got := nm.GetNumEndpointsByContainerID(containerID) + Expect(3).To(Equal(got)) }) }) Context("When network does not exist", func() { It("Should return zero endpoints", func() { - netNs := "989c079b-45a6-485f-8f9e-88b05d6c55c9" + containerID := "989c079b-45a6-485f-8f9e-88b05d6c55c9" nm := &networkManager{ ExternalInterfaces: make(map[string]*externalInterface), } - got := nm.GetNumEndpointsInNetNs(netNs) + got := nm.GetNumEndpointsByContainerID(containerID) Expect(got).To(Equal(0)) }) }) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 981d77e03f..7397c6f16e 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -5,7 +5,6 @@ import ( "net" "strings" - "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/iptables" "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/netio" @@ -405,10 +404,8 @@ func (client *TransparentVlanEndpointClient) ConfigureVnetInterfacesAndRoutesImp } // Delete old route if any for this IP - logger.Printf("[transparent-vlan] Deleting old route if any.") - if err = deleteRoutes(client.netlink, client.netioshim, "", routeInfoList); err != nil { - return errors.Wrap(err, "failed deleting routes to vnet specific to this container") - } + err = deleteRoutes(client.netlink, client.netioshim, "", routeInfoList) + log.Printf("[transparent-vlan] Deleting old routes returned:%v", err) if err = addRoutes(client.netlink, client.netioshim, client.vnetVethName, routeInfoList); err != nil { return errors.Wrap(err, "failed adding routes to vnet specific to this container") From 76606f3f26a915de368241afbd6abcdd825911e3 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Thu, 13 Apr 2023 01:01:09 -0700 Subject: [PATCH 08/14] lint fixes --- network/api.go | 1 + network/endpoint_test.go | 5 +++-- network/networkutils/networkutils_linux.go | 4 ++-- network/snat/snat_linux.go | 4 ++-- network/transparent_vlan_endpointclient_linux.go | 2 +- network/transparent_vlan_endpointclient_linux_test.go | 4 ++-- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/network/api.go b/network/api.go index 6c8374439d..88b57c1ac4 100644 --- a/network/api.go +++ b/network/api.go @@ -20,6 +20,7 @@ var ( errMultipleEndpointsFound = fmt.Errorf("Multiple endpoints found") errEndpointInUse = fmt.Errorf("Endpoint is already joined to a sandbox") errEndpointNotInUse = fmt.Errorf("Endpoint is not joined to a sandbox") + errNamespaceCreation = fmt.Errorf("Network namespace creation error") ) type networkNotFoundError struct{} diff --git a/network/endpoint_test.go b/network/endpoint_test.go index 99caaeffa6..521f4eac02 100644 --- a/network/endpoint_test.go +++ b/network/endpoint_test.go @@ -4,7 +4,6 @@ package network import ( - "errors" "net" "testing" @@ -12,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/netlink" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/pkg/errors" ) func TestEndpoint(t *testing.T) { @@ -298,6 +298,7 @@ var _ = Describe("Test Endpoint", func() { nlc := netlink.NewMockNetlink(false, "") nlc.SetAddRouteValidationFn(func(r *netlink.Route) error { Expect(r.LinkIndex).To(Equal(0)) + //nolint:goerr113 return errors.New("Cannot add route") }) @@ -306,7 +307,7 @@ var _ = Describe("Test Endpoint", func() { Expect(ifName).To(Equal("")) return &net.Interface{ Index: 0, - }, errors.New("interface not found") + }, errors.Wrapf(netio.ErrInterfaceNil, "Cannot get interface") }) err := addRoutes(nlc, netiocl, "", []RouteInfo{{Dst: *dst, DevName: ""}}) diff --git a/network/networkutils/networkutils_linux.go b/network/networkutils/networkutils_linux.go index 181d9514be..c068bbd85f 100644 --- a/network/networkutils/networkutils_linux.go +++ b/network/networkutils/networkutils_linux.go @@ -4,7 +4,6 @@ package networkutils import ( - "errors" "fmt" "net" @@ -12,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/netlink" "github.com/Azure/azure-container-networking/platform" + "github.com/pkg/errors" ) /*RFC For Private Address Space: https://tools.ietf.org/html/rfc1918 @@ -270,7 +270,7 @@ func (nu NetworkUtils) DisableRAForInterface(ifName string) error { func (nu NetworkUtils) SetProxyArp(ifName string) error { cmd := fmt.Sprintf("echo 1 > /proc/sys/net/ipv4/conf/%v/proxy_arp", ifName) _, err := nu.plClient.ExecuteCommand(cmd) - return err + return errors.Wrapf(err, "failed to set proxy arp for interface %v", ifName) } func getPrivateIPSpace() []string { diff --git a/network/snat/snat_linux.go b/network/snat/snat_linux.go index c6d77bd8b3..92e0c204c7 100644 --- a/network/snat/snat_linux.go +++ b/network/snat/snat_linux.go @@ -4,7 +4,6 @@ package snat import ( - "errors" "fmt" "net" "strings" @@ -15,6 +14,7 @@ import ( "github.com/Azure/azure-container-networking/netlink" "github.com/Azure/azure-container-networking/network/networkutils" "github.com/Azure/azure-container-networking/platform" + "github.com/pkg/errors" ) const ( @@ -87,7 +87,7 @@ func (client *Client) CreateSnatEndpoint() error { // Enable proxy arp on bridge if err := nuc.SetProxyArp(SnatBridgeName); err != nil { log.Printf("Enabling proxy arp failed with error %v", err) - return err + return errors.Wrap(err, "") } } diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 7397c6f16e..81e1cb78e4 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -151,7 +151,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } if client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { - deleteNSIfNotNilErr = fmt.Errorf("vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) + deleteNSIfNotNilErr = errors.Wrap(errNamespaceCreation, "vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) log.Errorf(deleteNSIfNotNilErr.Error()) return deleteNSIfNotNilErr } diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 51fd291517..9600236324 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -49,11 +49,11 @@ func (netns *mockNetns) DeleteNamed(name string) (err error) { return netns.deleteNamed(name) } -func (netns *mockNetns) IsNamespaceEqual(fd1, fd2 int) bool { +func (netns *mockNetns) IsNamespaceEqual(_, _ int) bool { return false } -func (netns *mockNetns) NamespaceUniqueID(fd int) string { +func (netns *mockNetns) NamespaceUniqueID(_ int) string { return "nsid" } From dc94fc430b37a9c608df32f577eff8c09cdfe178 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Thu, 13 Apr 2023 16:12:37 -0700 Subject: [PATCH 09/14] windows lint fixes --- network/endpoint_linux_test.go | 137 ++++++++++++++++++ network/endpoint_test.go | 121 ---------------- network/manager_mock.go | 4 +- .../transparent_vlan_endpointclient_linux.go | 2 +- 4 files changed, 140 insertions(+), 124 deletions(-) create mode 100644 network/endpoint_linux_test.go diff --git a/network/endpoint_linux_test.go b/network/endpoint_linux_test.go new file mode 100644 index 0000000000..1b42e4bf12 --- /dev/null +++ b/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 + 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()) + }) + }) +}) diff --git a/network/endpoint_test.go b/network/endpoint_test.go index 521f4eac02..2f758910d4 100644 --- a/network/endpoint_test.go +++ b/network/endpoint_test.go @@ -4,14 +4,10 @@ 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 TestEndpoint(t *testing.T) { @@ -197,121 +193,4 @@ var _ = Describe("Test Endpoint", 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 - 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()) - }) - }) }) diff --git a/network/manager_mock.go b/network/manager_mock.go index 524a64672e..a9f036416f 100644 --- a/network/manager_mock.go +++ b/network/manager_mock.go @@ -115,8 +115,8 @@ func (nm *MockNetworkManager) FindNetworkIDFromNetNs(netNs string) (string, erro return "", errNetworkNotFound } -// GetNumEndpointsInNetNs mock -func (nm *MockNetworkManager) GetNumEndpointsInNetNs(netNs string) int { +// GetNumEndpointsByContainerID mock +func (nm *MockNetworkManager) GetNumEndpointsByContainerID(containerID string) int { // based on the GetAllEndpoints func above, it seems that this mock is only intended to be used with // one network, so just return the number of endpoints if network exists numEndpoints := 0 diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 81e1cb78e4..3d6e3ffed7 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -151,7 +151,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } if client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { - deleteNSIfNotNilErr = errors.Wrap(errNamespaceCreation, "vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) + deleteNSIfNotNilErr = errors.Wrapf(errNamespaceCreation, "vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) log.Errorf(deleteNSIfNotNilErr.Error()) return deleteNSIfNotNilErr } From f003b2c9f2373d7fcd36bc192c8973236a4307c9 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Fri, 14 Apr 2023 09:35:32 -0700 Subject: [PATCH 10/14] lint fixes --- network/api.go | 1 - network/endpoint_linux_test.go | 2 +- network/manager_mock.go | 2 +- network/transparent_vlan_endpointclient_linux.go | 4 ++++ 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/network/api.go b/network/api.go index 88b57c1ac4..6c8374439d 100644 --- a/network/api.go +++ b/network/api.go @@ -20,7 +20,6 @@ var ( errMultipleEndpointsFound = fmt.Errorf("Multiple endpoints found") errEndpointInUse = fmt.Errorf("Endpoint is already joined to a sandbox") errEndpointNotInUse = fmt.Errorf("Endpoint is not joined to a sandbox") - errNamespaceCreation = fmt.Errorf("Network namespace creation error") ) type networkNotFoundError struct{} diff --git a/network/endpoint_linux_test.go b/network/endpoint_linux_test.go index 1b42e4bf12..58f3ae43dd 100644 --- a/network/endpoint_linux_test.go +++ b/network/endpoint_linux_test.go @@ -118,7 +118,7 @@ var _ = Describe("Test TestEndpointLinux", func() { nlc := netlink.NewMockNetlink(false, "") nlc.SetAddRouteValidationFn(func(r *netlink.Route) error { Expect(r.LinkIndex).To(Equal(0)) - //nolint:goerr113 + //nolint:goerr113 // for testing return errors.New("Cannot add route") }) diff --git a/network/manager_mock.go b/network/manager_mock.go index a9f036416f..8b8f4af311 100644 --- a/network/manager_mock.go +++ b/network/manager_mock.go @@ -116,7 +116,7 @@ func (nm *MockNetworkManager) FindNetworkIDFromNetNs(netNs string) (string, erro } // GetNumEndpointsByContainerID mock -func (nm *MockNetworkManager) GetNumEndpointsByContainerID(containerID string) int { +func (nm *MockNetworkManager) GetNumEndpointsByContainerID(_ string) int { // based on the GetAllEndpoints func above, it seems that this mock is only intended to be used with // one network, so just return the number of endpoints if network exists numEndpoints := 0 diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 3d6e3ffed7..a85a1d68d2 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -26,6 +26,10 @@ const ( DisableRPFilterCmd = "sysctl -w net.ipv4.conf.all.rp_filter=0" // Command to disable the rp filter for tunneling ) +var ( + errNamespaceCreation = fmt.Errorf("Network namespace creation error") +) + type netnsClient interface { Get() (fileDescriptor int, err error) GetFromName(name string) (fileDescriptor int, err error) From f38151009cdde5b92d859391bb2b395e6979be4e Mon Sep 17 00:00:00 2001 From: tamanoha Date: Fri, 14 Apr 2023 15:40:39 -0700 Subject: [PATCH 11/14] fix issues with network namespace creation and vlan interface creation --- cni/network/network.go | 6 +- .../transparent_vlan_endpointclient_linux.go | 74 +++++++++++++++---- 2 files changed, 62 insertions(+), 18 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index d7ce953fb5..ca4929858f 100644 --- a/cni/network/network.go +++ b/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 diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index a85a1d68d2..0ca9c1b427 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "strings" + "time" "github.com/Azure/azure-container-networking/iptables" "github.com/Azure/azure-container-networking/log" @@ -118,6 +119,35 @@ func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) }) } +func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS, numRetries int) error { + var isNamespaceUnique bool + + for i := 0; i < numRetries; i++ { + vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) + if err != nil { + return errors.Wrap(err, "failed to create vnet ns") + } + log.Printf("Vnet Namespace created: %s", client.netnsClient.NamespaceUniqueID(vnetNS)) + if !client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { + client.vnetNSFileDescriptor = vnetNS + isNamespaceUnique = true + break + } + log.Printf("Vnet Namespace is the same as VM namespace. Deleting and retrying...") + delErr := client.netnsClient.DeleteNamed(client.vnetNSName) + if delErr != nil { + log.Errorf("failed to cleanup/delete ns after failing to create vlan veth:%v", delErr) + } + time.Sleep(100 * time.Millisecond) + } + + if !isNamespaceUnique { + return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are same") + } + + return nil +} + // Called from AddEndpoints, Namespace: VM func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) error { vmNS, err := client.netnsClient.Get() @@ -126,19 +156,18 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } log.Printf("[transparent vlan] Checking if NS exists...") - vnetNS, existingErr := client.netnsClient.GetFromName(client.vnetNSName) + var existingErr error + client.vnetNSFileDescriptor, existingErr = client.netnsClient.GetFromName(client.vnetNSName) // If the ns does not exist, the below code will trigger to create it // This will also (we assume) mean the vlan veth does not exist if existingErr != nil { // We assume the only possible error is that the namespace doesn't exist log.Printf("[transparent vlan] No existing NS detected. Creating the vnet namespace and switching to it") - vnetNS, err = client.netnsClient.NewNamed(client.vnetNSName) - if err != nil { - return errors.Wrap(err, "failed to create vnet ns") + + if err := client.createNetworkNamespace(vmNS, 5); err != nil { + return errors.Wrap(err, "") } - log.Printf("Vnet Namespace created: %s", client.netnsClient.NamespaceUniqueID(vnetNS)) - client.vnetNSFileDescriptor = vnetNS deleteNSIfNotNilErr := client.netnsClient.Set(vmNS) // Any failure will trigger removing the namespace created defer func() { @@ -154,12 +183,6 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er return errors.Wrap(deleteNSIfNotNilErr, "failed to set current ns to vm") } - if client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { - deleteNSIfNotNilErr = errors.Wrapf(errNamespaceCreation, "vnet ns:%d and vm ns:%d are the same. deleting vnet namespace created", vnetNS, vmNS) - log.Errorf(deleteNSIfNotNilErr.Error()) - return deleteNSIfNotNilErr - } - // Now create vlan veth log.Printf("[transparent vlan] Create the host vlan link after getting eth0: %s", client.primaryHostIfName) // Get parent interface index. Index is consistent across libraries. @@ -179,8 +202,11 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // Create vlan veth deleteNSIfNotNilErr = vishnetlink.LinkAdd(link) if deleteNSIfNotNilErr != nil { - // Any failure to add the link should error (auto delete NS) - return errors.Wrap(deleteNSIfNotNilErr, "failed to create vlan vnet link after making new ns") + // ignore link already exists error + if !strings.Contains(deleteNSIfNotNilErr.Error(), "file exists") { + // Any failure to add the link should error (auto delete NS) + return errors.Wrap(deleteNSIfNotNilErr, "failed to create vlan vnet link after making new ns") + } } defer func() { if deleteNSIfNotNilErr != nil { @@ -190,6 +216,19 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } } }() + + // sometimes there is slight delay in interface creation. check if it exists + for i := 0; i < 10; i++ { + if _, err = client.netioshim.GetNetworkInterfaceByName(client.vlanIfName); err == nil { + break + } + time.Sleep(100 * time.Millisecond) + } + if err != nil { + deleteNSIfNotNilErr = errors.Wrapf(err, "failed to get vlan veth interface:%s", client.vlanIfName) + return deleteNSIfNotNilErr + } + deleteNSIfNotNilErr = client.netUtilsClient.DisableRAForInterface(client.vlanIfName) if deleteNSIfNotNilErr != nil { return errors.Wrap(deleteNSIfNotNilErr, "failed to disable router advertisements for vlan vnet link") @@ -203,7 +242,6 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } else { log.Printf("[transparent vlan] Existing NS (%s) detected. Assuming %s exists too", client.vnetNSName, client.vlanIfName) } - client.vnetNSFileDescriptor = vnetNS // Get the default constant host veth mac mac, err := net.ParseMAC(defaultHostVethHwAddr) @@ -437,7 +475,7 @@ func (client *TransparentVlanEndpointClient) GetVnetRoutes(ipAddresses []net.IPN } else { ipNet = net.IPNet{IP: ipAddr.IP, Mask: net.CIDRMask(ipv6FullMask, ipv6Bits)} } - log.Printf("[net] transparent vlan client adding route for the ip %v", ipNet.String()) + log.Printf("[net] Getting route for this ip %v", ipNet.String()) routeInfo.Dst = ipNet routeInfoList = append(routeInfoList, routeInfo) @@ -532,6 +570,10 @@ func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, g return errors.Wrap(err, "failed to remove routes") } + if err := client.netlink.DeleteLink(client.vnetVethName); err != nil { + return errors.Wrapf(err, "DeleteLink for %v failed: %v", client.vnetVethName) + } + routesLeft, err := getNumRoutesLeft() if err != nil { return err From 48e12c5b1119daf2fe97ac8d7fbef3a829340d70 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Fri, 14 Apr 2023 16:00:12 -0700 Subject: [PATCH 12/14] Removed deletehostveth flag and delete host veth on delete endpoint trigger --- network/bridge_endpointclient_linux.go | 2 +- network/endpoint_linux.go | 4 ++-- network/manager.go | 2 +- network/mock_endpointclient.go | 2 +- network/ovs_endpointclient_linux.go | 2 +- network/transparent_endpointclient_linux.go | 2 +- .../transparent_vlan_endpointclient_linux.go | 22 +++++-------------- ...nsparent_vlan_endpointclient_linux_test.go | 2 +- 8 files changed, 13 insertions(+), 25 deletions(-) diff --git a/network/bridge_endpointclient_linux.go b/network/bridge_endpointclient_linux.go index 4de67e26f3..4dc591db61 100644 --- a/network/bridge_endpointclient_linux.go +++ b/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 { diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index 81e2dfc459..52950fc6f7 100644 --- a/network/endpoint_linux.go +++ b/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 } diff --git a/network/manager.go b/network/manager.go index 9e0e7631fc..e640feb464 100644 --- a/network/manager.go +++ b/network/manager.go @@ -49,7 +49,7 @@ type EndpointClient interface { MoveEndpointsToContainerNS(epInfo *EndpointInfo, nsID uintptr) error SetupContainerInterfaces(epInfo *EndpointInfo) error ConfigureContainerInterfacesAndRoutes(epInfo *EndpointInfo) error - DeleteEndpoints(ep *endpoint, deleteHostVeth bool) error + DeleteEndpoints(ep *endpoint) error } // NetworkManager manages the set of container networking resources. diff --git a/network/mock_endpointclient.go b/network/mock_endpointclient.go index df35df6fd6..deff106f3c 100644 --- a/network/mock_endpointclient.go +++ b/network/mock_endpointclient.go @@ -58,7 +58,7 @@ func (client *MockEndpointClient) ConfigureContainerInterfacesAndRoutes(_ *Endpo return nil } -func (client *MockEndpointClient) DeleteEndpoints(ep *endpoint, _ bool) error { +func (client *MockEndpointClient) DeleteEndpoints(ep *endpoint) error { delete(client.endpoints, ep.Id) return nil } diff --git a/network/ovs_endpointclient_linux.go b/network/ovs_endpointclient_linux.go index d71638084c..a3d0874ef6 100644 --- a/network/ovs_endpointclient_linux.go +++ b/network/ovs_endpointclient_linux.go @@ -233,7 +233,7 @@ func (client *OVSEndpointClient) ConfigureContainerInterfacesAndRoutes(epInfo *E return addRoutes(client.netlink, client.netioshim, client.containerVethName, epInfo.Routes) } -func (client *OVSEndpointClient) DeleteEndpoints(ep *endpoint, _ bool) error { +func (client *OVSEndpointClient) DeleteEndpoints(ep *endpoint) error { log.Printf("[ovs] Deleting veth pair %v %v.", ep.HostIfName, ep.IfName) err := client.netlink.DeleteLink(ep.HostIfName) if err != nil { diff --git a/network/transparent_endpointclient_linux.go b/network/transparent_endpointclient_linux.go index 1e56dc9add..013515af51 100644 --- a/network/transparent_endpointclient_linux.go +++ b/network/transparent_endpointclient_linux.go @@ -314,6 +314,6 @@ func (client *TransparentEndpointClient) setIPV6NeighEntry() error { return nil } -func (client *TransparentEndpointClient) DeleteEndpoints(_ *endpoint, _ bool) error { +func (client *TransparentEndpointClient) DeleteEndpoints(_ *endpoint) error { return nil } diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 062ae79fd7..e45226b64d 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -538,7 +538,7 @@ func (client *TransparentVlanEndpointClient) AddDefaultArp(interfaceName, destMa return nil } -func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint, deleteHosVeth bool) error { +func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error { // Vnet NS err := ExecuteInNS(client.vnetNSName, func() error { // Passing in functionality to get number of routes after deletion @@ -550,7 +550,7 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint, delet return len(routes), nil } - return client.DeleteEndpointsImpl(ep, getNumRoutesLeft, deleteHosVeth) + return client.DeleteEndpointsImpl(ep, getNumRoutesLeft) }) if err != nil { return err @@ -564,30 +564,18 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint, delet } // getNumRoutesLeft is a function which gets the current number of routes in the namespace. Namespace: Vnet -func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, getNumRoutesLeft func() (int, error), deleteHostVeth bool) error { +func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, getNumRoutesLeft func() (int, error)) 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") } + log.Printf("Deleting host veth %v", client.vnetVethName) + // Delete Host Veth if err := client.netlink.DeleteLink(client.vnetVethName); err != nil { return errors.Wrapf(err, "DeleteLink for %v failed: %v", client.vnetVethName) } - routesLeft, err := getNumRoutesLeft() - if err != nil { - return err - } - - log.Printf("[transparent vlan] There are %d routes remaining after deletion", routesLeft) - - // Delete Host Veth - if deleteHostVeth { - if err := client.netlink.DeleteLink(client.vnetVethName); err != nil { - return errors.Wrap(err, "failed to delete host veth") - } - } - // TODO: revist if this require in future. //nolint gocritic /* if routesLeft <= numDefaultRoutes { diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 1be32260c4..9600236324 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -473,7 +473,7 @@ 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, false) + 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()) From f39c515e743ea931f18ee5defc8b0d2f9b72cfbf Mon Sep 17 00:00:00 2001 From: tamanoha Date: Fri, 14 Apr 2023 16:17:53 -0700 Subject: [PATCH 13/14] lint fix --- .../transparent_vlan_endpointclient_linux.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index e45226b64d..c8217719ca 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -27,8 +27,11 @@ const ( DisableRPFilterCmd = "sysctl -w net.ipv4.conf.all.rp_filter=0" // Command to disable the rp filter for tunneling ) +var errNamespaceCreation = fmt.Errorf("Network namespace creation error") + var ( - errNamespaceCreation = fmt.Errorf("Network namespace creation error") + numRetries = 5 + sleepInMs = 100 ) type netnsClient interface { @@ -138,7 +141,7 @@ func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS, numRet if delErr != nil { log.Errorf("failed to cleanup/delete ns after failing to create vlan veth:%v", delErr) } - time.Sleep(100 * time.Millisecond) + time.Sleep(time.Duration(sleepInMs) * time.Millisecond) } if !isNamespaceUnique { @@ -164,7 +167,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // We assume the only possible error is that the namespace doesn't exist log.Printf("[transparent vlan] No existing NS detected. Creating the vnet namespace and switching to it") - if err := client.createNetworkNamespace(vmNS, 5); err != nil { + if err = client.createNetworkNamespace(vmNS, numRetries); err != nil { return errors.Wrap(err, "") } @@ -218,11 +221,11 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er }() // sometimes there is slight delay in interface creation. check if it exists - for i := 0; i < 10; i++ { + for i := 0; i < numRetries; i++ { if _, err = client.netioshim.GetNetworkInterfaceByName(client.vlanIfName); err == nil { break } - time.Sleep(100 * time.Millisecond) + time.Sleep(time.Duration(sleepInMs) * time.Millisecond) } if err != nil { deleteNSIfNotNilErr = errors.Wrapf(err, "failed to get vlan veth interface:%s", client.vlanIfName) @@ -564,7 +567,7 @@ 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, getNumRoutesLeft func() (int, error)) error { +func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, _ func() (int, error)) 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") @@ -573,7 +576,7 @@ func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, g log.Printf("Deleting host veth %v", client.vnetVethName) // Delete Host Veth if err := client.netlink.DeleteLink(client.vnetVethName); err != nil { - return errors.Wrapf(err, "DeleteLink for %v failed: %v", client.vnetVethName) + return errors.Wrapf(err, "deleteLink for %v failed", client.vnetVethName) } // TODO: revist if this require in future. From fe952ca4852681d3c3090384fd058a1c2ad4d243 Mon Sep 17 00:00:00 2001 From: tamanoha Date: Mon, 17 Apr 2023 11:16:37 -0700 Subject: [PATCH 14/14] address comment --- netlink/mocknetlink.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netlink/mocknetlink.go b/netlink/mocknetlink.go index 0b0b4f48db..405038a263 100644 --- a/netlink/mocknetlink.go +++ b/netlink/mocknetlink.go @@ -18,8 +18,8 @@ type routeValidateFn func(route *Route) error type MockNetlink struct { returnError bool errorString string - deleteRouteFn func(route *Route) error - addRouteFn func(route *Route) error + deleteRouteFn routeValidateFn + addRouteFn routeValidateFn } func NewMockNetlink(returnError bool, errorString string) *MockNetlink {