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

Added subnet IP address parsing for advertise addresses (eg. 10.1.0.0… #223

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
9 participants
@apognu
Contributor

apognu commented Oct 6, 2015

This was mentionned in #186

This pull request adds the possibility of specifying advertise addresses as a subnet in CIDR form. We will iterate through the host's interfaces to find a configured IP address within that subnet and use it for advertising.

I used the form 10.0.0.0/8:4648 for the specification, not sure if that's the more readable, comments could be welcome.

Also, I assume the port is given in the advertise address, I don't know if the server config spec allows to specify the advertise address without a port (where we would use the port from the ports section), I could easily add that feature if necessary.

Regards,
Antoine.

@apognu

This comment has been minimized.

Show comment
Hide comment
@apognu

apognu Oct 6, 2015

Contributor

I should probably write some tests, will amend PR.

Contributor

apognu commented Oct 6, 2015

I should probably write some tests, will amend PR.

@cbednarski

This comment has been minimized.

Show comment
Hide comment
@cbednarski

cbednarski Oct 6, 2015

Contributor

Tests would definitely be appreciated :)

Contributor

cbednarski commented Oct 6, 2015

Tests would definitely be appreciated :)

@apognu

This comment has been minimized.

Show comment
Hide comment
@apognu

apognu Oct 7, 2015

Contributor

Is what's in apognu/nomad@9715f26 enough?

Contributor

apognu commented Oct 7, 2015

Is what's in apognu/nomad@9715f26 enough?

@cbednarski

This comment has been minimized.

Show comment
Hide comment
@cbednarski

cbednarski Oct 7, 2015

Contributor

@apognu Whoops, I missed the test commit somehow. The code and tests look great!

/cc @ryanuber

Contributor

cbednarski commented Oct 7, 2015

@apognu Whoops, I missed the test commit somehow. The code and tests look great!

/cc @ryanuber

Show outdated Hide outdated helper/inet/inet.go
const (
address = iota
cidr
)

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

These constants make it much harder to read the code that uses them. We should just use the explicit index in the slices directly for clarity.

@ryanuber

ryanuber Oct 7, 2015

Member

These constants make it much harder to read the code that uses them. We should just use the explicit index in the slices directly for clarity.

