Add kubernetes support to docker/cli #721

silvin-lubecki · 2017-11-30T15:32:42Z

This PR introduces docker stack support for kubernetes in the docker cli using the docker compose controller embedded in docker4mac beta.

Adds a new orchestrator config field, a new --orchestrator and a DOCKER_ORCHESTRATOR environment variable to switch between swarm and kubernetes orchestrators.
```
{
  // […]
  "credsStore": "osxkeychain",
  "orchestrator": "kubernetes"
}
```
When in kubernetes mode:
- adds a --namespace on stack subcommands to be able to point to the correct kubernetes namespace
- adds a --kubeconfig flag to stack subcommands — and take KUBECONFIG environment variable into account — to be able to point to a kubernetes cluster using the kubernetes configuration file.
- --filter on docker stack services are disabled.
- orchestrator commands like secret, service, config, node will error out because they are not implemented yet (it's on the way, but won't be present in this PR)

This adds new kubernetes and swarm annotation to define if a command/flag is supported by one of the other (or both).

All of this is marked experimental (thus we depend on #758 for this one to get merged)

🦁

Signed-off-by: Vincent Demeester vincent@sbr.pm
Signed-off-by: Silvin Lubecki silvin.lubecki@docker.com

codecov-io · 2017-12-01T10:19:30Z

Codecov Report

Merging #721 into master will decrease coverage by 2.56%.
The diff coverage is 13.04%.

@@            Coverage Diff            @@
##           master    #721      +/-   ##
=========================================
- Coverage   53.46%   50.9%   -2.57%     
=========================================
  Files         218     237      +19     
  Lines       14642   15338     +696     
=========================================
- Hits         7829    7808      -21     
- Misses       6327    7028     +701     
- Partials      486     502      +16

dnephin · 2017-12-01T16:34:07Z

cli/command/orchestrator/orchestrator.go

+	unset = Orchestrator("unset")
+
+	defaultOrchestrator = Swarm
+	dockerOrchestrator  = "DOCKER_ORCHESTRATOR"


s/dockerOrchestrator/envVarDockerOrchestrator so it's obvious this is an environment variable key and not something else.

dnephin · 2017-12-01T16:48:08Z

cli/command/orchestrator/orchestrator.go

+		return o
+	}
+	// Check config file
+	if configFile := cliconfig.LoadDefaultConfigFile(dockerCli.Err()); configFile != nil {


This is going to break anyone that uses the --config flag because GetOrchestrator() is called from New...Comand which happens before flag parsing.

dnephin · 2017-12-01T16:51:14Z

cli/command/stack/kubernetes/api/client/clientset_generated/clientset/clientset.go

@@ -0,0 +1,88 @@
+package clientset


This path is kind of overly long.

How about moving cli/command/stack/kubernetes to /kubernetes ? It's not really part of the command router, so it doesn't belong under cli/command.

(more on this here #721 (comment))

dnephin · 2017-12-01T16:53:15Z

cli/command/stack/kubernetes/api/client/clientset_generated/clientset/doc.go

@@ -0,0 +1,4 @@
+// This package is generated by client-gen with custom arguments.


What is client-gen ? It's strange that it only puts this comment in doc.go and not in every file.

Can we include the steps to re-generate these files in a go:generate comment, or in the Makefile?

If possible could we have this tool write out the "this file is generated" comment to every file it generates, not just doc. There is actually a GO convention for this which it should use:

golang/go#13560 (comment)

^// Code generated .* DO NOT EDIT\.$

dnephin · 2017-12-01T17:03:04Z

e2e/orchestrator/version_test.go

+	assert.Contains(t, result.Stdout(), "Orchestrator: kubernetes")
+}
+
+func TestVersionWithOverridenConfigOrchestrator(t *testing.T) {


https://github.com/docker/cli/blob/master/TESTING.md#guidelines

e2e tests are for commands. orchestrator is not a command. Please move one of these to e2e/system/version_test.go.

The rest should not be e2e tests. They test client-only logic, so they should be unit tests.

dnephin · 2017-12-01T17:04:20Z

vendor.conf

+github.com/go-openapi/jsonpointer 46af16f9f7b149af66e5d1bd010e3574dc06de98
+github.com/go-openapi/jsonreference 13c6e3589ad90f49bd3e3bbe2c2cb3d7a4142272
+github.com/go-openapi/spec 6aced65f8501fe1217321abf0749d354824ba2ff
+github.com/go-openapi/swag 1d0bd113de87027671077d3c71eb3ac5d7dbba72


I'm going to look into some of these dependencies, this still seems like a crazy number of dependencies for a client.

I guess the k8s machinery dependencies are just bloated, so there isn't much we can do about this for now...

dnephin · 2017-12-01T17:04:41Z

cli/command/orchestrator/orchestrator.go

@@ -0,0 +1,53 @@
+package orchestrator


Could this be in cli/command package, or are there issues with circular imports?

dnephin · 2017-12-01T17:06:54Z

cli/command/stack/cmd.go

+		kubernetes.AddStackCommands(cmd, dockerCli)
+	case orchestrator.Swarm:
+		swarm.AddStackCommands(cmd, dockerCli)
+	}


I don't see how this can work if we want to be able to set the orchestrator from the config or a flag, and I think we should support both. This happens before flag parsing and config loading (which depends on flag parsing).

I think this branching needs to be done as part of the Run() of the commands.

The same commands exits for both orchestrators, so the implementation of the Run() can be switch and call the Run() for the correct orchestrator.

dnephin · 2017-12-01T17:27:45Z

For directory structure, how about this:

Move non-generated code out of the generated code tree:

git mv cli/command/stack/kubernetes/api/labels cli/command/stack/kubernetes/labels

Move generated code out of the cli/command tree

mkdir kubernetes
git mv cli/command/stack/kubernetes/api kubernetes/api

Add a README to the kubernetes/api package that explains:

this is a client library for x
this code is generated from a spec that is not available

AkihiroSuda · 2017-12-01T17:56:19Z

cli/command/stack/cmd.go

+	case orchestrator.Kubernetes:
+		cmd = kubernetes.NewTopLevelDeployCommand(dockerCli)
+	case orchestrator.Swarm:
+		cmd = swarm.NewTopLevelDeployCommand(dockerCli)


nil panic on default?

(I meant it should not panic)

AkihiroSuda · 2017-12-01T17:58:02Z

cli/command/stack/cmd.go

+		cmd = kubernetes.NewTopLevelDeployCommand(dockerCli)
+	case orchestrator.Swarm:
+		cmd = swarm.NewTopLevelDeployCommand(dockerCli)
+	}
 	// Remove the aliases at the top level
 	cmd.Aliases = []string{}
 	cmd.Annotations = map[string]string{"experimental": "", "version": "1.25"}


