From f679a9b69073d7975e9e45df126630015efaa3aa Mon Sep 17 00:00:00 2001 From: Lan Luo Date: Thu, 4 Jan 2024 11:11:55 +0800 Subject: [PATCH] Fix antctl trace-packet arguments missing issue Fix the following error when running `antctl trace-packet`, which is caused by missing arguments for ovs-appctl command. ``` syntax error at br-int (or the bridge name was omitted) ovs-appctl: /var/run/openvswitch/ovs-vswitchd.103.ctl: server returned an error ``` Signed-off-by: Lan Luo --- .../apiserver/handlers/ovstracing/handler.go | 12 +++++---- .../handlers/ovstracing/handler_test.go | 25 ++++++++++++++++++- pkg/ovs/ovsctl/appctl.go | 3 ++- pkg/ovs/ovsctl/ovsctl.go | 2 +- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/pkg/agent/apiserver/handlers/ovstracing/handler.go b/pkg/agent/apiserver/handlers/ovstracing/handler.go index ea29f4cf4ad..584e8cc866f 100644 --- a/pkg/agent/apiserver/handlers/ovstracing/handler.go +++ b/pkg/agent/apiserver/handlers/ovstracing/handler.go @@ -76,7 +76,7 @@ func getServiceClusterIP(aq querier.AgentQuerier, name, namespace string) (net.I if k8serrors.IsNotFound(err) { return nil, handlers.NewHandlerError(errors.New("Service not found"), http.StatusNotFound) } - klog.Errorf("Failed to get Service from Kubernetes API: %v", err) + klog.ErrorS(err, "Failed to get Service from Kubernetes API") return nil, handlers.NewHandlerError(errors.New("Kubernetes API error"), http.StatusInternalServerError) } return net.ParseIP(srv.Spec.ClusterIP).To4(), nil @@ -127,7 +127,7 @@ func getPeerAddress(aq querier.AgentQuerier, peer *tracingPeer, addrFamily uint8 err := handlers.NewHandlerError(fmt.Errorf("Pod %s/%s not found", peer.namespace, peer.name), http.StatusNotFound) return nil, nil, err } - klog.Errorf("Failed to get Pod from Kubernetes API: %v", err) + klog.ErrorS(err, "Failed to get Pod from Kubernetes API") return nil, nil, handlers.NewHandlerError(errors.New("Kubernetes API error"), http.StatusInternalServerError) } // Return IP only assuming it should be a remote Pod. @@ -356,7 +356,7 @@ func validateRequest(r *http.Request) (*request, *handlers.HandlerError) { return &request, nil } -// HandleFunc returns the function which can handle API requests to "/ovsflows". +// HandleFunc returns the function which can handle API requests to "/ovstracing". func HandleFunc(aq querier.AgentQuerier) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { var traceReq *ovsctl.TracingRequest @@ -380,9 +380,11 @@ func HandleFunc(aq querier.AgentQuerier) http.HandlerFunc { // ovs-appctl has been executed but returned an error (e.g. the provided // "flow" expression is incorrect). Return the error output to the client in // this case. - out = execErr.GetErrorOutput() + klog.ErrorS(execErr, "Tracing command failed") + http.Error(w, execErr.GetErrorOutput(), http.StatusInternalServerError) + return } else { - klog.Errorf("Failed to execute tracing command: %v", err) + klog.ErrorS(err, "Failed to execute tracing command") http.Error(w, "failed to execute tracing command", http.StatusInternalServerError) return } diff --git a/pkg/agent/apiserver/handlers/ovstracing/handler_test.go b/pkg/agent/apiserver/handlers/ovstracing/handler_test.go index a4a12f14e57..ea3c01c068a 100644 --- a/pkg/agent/apiserver/handlers/ovstracing/handler_test.go +++ b/pkg/agent/apiserver/handlers/ovstracing/handler_test.go @@ -20,6 +20,8 @@ import ( "net" "net/http" "net/http/httptest" + "os" + "os/exec" "testing" "github.com/stretchr/testify/assert" @@ -78,6 +80,7 @@ type testCase struct { query string calledTrace bool expectedStatus int + execErr *ovsctl.ExecError } func TestPodFlows(t *testing.T) { @@ -197,6 +200,26 @@ func TestPodFlows(t *testing.T) { calledTrace: true, expectedStatus: http.StatusOK, }, + { + test: "Mock syntax error", + query: "", + calledTrace: true, + expectedStatus: http.StatusInternalServerError, + execErr: ovsctl.NewExecError(&exec.ExitError{ + ProcessState: &os.ProcessState{}, + Stderr: []byte("syntax error"), + }, "server returned an error"), + }, + { + test: "Mock command exec error", + query: "", + calledTrace: true, + expectedStatus: http.StatusInternalServerError, + execErr: ovsctl.NewExecError(&exec.Error{ + Name: "fake err", + Err: errors.New("command not run correctly"), + }, "server returned an error"), + }, } for i := range testcases { tc := testcases[i] @@ -243,7 +266,7 @@ func TestPodFlows(t *testing.T) { if tc.expectedStatus == http.StatusOK { ctl.EXPECT().Trace(gomock.Any()).Return(testTraceResult, nil).Times(1) } else { - ctl.EXPECT().Trace(gomock.Any()).Return(nil, errors.New("tracing error")).Times(1) + ctl.EXPECT().Trace(gomock.Any()).Return("", tc.execErr).Times(1) } } } else { diff --git a/pkg/ovs/ovsctl/appctl.go b/pkg/ovs/ovsctl/appctl.go index 3b0232ee03d..1495eb761a6 100644 --- a/pkg/ovs/ovsctl/appctl.go +++ b/pkg/ovs/ovsctl/appctl.go @@ -38,10 +38,11 @@ func (r *ovsAppctlRunner) RunAppctlCmd(cmd string, needsBridge bool, args ...str if needsBridge { cmdArgs = append(cmdArgs, r.bridge) } + cmdArgs = append(cmdArgs, args...) ovsCmd := exec.CommandContext(context.TODO(), "ovs-appctl", cmdArgs...) out, err := ovsCmd.CombinedOutput() if err != nil { - return nil, newExecError(err, string(out)) + return nil, NewExecError(err, string(out)) } return out, nil } diff --git a/pkg/ovs/ovsctl/ovsctl.go b/pkg/ovs/ovsctl/ovsctl.go index 6fe1aeede79..a9da9daa18d 100644 --- a/pkg/ovs/ovsctl/ovsctl.go +++ b/pkg/ovs/ovsctl/ovsctl.go @@ -261,7 +261,7 @@ func newBadRequestError(msg string) BadRequestError { return BadRequestError(msg) } -func newExecError(err error, errorOutput string) *ExecError { +func NewExecError(err error, errorOutput string) *ExecError { e := &ExecError{error: err} if e.CommandExecuted() { e.errorOutput = errorOutput