Fix issue for `--fixed-cidr` when bridge has multiple addresses #26659

yongtang · 2016-09-17T05:56:21Z

- What I did

This fix tries to address the issue raised in #26341 where multiple addresses in a bridge may cause --fixed-cidr to not have the correct addresses.

The issue is that netutils.ElectInterfaceAddresses(bridgeName) only returns the first IPv4 address.

- How I did it

This fix (together with the PR created in libnetwork moby/libnetwork#1452) changes ElectInterfaceAddresses() and addresses() so that all IPv4 addresses are returned. This will allow the possibility of selectively choose the address needed.

In daemon_unix.go, bridge address is chosen by comparing with the --fixed-cidr first, thus resolve the issue in #26341.

- How to verify it
This fix is tested manually, as is described in #26341:

brctl addbr cbr0
ip addr add 10.111.111.111/20 dev cbr0 label cbr0:main
ip addr add 10.222.222.222/12 dev cbr0 label cbr0:docker
ip link set cbr0 up
docker daemon --bridge=cbr0 --iptables=false --ip-masq=false --fixed-cidr=10.222.222.222/24
docker run --rm busybox ip route get 8.8.8.8 | grep -Po 'src.*'
src 10.222.222.0

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #26341.

This fix is related to libnetwork PR moby/libnetwork#1452

libnetwork vendoring:
Fixes #22204
Fixes #24637
Fixes #27157
Also, moby/libnetwork#1333, moby/libnetwork#1480

Signed-off-by: Yong Tang yong.tang.github@outlook.com

cpuguy83

ping @mavenugo

cpuguy83 · 2016-09-17T12:09:02Z

daemon/daemon_unix.go

+		if config.bridgeConfig.FixedCIDR != "" {
+			_, fCIDR, err := net.ParseCIDR(config.bridgeConfig.FixedCIDR)
+			if err != nil {
+				return err


Can you use errors.Wrap() here from github.com/pkg/errors? It's already vendored.

cpuguy83 · 2016-09-17T12:11:40Z

vendor/src/github.com/docker/libnetwork/drivers/bridge/interface.go

 // bridge interface.
-func (i *bridgeInterface) addresses() (netlink.Addr, []netlink.Addr, error) {
+func (i *bridgeInterface) addresses() (netlink.Addr, []netlink.Addr, []netlink.Addr, error) {


This is pretty unwieldy and I really have no idea what each return actually is without reading the comment above.

Seems like we can at least remove the first return and just call [0] to get the "first ipv4 address"

Thanks @cpuguy83 the PR has been updated.

cpuguy83 · 2016-09-17T12:15:06Z

vendor/src/github.com/docker/libnetwork/drivers/bridge/setup_ipv4.go

 	if err != nil {
 		return fmt.Errorf("failed to retrieve bridge interface addresses: %v", err)
 	}

+	// Iterate through all IPv4 addresses in case multiple IPv4 addresses exist
+	if config.AddressIPv4 != nil {


Seems like a pretty common pattern in the patch.

Thanks @cpuguy83. The PR has been updated with the logic wrapped into a new func.

yongtang · 2016-09-17T17:04:34Z

@cpuguy83 Thanks for the review. The PR has been updated. Please take a look and let me know if there are any other issues.

thaJeztah · 2016-10-21T17:30:00Z

ping @aboch @mrjana PTAL

aboch · 2016-10-21T18:03:27Z

docker changes look good to me
I already lgtmed the libnetwork changes

aboch · 2016-10-21T18:06:59Z

daemon/daemon_unix.go

+	}
+
+	nw := nwList[0]
+	if config.bridgeConfig.FixedCIDR != "" {


Minor comment, you may want to evaluate this only if len(nwList) > 1.
It will also make it clear why you are looking for an (at first) unrelated extra data (fixedCIDR) to make a better selection on the IPv4 network to pick.
Up to you if you want to make the change.

Thanks @aboch for the review. The PR has been updated with the changes.

This fix tries to address the issue raised in 26341 where multiple addresses in a bridge may cause `--fixed-cidr` to not have the correct addresses. The issue is that `netutils.ElectInterfaceAddresses(bridgeName)` only returns the first IPv4 address. This fix (together with the PR created in libnetwork ) changes `ElectInterfaceAddresses()` and `addresses()` so that all IPv4 addresses are returned. This will allow the possibility of selectively choose the address needed. In `daemon_unix.go`, bridge address is chosen by comparing with the `--fixed-cidr` first, thus resolve the issue in 26341. This fix is tested manually, as is described in 26341: ``` brctl addbr cbr0 ip addr add 10.111.111.111/20 dev cbr0 label cbr0:main ip addr add 10.222.222.222/12 dev cbr0 label cbr0:docker ip link set cbr0 up docker daemon --bridge=cbr0 --iptables=false --ip-masq=false --fixed-cidr=10.222.222.222/24 docker run --rm busybox ip route get 8.8.8.8 | grep -Po 'src.*' src 10.222.222.0 ``` This fix fixes 26341. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This fix updates libnetwork to f4338b6f1085ccfe5972e655cca8a1d15d73439d. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-10-27T17:32:09Z

@aboch The PR has been rebased and libnetwork has been vendored as well. Please take a look and let me know if there are any issues.

aboch · 2016-10-27T20:07:00Z

Thanks @yongtang I will update the issue's description to include which docker PRs the libnetwork vendoring will fix

aboch · 2016-10-27T20:14:13Z

Changes look good to me

thaJeztah

LGTM

crosbymichael · 2016-10-28T18:04:55Z

LGTM

rogaha · 2016-11-21T00:23:45Z

Thanks for working on it @crosbymichael! 👍

This was referenced Sep 17, 2016

Fix issue for --fixed-cidr when bridge has multiple addresses moby/libnetwork#1452

Merged

--fixed-cidr doesn't work correctly when the bridge device has multiple IP addresses #26341

Closed

cpuguy83 requested changes Sep 17, 2016

View reviewed changes

vdemeester added status/1-design-review area/networking labels Sep 17, 2016

yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from 75bff91 to 16882ea Compare September 17, 2016 16:43

thaJeztah added the status/needs-vendoring label Sep 20, 2016

aboch reviewed Oct 21, 2016

View reviewed changes

yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from 16882ea to 1afabb1 Compare October 21, 2016 21:27

thaJeztah added status/2-code-review and removed status/1-design-review labels Oct 22, 2016

yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from 1afabb1 to e0e9e5b Compare October 26, 2016 20:42

yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from e0e9e5b to 4fb3836 Compare October 27, 2016 03:13

Vendor libnetwork to f4338b6f1085ccfe5972e655cca8a1d15d73439d

fc62ad6

This fix updates libnetwork to f4338b6f1085ccfe5972e655cca8a1d15d73439d. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from 4fb3836 to fc62ad6 Compare October 27, 2016 16:13

thaJeztah removed the status/needs-vendoring label Oct 27, 2016

thaJeztah added this to the 1.13.0 milestone Oct 27, 2016

thaJeztah approved these changes Oct 27, 2016

View reviewed changes

crosbymichael merged commit 1e989ab into moby:master Oct 28, 2016

yongtang deleted the 26341-fixed-cidr-multiple-addresses-bridge branch October 28, 2016 18:07

thaJeztah added the impact/changelog label Oct 28, 2016

thaJeztah mentioned this pull request Nov 25, 2016

Containers can't connect to each other over overlay network - swarm 1.12 #28799

Closed

neilsmind mentioned this pull request Dec 7, 2016

Kong don't work on Docker Cloud Kong/kong#1836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue for `--fixed-cidr` when bridge has multiple addresses #26659

Fix issue for `--fixed-cidr` when bridge has multiple addresses #26659

yongtang commented Sep 17, 2016 •

edited by aboch

Loading

cpuguy83 left a comment

cpuguy83 Sep 17, 2016

yongtang Sep 17, 2016

cpuguy83 Sep 17, 2016

yongtang Sep 17, 2016

cpuguy83 Sep 17, 2016

yongtang Sep 17, 2016

yongtang commented Sep 17, 2016

thaJeztah commented Oct 21, 2016

aboch commented Oct 21, 2016

aboch Oct 21, 2016

yongtang Oct 21, 2016

yongtang commented Oct 27, 2016

aboch commented Oct 27, 2016

aboch commented Oct 27, 2016

thaJeztah left a comment

crosbymichael commented Oct 28, 2016

rogaha commented Nov 21, 2016

Fix issue for --fixed-cidr when bridge has multiple addresses #26659

Fix issue for --fixed-cidr when bridge has multiple addresses #26659

Conversation

yongtang commented Sep 17, 2016 • edited by aboch Loading

cpuguy83 left a 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

yongtang commented Sep 17, 2016

thaJeztah commented Oct 21, 2016

aboch commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Oct 27, 2016

aboch commented Oct 27, 2016

aboch commented Oct 27, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

crosbymichael commented Oct 28, 2016

rogaha commented Nov 21, 2016

Fix issue for `--fixed-cidr` when bridge has multiple addresses #26659

Fix issue for `--fixed-cidr` when bridge has multiple addresses #26659

yongtang commented Sep 17, 2016 •

edited by aboch

Loading