Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exec credential provider: handle wrapped exec errors #103772

Closed
wants to merge 1 commit into from
Closed

exec credential provider: handle wrapped exec errors #103772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,12 @@ func (a *Authenticator) refreshCredsLocked(r *clientauthentication.Response) err
//
// It must be called while holding the Authenticator's mutex.
func (a *Authenticator) wrapCmdRunErrorLocked(err error) error {
switch err.(type) {
case *exec.Error: // Binary does not exist (see exec.Error).
execError := &exec.Error{}
exitError := &exec.ExitError{}
switch {
case errors.As(err, &execError): // Binary does not exist (see exec.Error).
Copy link
Contributor Author

@ankeesler ankeesler Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't figure out how to work in os.IsNotExist() here. I tried both errors.As(err, &execError) && os.IsNotExist(err) and errors.As(err, &execError) && os.IsNotExist(execError), but these logical expressions never evaluated to true for me.

builder := strings.Builder{}
fmt.Fprintf(&builder, "exec: executable %s not found", a.cmd)
fmt.Fprintf(&builder, "exec: binary %s not found", a.cmd)

a.sometimes.Do(func() {
fmt.Fprint(&builder, installHintVerboseHelp)
Expand All @@ -524,14 +526,16 @@ func (a *Authenticator) wrapCmdRunErrorLocked(err error) error {

return errors.New(builder.String())

case *exec.ExitError: // Binary execution failed (see exec.Cmd.Run()).
e := err.(*exec.ExitError)
case errors.As(err, &exitError): // Binary execution failed (see exec.Cmd.Run()).
return fmt.Errorf(
"exec: executable %s failed with exit code %d",
"exec: binary %s failed with exit code %d",
a.cmd,
e.ProcessState.ExitCode(),
exitError.ProcessState.ExitCode(),
)

case os.IsPermission(err):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this how you were hoping to use os.IsPermission()?

return fmt.Errorf("exec: binary %s not executable", a.cmd)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in love with changing this error message to say "binary" instead of "executable" (because the plugin could be a bash script, which I would not consider to be a binary), but I wanted to avoid the redundancy of the error message "executable blah is not executable". Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new unit test message binary ./testdata/not-executable.sh not executable seems a little odd as @ankeesler pointed at the above.
How about keeping "executable" for the other places and removing "executable" from the above case os.IsPermission only to avoid unnecessary confusions?


default:
return fmt.Errorf("exec: %v", err)
}
Expand Down
51 changes: 33 additions & 18 deletions staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ func TestRefreshCreds(t *testing.T) {
wantCreds credentials
wantExpiry time.Time
wantErr bool
wantErrSubstr string
wantErrSubstrs []string
}{
{
name: "basic-request",
Expand Down Expand Up @@ -701,8 +701,8 @@ func TestRefreshCreds(t *testing.T) {
APIVersion: "client.authentication.k8s.io/v1beta1",
InteractiveMode: api.AlwaysExecInteractiveMode,
},
wantErr: true,
wantErrSubstr: "exec plugin cannot support interactive mode: standard input is not a terminal",
wantErr: true,
wantErrSubstrs: []string{"exec plugin cannot support interactive mode: standard input is not a terminal"},
},
{
name: "beta-basic-request-with-always-interactive-mode-and-terminal-and-stdin-unavailable",
Expand All @@ -711,9 +711,9 @@ func TestRefreshCreds(t *testing.T) {
InteractiveMode: api.AlwaysExecInteractiveMode,
StdinUnavailable: true,
},
isTerminal: true,
wantErr: true,
wantErrSubstr: "exec plugin cannot support interactive mode: standard input is unavailable",
isTerminal: true,
wantErr: true,
wantErrSubstrs: []string{"exec plugin cannot support interactive mode: standard input is unavailable"},
},
{
name: "beta-basic-request-with-always-interactive-mode-and-terminal-and-stdin-unavailable-with-message",
Expand All @@ -723,9 +723,9 @@ func TestRefreshCreds(t *testing.T) {
StdinUnavailable: true,
StdinUnavailableMessage: "some message",
},
isTerminal: true,
wantErr: true,
wantErrSubstr: "exec plugin cannot support interactive mode: standard input is unavailable: some message",
isTerminal: true,
wantErr: true,
wantErrSubstrs: []string{"exec plugin cannot support interactive mode: standard input is unavailable: some message"},
},
{
name: "beta-basic-request-with-always-interactive-mode-and-terminal",
Expand Down Expand Up @@ -821,18 +821,29 @@ func TestRefreshCreds(t *testing.T) {
InstallHint: "some install hint",
InteractiveMode: api.IfAvailableExecInteractiveMode,
},
wantErr: true,
wantErrSubstr: "some install hint",
wantErr: true,
wantErrSubstrs: []string{"binary does not exist not found", "some install hint"},
},
{
name: "binary-not-executable",
config: api.ExecConfig{
APIVersion: "client.authentication.k8s.io/v1beta1",
Command: "./testdata/not-executable.sh",
InstallHint: "some install hint",
InteractiveMode: api.IfAvailableExecInteractiveMode,
},
wantErr: true,
wantErrSubstrs: []string{"binary ./testdata/not-executable.sh not executable"},
},
{
name: "binary-fails",
config: api.ExecConfig{
APIVersion: "client.authentication.k8s.io/v1beta1",
InteractiveMode: api.IfAvailableExecInteractiveMode,
},
exitCode: 73,
wantErr: true,
wantErrSubstr: "73",
exitCode: 73,
wantErr: true,
wantErrSubstrs: []string{"73"},
},
{
name: "alpha-with-cluster-is-ignored",
Expand Down Expand Up @@ -1007,8 +1018,8 @@ func TestRefreshCreds(t *testing.T) {
config: api.ExecConfig{
APIVersion: "client.authentication.k8s.io/v1",
},
wantErr: true,
wantErrSubstr: `exec plugin cannot support interactive mode: unknown interactiveMode: ""`,
wantErr: true,
wantErrSubstrs: []string{`exec plugin cannot support interactive mode: unknown interactiveMode: ""`},
},
}

Expand Down Expand Up @@ -1040,8 +1051,12 @@ func TestRefreshCreds(t *testing.T) {
if err := a.refreshCredsLocked(test.response); err != nil {
if !test.wantErr {
t.Errorf("get token %v", err)
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
t.Errorf("expected error with substring '%v' got '%v'", test.wantErrSubstr, err.Error())
} else {
for _, wantErrSubstr := range test.wantErrSubstrs {
if !strings.Contains(err.Error(), wantErrSubstr) {
t.Errorf("expected error with substring '%v' got '%v'", wantErrSubstr, err.Error())
}
}
}
return
}
Expand Down
17 changes: 17 additions & 0 deletions staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/testdata/not-executable.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/bin/bash -e

# Copyright 2021 The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# This file is not executable.
6 changes: 3 additions & 3 deletions test/integration/client/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ func execPluginClientTests(t *testing.T, unauthorizedCert, unauthorizedKey []byt
clientConfigFunc: func(c *rest.Config) {
c.ExecProvider.Command = "does not exist"
},
wantGetCertificateErrorPrefix: "exec: executable does not exist not found",
wantGetCertificateErrorPrefix: "exec: binary does not exist not found",
wantClientErrorPrefix: `Get "https`,
wantMetrics: &execPluginMetrics{calls: []execPluginCall{{exitCode: 1, callStatus: "plugin_not_found_error"}}},
},
Expand All @@ -389,7 +389,7 @@ func execPluginClientTests(t *testing.T, unauthorizedCert, unauthorizedKey []byt
clientConfigFunc: func(c *rest.Config) {
c.ExecProvider.Command = "./testdata/exec-plugin-not-executable.sh"
},
wantGetCertificateErrorPrefix: "exec: fork/exec ./testdata/exec-plugin-not-executable.sh: permission denied",
wantGetCertificateErrorPrefix: "exec: binary ./testdata/exec-plugin-not-executable.sh not executable",
wantClientErrorPrefix: `Get "https`,
wantMetrics: &execPluginMetrics{calls: []execPluginCall{{exitCode: 1, callStatus: "plugin_not_found_error"}}},
},
Expand All @@ -403,7 +403,7 @@ func execPluginClientTests(t *testing.T, unauthorizedCert, unauthorizedKey []byt
},
}
},
wantGetCertificateErrorPrefix: "exec: executable testdata/exec-plugin.sh failed with exit code 10",
wantGetCertificateErrorPrefix: "exec: binary testdata/exec-plugin.sh failed with exit code 10",
wantClientErrorPrefix: `Get "https`,
wantMetrics: &execPluginMetrics{calls: []execPluginCall{{exitCode: 10, callStatus: "plugin_execution_error"}}},
},
Expand Down