Add support for late binding to IP addresses using go-sockaddr/template #2399
Conversation
There was a problem hiding this comment.
It seems to me that this code could be all replaced by the functionality provided by go-sockaddr/template in a more flexible way, but I may have miss some behaviors there.
You're absolutely right about that! Definitely something I had been wanting to get to, so thanks for the PR!
Left some comments where your new logic needs to match the old logic. We'll also need to update CHANGELOG and docs (in website/).
Thanks again for taking a stab at this. I think sockaddr/template will make people very happy.
command/agent/config.go
Outdated
| if ip.IsLoopback() && dev { | ||
| // loopback is fine for dev mode | ||
| return net.JoinHostPort(ip.String(), strconv.Itoa(defport)), nil | ||
| ipStr, err := parseSingleIPTemplate(host) |
There was a problem hiding this comment.
I think you need to parse the template before attempting to SplitHostPort as SplitHostPort may fail on valid templates.
command/agent/config.go
Outdated
| } | ||
| return "", fmt.Errorf("No valid advertise addresses, please set `advertise` manually") | ||
| // Fallback to bind address, as it has been resolved before. | ||
| return net.JoinHostPort(bind, strconv.Itoa(defport)), nil |
There was a problem hiding this comment.
There are 2 subtle behaviors I think your code is missing:
- Advertising
localhostis disallowed unless:
- Explicitly configured per advertise addr (basically use any explicitly configured addresses without asking questions)
- DevMode == true
- Default to advertising the IP the hostname resolves as.
- Sadly outside of Google Compute Engine few systems are properly configured to be able to resolve their hostname.
Since 2. doesn't work well in practice I'd be open to changing it to GetPrivateIP, but we'll need to update docs and mark this as a backward incompatible change in CHANGELOG.md
This is out of scope for what you're trying to do, but I also think we'll move bind to default to GetPrivateIP+127.0.0.1 in the future. Sadly we don't support multiple bind addresses at the moment, so it's going to be a larger effort.
There was a problem hiding this comment.
FWIW, Consul will be likely making a change to its defaults in time for its 0.8 release to use GetPrivateIP as its default bind address. And yes, listening on multiple interfaces is something we all want to do (though I'd argue it should be a UNIX socket in /tmp vs 127.0.0.1 that way filesystem permissions could be applied if necessary, and you can figure out the UID of who is connecting to you via a unix socket, but not via loopback - but more on that when we get closer to multiple listeners).
There was a problem hiding this comment.
@schmichael OK, so I changed the code slightly and re-added bits from the previous version, except:
- I removed the
os.Hostname()and relatednet.LookupIP(host)as per the problems you exposed in 2. and I'm looking up the private IP address usingGetPrivateIPinstead. - before that, I'm trying to use the bind address. I still need to do the
ip.IsLinkLocalUnicast() || ip.IsGlobalUnicast()dance since the bind address could be something likelocalhostwhich needs to be parsed in order to useIsLoopback(). I thought I could remove this part and profit from what go-sockaddr/template offers for this instead, but I'm not sure how I could prevent 127.0.0.1 to be advertised without resolving the bind address value, in which case I need to call the functions above to do the right thing...
| t.Fatalf("expected HTTP advertise address 127.0.0.1:4646, got %s", c.AdvertiseAddrs.HTTP) | ||
| } | ||
|
|
||
| if c.AdvertiseAddrs.RPC != "127.0.0.1:4647" { |
There was a problem hiding this comment.
Yeah, we don't want this to happen unless AdvertiseAddrs.RPC is explicitly set to localhost.
The reason is advertising localhost any time other than for a local test node can do Very Bad Things. The worst case scenario is having a server agent advertise localhost to the cluster - even briefly! Nodes will spam their own localhost trying to contact that other node that advertised localhost. CPU and network usage will be extremely high, but things might still be able to limp along depending on other factors. So basically you end up with a crippled cluster in a difficult to diagnose way.
We could add better heuristics for advertising localhost... like only disallow it if boostrap_expect>1. But then things just get even more magical and complicated. I'd prefer to just force everyone not in dev mode to advertise a real address! :)
There was a problem hiding this comment.
Ok, but the configuration explicitly enabled dev mode in this case, so ... I guess it's OK to advertise 127.0.0.1 here?
I'm going to add another test to specifically check that 127.0.0.1 is NOT advertised except if dev mode is enabled or if it has been explicitly configured to do so, but could I make this particular test above clearer?
There was a problem hiding this comment.
re: dev mode, yes, that sounds good and is a sane approach.
|
@schmichael I updated the PR with some bits in the documentation. AFAIK Consul hasn't really advertise there support in their documentation, except for the entry in the Changelog, so I'm not really sure how to document it here. I can add a Is there anything else that should be done to move forward with this PR? |
|
Terribly sorry for letting this languish for so long! Thanks! |
A slight improvement to #2399 - if bind is localhost, return an error instead of advertising a private ip. The advertised ip isn't valid and will just cause errors on use. It's better to fail with an error message instructing users how to fix the problem.
A slight improvement to #2399 - if bind is localhost, return an error instead of advertising a private ip. The advertised ip isn't valid and will just cause errors on use. It's better to fail with an error message instructing users how to fix the problem.
Don't advertise sockaddr support just yet; focus on the saner advertise default
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds support for late IP addresses binding as was merged in Consul in 0.7.2 and is based loosely on the same code.
As far as I'm aware, all the
*addrflags or configuration options should be able to use this new format. I converted the code innormalizeAdvertise(), but I'm not sure about the conversion. It seems to me that this code could be all replaced by the functionality provided by go-sockaddr/template in a more flexible way, but I may have miss some behaviors there.