Merge pull request k8snetworkplumbingwg#186 from pliurh/ds_chroot

OCPBUGS-18995: Move chroot from multus main process to its child processes (k8snetworkplumbingwg#1161)

deployments/multus-daemonset-thick.yml

@@ -166,6 +166,11 @@ spec:
           volumeMounts:
             - name: cni
               mountPath: /host/etc/cni/net.d
+            # multus-daemon expects that cni-bin path must be identical between pod and container host.
+            # e.g. if the cni bin is in '/opt/cni/bin' on the container host side, then it should be mount to '/opt/cni/bin' in multus-daemon,
+            # not to any other directory, like '/opt/bin' or '/usr/bin'.
+            - name: cni-bin
+              mountPath: /opt/cni/bin
             - name: host-run
               mountPath: /host/run
             - name: host-var-lib-cni-multus

pkg/k8sclient/k8sclient.go

@@ -286,11 +286,6 @@ func getKubernetesDelegate(client *ClientInfo, net *types.NetworkSelectionElemen
 		}
 	}
 
-	// acquire lock to access file
-	if types.ChrootMutex != nil {
-		types.ChrootMutex.Lock()
-		defer types.ChrootMutex.Unlock()
-	}
 	configBytes, err := netutils.GetCNIConfig(customResource, confdir)
 	if err != nil {
 		return nil, resourceMap, err
@@ -466,12 +461,6 @@ func getNetDelegate(client *ClientInfo, pod *v1.Pod, netname, confdir, namespace
 
 		// option2) search CNI json config file, which has <netname> as CNI name, from confDir
 
-		// acquire lock to access file
-		if types.ChrootMutex != nil {
-			types.ChrootMutex.Lock()
-			defer types.ChrootMutex.Unlock()
-		}
-
 		configBytes, err = netutils.GetCNIConfigFromFile(netname, confdir)
 		if err == nil {
 			delegate, err := types.LoadDelegateNetConf(configBytes, nil, "", "")
@@ -481,12 +470,6 @@ func getNetDelegate(client *ClientInfo, pod *v1.Pod, netname, confdir, namespace
 			return delegate, resourceMap, nil
 		}
 	} else {
-		// acquire lock to access file
-		if types.ChrootMutex != nil {
-			types.ChrootMutex.Lock()
-			defer types.ChrootMutex.Unlock()
-		}
-
 		fInfo, err := os.Stat(netname)
 		if err != nil {
 			return nil, resourceMap, err

pkg/server/exec_chroot.go

@@ -20,10 +20,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"os"
 	"os/exec"
 	"strings"
-	"sync"
 	"syscall"
 	"time"
 
@@ -34,91 +32,24 @@ import (
 
 // ChrootExec implements invoke.Exec to execute CNI with chroot
 type ChrootExec struct {
-	Stderr     io.Writer
-	chrootDir  string
-	workingDir string   // working directory in the outer root
-	outerRoot  *os.File // outer root directory
+	Stderr    io.Writer
+	chrootDir string
 	version.PluginDecoder
-	mu sync.Mutex
 }
 
 var _ invoke.Exec = &ChrootExec{}
 
-func (e *ChrootExec) chroot() error {
-	var err error
-	e.workingDir, err = os.Getwd()
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "getwd before chroot failed: %v\n", err)
-		return fmt.Errorf("getwd before chroot failed: %v", err)
-	}
-
-	e.outerRoot, err = os.Open("/")
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "getwd before chroot failed: %v\n", err)
-		return fmt.Errorf("getwd before chroot failed: %v", err)
-	}
-
-	if err := syscall.Chroot(e.chrootDir); err != nil {
-		fmt.Fprintf(os.Stderr, "chroot to %s failed: %v\n", e.chrootDir, err)
-		return fmt.Errorf("chroot to %s failed: %v", e.chrootDir, err)
-	}
-
-	if err := os.Chdir("/"); err != nil {
-		fmt.Fprintf(os.Stderr, "chdir to \"/\" failed: %v\n", err)
-		return fmt.Errorf("chdir to \"/\" failed: %v", err)
-	}
-
-	return nil
-}
-
-func (e *ChrootExec) escape() error {
-	if e.outerRoot == nil || e.workingDir == "" {
-		return nil
-	}
-
-	// change directory to outer root and close it
-	if err := syscall.Fchdir(int(e.outerRoot.Fd())); err != nil {
-		fmt.Fprintf(os.Stderr, "changing directory to outer root failed: %v\n", err)
-		return fmt.Errorf("changing directory to outer root failed: %v", err)
-	}
-
-	if err := e.outerRoot.Close(); err != nil {
-		fmt.Fprintf(os.Stderr, "closing outer root failed: %v\n", err)
-		return fmt.Errorf("closing outer root failed: %v", err)
-	}
-
-	// chroot to current directory aka "." being the outer root
-	if err := syscall.Chroot("."); err != nil {
-		fmt.Fprintf(os.Stderr, "chroot to current directory failed: %v\n", err)
-		return fmt.Errorf("chroot to current directory failed: %v", err)
-	}
-
-	if err := os.Chdir(e.workingDir); err != nil {
-		fmt.Fprintf(os.Stderr, "chdir to working directory failed: %v\n", err)
-		return fmt.Errorf("chdir to working directory failed: %v", err)
-	}
-	e.outerRoot = nil
-	e.workingDir = ""
-
-	return nil
-}
-
 // ExecPlugin executes CNI plugin with given environment/stdin data.
 func (e *ChrootExec) ExecPlugin(ctx context.Context, pluginPath string, stdinData []byte, environ []string) ([]byte, error) {
-	// lock and do chroot to execute plugin with host root
-	e.mu.Lock()
-	defer e.mu.Unlock()
-	err := e.chroot()
-	defer e.escape()
-
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "ExecPlugin failed at chroot: %v\n", err)
-		return nil, fmt.Errorf("ExecPlugin failed at chroot: %v", err)
-	}
+	var err error
 
 	stdout := &bytes.Buffer{}
 	stderr := &bytes.Buffer{}
 	c := exec.CommandContext(ctx, pluginPath)
+	// execute delegate CNI with host filesystem context.
+	c.SysProcAttr = &syscall.SysProcAttr{
+		Chroot: e.chrootDir,
+	}
 	c.Env = environ
 	c.Stdin = bytes.NewBuffer(stdinData)
 	c.Stdout = stdout
@@ -169,14 +100,5 @@ func (e *ChrootExec) pluginErr(err error, stdout, stderr []byte) error {
 
 // FindInPath try to find CNI plugin based on given path
 func (e *ChrootExec) FindInPath(plugin string, paths []string) (string, error) {
-	e.mu.Lock()
-	defer e.mu.Unlock()
-	err := e.chroot()
-	defer e.escape()
-	if err != nil {
-		fmt.Fprintf(os.Stderr, "FindInPath failed at chroot: %v\n", err)
-		return "", fmt.Errorf("FindInPath failed at chroot: %v", err)
-	}
-
 	return invoke.FindInPath(plugin, paths)
 }

pkg/server/exec_chroot_test.go

@@ -43,15 +43,4 @@ var _ = Describe("exec_chroot", func() {
 		_, err := chrootExec.ExecPlugin(context.Background(), "/bin/true", nil, nil)
 		Expect(err).To(HaveOccurred())
 	})
-
-	It("Call ChrootExec.FindInPath with dummy", func() {
-		chrootExec := &ChrootExec{
-			Stderr:    os.Stderr,
-			chrootDir: "/usr/bin",
-		}
-
-		_, err := chrootExec.FindInPath("true", []string{"/"})
-		Expect(err).NotTo(HaveOccurred())
-	})
-
 })

pkg/server/server.go

@@ -210,7 +210,6 @@ func NewCNIServer(daemonConfig *ControllerNetConf, serverConfig []byte, ignoreRe
 			Stderr:    os.Stderr,
 			chrootDir: daemonConfig.ChrootDir,
 		}
-		types.ChrootMutex = &chrootExec.mu
 		exec = chrootExec
 		logging.Verbosef("server configured with chroot: %s", daemonConfig.ChrootDir)
 	}

pkg/types/conf.go

@@ -21,7 +21,6 @@ import (
 	"net"
 	"os"
 	"strings"
-	"sync"
 	"time"
 
 	utilwait "k8s.io/apimachinery/pkg/util/wait"
@@ -43,9 +42,6 @@ const (
 	defaultNonIsolatedNamespace   = "default"
 )
 
-// ChrootMutex provides lock to access host filesystem
-var ChrootMutex *sync.Mutex
-
 // LoadDelegateNetConfList reads DelegateNetConf from bytes
 func LoadDelegateNetConfList(bytes []byte, delegateConf *DelegateNetConf) error {
 	logging.Debugf("LoadDelegateNetConfList: %s, %v", string(bytes), delegateConf)