"version_kubernetes": "v1beta1"?

@AkihiroSuda depends on what you wanna check here : the version of kubernetes, the version of the "k8s" native api or the version of our api extension — or all of those 😝.

I think the version of the stack API extension is correct to check here. (And the stack API extension may provide info for minimum supported k8s version and so on)

AkihiroSuda · 2017-12-01T17:58:48Z

cli/command/stack/common/opts.go

 	flags.StringVarP(opt, "compose-file", "c", "", "Path to a Compose file")
 	flags.SetAnnotation("compose-file", "version", []string{"1.25"})
 }

-func addBundlefileFlag(opt *string, flags *pflag.FlagSet) {
+// AddBundlefileFlag adds bundle-file file to the specified flagset
+func AddBundlefileFlag(opt *string, flags *pflag.FlagSet) {


I guess you can drop support for DAB now

AkihiroSuda · 2017-12-01T17:59:53Z

cli/command/stack/kubernetes/api/client/clientset_generated/clientset/clientset.go

+
+// ComposeV1beta1 retrieves the ComposeV1beta1Client
+func (c *Clientset) ComposeV1beta1() composev1beta1.ComposeV1beta1Interface {
+	if c == nil {


How c can be nil?

It's generated code 😓

AkihiroSuda · 2017-12-01T18:07:36Z

vendor.conf

@@ -1,7 +1,7 @@
 github.com/agl/ed25519 d2b94fd789ea21d12fac1a4443dd3a3f79cda72c
 github.com/Azure/go-ansiterm d6e3b3328b783f23731bc4d058875b0371ff8109
 github.com/containerd/continuity 35d55c5e8dd23b32037d56cf97174aff3efdfa83
-github.com/coreos/etcd v3.2.1
+# github.com/coreos/etcd v3.2.1


you can remove comments

Yes please, also would be good to keep this file sorted. Right now the diff is hard to read because some deps moved

AkihiroSuda · 2017-12-02T18:16:16Z

It might be helpful to add logrus.Warn("DOCKER_ORCHESTRATOR=kubernetes is not supported for this command") to docker service and docker node?

vieux · 2017-12-04T23:17:08Z

@vdemeester once the CI is green, could we maybe split the PR in 3 commits: vendor, generated code and actual new code ?

n4ss

First step of review on this big PR.

Congrats for the work 👏

n4ss · 2017-12-04T17:55:29Z

cli/command/orchestrator.go

+	orchestratorUnset = Orchestrator("unset")
+
+	defaultOrchestrator = OrchestratorSwarm
+	dockerOrchestrator  = "DOCKER_ORCHESTRATOR"


nit: dockerEnvOrchestrator?

n4ss · 2017-12-04T17:58:32Z

cli/command/orchestrator.go

+
+func normalize(flag string) Orchestrator {
+	switch strings.ToLower(flag) {
+	case "kubernetes", "k8s":


I would store these hardcoded accepted values in const variables and refer to them in tests.

n4ss · 2017-12-04T18:02:11Z

cli/command/orchestrator.go

+func GetOrchestrator(orchestrator string) Orchestrator {
+	// Check environment variable
+	env := os.Getenv(dockerOrchestrator)
+	if o := normalize(env); o != orchestratorUnset {


We should make sure we specify the precedence in:
environment variable > config file > default

Also if makes a typo on "kubernetes" or "k8s", it will default to swarm without any warning/failure. Should we fail (as the user might want to use kubernetes) or at least have a warning that the environment variable value was invalid?

n4ss · 2017-12-04T18:13:19Z

cli/command/stack/cmd.go

 	)
+	flags := cmd.PersistentFlags()
+	flags.String("namespace", "default", "Kubernetes namespace to use")


It'd probably be cleaner to have those flags in a stackOptions struct

Can't do that one, given how cobra works, we can just use PersistentFlags().Changed("namespace") for those.

n4ss · 2017-12-04T18:22:39Z

cli/command/stack/kubernetes/cli.go

+	kubeNamespace string
+}
+
+// WrapCli wraps command.Cli with kubernetes specifiecs


nit-typo: specifics

n4ss · 2017-12-04T22:54:35Z

cli/command/stack/kubernetes/deploy.go

+	}
+
+	// Parse the compose file
+	stack, cfg, err := LoadStack(opts.Namespace, opts.Composefile)


We have stack and stacks variables where one is not the subset of the other, it's kinda confusing.

n4ss · 2017-12-04T23:03:14Z

cli/command/stack/kubernetes/deploy.go

+		return err
+	}
+
+	if in, err := stacks.Get(stack.Name, metav1.GetOptions{}); err == nil {


You also may want to rename in to stackObject or something more explicit.

Also the Get call might fail for other reasons than the stack.Name not being referenced. Worth adding a test on the error value before trying a stacks.Create?

n4ss · 2017-12-04T23:26:39Z

cli/command/stack/kubernetes/loader.go

+	}
+
+	cfg, err := loader.Load(composetypes.ConfigDetails{
+		WorkingDir: ".",


same as before for the os.Getwd()

n4ss · 2017-12-04T23:30:11Z

cli/command/stack/kubernetes/loader.go

+		for k, v := range svc.Environment {
+			env[k] = v
+		}
+		parsed["services"].(iMap)[svc.Name].(iMap)["environment"] = env


Should we have some form of checking of key presence in the successive maps?

n4ss · 2017-12-04T23:33:42Z

cli/command/stack/kubernetes/loader.go

+
+	for _, value := range env {
+		parts := strings.SplitN(value, "=", 2)
+


We should have length checking here too.

Well, it comes from os.Environ so.. it will always have the = I think. That said, this whole file should go away in a follow up

chris-crone · 2018-01-03T14:41:41Z

@thaJeztah I believe @silvin-lubecki and I found the issue with secrets: you cannot have a kube secret with an underscore.

kubectl create secret generic my_secret --from-literal=file=toto
The Secret "my_secret" is invalid: metadata.name: Invalid value: "my_secret": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

If you change your example to mysecret it will work as expected.

The underlying issue is that we weren't checking for returned errors when creating the secret. Silvin will push a fix for this.

thaJeztah · 2018-01-03T14:44:18Z

oh, awesome! was just running this PR again for another test 👍

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki · 2018-01-03T14:57:48Z

PTAL @thaJeztah, just fixed it.

thaJeztah · 2018-01-03T15:08:21Z

Is it correct that we don't namespace services from a stack?

Deploy the stack as "foobar";

$ docker stack deploy -c docker-compose.yml foobar
Stack foobar was created
Waiting for the stack to be stable and running...
 - Service db has one container running
 - Service web has one container running
Stack foobar is stable and running

Deployg the stack as "foobar2":

$ docker stack deploy -c docker-compose.yml foobar2
service db already present in stack named foobar

We should have a solution for that, because multiple stacks will have the same service names (and this is the reason (when using docker-compose or swarmkit service-names are prefixed with their stack-name).

thaJeztah · 2018-01-03T15:15:38Z

Hm, we also need to find a solution for re-deploying a stack that uses secrets. Secrets are immutable, but when using swarmkit, docker detects that if the content didn't change, and in that case skips creating/updating the secret (and only produces an error if the secret/config changed);

$ docker stack deploy -c docker-compose.yml foobar
secrets "mysecret" already exists

vdemeester · 2018-01-03T15:21:46Z

@thaJeztah we should create issues for those to discuss them and keep track of updates on it 👼

silvin-lubecki · 2018-01-03T15:24:53Z

@thaJeztah you can use the namespace flag:

--namespace string    Kubernetes namespace to use (default "default")

thaJeztah · 2018-01-03T15:34:15Z

@silvin-lubecki yes; probably something we should do by default (e.g., create a namespace com.docker.stacks.<name-of-stack>, or whatever convention 😄)

@vdemeester I'll create an issue 👍

thaJeztah · 2018-01-03T15:35:04Z

Regarding seccomp being set to unconfined by default (see my "some interesting bits" #721 (comment)); this was changed upstream in kubernetes in kubernetes/kubernetes#21790. As explained in kubernetes/kubernetes#20870, the reason for this was that the default seccomp profile was more restrictive, and blocked certain actions, even if (e.g.) --cap-add SYS_ADMIN was used (see moby/moby#20245).

However, with moby/moby#22554, I don't think this is still a problem, so we may want to change this behavior and, for Docker 1.12 and up, enable the default seccomp profile

I'll open an issue for that as well

silvin-lubecki · 2018-01-03T15:39:03Z

@thaJeztah yes we thought about that too, but as we wanted to keep it simple, we decided to put everything in default namespace. Depending the feedbacks we will receive, we may change it in a further PR?

dnephin · 2018-01-03T15:43:43Z

I don't think we should add the namespace flag and ignore the stack positional argument. We should use the stack positional argument as the namespace and remove the flag.

thaJeztah · 2018-01-03T15:56:09Z

opened #776 and #777

dnephin

I guess we can fix the remaining issues in a follow up

LGTM

thaJeztah

LGTM as well; it's a preview, so let's get this out so that people can play with it 👍

thaJeztah · 2018-01-03T16:20:58Z

🎉

mdlinville · 2018-01-11T17:22:37Z

When I created the YAML files for 18.01 after this patch was applied, the one for docker config showed that swarm: false which I think is incorrect. This is kind of serious.

silvin-lubecki requested review from dnephin and vdemeester as code owners November 30, 2017 15:32

GordonTheTurtle added dco/no status/0-triage labels Nov 30, 2017

vdemeester requested review from vieux, simonferquel and thaJeztah November 30, 2017 15:34

vdemeester changed the title ~~Add kubernetes support to docker/cli for master/pinata~~ Add kubernetes support to docker/cli Nov 30, 2017

silvin-lubecki force-pushed the kube branch from b96ec86 to d94fa1f Compare November 30, 2017 15:42

GordonTheTurtle removed the dco/no label Nov 30, 2017

silvin-lubecki force-pushed the kube branch from d94fa1f to fc07c90 Compare November 30, 2017 15:50

docker deleted a comment from GordonTheTurtle Nov 30, 2017

vdemeester added status/2-code-review and removed status/0-triage labels Nov 30, 2017

vdemeester force-pushed the kube branch 3 times, most recently from afe4fc0 to 6c2693f Compare December 1, 2017 14:28

dnephin suggested changes Dec 1, 2017

View reviewed changes

AkihiroSuda reviewed Dec 1, 2017

View reviewed changes

vdemeester force-pushed the kube branch 2 times, most recently from 987aabb to 6851afc Compare December 4, 2017 14:26

n4ss reviewed Dec 4, 2017

View reviewed changes

silvin-lubecki force-pushed the kube branch from 2184594 to f1b1161 Compare January 3, 2018 09:23

Check and return error while creating kubernetes secret and config maps.

18c44e0

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

This was referenced Jan 3, 2018

[k8s] stacks should have their own namespace #776

Closed

[k8s] updating a stack with secrets should detect that the secret didn't change #777

Closed

dnephin approved these changes Jan 3, 2018

View reviewed changes

thaJeztah mentioned this pull request Jan 3, 2018

[k8s] document differences and current limitations of stack deploy on k8s #778

Closed

thaJeztah approved these changes Jan 3, 2018

View reviewed changes

thaJeztah mentioned this pull request Jan 3, 2018

[k8s] improve test-coverage for experimental k8s integration #779

Closed

vdemeester merged commit e708c90 into docker:master Jan 3, 2018

GordonTheTurtle added this to the 18.01.0 milestone Jan 3, 2018

thaJeztah added area/kubernetes impact/changelog labels Jan 4, 2018

This was referenced Jan 11, 2018

Fix docs YAML generation setting "kubernetes" for "swarm" #802

Merged

Remove aliases for orchestrator #803

Merged

mdlinville mentioned this pull request Jan 12, 2018

Document Kubernetes support for docker stack commands. docker/docs#5726

Closed

silvin-lubecki deleted the kube branch January 29, 2018 10:05

		@@ -0,0 +1,4 @@
		// This package is generated by client-gen with custom arguments.


		for _, value := range env {
		parts := strings.SplitN(value, "=", 2)

Add kubernetes support to docker/cli #721

Add kubernetes support to docker/cli #721

Conversation

silvin-lubecki commented Nov 30, 2017 • edited by vdemeester Loading

codecov-io commented Dec 1, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

dnephin commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 2, 2017

vieux commented Dec 4, 2017

n4ss left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-crone commented Jan 3, 2018 • edited Loading

thaJeztah commented Jan 3, 2018

silvin-lubecki commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

vdemeester commented Jan 3, 2018

silvin-lubecki commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

thaJeztah commented Jan 3, 2018

silvin-lubecki commented Jan 3, 2018

dnephin commented Jan 3, 2018 • edited Loading

thaJeztah commented Jan 3, 2018

dnephin left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Jan 3, 2018

mdlinville commented Jan 11, 2018

silvin-lubecki commented Nov 30, 2017 •

edited by vdemeester

Loading

codecov-io commented Dec 1, 2017 •

edited

Loading

dnephin Dec 1, 2017 •

edited

Loading

dnephin Dec 1, 2017 •

edited

Loading

dnephin Dec 1, 2017 •

edited

Loading

dnephin commented Dec 1, 2017 •

edited

Loading

AkihiroSuda Dec 1, 2017 •

edited

Loading

n4ss left a comment •

edited

Loading

chris-crone commented Jan 3, 2018 •

edited

Loading

dnephin commented Jan 3, 2018 •

edited

Loading