Show outdated Hide outdated helper/inet/inet.go
// Split subnet CIDR IP from the port
advertiseTokens := strings.Split(ip, ":")
if len(advertiseTokens) != 2 {
return "", errors.New(fmt.Sprintf("port was not given in advertise subnet: `%s`", ip))

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

This is going to force the end user to use CIDR notation for the advertise address because of where this function is called and how the error is handled. This needs to be optional so that specifying an exact IP is still supported.

@ryanuber

ryanuber Oct 7, 2015

Member

This is going to force the end user to use CIDR notation for the advertise address because of where this function is called and how the error is handled. This needs to be optional so that specifying an exact IP is still supported.

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

I missed the check directly above this block, so this actually still works. Sorry for the confusion!

@ryanuber

ryanuber Oct 7, 2015

Member

I missed the check directly above this block, so this actually still works. Sorry for the confusion!

Show outdated Hide outdated helper/inet/inet.go
// Parse subnet IP and netmask
subnetIp := net.ParseIP(ipTokens[address])
if subnetIp == nil {
return "", errors.New(fmt.Sprintf("advertise subnet address out of range: `%s`", ipTokens[address]))

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

All of these should use fmt.Errorf. We then don't even need to import errors.

@ryanuber

ryanuber Oct 7, 2015

Member

All of these should use fmt.Errorf. We then don't even need to import errors.

Show outdated Hide outdated helper/inet/inet.go
if subnetIp == nil {
return "", errors.New(fmt.Sprintf("advertise subnet address out of range: `%s`", ipTokens[address]))
}
subnetCidr, err := strconv.ParseInt(ipTokens[cidr], 10, 32)

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

I'd just use strconv.Atoi() here. Not sure why a bitsize of 32 would be important if the max value is the integer 32.

@ryanuber

ryanuber Oct 7, 2015

Member

I'd just use strconv.Atoi() here. Not sure why a bitsize of 32 would be important if the max value is the integer 32.

Show outdated Hide outdated helper/inet/inet.go
return "", errors.New(fmt.Sprintf("cannot parse advertise subnet CIDR: `%s`", ipTokens[cidr]))
}
subnet := net.IPNet{IP: subnetIp, Mask: net.CIDRMask(int(subnetCidr), 32)}

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

So instead of doing all the work ourselves of splitting the CIDR string and parsing the parts, can we just use net.ParseCIDR()? It returns a net.IP and a *net.IPNet, which I think should get us the data we need.

@ryanuber

ryanuber Oct 7, 2015

Member

So instead of doing all the work ourselves of splitting the CIDR string and parsing the parts, can we just use net.ParseCIDR()? It returns a net.IP and a *net.IPNet, which I think should get us the data we need.

Show outdated Hide outdated helper/inet/inet.go
ifaces, err := net.Interfaces()
if err != nil {
return "", errors.New("could not retrieve interface list")

This comment has been minimized.

@ryanuber

ryanuber Oct 7, 2015

Member

Use fmt.Sprintf() and show the error generated

@ryanuber

ryanuber Oct 7, 2015

Member

Use fmt.Sprintf() and show the error generated

@ryanuber

This comment has been minimized.

Show comment
Hide comment
@ryanuber

ryanuber Oct 7, 2015

Member

@apognu left some comments, but looking good!

Member

ryanuber commented Oct 7, 2015

@apognu left some comments, but looking good!

@apognu

This comment has been minimized.

Show comment
Hide comment
@apognu

apognu Oct 7, 2015

Contributor

Thanks for your review, will fix tomorrow.

I don't get the first comment though. The end-user is able to specify both "legacy" form (10.1.1.1:4648) and subnet form (10.1.1.0/24:4648), the function is transparent and returns the IP:PORT untouched if there's not CIDR notation in it. Maybe I didn't get the comment right.

Contributor

apognu commented Oct 7, 2015

Thanks for your review, will fix tomorrow.

I don't get the first comment though. The end-user is able to specify both "legacy" form (10.1.1.1:4648) and subnet form (10.1.1.0/24:4648), the function is transparent and returns the IP:PORT untouched if there's not CIDR notation in it. Maybe I didn't get the comment right.

@ryanuber

This comment has been minimized.

Show comment
Hide comment
@ryanuber

ryanuber Oct 7, 2015

Member

You are right - I just missed the fast-path check at the top somehow. Thanks!

Member

ryanuber commented Oct 7, 2015

You are right - I just missed the fast-path check at the top somehow. Thanks!

@apognu

This comment has been minimized.

Show comment
Hide comment
@apognu

apognu Oct 8, 2015

Contributor

@ryanuber : this should answer all your comments.

Contributor

apognu commented Oct 8, 2015

@ryanuber : this should answer all your comments.

// Split subnet CIDR IP from the port
tokens := strings.Split(ip, ":")
if len(tokens) != 2 {
return "", fmt.Errorf("port was not given in advertise subnet: `%s`", ip)

This comment has been minimized.

@dadgar

dadgar Oct 12, 2015

Contributor

If you want to quote these maybe use %q; here and below

@dadgar

dadgar Oct 12, 2015

Contributor

If you want to quote these maybe use %q; here and below

Show outdated Hide outdated helper/inet/inet_test.go
func TestAdvertiseFromSubnet(t *testing.T) {
// CIDR is out of range
if ad, err := AdvertiseIpFromSubnet("127.0.0.0/43:4648"); err == nil {

This comment has been minimized.

@dadgar

dadgar Oct 12, 2015

Contributor

Each of these should be its own test.

@dadgar

dadgar Oct 12, 2015

Contributor

Each of these should be its own test.

This comment has been minimized.

@apognu

apognu Oct 12, 2015

Contributor

I don't really get that comment. Care to elaborate?

@apognu

apognu Oct 12, 2015

Contributor

I don't really get that comment. Care to elaborate?

This comment has been minimized.

@dadgar

dadgar Oct 12, 2015

Contributor

You are testing different invalid and valid cases. Each of the cases should be in its own test function.

@dadgar

dadgar Oct 12, 2015

Contributor

You are testing different invalid and valid cases. Each of the cases should be in its own test function.

@dadgar

This comment has been minimized.

Show comment
Hide comment
@dadgar

dadgar Oct 12, 2015

Contributor

Looks good! Left small comments

Contributor

dadgar commented Oct 12, 2015

Looks good! Left small comments

@HenryTheHamster

This comment has been minimized.

Show comment
Hide comment
@HenryTheHamster

HenryTheHamster Oct 22, 2015

Any movement on this? This feature is something we'd very much like to see in Nomad, and this seems like a great implementation.

Any movement on this? This feature is something we'd very much like to see in Nomad, and this seems like a great implementation.

@apognu

This comment has been minimized.

Show comment
Hide comment
@apognu

apognu Oct 25, 2015

Contributor

Sorry, have been busy for the last two weeks, will fix soon.

Contributor

apognu commented Oct 25, 2015

Sorry, have been busy for the last two weeks, will fix soon.

@armon

This comment has been minimized.

Show comment
Hide comment
@armon

armon Oct 26, 2015

Member

@apognu On further discussion, I think it would be a more consistent user experience if we allow binding to an interface. Most operators and developers are not very comfortable with CIDR blocks, and effectively you are asking the system to just resolve the IP for an interface (unless you will have multiple interfaces in the same CIDR which is an unusual arrangement). This way users can specify a specific IP (current) or an interface which is resolved at runtime.

Member

armon commented Oct 26, 2015

@apognu On further discussion, I think it would be a more consistent user experience if we allow binding to an interface. Most operators and developers are not very comfortable with CIDR blocks, and effectively you are asking the system to just resolve the IP for an interface (unless you will have multiple interfaces in the same CIDR which is an unusual arrangement). This way users can specify a specific IP (current) or an interface which is resolved at runtime.

@DanielDent

This comment has been minimized.

Show comment
Hide comment
@DanielDent

DanielDent Jan 16, 2016

I opened hashicorp/consul#1620 on this same issue.

@armon I'd like to offer some additional thoughts for your consideration. I don't agree that an interface name is necessarily easily predictable across deployed instances. This is probably even more true for Nomad than it is for Consul where I first raised this issue. Interface names can depend on the order in which interfaces are brought up - on some systems they can depend on some stateful ideas. On a bare metal Debian instance, /etc/udev/rules.d/70-persistent-net.rules is auto-generated during installation.

Interface names often are associated with a network technology. Some machines might be connected to the cluster over a VPN (and have an interface name based on that), while other machines might have the SDN interconnect offloaded to a hypervisor or network-provided routing infrastructure.

Some sites may even be using IP addresses directly for service discovery/bootstrapping purposes. It might involve anycasted IP addresses, IP addresses which are NATted to another IP, an IP address which gets announced to routing infrastructure using a BGP/IGP approach, or a DHCP server cluster which uses MAC addresses/VLAN tags/switch port metadata to assign static IP addresses to some or all nodes.

With heterogenous infrastructure, an IP subnet approach will work in cases where interface based approaches won't.

Furthermore, a single network interface can have multiple IP addresses associated with it. This is especially common in IPv6 configurations, but it can be come up with IPv4 too. A machine having many IPv6 addresses with a variety of scopes is pretty standard. Allowing users to specify an interface name might not actually help disambiguate the appropriate address on which to bind.

I opened hashicorp/consul#1620 on this same issue.

@armon I'd like to offer some additional thoughts for your consideration. I don't agree that an interface name is necessarily easily predictable across deployed instances. This is probably even more true for Nomad than it is for Consul where I first raised this issue. Interface names can depend on the order in which interfaces are brought up - on some systems they can depend on some stateful ideas. On a bare metal Debian instance, /etc/udev/rules.d/70-persistent-net.rules is auto-generated during installation.

Interface names often are associated with a network technology. Some machines might be connected to the cluster over a VPN (and have an interface name based on that), while other machines might have the SDN interconnect offloaded to a hypervisor or network-provided routing infrastructure.

Some sites may even be using IP addresses directly for service discovery/bootstrapping purposes. It might involve anycasted IP addresses, IP addresses which are NATted to another IP, an IP address which gets announced to routing infrastructure using a BGP/IGP approach, or a DHCP server cluster which uses MAC addresses/VLAN tags/switch port metadata to assign static IP addresses to some or all nodes.

With heterogenous infrastructure, an IP subnet approach will work in cases where interface based approaches won't.

Furthermore, a single network interface can have multiple IP addresses associated with it. This is especially common in IPv6 configurations, but it can be come up with IPv4 too. A machine having many IPv6 addresses with a variety of scopes is pretty standard. Allowing users to specify an interface name might not actually help disambiguate the appropriate address on which to bind.

}
// Split subnet CIDR IP from the port
tokens := strings.Split(ip, ":")

This comment has been minimized.

@karnauskas

karnauskas Jan 16, 2016

This won't work with IPv6

@karnauskas

karnauskas Jan 16, 2016

This won't work with IPv6

@skozin

This comment has been minimized.

Show comment
Hide comment
@skozin

skozin Jan 20, 2016

I completely agree with @DanielDent here. Interface names are really bad means of expressing high-level concepts, because they depend on many factors, some of which are unpredictable and/or setup-specific. On the other hand, subnets in a cluster are stable and known in advance in the majority of cases.

skozin commented Jan 20, 2016

I completely agree with @DanielDent here. Interface names are really bad means of expressing high-level concepts, because they depend on many factors, some of which are unpredictable and/or setup-specific. On the other hand, subnets in a cluster are stable and known in advance in the majority of cases.

@dadgar

This comment has been minimized.

Show comment
Hide comment
@dadgar

dadgar Mar 22, 2016

Contributor

#941 allows binding to an interface name which solves a lot of the configuration pain. If more complicated configuration is needed, a script can be written to dynamically generate the Nomad config.

Contributor

dadgar commented Mar 22, 2016

#941 allows binding to an interface name which solves a lot of the configuration pain. If more complicated configuration is needed, a script can be written to dynamically generate the Nomad config.

@dadgar dadgar closed this Mar 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment