-
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
Refactor UNIX domain socket #612
Conversation
|
||
// errSocketFileExists is the human-friendly error message displayed when | ||
// trying to bind a socket to an existing file. | ||
errSocketFileExists = "A file exists at the requested socket path %q. " + |
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.
Can we surround the path with quotes or a special character? If the path has spaces it it, it might be difficult to distinguish.
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.
%q
's got your back, jack :) http://play.golang.org/p/V6PfAl5PWn
%q a double-quoted string safely escaped with Go syntax
This looks really good to me 👍 . I looked at the behavior of nginx and puma (some popular webservers) and they follow the exact same paradigm: socket is owned by the user running the process. Similarly, postgres outputs an error when you try to listen on a file that already exists. |
Hey @jefferai I am sorry if you feel like we went behind your back. That was certainly no one's intention. We discussed the changes at length as a team and we decided that many of the behaviors introduced were not in parallel with the Principle of Least Surprise or other tools in the industry. Please allow me to elaborate further.
If there are specific issues I did not cover in my response, please let me know comment on the appropriate lines in the diff of this PR and we can definitely revisit the decision(s). |
Hi @sethvargo @ryanuber @armon First off, I want to point out that this is an example of being a terrible upstream. The syntax was proposed on the mailing list. Nobody, including Consul team members, objected. Ryan asked about it on the previous pull request, then after I explained my rationale, he thanked me for it and expressed no reservation. You guys could have had a further discussion with me on the previous PR. You could have expressed concern and asked for community input on the mailing list. You could have poked me in email or on IRC. You could even have gotten me to make needed changes prior to you merging them so that my time wasn't wasted coding them in and writing tests for them only for you guys to waste your time reverting them less than 24 hours later. That sucks. So let's go over the problems with these changes.
Here's another example, using everyone's favorite netcat:
As a contrast, let's see the workflow with nginx:
I don't know where you guys got your intel, but it's wrong. You care about principle of least surprise...so do I. The least surprise solution is not to leave a dead socket lying around that the user can't do anything with anyways and then fail to start because of it. That will just leave users pissed off when a simple restart of consul fails to work. The least surprise solution is to do what everyone else is doing, and remove the socket and then re-create it. |
@jefferai @ryanuber @sethvargo Okay, so how about the following:
Thoughts? |
@armon That's fine -- as I said in the other pull request, changing configuration is the ideal option. I can see arguments as to why you'd want to be able to specify user/group individually for each socket (since RPC and HTTP have different APIs, and AFAIK not all things can be done on each). Maybe that's too advanced, but this might be the best time to implement something along those lines. Unfortunately, the "best" solution that I can think of would require deprecating or changing |
@jefferai Is this a use case you guys have currently? (Having distinct permissions per-socket). If it isn't then I'd prefer to keep is simple and just have a single set of
|
@armon It's not a current use-case, no, although I can't speak for everyone here. But, I was simply figuring that if that might be useful to anyone down the road, easier to change things before people use them than after. :-) |
@armon @sethvargo @ryanuber BTW, since nginx is a common example for you guys, nginx uses |
Perfect. So I think we can do:
Then in the future, we can add "rpc" and "dns" override blocks within "unix_sockets" if needed. |
@armon I'm guessing you meant "rpc" and "http"; there's no reason for DNS to be over Unix sockets. (Or rather, not for the same reasons that drove this implementation.) But yes, that looks great. |
This is a follow-on from #587. There are a couple adjustments to the functionality of the feature, which I’ll outline below. Due to these changes in functionality, a good deal of code and tests have been adjusted as well.
Ownership and permissions of the socket are not managed by Consul.
After an internal discussion about this, we think the best thing for Consul to do is to simply allow the socket to be created by the underlying system calls, and leave it at that. This same approach is taken by a number of other projects, and after some research, it has become more clear why that is. Looking into the OS documentation on a standard Linux distribution (
man 7 unix
), we will find that sockets in Linux obey the file permissions of their parent directory, and that some BSD and other distributions ignore the permissions on the socket files completely. Because of this, we will just accept a standard UNIX domain socket path likeunix:///path/to/file
, which unambiguously creates the socket at that path per the OS’es implementation, and allows the operator to configure permissions at the directory above.Consul errors when the specified socket path already exists
This was discussed in the original PR. After some more discussions and opinions, we’ve decided to err on the side of caution, and the safest thing to do in cases where the socket path already exists is to throw an error. The socket bind error from the underlying OS might be confusing to a novice user, so we’ve added a more human-friendly error message in this case explaining that it is possible the socket file was not cleaned up due to forced shutdown.