Support whitespace in command path for gcp auth plugin

Specific use case is GKE users running gcloud/kubectl on Windows
with a cloud sdk installation path containing spaces. Also improving
test coverage using trick borrowed from exec_test.go

staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go

@@ -41,6 +41,9 @@ func init() {
 	}
 }
 
+// Stubbable for testing
+var execCommand = exec.Command
+
 // gcpAuthProvider is an auth provider plugin that uses GCP credentials to provide
 // tokens for kubectl to authenticate itself to the apiserver. A sample json config
 // is provided below with all recognized options described.
@@ -62,10 +65,13 @@ func init() {
 //       # These options direct the plugin to execute a specified command and parse
 //       # token and expiry time from the output of the command.
 //
-//       # Command to execute for access token. String is split on whitespace
-//       # with first field treated as the executable, remaining fields as args.
-//       # Command output will be parsed as JSON.
-//       "cmd-path": "/usr/bin/gcloud config config-helper --output=json",
+//       # Command to execute for access token. Command output will be parsed as JSON.
+//       # If "cmd-args" is not present, this value will be split on whitespace, with
+//       # the first element interpreted as the command, remaining elements as args.
+//       "cmd-path": "/usr/bin/gcloud",
+//
+//       # Arguments to pass to command to execute for access token.
+//       "cmd-args": "config config-helper --output=json"
 //
 //       # JSONPath to the string field that represents the access token in
 //       # command output. If omitted, defaults to "{.access_token}".
@@ -89,11 +95,21 @@ type gcpAuthProvider struct {
 }
 
 func newGCPAuthProvider(_ string, gcpConfig map[string]string, persister restclient.AuthProviderConfigPersister) (restclient.AuthProvider, error) {
-	cmd, useCmd := gcpConfig["cmd-path"]
 	var ts oauth2.TokenSource
 	var err error
-	if useCmd {
-		ts, err = newCmdTokenSource(cmd, gcpConfig["token-key"], gcpConfig["expiry-key"], gcpConfig["time-fmt"])
+	if cmd, useCmd := gcpConfig["cmd-path"]; useCmd {
+		if len(cmd) == 0 {
+			return nil, fmt.Errorf("missing access token cmd")
+		}
+		var args []string
+		if cmdArgs, ok := gcpConfig["cmd-args"]; ok {
+			args = strings.Fields(cmdArgs)
+		} else {
+			fields := strings.Fields(cmd)
+			cmd = fields[0]
+			args = fields[1:]
+		}
+		ts = newCmdTokenSource(cmd, args, gcpConfig["token-key"], gcpConfig["expiry-key"], gcpConfig["time-fmt"])
 	} else {
 		ts, err = google.DefaultTokenSource(context.Background(), "https://www.googleapis.com/auth/cloud-platform")
 	}
@@ -192,7 +208,7 @@ type commandTokenSource struct {
 	timeFmt   string
 }
 
-func newCmdTokenSource(cmd, tokenKey, expiryKey, timeFmt string) (*commandTokenSource, error) {
+func newCmdTokenSource(cmd string, args []string, tokenKey, expiryKey, timeFmt string) *commandTokenSource {
 	if len(timeFmt) == 0 {
 		timeFmt = time.RFC3339Nano
 	}
@@ -202,25 +218,21 @@ func newCmdTokenSource(cmd, tokenKey, expiryKey, timeFmt string) (*commandTokenS
 	if len(expiryKey) == 0 {
 		expiryKey = "{.token_expiry}"
 	}
-	fields := strings.Fields(cmd)
-	if len(fields) == 0 {
-		return nil, fmt.Errorf("missing access token cmd")
-	}
 	return &commandTokenSource{
-		cmd:       fields[0],
-		args:      fields[1:],
+		cmd:       cmd,
+		args:      args,
 		tokenKey:  tokenKey,
 		expiryKey: expiryKey,
 		timeFmt:   timeFmt,
-	}, nil
+	}
 }
 
 func (c *commandTokenSource) Token() (*oauth2.Token, error) {
-	fullCmd := fmt.Sprintf("%s %s", c.cmd, strings.Join(c.args, " "))
-	cmd := exec.Command(c.cmd, c.args...)
+	fullCmd := strings.Join(append([]string{c.cmd}, c.args...), " ")
+	cmd := execCommand(c.cmd, c.args...)
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("error executing access token command %q: %v", fullCmd, err)
+		return nil, fmt.Errorf("error executing access token command %q: err=%v output=%s", fullCmd, err, output)
 	}
 	token, err := c.parseTokenCmdOutput(output)
 	if err != nil {
@@ -241,11 +253,11 @@ func (c *commandTokenSource) parseTokenCmdOutput(output []byte) (*oauth2.Token,
 
 	accessToken, err := parseJSONPath(data, "token-key", c.tokenKey)
 	if err != nil {
-		return nil, fmt.Errorf("error parsing token-key %q: %v", c.tokenKey, err)
+		return nil, fmt.Errorf("error parsing token-key %q from %q: %v", c.tokenKey, string(output), err)
 	}
 	expiryStr, err := parseJSONPath(data, "expiry-key", c.expiryKey)
 	if err != nil {
-		return nil, fmt.Errorf("error parsing expiry-key %q: %v", c.expiryKey, err)
+		return nil, fmt.Errorf("error parsing expiry-key %q from %q: %v", c.expiryKey, string(output), err)
 	}
 	var expiry time.Time
 	if t, err := time.Parse(c.timeFmt, expiryStr); err != nil {

staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp_test.go

@@ -18,6 +18,8 @@ package gcp
 
 import (
 	"fmt"
+	"os"
+	"os/exec"
 	"reflect"
 	"strings"
 	"sync"
@@ -27,118 +29,229 @@ import (
 	"golang.org/x/oauth2"
 )
 
+type fakeOutput struct {
+	args   []string
+	output string
+}
+
+var (
+	wantCmd []string
+	// Output for fakeExec, keyed by command
+	execOutputs = map[string]fakeOutput{
+		"/default/no/args": {
+			args: []string{},
+			output: `{
+  "access_token": "faketoken",
+  "token_expiry": "2016-10-31T22:31:09.123000000Z"
+}`},
+		"/default/legacy/args": {
+			args: []string{"arg1", "arg2", "arg3"},
+			output: `{
+  "access_token": "faketoken",
+  "token_expiry": "2016-10-31T22:31:09.123000000Z"
+}`},
+		"/space in path/customkeys": {
+			args: []string{"can", "haz", "auth"},
+			output: `{
+  "token": "faketoken",
+  "token_expiry": {
+    "datetime": "2016-10-31 22:31:09.123"
+  }
+}`},
+		"missing/tokenkey/noargs": {
+			args: []string{},
+			output: `{
+  "broken": "faketoken",
+  "token_expiry": {
+    "datetime": "2016-10-31 22:31:09.123000000Z"
+  }
+}`},
+		"missing/expirykey/legacyargs": {
+			args: []string{"split", "on", "whitespace"},
+			output: `{
+  "access_token": "faketoken",
+  "expires": "2016-10-31T22:31:09.123000000Z"
+}`},
+		"invalid expiry/timestamp": {
+			args: []string{"foo", "--bar", "--baz=abc,def"},
+			output: `{
+  "access_token": "faketoken",
+  "token_expiry": "sometime soon, idk"
+}`},
+		"badjson": {
+			args: []string{},
+			output: `{
+  "access_token": "faketoken",
+  "token_expiry": "sometime soon, idk"
+  ------
+`},
+	}
+)
+
+func fakeExec(command string, args ...string) *exec.Cmd {
+	cs := []string{"-test.run=TestHelperProcess", "--", command}
+	cs = append(cs, args...)
+	cmd := exec.Command(os.Args[0], cs...)
+	cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
+	return cmd
+}
+
+func TestHelperProcess(t *testing.T) {
+	if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
+		return
+	}
+	// Strip out the leading args used to exec into this function.
+	gotCmd := os.Args[3]
+	gotArgs := os.Args[4:]
+	output, ok := execOutputs[gotCmd]
+	if !ok {
+		fmt.Fprintf(os.Stdout, "unexpected call cmd=%q args=%v\n", gotCmd, gotArgs)
+		os.Exit(1)
+	} else if !reflect.DeepEqual(output.args, gotArgs) {
+		fmt.Fprintf(os.Stdout, "call cmd=%q got args %v, want: %v\n", gotCmd, gotArgs, output.args)
+		os.Exit(1)
+	}
+	fmt.Fprintf(os.Stdout, output.output)
+	os.Exit(0)
+}
+
+func errEquiv(got, want error) bool {
+	if got == want {
+		return true
+	}
+	if got != nil && want != nil {
+		return strings.Contains(got.Error(), want.Error())
+	}
+	return false
+}
+
 func TestCmdTokenSource(t *testing.T) {
+	execCommand = fakeExec
 	fakeExpiry := time.Date(2016, 10, 31, 22, 31, 9, 123000000, time.UTC)
 	customFmt := "2006-01-02 15:04:05.999999999"
 
 	tests := []struct {
 		name             string
-		output                            []byte
-		cmd, tokenKey, expiryKey, timeFmt string
+		gcpConfig        map[string]string
 		tok              *oauth2.Token
-		expectErr                         error
+		newErr, tokenErr error
 	}{
 		{
-			"defaults",
-			[]byte(`{
-  "access_token": "faketoken",
-  "token_expiry": "2016-10-31T22:31:09.123000000Z"
-}`),
-			"/fake/cmd/path", "", "", "",
+			"default",
+			map[string]string{
+				"cmd-path": "/default/no/args",
+			},
 			&oauth2.Token{
 				AccessToken: "faketoken",
 				TokenType:   "Bearer",
 				Expiry:      fakeExpiry,
 			},
 			nil,
+			nil,
 		},
 		{
-			"custom keys",
-			[]byte(`{
-  "token": "faketoken",
-  "token_expiry": {
-    "datetime": "2016-10-31 22:31:09.123"
-  }
-}`),
-			"/fake/cmd/path", "{.token}", "{.token_expiry.datetime}", customFmt,
+			"default legacy args",
+			map[string]string{
+				"cmd-path": "/default/legacy/args arg1 arg2 arg3",
+			},
 			&oauth2.Token{
 				AccessToken: "faketoken",
 				TokenType:   "Bearer",
 				Expiry:      fakeExpiry,
 			},
 			nil,
+			nil,
+		},
+
+		{
+			"custom keys",
+			map[string]string{
+				"cmd-path":   "/space in path/customkeys",
+				"cmd-args":   "can haz auth",
+				"token-key":  "{.token}",
+				"expiry-key": "{.token_expiry.datetime}",
+				"time-fmt":   customFmt,
+			},
+			&oauth2.Token{
+				AccessToken: "faketoken",
+				TokenType:   "Bearer",
+				Expiry:      fakeExpiry,
+			},
+			nil,
+			nil,
 		},
 		{
 			"missing cmd",
-			nil,
-			"", "", "", "",
+			map[string]string{
+				"cmd-path": "",
+			},
 			nil,
 			fmt.Errorf("missing access token cmd"),
+			nil,
 		},
 		{
 			"missing token-key",
-			[]byte(`{
-  "broken": "faketoken",
-  "token_expiry": {
-    "datetime": "2016-10-31 22:31:09.123000000Z"
-  }
-}`),
-			"/fake/cmd/path", "{.token}", "", "",
+			map[string]string{
+				"cmd-path":  "missing/tokenkey/noargs",
+				"token-key": "{.token}",
+			},
+			nil,
 			nil,
 			fmt.Errorf("error parsing token-key %q", "{.token}"),
 		},
+
 		{
 			"missing expiry-key",
-			[]byte(`{
-  "access_token": "faketoken",
-  "expires": "2016-10-31T22:31:09.123000000Z"
-}`),
-			"/fake/cmd/path", "", "{.expiry}", "",
+			map[string]string{
+				"cmd-path":   "missing/expirykey/legacyargs split on whitespace",
+				"expiry-key": "{.expiry}",
+			},
+			nil,
 			nil,
 			fmt.Errorf("error parsing expiry-key %q", "{.expiry}"),
 		},
 		{
 			"invalid expiry timestamp",
-			[]byte(`{
-  "access_token": "faketoken",
-  "token_expiry": "sometime soon, idk"
-}`),
-			"/fake/cmd/path", "", "", "",
+			map[string]string{
+				"cmd-path": "invalid expiry/timestamp",
+				"cmd-args": "foo --bar --baz=abc,def",
+			},
 			&oauth2.Token{
 				AccessToken: "faketoken",
 				TokenType:   "Bearer",
 				Expiry:      time.Time{},
 			},
 			nil,
+			nil,
 		},
 		{
 			"bad JSON",
-			[]byte(`{
-  "access_token": "faketoken",
-  "token_expiry": "sometime soon, idk"
-  ------
-`),
-			"/fake/cmd", "", "", "",
+			map[string]string{
+				"cmd-path": "badjson",
+			},
+			nil,
 			nil,
 			fmt.Errorf("invalid character '-' after object key:value pair"),
 		},
 	}
 
 	for _, tc := range tests {
-		ts, err := newCmdTokenSource(tc.cmd, tc.tokenKey, tc.expiryKey, tc.timeFmt)
-		if err != nil {
-			if !strings.Contains(err.Error(), tc.expectErr.Error()) {
-				t.Errorf("%s newCmdTokenSource error: %v, want %v", tc.name, err, tc.expectErr)
-			}
+		provider, err := newGCPAuthProvider("", tc.gcpConfig, nil /* persister */)
+		if !errEquiv(err, tc.newErr) {
+			t.Errorf("%q newGCPAuthProvider error: got %v, want %v", tc.name, err, tc.newErr)
 			continue
 		}
-		tok, err := ts.parseTokenCmdOutput(tc.output)
-
-		if err != tc.expectErr && !strings.Contains(err.Error(), tc.expectErr.Error()) {
-			t.Errorf("%s parseCmdTokenSource error: %v, want %v", tc.name, err, tc.expectErr)
+		if err != nil {
+			continue
+		}
+		ts := provider.(*gcpAuthProvider).tokenSource.(*cachedTokenSource).source.(*commandTokenSource)
+		wantCmd = append([]string{ts.cmd}, ts.args...)
+		tok, err := ts.Token()
+		if !errEquiv(err, tc.tokenErr) {
+			t.Errorf("%q Token() error: got %v, want %v", tc.name, err, tc.tokenErr)
 		}
 		if !reflect.DeepEqual(tok, tc.tok) {
-			t.Errorf("%s got token %v, want %v", tc.name, tok, tc.tok)
+			t.Errorf("%q Token() got %v, want %v", tc.name, tok, tc.tok)
 		}
 	}
 }
@@ -161,6 +274,7 @@ func (f *fakePersister) Persist(cache map[string]string) error {
 func (f *fakePersister) read() map[string]string {
 	ret := map[string]string{}
 	f.lk.Lock()
+	defer f.lk.Unlock()
 	for k, v := range f.cache {
 		ret[k] = v
 	}