Merge pull request #1767 from thaJeztah/carry_317_network_advanced

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

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"
@@ -97,7 +98,7 @@ type containerOptions struct {
 	ioMaxBandwidth     opts.MemBytes
 	ioMaxIOps          uint64
 	swappiness         int64
-	netMode            string
+	netMode            opts.NetworkOpt
 	macAddress         string
 	ipv4Address        string
 	ipv6Address        string
@@ -215,8 +216,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 +614,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,
@@ -654,39 +655,9 @@ 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
-
-		epConfig.IPAMConfig = &networktypes.EndpointIPAMConfig{
-			IPv4Address: copts.ipv4Address,
-			IPv6Address: copts.ipv6Address,
-		}
-
-		if copts.linkLocalIPs.Len() > 0 {
-			epConfig.IPAMConfig.LinkLocalIPs = make([]string, copts.linkLocalIPs.Len())
-			copy(epConfig.IPAMConfig.LinkLocalIPs, copts.linkLocalIPs.GetAll())
-		}
-	}
-
-	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
-	}
-
-	if copts.aliases.Len() > 0 {
-		epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)]
-		if epConfig == nil {
-			epConfig = &networktypes.EndpointSettings{}
-		}
-		epConfig.Aliases = make([]string, copts.aliases.Len())
-		copy(epConfig.Aliases, copts.aliases.GetAll())
-		networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig
+	networkingConfig.EndpointsConfig, err = parseNetworkOpts(copts)
+	if err != nil {
+		return nil, err
 	}
 
 	return &containerConfig{
@@ -696,6 +667,112 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 	}, nil
 }
 
+// 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
+}
+
+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())
+	}
+
+	// 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
+
+	// 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
+}
+
+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")
+		}
+	}
+
+	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,
+		}
+	}
+	return epConfig, nil
+}
+
 func parsePortOpts(publishOpts []string) ([]string, error) {
 	optsList := []string{}
 	for _, publish := range publishOpts {

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()

cli/command/network/connect.go

@@ -2,6 +2,8 @@ package network
 
 import (
 	"context"
+	"fmt"
+	"strings"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
@@ -18,6 +20,7 @@ type connectOptions struct {
 	links        opts.ListOpts
 	aliases      []string
 	linklocalips []string
+	driverOpts   []string
 }
 
 func newConnectCommand(dockerCli command.Cli) *cobra.Command {
@@ -42,22 +45,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
+}

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.
@@ -95,6 +99,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 {

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)
+		})
 	}
 }