network: Extract netns execution and use it in the setup

The network namespace processing is extracted from the isolation
structure to its own package.

In order to support tests, the `NetConf` object can be created using
a custom `netns` factory. This is transparent to the production calls,
by providing a default factory.

No unit tests are written for the wrapping `netns` package as no logic
is exercised. It just forwards calls to the underlying `ns` external package.

Signed-off-by: Edward Haas <edwardh@redhat.com>

pkg/network/netns/BUILD.bazel

@@ -0,0 +1,9 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["netns.go"],
+    importpath = "kubevirt.io/kubevirt/pkg/network/netns",
+    visibility = ["//visibility:public"],
+    deps = ["//vendor/github.com/containernetworking/plugins/pkg/ns:go_default_library"],
+)

pkg/network/netns/netns.go

@@ -0,0 +1,45 @@
+/*
+ * This file is part of the KubeVirt project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2021 Red Hat, Inc.
+ *
+ */
+
+package netns
+
+import (
+	"fmt"
+
+	"github.com/containernetworking/plugins/pkg/ns"
+)
+
+type NetNS struct {
+	nspath string
+}
+
+func New(pid int) NetNS {
+	return NetNS{nspath: fmt.Sprintf("/proc/%d/ns/net", pid)}
+}
+
+func (n NetNS) Do(f func() error) error {
+	netns, err := ns.GetNS(n.nspath)
+	if err != nil {
+		return fmt.Errorf("failed to fetch network namespace object: %v", err)
+	}
+
+	return netns.Do(func(_ ns.NetNS) error {
+		return f()
+	})
+}

pkg/network/setup/BUILD.bazel

