[do not merge] Fast context switch: foundation by simonferquel · Pull Request #1500 · docker/cli

simonferquel · 2018-11-07T10:50:00Z

- What I did

This is foundational work for enabling support for fast context switch. ~~This is related to moby/moby#38148~~. -> actually, the code from moby/moby has been pulled out into this PR. Keeping the link for discussion history

What it does is that it modifies how the CLI behaves to configure clients for both Docker engine and Kubernetes. It also change the way the default value for orchestrator in stacks related command is defined, making it possible to set a different default value on each context

- How I did it

This leverages the moby side PR, and add similar facilities to create/parse endpoint information for Kubernetes

- How to verify it

Unit tests + upcoming commands PR related to context switch

- Description for the changelog

Fast context switch: you can now store, import and export credentials for different clusters and switch between them quickly using the command line

- A picture of a cute animal (not mandatory but encouraged)

silvin-lubecki · 2018-11-08T10:57:32Z

Please rebase 🦁

ijc · 2018-11-08T13:51:06Z

Vast majority of the commits here seem to be unrelated test case fixes? Maybe they should be split out?

codecov-io · 2018-11-09T12:15:17Z

Codecov Report

Merging #1500 into master will increase coverage by 0.12%.
The diff coverage is 58.02%.

@@ Coverage Diff @@## master #1500 +/- ##
==========================================
+ Coverage 55.26% 55.38% +0.12% 
==========================================
Files 289 296 +7 Lines 19385 19853 +468 ==========================================
+ Hits 10713 10996 +283 - Misses 7977 8106 +129 - Partials 695 751 +56

silvin-lubecki · 2018-11-09T14:58:45Z

