From c4844b1fddf6d86e1eece5cf2e92cd975aa5a422 Mon Sep 17 00:00:00 2001 From: Abhinandan Prativadi Date: Mon, 10 Jul 2017 13:44:45 -0700 Subject: [PATCH 1/3] Cli change to pass driver specific options to docker run 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/cli#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 Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts.go | 29 +++++++++++++++++++++++++---- cli/command/network/connect.go | 29 ++++++++++++++++++++++++++--- opts/network.go | 10 ++++++++++ 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 6b1018508101..327c91e3adac 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -97,7 +97,7 @@ type containerOptions struct { ioMaxBandwidth opts.MemBytes ioMaxIOps uint64 swappiness int64 - netMode string + netMode opts.NetworkOpt macAddress string ipv4Address string ipv6Address string @@ -215,8 +215,8 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { flags.VarP(&copts.publish, "publish", "p", "Publish a container's port(s) to the host") flags.BoolVarP(&copts.publishAll, "publish-all", "P", false, "Publish all exposed ports to random ports") // We allow for both "--net" and "--network", although the latter is the recommended way. - flags.StringVar(&copts.netMode, "net", "default", "Connect a container to a network") - flags.StringVar(&copts.netMode, "network", "default", "Connect a container to a network") + flags.Var(&copts.netMode, "net", "Connect a container to a network") + flags.Var(&copts.netMode, "network", "Connect a container to a network") flags.MarkHidden("net") // We allow for both "--net-alias" and "--network-alias", although the latter is the recommended way. flags.Var(&copts.aliases, "net-alias", "Add network-scoped alias for the container") @@ -613,8 +613,8 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con DNSOptions: copts.dnsOptions.GetAllOrEmpty(), ExtraHosts: copts.extraHosts.GetAll(), VolumesFrom: copts.volumesFrom.GetAll(), - NetworkMode: container.NetworkMode(copts.netMode), IpcMode: container.IpcMode(copts.ipcMode), + NetworkMode: container.NetworkMode(copts.netMode.NetworkMode()), PidMode: pidMode, UTSMode: utsMode, UsernsMode: usernsMode, @@ -678,6 +678,27 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con copy(epConfig.Links, hostConfig.Links) networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig } + value := copts.netMode.Value() + + if value != nil && hostConfig.NetworkMode.IsUserDefined() { + epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] + if epConfig == nil { + epConfig = &networktypes.EndpointSettings{} + } + + if len(value[0].Aliases) > 0 { + if copts.aliases.Len() > 0 { + return nil, fmt.Errorf("ambiguity in alias options provided") + } + epConfig.Aliases = append(epConfig.Aliases, value[0].Aliases...) + } + + if len(value[0].DriverOpts) > 0 { + epConfig.DriverOpts = make(map[string]string) + epConfig.DriverOpts = value[0].DriverOpts + } + networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig + } if copts.aliases.Len() > 0 { epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] diff --git a/cli/command/network/connect.go b/cli/command/network/connect.go index 7ff055aaba88..a5a4e12408c0 100644 --- a/cli/command/network/connect.go +++ b/cli/command/network/connect.go @@ -3,6 +3,9 @@ package network import ( "context" + "fmt" + "strings" + "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/opts" @@ -18,6 +21,7 @@ type connectOptions struct { links opts.ListOpts aliases []string linklocalips []string + driverOpts []string } func newConnectCommand(dockerCli command.Cli) *cobra.Command { @@ -42,22 +46,41 @@ func newConnectCommand(dockerCli command.Cli) *cobra.Command { flags.Var(&options.links, "link", "Add link to another container") flags.StringSliceVar(&options.aliases, "alias", []string{}, "Add network-scoped alias for the container") flags.StringSliceVar(&options.linklocalips, "link-local-ip", []string{}, "Add a link-local address for the container") - + flags.StringSliceVar(&options.driverOpts, "driver-opt", []string{}, "driver options for the network") return cmd } func runConnect(dockerCli command.Cli, options connectOptions) error { client := dockerCli.Client() + driverOpts, err := convertDriverOpt(options.driverOpts) + if err != nil { + return err + } epConfig := &network.EndpointSettings{ IPAMConfig: &network.EndpointIPAMConfig{ IPv4Address: options.ipaddress, IPv6Address: options.ipv6address, LinkLocalIPs: options.linklocalips, }, - Links: options.links.GetAll(), - Aliases: options.aliases, + Links: options.links.GetAll(), + Aliases: options.aliases, + DriverOpts: driverOpts, } return client.NetworkConnect(context.Background(), options.network, options.container, epConfig) } + +func convertDriverOpt(opts []string) (map[string]string, error) { + driverOpt := make(map[string]string) + for _, opt := range opts { + parts := strings.SplitN(opt, "=", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("invalid key/value pair format in driver options") + } + key := strings.TrimSpace(parts[0]) + value := strings.TrimSpace(parts[1]) + driverOpt[key] = value + } + return driverOpt, nil +} diff --git a/opts/network.go b/opts/network.go index ec4967ff320d..3026a7efee67 100644 --- a/opts/network.go +++ b/opts/network.go @@ -95,6 +95,16 @@ func (n *NetworkOpt) String() string { return "" } +// NetworkMode return the network mode for the network option +func (n *NetworkOpt) NetworkMode() string { + networkIDOrName := "default" + netOptVal := n.Value() + if len(netOptVal) > 0 { + networkIDOrName = netOptVal[0].Target + } + return networkIDOrName +} + func parseDriverOpt(driverOpt string) (string, string, error) { parts := strings.SplitN(driverOpt, "=", 2) if len(parts) != 2 { From a88d17c2a4ac2445f264d58632b1b6e460e99921 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 3 Apr 2019 12:33:55 +0200 Subject: [PATCH 2/3] Minor touch-ups in network-option tests Signed-off-by: Sebastiaan van Stijn --- opts/network_test.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/opts/network_test.go b/opts/network_test.go index 5219d14a3aab..1867d8ac8b39 100644 --- a/opts/network_test.go +++ b/opts/network_test.go @@ -28,7 +28,7 @@ func TestNetworkOptLegacySyntax(t *testing.T) { } } -func TestNetworkOptCompleteSyntax(t *testing.T) { +func TestNetworkOptAdvancedSyntax(t *testing.T) { testCases := []struct { value string expected []NetworkAttachmentOpts @@ -69,13 +69,15 @@ func TestNetworkOptCompleteSyntax(t *testing.T) { }, } for _, tc := range testCases { - var network NetworkOpt - assert.NilError(t, network.Set(tc.value)) - assert.Check(t, is.DeepEqual(tc.expected, network.Value())) + t.Run(tc.value, func(t *testing.T) { + var network NetworkOpt + assert.NilError(t, network.Set(tc.value)) + assert.Check(t, is.DeepEqual(tc.expected, network.Value())) + }) } } -func TestNetworkOptInvalidSyntax(t *testing.T) { +func TestNetworkOptAdvancedSyntaxInvalid(t *testing.T) { testCases := []struct { value string expectedError string @@ -94,7 +96,9 @@ func TestNetworkOptInvalidSyntax(t *testing.T) { }, } for _, tc := range testCases { - var network NetworkOpt - assert.ErrorContains(t, network.Set(tc.value), tc.expectedError) + t.Run(tc.value, func(t *testing.T) { + var network NetworkOpt + assert.ErrorContains(t, network.Set(tc.value), tc.expectedError) + }) } } From 5bc09639cc0aa382fdfcd9ca94d7817c8b3073c0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 20 Mar 2019 17:53:44 +0100 Subject: [PATCH 3/3] Refactor network parsing, add preliminary support for multiple networks 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 --- cli/command/container/opts.go | 150 ++++++++++++++++++++--------- cli/command/container/opts_test.go | 133 +++++++++++++++++++++++++ cli/command/network/connect.go | 1 - opts/network.go | 10 +- 4 files changed, 243 insertions(+), 51 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 327c91e3adac..fa17a8859fdd 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -17,6 +17,7 @@ import ( networktypes "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/strslice" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" "github.com/pkg/errors" @@ -654,67 +655,122 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con EndpointsConfig: make(map[string]*networktypes.EndpointSettings), } - if copts.ipv4Address != "" || copts.ipv6Address != "" || copts.linkLocalIPs.Len() > 0 { - epConfig := &networktypes.EndpointSettings{} - networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig + networkingConfig.EndpointsConfig, err = parseNetworkOpts(copts) + if err != nil { + return nil, err + } - epConfig.IPAMConfig = &networktypes.EndpointIPAMConfig{ - IPv4Address: copts.ipv4Address, - IPv6Address: copts.ipv6Address, - } + return &containerConfig{ + Config: config, + HostConfig: hostConfig, + NetworkingConfig: networkingConfig, + }, nil +} - if copts.linkLocalIPs.Len() > 0 { - epConfig.IPAMConfig.LinkLocalIPs = make([]string, copts.linkLocalIPs.Len()) - copy(epConfig.IPAMConfig.LinkLocalIPs, copts.linkLocalIPs.GetAll()) +// parseNetworkOpts converts --network advanced options to endpoint-specs, and combines +// them with the old --network-alias and --links. If returns an error if conflicting options +// are found. +// +// this function may return _multiple_ endpoints, which is not currently supported +// by the daemon, but may be in future; it's up to the daemon to produce an error +// in case that is not supported. +func parseNetworkOpts(copts *containerOptions) (map[string]*networktypes.EndpointSettings, error) { + var ( + endpoints = make(map[string]*networktypes.EndpointSettings, len(copts.netMode.Value())) + hasUserDefined, hasNonUserDefined bool + ) + + for i, n := range copts.netMode.Value() { + if container.NetworkMode(n.Target).IsUserDefined() { + hasUserDefined = true + } else { + hasNonUserDefined = true + } + if i == 0 { + // The first network corresponds with what was previously the "only" + // network, and what would be used when using the non-advanced syntax + // `--network-alias`, `--link`, `--ip`, `--ip6`, and `--link-local-ip` + // are set on this network, to preserve backward compatibility with + // the non-advanced notation + if err := applyContainerOptions(&n, copts); err != nil { + return nil, err + } } + ep, err := parseNetworkAttachmentOpt(n) + if err != nil { + return nil, err + } + if _, ok := endpoints[n.Target]; ok { + return nil, errdefs.InvalidParameter(errors.Errorf("network %q is specified multiple times", n.Target)) + } + endpoints[n.Target] = ep } + if hasUserDefined && hasNonUserDefined { + return nil, errdefs.InvalidParameter(errors.New("conflicting options: cannot attach both user-defined and non-user-defined network-modes")) + } + return endpoints, nil +} - if hostConfig.NetworkMode.IsUserDefined() && len(hostConfig.Links) > 0 { - epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] - if epConfig == nil { - epConfig = &networktypes.EndpointSettings{} - } - epConfig.Links = make([]string, len(hostConfig.Links)) - copy(epConfig.Links, hostConfig.Links) - networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig +func applyContainerOptions(n *opts.NetworkAttachmentOpts, copts *containerOptions) error { + // TODO should copts.MacAddress actually be set on the first network? (currently it's not) + // TODO should we error if _any_ advanced option is used? (i.e. forbid to combine advanced notation with the "old" flags (`--network-alias`, `--link`, `--ip`, `--ip6`)? + if len(n.Aliases) > 0 && copts.aliases.Len() > 0 { + return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --network-alias and per-network alias")) + } + if len(n.Links) > 0 && copts.links.Len() > 0 { + return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --link and per-network links")) + } + if copts.aliases.Len() > 0 { + n.Aliases = make([]string, copts.aliases.Len()) + copy(n.Aliases, copts.aliases.GetAll()) + } + if copts.links.Len() > 0 { + n.Links = make([]string, copts.links.Len()) + copy(n.Links, copts.links.GetAll()) } - value := copts.netMode.Value() - if value != nil && hostConfig.NetworkMode.IsUserDefined() { - epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] - if epConfig == nil { - epConfig = &networktypes.EndpointSettings{} - } + // TODO add IPv4/IPv6 options to the csv notation for --network, and error-out in case of conflicting options + n.IPv4Address = copts.ipv4Address + n.IPv6Address = copts.ipv6Address - if len(value[0].Aliases) > 0 { - if copts.aliases.Len() > 0 { - return nil, fmt.Errorf("ambiguity in alias options provided") - } - epConfig.Aliases = append(epConfig.Aliases, value[0].Aliases...) - } + // TODO should linkLocalIPs be added to the _first_ network only, or to _all_ networks? (should this be a per-network option as well?) + if copts.linkLocalIPs.Len() > 0 { + n.LinkLocalIPs = make([]string, copts.linkLocalIPs.Len()) + copy(n.LinkLocalIPs, copts.linkLocalIPs.GetAll()) + } + return nil +} - if len(value[0].DriverOpts) > 0 { - epConfig.DriverOpts = make(map[string]string) - epConfig.DriverOpts = value[0].DriverOpts +func parseNetworkAttachmentOpt(ep opts.NetworkAttachmentOpts) (*networktypes.EndpointSettings, error) { + if strings.TrimSpace(ep.Target) == "" { + return nil, errors.New("no name set for network") + } + if !container.NetworkMode(ep.Target).IsUserDefined() { + if len(ep.Aliases) > 0 { + return nil, errors.New("network-scoped aliases are only supported for user-defined networks") + } + if len(ep.Links) > 0 { + return nil, errors.New("links are only supported for user-defined networks") } - networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig } - if copts.aliases.Len() > 0 { - epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] - if epConfig == nil { - epConfig = &networktypes.EndpointSettings{} + epConfig := &networktypes.EndpointSettings{} + epConfig.Aliases = append(epConfig.Aliases, ep.Aliases...) + if len(ep.DriverOpts) > 0 { + epConfig.DriverOpts = make(map[string]string) + epConfig.DriverOpts = ep.DriverOpts + } + if len(ep.Links) > 0 { + epConfig.Links = ep.Links + } + if ep.IPv4Address != "" || ep.IPv6Address != "" || len(ep.LinkLocalIPs) > 0 { + epConfig.IPAMConfig = &networktypes.EndpointIPAMConfig{ + IPv4Address: ep.IPv4Address, + IPv6Address: ep.IPv6Address, + LinkLocalIPs: ep.LinkLocalIPs, } - epConfig.Aliases = make([]string, copts.aliases.Len()) - copy(epConfig.Aliases, copts.aliases.GetAll()) - networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig } - - return &containerConfig{ - Config: config, - HostConfig: hostConfig, - NetworkingConfig: networkingConfig, - }, nil + return epConfig, nil } func parsePortOpts(publishOpts []string) ([]string, error) { diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index 3a6aa5857054..bf6387162cb7 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -390,6 +390,139 @@ func TestParseDevice(t *testing.T) { } +func TestParseNetworkConfig(t *testing.T) { + tests := []struct { + name string + flags []string + expected map[string]*networktypes.EndpointSettings + expectedCfg container.HostConfig + expectedErr string + }{ + { + name: "single-network-legacy", + flags: []string{"--network", "net1"}, + expected: map[string]*networktypes.EndpointSettings{"net1": {}}, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "single-network-advanced", + flags: []string{"--network", "name=net1"}, + expected: map[string]*networktypes.EndpointSettings{"net1": {}}, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "single-network-legacy-with-options", + flags: []string{ + "--ip", "172.20.88.22", + "--ip6", "2001:db8::8822", + "--link", "foo:bar", + "--link", "bar:baz", + "--link-local-ip", "169.254.2.2", + "--link-local-ip", "fe80::169:254:2:2", + "--network", "name=net1", + "--network-alias", "web1", + "--network-alias", "web2", + }, + expected: map[string]*networktypes.EndpointSettings{ + "net1": { + IPAMConfig: &networktypes.EndpointIPAMConfig{ + IPv4Address: "172.20.88.22", + IPv6Address: "2001:db8::8822", + LinkLocalIPs: []string{"169.254.2.2", "fe80::169:254:2:2"}, + }, + Links: []string{"foo:bar", "bar:baz"}, + Aliases: []string{"web1", "web2"}, + }, + }, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "multiple-network-advanced-mixed", + flags: []string{ + "--ip", "172.20.88.22", + "--ip6", "2001:db8::8822", + "--link", "foo:bar", + "--link", "bar:baz", + "--link-local-ip", "169.254.2.2", + "--link-local-ip", "fe80::169:254:2:2", + "--network", "name=net1,driver-opt=field1=value1", + "--network-alias", "web1", + "--network-alias", "web2", + "--network", "net2", + "--network", "name=net3,alias=web3,driver-opt=field3=value3", + }, + expected: map[string]*networktypes.EndpointSettings{ + "net1": { + DriverOpts: map[string]string{"field1": "value1"}, + IPAMConfig: &networktypes.EndpointIPAMConfig{ + IPv4Address: "172.20.88.22", + IPv6Address: "2001:db8::8822", + LinkLocalIPs: []string{"169.254.2.2", "fe80::169:254:2:2"}, + }, + Links: []string{"foo:bar", "bar:baz"}, + Aliases: []string{"web1", "web2"}, + }, + "net2": {}, + "net3": { + DriverOpts: map[string]string{"field3": "value3"}, + Aliases: []string{"web3"}, + }, + }, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "single-network-advanced-with-options", + flags: []string{"--network", "name=net1,alias=web1,alias=web2,driver-opt=field1=value1,driver-opt=field2=value2"}, + expected: map[string]*networktypes.EndpointSettings{ + "net1": { + DriverOpts: map[string]string{ + "field1": "value1", + "field2": "value2", + }, + Aliases: []string{"web1", "web2"}, + }, + }, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "multiple-networks", + flags: []string{"--network", "net1", "--network", "name=net2"}, + expected: map[string]*networktypes.EndpointSettings{"net1": {}, "net2": {}}, + expectedCfg: container.HostConfig{NetworkMode: "net1"}, + }, + { + name: "conflict-network", + flags: []string{"--network", "duplicate", "--network", "name=duplicate"}, + expectedErr: `network "duplicate" is specified multiple times`, + }, + { + name: "conflict-options", + flags: []string{"--network", "name=net1,alias=web1", "--network-alias", "web1"}, + expectedErr: `conflicting options: cannot specify both --network-alias and per-network alias`, + }, + { + name: "invalid-mixed-network-types", + flags: []string{"--network", "name=host", "--network", "net1"}, + expectedErr: `conflicting options: cannot attach both user-defined and non-user-defined network-modes`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, hConfig, nwConfig, err := parseRun(tc.flags) + + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + return + } + + assert.NilError(t, err) + assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedCfg.NetworkMode) + assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected) + }) + } +} + func TestParseModes(t *testing.T) { // pid ko flags, copts := setupRunFlags() diff --git a/cli/command/network/connect.go b/cli/command/network/connect.go index a5a4e12408c0..04108e20fadb 100644 --- a/cli/command/network/connect.go +++ b/cli/command/network/connect.go @@ -2,7 +2,6 @@ package network import ( "context" - "fmt" "strings" diff --git a/opts/network.go b/opts/network.go index 3026a7efee67..4f5b53b1b60d 100644 --- a/opts/network.go +++ b/opts/network.go @@ -15,9 +15,13 @@ const ( // NetworkAttachmentOpts represents the network options for endpoint creation type NetworkAttachmentOpts struct { - Target string - Aliases []string - DriverOpts map[string]string + Target string + Aliases []string + DriverOpts map[string]string + Links []string // TODO add support for links in the csv notation of `--network` + IPv4Address string // TODO add support for IPv4-address in the csv notation of `--network` + IPv6Address string // TODO add support for IPv6-address in the csv notation of `--network` + LinkLocalIPs []string // TODO add support for LinkLocalIPs in the csv notation of `--network` ? } // NetworkOpt represents a network config in swarm mode.