@@ -17,6 +17,7 @@ go_library(
         "//pkg/network/driver:go_default_library",
         "//pkg/network/errors:go_default_library",
         "//pkg/network/infraconfigurators:go_default_library",
+        "//pkg/network/netns:go_default_library",
         "//pkg/virt-launcher/virtwrap/api:go_default_library",
         "//staging/src/kubevirt.io/api/core/v1:go_default_library",
         "//staging/src/kubevirt.io/client-go/log:go_default_library",

pkg/network/setup/netconf.go

@@ -23,26 +23,42 @@ import (
 	"fmt"
 	"sync"
 
+	"kubevirt.io/kubevirt/pkg/network/netns"
+
 	v1 "kubevirt.io/api/core/v1"
 	"kubevirt.io/kubevirt/pkg/network/cache"
 )
 
 type NetConf struct {
 	setupCompleted    sync.Map
 	ifaceCacheFactory cache.InterfaceCacheFactory
+	nsFactory         nsFactory
+}
+
+type nsFactory func(int) NSExecutor
+
+type NSExecutor interface {
+	Do(func() error) error
 }
 
 func NewNetConf(ifaceCacheFactory cache.InterfaceCacheFactory) *NetConf {
+	return NewNetConfWithNSFactory(ifaceCacheFactory, func(pid int) NSExecutor {
+		return netns.New(pid)
+	})
+}
+
+func NewNetConfWithNSFactory(ifaceCacheFactory cache.InterfaceCacheFactory, nsFactory nsFactory) *NetConf {
 	return &NetConf{
 		setupCompleted:    sync.Map{},
 		ifaceCacheFactory: ifaceCacheFactory,
+		nsFactory:         nsFactory,
 	}
 }
 
 // Setup applies (privilege) network related changes for an existing virt-launcher pod.
 // As the changes are performed in the virt-launcher network namespace, which is relative expensive,
 // an early cache check is performed to avoid executing the same operation again (if the last one completed).
-func (c *NetConf) Setup(vmi *v1.VirtualMachineInstance, launcherPid int, doNetNS func(func() error) error, preSetup func() error) error {
+func (c *NetConf) Setup(vmi *v1.VirtualMachineInstance, launcherPid int, preSetup func() error) error {
 	id := vmi.UID
 	if _, exists := c.setupCompleted.Load(id); exists {
 		return nil
@@ -53,7 +69,9 @@ func (c *NetConf) Setup(vmi *v1.VirtualMachineInstance, launcherPid int, doNetNS
 	}
 
 	netConfigurator := NewVMNetworkConfigurator(vmi, c.ifaceCacheFactory)
-	err := doNetNS(func() error {
+
+	ns := c.nsFactory(launcherPid)
+	err := ns.Do(func() error {
 		return netConfigurator.SetupPodNetworkPhase1(launcherPid)
 	})
 	if err != nil {

pkg/network/setup/netconf_test.go

@@ -35,37 +35,37 @@ var _ = Describe("netconf", func() {
 	var vmi *v1.VirtualMachineInstance
 
 	BeforeEach(func() {
-		netConf = netsetup.NewNetConf(&interfaceCacheFactoryStub{})
-
+		netConf = netsetup.NewNetConfWithNSFactory(&interfaceCacheFactoryStub{}, nsNoopFactory)
 		vmi = &v1.VirtualMachineInstance{}
 		vmi.UID = "123"
 	})
 
 	It("runs setup successfully", func() {
-		Expect(netConf.Setup(vmi, 0, doNetNSDummyNoop, netPreSetupDummyNoop)).To(Succeed())
+		Expect(netConf.Setup(vmi, 0, netPreSetupDummyNoop)).To(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeTrue())
 	})
 
 	It("runs teardown successfully", func() {
-		Expect(netConf.Setup(vmi, 0, doNetNSDummyNoop, netPreSetupDummyNoop)).To(Succeed())
+		Expect(netConf.Setup(vmi, 0, netPreSetupDummyNoop)).To(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeTrue())
 		Expect(netConf.Teardown(vmi)).To(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeFalse())
 	})
 
 	It("skips secondary setup runs", func() {
-		Expect(netConf.Setup(vmi, 0, doNetNSDummyNoop, netPreSetupDummyNoop)).To(Succeed())
-		Expect(netConf.Setup(vmi, 0, doNetNSDummyNoop, netPreSetupFail)).To(Succeed())
+		Expect(netConf.Setup(vmi, 0, netPreSetupDummyNoop)).To(Succeed())
+		Expect(netConf.Setup(vmi, 0, netPreSetupFail)).To(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeTrue())
 	})
 
 	It("fails the pre-setup run", func() {
-		Expect(netConf.Setup(vmi, 0, doNetNSDummyNoop, netPreSetupFail)).NotTo(Succeed())
+		Expect(netConf.Setup(vmi, 0, netPreSetupFail)).NotTo(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeFalse())
 	})
 
 	It("fails the setup run", func() {
-		Expect(netConf.Setup(vmi, 0, doNetNSFail, netPreSetupDummyNoop)).NotTo(Succeed())
+		netConf := netsetup.NewNetConfWithNSFactory(&interfaceCacheFactoryStub{}, nsFailureFactory)
+		Expect(netConf.Setup(vmi, 0, netPreSetupDummyNoop)).NotTo(Succeed())
 		Expect(netConf.SetupCompleted(vmi)).To(BeFalse())
 	})
 
@@ -106,8 +106,19 @@ func (p podInterfaceCacheStoreStub) Remove() error {
 	return nil
 }
 
-func doNetNSDummyNoop(func() error) error { return nil }
-func netPreSetupDummyNoop() error         { return nil }
+type netnsStub struct {
+	shouldFail bool
+}
+
+func (n netnsStub) Do(func() error) error {
+	if n.shouldFail {
+		return fmt.Errorf("do-netns failure")
+	}
+	return nil
+}
+func nsNoopFactory(_ int) netsetup.NSExecutor    { return netnsStub{} }
+func nsFailureFactory(_ int) netsetup.NSExecutor { return netnsStub{shouldFail: true} }
+
+func netPreSetupDummyNoop() error { return nil }
 
-func doNetNSFail(func() error) error { return fmt.Errorf("do-netns failure") }
-func netPreSetupFail() error         { return fmt.Errorf("pre-setup failure") }
+func netPreSetupFail() error { return fmt.Errorf("pre-setup failure") }

pkg/virt-handler/isolation/BUILD.bazel

@@ -21,7 +21,6 @@ go_library(
         "//pkg/virt-handler/virt-chroot:go_default_library",
         "//staging/src/kubevirt.io/api/core/v1:go_default_library",
         "//staging/src/kubevirt.io/client-go/log:go_default_library",
-        "//vendor/github.com/containernetworking/plugins/pkg/ns:go_default_library",
         "//vendor/github.com/golang/mock/gomock:go_default_library",
         "//vendor/github.com/mitchellh/go-ps:go_default_library",
         "//vendor/github.com/moby/sys/mountinfo:go_default_library",

pkg/virt-handler/isolation/generated_mock_isolation.go

@@ -99,16 +99,6 @@ func (_mr *_MockIsolationResultRecorder) NetNamespace() *gomock.Call {
 	return _mr.mock.ctrl.RecordCall(_mr.mock, "NetNamespace")
 }
 
-func (_m *MockIsolationResult) DoNetNS(_param0 func() error) error {
-	ret := _m.ctrl.Call(_m, "DoNetNS", _param0)
-	ret0, _ := ret[0].(error)
-	return ret0
-}
-
-func (_mr *_MockIsolationResultRecorder) DoNetNS(arg0 interface{}) *gomock.Call {
-	return _mr.mock.ctrl.RecordCall(_mr.mock, "DoNetNS", arg0)
-}
-
 func (_m *MockIsolationResult) Mounts(_param0 mountinfo.FilterFunc) ([]*mountinfo.Info, error) {
 	ret := _m.ctrl.Call(_m, "Mounts", _param0)
 	ret0, _ := ret[0].([]*mountinfo.Info)

pkg/virt-handler/isolation/isolation.go

@@ -32,7 +32,6 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/containernetworking/plugins/pkg/ns"
 	mount "github.com/moby/sys/mountinfo"
 
 	"kubevirt.io/client-go/log"
@@ -55,8 +54,6 @@ type IsolationResult interface {
 	MountNamespace() string
 	// full path to the network namespace
 	NetNamespace() string
-	// execute a function in the process network namespace
-	DoNetNS(func() error) error
 	// mounts for the process
 	Mounts(mount.FilterFunc) ([]*mount.Info, error)
 }
@@ -72,16 +69,6 @@ func NewIsolationResult(pid, ppid int, slice string, controller []string) Isolat
 	return &RealIsolationResult{pid: pid, ppid: ppid, slice: slice, controller: controller}
 }
 
-func (r *RealIsolationResult) DoNetNS(f func() error) error {
-	netns, err := ns.GetNS(r.NetNamespace())
-	if err != nil {
-		return fmt.Errorf("failed to get launcher pod network namespace: %v", err)
-	}
-	return netns.Do(func(_ ns.NetNS) error {
-		return f()
-	})
-}
-
 func (r *RealIsolationResult) PIDNamespace() string {
 	return fmt.Sprintf("/proc/%d/ns/pid", r.pid)
 }

pkg/virt-handler/vm.go

@@ -80,7 +80,7 @@ import (
 )
 
 type netconf interface {
-	Setup(vmi *v1.VirtualMachineInstance, launcherPid int, doNetNS func(func() error) error, preSetup func() error) error
+	Setup(vmi *v1.VirtualMachineInstance, launcherPid int, preSetup func() error) error
 	Teardown(vmi *v1.VirtualMachineInstance) error
 	SetupCompleted(vmi *v1.VirtualMachineInstance) bool
 }
@@ -494,7 +494,7 @@ func (d *VirtualMachineController) setupNetwork(vmi *v1.VirtualMachineInstance)
 	rootMount := isolationRes.MountRoot()
 	requiresDeviceClaim := virtutil.IsNonRootVMI(vmi) && virtutil.WantVirtioNetDevice(vmi)
 
-	return d.netConf.Setup(vmi, isolationRes.Pid(), isolationRes.DoNetNS, func() error {
+	return d.netConf.Setup(vmi, isolationRes.Pid(), func() error {
 		if requiresDeviceClaim {
 			if err := d.claimDeviceOwnership(rootMount, "vhost-net"); err != nil {
 				return neterrors.CreateCriticalNetworkError(fmt.Errorf("failed to set up vhost-net device, %s", err))

pkg/virt-handler/vm_test.go

@@ -2967,7 +2967,7 @@ type netConfStub struct {
 	SetupError error
 }
 
-func (nc *netConfStub) Setup(vmi *v1.VirtualMachineInstance, launcherPid int, doNetNS func(func() error) error, preSetup func() error) error {
+func (nc *netConfStub) Setup(vmi *v1.VirtualMachineInstance, launcherPid int, preSetup func() error) error {
 	if nc.SetupError != nil {
 		return nc.SetupError
 	}