core: add missing error type check to exec

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))
 	}
 }

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 (won't compile) */
+		{"*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 (won't compile) */
+		{"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"),
+			-1, 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)
+}