From 9e1b42e642dac97de474f20377858db6a18dc766 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Thu, 13 Jul 2023 11:50:01 +0200 Subject: [PATCH] Add missing opts to --network advanced syntax The new advanced --network syntax introduced in docker/cli#1767 is lacking support for `link-local-ip` and `mac-address` fields. This commit adds both. Signed-off-by: Albin Kerouanton --- cli/command/container/opts.go | 26 +++++++++++++++--- cli/command/container/opts_test.go | 44 ++++++++++++++++++++++++++++-- opts/network.go | 11 ++++++-- opts/network_test.go | 20 ++++++++++++++ 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index cd5d69f7e7..7b338c25ce 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -712,6 +712,12 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con return nil, err } + // Put the endpoint-specific MacAddress of the "main" network attachment into the container Config for backward + // compatibility with older daemons. + if nw, ok := networkingConfig.EndpointsConfig[hostConfig.NetworkMode.NetworkName()]; ok { + config.MacAddress = nw.MacAddress + } + return &containerConfig{ Config: config, HostConfig: hostConfig, @@ -773,8 +779,7 @@ func parseNetworkOpts(copts *containerOptions) (map[string]*networktypes.Endpoin 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) +func applyContainerOptions(n *opts.NetworkAttachmentOpts, copts *containerOptions) error { //nolint:gocyclo // 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")) @@ -788,6 +793,12 @@ func applyContainerOptions(n *opts.NetworkAttachmentOpts, copts *containerOption if n.IPv6Address != "" && copts.ipv6Address != "" { return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --ip6 and per-network IPv6 address")) } + if n.MacAddress != "" && copts.macAddress != "" { + return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --mac-address and per-network MAC address")) + } + if len(n.LinkLocalIPs) > 0 && copts.linkLocalIPs.Len() > 0 { + return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --link-local-ip and per-network link-local IP addresses")) + } if copts.aliases.Len() > 0 { n.Aliases = make([]string, copts.aliases.Len()) copy(n.Aliases, copts.aliases.GetAll()) @@ -802,8 +813,9 @@ func applyContainerOptions(n *opts.NetworkAttachmentOpts, copts *containerOption if copts.ipv6Address != "" { 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.macAddress != "" { + n.MacAddress = copts.macAddress + } if copts.linkLocalIPs.Len() > 0 { n.LinkLocalIPs = make([]string, copts.linkLocalIPs.Len()) copy(n.LinkLocalIPs, copts.linkLocalIPs.GetAll()) @@ -840,6 +852,12 @@ func parseNetworkAttachmentOpt(ep opts.NetworkAttachmentOpts) (*networktypes.End LinkLocalIPs: ep.LinkLocalIPs, } } + if ep.MacAddress != "" { + if _, err := opts.ValidateMACAddress(ep.MacAddress); err != nil { + return nil, errors.Errorf("%s is not a valid mac address", ep.MacAddress) + } + epConfig.MacAddress = ep.MacAddress + } return epConfig, nil } diff --git a/cli/command/container/opts_test.go b/cli/command/container/opts_test.go index ceef26cb57..2fccddf083 100644 --- a/cli/command/container/opts_test.go +++ b/cli/command/container/opts_test.go @@ -510,6 +510,7 @@ func TestParseNetworkConfig(t *testing.T) { name string flags []string expected map[string]*networktypes.EndpointSettings + expectedCfg container.Config expectedHostCfg container.HostConfig expectedErr string }{ @@ -565,6 +566,7 @@ func TestParseNetworkConfig(t *testing.T) { "--network-alias", "web2", "--network", "net2", "--network", "name=net3,alias=web3,driver-opt=field3=value3,ip=172.20.88.22,ip6=2001:db8::8822", + "--network", "name=net4,mac-address=02:32:1c:23:00:04,link-local-ip=169.254.169.254", }, expected: map[string]*networktypes.EndpointSettings{ "net1": { @@ -586,12 +588,18 @@ func TestParseNetworkConfig(t *testing.T) { }, Aliases: []string{"web3"}, }, + "net4": { + MacAddress: "02:32:1c:23:00:04", + IPAMConfig: &networktypes.EndpointIPAMConfig{ + LinkLocalIPs: []string{"169.254.169.254"}, + }, + }, }, expectedHostCfg: 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,ip=172.20.88.22,ip6=2001:db8::8822"}, + flags: []string{"--network", "name=net1,alias=web1,alias=web2,driver-opt=field1=value1,driver-opt=field2=value2,ip=172.20.88.22,ip6=2001:db8::8822,mac-address=02:32:1c:23:00:04"}, expected: map[string]*networktypes.EndpointSettings{ "net1": { DriverOpts: map[string]string{ @@ -602,9 +610,11 @@ func TestParseNetworkConfig(t *testing.T) { IPv4Address: "172.20.88.22", IPv6Address: "2001:db8::8822", }, - Aliases: []string{"web1", "web2"}, + Aliases: []string{"web1", "web2"}, + MacAddress: "02:32:1c:23:00:04", }, }, + expectedCfg: container.Config{MacAddress: "02:32:1c:23:00:04"}, expectedHostCfg: container.HostConfig{NetworkMode: "net1"}, }, { @@ -613,6 +623,18 @@ func TestParseNetworkConfig(t *testing.T) { expected: map[string]*networktypes.EndpointSettings{"net1": {}, "net2": {}}, expectedHostCfg: container.HostConfig{NetworkMode: "net1"}, }, + { + name: "advanced-options-with-standalone-mac-address-flag", + flags: []string{"--network=name=net1,alias=foobar", "--mac-address", "52:0f:f3:dc:50:10"}, + expected: map[string]*networktypes.EndpointSettings{ + "net1": { + Aliases: []string{"foobar"}, + MacAddress: "52:0f:f3:dc:50:10", + }, + }, + expectedCfg: container.Config{MacAddress: "52:0f:f3:dc:50:10"}, + expectedHostCfg: container.HostConfig{NetworkMode: "net1"}, + }, { name: "conflict-network", flags: []string{"--network", "duplicate", "--network", "name=duplicate"}, @@ -638,11 +660,26 @@ func TestParseNetworkConfig(t *testing.T) { flags: []string{"--network", "name=host", "--network", "net1"}, expectedErr: `conflicting options: cannot attach both user-defined and non-user-defined network-modes`, }, + { + name: "conflict-options-link-local-ip", + flags: []string{"--network", "name=net1,link-local-ip=169.254.169.254", "--link-local-ip", "169.254.10.8"}, + expectedErr: `conflicting options: cannot specify both --link-local-ip and per-network link-local IP addresses`, + }, + { + name: "conflict-options-mac-address", + flags: []string{"--network", "name=net1,mac-address=02:32:1c:23:00:04", "--mac-address", "02:32:1c:23:00:04"}, + expectedErr: `conflicting options: cannot specify both --mac-address and per-network MAC address`, + }, + { + name: "invalid-mac-address", + flags: []string{"--network", "name=net1,mac-address=foobar"}, + expectedErr: "foobar is not a valid mac address", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - _, hConfig, nwConfig, err := parseRun(tc.flags) + config, hConfig, nwConfig, err := parseRun(tc.flags) if tc.expectedErr != "" { assert.Error(t, err, tc.expectedErr) @@ -650,6 +687,7 @@ func TestParseNetworkConfig(t *testing.T) { } assert.NilError(t, err) + assert.DeepEqual(t, config.MacAddress, tc.expectedCfg.MacAddress) assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedHostCfg.NetworkMode) assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected) }) diff --git a/opts/network.go b/opts/network.go index 12c3977b1b..e36ef405d1 100644 --- a/opts/network.go +++ b/opts/network.go @@ -12,6 +12,8 @@ const ( networkOptAlias = "alias" networkOptIPv4Address = "ip" networkOptIPv6Address = "ip6" + networkOptMacAddress = "mac-address" + networkOptLinkLocalIP = "link-local-ip" driverOpt = "driver-opt" ) @@ -23,7 +25,8 @@ type NetworkAttachmentOpts struct { Links []string // TODO add support for links in the csv notation of `--network` IPv4Address string IPv6Address string - LinkLocalIPs []string // TODO add support for LinkLocalIPs in the csv notation of `--network` ? + LinkLocalIPs []string + MacAddress string } // NetworkOpt represents a network config in swarm mode. @@ -32,7 +35,7 @@ type NetworkOpt struct { } // Set networkopts value -func (n *NetworkOpt) Set(value string) error { +func (n *NetworkOpt) Set(value string) error { //nolint:gocyclo longSyntax, err := regexp.MatchString(`\w+=\w+(,\w+=\w+)*`, value) if err != nil { return err @@ -66,6 +69,10 @@ func (n *NetworkOpt) Set(value string) error { netOpt.IPv4Address = val case networkOptIPv6Address: netOpt.IPv6Address = val + case networkOptMacAddress: + netOpt.MacAddress = val + case networkOptLinkLocalIP: + netOpt.LinkLocalIPs = append(netOpt.LinkLocalIPs, val) case driverOpt: key, val, err = parseDriverOpt(val) if err != nil { diff --git a/opts/network_test.go b/opts/network_test.go index ca595927e5..8a5b66fb11 100644 --- a/opts/network_test.go +++ b/opts/network_test.go @@ -78,6 +78,26 @@ func TestNetworkOptAdvancedSyntax(t *testing.T) { }, }, }, + { + value: "name=docknet1,mac-address=52:0f:f3:dc:50:10", + expected: []NetworkAttachmentOpts{ + { + Target: "docknet1", + Aliases: []string{}, + MacAddress: "52:0f:f3:dc:50:10", + }, + }, + }, + { + value: "name=docknet1,link-local-ip=169.254.169.254,link-local-ip=169.254.10.10", + expected: []NetworkAttachmentOpts{ + { + Target: "docknet1", + Aliases: []string{}, + LinkLocalIPs: []string{"169.254.169.254", "169.254.10.10"}, + }, + }, + }, } for _, tc := range testCases { tc := tc