Stop using /bin/sh for OVS commands (#5364)

We are currently using the shell (/bin/sh) to run ovs-appctl and
ovs-ofctl commands, but there does not seem to be a reason to do so.

Instead, we can invoke the binary directly with exec.Command and pass
the arguments as a slice.

Before this change, ovsVSwitchdUDS would return
"/var/run/openvswitch/ovs-vswitchd.*.ctl" as a fallback value in case
the PID of ovs-vswitchd could not be determined. The shell would then
expand the wildcard patterns. I now believe that it is better to return
an error in that case. We want to make sure that we use a valid file
always (and returning the glob pattern would cause an error if multiple
files match). If we run into issues because of this approach, we can
update the code to expand the glob pattern programmatically and, if a
single path matches, we can return that path.

Signed-off-by: Antonin Bas <[email protected]>

pkg/agent/cniserver/sriov.go

@@ -63,9 +63,6 @@ func GetPodContainerDeviceIDs(podName string, podNamespace string) ([]string, er
 	defer conn.Close()
 
 	client := podresourcesv1alpha1.NewPodResourcesListerClient(conn)
-	if client == nil {
-		return []string{}, fmt.Errorf("error getting the lister client for Pod resources")
-	}
 
 	podResources, err := client.List(ctx, &podresourcesv1alpha1.ListPodResourcesRequest{})
 	if err != nil {

pkg/ovs/ovsctl/appctl.go

@@ -15,28 +15,31 @@
 package ovsctl
 
 import (
+	"context"
 	"fmt"
-	"strings"
+	"os/exec"
 )
 
 type ovsAppctlRunner struct {
 	bridge string
 }
 
-func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError) {
+func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error) {
 	// Use the control UNIX domain socket to connect to ovs-vswitchd, as Agent can
 	// run in a different PID namespace from ovs-vswitchd. Relying on ovs-appctl to
 	// determine the control socket based on the pidfile will then give a "stale
 	// pidfile" error, as it tries to validate that the PID read from the pidfile
 	// corresponds to a valid process in the current PID namespace.
-	var cmdStr string
+	uds, err := ovsVSwitchdUDS(context.TODO())
+	if err != nil {
+		return nil, fmt.Errorf("failed to get UDS for OVS: %w", err)
+	}
+	cmdArgs := []string{"-t", uds, cmd}
 	if needsBridge {
-		cmdStr = fmt.Sprintf("ovs-appctl -t %s %s %s", ovsVSwitchdUDS(), cmd, r.bridge)
-	} else {
-		cmdStr = fmt.Sprintf("ovs-appctl -t %s %s", ovsVSwitchdUDS(), cmd)
+		cmdArgs = append(cmdArgs, r.bridge)
 	}
-	cmdStr = cmdStr + " " + strings.Join(args, " ")
-	out, err := getOVSCommand(cmdStr).CombinedOutput()
+	ovsCmd := exec.CommandContext(context.TODO(), "ovs-appctl", cmdArgs...)
+	out, err := ovsCmd.CombinedOutput()
 	if err != nil {
 		return nil, newExecError(err, string(out))
 	}

pkg/ovs/ovsctl/interface.go

@@ -19,7 +19,7 @@ import (
 )
 
 type OVSAppctlRunner interface {
-	RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError)
+	RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error)
 }
 
 type OVSOfctlRunner interface {

pkg/ovs/ovsctl/ofctl.go

@@ -15,27 +15,23 @@
 package ovsctl
 
 import (
-	"fmt"
-	"strings"
+	"context"
+	"os/exec"
 )
 
 type ovsOfctlRunner struct {
 	bridge string
 }
 
 func (r *ovsOfctlRunner) RunOfctlCmd(cmd string, args ...string) ([]byte, error) {
-	return runOfctlCmd(true, cmd, r.bridge, args...)
+	return runOfctlCmd(context.TODO(), true, cmd, r.bridge, args...)
 }
 
-func runOfctlCmd(openflow15 bool, cmd string, bridge string, args ...string) ([]byte, error) {
-	cmdStr := fmt.Sprintf("ovs-ofctl %s %s", cmd, bridge)
-	cmdStr = cmdStr + " " + strings.Join(args, " ")
+func runOfctlCmd(ctx context.Context, openflow15 bool, cmd string, bridge string, args ...string) ([]byte, error) {
+	cmdArgs := append([]string{cmd, bridge}, args...)
 	if openflow15 {
-		cmdStr += " -O Openflow15"
+		cmdArgs = append(cmdArgs, "-O", "Openflow15")
 	}
-	out, err := getOVSCommand(cmdStr).Output()
-	if err != nil {
-		return nil, err
-	}
-	return out, nil
+	ovsCmd := exec.CommandContext(ctx, "ovs-ofctl", cmdArgs...)
+	return ovsCmd.Output()
 }

pkg/ovs/ovsctl/ovsctl.go

@@ -17,6 +17,7 @@ package ovsctl
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"fmt"
 	"net"
 	"strconv"
@@ -181,7 +182,7 @@ func (c *ovsCtlClient) runTracing(flow string) (string, error) {
 	return string(out), nil
 }
 
-func (c *ovsCtlClient) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, *ExecError) {
+func (c *ovsCtlClient) RunAppctlCmd(cmd string, needsBridge bool, args ...string) ([]byte, error) {
 	return c.ovsAppctlRunner.RunAppctlCmd(cmd, needsBridge, args...)
 }
 
@@ -399,7 +400,7 @@ func (c *ovsCtlClient) DumpPortsDesc() ([][]string, error) {
 func (c *ovsCtlClient) SetPortNoFlood(ofport int) error {
 	// This command does not have standard output, and only has standard err when running with error.
 	// NOTE THAT, THIS CONFIGURATION MUST WORK WITH OpenFlow10.
-	_, err := runOfctlCmd(false, "mod-port", c.bridge, strconv.FormatUint(uint64(ofport), 10), "no-flood")
+	_, err := runOfctlCmd(context.TODO(), false, "mod-port", c.bridge, strconv.FormatUint(uint64(ofport), 10), "no-flood")
 	if err != nil {
 		return fmt.Errorf("fail to set no-food config for port %d on bridge %s: %v", ofport, c.bridge, err)
 	}

pkg/ovs/ovsctl/ovsctl_others.go

@@ -18,9 +18,9 @@
 package ovsctl
 
 import (
+	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -44,7 +44,7 @@ func readOVSVSwitchdPID() (int, error) {
 }
 
 // ovsVSwitchdUDS returns the file path of the ovs-vswitchd control UNIX domain socket.
-func ovsVSwitchdUDS() string {
+func ovsVSwitchdUDS(ctx context.Context) (string, error) {
 	// It is a bit sub-optimal to read the PID every time we need it, but ovs-vswitchd restarts
 	// are possible. Besides, this value is only used when invoking ovs-appctl (as a new
 	// process) at the moment, so the overhead of reading the PID from file should not be a
@@ -53,25 +53,19 @@ func ovsVSwitchdUDS() string {
 	var readErr error
 	startTime := time.Now()
 	hasFailure := false
-	pollErr := wait.PollImmediate(50*time.Millisecond, 5*time.Second, func() (bool, error) {
+	err := wait.PollImmediateWithContext(ctx, 50*time.Millisecond, 5*time.Second, func(ctx context.Context) (bool, error) {
 		pid, readErr = readOVSVSwitchdPID()
 		if readErr != nil {
 			hasFailure = true
 			return false, nil
 		}
 		return true, nil
 	})
-	if pollErr != nil {
-		klog.ErrorS(readErr, "Failed to read ovs-vswitchd PID")
-		// that seems like a reasonable value to return if we cannot read the PID
-		return "/var/run/openvswitch/ovs-vswitchd.*.ctl"
+	if err != nil {
+		return "", fmt.Errorf("failed to read ovs-vswitchd PID: %w", readErr)
 	}
 	if hasFailure {
 		klog.V(2).InfoS("Waited for ovs-vswitchd PID to be ready", "duration", time.Since(startTime))
 	}
-	return fmt.Sprintf("/var/run/openvswitch/ovs-vswitchd.%d.ctl", pid)
-}
-
-func getOVSCommand(cmdStr string) *exec.Cmd {
-	return exec.Command("/bin/sh", "-c", cmdStr)
+	return fmt.Sprintf("/var/run/openvswitch/ovs-vswitchd.%d.ctl", pid), nil
 }

pkg/ovs/ovsctl/ovsctl_windows.go

@@ -17,13 +17,11 @@
 
 package ovsctl
 
-import "os/exec"
+import (
+	"context"
+)
 
 // ovsVSwitchdUDS returns the file path of the ovs-vswitchd control named pipe.
-func ovsVSwitchdUDS() string {
-	return "c:/openvswitch/var/run/openvswitch/ovs-vswitchd.ctl"
-}
-
-func getOVSCommand(cmdStr string) *exec.Cmd {
-	return exec.Command("cmd.exe", "/c", cmdStr)
+func ovsVSwitchdUDS(ctx context.Context) (string, error) {
+	return "c:/openvswitch/var/run/openvswitch/ovs-vswitchd.ctl", nil
 }