From d4498289f35413400537206d52f8ce8f4e341309 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Fri, 17 Sep 2021 17:37:01 -0600 Subject: [PATCH] core: add missing error type check to exec In ExtractExitCode, there is one error type that can be valid as a pointer or not-as-a-pointer. Add a case to the type check for the non-pointer condition. Resolves #8280 Signed-off-by: Blaine Gardner --- pkg/util/exec/exec.go | 11 +++-- pkg/util/exec/exec_test.go | 97 +++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/pkg/util/exec/exec.go b/pkg/util/exec/exec.go index cd11f481f6c2a..67913e1f512ab 100644 --- a/pkg/util/exec/exec.go +++ b/pkg/util/exec/exec.go @@ -336,18 +336,21 @@ func ExtractExitCode(err error) (int, error) { case *kexec.CodeExitError: return errType.ExitStatus(), nil + // have to check both *kexec.CodeExitError and kexec.CodeExitError because CodeExitError methods + // are not defined with pointer receivers; both pointer and non-pointers are valid `error`s. + case kexec.CodeExitError: + return errType.ExitStatus(), nil + case *kerrors.StatusError: return int(errType.ErrStatus.Code), nil default: logger.Debugf(err.Error()) - // This is ugly but I don't know why the type assertion does not work... - // Whatever I've tried I can see the type "exec.CodeExitError" but none of the "case" nor other attempts with "errors.As()" worked :( - // So I'm parsing the Error string until we have a solution + // This is ugly, but it's a decent backup just in case the error isn't a type above. if strings.Contains(err.Error(), "command terminated with exit code") { a := strings.SplitAfter(err.Error(), "command terminated with exit code") return strconv.Atoi(strings.TrimSpace(a[1])) } - return 0, errors.Errorf("error %#v is not an ExitError nor CodeExitError but is %v", err, reflect.TypeOf(err)) + return -1, errors.Errorf("error %#v is an unknown error type: %v", err, reflect.TypeOf(err)) } } diff --git a/pkg/util/exec/exec_test.go b/pkg/util/exec/exec_test.go index dd948d0d4757b..ebb88a85d5b21 100644 --- a/pkg/util/exec/exec_test.go +++ b/pkg/util/exec/exec_test.go @@ -17,9 +17,16 @@ limitations under the License. package exec import ( - "errors" + "fmt" + "os" "os/exec" + "strconv" "testing" + + "github.com/pkg/errors" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kexec "k8s.io/utils/exec" ) func Test_assertErrorType(t *testing.T) { @@ -43,3 +50,91 @@ func Test_assertErrorType(t *testing.T) { }) } } + +func TestExtractExitCode(t *testing.T) { + mockExecExitError := func(retcode int) *exec.ExitError { + // we can't create an exec.ExitError directly, but we can get one by running a command that fails + // use go's type assertion to be sure we are returning exactly *exec.ExitError + cmd := mockExecCommandReturns("stdout", "stderr", retcode) + err := cmd.Run() + + ee, ok := err.(*exec.ExitError) + if !ok { + t.Fatalf("failed to create an *exec.ExitError. instead %T", err) + } + return ee + } + + expectError := true + noError := false + + tests := []struct { + name string + inputErr error + want int + wantErr bool + }{ + {"*exec.ExitError", + mockExecExitError(3), + 3, noError}, + /* {"exec.ExitError", // non-pointer case is impossible */ + {"*kexec.CodeExitError (pointer)", + &kexec.CodeExitError{Err: errors.New("some error"), Code: 4}, + 4, noError}, + {"kexec.CodeExitError (non-pointer)", + kexec.CodeExitError{Err: errors.New("some error"), Code: 5}, + 5, noError}, + {"*kerrors.StatusError", + &kerrors.StatusError{ErrStatus: metav1.Status{Code: 6}}, + 6, noError}, + /* {"kerrors.StatusError", // non-pointer case is impossible */ + {"unknown error type with error code extractable from error message", + errors.New("command terminated with exit code 7"), + 7, noError}, + {"unknown error type with no extractable error code", + errors.New("command with no extractable error code even with an int here: 8"), + 0, expectError}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ExtractExitCode(tt.inputErr) + if (err != nil) != tt.wantErr { + t.Errorf("ExtractExitCode() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("ExtractExitCode() = %v, want %v", got, tt.want) + } + }) + } +} + +// Mock an exec command where we really only care about the return values +// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go +func mockExecCommandReturns(stdout, stderr string, retcode int) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=TestExecHelperProcess") //nolint:gosec //Rook controls the input to the exec arguments + cmd.Env = append(os.Environ(), + "GO_WANT_HELPER_PROCESS=1", + fmt.Sprintf("GO_HELPER_PROCESS_STDOUT=%s", stdout), + fmt.Sprintf("GO_HELPER_PROCESS_STDERR=%s", stderr), + fmt.Sprintf("GO_HELPER_PROCESS_RETCODE=%d", retcode), + ) + return cmd +} + +// TestHelperProcess isn't a real test. It's used as a helper process. +// Inspired by: https://github.com/golang/go/blob/master/src/os/exec/exec_test.go +func TestExecHelperProcess(*testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { + return + } + + // test should set these in its environment to control the output of the test commands + fmt.Fprint(os.Stdout, os.Getenv("GO_HELPER_PROCESS_STDOUT")) + fmt.Fprint(os.Stderr, os.Getenv("GO_HELPER_PROCESS_STDERR")) + rc, err := strconv.Atoi(os.Getenv("GO_HELPER_PROCESS_RETCODE")) + if err != nil { + panic(err) + } + os.Exit(rc) +}