From 995ecfe84ea92d77cac2921babbbe03f7c2e4967 Mon Sep 17 00:00:00 2001 From: Jeff Lowdermilk Date: Fri, 17 Feb 2017 10:01:20 -0800 Subject: [PATCH] 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 --- .../plugin/pkg/client/auth/gcp/gcp.go | 52 ++-- .../plugin/pkg/client/auth/gcp/gcp_test.go | 238 +++++++++++++----- 2 files changed, 208 insertions(+), 82 deletions(-) diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go index ac9faf46c8e..142ffdd50eb 100644 --- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp.go +++ b/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 { diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp_test.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp_test.go index cb64b8380ca..755fbcd8f48 100644 --- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/gcp/gcp_test.go +++ b/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" ) -func TestCmdTokenSource(t *testing.T) { - fakeExpiry := time.Date(2016, 10, 31, 22, 31, 9, 123000000, time.UTC) - customFmt := "2006-01-02 15:04:05.999999999" +type fakeOutput struct { + args []string + output string +} - tests := []struct { - name string - output []byte - cmd, tokenKey, expiryKey, timeFmt string - tok *oauth2.Token - expectErr error - }{ - { - "defaults", - []byte(`{ +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" -}`), - "/fake/cmd/path", "", "", "", - &oauth2.Token{ - AccessToken: "faketoken", - TokenType: "Bearer", - Expiry: fakeExpiry, - }, - nil, - }, - { - "custom keys", - []byte(`{ +}`}, + "/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" } -}`), - "/fake/cmd/path", "{.token}", "{.token_expiry.datetime}", customFmt, +}`}, + "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 + gcpConfig map[string]string + tok *oauth2.Token + newErr, tokenErr error + }{ + { + "default", + map[string]string{ + "cmd-path": "/default/no/args", + }, &oauth2.Token{ AccessToken: "faketoken", TokenType: "Bearer", Expiry: fakeExpiry, }, nil, + nil, + }, + { + "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 }