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.

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

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

Merged

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Sep 17, 2016

This fix tries to address the issue raised in:
moby/moby#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 changes ElectInterfaceAddresses() and addresses() so that all IPv4 addresses are returned. This will allow the possibility of selectively choose the address needed.

This fix is related to docker PR:
moby/moby#26659

Signed-off-by: Yong Tang [email protected]

Copy link
Contributor

@aboch aboch left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @yongtang. I have a couple of comments.

@@ -62,15 +62,15 @@ func GenerateIfaceName(nlh *netlink.Handle, prefix string, len int) (string, err
}

// ElectInterfaceAddresses looks for an interface on the OS with the
// specified name and returns its IPv4 and IPv6 addresses in CIDR
// specified name and returns its first IPv4 address, all IPv4 addresses,
Copy link
Contributor

@aboch aboch Sep 17, 2016

Choose a reason for hiding this comment

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

I think it is better that this function "[...] returns all its IPv4 and IPv6 addresses in CIDR notation. If a failure in retrieving the addresses or no IPv4 address is found, an error is returned."
This will allow us to drop the first *net.IPNet in the return list, which is superflous given we now are adding the v4 list.
Also, erroring out if no v4 is found (and clearly spelling it out in the API comments), will maintain current logic and facilitate caller's job, which would be able to access the v4List[0] element regardless if the funciton succeeded.

v4List, v6List, err := ElectInterfaceAdresses(...)
if err != nil {
    return err
}
nw4 := v4List[0]
If CIDR selector specified {
    for _, nw := range v4List {
        if selector.Contains(nw.IP) {
            nw4 = nw
            break
        }
    }
}

I think @cpuguy83 also suggested something similar while reviewing your docker/docker PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aboch. The pull request has been updated.

@@ -52,27 +52,27 @@ func (i *bridgeInterface) exists() bool {
return i.Link != nil
}

// addresses returns a single IPv4 address and all IPv6 addresses for the
// addresses returns the first IPv4 address, all IPv4 addresses, and all IPv6 addresses for the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend the same for this bridge function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@yongtang yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch 2 times, most recently from afe6d25 to 6e52938 Compare September 17, 2016 16:52
@yongtang
Copy link
Member Author

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

@yongtang yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch 2 times, most recently from 293cc52 to 36510bd Compare September 17, 2016 19:41
@aboch
Copy link
Contributor

aboch commented Sep 19, 2016

LGTM

@thaJeztah
Copy link
Member

ping @mavenugo @mrjana ptal

@aboch
Copy link
Contributor

aboch commented Oct 21, 2016

@yongtang Please rebase

This fix tries to address the issue raised in:
moby/moby#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 changes `ElectInterfaceAddresses()` and `addresses()`
so that all IPv4 addresses are returned. This will allow the
possibility of selectively choose the address needed.

Signed-off-by: Yong Tang <[email protected]>
@yongtang yongtang force-pushed the 26341-fixed-cidr-multiple-addresses-bridge branch from 36510bd to e02f9e9 Compare October 21, 2016 20:59
@yongtang
Copy link
Member Author

Thanks @aboch. The PR has been rebased and updated.

@aboch aboch merged commit f4338b6 into moby:master Oct 26, 2016
@yongtang yongtang deleted the 26341-fixed-cidr-multiple-addresses-bridge branch November 2, 2016 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants