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

abhi · 2017-06-05T21:04:17Z

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 --net-alias=web2.0 docknet2 dockcontainer

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

cc @mavenugo

aaronlehmann · 2017-06-13T21:49:24Z

cli/command/network/connect.go

 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types/network"
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context"
+	"strings"


Put fmt and strings first in the list, then a blank line before the non-stdlib imports.

aaronlehmann · 2017-06-13T21:50:46Z

cli/command/network/connect.go

+		parts := strings.SplitN(opt, "=", 2)
+		if len(parts) != 2 {
+			err := fmt.Errorf("invalid key value pair format in driver options")
+			return nil, err


return nil, fmt.Errorf("invalid key/value pair format in driver options")

Perhaps use errors.New("invalid key/value pair format in driver options"), instead of fmt.Errorf()

aaronlehmann · 2017-06-13T21:52:42Z

cli/command/container/opts.go

@@ -899,3 +921,12 @@ func validateAttach(val string) (string, error) {
 	}
 	return val, errors.Errorf("valid streams are STDIN, STDOUT and STDERR")
 }
+
+func convertToNetworkmode(netmode opts.NetworkOpt) container.NetworkMode {


How about adding a NetworkMode method to opts.NetworkOpt?

It would be useful but opts.NetworkOpt is shared between services and container (docker run) and involves different conversion methods.

aaronlehmann · 2017-06-13T21:55:37Z

cli/command/container/opts.go

+	networkIDOrName := "default"
+	for _, net := range netmode.Value() {
+		networkIDOrName = net.Target
+		break


I would instead write this as:

netOptVal := netmode.Value() if len(netOptVal) > 0 { networkIDOrName = netOptVal[0].Target }

But is it correct to silently ignore everything beyond the first value?

docker run does not support multiple networks . if user does give multiple networks only the first network is considered.
So in the above code I decided to do the same as well to keep it in sync.
And if no network options is given then the ID would be "default"

docker run does not support multiple networks . if user does give multiple networks only the first network is considered.

The reason for only supporting a single network, was that there was no way to pass options per-network (also, --network-alias would be applied to all networks that you attached the container to) - also see moby/moby#31964, and issues linked from that.

If the API supports multiple networks on create, perhaps we should change that (can be a follow-up to this PR), as this should now be possible from a UX perspective;

docker run \ --network name=docknet,alias=web1.0,driver-opt=field1=value1 \ --network name=second-net,alias=foobar,driver-opt=hello=world \ nginx:alpine

(edit: removed --driver-opt because there's no such thing 😄)

fcrisciani · 2017-07-06T18:36:10Z

@abhinandanpb can you address @aaronlehmann comments?

thaJeztah · 2017-07-06T18:40:11Z

cli/command/network/connect.go

+			return nil, err
+		}
+		key := strings.TrimSpace(strings.ToLower(parts[0]))
+		value := strings.TrimSpace(strings.ToLower(parts[1]))


Are all options and values lowercase? If not needed, I'd rather not transform options that were passed by the user (in case some particular driver has options that are case sensitive)

Better to have the daemon return an error if an option is not valid

(I realize I didn't check the implementation for services, we may want to consider doing the same there)

yes I thought of that as well. But I saw that the other options like port were converting to lower case. So I decided to maintain the uniformity.

For port we probably have more control over the available options (although I'd be in favour of moving validation to the daemon as well); for plugins/drivers we have no knowledge whatsoever what's supported.

It should be easy to add this back in future if we want, more difficult to remove ("Docker 17.07 supported opt=foObAr and now produces an error!")

thaJeztah · 2017-07-06T23:25:05Z

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

Looks like I didn't look closely at those examples, some remarks;

we "deprecated" --net in favour of --network. Although --net still works (for backward compatibility), would it be possible to only accept the csv syntax when using the --network flag, and report an error when using it for the --net flag? Trying to avoid adding new features to a "frozen" flag
I wonder if we should disallow combining --net-alias and --network (csv notation), as the outcome may be ambiguous, thinking of what happens with;

docker run -itd --network name=docknet,alias=web1.0 --network-alias=foobar alpine

Would it be both web1.0 and foobar as alias? One of them? Perhaps it's okay, just wanted to put it out here for discussion

abhi · 2017-07-07T16:14:34Z

1. we "deprecated" --net in favour of --network. Although --net still works (for backward compatibility), would it be possible to only accept the csv syntax when using the --network flag, and report an error when using it for the --net flag? Trying to avoid adding new features to a "frozen" flag

Yes thats doable and makes sense as well. I can update the PR with the change.

I wonder if we should disallow combining --net-alias and --network (csv notation), as the outcome may be ambiguous, thinking of what happens with;

yes its disallowed. And I believe we should to avoid ambiguity.

@thaJeztah ^

abhi · 2017-07-10T18:38:03Z

@thaJeztah looks like it got closed because of another PR referring to it. Can you please reopen this one ?

GordonTheTurtle added the status/0-triage label Jun 5, 2017

aaronlehmann reviewed Jun 13, 2017

View reviewed changes

thaJeztah added status/1-design-review and removed status/0-triage labels Jul 6, 2017

thaJeztah reviewed Jul 6, 2017

View reviewed changes

This was referenced Jul 10, 2017

17.06 L4 ingress network not accessable moby/moby#34003

Closed

Control interface name in docker multiple networking moby/moby#25181

Open

abhi closed this Jul 10, 2017

abhi force-pushed the master branch from e208ea3 to 2eac0bb Compare July 10, 2017 17:29

abhi mentioned this pull request Jul 10, 2017

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

Closed

thaJeztah mentioned this pull request Mar 21, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

abhi commented Jun 5, 2017 •

edited

aaronlehmann Jun 13, 2017

aaronlehmann Jun 13, 2017

thaJeztah Jul 6, 2017

aaronlehmann Jun 13, 2017

abhi Jul 7, 2017

aaronlehmann Jun 13, 2017

abhi Jul 6, 2017

thaJeztah Jul 6, 2017 •

edited

fcrisciani commented Jul 6, 2017

thaJeztah Jul 6, 2017

abhi Jul 6, 2017

thaJeztah Jul 8, 2017

thaJeztah commented Jul 6, 2017

abhi commented Jul 7, 2017

abhi commented Jul 10, 2017

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

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

Conversation

abhi commented Jun 5, 2017 • edited

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

thaJeztah Jul 6, 2017 • edited

Choose a reason for hiding this comment

fcrisciani commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jul 6, 2017

abhi commented Jul 7, 2017

abhi commented Jul 10, 2017

abhi commented Jun 5, 2017 •

edited

thaJeztah Jul 6, 2017 •

edited