Support for com.docker.network.container_iface_prefix driver label #1667

wnagele · 2017-02-27T22:01:28Z

This adds support for a custom interface prefix. Can be used to address issues such as moby/moby#20067. If the label is not used the default behaviour prevails.

aboch · 2017-02-27T23:58:26Z

osl/interface_linux.go

@@ -241,8 +241,9 @@ func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...If
 	if n.isDefault {
 		i.dstName = i.srcName
 	} else {
-		i.dstName = fmt.Sprintf("%s%d", i.dstName, n.nextIfIndex)
-		n.nextIfIndex++
+		var dstPrefix = i.dstName


You do not need to declare the dstPrefix variable. Just reuse the dstPrefix from the function input parameters.

Fixed - using dstNamePrefix now. dstPrefix is for the name inside the host - thus using it here is wrong and confusing.

aboch · 2017-02-27T23:59:55Z

drivers/bridge/bridge.go

+	Mtu                      int
+	DefaultBindingIP         net.IP
+	DefaultBridge            bool
+	ContainerInterfacePrefix string


As this is internal, please rename to ContainerIfacePrefix so that we can reduce this change's footprint.

aboch · 2017-02-28T00:00:06Z

drivers/bridge/bridge.go

@@ -1217,7 +1220,11 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,
 	}

 	iNames := jinfo.InterfaceName()
-	err = iNames.SetNames(endpoint.srcName, containerVethPrefix)


For readability, I would rename the constant to defaultContainerVethPrefix and here have

containerVethPrefix := defaultContainerVethPrefix if network.config.ContainerIfacePrefix != "" { containerVethPrefix = network.config.ContainerInterfacePrefix } err = iNames.SetNames(endpoint.srcName, containerVethPrefix)

aboch · 2017-02-28T00:01:15Z

osl/namespace_linux.go

+	gwv6          net.IP
+	staticRoutes  []*types.StaticRoute
+	neighbors     []*neigh
+	nextIfIndexes map[string]int


Again to reduce the fix footprint, I would leave the variable name as is, even if it now is a map. At the end it is one index per interface.

wnagele · 2017-02-28T12:11:20Z

All comments addressed now.

aboch · 2017-02-28T16:35:22Z

osl/interface_linux.go

