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

cli/command/container: remove reportError, and put StatusError to use #5236

Merged
merged 1 commit into from
Jul 22, 2024
Merged

cli/command/container: remove reportError, and put StatusError to use #5236

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: 12 additions & 6 deletions cli/command/container/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command {

func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error {
if err := validatePullOpt(options.pull); err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
newEnv := []string{}
Expand All @@ -107,12 +109,16 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet,
copts.env = *opts.NewListOptsRef(&newEnv, nil)
containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
if err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
id, err := createContainer(ctx, dockerCli, containerCfg, options)
if err != nil {
Expand Down
12 changes: 6 additions & 6 deletions cli/command/container/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ func TestCreateContainerImagePullPolicyInvalid(t *testing.T) {

statusErr := cli.StatusError{}
assert.Check(t, errors.As(err, &statusErr))
assert.Equal(t, statusErr.StatusCode, 125)
assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg))
assert.Check(t, is.Equal(statusErr.StatusCode, 125))
laurazard marked this conversation as resolved.
Show resolved Hide resolved
assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg))
})
}
}
Expand Down Expand Up @@ -285,7 +285,7 @@ func TestNewCreateCommandWithWarnings(t *testing.T) {
for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
cli := test.NewFakeCli(&fakeClient{
fakeCLI := test.NewFakeCli(&fakeClient{
createContainerFunc: func(config *container.Config,
hostConfig *container.HostConfig,
networkingConfig *network.NetworkingConfig,
Expand All @@ -295,15 +295,15 @@ func TestNewCreateCommandWithWarnings(t *testing.T) {
return container.CreateResponse{}, nil
},
})
cmd := NewCreateCommand(cli)
cmd := NewCreateCommand(fakeCLI)
cmd.SetOut(io.Discard)
cmd.SetArgs(tc.args)
err := cmd.Execute()
assert.NilError(t, err)
if tc.warning {
golden.Assert(t, cli.ErrBuffer().String(), tc.name+".golden")
golden.Assert(t, fakeCLI.ErrBuffer().String(), tc.name+".golden")
} else {
assert.Equal(t, cli.ErrBuffer().String(), "")
assert.Equal(t, fakeCLI.ErrBuffer().String(), "")
}
})
}
Expand Down
78 changes: 46 additions & 32 deletions cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command {

func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error {
if err := validatePullOpt(ropts.pull); err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
newEnv := []string{}
Expand All @@ -99,12 +101,16 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro
containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
// just in case the parse does not exit
if err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
return runContainer(ctx, dockerCli, ropts, copts, containerCfg)
}
Expand Down Expand Up @@ -139,8 +145,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption

containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions)
if err != nil {
reportError(stderr, "run", err.Error(), true)
return runStartContainerErr(err)
return toStatusError(err)
}
if runOpts.sigProxy {
sigc := notifyAllSignals()
Expand Down Expand Up @@ -204,12 +209,11 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
<-errCh
}

reportError(stderr, "run", err.Error(), false)
if copts.autoRemove {
// wait container to be removed
<-statusChan
}
return runStartContainerErr(err)
return toStatusError(err)
Comment on lines -207 to +216
Copy link
Member Author

Choose a reason for hiding this comment

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

Slight change here; previously we would print the error before the container was removed; this would be for the "old API, --rm running on the client-side" case. I don't think this would make much of a difference in practice, and it would only be for old API versions (API 1.24 I think?)

}

if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
Expand Down Expand Up @@ -292,30 +296,40 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, containerID str
return resp.Close, nil
}

// reportError is a utility method that prints a user-friendly message
// containing the error that occurred during parsing and a suggestion to get help
func reportError(stderr io.Writer, name string, str string, withHelp bool) {
str = strings.TrimSuffix(str, ".") + "."
if withHelp {
str += "\nSee 'docker " + name + " --help'."
}
_, _ = fmt.Fprintln(stderr, "docker:", str)
// withHelp decorates the error with a suggestion to use "--help".
func withHelp(err error, commandName string) error {
return fmt.Errorf("docker: %w\n\nRun 'docker %s --help' for more information", err, commandName)
}

// if container start fails with 'not found'/'no such' error, return 127
// if container start fails with 'permission denied' error, return 126
// return 125 for generic docker daemon failures
func runStartContainerErr(err error) error {
trimmedErr := strings.TrimPrefix(err.Error(), "Error response from daemon: ")
statusError := cli.StatusError{StatusCode: 125}
if strings.Contains(trimmedErr, "executable file not found") ||
strings.Contains(trimmedErr, "no such file or directory") ||
strings.Contains(trimmedErr, "system cannot find the file specified") {
statusError = cli.StatusError{StatusCode: 127}
} else if strings.Contains(trimmedErr, syscall.EACCES.Error()) ||
strings.Contains(trimmedErr, syscall.EISDIR.Error()) {
statusError = cli.StatusError{StatusCode: 126}
// toStatusError attempts to detect specific error-conditions to assign
// an appropriate exit-code for situations where the problem originates
// from the container. It returns [cli.StatusError] with the original
// error message and the Status field set as follows:
//
// - 125: for generic failures sent back from the daemon
// - 126: if container start fails with 'permission denied' error
// - 127: if container start fails with 'not found'/'no such' error
func toStatusError(err error) error {
// TODO(thaJeztah): some of these errors originate from the container: should we actually suggest "--help" for those?

errMsg := err.Error()

if strings.Contains(errMsg, "executable file not found") || strings.Contains(errMsg, "no such file or directory") || strings.Contains(errMsg, "system cannot find the file specified") {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 127,
}
}

return statusError
if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 126,
}
}

return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
10 changes: 6 additions & 4 deletions cli/command/container/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,10 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) {
cmd.SetOut(io.Discard)
cmd.SetErr(io.Discard)
err := cmd.Execute()
assert.Assert(t, err != nil)
assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
statusErr := cli.StatusError{}
assert.Check(t, errors.As(err, &statusErr))
assert.Check(t, is.Equal(statusErr.StatusCode, 125))
assert.Check(t, is.ErrorContains(err, tc.expectedError))
})
}
}
Expand Down Expand Up @@ -179,8 +181,8 @@ func TestRunContainerImagePullPolicyInvalid(t *testing.T) {

statusErr := cli.StatusError{}
assert.Check(t, errors.As(err, &statusErr))
assert.Equal(t, statusErr.StatusCode, 125)
assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg))
assert.Check(t, is.Equal(statusErr.StatusCode, 125))
laurazard marked this conversation as resolved.
Show resolved Hide resolved
assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg))
})
}
}
Loading