-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
F sockaddr #2546
F sockaddr #2546
Conversation
…untime using a predefined heuristic as specified in go-sockaddr. PTAL: @slackpad
We don't currently support binding to multiple IP addresses, but when we do the protocol will be to have space delimited IPs. As is this is currently useful for specifying a static fallback address. For example: ``` bind = `{{ GetPrivateIP }} 127.0.0.1 ::1` ``` If `GetPrivateIP` fails to find a correct local address, Consul will bind to `127.0.0.1` automatically.
`GetDefaultInterfaces` isn't being tested or guaranteed to work on any platform atm.
@sean- mind rebasing this to resolve the conflict? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some questions to the review.
switch numIPs := len(ips); { | ||
case numIPs == 0: | ||
return nil, errors.New("No forwardable IP addresses found. Please configure one.") | ||
case numIPs >= 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be == 1
it's only safe to use this if there is just one.
if ip := net.ParseIP(config.AdvertiseAddr); ip == nil { | ||
return nil, fmt.Errorf("Failed to parse advertise address: %v", config.AdvertiseAddr) | ||
// Parse config.AdvertiseAddr to resolve an IP address | ||
out, err := template.Parse(config.AdvertiseAddr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to make sure the result of the parse only has one address and then set config.AdvertiseAddr
to that result, since you won't parse this template again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was prepping the code for multiple listeners, but backed out of that w/ the most recent push. It will be easy enough to update and support when we're ready, however.
} | ||
|
||
// Try to get an advertise address | ||
var bindIPs []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this up at this scope but not used anywhere except the one clause? Did you mean to stick the advertise address into this for each of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I backed out of a simplification and this was residual. I just pushed an update that completed this work.
This matches the docs, however the current behavior is inconsistent. It would be a breaking change, but `0.0.0.0` should match only IPv4 addresses, `[::]` should match IPv6-only addresses, and the empty string should search through all IP addresses, either IPv4 or IPv6.
Changing the semantics of default, unconfigured behavior feels risky at this time therefore closing this PR. Resubmitting this PR focused specifically on the 0.7.X branch. This PR will be resubmitted once the 0.8 branch is cut and we can make changes to the default behaviors for unconfigured bind and advertise address parameters. |
#2786 is a continuation of this in that it's no longer necessary to specify the |
Support late binding to IP addresses based on runtime information via the hashicorp/go-sockaddr library. Example syntax:
Fixes: #1478 #1620 #1570 #1110 #725 #1079
Possibly helps address: #2330 #2505 #473 #1914
And is the first step toward addressing: #2217 #1185
Known issues:
GetDefaultInterfaces
which is used byGetPrivateInterfaces
andGetPublicInterfaces
. Linux, BSD, and Illumos/Solaris should just work™. Windows support forGetDefaultInterfaces
should land shortly and needs to be complete before enabling this in the generic case.