[carry 317] Cli change to pass driver specific options to docker run #1767

thaJeztah · 2019-03-21T10:35:10Z

carries #317
closes #317
closes #156

partially implements: RFC: add advanced "csv" syntax for "--net" / "--network" moby/moby#31964 (RFC: add advanced "csv" syntax for "--net" / "--network")
relates to feature: pass IPAM options at docker run moby/libnetwork#2066 feature: pass IPAM options at docker run

Adds preliminary support for multiple networks on docker run now that this syntax would allow this; the daemon does not yet support this, so specifying multiple networks will still produce an error

Opened this PR since #156 was closed.

The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's.
The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since #62 . This commit extends this support to docker run command as well.
For docker connect command --driver-opt is added to pass driver specific options for the network the container is connecting to.

Following is supported:

docker run -itd --net name=docknet,alias=web1.0,driver-opt=field1=value1 alpine
docker run -itd --net name=docknet,driver-opt=field1=value1 --net-alias=web1.0 alpine
docker run -itd --net name=docknet --net-alias web1 alpine

docker connect network --driver-opt field1=value1 --alias=web2.0 docknet2 dockcontainer

Signed-off-by: Abhinandan Prativadi abhi@docker.com

codecov-io · 2019-03-21T10:41:31Z

Codecov Report

Merging #1767 into master will decrease coverage by 0.15%.
The diff coverage is 23.94%.

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
- Coverage   56.32%   56.17%   -0.16%     
==========================================
  Files         307      307              
  Lines       21177    21210      +33     
==========================================
- Hits        11928    11914      -14     
- Misses       8382     8425      +43     
- Partials      867      871       +4

codecov-io · 2019-03-21T10:41:31Z

Codecov Report

Merging #1767 into master will increase coverage by 0.11%.
The diff coverage is 73.11%.

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
+ Coverage   56.28%   56.39%   +0.11%     
==========================================
  Files         308      308              
  Lines       21299    21360      +61     
==========================================
+ Hits        11988    12047      +59     
+ Misses       8436     8434       -2     
- Partials      875      879       +4

thaJeztah · 2019-03-21T11:32:40Z

@vdemeester @silvin-lubecki I marked this as "WIP", but perhaps it's already ready for review (possibly tests will have to be added still); I'm on PTO until Monday, but if you think this would be good to have for this release; I can add tests in a follow-up

thaJeztah · 2019-03-21T11:34:45Z

To support multiple networks; this part will have to be removed from the engine, but otherwise (I think) that all works; https://github.com/moby/moby/blob/c7105e3c99286c88855c8d35079f957281dbf581/daemon/create.go#L308-L314

vdemeester

LGTM 🐯

thaJeztah · 2019-04-03T12:39:33Z

🤔

WARNING: deadline exceeded by linter unparam (try increasing --deadline)
cli/command/container/hijack.go:188:1:warning: nolint directive did not match any issue (nolint)
cli/command/trust/signer_remove.go:79:1:warning: nolint directive did not match any issue (nolint)

thaJeztah · 2019-04-03T12:52:24Z

ping @vdemeester @silvin-lubecki I think this should be ok now; I added tests.

Will need follow-ups for the completion-scripts (@albers) and for documentation 🤗

vdemeester

Still LGTM 💃

cli/command/container/opts.go

thaJeztah · 2019-04-03T14:29:49Z

opts/network_test.go

 	}
 }

