From 41b71225261570b8738afabdd6044575530b39e3 Mon Sep 17 00:00:00 2001 From: Alexander <39818795+QxBytes@users.noreply.github.com> Date: Wed, 4 Dec 2024 11:47:41 -0800 Subject: [PATCH] fix: return error on interface not found and always clean up snat ep (#3226) * return error on interface not found and always clean up snat ep * modify snat client to accept interfaces for unit testing * address linter issues --- network/endpoint.go | 2 +- network/endpoint_linux.go | 1 - network/manager.go | 1 + network/ovs_endpoint_snatroute_linux.go | 1 + network/snat/snat_linux.go | 18 +++++- network/snat/snat_linux_test.go | 57 ++++++++++++++----- ...ansparent_vlan_endpoint_snatroute_linux.go | 1 + .../transparent_vlan_endpointclient_linux.go | 2 +- ...nsparent_vlan_endpointclient_linux_test.go | 8 ++- 9 files changed, 68 insertions(+), 23 deletions(-) diff --git a/network/endpoint.go b/network/endpoint.go index 0dc122ce9c..d06448d389 100644 --- a/network/endpoint.go +++ b/network/endpoint.go @@ -151,7 +151,7 @@ type apipaClient interface { } func (epInfo *EndpointInfo) PrettyString() string { - return fmt.Sprintf("Id:%s ContainerID:%s NetNsPath:%s IfName:%s IfIndex:%d MacAddr:%s IPAddrs:%v Gateways:%v Data:%+v NICType: %s NetworkContainerID: %s HostIfName: %s NetNs: %s Options: %v", + return fmt.Sprintf("EndpointID:%s ContainerID:%s NetNsPath:%s IfName:%s IfIndex:%d MacAddr:%s IPAddrs:%v Gateways:%v Data:%+v NICType: %s NetworkContainerID: %s HostIfName: %s NetNs: %s Options: %v", epInfo.EndpointID, epInfo.ContainerID, epInfo.NetNsPath, epInfo.IfName, epInfo.IfIndex, epInfo.MacAddress.String(), epInfo.IPAddresses, epInfo.Gateways, epInfo.Data, epInfo.NICType, epInfo.NetworkContainerID, epInfo.HostIfName, epInfo.NetNs, epInfo.Options) } diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go index 3f2d8aa771..5f57a66d51 100644 --- a/network/endpoint_linux.go +++ b/network/endpoint_linux.go @@ -279,7 +279,6 @@ func (nw *network) deleteEndpointImpl(nl netlink.NetlinkInterface, plc platform. epInfo := ep.getInfo() if nw.Mode == opModeTransparentVlan { epClient = NewTransparentVlanEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, plc, nsc, iptc) - } else { epClient = NewOVSEndpointClient(nw, epInfo, ep.HostIfName, "", ep.VlanID, ep.LocalIP, nl, ovsctl.NewOvsctl(), plc, iptc) } diff --git a/network/manager.go b/network/manager.go index f1f0771441..e5d6d57330 100644 --- a/network/manager.go +++ b/network/manager.go @@ -439,6 +439,7 @@ func (nm *networkManager) UpdateEndpointState(eps []*endpoint) error { logger.Info("Update endpoint API returend ", zap.String("podname: ", response.ReturnCode.String())) return nil } + func validateUpdateEndpointState(endpointID string, ifNameToIPInfoMap map[string]*restserver.IPInfo) error { if endpointID == "" { return errors.New("endpoint id empty while validating update endpoint state") diff --git a/network/ovs_endpoint_snatroute_linux.go b/network/ovs_endpoint_snatroute_linux.go index 12bd40ab80..3f0858af5a 100644 --- a/network/ovs_endpoint_snatroute_linux.go +++ b/network/ovs_endpoint_snatroute_linux.go @@ -33,6 +33,7 @@ func (client *OVSEndpointClient) NewSnatClient(snatBridgeIP, localIP string, epI client.netlink, client.plClient, client.iptablesClient, + client.netioshim, ) } } diff --git a/network/snat/snat_linux.go b/network/snat/snat_linux.go index 936ce8ef84..76c541c4f6 100644 --- a/network/snat/snat_linux.go +++ b/network/snat/snat_linux.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/cni/log" "github.com/Azure/azure-container-networking/ebtables" "github.com/Azure/azure-container-networking/iptables" + "github.com/Azure/azure-container-networking/netio" "github.com/Azure/azure-container-networking/netlink" "github.com/Azure/azure-container-networking/network/networkutils" "github.com/Azure/azure-container-networking/platform" @@ -55,6 +56,7 @@ type Client struct { netlink netlink.NetlinkInterface plClient platform.ExecClient ipTablesClient ipTablesClient + netioClient netio.NetIOInterface } func NewSnatClient(hostIfName string, @@ -67,6 +69,7 @@ func NewSnatClient(hostIfName string, nl netlink.NetlinkInterface, plClient platform.ExecClient, iptc ipTablesClient, + nio netio.NetIOInterface, ) Client { snatClient := Client{ hostSnatVethName: hostIfName, @@ -78,6 +81,7 @@ func NewSnatClient(hostIfName string, netlink: nl, plClient: plClient, ipTablesClient: iptc, + netioClient: nio, } snatClient.SkipAddressesFromBlock = append(snatClient.SkipAddressesFromBlock, skipAddressesFromBlock...) @@ -223,7 +227,11 @@ func (client *Client) AllowInboundFromHostToNC() error { return newErrorSnatClient(err.Error()) } - snatContainerVeth, _ := net.InterfaceByName(client.containerSnatVethName) + snatContainerVeth, err := client.netioClient.GetNetworkInterfaceByName(client.containerSnatVethName) + if err != nil { + logger.Info("Could not find interface", zap.String("containerSnatVethName", client.containerSnatVethName)) + return errors.Wrap(newErrorSnatClient(err.Error()), "could not find container snat veth name for allow host to nc") + } // Add static arp entry for localIP to prevent arp going out of VM logger.Info("Adding static arp entry for ip", zap.Any("containerIP", containerIP), @@ -319,7 +327,11 @@ func (client *Client) AllowInboundFromNCToHost() error { return err } - snatContainerVeth, _ := net.InterfaceByName(client.containerSnatVethName) + snatContainerVeth, err := client.netioClient.GetNetworkInterfaceByName(client.containerSnatVethName) + if err != nil { + logger.Info("Could not find interface", zap.String("containerSnatVethName", client.containerSnatVethName)) + return errors.Wrap(newErrorSnatClient(err.Error()), "could not find container snat veth name for allow nc to host") + } // Add static arp entry for localIP to prevent arp going out of VM logger.Info("Adding static arp entry for ip", zap.Any("containerIP", containerIP), zap.String("HardwareAddr", snatContainerVeth.HardwareAddr.String())) @@ -416,7 +428,7 @@ func (client *Client) DropArpForSnatBridgeApipaRange(snatBridgeIP, azSnatVethIfN // This function creates linux bridge which will be used for outbound connectivity by NCs func (client *Client) createSnatBridge(snatBridgeIP, hostPrimaryMac string) error { - _, err := net.InterfaceByName(SnatBridgeName) + _, err := client.netioClient.GetNetworkInterfaceByName(SnatBridgeName) if err == nil { logger.Info("Snat Bridge already exists") } else { diff --git a/network/snat/snat_linux_test.go b/network/snat/snat_linux_test.go index 0ffee1ebf2..fcf4a861d3 100644 --- a/network/snat/snat_linux_test.go +++ b/network/snat/snat_linux_test.go @@ -4,12 +4,30 @@ import ( "os" "testing" - "github.com/Azure/azure-container-networking/iptables" + "github.com/Azure/azure-container-networking/netio" "github.com/Azure/azure-container-networking/netlink" ) var anyInterface = "dummy" +type mockIPTablesClient struct{} + +func (c mockIPTablesClient) InsertIptableRule(_, _, _, _, _ string) error { + return nil +} + +func (c mockIPTablesClient) AppendIptableRule(_, _, _, _, _ string) error { + return nil +} + +func (c mockIPTablesClient) DeleteIptableRule(_, _, _, _, _ string) error { + return nil +} + +func (c mockIPTablesClient) CreateChain(_, _, _ string) error { + return nil +} + func TestMain(m *testing.M) { exitCode := m.Run() @@ -18,16 +36,22 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } -func TestAllowInboundFromHostToNC(t *testing.T) { - nl := netlink.NewNetlink() - iptc := iptables.NewClient() - client := &Client{ +func GetTestClient(nl netlink.NetlinkInterface, iptc ipTablesClient, nio netio.NetIOInterface) *Client { + return &Client{ SnatBridgeIP: "169.254.0.1/16", localIP: "169.254.0.4/16", containerSnatVethName: anyInterface, netlink: nl, ipTablesClient: iptc, + netioClient: nio, } +} + +func TestAllowInboundFromHostToNC(t *testing.T) { + nl := netlink.NewMockNetlink(false, "") + iptc := &mockIPTablesClient{} + nio := netio.NewMockNetIO(false, 0) + client := GetTestClient(nl, iptc, nio) if err := nl.AddLink(&netlink.DummyLink{ LinkInfo: netlink.LinkInfo{ @@ -65,18 +89,18 @@ func TestAllowInboundFromHostToNC(t *testing.T) { if err := nl.DeleteLink(SnatBridgeName); err != nil { t.Errorf("Error removing snat bridge: %v", err) } + + client.netioClient = netio.NewMockNetIO(true, 1) + if err := client.AllowInboundFromHostToNC(); err == nil { + t.Errorf("Expected error when interface not found in allow host to nc but got nil") + } } func TestAllowInboundFromNCToHost(t *testing.T) { - nl := netlink.NewNetlink() - iptc := iptables.NewClient() - client := &Client{ - SnatBridgeIP: "169.254.0.1/16", - localIP: "169.254.0.4/16", - containerSnatVethName: anyInterface, - netlink: nl, - ipTablesClient: iptc, - } + nl := netlink.NewMockNetlink(false, "") + iptc := &mockIPTablesClient{} + nio := netio.NewMockNetIO(false, 0) + client := GetTestClient(nl, iptc, nio) if err := nl.AddLink(&netlink.DummyLink{ LinkInfo: netlink.LinkInfo{ @@ -114,4 +138,9 @@ func TestAllowInboundFromNCToHost(t *testing.T) { if err := nl.DeleteLink(SnatBridgeName); err != nil { t.Errorf("Error removing snat bridge: %v", err) } + + client.netioClient = netio.NewMockNetIO(true, 1) + if err := client.AllowInboundFromNCToHost(); err == nil { + t.Errorf("Expected error when interface not found in allow nc to host but got nil") + } } diff --git a/network/transparent_vlan_endpoint_snatroute_linux.go b/network/transparent_vlan_endpoint_snatroute_linux.go index d997ead960..4c3902ddd3 100644 --- a/network/transparent_vlan_endpoint_snatroute_linux.go +++ b/network/transparent_vlan_endpoint_snatroute_linux.go @@ -21,6 +21,7 @@ func (client *TransparentVlanEndpointClient) NewSnatClient(snatBridgeIP, localIP client.netlink, client.plClient, client.iptablesClient, + client.netioshim, ) } } diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 731353c231..acf0b8d204 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -635,7 +635,7 @@ func (client *TransparentVlanEndpointClient) DeleteEndpoints(ep *endpoint) error return client.DeleteEndpointsImpl(ep, getNumRoutesLeft) }) if err != nil { - return err + logger.Warn("could not delete transparent vlan endpoints", zap.String("errorMsg", err.Error())) } // VM NS diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index be64142bc5..1099b97f12 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -15,9 +15,11 @@ import ( "github.com/stretchr/testify/require" ) -var errNetnsMock = errors.New("mock netns error") -var errMockNetIOFail = errors.New("netio fail") -var errMockNetIONoIfFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: errors.New("no such network interface")} +var ( + errNetnsMock = errors.New("mock netns error") + errMockNetIOFail = errors.New("netio fail") + errMockNetIONoIfFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: errors.New("no such network interface")} +) func newNetnsErrorMock(errStr string) error { return errors.Wrap(errNetnsMock, errStr)