[Context-Switch] Foundation of fast context switch #38148
Conversation
a302e27 to
491deb8
Compare
pkg/contextstore/store.go
Outdated
There was a problem hiding this comment.
I think this interface should support DOCKER_HOST=ssh:// credentials as well. (Implementation can be follow-up PR)
There was a problem hiding this comment.
Yes, but if I understand it correctly, the ssh support is purely cli-side. So I think we'll solve that in the cli.
And that is one thing I kind of dislike with the ssh support: it seems very difficult for 3rd party clients to leverage it
There was a problem hiding this comment.
@AkihiroSuda btw, do you think it is possible to modify the ssh implementation to bring it to moby/moby/client ? I did not dig sufficiently into that subject to have a clear vision about it, but that might both simplify things a lot for the context-store integration, and makes it available to 3rd party client tools.
There was a problem hiding this comment.
Technically yes, although I'm not sure we want to introduce docker system dial-stdio dependency on this repo
There was a problem hiding this comment.
It feels like the subject needs a bit more discussion, and on my side a bit more digging into the design of the ssh support.
What I plan to do is to open PRs on the CLI side without supporting connection helpers, and after that, we can discuss how to make all this fit together
There was a problem hiding this comment.
FYI the design of the SSH support has a long story.. docker/go-connections#39 (comment)
491deb8 to
1ec399f
Compare
Codecov Report
@@ Coverage Diff @@
## master #38148 +/- ##
=========================================
Coverage ? 36.3%
=========================================
Files ? 614
Lines ? 45539
Branches ? 0
=========================================
Hits ? 16531
Misses ? 26726
Partials ? 2282 |
1ec399f to
8c7e649
Compare
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
8c7e649 to
4b476b6
Compare
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
| // if contextNameOverride is set -> load this context from context store | ||
| // if DOCKER_CONTEXT env variable is set -> load this context from context store | ||
| // if DOCKER_HOST is set -> fallback to FromEnv | ||
| // if a current context is set in the store -> load this context |
There was a problem hiding this comment.
Do you have a TODO to cause the documentation to be updated? (Seems to not be in this repo)
There was a problem hiding this comment.
Nice catch. @thaJeztah where should we put this kind of documentation (e.g.: now the preferred way to initialize a client is to use WithContextStoreOrEnv)?
| // if DOCKER_HOST is set -> fallback to FromEnv | ||
| // if a current context is set in the store -> load this context | ||
| // fallback to "default" client | ||
| func WithContextStoreOrEnv(contextStoreDir, contextNameOverride string) func(*Client) error { |
There was a problem hiding this comment.
I appreciate there is precedent here in WithTLSClientConfig but it seems a bit odd to me to be requiring all callers to opt in to what really ought to be the default behaviour.
IOW -- perhaps the functionality here ought to be folded in to NewClientWithOpts (in place of the call to defaultHTTPClient).
More a question for the maintainers rather than the author I guess.
There was a problem hiding this comment.
Yeah... the thing is it also introduce the 2 new parameters. Do we really want every client code to specify those, even if at the end they do things like WithHost("/var/run/docker.sock") ?
There was a problem hiding this comment.
They both have sensible defaults, so could be separate independent WithContextStore and WithContextName, but without them the default could be to use the default/current context from the default store.
So (IMHO) NewClientWithOptions() with
E839
no WithFoo options given should DTRT.
There was a problem hiding this comment.
I am waiting for feedback on this. I know that at the time being, for exemple, NewClientWithOptions without any parameter does not look for env variables. I am not sure if we want to change that.
There was a problem hiding this comment.
Covered the contextstore now, some comments, none major.
However I think the on disk formats needs to be documented since right now it is just defined in code, which means it becomes harder to reason about evolutionary changes etc. An in tree doc or (better) a package level godoc would seem reasonable places to write that.
I think the same is probably true of the tar export format, unless you want to document that the format is opaque and the output of Export can only validly be passed to Import. Even then you need to potentially handle evolution of the internal schema, in which case having it be documented would be good.
Speaking of the tar export -- is using the tar format necessary, wouldn't a JSON dump be less code to maintain?
I understand why one might want to lock down the TLS material, but what is the use case for the flipside -- when/why would someone want to expose the other stuff? Just having everything under the stricter regimen would be simpler. Did you consider other kinds of sensitive information but non-TLS information (perhaps for use with a token based system)? If we don't have any current need for such a thing that's fine, but we should at least have some idea in our head how we will extend the format to cover such things. |
|
@ijc from internal discussion, we'd like to leverage the host OS secure storage facilities to store TLS data (e.g. the windows Certificate store or th Mac OS Keychain). We are not sure about the shape it will have, but as a first step, having Metadata storage separate from TLS material seemed a good start. |
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
There was a problem hiding this comment.
The UX related to this PR will mostly appear in PRs against docker CLI, but everything required to interact with the docker context-store will live in the moby/moby repo (mostly to avoid dependent projects to have to vendor Docker cli)
As first, I thought it would live in moby/moby as there was a server side of it. As there is not, I'm not sure why putting this into moby/moby instead of docker/cli.
The functional arguments on the client make it easy to implement the feature outside of the client package.
I also am not sure to see what would be wrong to vendor the docker/cli part of that feature instead of moby/moby… Why would credential-helpers be outside of moby/moby (on it's own repository), but not contextstore ?
| "net/http" | ||
| "os" | ||
|
|
||
| "github.com/docker/docker/client/context" |
There was a problem hiding this comment.
We may want to always alias the import (it's already pretty messy when reading some part of the moby code with the different package name context …)
| } | ||
|
|
||
| // LoadTLSData loads TLS materials for the context | ||
| func (c *Context) LoadTLSData(s contextstore.Store) (*TLSData, error) { |
There was a problem hiding this comment.
nit: I wonder why it needs to be attached to the Context struct… func LoadTLSDataFromStore(name string, s contextstore.Store)
| } | ||
|
|
||
| // LoadTLSConfig extracts a context docker endpoint TLS config | ||
| func (c *Context) LoadTLSConfig(s contextstore.Store) (*tls.Config, error) { |
There was a problem hiding this comment.
Same question/nit here, we really only use SkipTLSVerify in the func
|
|
||
| // Context is a typed wrapper around a context-store generic context describing | ||
| // a Docker Engine endpoint | ||
| type Context struct { |
There was a problem hiding this comment.
not-a-nit-but-maybe: Endpoint maybe would be better than Context ? 😝
I know the name is kinda a "long" shot but I'm not a fan of Context.
| ) | ||
|
|
||
| // SetDockerEndpoint sets the docker endpoint of a context | ||
| func SetDockerEndpoint(s contextstore.Store, name, host, apiVersion string, ca, cert, key []byte, skipTLSVerify bool) error { |
There was a problem hiding this comment.
And here I would attach SetDockerEndpoint to contextstore.Store (or sthg more like s.Save(context))
| // of the remote system. TLS data and metadata are stored separately, so that in the future, we will be able to store sensitive | ||
| // information in a more secure way, depending on the os we are running on (e.g.: on Windows we could use the user Certificate Store, on Mac OS the user Keychain...) | ||
| // | ||
| // current implementation is purely file based with the following structure: |
| // - tls/ | ||
| // - context1/endpoint1/: directory containing TLS data for the endpoint1 in context1 | ||
| // | ||
| // the context store itself has abolutely no knowledge about a docker or a kubernetes endpoint should contain in term of metadata or TLS config |
|
|
||
| func (s *metadataStore) remove(name string) error { | ||
| contextDir := s.contextDir(name) | ||
| return os.RemoveAll(contextDir) |
There was a problem hiding this comment.
nit: inline ? (os.RemoveAll(s.contextDir(name)))
| return !s.IsDir() | ||
| } | ||
|
|
||
| func listRecursivelyMetadataDirs(root string) ([]string, error) { |
There was a problem hiding this comment.
Why not using filepath.Walk ?
| } | ||
|
|
||
| // Export exports an existing namespace into a stream | ||
| func Export(name string, s Store) io.ReadCloser { |
There was a problem hiding this comment.
Wouldn't this be simplified by using pkg/archive ?
|
@vdemeester after some time thinking about it, I think you are right about putting all this in docker/cli. I primarily did this way because of the misleading WithFromEnv function that should IMO live in the cli repo as well. |
|
Closing this, as all the code has now been moved to docker/cli#1500. |
- What I did
This is the foundation of a feature we at Docker want to bring to the docker client ecosystem: A way to store credentials to connect to different clusters and quickly switch from one to the other. The UX related to this PR will mostly appear in PRs against docker CLI, but everything required to interact with the docker context-store will live in the moby/moby repo (mostly to avoid dependent projects to have to vendor Docker cli)
- How I did it
First, I introduced a pkg/contextstore package that does the actual context storage. A context is a named entity that contains arbitrary metadata, and references named endpoints (on the moby/moby side only one endpoint is actually created/consumed, but on the CLI side, we'll complement those contexts with endpoints targeting Kubernetes for our Compose on Kubernetes support). An endpoint also contains arbitrary metadata plus references to TLS materials. The contextstore actually stores metadata and TLS material in different directories to make it easy to put different access rights on them, or put sensitive data on an encrypted filesystem.
Then the client/context package provides facilities to create and parse docker contexts and endpoints in order to configure a client
Client package has been updated with a new WithContextStoreOrEnv that will interact with the context store to configure the client, or fallback to the "legacy" environment variables if there is no context data.
- How to verify it
The PR comes with unit tests. User testing will be available soon with the docker/cli PRs coming
- Description for the changelog
Introduce a context store to make it easy to store and use different sets of credentials to connect to different engines