-func TestNetworkOptInvalidSyntax(t *testing.T) {
+func TestNetworkOptdvancedSyntaxInvalid(t *testing.T) {


oh, typo; fixing

The commit contains cli changes to support driver options for a network in docker run and docker network connect cli's. The driver-opt, aliases is now supported in the form of csv as per network option in service commands in swarm mode since docker#62 . This commit extends this support to docker run command as well. For docker connect command `--driver-opt` is added to pass driver specific options for the network the container is connecting to. Signed-off-by: Abhinandan Prativadi <abhi@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/container/opts.go

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

silvin-lubecki

LGTM

This refactors the way networking options are parsed, and makes the client able to pass options for multiple networks. Currently, the daemon does not yet accept multiple networks when creating a container, and will produce an error. For backward-compatibility, the following global networking-related options are associated with the first network (in case multiple networks are set); - `--ip` - `--ip6` - `--link` - `--link-local-ip` - `--network-alias` Not all of these options are supported yet in the advanced notation, but for options that are supported, setting both the per-network option and the global option will produce a "conflicting options" error. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

silvin-lubecki

Still LGTM 🕺

For backward compatibility: if no custom options are provided for the network, and only a single network is specified, omit the endpoint-configuration on the client (the daemon will still create it when creating the container) This fixes an issue on older versions of legacy Swarm, which did not support `NetworkingConfig.EndpointConfig`. This was introduced in 5bc0963 (docker#1767) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

For backward compatibility: if no custom options are provided for the network, and only a single network is specified, omit the endpoint-configuration on the client (the daemon will still create it when creating the container) This fixes an issue on older versions of legacy Swarm, which did not support `NetworkingConfig.EndpointConfig`. This was introduced in 5bc0963 (docker#1767) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 393c4f3) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

For backward compatibility: if no custom options are provided for the network, and only a single network is specified, omit the endpoint-configuration on the client (the daemon will still create it when creating the container) This fixes an issue on older versions of legacy Swarm, which did not support `NetworkingConfig.EndpointConfig`. This was introduced in 5bc0963 (docker#1767) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

For backward compatibility: if no custom options are provided for the network, and only a single network is specified, omit the endpoint-configuration on the client (the daemon will still create it when creating the container) This fixes an issue on older versions of legacy Swarm, which did not support `NetworkingConfig.EndpointConfig`. This was introduced in 5bc0963 (docker#1767) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 4d7e6bf) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cpuguy83 · 2021-03-18T17:04:48Z

cli/command/container/opts.go

+			return nil, err
+		}
+		if _, ok := endpoints[n.Target]; ok {
+			return nil, errdefs.InvalidParameter(errors.Errorf("network %q is specified multiple times", n.Target))


Any particular reason this was added?
We have a test in integration-cli in moby/moby that tests that this exact case is successful.

Opened moby/moby#42163 to discuss removing the test case that this validation breaks.

albers · 2021-03-18T19:09:46Z

@thaJeztah The csv syntax is a big problem in bash completion because it does not treat , as a word separator.
That's why I did not add this syntax for any of the commands that have it.

david-l-riley · 2021-09-02T14:58:11Z

To support multiple networks; this part will have to be removed from the engine, but otherwise (I think) that all works; https://github.com/moby/moby/blob/c7105e3c99286c88855c8d35079f957281dbf581/daemon/create.go#L308-L314

Is there a reason this isn't done yet? I'm happy to file a PR to make this, but given how simple it seems I just want to make sure there's not a deeper reason it hasn't been done. Seems like an arbitrary check for something the engine should be able to do just fine? Does an API spec need to be updated?

thaJeztah · 2021-09-02T15:09:43Z

Is there a reason this isn't done yet? I'm happy to file a PR to make this, but given how simple it seems I just want to make sure there's not a deeper reason it hasn't been done. Seems like an arbitrary check for something the engine should be able to do just fine? Does an API spec need to be updated?

No specific reason, other than that nobody has come round to it yet. When making that change, it should probably be gated by API version (i.e., keep the old behavior for existing API versions), and tests will be needed; the network code is known to be "tricky" at times (lots of corner-cases and due to complexity on setting up the network, there's various potential race-conditions).

That said; at least having a (draft) pull request that removes the check, and verifies it works never hurts.

And once it's in a release, we can possibly update docker-compose as well to consume it (connecting multiple networks instead of connecting them one at a time)

david-l-riley · 2021-09-02T15:30:31Z

No specific reason, other than that nobody has come round to it yet. When making that change, it should probably be gated by API version (i.e., keep the old behavior for existing API versions), and tests will be needed; the network code is known to be "tricky" at times (lots of corner-cases and due to complexity on setting up the network, there's various potential race-conditions).

That's a perfectly good reason. :-) Just wanted to make sure it wasn't a "this turned out to be unexpectedly hard" thing.

That said; at least having a (draft) pull request that removes the check, and verifies it works never hurts.

I think I can make that happen.

And once it's in a release, we can possibly update docker-compose as well to consume it (connecting multiple networks instead of connecting them one at a time)

This is basically adjacent to my motivation; attaching multiple networks to a container is easy in docker-compose, but for local dev testing with docker run -it --rm it's been a headache trying to make it work without either scripting the whole thing with a create/network attach/start sequence, which makes --rm not do what's desired, or running a separate terminal/job to attach it. Making docker-compose do it the "right" way would be a nice cherry on top.

thaJeztah · 2021-09-02T15:46:57Z

100% agreed! If it all works, it's a lot cleaner (create this container attached to these networks, with these options) all in a single API call 🎉

The new advanced --network syntax has been introduced in docker#1767 but without support for link-local-ip and mac-address fields. This commit adds both. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

The new advanced --network syntax introduced in docker#1767 is lacking support for `link-local-ip` and `mac-address` fields. This commit adds both. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

GordonTheTurtle added the status/0-triage label Mar 21, 2019

thaJeztah force-pushed the carry_317_network_advanced branch from 600ce6a to 375ba2b Compare March 21, 2019 10:44

vdemeester approved these changes Mar 21, 2019

View reviewed changes

thaJeztah force-pushed the carry_317_network_advanced branch from 375ba2b to 0c146eb Compare April 3, 2019 12:13

thaJeztah force-pushed the carry_317_network_advanced branch from 0c146eb to e447b6d Compare April 3, 2019 12:41

thaJeztah added docs/revisit impact/changelog impact/documentation status/2-code-review and removed status/0-triage labels Apr 3, 2019

thaJeztah changed the title ~~[WIP][carry 317] Cli change to pass driver specific options to docker run~~ [carry 317] Cli change to pass driver specific options to docker run Apr 3, 2019

thaJeztah mentioned this pull request Apr 3, 2019

Add ip address to advanced options #1803

Merged

vdemeester approved these changes Apr 3, 2019

View reviewed changes

silvin-lubecki reviewed Apr 3, 2019

View reviewed changes

cli/command/container/opts.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the carry_317_network_advanced branch from e447b6d to 0935350 Compare April 3, 2019 14:28

thaJeztah commented Apr 3, 2019

View reviewed changes

silvin-lubecki reviewed Apr 3, 2019

View reviewed changes

cli/command/container/opts.go Show resolved Hide resolved

Minor touch-ups in network-option tests

a88d17c

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

thaJeztah force-pushed the carry_317_network_advanced branch from 0935350 to a7ee071 Compare April 3, 2019 14:31

silvin-lubecki approved these changes Apr 3, 2019

View reviewed changes

thaJeztah force-pushed the carry_317_network_advanced branch from a7ee071 to 5bc0963 Compare April 3, 2019 14:43

silvin-lubecki approved these changes Apr 3, 2019

View reviewed changes

GordonTheTurtle added this to the 19.03.0 milestone Apr 3, 2019

thaJeztah deleted the carry_317_network_advanced branch April 3, 2019 15:03

thaJeztah mentioned this pull request Apr 3, 2019

Cli change to pass driver specific options to docker run #317

Closed

wsong mentioned this pull request Apr 30, 2019

19.03 CLI always sets NetworkingConfig.EndpointsConfig on any container with a network #1854

Closed

thaJeztah mentioned this pull request Jun 21, 2019

Fix advanced options for backward compatibility #1963

Merged

thaJeztah mentioned this pull request Jun 24, 2019

[19.03 backport] Fix advanced options for backward compatibility #1967

Merged

cpuguy83 reviewed Mar 18, 2021

View reviewed changes

cpuguy83 mentioned this pull request Mar 18, 2021

Remove cli test for duplicate --net/--network opts moby/moby#42163

Merged

david-l-riley mentioned this pull request Sep 2, 2021

Feature request for docker run: Ability to connect to multiple networks #2455

Open

thaJeztah mentioned this pull request Dec 20, 2022

Ambiguous error message when creating a container with more than one network moby/moby#44613

Closed

akerouanton mentioned this pull request Jul 7, 2023

api: Allow ContainerCreate to take several EndpointsConfig for >= API 1.44 moby/moby#45906

Merged

akerouanton mentioned this pull request Jul 13, 2023

Add missing opts to --network advanced syntax #4419

Merged

akerouanton mentioned this pull request Mar 5, 2024

network: multiple nets with --network docker/docs#19566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[carry 317] Cli change to pass driver specific options to docker run #1767

[carry 317] Cli change to pass driver specific options to docker run #1767

thaJeztah commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 21, 2019

codecov-io commented Mar 21, 2019 •

edited

Loading

thaJeztah commented Mar 21, 2019

thaJeztah commented Mar 21, 2019

vdemeester left a comment

thaJeztah commented Apr 3, 2019

thaJeztah commented Apr 3, 2019

vdemeester left a comment

thaJeztah Apr 3, 2019

silvin-lubecki left a comment

silvin-lubecki left a comment

cpuguy83 Mar 18, 2021

cpuguy83 Mar 18, 2021

albers commented Mar 18, 2021

david-l-riley commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

david-l-riley commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

[carry 317] Cli change to pass driver specific options to docker run #1767

[carry 317] Cli change to pass driver specific options to docker run #1767

Conversation

thaJeztah commented Mar 21, 2019 • edited Loading

codecov-io commented Mar 21, 2019

Codecov Report

codecov-io commented Mar 21, 2019 • edited Loading

Codecov Report

thaJeztah commented Mar 21, 2019

thaJeztah commented Mar 21, 2019

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 3, 2019

thaJeztah commented Apr 3, 2019

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah Apr 3, 2019

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

cpuguy83 Mar 18, 2021

Choose a reason for hiding this comment

cpuguy83 Mar 18, 2021

Choose a reason for hiding this comment

albers commented Mar 18, 2021

david-l-riley commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

david-l-riley commented Sep 2, 2021

thaJeztah commented Sep 2, 2021

thaJeztah commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 21, 2019 •

edited

Loading