@@ -241,8 +241,9 @@ func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...If
 	if n.isDefault {
 		i.dstName = i.srcName
 	} else {
-		i.dstName = fmt.Sprintf("%s%d", i.dstName, n.nextIfIndex)
-		n.nextIfIndex++
+		dstNamePrefix := i.dstName


please remove this line
and modify next one to

i.dstName = fmt.Sprintf("%s%d", dstPrefix, n.nextIfIndex[dstPrefix])

dstPrefix is the prefix of the interface in the host system - I am trying to modify the one in the container. If I override this it will change the logic in the code following. I do not think this is correct - please clarify.

The only reason I assign dstName to a new dstNamePrefix is that the original data of dstName is to be modified and dstNamePrefix is needed for the increment.

i.srcName is the original name of the veth pipe end which is going to be moved into the container's netns.
i.dstName is the new name the above interface will be renamed to, once it is moved.

dstPrefix is the prefix of the interface in the host system

No, dstPrefix the network driver chosen prefix for the new interface name once we move it into the netns.
func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...IfaceOption) error {

At this point of the code i.dstName == dstPrefix (L229 i := &nwIface{srcName: srcName, dstName: dstPrefix, ns: n})
IMO, original code was already confusing and should have been written as
i.dstName = fmt.Sprintf("%s%d", dstPrefix, n.nextIfIndex)

You are of course correct. Should have read up a few lines in the code. Have made this modification now as well.

aboch · 2017-02-28T20:16:42Z

drivers/bridge/labels.go

@@ -15,4 +15,7 @@ const (

 	// DefaultBridge label
 	DefaultBridge = "com.docker.network.bridge.default_bridge"
+
+	// ContainerIfacePrefix label
+	ContainerIfacePrefix = "com.docker.network.bridge.container_iface_prefix"


Looking at this again, it makes me think people may want the same option while using different network drivers in future.

In a way it s not specific to a network driver (like the MTU option we have now).

So I was thinking it may be better we make this option driver agnostic from the beginning, and define it in https://github.com/docker/libnetwork/blob/master/netlabel/labels.go

as

ContainerIfacePrefix = "com.docker.network.container_iface_prefix

Also wondering whether we should keep a 1-to-1 match to the networking objects (here would be sandbox_iface_PREFIX) or stick to container given it is a user input option. It is probably better to leave it as container_iface_prefix, more clear.

That is now done as well.

aboch · 2017-02-28T20:17:38Z

Thanks @wnagele , I only have one last comment about making the new option driver agnostic.

Signed-off-by: Wolfgang Nagele <[email protected]>

aboch · 2017-03-01T04:00:37Z

Thanks @wnagele

LGTM

wnagele · 2017-03-01T09:22:24Z

Merge? :)

aboch · 2017-03-01T17:30:15Z

@wnagele
For most of the PRs we usually require two mantainers' LGTMs before merging.

wnagele · 2017-03-12T15:08:40Z

Anybody else able to review this for a merge? @sanimej @mavenugo

mavenugo · 2017-03-12T20:52:08Z

osl/interface_linux.go

@@ -241,8 +241,8 @@ func (n *networkNamespace) AddInterface(srcName, dstPrefix string, options ...If
 	if n.isDefault {
 		i.dstName = i.srcName
 	} else {
-		i.dstName = fmt.Sprintf("%s%d", i.dstName, n.nextIfIndex)
-		n.nextIfIndex++
+		i.dstName = fmt.Sprintf("%s%d", dstPrefix, n.nextIfIndex[dstPrefix])


j/c. are you relying on the fact that the map access on new prefix will return 0 ?

Correct - basically just moved the original method towards using a map to track all prefixes independently.

mavenugo

LGTM

It would have been better to have changed other drivers as well. But we can deal with it in individual PRs.

thaJeztah · 2021-05-31T13:25:38Z

Updated the title to include container_iface_prefix instead of container_interface_prefix (see docker/cli#2984)

GordonTheTurtle added the dco/no label Feb 27, 2017

wnagele mentioned this pull request Feb 27, 2017

docker network namespace ethernet interfaces race condition moby/moby#20067

Open

GordonTheTurtle removed the dco/no label Feb 27, 2017

aboch reviewed Feb 27, 2017

View reviewed changes

aboch reviewed Feb 28, 2017

View reviewed changes

Support for com.docker.network.bridge.container_interface_prefix label

40bae11

Signed-off-by: Wolfgang Nagele <[email protected]>

mavenugo reviewed Mar 12, 2017

View reviewed changes

mavenugo approved these changes Mar 13, 2017

View reviewed changes

mavenugo merged commit af59385 into moby:master Mar 13, 2017

This was referenced Mar 14, 2017

Allow user to replace ingress network moby/moby#31714

Merged

bump 17.04.0-rc1 moby/moby#31811

Merged

aboch changed the title ~~Support for com.docker.network.bridge.container_interface_prefix label~~ Support for com.docker.network.container_interface_prefix driver label Mar 14, 2017

thaJeztah mentioned this pull request Sep 18, 2019

Add Support for com.docker.network.container_iface_prefix label docker/cli#2092

Closed

thaJeztah changed the title ~~Support for com.docker.network.container_interface_prefix driver label~~ Support for com.docker.network.container_iface_prefix driver label May 31, 2021

This was referenced May 31, 2021

docs: Fix wrong bridge driver option docker/cli#2984

Merged

changelog: fix incorrectly named libnetwork label moby/moby#42446

Merged

changelog: fix incorrectly named libnetwork label docker/docs#12921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for com.docker.network.container_iface_prefix driver label #1667

Support for com.docker.network.container_iface_prefix driver label #1667

wnagele commented Feb 27, 2017

aboch Feb 27, 2017

wnagele Feb 28, 2017

aboch Feb 27, 2017

wnagele Feb 28, 2017

aboch Feb 28, 2017

wnagele Feb 28, 2017

aboch Feb 28, 2017

wnagele Feb 28, 2017

wnagele commented Feb 28, 2017

aboch Feb 28, 2017

wnagele Feb 28, 2017

aboch Feb 28, 2017

wnagele Feb 28, 2017

aboch Feb 28, 2017

wnagele Mar 1, 2017

aboch commented Feb 28, 2017

aboch commented Mar 1, 2017

wnagele commented Mar 1, 2017

aboch commented Mar 1, 2017

wnagele commented Mar 12, 2017

mavenugo Mar 12, 2017

wnagele Mar 12, 2017

mavenugo left a comment

thaJeztah commented May 31, 2021

Support for com.docker.network.container_iface_prefix driver label #1667

Support for com.docker.network.container_iface_prefix driver label #1667

Conversation

wnagele commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wnagele commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Feb 28, 2017

aboch commented Mar 1, 2017

wnagele commented Mar 1, 2017

aboch commented Mar 1, 2017

wnagele commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo left a comment

Choose a reason for hiding this comment

thaJeztah commented May 31, 2021