Automated cherry pick of #5364: Stop using /bin/sh for OVS commands

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
 }