Skip to content

context: improvements for handling non-existing contexts and errors #3868

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

Closed
thaJeztah wants to merge 5 commits into from
Closed

context: improvements for handling non-existing contexts and errors #3868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
566ea61
cli/command: DockerCli: keep reference to options for later use
thaJeztah Nov 4, 2022
37b6dce
cli/command: add Cli.CurrentVersion() function
thaJeztah Nov 4, 2022
edaf16a
cli/command/context: context ls: add ERROR column, and don't fail early
thaJeztah Nov 4, 2022
96e97de
cli/command/context: context ls: always show current context
thaJeztah Nov 4, 2022
685449f
cli/command/context: allow context show to print context, even on error
thaJeztah Nov 4, 2022
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
20 changes: 15 additions & 5 deletions cli/command/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type Cli interface {
ServerInfo() ServerInfo
NotaryClient(imgRefAndAuth trust.ImageRefAndAuth, actions []string) (notaryclient.Repository, error)
DefaultVersion() string
CurrentVersion() string
ManifestStore() manifeststore.Store
RegistryClient(bool) registryclient.RegistryClient
ContentTrustEnabled() bool
Expand All @@ -68,14 +69,14 @@ type Cli interface {
// Instances of the client can be returned from NewDockerCli.
type DockerCli struct {
configFile *configfile.ConfigFile
options *cliflags.ClientOptions
in *streams.In
out *streams.Out
err io.Writer
client client.APIClient
serverInfo ServerInfo
contentTrust bool
contextStore store.Store
currentContext string
dockerEndpoint docker.Endpoint
contextStoreConfig store.Config
initTimeout time.Duration
Expand All @@ -86,6 +87,15 @@ func (cli *DockerCli) DefaultVersion() string {
return api.DefaultVersion
}

// CurrentVersion returns the API version currently negotiated, or the default
// version otherwise.
func (cli *DockerCli) CurrentVersion() string {
if cli.client == nil {
return api.DefaultVersion
}
return cli.client.ClientVersion()
}

// Client returns the APIClient
func (cli *DockerCli) Client() client.APIClient {
return cli.client
Expand Down Expand Up @@ -216,16 +226,16 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
}

cli.loadConfigFile()
cli.options = opts

baseContextStore := store.New(config.ContextStoreDir(), cli.contextStoreConfig)
cli.contextStore = &ContextStoreWithDefault{
Store: baseContextStore,
Resolver: func() (*DefaultContext, error) {
return ResolveDefaultContext(opts, cli.contextStoreConfig)
return ResolveDefaultContext(cli.options, cli.contextStoreConfig)
},
}
cli.currentContext = resolveContextName(opts, cli.configFile)
cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, cli.currentContext)
cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, resolveContextName(opts, cli.configFile))
if err != nil {
return errors.Wrap(err, "unable to resolve docker endpoint")
}
Expand Down Expand Up @@ -390,7 +400,7 @@ func (cli *DockerCli) ContextStore() store.Store {
// CurrentContext does not validate if the given context exists or if it's
// valid; errors may occur when trying to use it.
func (cli *DockerCli) CurrentContext() string {
return cli.currentContext
return resolveContextName(cli.options, cli.configFile)
}

// CurrentContext returns the current context name, based on flags,
Expand Down
2 changes: 1 addition & 1 deletion cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func runRun(dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copt
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
}
if err = validateAPIVersion(containerConfig, dockerCli.Client().ClientVersion()); err != nil {
if err = validateAPIVersion(containerConfig, dockerCli.CurrentVersion()); err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
}
Expand Down
32 changes: 30 additions & 2 deletions cli/command/context/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,26 +50,54 @@ func runList(dockerCli command.Cli, opts *listOptions) error {
}
var (
curContext = dockerCli.CurrentContext()
curFound bool
contexts []*formatter.ClientContext
)
for _, rawMeta := range contextMap {
isCurrent := rawMeta.Name == curContext
if isCurrent {
curFound = true
}
meta, err := command.GetDockerContext(rawMeta)
if err != nil {
return err
// Add a stub-entry to the list, including the error-message
// indicating that the context couldn't be loaded.
contexts = append(contexts, &formatter.ClientContext{
Name: rawMeta.Name,
Current: isCurrent,
Error: err.Error(),
})
continue
}
var errMsg string
dockerEndpoint, err := docker.EndpointFromContext(rawMeta)
if err != nil {
return err
errMsg = err.Error()
}
desc := formatter.ClientContext{
Name: rawMeta.Name,
Current: isCurrent,
Description: meta.Description,
DockerEndpoint: dockerEndpoint.Host,
Error: errMsg,
}
contexts = append(contexts, &desc)
}
if !curFound {
// The currently specified context wasn't found. We add a stub-entry
// to the list, including the error-message indicating that the context
// wasn't found.
var errMsg string
_, err := dockerCli.ContextStore().GetMetadata(curContext)
if err != nil {
errMsg = err.Error()
}
contexts = append(contexts, &formatter.ClientContext{
Name: curContext,
Current: true,
Error: errMsg,
})
}
sort.Slice(contexts, func(i, j int) bool {
return sortorder.NaturalLess(contexts[i].Name, contexts[j].Name)
})
Expand Down
8 changes: 8 additions & 0 deletions cli/command/context/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,11 @@ func TestListQuiet(t *testing.T) {
assert.NilError(t, runList(cli, &listOptions{quiet: true}))
golden.Assert(t, cli.OutBuffer().String(), "quiet-list.golden")
}

func TestListError(t *testing.T) {
cli := makeFakeCli(t)
cli.SetCurrentContext("nosuchcontext")
cli.OutBuffer().Reset()
assert.NilError(t, runList(cli, &listOptions{}))
golden.Assert(t, cli.OutBuffer().String(), "list-with-error.golden")
}
4 changes: 2 additions & 2 deletions cli/command/context/show.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ func newShowCommand(dockerCli command.Cli) *cobra.Command {

func runShow(dockerCli command.Cli) error {
context := dockerCli.CurrentContext()
metadata, err := dockerCli.ContextStore().GetMetadata(context)
fmt.Fprintln(dockerCli.Out(), context)
_, err := dockerCli.ContextStore().GetMetadata(context)
if err != nil {
return err
}
fmt.Fprintln(dockerCli.Out(), metadata.Name)
return nil
}
Comment on lines 26 to 34
Copy link
Member Author

@thaJeztah thaJeztah Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As this is a new command, we can still change; do we think we should produce an error at all if the context wasn't found, or just "show the name of the context" (basically: this is what's configured to use, but you'll learn if it works once you use it)?

We could still opt for printing the error (on STDERR) as a warning, but make the command always return a zero exit code ("success").

Copy link
Contributor

@sam-thibault sam-thibault Nov 21, 2022

Choose a reason for hiding this comment

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

It seems like it should mimic existing behavior for other commands. e.g. for inspect:

if dockerCli.CurrentContext() == "" {
	return errors.New("no context specified")
}

Copy link
Member Author

@thaJeztah thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

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

Hm... right; so docker context inspect expects a positional argument. It looks like there's a bug in the compose-cli wrapper, as it hijacks the docker context subcommands, and it looks like the usage information is incorrect;

This is the output for compose-cli;

docker context inspect --help
Display detailed information on one or more contexts

Usage:
  docker context inspect [flags]

Flags:
  -f, --format string   Format the output using the given Go template
  -h, --help            Help for inspect

And the output for the actual docker cli;

com.docker.cli context inspect --help

Usage:  docker context inspect [OPTIONS] [CONTEXT] [CONTEXT...]

Display detailed information on one or more contexts

Options:
  -f, --format string   Format the output using the given Go template

Copy link
Member Author

@thaJeztah thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

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

As to the docker context show; that command was added in #3567. Basically its sole purpose is to show what context is configured, so that it can be used in prompts. An earlier PR (#2500) added it to docker info for that purpose, but docker context show is more convenient.

As it (likely) will be used for prompts, the question is, should it always validate? Without validation, the value is still valid (it shows what's configured), but even if we check if the context is valid (can be found, and decoded), it may still be pointing to a host that's not reachable.

3 changes: 3 additions & 0 deletions cli/command/context/testdata/list-with-error.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
NAME DESCRIPTION DOCKER ENDPOINT ERROR
default Current DOCKER_HOST based configuration unix:///var/run/docker.sock
nosuchcontext * load context "nosuchcontext": context does n…
10 changes: 5 additions & 5 deletions cli/command/context/testdata/list.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
NAME DESCRIPTION DOCKER ENDPOINT
current * description of current https://someswarmserver.example.com
default Current DOCKER_HOST based configuration unix:///var/run/docker.sock
other description of other https://someswarmserver.example.com
unset description of unset https://someswarmserver.example.com
NAME DESCRIPTION DOCKER ENDPOINT ERROR
Copy link
Contributor

@sam-thibault sam-thibault Nov 21, 2022

Choose a reason for hiding this comment

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

ERROR looks like a copy/paste mistake here

Copy link
Member Author

@thaJeztah thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

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

It's the title for the ERROR column that was added, but looking at this, it looks like I have more work to do. The context store itself already fails "full stop" if it encounters an error, so without some of the other changes from #3847, it looks like this doesn't work 😞

current * description of current https://someswarmserver.example.com
default Current DOCKER_HOST based configuration unix:///var/run/docker.sock
other description of other https://someswarmserver.example.com
unset description of unset https://someswarmserver.example.com
16 changes: 12 additions & 4 deletions cli/command/formatter/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package formatter

const (
// ClientContextTableFormat is the default client context format
ClientContextTableFormat = "table {{.Name}}{{if .Current}} *{{end}}\t{{.Description}}\t{{.DockerEndpoint}}"
// ClientContextTableFormat is the default client context format.
ClientContextTableFormat = "table {{.Name}}{{if .Current}} *{{end}}\t{{.Description}}\t{{.DockerEndpoint}}\t{{.Error}}"

dockerEndpointHeader = "DOCKER ENDPOINT"
quietContextFormat = "{{.Name}}"
Expand All @@ -11,10 +11,10 @@ const (
// NewClientContextFormat returns a Format for rendering using a Context
func NewClientContextFormat(source string, quiet bool) Format {
if quiet {
return Format(quietContextFormat)
return quietContextFormat
}
if source == TableFormatKey {
return Format(ClientContextTableFormat)
return ClientContextTableFormat
}
return Format(source)
}
Expand All @@ -25,6 +25,7 @@ type ClientContext struct {
Description string
DockerEndpoint string
Current bool
Error string
}

// ClientContextWrite writes formatted contexts using the Context
Expand All @@ -51,6 +52,7 @@ func newClientContextContext() *clientContextContext {
"Name": NameHeader,
"Description": DescriptionHeader,
"DockerEndpoint": dockerEndpointHeader,
"Error": ErrorHeader,
}
return &ctx
}
Expand All @@ -75,6 +77,12 @@ func (c *clientContextContext) DockerEndpoint() string {
return c.c.DockerEndpoint
}

// Error returns the truncated error (if any) that occurred when loading the context.
func (c *clientContextContext) Error() string {
// TODO(thaJeztah) add "--no-trunc" option to context ls and set default to 30 cols to match "docker service ps"
return Ellipsis(c.c.Error, 45)
}

// KubernetesEndpoint returns the kubernetes endpoint.
//
// Deprecated: support for kubernetes endpoints in contexts has been removed, and this formatting option will always be empty.
Expand Down
1 change: 1 addition & 0 deletions cli/command/formatter/custom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const (
StatusHeader = "STATUS"
PortsHeader = "PORTS"
ImageHeader = "IMAGE"
ErrorHeader = "ERROR"
ContainerIDHeader = "CONTAINER ID"
)

Expand Down
2 changes: 1 addition & 1 deletion cli/command/system/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func runVersion(dockerCli command.Cli, opts *versionOptions) error {
Client: clientVersion{
Platform: struct{ Name string }{version.PlatformName},
Version: version.Version,
APIVersion: dockerCli.Client().ClientVersion(),
APIVersion: dockerCli.CurrentVersion(),
DefaultAPIVersion: dockerCli.DefaultVersion(),
GoVersion: runtime.Version(),
GitCommit: version.GitCommit,
Expand Down
8 changes: 4 additions & 4 deletions cli/command/task/formatter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func FormatWrite(ctx formatter.Context, tasks []swarm.Task, names map[string]str
"Node": nodeHeader,
"DesiredState": desiredStateHeader,
"CurrentState": currentStateHeader,
"Error": errorHeader,
"Error": formatter.ErrorHeader,
"Ports": formatter.PortsHeader,
}
return ctx.Write(&taskCtx, render)
Expand Down Expand Up @@ -124,11 +124,11 @@ func (c *taskContext) CurrentState() string {
func (c *taskContext) Error() string {
// Trim and quote the error message.
taskErr := c.task.Status.Err
if c.trunc && len(taskErr) > maxErrLength {
taskErr = fmt.Sprintf("%s…", taskErr[:maxErrLength-1])
if c.trunc {
taskErr = formatter.Ellipsis(taskErr, maxErrLength)
}
if len(taskErr) > 0 {
taskErr = fmt.Sprintf("\"%s\"", taskErr)
taskErr = fmt.Sprintf(`"%s"`, taskErr)
}
return taskErr
}
Expand Down
17 changes: 8 additions & 9 deletions cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
cliflags "github.com/docker/cli/cli/flags"
"github.com/docker/cli/cli/version"
"github.com/docker/docker/api/types/versions"
"github.com/docker/docker/client"
"github.com/moby/buildkit/util/appcontext"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -274,7 +273,7 @@ func main() {
}

type versionDetails interface {
Client() client.APIClient
CurrentVersion() string
ServerInfo() command.ServerInfo
}

Expand Down Expand Up @@ -333,7 +332,7 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
return false
}
}
versionOlderThan = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
versionOlderThan = func(v string) bool { return versions.LessThan(details.CurrentVersion(), v) }
)

cmd.Flags().VisitAll(func(f *pflag.Flag) {
Expand Down Expand Up @@ -390,8 +389,8 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
if !f.Changed {
return
}
if !isVersionSupported(f, details.Client().ClientVersion()) {
errs = append(errs, fmt.Sprintf(`"--%s" requires API version %s, but the Docker daemon API version is %s`, f.Name, getFlagAnnotation(f, "version"), details.Client().ClientVersion()))
if !isVersionSupported(f, details.CurrentVersion()) {
errs = append(errs, fmt.Sprintf(`"--%s" requires API version %s, but the Docker daemon API version is %s`, f.Name, getFlagAnnotation(f, "version"), details.CurrentVersion()))
return
}
if !isOSTypeSupported(f, details.ServerInfo().OSType) {
Expand All @@ -417,11 +416,11 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
for curr := cmd; curr != nil; curr = curr.Parent() {
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.Client().ClientVersion(), cmdVersion) {
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.Client().ClientVersion())
if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.CurrentVersion(), cmdVersion) {
return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.CurrentVersion())
}
if os, ok := curr.Annotations["ostype"]; ok && os != details.ServerInfo().OSType {
return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), os, details.ServerInfo().OSType)
if ost, ok := curr.Annotations["ostype"]; ok && ost != details.ServerInfo().OSType {
return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), ost, details.ServerInfo().OSType)
}
if _, ok := curr.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
Expand Down
6 changes: 3 additions & 3 deletions e2e/context/testdata/context-ls-notls.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
NAME DESCRIPTION DOCKER ENDPOINT
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
remote my remote cluster ssh://someserver
NAME DESCRIPTION DOCKER ENDPOINT ERROR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
remote my remote cluster ssh://someserver
6 changes: 3 additions & 3 deletions e2e/context/testdata/context-ls-tls.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
NAME DESCRIPTION DOCKER ENDPOINT
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
test unix:///var/run/docker.sock
NAME DESCRIPTION DOCKER ENDPOINT ERROR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
test unix:///var/run/docker.sock
6 changes: 3 additions & 3 deletions e2e/context/testdata/context-ls.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
NAME DESCRIPTION DOCKER ENDPOINT
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
remote my remote cluster ssh://someserver
NAME DESCRIPTION DOCKER ENDPOINT ERROR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock
remote my remote cluster ssh://someserver
5 changes: 5 additions & 0 deletions internal/test/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ func (c *FakeCli) Client() client.APIClient {
return c.client
}

// CurrentVersion returns the API version used by FakeCli.
func (c *FakeCli) CurrentVersion() string {
return c.DefaultVersion()
}

// Out returns the output stream (stdout) the cli should write on
func (c *FakeCli) Out() *streams.Out {
return c.out
Expand Down