Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add missing opts to --network advanced syntax #4419

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Add missing opts to --network advanced syntax #4419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58f4260
Rename expectedCfg into expectedHostCfg
akerouanton Sep 10, 2023
9e1b42e
Add missing opts to --network advanced syntax
akerouanton Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"))
Expand All @@ -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"))
}
Comment on lines +796 to +798
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't checked out the code locally yet, but I wonder now if these errors are always correct; this error is legit if I would do the following;

This is a conflict, because I'm setting the mac-address TWICE for the default (bridge) network;

docker run \
    --mac-address=foo \
    --network name=bridge,mac-address=bar \
   myimage

This is NOT a conflict, because I'm setting the mac-address twice, but for different networks; for bridge, I set it to foo and for mynetwork, I set it to bar;

docker run \
    --mac-address=foo \
    --network bridge \
    --network name=mynetwork,mac-address=bar \
   myimage

This one is more tricky though, as this one could be seen in 2 different ways;

  • an implicit --network=bridge with a mac-address specified for the default (bridge) network
  • a conflict (because I specified the network to be mynetwork, NOT bridge, and specified the mac-address twice for that network);
docker run \
    --mac-address=foo \
    --network name=mynetwork,mac-address=bar \
   myimage

(Taking mac-address as example, but the same applies to other options that also have a separate --flag)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original idea was to make sure no potentially ambiguous notations could be used. I think it will avoid some mistakes, so it's probably better to stick with that UX.

As it's mostly unrelated to my PR, could you open an issue if you think it's worth revisiting?

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())
Expand All @@ -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())
Expand Down Expand Up @@ -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
}

Expand Down
86 changes: 62 additions & 24 deletions cli/command/container/opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,23 +507,24 @@ 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 string
flags []string
expected map[string]*networktypes.EndpointSettings
expectedCfg container.Config
expectedHostCfg container.HostConfig
expectedErr string
}{
{
name: "single-network-legacy",
flags: []string{"--network", "net1"},
expected: map[string]*networktypes.EndpointSettings{},
expectedCfg: container.HostConfig{NetworkMode: "net1"},
name: "single-network-legacy",
flags: []string{"--network", "net1"},
expected: map[string]*networktypes.EndpointSettings{},
expectedHostCfg: container.HostConfig{NetworkMode: "net1"},
},
{
name: "single-network-advanced",
flags: []string{"--network", "name=net1"},
expected: map[string]*networktypes.EndpointSettings{},
expectedCfg: container.HostConfig{NetworkMode: "net1"},
name: "single-network-advanced",
flags: []string{"--network", "name=net1"},
expected: map[string]*networktypes.EndpointSettings{},
expectedHostCfg: container.HostConfig{NetworkMode: "net1"},
},
{
name: "single-network-legacy-with-options",
Expand All @@ -549,7 +550,7 @@ func TestParseNetworkConfig(t *testing.T) {
Aliases: []string{"web1", "web2"},
},
},
expectedCfg: container.HostConfig{NetworkMode: "net1"},
expectedHostCfg: container.HostConfig{NetworkMode: "net1"},
},
{
name: "multiple-network-advanced-mixed",
Expand All @@ -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": {
Expand All @@ -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"},
},
},
},
expectedCfg: container.HostConfig{NetworkMode: "net1"},
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{
Expand All @@ -602,16 +610,30 @@ 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.HostConfig{NetworkMode: "net1"},
expectedCfg: container.Config{MacAddress: "02:32:1c:23:00:04"},
expectedHostCfg: 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: "multiple-networks",
flags: []string{"--network", "net1", "--network", "name=net2"},
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",
Expand All @@ -638,19 +660,35 @@ 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)
return
}

assert.NilError(t, err)
assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedCfg.NetworkMode)
assert.DeepEqual(t, config.MacAddress, tc.expectedCfg.MacAddress)
assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedHostCfg.NetworkMode)
assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected)
})
}
Expand Down
11 changes: 9 additions & 2 deletions opts/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const (
networkOptAlias = "alias"
networkOptIPv4Address = "ip"
networkOptIPv6Address = "ip6"
networkOptMacAddress = "mac-address"
networkOptLinkLocalIP = "link-local-ip"
driverOpt = "driver-opt"
)

Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
20 changes: 20 additions & 0 deletions opts/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading