-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
nsqd: ability to bind unix sockets #1434
Conversation
1c7b31e
to
78dc7fe
Compare
nsqd/http.go
Outdated
@@ -107,8 +108,7 @@ func freeMemory(w http.ResponseWriter, req *http.Request, ps httprouter.Params) | |||
|
|||
func (s *httpServer) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
if !s.tlsEnabled && s.tlsRequired { | |||
resp := fmt.Sprintf(`{"message": "TLS_REQUIRED", "https_port": %d}`, | |||
s.nsqd.RealHTTPSAddr().Port) | |||
resp := fmt.Sprintf(`{"message": "TLS_REQUIRED", "https_addr": "%s"}`, s.nsqd.RealHTTPSAddr()) |
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.
This is a backwards-incompatible change, we want to keep the old https_port
part of the response if nsqd is not using the new unix-socket address feature.
It seems that this may not really be relevant for unix-sockets listeners anyway - you probably wouldn't use tls on the unix socket. Only a local process could connect to a unix socket, tls is less relevant so you'd probably use plain http, or it would be an https proxy like nginx terminating tls then forwarding the request to the nsqd http unix socket. So if tls-required and https-address is a unix socket ... I'd just put 0 for https_port
for now I guess.
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.
Just figured out that internal/http_api/api_request.go
GETV1/POSTV1
has retries by that port. I just disable ability to using https via unix socket and revert part of code.
I'm OK with either way, I can see arguments for removing this (not really useful) and keeping it (for consistency with the other address options). Anyway, this looks good to me, can you rebase+squash your commits down to a single commit? |
- Using -1 for tcp/http port for /info endpoint - Disable ability using https for unix sockets
875b681
to
84ea8f6
Compare
Done. |
Thanks! |
Hello, this PR provided unix socket support.
nsqd may started with
I've adopted pynsq and go-nsq for connecting to unix sockets as well. I'll create PRs after resolution.