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

Allow :: as a bind address (binds to first public IPv6 address) #1219

Merged
merged 1 commit into from Mar 19, 2016

Conversation

42wim
Copy link
Contributor

@42wim 42wim commented Sep 5, 2015

This patch allows "::" to -bind (as the IPv6 counterpart for the IPv4 0.0.0.0)
The behaviour is opposite of the IPv4 though, it will look for the first public accessible IPv6 address to bind to. (instead of the first private accessible address with IPv4).

As IPv6 does not really have private addresses (not including ULA), if you want to use IPv6 you probably want to bind to the Global Unicast Address.

With these modifications, you can run IPv6-only containers with consul without knowing the IPv6 address beforehand. And thus can setup a complete IPv6-only consul environment.

If you're interested in merging this I'll add tests too.

Impacts #725
Fixes #529 if -bind :: is used

@ryanbreen
Copy link
Contributor

This looks cool. Personally, I don't see any reason not to merge (with tests, though).

@42wim
Copy link
Contributor Author

42wim commented Sep 12, 2015

Added tests (based on the ipv4 ones) and rebased

@ryanbreen
Copy link
Contributor

Cool, this LGTM. Only question is whether @armon, @ryanuber, or @slackpad have philosophical objections to "first public IPv6" being a supported config when we've only ever offered first available private IP binding for IPv4.

I know there are systemic differences between IPv6 and IPv4 that make public IPs more appropriate in IPv6, but this is enough of a difference that I'm not comfortable merging without one of the project owners weighing in.

@42wim
Copy link
Contributor Author

42wim commented Sep 12, 2015

Ok, also maybe I should rename it to the "first Global Unicast Address" instead of "first public IPv6" address.

@42wim
Copy link
Contributor Author

42wim commented Nov 9, 2015

ping @armon, @ryanuber, @slackpad
Any comments ?

@slackpad slackpad added type/enhancement Proposed improvement or new feature thinking More time is needed to research by the Consul Contributors labels Jan 8, 2016
@slackpad slackpad self-assigned this Jan 8, 2016
@slackpad
Copy link
Contributor

Hi @42wim wow sorry for the delay on this one! I think we can take this if you can to a rebase and use [::] instead of :: so that it doesn't break any port number parsing on this in the future.

@slackpad slackpad removed the thinking More time is needed to research by the Consul Contributors label Mar 18, 2016
@42wim
Copy link
Contributor Author

42wim commented Mar 18, 2016

Ok, I changed it to [::] and rebased

if ip.To4() != nil {
continue
}
// do not bind link-local (fe80::/10) / ULA (fc00::/7) / loopback (::1)
Copy link
Contributor

Choose a reason for hiding this comment

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

@42wim I had one other question for you - this will catch the cases you care about - will it knock out any useful addresses? It looks like it's blocking a superset of what you want to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slackpad Those FX::/yy ranges are reserved for specials. Never say never, but the chance that a new global unicast address range will be allocated from that range is very slim.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed Go's net library had support for a couple of these so I tweaked this a little to be more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! That's much better.

@slackpad
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants