From cf78a58de1760f8bb839d0fe3bc1797bf7ca2b84 Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Wed, 26 Jul 2023 15:04:32 -0700 Subject: [PATCH 1/2] Stop using /bin/sh for OVS commands 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 --- pkg/agent/cniserver/sriov.go | 3 --- pkg/ovs/ovsctl/appctl.go | 19 +++++++++++-------- pkg/ovs/ovsctl/interface.go | 2 +- pkg/ovs/ovsctl/mock_ovsctl_test.go | 4 ++-- pkg/ovs/ovsctl/ofctl.go | 20 ++++++++------------ pkg/ovs/ovsctl/ovsctl.go | 5 +++-- pkg/ovs/ovsctl/ovsctl_others.go | 18 ++++++------------ pkg/ovs/ovsctl/ovsctl_windows.go | 12 +++++------- pkg/ovs/ovsctl/testing/mock_ovsctl.go | 4 ++-- 9 files changed, 38 insertions(+), 49 deletions(-) diff --git a/pkg/agent/cniserver/sriov.go b/pkg/agent/cniserver/sriov.go index 601f74c2826..7bf0561710a 100644 --- a/pkg/agent/cniserver/sriov.go +++ b/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 { diff --git a/pkg/ovs/ovsctl/appctl.go b/pkg/ovs/ovsctl/appctl.go index 37435433719..404a1055204 100644 --- a/pkg/ovs/ovsctl/appctl.go +++ b/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-apptcl", cmdArgs...) + out, err := ovsCmd.CombinedOutput() if err != nil { return nil, newExecError(err, string(out)) } diff --git a/pkg/ovs/ovsctl/interface.go b/pkg/ovs/ovsctl/interface.go index e022697f8d8..e39b2d20a9c 100644 --- a/pkg/ovs/ovsctl/interface.go +++ b/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 { diff --git a/pkg/ovs/ovsctl/mock_ovsctl_test.go b/pkg/ovs/ovsctl/mock_ovsctl_test.go index b000ec116a8..929132973d7 100644 --- a/pkg/ovs/ovsctl/mock_ovsctl_test.go +++ b/pkg/ovs/ovsctl/mock_ovsctl_test.go @@ -91,7 +91,7 @@ func (m *MockOVSAppctlRunner) EXPECT() *MockOVSAppctlRunnerMockRecorder { } // RunAppctlCmd mocks base method -func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, *ExecError) { +func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, error) { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1} for _, a := range arg2 { @@ -99,7 +99,7 @@ func (m *MockOVSAppctlRunner) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...strin } ret := m.ctrl.Call(m, "RunAppctlCmd", varargs...) ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(*ExecError) + ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/pkg/ovs/ovsctl/ofctl.go b/pkg/ovs/ovsctl/ofctl.go index eec207836dd..0d9b8240e70 100644 --- a/pkg/ovs/ovsctl/ofctl.go +++ b/pkg/ovs/ovsctl/ofctl.go @@ -15,8 +15,8 @@ package ovsctl import ( - "fmt" - "strings" + "context" + "os/exec" ) type ovsOfctlRunner struct { @@ -24,18 +24,14 @@ type ovsOfctlRunner struct { } 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() } diff --git a/pkg/ovs/ovsctl/ovsctl.go b/pkg/ovs/ovsctl/ovsctl.go index 52df32e8a13..247cc8e6a15 100644 --- a/pkg/ovs/ovsctl/ovsctl.go +++ b/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) } diff --git a/pkg/ovs/ovsctl/ovsctl_others.go b/pkg/ovs/ovsctl/ovsctl_others.go index ada7936d151..c4a58ff2a39 100644 --- a/pkg/ovs/ovsctl/ovsctl_others.go +++ b/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,7 +53,7 @@ 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 @@ -61,17 +61,11 @@ func ovsVSwitchdUDS() string { } 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 } diff --git a/pkg/ovs/ovsctl/ovsctl_windows.go b/pkg/ovs/ovsctl/ovsctl_windows.go index fef17bf110f..3a05b478004 100644 --- a/pkg/ovs/ovsctl/ovsctl_windows.go +++ b/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 } diff --git a/pkg/ovs/ovsctl/testing/mock_ovsctl.go b/pkg/ovs/ovsctl/testing/mock_ovsctl.go index fc8b37f2c35..24733d1e7cb 100644 --- a/pkg/ovs/ovsctl/testing/mock_ovsctl.go +++ b/pkg/ovs/ovsctl/testing/mock_ovsctl.go @@ -191,7 +191,7 @@ func (mr *MockOVSCtlClientMockRecorder) GetDPFeatures() *gomock.Call { } // RunAppctlCmd mocks base method -func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, *ovsctl.ExecError) { +func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) ([]byte, error) { m.ctrl.T.Helper() varargs := []interface{}{arg0, arg1} for _, a := range arg2 { @@ -199,7 +199,7 @@ func (m *MockOVSCtlClient) RunAppctlCmd(arg0 string, arg1 bool, arg2 ...string) } ret := m.ctrl.Call(m, "RunAppctlCmd", varargs...) ret0, _ := ret[0].([]byte) - ret1, _ := ret[1].(*ovsctl.ExecError) + ret1, _ := ret[1].(error) return ret0, ret1 } From da070f7f650a883c9d8837c4eb346c0d542a47fe Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Tue, 8 Aug 2023 10:07:12 -0700 Subject: [PATCH 2/2] Fix typo: s/ovs-apptcl/ovs-appctl Signed-off-by: Antonin Bas --- pkg/ovs/ovsctl/appctl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ovs/ovsctl/appctl.go b/pkg/ovs/ovsctl/appctl.go index 404a1055204..3b0232ee03d 100644 --- a/pkg/ovs/ovsctl/appctl.go +++ b/pkg/ovs/ovsctl/appctl.go @@ -38,7 +38,7 @@ func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...str if needsBridge { cmdArgs = append(cmdArgs, r.bridge) } - ovsCmd := exec.CommandContext(context.TODO(), "ovs-apptcl", cmdArgs...) + ovsCmd := exec.CommandContext(context.TODO(), "ovs-appctl", cmdArgs...) out, err := ovsCmd.CombinedOutput() if err != nil { return nil, newExecError(err, string(out))