From 0e709ad3ed98ead174a33276e465bb976b91153b Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 5 Oct 2023 16:56:05 -0700 Subject: [PATCH 01/17] Make initial changes to enable forwarding Reuse existing code to enable ip forwarding Add forwarding message Add transparent vlan config to dropgz Add enable ip forward to each network namespace for redundancy --- network/endpoint_snatroute_linux.go | 2 +- network/networkutils/networkutils_linux.go | 2 +- network/transparent_vlan_endpointclient_linux.go | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/network/endpoint_snatroute_linux.go b/network/endpoint_snatroute_linux.go index 6b659327c3..3f45a7f16e 100644 --- a/network/endpoint_snatroute_linux.go +++ b/network/endpoint_snatroute_linux.go @@ -37,7 +37,7 @@ func AddSnatEndpointRules(snatClient *snat.Client, hostToNC, ncToHost bool, nl n return errors.Wrap(err, "failed to block ip addresses on snat bridge") } nuc := networkutils.NewNetworkUtils(nl, plc) - if err := nuc.EnableIPForwarding(snat.SnatBridgeName); err != nil { + if err := nuc.EnableIPForwarding(); err != nil { return errors.Wrap(err, "failed to enable ip forwarding") } diff --git a/network/networkutils/networkutils_linux.go b/network/networkutils/networkutils_linux.go index c6b94ddb2e..667d95edee 100644 --- a/network/networkutils/networkutils_linux.go +++ b/network/networkutils/networkutils_linux.go @@ -201,7 +201,7 @@ func BlockIPAddresses(bridgeName, action string) error { } // This fucntion enables ip forwarding in VM and allow forwarding packets from the interface -func (nu NetworkUtils) EnableIPForwarding(ifName string) error { +func (nu NetworkUtils) EnableIPForwarding() error { // Enable ip forwading on linux vm. // sysctl -w net.ipv4.ip_forward=1 cmd := fmt.Sprint(enableIPForwardCmd) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 5532db20d1..9a88a1c47f 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -310,6 +310,11 @@ func (client *TransparentVlanEndpointClient) PopulateVnet(epInfo *EndpointInfo) if err != nil { return errors.Wrap(err, "transparent vlan failed to disable rp filter vlan interface in vnet") } + // Ensure ip forwarding is enabled (even though it should be enabled on network create already) + err = client.netUtilsClient.EnableIPForwarding() + if err != nil { + return errors.Wrap(err, "transparent vlan failed to enable ip forwarding in vnet") + } return nil } From b0a9376dbeee29f9097c80416e4e0bc7443af5b0 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Fri, 20 Oct 2023 17:32:25 -0700 Subject: [PATCH 02/17] Add general run with retries function --- .../transparent_vlan_endpointclient_linux.go | 22 +++++--- ...nsparent_vlan_endpointclient_linux_test.go | 52 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 9a88a1c47f..832acc6f3a 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -26,14 +26,11 @@ const ( tunnelingTable = 2 // Packets not entering on the vlan interface go to this routing table tunnelingMark = 333 // The packets that are to tunnel will be marked with this number DisableRPFilterCmd = "sysctl -w net.ipv4.conf.all.rp_filter=0" // Command to disable the rp filter for tunneling + numRetries = 5 + sleepInMs = 100 ) -var errNamespaceCreation = fmt.Errorf("Network namespace creation error") - -var ( - numRetries = 5 - sleepInMs = 100 -) +var errNamespaceCreation = fmt.Errorf("network namespace creation error") type netnsClient interface { Get() (fileDescriptor int, err error) @@ -642,3 +639,16 @@ func ExecuteInNS(nsc NamespaceClientInterface, nsName string, f func() error) er }() return f() } + +func RunWithRetries(f func() error, maxRuns int, sleepMs int) error { + var err error = nil + for i := 0; i < maxRuns; i++ { + err = f() + if err == nil { + break + } + logger.Info("Function failed, retrying after delay...", zap.Error(err)) + time.Sleep(time.Duration(sleepMs) * time.Millisecond) + } + return err +} diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 9600236324..d94f88962e 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -689,3 +689,55 @@ func TestTransparentVlanConfigureContainerInterfacesAndRoutes(t *testing.T) { }) } } +func createFunctionWithFailurePattern(errorPattern []error) func() error { + s := 0 + return func() error { + if s >= len(errorPattern) { + return nil + } + result := errorPattern[s] + s += 1 + return result + } +} +func TestRunWithRetries(t *testing.T) { + var errMock = errors.New("mock error") + var runs = 4 + tests := []struct { + name string + wantErr bool + f func() error + }{ + { + name: "Succeed on first try", + f: createFunctionWithFailurePattern([]error{}), + wantErr: false, + }, + { + name: "Succeed on first try do not check again", + f: createFunctionWithFailurePattern([]error{nil, errMock, errMock, errMock}), + wantErr: false, + }, + { + name: "Succeed on last try", + f: createFunctionWithFailurePattern([]error{errMock, errMock, errMock, nil, errMock}), + wantErr: false, + }, + { + name: "Fail after too many attempts", + f: createFunctionWithFailurePattern([]error{errMock, errMock, errMock, errMock, nil, nil}), + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + err := RunWithRetries(tt.f, runs, 100) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} From a850fda7229c76cc6a17d4ffe274be979f80d5ab Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 23 Oct 2023 08:59:28 -0700 Subject: [PATCH 03/17] Migrate existing code to use retry function --- .../transparent_vlan_endpointclient_linux.go | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 832acc6f3a..e83c230acb 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -123,10 +123,8 @@ func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) }) } -func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS, numRetries int) error { - var isNamespaceUnique bool - - for i := 0; i < numRetries; i++ { +func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) error { + createNsImpl := func() error { vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) if err != nil { return errors.Wrap(err, "failed to create vnet ns") @@ -134,22 +132,19 @@ func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS, numRet logger.Info("Vnet Namespace created", zap.String("vnetNS", client.netnsClient.NamespaceUniqueID(vnetNS))) if !client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { client.vnetNSFileDescriptor = vnetNS - isNamespaceUnique = true - break + return nil } + // the vnet and vm namespace are the same by this point logger.Info("Vnet Namespace is the same as VM namespace. Deleting and retrying...") delErr := client.netnsClient.DeleteNamed(client.vnetNSName) if delErr != nil { logger.Error("failed to cleanup/delete ns after failing to create vlan veth", zap.Any("error:", delErr.Error())) } - time.Sleep(time.Duration(sleepInMs) * time.Millisecond) + return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are the same") } + err := RunWithRetries(createNsImpl, numRetries, sleepInMs) - if !isNamespaceUnique { - return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are same") - } - - return nil + return err } // Called from AddEndpoints, Namespace: VM @@ -168,7 +163,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // We assume the only possible error is that the namespace doesn't exist logger.Info("No existing NS detected. Creating the vnet namespace and switching to it") - if err = client.createNetworkNamespace(vmNS, numRetries); err != nil { + if err = client.createNetworkNamespace(vmNS); err != nil { return errors.Wrap(err, "") } @@ -222,12 +217,11 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er }() // sometimes there is slight delay in interface creation. check if it exists - for i := 0; i < numRetries; i++ { - if _, err = client.netioshim.GetNetworkInterfaceByName(client.vlanIfName); err == nil { - break - } - time.Sleep(time.Duration(sleepInMs) * time.Millisecond) - } + err = RunWithRetries(func() error { + _, err = client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) + return err + }, numRetries, sleepInMs) + if err != nil { deleteNSIfNotNilErr = errors.Wrapf(err, "failed to get vlan veth interface:%s", client.vlanIfName) return deleteNSIfNotNilErr From a9c6449d77dc37303c9d73b802571e680b1c6de3 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 23 Oct 2023 17:51:47 -0700 Subject: [PATCH 04/17] Add retries for checking of vnet and container veth creation --- .../transparent_vlan_endpointclient_linux.go | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index e83c230acb..41c0381067 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -251,6 +251,28 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er if err = client.netUtilsClient.CreateEndpoint(client.vnetVethName, client.containerVethName, mac); err != nil { return errors.Wrap(err, "failed to create veth pair") } + + // Ensure vnet veth is created, as there may be a slight delay + err = RunWithRetries(func() error { + var getErr error + _, getErr = client.netioshim.GetNetworkInterfaceByName(client.vnetVethName) + return getErr + }, numRetries, sleepInMs) + if err != nil { + return errors.Wrap(err, "vnet veth does not exist") + } + + // Ensure container veth is created, as there may be a slight delay + var containerIf *net.Interface = nil + err = RunWithRetries(func() error { + var getErr error + containerIf, getErr = client.netioshim.GetNetworkInterfaceByName(client.containerVethName) + return getErr + }, numRetries, sleepInMs) + if err != nil { + return errors.Wrap(err, "container veth does not exist") + } + // Disable RA for veth pair, and delete if any failure if err = client.netUtilsClient.DisableRAForInterface(client.vnetVethName); err != nil { if delErr := client.netlink.DeleteLink(client.vnetVethName); delErr != nil { @@ -272,10 +294,6 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er return errors.Wrap(err, "failed to move vnetVethName into vnet ns, deleting") } - containerIf, err := client.netioshim.GetNetworkInterfaceByName(client.containerVethName) - if err != nil { - return errors.Wrap(err, "container veth does not exist") - } client.containerMac = containerIf.HardwareAddr return nil } From c2bfac3a04e3e85db94210ee63e877b2281c4383 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Tue, 24 Oct 2023 18:29:17 -0700 Subject: [PATCH 05/17] Add preliminary repair item fixes --- .../transparent_vlan_endpointclient_linux.go | 55 ++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 41c0381067..aa0a10a922 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -123,6 +123,42 @@ func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) }) } +// Called from PopulateVM, Namespace: VM and Vnet +func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { + // Clean up vlan interface in the VM namespace and ensure the network namespace (if it exists) has a vlan interface + logger.Info("Checking if NS and vlan veth exists...") + var existingErr error + client.vnetNSFileDescriptor, existingErr = client.netnsClient.GetFromName(client.vnetNSName) + if existingErr == nil { + // The namespace exists + vlanIfNotFoundErr := ExecuteInNS(client.vnetNSName, func() error { + // Ensure the vlan interface exists in the namespace + _, err := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) + return err + }) + if vlanIfNotFoundErr != nil { + logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...") + delErr := client.netnsClient.DeleteNamed(client.vnetNSName) + if delErr != nil { + return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") + } + } + logger.Info("Veth interface was found in namespace") + } + // Delete the vlan interface in the VM namespace if it exists + _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) + if vlanIfInVMErr == nil { + // The vlan interface exists in the VM ns because it failed to move into the network ns previously and needs to be cleaned up + logger.Info("Vlan interface exists on the VM namespace, deleting", zap.String("vlanIfName", client.vlanIfName)) + // TODO: Failing to clean up the vlan interface isn't necessarily fatal is it? + if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil { + return errors.Wrap(delErr, "failed to clean up vlan interface") + } + } + return nil +} + +// Called from PopulateVM, Namespace: VM func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) error { createNsImpl := func() error { vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) @@ -153,6 +189,9 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er if err != nil { return errors.Wrap(err, "failed to get vm ns handle") } + if err := client.ensureCleanPopulateVM(); err != nil { + return errors.Wrap(err, "failed to ensure both network namespace and vlan veth were both present or both absent") + } logger.Info("Checking if NS exists...") var existingErr error @@ -237,8 +276,22 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er if deleteNSIfNotNilErr != nil { return errors.Wrap(deleteNSIfNotNilErr, "deleting vlan veth in vm ns due to addendpoint failure") } + + // confirm vlan veth was moved successfully + deleteNSIfNotNilErr = RunWithRetries(func() error { + // retry checking in the namespace if the vlan interface is not detected + return ExecuteInNS(client.vnetNSName, func() error { + _, vlanIfExistsErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) + // errors if the vlan interface does not exist + return vlanIfExistsErr + }) + }, numRetries, sleepInMs) + if deleteNSIfNotNilErr != nil { + return errors.Wrap(deleteNSIfNotNilErr, "failed to detect vlan veth inside vnet namespace") + } + logger.Info("Moving vlan veth into namespace confirmed") } else { - logger.Info("Existing NS detected. Assuming exists too", zap.String("vnetNSName", client.vnetNSName), zap.String("vlanIfName", client.vlanIfName)) + logger.Info("Existing NS detected. Vlan interface should exist or namespace would've been deleted.", zap.String("vnetNSName", client.vnetNSName), zap.String("vlanIfName", client.vlanIfName)) } // Get the default constant host veth mac From 043e795774d1a5fc6a0a5a7acd0a64c0132831d4 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Tue, 24 Oct 2023 18:43:46 -0700 Subject: [PATCH 06/17] Clarify log message --- network/transparent_vlan_endpointclient_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index aa0a10a922..b0121b37de 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -143,7 +143,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") } } - logger.Info("Veth interface was found in namespace") + logger.Info("Vlan veth interface was found in namespace") } // Delete the vlan interface in the VM namespace if it exists _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) From d78215de05e9f5a631183dffe1f297c634416972 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 26 Oct 2023 11:46:08 -0700 Subject: [PATCH 07/17] Fix unit tests, move ensure clean populate vm to add endpoints, remove ip forwarding fix as it is in other branch --- .../transparent_vlan_endpointclient_linux.go | 26 ++-- ...nsparent_vlan_endpointclient_linux_test.go | 112 +++++++++++++++++- 2 files changed, 121 insertions(+), 17 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index b0121b37de..ae1a882310 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -110,8 +110,10 @@ func NewTransparentVlanEndpointClient( // Adds interfaces to the vnet (created if not existing) and vm namespace func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) error { // VM Namespace - err := client.PopulateVM(epInfo) - if err != nil { + if err := client.ensureCleanPopulateVM(); err != nil { + return errors.Wrap(err, "failed to ensure both network namespace and vlan veth were both present or both absent") + } + if err := client.PopulateVM(epInfo); err != nil { return err } if err := client.AddSnatEndpoint(); err != nil { @@ -131,7 +133,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { client.vnetNSFileDescriptor, existingErr = client.netnsClient.GetFromName(client.vnetNSName) if existingErr == nil { // The namespace exists - vlanIfNotFoundErr := ExecuteInNS(client.vnetNSName, func() error { + vlanIfNotFoundErr := ExecuteInNS(client.nsClient, client.vnetNSName, func() error { // Ensure the vlan interface exists in the namespace _, err := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) return err @@ -143,7 +145,6 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") } } - logger.Info("Vlan veth interface was found in namespace") } // Delete the vlan interface in the VM namespace if it exists _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) @@ -161,6 +162,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { // Called from PopulateVM, Namespace: VM func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) error { createNsImpl := func() error { + // If this call succeeds, the namespace is set to the new namespace vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) if err != nil { return errors.Wrap(err, "failed to create vnet ns") @@ -174,7 +176,7 @@ func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) er logger.Info("Vnet Namespace is the same as VM namespace. Deleting and retrying...") delErr := client.netnsClient.DeleteNamed(client.vnetNSName) if delErr != nil { - logger.Error("failed to cleanup/delete ns after failing to create vlan veth", zap.Any("error:", delErr.Error())) + logger.Error("failed to cleanup/delete ns after noticing vnet ns is the same as vm ns", zap.Any("error:", delErr.Error())) } return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are the same") } @@ -189,9 +191,6 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er if err != nil { return errors.Wrap(err, "failed to get vm ns handle") } - if err := client.ensureCleanPopulateVM(); err != nil { - return errors.Wrap(err, "failed to ensure both network namespace and vlan veth were both present or both absent") - } logger.Info("Checking if NS exists...") var existingErr error @@ -245,6 +244,8 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // 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") } + // Prevent accidentally deleting NS and vlan interface since we ignore this error + deleteNSIfNotNilErr = nil } defer func() { if deleteNSIfNotNilErr != nil { @@ -280,7 +281,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // confirm vlan veth was moved successfully deleteNSIfNotNilErr = RunWithRetries(func() error { // retry checking in the namespace if the vlan interface is not detected - return ExecuteInNS(client.vnetNSName, func() error { + return ExecuteInNS(client.nsClient, client.vnetNSName, func() error { _, vlanIfExistsErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) // errors if the vlan interface does not exist return vlanIfExistsErr @@ -372,11 +373,6 @@ func (client *TransparentVlanEndpointClient) PopulateVnet(epInfo *EndpointInfo) if err != nil { return errors.Wrap(err, "transparent vlan failed to disable rp filter vlan interface in vnet") } - // Ensure ip forwarding is enabled (even though it should be enabled on network create already) - err = client.netUtilsClient.EnableIPForwarding() - if err != nil { - return errors.Wrap(err, "transparent vlan failed to enable ip forwarding in vnet") - } return nil } @@ -712,7 +708,7 @@ func RunWithRetries(f func() error, maxRuns int, sleepMs int) error { if err == nil { break } - logger.Info("Function failed, retrying after delay...", zap.Error(err)) + logger.Info("Retrying after delay...", zap.String("error", err.Error())) time.Sleep(time.Duration(sleepMs) * time.Millisecond) } return err diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index d94f88962e..a9fb013385 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/azure-container-networking/platform" "github.com/pkg/errors" "github.com/stretchr/testify/require" + "go.uber.org/zap" ) var errNetnsMock = errors.New("mock netns error") @@ -77,6 +78,38 @@ func defaultDeleteNamed(name string) error { return nil } +// This mock netioshim provides more flexbility in when it errors compared to the one in the netio package +type mockNetIO struct { + existingInterfaces map[string]bool +} + +func (ns *mockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, error) { + var ErrMockNetIOFail = errors.New("netio fail") + if ns.existingInterfaces[name] { + hwAddr, _ := net.ParseMAC("ab:cd:ef:12:34:56") + return &net.Interface{ + //nolint:gomnd // Dummy MTU + MTU: 1000, + Name: name, + HardwareAddr: hwAddr, + //nolint:gomnd // Dummy interface index + Index: 2, + }, nil + } else { + return nil, errors.Wrap(ErrMockNetIOFail, name) + } +} +func (ns *mockNetIO) GetNetworkInterfaceAddrs(iface *net.Interface) ([]net.Addr, error) { + return []net.Addr{}, nil +} + +func defaultExecuteInNSFn(nsName string, f func() error) error { + logger.Info("[MockExecuteInNS] Entering ns", zap.String("nsName", nsName)) + defer func() { + logger.Info("[MockExecuteInNS] Exiting ns", zap.String("nsName", nsName)) + }() + return f() +} func TestTransparentVlanAddEndpoints(t *testing.T) { nl := netlink.NewMockNetlink(false, "") plc := platform.NewMockExecClient(false) @@ -87,6 +120,75 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { epInfo *EndpointInfo wantErr bool wantErrMsg string + }{ + // Ensuring vnet namespace and vlan both exist or are both absent before populating the vm + { + name: "Ensure clean populate VM neither vnet ns nor vlan if exists", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netnsClient: &mockNetns{ + get: defaultGet, + getFromName: func(name string) (fileDescriptor int, err error) { + return 0, newNetnsErrorMock("netns failure") + }, + }, + netlink: netlink.NewMockNetlink(false, ""), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: netio.NewMockNetIO(false, 0), + executeInNSFn: defaultExecuteInNSFn, + }, + epInfo: &EndpointInfo{}, + wantErr: false, + }, + { + name: "Ensure clean populate VM vnet ns but vlan if does not", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netnsClient: &mockNetns{ + get: defaultGet, + getFromName: defaultGetFromName, + deleteNamed: func(name string) (err error) { + return newNetnsErrorMock("netns failure") + }, + }, + netlink: netlink.NewMockNetlink(false, ""), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: netio.NewMockNetIO(true, 1), + executeInNSFn: defaultExecuteInNSFn, + }, + epInfo: &EndpointInfo{}, + wantErr: true, + wantErrMsg: "failed to cleanup/delete ns after noticing vlan veth does not exist: netns failure: " + errNetnsMock.Error(), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + err := tt.client.ensureCleanPopulateVM() + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErrMsg, "Expected:%v actual:%v", tt.wantErrMsg, err.Error()) + } else { + require.NoError(t, err) + } + }) + } + tests = []struct { + name string + client *TransparentVlanEndpointClient + epInfo *EndpointInfo + wantErr bool + wantErrMsg string }{ // Populating VM with data and creating interfaces/links { @@ -206,11 +308,16 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { netlink: netlink.NewMockNetlink(false, ""), plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), - netioshim: netio.NewMockNetIO(true, 1), + netioshim: &mockNetIO{ + existingInterfaces: map[string]bool{ + "A1veth0": true, + }, + }, + executeInNSFn: defaultExecuteInNSFn, }, epInfo: &EndpointInfo{}, wantErr: true, - wantErrMsg: "container veth does not exist: " + netio.ErrMockNetIOFail.Error() + ":B1veth0", + wantErrMsg: "container veth does not exist: B1veth0: " + netio.ErrMockNetIOFail.Error() + "", }, { name: "Add endpoints NetNS Get fail", @@ -703,6 +810,7 @@ func createFunctionWithFailurePattern(errorPattern []error) func() error { func TestRunWithRetries(t *testing.T) { var errMock = errors.New("mock error") var runs = 4 + tests := []struct { name string wantErr bool From b6ff6713145992c7582a29409c48cc1b70ba0fff Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 26 Oct 2023 14:57:46 -0700 Subject: [PATCH 08/17] Move setting link netns and confirmation to function --- .../transparent_vlan_endpointclient_linux.go | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index ae1a882310..0f4dc85429 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -151,7 +151,6 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { if vlanIfInVMErr == nil { // The vlan interface exists in the VM ns because it failed to move into the network ns previously and needs to be cleaned up logger.Info("Vlan interface exists on the VM namespace, deleting", zap.String("vlanIfName", client.vlanIfName)) - // TODO: Failing to clean up the vlan interface isn't necessarily fatal is it? if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil { return errors.Wrap(delErr, "failed to clean up vlan interface") } @@ -185,6 +184,28 @@ func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) er return err } +// Called from PopulateVM, Namespace: VM and namespace represented by fd +func (client *TransparentVlanEndpointClient) setLinkNetNSAndConfirm(name string, fd uintptr) error { + logger.Info("Move link to NS", zap.String("ifName", name), zap.Any("NSFileDescriptor", fd)) + err := client.netlink.SetLinkNetNs(name, fd) + if err != nil { + return errors.Wrapf(err, "failed to set %v inside namespace %v", name, fd) + } + + // confirm veth was moved successfully + err = RunWithRetries(func() error { + // retry checking in the namespace if the interface is not detected + return client.executeInNSFn(client.vnetNSName, func() error { + _, ifDetectedErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) + return ifDetectedErr + }) + }, numRetries, sleepInMs) + if err != nil { + return errors.Wrapf(err, "failed to detect %v inside namespace %v", name, fd) + } + return nil +} + // Called from AddEndpoints, Namespace: VM func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) error { vmNS, err := client.netnsClient.Get() @@ -273,22 +294,9 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } // vlan veth was created successfully, so move the vlan veth you created logger.Info("Move vlan link to vnet NS", zap.String("vlanIfName", client.vlanIfName), zap.Any("vnetNSFileDescriptor", uintptr(client.vnetNSFileDescriptor))) - deleteNSIfNotNilErr = client.netlink.SetLinkNetNs(client.vlanIfName, uintptr(client.vnetNSFileDescriptor)) - if deleteNSIfNotNilErr != nil { - return errors.Wrap(deleteNSIfNotNilErr, "deleting vlan veth in vm ns due to addendpoint failure") - } - - // confirm vlan veth was moved successfully - deleteNSIfNotNilErr = RunWithRetries(func() error { - // retry checking in the namespace if the vlan interface is not detected - return ExecuteInNS(client.nsClient, client.vnetNSName, func() error { - _, vlanIfExistsErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) - // errors if the vlan interface does not exist - return vlanIfExistsErr - }) - }, numRetries, sleepInMs) + deleteNSIfNotNilErr = client.setLinkNetNSAndConfirm(client.vlanIfName, uintptr(client.vnetNSFileDescriptor)) if deleteNSIfNotNilErr != nil { - return errors.Wrap(deleteNSIfNotNilErr, "failed to detect vlan veth inside vnet namespace") + return errors.Wrap(deleteNSIfNotNilErr, "failed to move or detect vlan veth inside vnet namespace") } logger.Info("Moving vlan veth into namespace confirmed") } else { @@ -341,13 +349,12 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er return errors.Wrap(err, "failed to disable RA on container veth, deleting") } - if err = client.netlink.SetLinkNetNs(client.vnetVethName, uintptr(client.vnetNSFileDescriptor)); err != nil { + if err = client.setLinkNetNSAndConfirm(client.vnetVethName, uintptr(client.vnetNSFileDescriptor)); err != nil { if delErr := client.netlink.DeleteLink(client.vnetVethName); delErr != nil { logger.Error("Deleting vnet veth failed on addendpoint failure with", zap.Error(delErr)) } - return errors.Wrap(err, "failed to move vnetVethName into vnet ns, deleting") + return errors.Wrap(err, "failed to move or detect vnetVethName in vnet ns, deleting") } - client.containerMac = containerIf.HardwareAddr return nil } @@ -555,7 +562,7 @@ func (client *TransparentVlanEndpointClient) GetVnetRoutes(ipAddresses []net.IPN // Helper that creates routing rules for the current NS which direct packets // to the virtual gateway ip on linkToName device interface -// Route 1: 169.254.1.1 dev +// Route 1: 169.254.2.1 dev // Route 2: default via 169.254.2.1 dev func (client *TransparentVlanEndpointClient) addDefaultRoutes(linkToName string, table int) error { // Add route for virtualgwip (ip route add 169.254.2.1/32 dev eth0) From 2e555e11d779d7f055b107ce2f8aaf6592cca448 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 26 Oct 2023 15:48:10 -0700 Subject: [PATCH 09/17] Add tests for setLinkNetNSAndConfirm --- ...nsparent_vlan_endpointclient_linux_test.go | 82 ++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index a9fb013385..aa9f5f7562 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -120,6 +120,83 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { epInfo *EndpointInfo wantErr bool wantErrMsg string + }{ + // Set the link network namespace and confirm that it was moved inside + { + name: "Set link netns good path", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netlink: netlink.NewMockNetlink(false, ""), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: netio.NewMockNetIO(false, 0), + executeInNSFn: defaultExecuteInNSFn, + }, + epInfo: &EndpointInfo{}, + wantErr: false, + }, + { + name: "Set link netns fail to set", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netlink: netlink.NewMockNetlink(true, "netlink fail"), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: netio.NewMockNetIO(false, 0), + executeInNSFn: defaultExecuteInNSFn, + }, + epInfo: &EndpointInfo{}, + wantErr: true, + wantErrMsg: "failed to set eth0.1", + }, + { + name: "Set link netns fail to detect", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netlink: netlink.NewMockNetlink(false, ""), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: &mockNetIO{ + existingInterfaces: map[string]bool{}, + }, + executeInNSFn: defaultExecuteInNSFn, + }, + epInfo: &EndpointInfo{}, + wantErr: true, + wantErrMsg: "failed to detect eth0.1", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + err := tt.client.setLinkNetNSAndConfirm(tt.client.vlanIfName, 1) + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErrMsg, "Expected:%v actual:%v", tt.wantErrMsg, err.Error()) + } else { + require.NoError(t, err) + } + }) + } + + tests = []struct { + name string + client *TransparentVlanEndpointClient + epInfo *EndpointInfo + wantErr bool + wantErrMsg string }{ // Ensuring vnet namespace and vlan both exist or are both absent before populating the vm { @@ -236,6 +313,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(false, 0), + executeInNSFn: defaultExecuteInNSFn, }, epInfo: &EndpointInfo{}, wantErr: false, @@ -262,7 +340,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { }, epInfo: &EndpointInfo{}, wantErr: true, - wantErrMsg: "failed to move vnetVethName into vnet ns, deleting: " + netlink.ErrorMockNetlink.Error() + " : netlink fail", + wantErrMsg: "failed to move or detect vnetVethName in vnet ns, deleting: failed to set A1veth0 inside namespace 1: " + netlink.ErrorMockNetlink.Error() + " : netlink fail", }, { name: "Add endpoints get interface fail for primary interface (eth0)", @@ -342,7 +420,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { wantErrMsg: "failed to get vm ns handle: netns failure: " + errNetnsMock.Error(), }, { - name: "Add endpoints NetNS Set fail", + name: "Add endpoints no vnet ns NetNS Set fail", client: &TransparentVlanEndpointClient{ primaryHostIfName: "eth0", vlanIfName: "eth0.1", From 9964f2fd8187cc56ddaec573ef4c7e5aa86422d4 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Fri, 27 Oct 2023 11:26:38 -0700 Subject: [PATCH 10/17] Address linter issues --- .../transparent_vlan_endpointclient_linux.go | 19 +++++++++---------- ...nsparent_vlan_endpointclient_linux_test.go | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 0f4dc85429..a51b607f1e 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -136,7 +136,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { vlanIfNotFoundErr := ExecuteInNS(client.nsClient, client.vnetNSName, func() error { // Ensure the vlan interface exists in the namespace _, err := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) - return err + return errors.Wrap(err, "failed to get vlan interface in namespace") }) if vlanIfNotFoundErr != nil { logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...") @@ -197,7 +197,7 @@ func (client *TransparentVlanEndpointClient) setLinkNetNSAndConfirm(name string, // retry checking in the namespace if the interface is not detected return client.executeInNSFn(client.vnetNSName, func() error { _, ifDetectedErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) - return ifDetectedErr + return errors.Wrap(ifDetectedErr, "failed to get vlan veth in namespace") }) }, numRetries, sleepInMs) if err != nil { @@ -280,7 +280,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // sometimes there is slight delay in interface creation. check if it exists err = RunWithRetries(func() error { _, err = client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) - return err + return errors.Wrap(err, "failed to get vlan veth") }, numRetries, sleepInMs) if err != nil { @@ -316,20 +316,19 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // Ensure vnet veth is created, as there may be a slight delay err = RunWithRetries(func() error { - var getErr error - _, getErr = client.netioshim.GetNetworkInterfaceByName(client.vnetVethName) - return getErr + _, getErr := client.netioshim.GetNetworkInterfaceByName(client.vnetVethName) + return errors.Wrap(getErr, "failed to get vnet veth") }, numRetries, sleepInMs) if err != nil { return errors.Wrap(err, "vnet veth does not exist") } // Ensure container veth is created, as there may be a slight delay - var containerIf *net.Interface = nil + var containerIf *net.Interface err = RunWithRetries(func() error { var getErr error containerIf, getErr = client.netioshim.GetNetworkInterfaceByName(client.containerVethName) - return getErr + return errors.Wrap(getErr, "failed to get container veth") }, numRetries, sleepInMs) if err != nil { return errors.Wrap(err, "container veth does not exist") @@ -708,8 +707,8 @@ func ExecuteInNS(nsc NamespaceClientInterface, nsName string, f func() error) er return f() } -func RunWithRetries(f func() error, maxRuns int, sleepMs int) error { - var err error = nil +func RunWithRetries(f func() error, maxRuns, sleepMs int) error { + var err error for i := 0; i < maxRuns; i++ { err = f() if err == nil { diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index aa9f5f7562..400b2b6354 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -84,7 +84,7 @@ type mockNetIO struct { } func (ns *mockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, error) { - var ErrMockNetIOFail = errors.New("netio fail") + ErrMockNetIOFail := errors.New("netio fail") if ns.existingInterfaces[name] { hwAddr, _ := net.ParseMAC("ab:cd:ef:12:34:56") return &net.Interface{ @@ -95,11 +95,11 @@ func (ns *mockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, err //nolint:gomnd // Dummy interface index Index: 2, }, nil - } else { - return nil, errors.Wrap(ErrMockNetIOFail, name) } + return nil, errors.Wrap(ErrMockNetIOFail, name) } -func (ns *mockNetIO) GetNetworkInterfaceAddrs(iface *net.Interface) ([]net.Addr, error) { + +func (ns *mockNetIO) GetNetworkInterfaceAddrs(_ *net.Interface) ([]net.Addr, error) { return []net.Addr{}, nil } @@ -110,6 +110,7 @@ func defaultExecuteInNSFn(nsName string, f func() error) error { }() return f() } + func TestTransparentVlanAddEndpoints(t *testing.T) { nl := netlink.NewMockNetlink(false, "") plc := platform.NewMockExecClient(false) @@ -395,7 +396,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { }, epInfo: &EndpointInfo{}, wantErr: true, - wantErrMsg: "container veth does not exist: B1veth0: " + netio.ErrMockNetIOFail.Error() + "", + wantErrMsg: "container veth does not exist: failed to get container veth: B1veth0: " + netio.ErrMockNetIOFail.Error() + "", }, { name: "Add endpoints NetNS Get fail", @@ -874,6 +875,7 @@ func TestTransparentVlanConfigureContainerInterfacesAndRoutes(t *testing.T) { }) } } + func createFunctionWithFailurePattern(errorPattern []error) func() error { s := 0 return func() error { @@ -881,13 +883,14 @@ func createFunctionWithFailurePattern(errorPattern []error) func() error { return nil } result := errorPattern[s] - s += 1 + s++ return result } } + func TestRunWithRetries(t *testing.T) { - var errMock = errors.New("mock error") - var runs = 4 + errMock := errors.New("mock error") + runs := 4 tests := []struct { name string From b162514ad2316da4281361bd34ea0a2b82529027 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 30 Oct 2023 16:15:17 -0700 Subject: [PATCH 11/17] Add vm ns to vnet namespace log statement --- network/transparent_vlan_endpointclient_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index a51b607f1e..592653a3d8 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -166,7 +166,7 @@ func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) er if err != nil { return errors.Wrap(err, "failed to create vnet ns") } - logger.Info("Vnet Namespace created", zap.String("vnetNS", client.netnsClient.NamespaceUniqueID(vnetNS))) + logger.Info("Vnet Namespace created", zap.String("vnetNS", client.netnsClient.NamespaceUniqueID(vnetNS)), zap.String("vmNS", client.netnsClient.NamespaceUniqueID(vmNS))) if !client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { client.vnetNSFileDescriptor = vnetNS return nil From c5e78c3c57fed9b3d0d5555a5d9e1a506b99f2ee Mon Sep 17 00:00:00 2001 From: QxBytes Date: Wed, 1 Nov 2023 15:42:20 -0700 Subject: [PATCH 12/17] Fix after rebasing in swift refactoring changes from master --- .../transparent_vlan_endpointclient_linux.go | 2 +- ...nsparent_vlan_endpointclient_linux_test.go | 30 ++++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 592653a3d8..fe8d752e50 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -195,7 +195,7 @@ func (client *TransparentVlanEndpointClient) setLinkNetNSAndConfirm(name string, // confirm veth was moved successfully err = RunWithRetries(func() error { // retry checking in the namespace if the interface is not detected - return client.executeInNSFn(client.vnetNSName, func() error { + return ExecuteInNS(client.nsClient, client.vnetNSName, func() error { _, ifDetectedErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) return errors.Wrap(ifDetectedErr, "failed to get vlan veth in namespace") }) diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 400b2b6354..fe834ecd74 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -13,7 +13,6 @@ import ( "github.com/Azure/azure-container-networking/platform" "github.com/pkg/errors" "github.com/stretchr/testify/require" - "go.uber.org/zap" ) var errNetnsMock = errors.New("mock netns error") @@ -103,12 +102,15 @@ func (ns *mockNetIO) GetNetworkInterfaceAddrs(_ *net.Interface) ([]net.Addr, err return []net.Addr{}, nil } -func defaultExecuteInNSFn(nsName string, f func() error) error { - logger.Info("[MockExecuteInNS] Entering ns", zap.String("nsName", nsName)) - defer func() { - logger.Info("[MockExecuteInNS] Exiting ns", zap.String("nsName", nsName)) - }() - return f() +func (ns *mockNetIO) GetNetworkInterfaceByMac(mac net.HardwareAddr) (*net.Interface, error) { + return &net.Interface{ + //nolint:gomnd // Dummy MTU + MTU: 1000, + Name: "eth1", + HardwareAddr: mac, + //nolint:gomnd // Dummy interface index + Index: 2, + }, nil } func TestTransparentVlanAddEndpoints(t *testing.T) { @@ -135,7 +137,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(false, 0), - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: false, @@ -152,7 +154,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(false, 0), - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, @@ -172,7 +174,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { netioshim: &mockNetIO{ existingInterfaces: map[string]bool{}, }, - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, @@ -218,7 +220,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(false, 0), - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: false, @@ -242,7 +244,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(true, 1), - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, @@ -314,7 +316,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: netio.NewMockNetIO(false, 0), - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: false, @@ -392,7 +394,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { "A1veth0": true, }, }, - executeInNSFn: defaultExecuteInNSFn, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, From 88ddd8e95bfedf524fa1666d67ab2b144bba75d7 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Fri, 3 Nov 2023 15:55:22 -0700 Subject: [PATCH 13/17] Address feedback and check error message --- network/networkutils/networkutils_linux.go | 2 +- .../transparent_vlan_endpointclient_linux.go | 26 ++++++---- ...nsparent_vlan_endpointclient_linux_test.go | 47 ++++++++++++++++--- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/network/networkutils/networkutils_linux.go b/network/networkutils/networkutils_linux.go index 667d95edee..4d7473b0b5 100644 --- a/network/networkutils/networkutils_linux.go +++ b/network/networkutils/networkutils_linux.go @@ -200,7 +200,7 @@ func BlockIPAddresses(bridgeName, action string) error { return nil } -// This fucntion enables ip forwarding in VM and allow forwarding packets from the interface +// This function enables ip forwarding in VM and allow forwarding packets from the interface func (nu NetworkUtils) EnableIPForwarding() error { // Enable ip forwading on linux vm. // sysctl -w net.ipv4.ip_forward=1 diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index fe8d752e50..98146b60ae 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -111,7 +111,7 @@ func NewTransparentVlanEndpointClient( func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) error { // VM Namespace if err := client.ensureCleanPopulateVM(); err != nil { - return errors.Wrap(err, "failed to ensure both network namespace and vlan veth were both present or both absent") + return errors.Wrap(err, "failed to ensure both network namespace and vlan veth were present or both absent") } if err := client.PopulateVM(epInfo); err != nil { return err @@ -133,18 +133,26 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { client.vnetNSFileDescriptor, existingErr = client.netnsClient.GetFromName(client.vnetNSName) if existingErr == nil { // The namespace exists - vlanIfNotFoundErr := ExecuteInNS(client.nsClient, client.vnetNSName, func() error { + vlanIfErr := ExecuteInNS(client.nsClient, client.vnetNSName, func() error { // Ensure the vlan interface exists in the namespace _, err := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) return errors.Wrap(err, "failed to get vlan interface in namespace") }) - if vlanIfNotFoundErr != nil { - logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...") - delErr := client.netnsClient.DeleteNamed(client.vnetNSName) - if delErr != nil { - return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") + + // If the vlan veth for sure doesn't exist, we delete the vnet ns, but if we can't query, we can't delete the ns (the vlan veth might exist!) + if vlanIfErr != nil { + if strings.Contains(vlanIfErr.Error(), "no such network interface") { + logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...") + delErr := client.netnsClient.DeleteNamed(client.vnetNSName) + if delErr != nil { + return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") + } + } else { + return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") } } + } else { + logger.Info("Expecting vnet namespace not to exist", zap.String("result", existingErr.Error())) } // Delete the vlan interface in the VM namespace if it exists _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) @@ -154,6 +162,8 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil { return errors.Wrap(delErr, "failed to clean up vlan interface") } + } else { + logger.Info("Expecting vlan interface not to exist", zap.String("result", vlanIfInVMErr.Error())) } return nil } @@ -714,7 +724,7 @@ func RunWithRetries(f func() error, maxRuns, sleepMs int) error { if err == nil { break } - logger.Info("Retrying after delay...", zap.String("error", err.Error())) + logger.Info("Retrying after delay...", zap.String("error", err.Error()), zap.Int("retry", i), zap.Int("sleepMs", sleepMs)) time.Sleep(time.Duration(sleepMs) * time.Millisecond) } return err diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index fe834ecd74..271fbdade7 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -16,6 +16,8 @@ import ( ) var errNetnsMock = errors.New("mock netns error") +var errMockNetIOFail = errors.New("netio fail") +var errMockNetIONoIfFail = errors.New("no such network interface") func newNetnsErrorMock(errStr string) error { return errors.Wrap(errNetnsMock, errStr) @@ -80,10 +82,10 @@ func defaultDeleteNamed(name string) error { // This mock netioshim provides more flexbility in when it errors compared to the one in the netio package type mockNetIO struct { existingInterfaces map[string]bool + err error } func (ns *mockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, error) { - ErrMockNetIOFail := errors.New("netio fail") if ns.existingInterfaces[name] { hwAddr, _ := net.ParseMAC("ab:cd:ef:12:34:56") return &net.Interface{ @@ -95,7 +97,7 @@ func (ns *mockNetIO) GetNetworkInterfaceByName(name string) (*net.Interface, err Index: 2, }, nil } - return nil, errors.Wrap(ErrMockNetIOFail, name) + return nil, errors.Wrap(ns.err, name) } func (ns *mockNetIO) GetNetworkInterfaceAddrs(_ *net.Interface) ([]net.Addr, error) { @@ -173,6 +175,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: &mockNetIO{ existingInterfaces: map[string]bool{}, + err: errMockNetIOFail, }, nsClient: NewMockNamespaceClient(), }, @@ -226,7 +229,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { wantErr: false, }, { - name: "Ensure clean populate VM vnet ns but vlan if does not", + name: "Ensure clean populate VM vnet ns exists vlan does not exist", client: &TransparentVlanEndpointClient{ primaryHostIfName: "eth0", vlanIfName: "eth0.1", @@ -243,13 +246,44 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { netlink: netlink.NewMockNetlink(false, ""), plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), - netioshim: netio.NewMockNetIO(true, 1), - nsClient: NewMockNamespaceClient(), + netioshim: &mockNetIO{ + existingInterfaces: map[string]bool{}, + err: errMockNetIONoIfFail, + }, + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, wantErrMsg: "failed to cleanup/delete ns after noticing vlan veth does not exist: netns failure: " + errNetnsMock.Error(), }, + { + name: "Ensure clean populate VM vnet ns exists unknown if vlan veth exists", + client: &TransparentVlanEndpointClient{ + primaryHostIfName: "eth0", + vlanIfName: "eth0.1", + vnetVethName: "A1veth0", + containerVethName: "B1veth0", + vnetNSName: "az_ns_1", + netnsClient: &mockNetns{ + get: defaultGet, + getFromName: defaultGetFromName, + deleteNamed: func(name string) (err error) { + return newNetnsErrorMock("netns failure") + }, + }, + netlink: netlink.NewMockNetlink(false, ""), + plClient: platform.NewMockExecClient(false), + netUtilsClient: networkutils.NewNetworkUtils(nl, plc), + netioshim: &mockNetIO{ + existingInterfaces: map[string]bool{}, + err: errMockNetIOFail, + }, + nsClient: NewMockNamespaceClient(), + }, + epInfo: &EndpointInfo{}, + wantErr: true, + wantErrMsg: "could not determine if vlan veth exists in vnet namespace", + }, } for _, tt := range tests { tt := tt @@ -393,12 +427,13 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { existingInterfaces: map[string]bool{ "A1veth0": true, }, + err: errMockNetIOFail, }, nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, - wantErrMsg: "container veth does not exist: failed to get container veth: B1veth0: " + netio.ErrMockNetIOFail.Error() + "", + wantErrMsg: "container veth does not exist: failed to get container veth: B1veth0: " + errMockNetIOFail.Error() + "", }, { name: "Add endpoints NetNS Get fail", From 14e85b6f981df253d047f2685d2d184d472ff6c4 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Wed, 8 Nov 2023 14:24:42 -0800 Subject: [PATCH 14/17] Address feedback --- .../transparent_vlan_endpointclient_linux.go | 63 +++++++++---------- ...nsparent_vlan_endpointclient_linux_test.go | 6 +- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 98146b60ae..37d8c68efa 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -3,6 +3,7 @@ package network import ( "fmt" "net" + "os" "strings" "time" @@ -125,7 +126,7 @@ func (client *TransparentVlanEndpointClient) AddEndpoints(epInfo *EndpointInfo) }) } -// Called from PopulateVM, Namespace: VM and Vnet +// Called from AddEndpoints, Namespace: VM and Vnet func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { // Clean up vlan interface in the VM namespace and ensure the network namespace (if it exists) has a vlan interface logger.Info("Checking if NS and vlan veth exists...") @@ -141,18 +142,18 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { // If the vlan veth for sure doesn't exist, we delete the vnet ns, but if we can't query, we can't delete the ns (the vlan veth might exist!) if vlanIfErr != nil { - if strings.Contains(vlanIfErr.Error(), "no such network interface") { - logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...") + var ev *os.SyscallError + if errors.As(vlanIfErr, &ev) { + return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") + } else { + // Assume any other error is the vlan interface not found + logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) delErr := client.netnsClient.DeleteNamed(client.vnetNSName) if delErr != nil { return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") } - } else { - return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") } } - } else { - logger.Info("Expecting vnet namespace not to exist", zap.String("result", existingErr.Error())) } // Delete the vlan interface in the VM namespace if it exists _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) @@ -162,36 +163,29 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil { return errors.Wrap(delErr, "failed to clean up vlan interface") } - } else { - logger.Info("Expecting vlan interface not to exist", zap.String("result", vlanIfInVMErr.Error())) } return nil } // Called from PopulateVM, Namespace: VM func (client *TransparentVlanEndpointClient) createNetworkNamespace(vmNS int) error { - createNsImpl := func() error { - // If this call succeeds, the namespace is set to the new namespace - vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) - if err != nil { - return errors.Wrap(err, "failed to create vnet ns") - } - logger.Info("Vnet Namespace created", zap.String("vnetNS", client.netnsClient.NamespaceUniqueID(vnetNS)), zap.String("vmNS", client.netnsClient.NamespaceUniqueID(vmNS))) - if !client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { - client.vnetNSFileDescriptor = vnetNS - return nil - } - // the vnet and vm namespace are the same by this point - logger.Info("Vnet Namespace is the same as VM namespace. Deleting and retrying...") - delErr := client.netnsClient.DeleteNamed(client.vnetNSName) - if delErr != nil { - logger.Error("failed to cleanup/delete ns after noticing vnet ns is the same as vm ns", zap.Any("error:", delErr.Error())) - } - return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are the same") + // If this call succeeds, the namespace is set to the new namespace + vnetNS, err := client.netnsClient.NewNamed(client.vnetNSName) + if err != nil { + return errors.Wrap(err, "failed to create vnet ns") } - err := RunWithRetries(createNsImpl, numRetries, sleepInMs) - - return err + logger.Info("Vnet Namespace created", zap.String("vnetNS", client.netnsClient.NamespaceUniqueID(vnetNS)), zap.String("vmNS", client.netnsClient.NamespaceUniqueID(vmNS))) + if !client.netnsClient.IsNamespaceEqual(vnetNS, vmNS) { + client.vnetNSFileDescriptor = vnetNS + return nil + } + // the vnet and vm namespace are the same by this point + logger.Info("Vnet Namespace is the same as VM namespace. Deleting...") + delErr := client.netnsClient.DeleteNamed(client.vnetNSName) + if delErr != nil { + logger.Error("failed to cleanup/delete ns after noticing vnet ns is the same as vm ns", zap.Any("error:", delErr.Error())) + } + return errors.Wrap(errNamespaceCreation, "vnet and vm namespace are the same") } // Called from PopulateVM, Namespace: VM and namespace represented by fd @@ -230,10 +224,13 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er // 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 - logger.Info("No existing NS detected. Creating the vnet namespace and switching to it") + logger.Info("No existing NS detected. Creating the vnet namespace and switching to it", zap.String("message", existingErr.Error())) - if err = client.createNetworkNamespace(vmNS); err != nil { - return errors.Wrap(err, "") + err = RunWithRetries(func() error { + return client.createNetworkNamespace(vmNS) + }, numRetries, sleepInMs) + if err != nil { + return errors.Wrap(err, "failed to create network namespace") } deleteNSIfNotNilErr := client.netnsClient.Set(vmNS) diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 271fbdade7..68c70ff26c 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -5,6 +5,7 @@ package network import ( "net" + "os" "testing" "github.com/Azure/azure-container-networking/netio" @@ -17,7 +18,8 @@ import ( var errNetnsMock = errors.New("mock netns error") var errMockNetIOFail = errors.New("netio fail") -var errMockNetIONoIfFail = errors.New("no such network interface") +var errMockNetIOSyscallFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: os.NewSyscallError("syscall error", errMockNetIOFail)} +var 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) @@ -276,7 +278,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { netUtilsClient: networkutils.NewNetworkUtils(nl, plc), netioshim: &mockNetIO{ existingInterfaces: map[string]bool{}, - err: errMockNetIOFail, + err: errMockNetIOSyscallFail, }, nsClient: NewMockNamespaceClient(), }, From c16b938fc3369d6f33da1bcf362c64a9331ab2ef Mon Sep 17 00:00:00 2001 From: QxBytes Date: Wed, 8 Nov 2023 14:44:37 -0800 Subject: [PATCH 15/17] Address linter issue --- network/transparent_vlan_endpointclient_linux.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 37d8c68efa..c33fb1cd61 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -145,13 +145,12 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { var ev *os.SyscallError if errors.As(vlanIfErr, &ev) { return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") - } else { - // Assume any other error is the vlan interface not found - logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) - delErr := client.netnsClient.DeleteNamed(client.vnetNSName) - if delErr != nil { - return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") - } + } + // Assume any other error is the vlan interface not found + logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) + delErr := client.netnsClient.DeleteNamed(client.vnetNSName) + if delErr != nil { + return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") } } } From fa40a7df4b07d0d85d1a16d938a79e35f681a701 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 9 Nov 2023 10:01:37 -0800 Subject: [PATCH 16/17] Address log feedback --- network/transparent_vlan_endpointclient_linux.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index c33fb1cd61..1e776247ae 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -147,7 +147,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") } // Assume any other error is the vlan interface not found - logger.Info("Vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) + logger.Info("vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) delErr := client.netnsClient.DeleteNamed(client.vnetNSName) if delErr != nil { return errors.Wrap(delErr, "failed to cleanup/delete ns after noticing vlan veth does not exist") @@ -158,7 +158,7 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { _, vlanIfInVMErr := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) if vlanIfInVMErr == nil { // The vlan interface exists in the VM ns because it failed to move into the network ns previously and needs to be cleaned up - logger.Info("Vlan interface exists on the VM namespace, deleting", zap.String("vlanIfName", client.vlanIfName)) + logger.Info("vlan interface exists on the VM namespace, deleting", zap.String("vlanIfName", client.vlanIfName)) if delErr := client.netlink.DeleteLink(client.vlanIfName); delErr != nil { return errors.Wrap(delErr, "failed to clean up vlan interface") } @@ -306,7 +306,7 @@ func (client *TransparentVlanEndpointClient) PopulateVM(epInfo *EndpointInfo) er } logger.Info("Moving vlan veth into namespace confirmed") } else { - logger.Info("Existing NS detected. Vlan interface should exist or namespace would've been deleted.", zap.String("vnetNSName", client.vnetNSName), zap.String("vlanIfName", client.vlanIfName)) + logger.Info("Existing NS detected. vlan interface should exist or namespace would've been deleted.", zap.String("vnetNSName", client.vnetNSName), zap.String("vlanIfName", client.vlanIfName)) } // Get the default constant host veth mac From 104935efcd1e96d7ebdf91c68aad3d352702bf60 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 9 Nov 2023 17:17:02 -0800 Subject: [PATCH 17/17] Address feedback by asserting any error is an interface not found error --- .../transparent_vlan_endpointclient_linux.go | 9 +------- ...nsparent_vlan_endpointclient_linux_test.go | 22 +++++++------------ 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/network/transparent_vlan_endpointclient_linux.go b/network/transparent_vlan_endpointclient_linux.go index 1e776247ae..56ae8517c3 100644 --- a/network/transparent_vlan_endpointclient_linux.go +++ b/network/transparent_vlan_endpointclient_linux.go @@ -3,7 +3,6 @@ package network import ( "fmt" "net" - "os" "strings" "time" @@ -139,14 +138,8 @@ func (client *TransparentVlanEndpointClient) ensureCleanPopulateVM() error { _, err := client.netioshim.GetNetworkInterfaceByName(client.vlanIfName) return errors.Wrap(err, "failed to get vlan interface in namespace") }) - - // If the vlan veth for sure doesn't exist, we delete the vnet ns, but if we can't query, we can't delete the ns (the vlan veth might exist!) if vlanIfErr != nil { - var ev *os.SyscallError - if errors.As(vlanIfErr, &ev) { - return errors.Wrap(vlanIfErr, "could not determine if vlan veth exists in vnet namespace") - } - // Assume any other error is the vlan interface not found + // Assume any error is the vlan interface not found logger.Info("vlan interface doesn't exist even though network namespace exists, deleting network namespace...", zap.String("message", vlanIfErr.Error())) delErr := client.netnsClient.DeleteNamed(client.vnetNSName) if delErr != nil { diff --git a/network/transparent_vlan_endpointclient_linux_test.go b/network/transparent_vlan_endpointclient_linux_test.go index 68c70ff26c..be64142bc5 100644 --- a/network/transparent_vlan_endpointclient_linux_test.go +++ b/network/transparent_vlan_endpointclient_linux_test.go @@ -5,7 +5,6 @@ package network import ( "net" - "os" "testing" "github.com/Azure/azure-container-networking/netio" @@ -18,7 +17,6 @@ import ( var errNetnsMock = errors.New("mock netns error") var errMockNetIOFail = errors.New("netio fail") -var errMockNetIOSyscallFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: os.NewSyscallError("syscall error", errMockNetIOFail)} var errMockNetIONoIfFail = &net.OpError{Op: "route", Net: "ip+net", Source: nil, Addr: nil, Err: errors.New("no such network interface")} func newNetnsErrorMock(errStr string) error { @@ -259,7 +257,7 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { wantErrMsg: "failed to cleanup/delete ns after noticing vlan veth does not exist: netns failure: " + errNetnsMock.Error(), }, { - name: "Ensure clean populate VM vnet ns exists unknown if vlan veth exists", + name: "Ensure clean populate VM cleanup straggling vlan if in vm ns", client: &TransparentVlanEndpointClient{ primaryHostIfName: "eth0", vlanIfName: "eth0.1", @@ -267,24 +265,20 @@ func TestTransparentVlanAddEndpoints(t *testing.T) { containerVethName: "B1veth0", vnetNSName: "az_ns_1", netnsClient: &mockNetns{ - get: defaultGet, - getFromName: defaultGetFromName, - deleteNamed: func(name string) (err error) { - return newNetnsErrorMock("netns failure") + get: defaultGet, + getFromName: func(name string) (fileDescriptor int, err error) { + return 0, newNetnsErrorMock("netns failure") }, }, - netlink: netlink.NewMockNetlink(false, ""), + netlink: netlink.NewMockNetlink(true, "netlink fail"), plClient: platform.NewMockExecClient(false), netUtilsClient: networkutils.NewNetworkUtils(nl, plc), - netioshim: &mockNetIO{ - existingInterfaces: map[string]bool{}, - err: errMockNetIOSyscallFail, - }, - nsClient: NewMockNamespaceClient(), + netioshim: netio.NewMockNetIO(false, 0), + nsClient: NewMockNamespaceClient(), }, epInfo: &EndpointInfo{}, wantErr: true, - wantErrMsg: "could not determine if vlan veth exists in vnet namespace", + wantErrMsg: "failed to clean up vlan interface", }, } for _, tt := range tests {