Resolve peer address before assigning it's IP to member #1634
Conversation
|
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
The previous behavour of a peer binding would not resolve a dns entry before trying to unwrap a string. This caused a thread panic that would kill the supervisor. The new behavour will try and resolve the ip's for dns entries and die if it is unable to do so. Signed-off-by: Elliott Davis <edavis@chef.io>
elliott-davis
force-pushed
the
resolve_peer_host
branch
from
9cdfdc2
to
f061c59
Compare
|
Awesome like a possom! Rerunning travis and appveyor whose breaks look unrelated. |
|
@thesentinels approve |
|
🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests. |
|
|
Travis CI has
|
💖 Travis CI reports this PR passed. It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now. I just want you and the contributor to answer me one question:
|
| @@ -87,7 +87,10 @@ impl Manager { | |||
| None)); | |||
| outputln!("Butterfly Member ID {}", server.member_id()); | |||
| for peer_addr in gconfig().gossip_peer() { | |||
| let addr: SocketAddr = peer_addr.parse().unwrap(); | |||
| let addrs: Vec<SocketAddr> = peer_addr.to_socket_addrs() | |||
| .expect("Unable to resolve domain") | |||
There was a problem hiding this comment.
This function returns a Result - we should add an error for failure to resolve, and return an error, so that the tooling can handle things like pretty printing the error.
There was a problem hiding this comment.
Doesn't the expect mostly take care of this? It conveys how the input failed and if it does fail, I don't see harm in the panic as long as its made clear to the user why.
There was a problem hiding this comment.
The application knows how to exit cleanly with an error message, with pretty colors and details about precisely where the error was generated in the source. Panic makes sure you can't do that.
Also, this gives optionality to the caller - if you wanted to panic, you easily can.
christophermaier
added
Focus:Supervisor
The previous behavour of a peer binding would not resolve a dns entry before trying to unwrap a string. This caused a thread panic that would kill the supervisor.
The new behavour will try and resolve the ip's for dns entries and die if it is unable to do so.