Use bind address for consul checks (solves #1574) #1600
Conversation
| @@ -418,6 +418,16 @@ func (a *Agent) setupServer() error { | |||
| }, | |||
| }, | |||
| } | |||
| // Resolve ServiceCheck addresses | |||
There was a problem hiding this comment.
Can you pull these into three methods, getHTTPAddr(preferBind bool), getRPCAddr(preferBind bool) and getSerfAddr(preferBind bool) and use those methods for the service and check.
https://github.com/hashicorp/nomad/blob/master/command/agent/agent.go#L175-L188
Then add a config option to the Consul config checks_use_advertise bool. This will default to false, but if people want the checks to use the advertise they can set it to true.
Please add tests and documentation to the website
|
@evan2645 Do you want to close this? In 0.5.1 I think we are going to take a fresh look at how we do network binding/advertising so we can tackle this then? |
|
@dadgar I have been meaning to clean this up but have not yet had the time. I am still willing to do this work if you think it is useful, however if it will be refactored/re-written soon, maybe not. We are currently running a fork with this patch applied. How far away do you think 0.5.1 is? Getting back on upstream is something we'd like to do. |
|
Okay I was just not sure if it was abandoned :) So the comment I suggested still applies with the refactor so it is worth while 👍 |
Conflicts: command/agent/config.go
|
@dadgar I pushed the changes requested. There are actually three modes we're looking for (real bind, advertise, and preferBind)... so I added a fourth method to account for the preferBind state. I simplified all the code setting bind and advertise config in all the places i could find it. added tests and docs. not sure if this is what you had in mind or not... lmk if it does not suffice |
|
fwiw, i'm pretty sure that failing test is unrelated to these changes |
|
would like to see this get merged sometime soon, we're still running a fork with it i'm more than happy to commit additional changes or open a new pr if this is not the direction you had in mind... just let me know what i can do to get it solved |
|
@evan2645 I am going to take this PR over as we are close to cutting an RC. Sorry for the delay it has been busy around here :) |
|
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. |
Suggested solution for #1574