diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 6b1018508101..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" @@ -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,46 +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 + ) - if hostConfig.NetworkMode.IsUserDefined() && len(hostConfig.Links) > 0 { - epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] - if epConfig == nil { - epConfig = &networktypes.EndpointSettings{} + 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)) } - epConfig.Links = make([]string, len(hostConfig.Links)) - copy(epConfig.Links, hostConfig.Links) - networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] = epConfig + 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 { - epConfig := networkingConfig.EndpointsConfig[string(hostConfig.NetworkMode)] - if epConfig == nil { - epConfig = &networktypes.EndpointSettings{} + 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.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 + 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) { 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 7ff055aaba88..04108e20fadb 100644 --- a/cli/command/network/connect.go +++ b/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 +} diff --git a/opts/network.go b/opts/network.go index ec4967ff320d..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. @@ -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 { 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) + }) } }