+	if tlsConfig != nil {
+ httpClient := cli.HTTPClient()
+ if transport, ok := httpClient.Transport.(*http.Transport); ok {
+ transport.TLSClientConfig = tlsConfig


Even if I don't know how to refactor that, it feels really smelly... (the down-casting and the check of the type)

yes, I know :(. impossible without modifying moby/moby

In a follow-up maybe? 🤗

simonferquel · 2018-11-23T10:19:52Z

@vdemeester aside from documentation completion, could you have another look at this? I think I fixed most of your raised issues.
@thaJeztah PTAL as well :)

vdemeester

LGTM 🐸

thaJeztah

sorry for the long delay; just leaving some thoughts after having a look (haven't been able to catch up with the whole discussion, so perhaps some of these have been discussed 😅)

thaJeztah · 2018-11-26T13:18:21Z

 	case "swarm":
 return OrchestratorSwarm, nil
-	case "":
+	case "", "unset":


not a big fan of "unset". Is there a way we can avoid that, and only use ""?

I agree. I had to handle the "unset" value case, as I normalize the orchestrator string both on context creation (to validate the input) and when I load the context. When creating the context, an empty orchestrator value gets normalized into "unset", so loading it must handle the special value correctly.
I could change the orchestratorUnset const, but I am worried about changing an existing behavior.

I could change the orchestratorUnset const, but I am worried about changing an existing behavior.
Ahm. yes, I see it was already there. At least orchestratorUnset is not exported, but we should be careful indeed.
@vdemeester @silvin-lubecki suggestions? (we can do that in a follow up if we want to)

I'd like to be a bit conservative here, and keep the "unset" constant as is.

Not big fan either. I think it's safe to change the constant to orchestratorUnset = Orchestrator("") and remove unset. No-one should rely on this constant.

OK; let's not block this PR for it, but do a follow-up @simonferquel

thaJeztah · 2018-11-26T14:02:45Z

+// ContextMetadata contains metadata about a context and its endpoints
+type ContextMetadata struct {
+	Metadata Metadata `json:"metadata,omitempty"`
+	Endpoints map[string]Metadata `json:"endpoints,omitempty"`


I was wondering why we didn't make this a map[string]EndpointMeta but I see there's two different versions of that (kube/docker), which is a pity (would've made things a bit more simple I guess 😞
typeEndpointMetastruct { context.EndpointMetaBaseAPIVersionstring } typeEndpointMetastruct { context.EndpointMetaBaseDefaultNamespacestring }
Wondering if we made this;
Endpointsmap[string]interface{}
Then we could do the conversion when storing, and store either KubeEndpoint or DockerEndpoint here 🤔

:+1 It would greatly simplify Endpoint Marshalling/Unmarshalling

Hmm, actually no, it wouldn't. But it would make it much cleaner (ContextMetadata would need to implement the json Marshal interface)

thaJeztah · 2018-11-26T14:11:18Z

 	// opts.ValidateHost is not used here, so as to allow connection helpers
 	hostOpt := opts.NewNamedListOptsRef("hosts", &commonOpts.Hosts, nil)
 	flags.VarP(hostOpt, "host", "H", "Daemon socket(s) to connect to")
+	flags.StringVarP(&commonOpts.Context, "context", "c", dockerContext,


Looks like we have to update the docs for this, or is that done in the other PR?

Should do in this one

Actually, makes more sense to put all the doc in the following PR so that documentation for commands impacting the default context etc. are documented in sync.
I think once I get 2 LGTM on this one, I will squash, rebase #1501 and close this one so that we merge the full context switch feature at once. Was very usefull for review to split though.

simonferquel · 2018-11-27T12:19:08Z

@thaJeztah PTAL at the last commit I added. It brings a way not to have to do manual marshaling at the cost of some complexity on the context store side. Not sure if I like it.

thaJeztah

Thanks for those changes, I think I like this better, but will have to give a more thorough look, and could use some eyes from @vdemeester 🤗

I left some small nits inline below.

thaJeztah · 2018-11-27T14:44:38Z

 // GetStackOrchestrator checks DOCKER_STACK_ORCHESTRATOR environment variable and configuration file
 // orchestrator value and returns user defined Orchestrator.
-func GetStackOrchestrator(flagValue, value string, stderr io.Writer) (Orchestrator, error) {
+func GetStackOrchestrator(flagValue, contextValue, globalDefault string, stderr io.Writer) (Orchestrator, error) {


Is this used externally? I noticed it's only used in the same package so could be un-exported

it was exported before. I changed the signature to put the contextValue in the middle. I suspect 3rd party code (like docker-app) leverages this function.

Ah, yes, possible

I confirm, docker-app uses it :)

thaJeztah · 2018-11-27T14:49:26Z

+func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile) (string, error) {
+	// spcifying both --host and --context is considered ambiguous
+	if opts.Context != "" && len(opts.Hosts) > 0 {
+ return "", errors.New("both --host and --context can't be set at the same time")


Looking for similar errors, perhaps (feedback welcome 😄);
Suggested change
return"", errors.New("both --host and --context can't be set at the same time")
return"", errors.New("Conflicting options: either specify --host or --context, not both")

in golang, errors should not begin with a capital letter :) but otherwise I like the wording

Yes, I was in doubt; I see that in the CLI we're not consistent, likely because this message is directly printed in the terminal (so won't be in a log with an Error: prefix (e.g.))

thaJeztah · 2018-11-27T14:50:18Z

+// - if Config file has a globally set "CurrentContext", use this value
+// - fallbacks to default HOST, uses TLS config from flags/env vars
+func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigFile) (string, error) {
+	// spcifying both --host and --context is considered ambiguous


s/spcifying/specifying, but I think the comment can be removed, because the code below (including the error-message) already describes what it does 😅

vdemeester

A squash will be required before merging 👼

silvin-lubecki · 2018-12-17T15:48:18Z

 	}
-	clientConfig := kubernetes.NewKubernetesConfig(kubeConfig)
-	config, err := clientConfig.ClientConfig()
+	var (


This code can be factorized with https://github.com/docker/cli/pull/1500/files#diff-8d7a16ab24f6e96e9673178204f32156R60

In a followup maybe ? the tricky part here is to avoid cyclic dependencies. We could always introduce a new package...

silvin-lubecki · 2018-12-17T15:57:33Z

 // the KUBECONFIG environment variable and command line flags.
-func NewKubernetesConfig(configPath string) clientcmd.ClientConfig {
+func NewKubernetesConfig(configPath string) (clientcmd.ClientConfig, error) {
+


nit: empty line

simonferquel · 2018-12-17T16:13:39Z

@silvin-lubecki @vdemeester @thaJeztah, I fixed all your feedback, and squashed everything. PTAL

This PR adds a store to the CLI, that can be leveraged to persist and retrieve credentials for various API endpoints, as well as context-specific settings (initially, default stack orchestrator, but we could expand that). This comes with the logic to persist and retrieve endpoints configs for both Docker and Kubernetes APIs. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

vdemeester

LGTM 🚒

thaJeztah

LGTM

left some nits/notes for follow-ups
thanks for this one!!

thaJeztah · 2018-12-19T09:46:40Z

 	case "swarm":
 return OrchestratorSwarm, nil
-	case "":
+	case "", "unset":


OK; let's not block this PR for it, but do a follow-up @simonferquel

thaJeztah · 2018-12-19T09:48:41Z

+	} else {
+ clientConfig, err = kubecontext.ConfigFromContext(dockerCli.CurrentContext(), dockerCli.ContextStore())
+	}
+	if err != nil {


Nit (no need to fix); should probably have been inside the else above

thaJeztah · 2018-12-19T10:05:04Z

+	defer kcFile.Close()
+	cfg := clientcmdapi.NewConfig()
+	cfg.AuthInfos["user"] = clientcmdapi.NewAuthInfo()
+	cfg.Contexts["context1"] = clientcmdapi.NewContext()


It's a pity that their constructors don't allow passing options for the new things 😞 they're currently not that useful

thaJeztah · 2018-12-19T10:07:04Z

+// - config.json: contains the current "default" context
+// - meta/
+// - context1/meta.json: contains context medata (key/value pairs) as well as a list of endpoints (themselves containing key/value pair metadata)
+// - contexts/can/also/be/folded/like/this/meta.json: same as context1, but for a context named `contexts/can/also/be/folded/like/this`


Inheritance would be good indeed (being able to set global options and override them per context); guess we can do so in a follow up?

thaJeztah · 2018-12-19T10:08:27Z

 	// opts.ValidateHost is not used here, so as to allow connection helpers
 	hostOpt := opts.NewNamedListOptsRef("hosts", &commonOpts.Hosts, nil)
 	flags.VarP(hostOpt, "host", "H", "Daemon socket(s) to connect to")
+	flags.StringVarP(&commonOpts.Context, "context", "c", "",


Needs a follow-up to update the completion scripts (and the documentation / man pages)
/cc @albers

I plan to add documentation as part of #1501. I might need help for completion scripts though.

andrewhsu · 2018-12-19T10:26:19Z

EDIT: i put comment in corresponding PR

@simonferquel when you get the chance, can you update the description with any user interaction changes? just a simple example of what is the new command or option or output from docker version or docker info or whatnot would help describe to the end user what this PR will bring it because there's a lot of code changes in this one.

thaJeztah · 2018-12-19T14:16:53Z

Discussing this PR with @simonferquel @silvin-lubecki @andrewhsu @tiborvass, and we think it might be good to merge this together with #1501, so we either want to create a feature branch (to collect the other changes that makes the feature complete), or just merge them to master together.

(That, to take into account possible issues that follow review / testing of #1501)

andrewhsu · 2018-12-19T14:51:35Z

talked with @simonferquel @silvin-lubecki @thaJeztah @tiborvass and will close this PR in favor of #1501 but keep the commit bd396cd in #1501 because this one is already LGTM'ed in order to finish up the cli ui work that builds on top of this.

simonferquel requested review from silvin-lubecki and vdemeester as code owners November 7, 2018 10:50

GordonTheTurtle added the status/0-triage label Nov 7, 2018

simonferquel mentioned this pull request Nov 7, 2018
Fast context switch: commands #1501
Merged

simonferquel force-pushed the use-context-foundation branch 5 times, most recently from 8d52d2e to b264b22Compare November 7, 2018 21:30

silvin-lubecki reviewed Nov 8, 2018
View reviewed changes

Comment threadcli/command/cli.go Outdated

ijc reviewed Nov 8, 2018
View reviewed changes

simonferquel force-pushed the use-context-foundation branch from b264b22 to 54a3f70Compare November 9, 2018 12:15

simonferquel force-pushed the use-context-foundation branch 2 times, most recently from a9d1a3e to ccb87a7Compare November 9, 2018 12:24