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.

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

docker context use: modify current kubeconfig #1620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonferquel
Copy link
Contributor

- What I did

For contexts with a Kubernetes endpoint, docker context use now modifies the current kubeconfig file, to make sure kubectl and docker CLIs target the same cluster. This can be skipped with --skip-kubeconfig.

- How I did it

Using the k8s clientcmd package, I parse and modify the ambient kubeconfig file with the configuration stored in the context store

- How to verify it

This is covered by unit tests

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

For contexts with a Kubernetes endpoint, `docker context use` now
modifies the current kubeconfig file, to make sure `kubectl` and
`docker` CLIs target the same cluster.
This can be skipped with `--skip-kubeconfig`.

Signed-off-by: Simon Ferquel <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #1620 into master will increase coverage by 0.03%.
The diff coverage is 75.8%.

@@            Coverage Diff             @@
##           master    #1620      +/-   ##
==========================================
+ Coverage   55.16%   55.19%   +0.03%     
==========================================
  Files         301      301              
  Lines       20384    20426      +42     
==========================================
+ Hits        11244    11275      +31     
- Misses       8335     8342       +7     
- Partials      805      809       +4

@simonferquel
Copy link
Contributor Author

@silvin-lubecki @thaJeztah PTAL
@garethr can you validate the UX?

@thaJeztah
Copy link
Member

I'm a bit on the fence for the --skip-kubeconfig flag; if a context defines both, shouldn't we just switch both? (otherwise I'm in context-x-and-a-bit, and docker context ls shows that I'm using context X, but actually I'm not using it fully)

@thaJeztah
Copy link
Member

thaJeztah commented Jan 19, 2019

If I want to switch just the context for kubernetes, I can still use the KUBECONFIG env-var to override (correct)?

If so, we should think of a way to indicate that docker, kube, or both are overridden from an environment variable 🤔

@simonferquel
Copy link
Contributor Author

I think there is a misunderstanding about what skip-kubeconfig does: it still applies the context for both all endpoints. What has changed with this PR is that if the context contains a kubernetes endpoint, docker context use also modified the current kubeconfig file so that kubectl and other tools using kubectl config file (helm, skaffold,...) connect to the same cluster as the docker CLI. When the flag is set, only the docker CLI is impacted by docker context use.

@simonferquel
Copy link
Contributor Author

For now, KUBECONFIG env var is not taken into account by the docker CLI when a context with a Kubernetes endpoint is set. Should I change that ?
@garethr WDYT?

@garethr
Copy link

garethr commented Jan 21, 2019

I'm not sure about mutating an existing Kubernetes config file. Could you provide more examples of how your envisage that working? It feels a little action at a distance, and I think we need a longer term view of interactions between the Docker CLI and the Kubernetes API to help decide the best course of action. The primary usecase for the Kubernetes part of context at present is passing the Kubernetes context to docker app.

@simonferquel
Copy link
Contributor Author

@garethr an example here:
I want to work with Compose on Kubernetes, and want to reference an external secret:

  • I switch to the context I want to use with docker context use dev-kube
  • I want to create the secret with kubectl create secret // here if docker context use doesn't modify my kubeconfig I also need to switch my kubectl context.
  • I do a docker stack deploy with a composefile referencing my secret.

It also applies to a workflow where I want to use Compose on Kubernetes + Skaffold, or Helm or any tool using kubeconfig files.

@simonferquel
Copy link
Contributor Author

If we need to further design work, I suggest we close this PR and maybe re-open it when we figure out what the experience should be.

@thaJeztah
Copy link
Member

thaJeztah commented Jan 21, 2019

Perhaps docker context use --scope=<docker|kubernetes> mycontext, but we'd need to have separate indicators for which one is active for which 😞

@simonferquel
Copy link
Contributor Author

@thaJeztah for the docker cli itself, the flag does not change anything (it still applies both docker and kubernetes endpoints configuration). It just impact external tools like kubectl.

The Default behavior before this PR is:

  • kubectl config use-context cluster-a
  • docker context use cluster-b
  • -> at this point kubectl points to cluster-a, and docker cli points to cluster-b (for both docker and kubernetes endpoints)

Behavior after this PR:

  • kubectl config use-context cluster-a
  • docker context use cluster-b: docker-cli ensure cluster-b context exists in kubectl config file with an equivalent config as what is stored in docker context store, and set it as the current context for kubectl (also applies to most client tools in the kubernetes ecosystem)
  • -> both docker CLI and kubectl point to cluster-b
  • --skip-kubeconfig reverts to the previous behavior, such that kubectl config is not modified

@simonferquel
Copy link
Contributor Author

Again, I am completely ok with not merging this if you feel it is confusing to have the docker CLI modify the kubectl config :). (it was just something that came up a very long time ago on a fast context switch design discussion)

@thaJeztah
Copy link
Member

for the docker cli itself, the flag does not change anything (it still applies both docker and kubernetes endpoints configuration)

Ah, yes, sorry, brainfart; you explained that above

if you feel it is confusing to have the docker CLI modify the kubectl config :). (it was just something that came up a very long time ago on a fast context switch design discussion)

Yes, definitely something to dig further into;

  • pro: docker context use can be your one-stop location to switch between environments
  • pro: prevent nasty situations where you (e.g.) deploy a stack, but forgot to switch your kubernetes context, then, using the native kube cli, you're interacting with the wrong cluster
  • con: we don't "own" the native kube cli; so should we mess with it?
  • pro/con: since the docker cli is not (yet, and likely never fully will be) a replacement for managing everything that kubernetes provides, so users will have to use a combination of both clients to engage with a kubernetes cluster. So; what's least confusing? Treat them both as part of the same "context" ("if I docker context use, I expect to be in the context/environment I defined", or "if I docker context use, I expect only the subset of kubernetes features that the docker cli uses to be switching to that environment"?)

@thaJeztah
Copy link
Member

@chris-crone @vdemeester @silvin-lubecki should we continue the discussion on this one?

@stephen-turner
Copy link

The way I imagined it would work (without having thought about it very hard) is that the docker context config wouldn't contain any kube credentials of its own, but only a pointer to a kube current-context. Then docker context switch would automatically switch the kube context too by calling kube config use-context. That avoids cloning the kube credentials, so that it's obvious what is the single source of truth for them. But maybe it doesn't cover some use case I haven't thought about.

@simonferquel
Copy link
Contributor Author

You can always remove entries in your kubeconfig file. So consistency could not be ensured

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants