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

Add support for docker run in swarm mode overlay #25962

Merged
merged 3 commits into from Sep 8, 2016

Conversation

Projects
None yet
@mrjana
Contributor

mrjana commented Aug 23, 2016

This PR adds support for running regular containers to be connected to
swarm mode multi-host network so that:
- containers connected to the same network across the cluster can
discover and connect to each other.
- Get access to services(and their associated loadbalancers)
connected to the same network

Signed-off-by: Jana Radhakrishnan mrjana@docker.com

@mrjana

This comment has been minimized.

Contributor

mrjana commented Aug 24, 2016

The corresponding engine-api PR: docker/engine-api#375

@@ -55,6 +56,7 @@ func newCreateCommand(dockerCli *client.DockerCli) *cobra.Command {
flags.StringSliceVar(&opts.labels, "label", []string{}, "Set metadata on a network")
flags.BoolVar(&opts.internal, "internal", false, "Restrict external access to the network")
flags.BoolVar(&opts.ipv6, "ipv6", false, "Enable IPv6 networking")
flags.BoolVar(&opts.manuallyAttachable, "manually-attachable", false, "Enable manual container attachment")

This comment has been minimized.

@mavenugo

mavenugo Aug 24, 2016

Contributor

Not a fan of the flag-name. But I will let other maintainers to chime in with comments.

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 24, 2016

Contributor

I don't even like that flag. I'm not sure why anyone would not set this.

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 24, 2016

Contributor

Same of course for the API change.

This comment has been minimized.

@mrjana

mrjana Aug 24, 2016

Contributor

The reason for the existence of the flag is to make sure that a worker node can run containers only on networks where they were explicitly given permission to do so. The concern is a compromised worker node can compromise security by being able to attach to any network and can do so repeatedly in an effort to deplete network resources and creating a DoS scenario. The way it is designed now, the damage such a compromised node can inflict is only limited to networks which have this flag enabled.

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 24, 2016

Contributor

But can't someone on a compromised worker node just create a bunch of services anyway?
Or docker run --net=container:<some task>?
Why would a compromised node that can attach containers to any given network be any different than a compromised node that can attach services to any given network?

This comment has been minimized.

@mavenugo

mavenugo Aug 25, 2016

Contributor

@mrjana since the need for this flag is explicitly to adhere to the swarmkit security model, could we name it appropriately (such as --worker-accessible / -w and that makes this network accessible by workers from a mgmt standpoint)

This comment has been minimized.

@aluzzardi

aluzzardi Aug 25, 2016

Member

@cpuguy83 The flag's existence is purely for security reasons.

Background: Networks are completely unreachable from nodes that are not running any service from that network (e.g. they don't gossip and they don't have the encryption keys). Isolation can be achieved for instance by limiting a service to a specific set of nodes through constraints.

However, there are (many) use cases that require manually attaching a container to a network using docker run. In order to keep our secure by default mantra, it was decided to have this as a separate flag.

We could think this through and perhaps switch the flag to on by default in future releases depending on user reaction.

/cc @diogomonica @icecrime

This comment has been minimized.

@diogomonica

diogomonica Aug 25, 2016

Contributor

Wait, @mrjana @mavenugo workers should never be able to manage anything. If you want a network to be joinable by any node, you mark it as --manually-attachable or --worker-accessible or whatever, but workers should never be able to do anything on it except be sent the secret to participate.

This comment has been minimized.

@diogomonica

diogomonica Aug 25, 2016

Contributor

Just so we're even more clear: any operation that changes the network's behavior (add or remove manually-attachable) should always be done on a manager.

This comment has been minimized.

@mavenugo

mavenugo Aug 25, 2016

Contributor

@diogomonica agreed and yes. thats the reason I just suggested the flag name change alone to be --worker-accessible (instead of manually-attachable).

@@ -462,7 +463,6 @@ func (opts *serviceOptions) ToService() (swarm.ServiceSpec, error) {
Delay: opts.update.delay,
FailureAction: opts.update.onFailure,
},
Networks: convertNetworks(opts.networks),

This comment has been minimized.

@mavenugo

mavenugo Aug 24, 2016

Contributor

I understand that it is swarmkit related changes. Since this is a Spec change, I just want to make sure if this spec change will impact a mixed 1.12.1 and later versions of 1.12.x / 1.13 in manager and workers ?

This comment has been minimized.

@mrjana

mrjana Aug 24, 2016

Contributor

The whole spec change is designed to to work in a cluster of nodes in different versions. But a docker 1.13 client will always only set the NetworkAttachmentConfig in TaskSpec.

This comment has been minimized.

@mavenugo

mavenugo Aug 24, 2016

Contributor

Great. Thanks for the clarification. Just to confirm , if the manager is 1.13 and some of the workers are 1.12.1, then the only limitation would be that the docker run will not work in those 1.12.1 workers. The 1.13 workers and managers can enjoy this feature. correct ?

This comment has been minimized.

@mrjana

mrjana Aug 24, 2016

Contributor

Yes, that is correct.

}
}
if c.agent != nil {
close(c.agentInitDone)

This comment has been minimized.

@ehazlett

ehazlett Aug 24, 2016

Contributor

This is causing a panic when shutting down the engine:

INFO[0074] Stopping manager                             
INFO[0074] Initializing Libnetwork Agent Local-addr=192.168.100.195 Adv-addr=192.168.100.195 Remote-addr = 
panic: close of closed channel

goroutine 148 [running]:
panic(0x15bdce0, 0xc420eecad0)
        /usr/local/go/src/runtime/panic.go:500 +0x1a1
github.com/docker/libnetwork.(*controller).agentSetup(0xc4200bee10, 0xc42000dd00, 0x0)
        /go/src/github.com/docker/docker/vendor/src/github.com/docker/libnetwork/agent.go:197 +0x2b3
github.com/docker/libnetwork.(*controller).clusterAgentInit(0xc4200bee10)
        /go/src/github.com/docker/docker/vendor/src/github.com/docker/libnetwork/controller.go:303 +0x191
created by github.com/docker/libnetwork.(*controller).SetClusterProvider
        /go/src/github.com/docker/docker/vendor/src/github.com/docker/libnetwork/controller.go:235 +0xe5

This comment has been minimized.

@mrjana

mrjana Aug 24, 2016

Contributor

@ehazlett Thanks for giving it a spin. Will take a look at this.

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Aug 24, 2016

I agree with @cpuguy83. I'm not sure why you would not set it in case you needed it later. Is there a reason?

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Aug 24, 2016

FWIW I also tested this across a multinode cluster and it works (container attaches to net and can ping, etc).

@mrjana

This comment has been minimized.

Contributor

mrjana commented Aug 24, 2016

I agree with @cpuguy83. I'm not sure why you would not set it in case you needed it later. Is there a reason?

If you need it later you would set it, but in a typical deployment the cluster admin can decide and plan ahead of time which networks they want to allow container attaches and which containers are solely for services.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Aug 25, 2016

Is a boolean enough here?
Since it is security-related would it make more since to have a full policy per network?

I'm bad at naming things but something like:

type NonSwarmAttachmentPolicy struct {
    MaxAttachments int // max number of containers that can be attached
    IPRange  cidr // sub-set of the full network range
    AttachFromWorker bool // allow attaching from worker node
}
@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Aug 25, 2016

Also thinking it should just allowed on manager nodes (maybe the example struct needs some tweaking there) by default. Otherwise we'll just get blowback from users about weird behavior. This way we can at least say "we don't allow it on worker nodes by default for security reasons, but you can do it from manager nodes".

@mrjana mrjana force-pushed the mrjana:net branch 3 times, most recently from d778e6e to d189dd9 Aug 25, 2016

@mrjana

This comment has been minimized.

Contributor

mrjana commented Aug 25, 2016

Also thinking it should just allowed on manager nodes (maybe the example struct needs some tweaking there) by default.

I think it makes simpler for users to reason about a flag without any additional context. The behavior of a command differing based on the role of the node just make things a bit more complicated IMO. Once we properly document the purpose of the flag, the users will always be able to reason out quickly that in swarm mode they need to add this flag if they have to attach a regular container to that network.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Aug 25, 2016

@mrjana We already have behavior differences on non-manager nodes.

@mrjana mrjana force-pushed the mrjana:net branch from d189dd9 to 4f4907a Aug 25, 2016

@aluzzardi

This comment has been minimized.

Member

aluzzardi commented Aug 25, 2016

@cpuguy83 @mrjana The reason is operations such as creating a service are part of swarm management by their nature, therefore it made sense to have them restricted to managers. Creating a container and attaching it to a network falls into the "payload" category - if a user wants to manually run a mysql (non-managed container), it would be very restricting to force to run it on manager nodes. Service creation has no impact on the underlying node while docker run does.

@ehazlett @cpuguy83 @mrjana Regarding that flag - I agree that it might be useful by default, however we decided not to do so for the time being (but this is still debatable). How would you feel about:

  • It's disabled by default
  • We allow users to change it using docker network update foo --manually-attachable=true
  • We may consider to enable it by default in the future depending on user reception of 1.13.

/cc @diogomonica @icecrime

@mrjana

This comment has been minimized.

Contributor

mrjana commented Aug 25, 2016

We allow users to change it using docker network update foo --manually-attachable=true

We did discuss this option and this is definitely something that we want to in the future.

@dhiltgen

This comment has been minimized.

Contributor

dhiltgen commented Aug 25, 2016

We allow users to change it using docker network update foo --manually-attachable=true

+1

Related: docker/libnetwork#1414

@mrjana mrjana force-pushed the mrjana:net branch 2 times, most recently from 4622360 to afbf2e5 Aug 25, 2016

@mavenugo

This comment has been minimized.

Contributor

mavenugo commented Aug 25, 2016

We allow users to change it using docker network update foo --manually-attachable=true

We can certainly discuss about mutability of the network object and reusing docker update. It is not just for this flag. There are other configurations that can be updated as well. But I dont think this PR should be the place for making network as a mutable object.

@diogomonica

This comment has been minimized.

Contributor

diogomonica commented Aug 25, 2016

@aluzzardi I strongly recommend against enabling this by default. That will be essentially disabling the whole security model, because no one will ever enable it, and we'll be in a situation where workers can join whatever networks they want (which is exactly what we designed our model against).

@diogomonica

This comment has been minimized.

Contributor

diogomonica commented Aug 25, 2016

@ehazlett the reason is: you should always use least-privilege. If you don't need random nodes attaching to random networks right now, don't enable it. If you ever need it, enable it temporarily. What I don't want is people to always enable it, since they will have a less secure cluster because of it.

And we should make it explicit that: if you should avoid using this feature. This makes this network vulnerable to the compromise of any worker node in the cluster, since any worker node can attach to it.

@ehazlett

This comment has been minimized.

Contributor

ehazlett commented Aug 25, 2016

@diogomonica cool thx. i didn't know what the attaching affected -- thx for clarifying.

@rogaha

This comment has been minimized.

Contributor

rogaha commented Aug 25, 2016

Indeed, thanks for clarifying @diogomonica. I've heard lots of feedback from people trying to use docker that way (docker run in swarm mode overlay) though.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Aug 25, 2016

OK, I think I understand the intent here...
What about --scope=global and --scope=swarm?

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Aug 25, 2016

btw, I choose these values b/c we're already using it for networks and it pretty much matches what we're doing here.

@mavenugo

This comment has been minimized.

Contributor

mavenugo commented Aug 25, 2016

@cpuguy83

What about --scope=global and --scope=swarm?

Though not directly related to --manually-attachable idea.... I like the suggestion, especially because it captures the intent and is inline with the definition of scope in swarm and non-swarm-mode. In non-swarm-mode, overlay networks are automatically placed in global scope and in swarm-mode it is placed in swarm scope. But letting the user specify that it also makes it possible (in the future) for the driver to accept or reject such a configuration.

1 drawback with this suggestion though is that we are overloading the concept of scope with this new requirement. And also scope is a concept managed and handled by drivers/plugins. Whereas this requirement has no involvement from the drivers.

@rogaha

This comment has been minimized.

Contributor

rogaha commented Oct 3, 2016

This is going to be on 1.12-rc2, right?

@mavenugo

This comment has been minimized.

Contributor

mavenugo commented Oct 3, 2016

@rogaha no. the milestone is for 1.13.0.

@rogaha

This comment has been minimized.

Contributor

rogaha commented Oct 3, 2016

ok, sounds good. thanks for the heads up @mavenugo.

@gprivitera

This comment has been minimized.

gprivitera commented Nov 15, 2016

Did it make it in 1.13rc1? Because I still have the exact same problem using that version.
@mrjana

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Nov 15, 2016

@gprivitera can you open an issue, and steps to reproduce? That makes it easier to track and prioritize if needed

@gprivitera

This comment has been minimized.

gprivitera commented Nov 15, 2016

#23901 It already exist, do you want me to create one for 1.13rc1?
edit: done.

@generalhenry generalhenry referenced this pull request Dec 13, 2016

Closed

Attachable Networks #559

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13
Pull request moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby#27702 added `--network` to `docker build`
- moby#25962 added `--attachable` to `docker network create`
- moby#27998 added `--compose-file` to `docker stack deploy`
- moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby#26061 added `--init` to `docker run` and `docker create`
- moby#26941 added `--init-path` to `docker run` and `docker create`
- moby#27958 added `--cpus` on `docker run` / `docker create`
- moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby#27596 added `--force` to `docker service update`
- moby#27857 added `--hostname` to `docker service create`
- moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017

Add version annotation to various flags added in 1.13
Pull request moby/moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby/moby#27702 added `--network` to `docker build`
- moby/moby#25962 added `--attachable` to `docker network create`
- moby/moby#27998 added `--compose-file` to `docker stack deploy`
- moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby/moby#26061 added `--init` to `docker run` and `docker create`
- moby/moby#26941 added `--init-path` to `docker run` and `docker create`
- moby/moby#27958 added `--cpus` on `docker run` / `docker create`
- moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby/moby#27596 added `--force` to `docker service update`
- moby/moby#27857 added `--hostname` to `docker service create`
- moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby/moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli

srust added a commit to srust/moby that referenced this pull request Nov 30, 2017

Add version annotation to various flags added in 1.13
Pull request moby#27745 added support for the
client to talk to older versions of the daemon. Various flags were added to
docker 1.13 that are not compatible with older daemons.

This PR adds annotations to those flags, so that they are automatically hidden
if the daemon is older than docker 1.13 (API 1.25).

Not all new flags affect the API (some are client-side only). The following
PR's added new flags to docker 1.13 that affect the API;

- moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime`
- moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm`
- moby#27702 added `--network` to `docker build`
- moby#25962 added `--attachable` to `docker network create`
- moby#27998 added `--compose-file` to `docker stack deploy`
- moby#22566 added `--stop-timeout` to `docker run` and `docker create`
- moby#26061 added `--init` to `docker run` and `docker create`
- moby#26941 added `--init-path` to `docker run` and `docker create`
- moby#27958 added `--cpus` on `docker run` / `docker create`
- moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create`
- moby#27596 added `--force` to `docker service update`
- moby#27857 added `--hostname` to `docker service create`
- moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update`
- moby#28076 added `--tty` on `docker service create` / `docker service update`
- moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update`
- moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment