Skip to content

Makes the default docker context behavior consistent#1719

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

Merged
silvin-lubecki merged 1 commit into from
Mar 18, 2019
Merged

Makes the default docker context behavior consistent #1719

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
91 changes: 44 additions & 47 deletions cli/command/cli.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,6 +3,7 @@ package command
import (
"context"
"io"
"io/ioutil"
"os"
"path/filepath"
"runtime"
Expand DownExpand Up@@ -209,12 +210,18 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize

cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)

cli.contextStore = store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
baseContextSore := store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
cli.contextStore = &ContextStoreWithDefault{
Store: baseContextSore,
Resolver: func() (*DefaultContext, error) {
return resolveDefaultContext(opts.Common, cli.ConfigFile(), cli.Err())
},
}
cli.currentContext, err = resolveContextName(opts.Common, cli.configFile, cli.contextStore)
if err != nil {
return err
}
cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, cli.currentContext, opts.Common)
cli.dockerEndpoint, err = resolveDockerEndpoint(cli.contextStore, cli.currentContext)
if err != nil {
return errors.Wrap(err, "unable to resolve docker endpoint")
}
Expand DownExpand Up@@ -252,12 +259,17 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize

// NewAPIClientFromFlags creates a new APIClient from command line flags
func NewAPIClientFromFlags(opts *cliflags.CommonOptions, configFile *configfile.ConfigFile) (client.APIClient, error) {
store := store.New(cliconfig.ContextStoreDir(), defaultContextStoreConfig())
store := &ContextStoreWithDefault{
Store: store.New(cliconfig.ContextStoreDir(), defaultContextStoreConfig()),
Resolver: func() (*DefaultContext, error) {
return resolveDefaultContext(opts, configFile, ioutil.Discard)
},
}
contextName, err := resolveContextName(opts, configFile, store)
if err != nil {
return nil, err
}
endpoint, err := resolveDockerEndpoint(store, contextName, opts)
endpoint, err := resolveDockerEndpoint(store, contextName)
if err != nil {
return nil, errors.Wrap(err, "unable to resolve docker endpoint")
}
Expand All@@ -278,18 +290,20 @@ func newAPIClientFromEndpoint(ep docker.Endpoint, configFile *configfile.ConfigF
return client.NewClientWithOpts(clientOpts...)
}

func resolveDockerEndpoint(s store.Store, contextName string, opts *cliflags.CommonOptions) (docker.Endpoint, error) {
if contextName != "" {
ctxMeta, err := s.GetContextMetadata(contextName)
if err != nil {
return docker.Endpoint{}, err
}
epMeta, err := docker.EndpointFromContext(ctxMeta)
if err != nil {
return docker.Endpoint{}, err
}
return docker.WithTLSData(s, contextName, epMeta)
func resolveDockerEndpoint(s store.Store, contextName string) (docker.Endpoint, error) {
ctxMeta, err := s.GetContextMetadata(contextName)
if err != nil {
return docker.Endpoint{}, err
}
epMeta, err := docker.EndpointFromContext(ctxMeta)
if err != nil {
return docker.Endpoint{}, err
}
return docker.WithTLSData(s, contextName, epMeta)
}

// Resolve the Docker endpoint for the default context (based on config, env vars and CLI flags)
func resolveDefaultDockerEndpoint(opts *cliflags.CommonOptions) (docker.Endpoint, error) {
host, err := getServerHost(opts.Hosts, opts.TLSOptions)
if err != nil {
return docker.Endpoint{}, err
Expand DownExpand Up@@ -384,38 +398,21 @@ func (cli *DockerCli) CurrentContext() string {

// StackOrchestrator resolves which stack orchestrator is in use
func (cli *DockerCli) StackOrchestrator(flagValue string) (Orchestrator, error) {
var ctxOrchestrator string

configFile := cli.configFile
if configFile == nil {
configFile = cliconfig.LoadDefaultConfigFile(cli.Err())
}

currentContext := cli.CurrentContext()
if currentContext == "" {
currentContext = configFile.CurrentContext
ctxRaw, err := cli.ContextStore().GetContextMetadata(currentContext)
if store.IsErrContextDoesNotExist(err) {
// case where the currentContext has been removed (CLI behavior is to fallback to using DOCKER_HOST based resolution)

@thaJeztahthaJeztahMar 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not for this PR, but something similar came up in another discussion with @simonferquel - I think this should be an error; we should not silently fall back to DOCKER_HOST (because then I may be running commands against the wrong environment, which is likely not what you want)

return GetStackOrchestrator(flagValue, "", cli.ConfigFile().StackOrchestrator, cli.Err())
}
if currentContext != "" {
contextstore := cli.contextStore
if contextstore == nil {
contextstore = store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
}
ctxRaw, err := contextstore.GetContextMetadata(currentContext)
if store.IsErrContextDoesNotExist(err) {
// case where the currentContext has been removed (CLI behavior is to fallback to using DOCKER_HOST based resolution)
return GetStackOrchestrator(flagValue, "", configFile.StackOrchestrator, cli.Err())
}
if err != nil {
return "", err
}
ctxMeta, err := GetDockerContext(ctxRaw)
if err != nil {
return "", err
}
ctxOrchestrator = string(ctxMeta.StackOrchestrator)
if err != nil {
return "", err
}

return GetStackOrchestrator(flagValue, ctxOrchestrator, configFile.StackOrchestrator, cli.Err())
ctxMeta, err := GetDockerContext(ctxRaw)
if err != nil {
return "", err
}
ctxOrchestrator := string(ctxMeta.StackOrchestrator)
return GetStackOrchestrator(flagValue, ctxOrchestrator, cli.ConfigFile().StackOrchestrator, cli.Err())
}

// DockerEndpoint returns the current docker endpoint
Expand DownExpand Up@@ -511,10 +508,10 @@ func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigF
return opts.Context, nil
}
if len(opts.Hosts) > 0 {
return "", nil
return DefaultContextName, nil
}
if _, present := os.LookupEnv("DOCKER_HOST"); present {
return "", nil
return DefaultContextName, nil
}
if ctxName, ok := os.LookupEnv("DOCKER_CONTEXT"); ok {
return ctxName, nil
Expand All@@ -526,7 +523,7 @@ func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigF
}
return config.CurrentContext, err
}
return "", nil
return DefaultContextName, nil
}

func defaultContextStoreConfig() store.Config {
Expand Down
27 changes: 26 additions & 1 deletion cli/command/context/create_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,7 +23,26 @@ func makeFakeCli(t *testing.T, opts ...func(*test.FakeCli)) (*test.FakeCli, func
store.EndpointTypeGetter(docker.DockerEndpoint, func() interface{} { return &docker.EndpointMeta{} }),
store.EndpointTypeGetter(kubernetes.KubernetesEndpoint, func() interface{} { return &kubernetes.EndpointMeta{} }),
)
store := store.New(dir, storeConfig)
store := &command.ContextStoreWithDefault{
Store: store.New(dir, storeConfig),
Resolver: func() (*command.DefaultContext, error) {
return &command.DefaultContext{
Meta: store.ContextMetadata{
Endpoints: map[string]interface{}{
docker.DockerEndpoint: docker.EndpointMeta{
Host: "unix:///var/run/docker.sock",
},
},
Metadata: command.DockerContext{
Description: "",
StackOrchestrator: command.OrchestratorSwarm,
},
Name: command.DefaultContextName,
},
TLS: store.ContextTLSData{},
}, nil
},
}
cleanup := func() {
os.RemoveAll(dir)
}
Expand DownExpand Up@@ -52,6 +71,12 @@ func TestCreateInvalids(t *testing.T) {
{
expecterErr: `context name cannot be empty`,
},
{
options: CreateOptions{
Name: "default",
},
expecterErr: `"default" is a reserved context name`,
},
{
options: CreateOptions{
Name: " ",
Expand Down
2 changes: 1 addition & 1 deletion cli/command/context/export.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -77,7 +77,7 @@ func writeTo(dockerCli command.Cli, reader io.Reader, dest string) error {

// RunExport exports a Docker context
func RunExport(dockerCli command.Cli, opts *ExportOptions) error {
if err := validateContextName(opts.ContextName); err != nil {
if err := validateContextName(opts.ContextName); err != nil && opts.ContextName != command.DefaultContextName {
return err
}
ctxMeta, err := dockerCli.ContextStore().GetContextMetadata(opts.ContextName)
Expand Down
3 changes: 0 additions & 3 deletions cli/command/context/inspect.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,9 +40,6 @@ func newInspectCommand(dockerCli command.Cli) *cobra.Command {

func runInspect(dockerCli command.Cli, opts inspectOptions) error {
getRefFunc := func(ref string) (interface{}, []byte, error) {
if ref == "default" {
return nil, nil, errors.New(`context "default" cannot be inspected`)
}
c, err := dockerCli.ContextStore().GetContextMetadata(ref)
if err != nil {
return nil, nil, err
Expand Down
27 changes: 3 additions & 24 deletions cli/command/context/list.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,6 @@ import (
"github.com/docker/cli/cli/command/formatter"
"github.com/docker/cli/cli/context/docker"
kubecontext "github.com/docker/cli/cli/context/kubernetes"
"github.com/docker/cli/kubernetes"
"github.com/spf13/cobra"
"vbom.ml/util/sortorder"
)
Expand DownExpand Up@@ -61,6 +60,9 @@ func runList(dockerCli command.Cli, opts *listOptions) error {
if kubernetesEndpoint != nil {
kubEndpointText = fmt.Sprintf("%s (%s)", kubernetesEndpoint.Host, kubernetesEndpoint.DefaultNamespace)
}
if rawMeta.Name == command.DefaultContextName {

@thaJeztahthaJeztahMar 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If dockerCli.ContextStore().ListContexts() is handled by ContextStoreWithDefault.ListContexts (), is this special case still needed, or would it already return the right information?

@jcsirotjcsirotMar 12, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@thaJeztah I didn't want to add specific code for import/export command to avoid, when importing an exported default context, having a second context with the "Current DOCKER_HOST based configuration" description. Therefore I decided to leave the description empty for the default context and add handle that case here.

@thaJeztahthaJeztahMar 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still a bit on the fence on this one (i.e., the same applies to other contexts, which also by default copy the description), but not a blocker

@jcsirotjcsirotMar 18, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed. I think it is less confusing for other contexts. Maybe it would be nice to add a --description flag to import command in order to override the imported context description

meta.Description = "Current DOCKER_HOST based configuration"
}
desc := formatter.ClientContext{
Name: rawMeta.Name,
Current: rawMeta.Name == curContext,
Expand All@@ -71,29 +73,6 @@ func runList(dockerCli command.Cli, opts *listOptions) error {
}
contexts = append(contexts, &desc)
}
if !opts.quiet {
desc := &formatter.ClientContext{
Name: "default",
Description: "Current DOCKER_HOST based configuration",
}
if dockerCli.CurrentContext() == "" {
orchestrator, _ := dockerCli.StackOrchestrator("")
kubEndpointText := ""
kubeconfig := kubernetes.NewKubernetesConfig("")
if cfg, err := kubeconfig.ClientConfig(); err == nil {
ns, _, _ := kubeconfig.Namespace()
if ns == "" {
ns = "default"
}
kubEndpointText = fmt.Sprintf("%s (%s)", cfg.Host, ns)
}
desc.Current = true
desc.StackOrchestrator = string(orchestrator)
desc.DockerEndpoint = dockerCli.DockerEndpoint().Host
desc.KubernetesEndpoint = kubEndpointText
}
contexts = append(contexts, desc)
}
sort.Slice(contexts, func(i, j int) bool {
return sortorder.NaturalLess(contexts[i].Name, contexts[j].Name)
})
Expand Down
15 changes: 0 additions & 15 deletions cli/command/context/list_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,7 +4,6 @@ import (
"testing"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/context/docker"
"gotest.tools/assert"
"gotest.tools/env"
"gotest.tools/golden"
Expand DownExpand Up@@ -36,20 +35,6 @@ func TestList(t *testing.T) {
golden.Assert(t, cli.OutBuffer().String(), "list.golden")
}

func TestListNoContext(t *testing.T) {
Comment thread
silvin-lubecki marked this conversation as resolved.
cli, cleanup := makeFakeCli(t)
defer cleanup()
defer env.Patch(t, "KUBECONFIG", "./testdata/test-kubeconfig")()
cli.SetDockerEndpoint(docker.Endpoint{
EndpointMeta: docker.EndpointMeta{
Host: "https://someswarmserver",
},
})
cli.OutBuffer().Reset()
assert.NilError(t, runList(cli, &listOptions{}))
golden.Assert(t, cli.OutBuffer().String(), "list.no-context.golden")
}

func TestListQuiet(t *testing.T) {
cli, cleanup := makeFakeCli(t)
defer cleanup()
Expand Down
9 changes: 9 additions & 0 deletions cli/command/context/remove_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -62,3 +62,12 @@ func TestRemoveCurrentForce(t *testing.T) {
assert.NilError(t, err)
assert.Equal(t, "", reloadedConfig.CurrentContext)
}

func TestRemoveDefault(t *testing.T) {
cli, cleanup := makeFakeCli(t)
defer cleanup()
createTestContextWithKubeAndSwarm(t, cli, "other", "all")
cli.SetCurrentContext("current")
err := RunRemove(cli, RemoveOptions{}, []string{"default"})
assert.ErrorContains(t, err, `default: context "default" cannot be removed`)
}
10 changes: 5 additions & 5 deletions cli/command/context/testdata/list.golden
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
current * description of current https://someswarmserver https://someserver (default) all
default Current DOCKER_HOST based configuration
other description of other https://someswarmserver https://someserver (default) all
unset description of unset https://someswarmserver https://someserver (default)
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
current * description of current https://someswarmserver https://someserver (default) all
default Current DOCKER_HOST based configuration unix:///var/run/docker.sockswarm
other description of other https://someswarmserver https://someserver (default) all
unset description of unset https://someswarmserver https://someserver (default)
2 changes: 0 additions & 2 deletions cli/command/context/testdata/list.no-context.golden
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions cli/command/context/testdata/quiet-list.golden
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
current
default
other
Loading