Skip to content
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

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Conversation

telepenin
Copy link
Contributor

@telepenin telepenin commented Dec 23, 2022

Hello, this PR provided unix socket support.

nsqd may started with

/build/nsqd --tcp-address /var/run/nsqd.sock --http-address /var/run/nsqd-http.sock --data-path /var/local/nsqd/

I've adopted pynsq and go-nsq for connecting to unix sockets as well. I'll create PRs after resolution.

apps/nsqd/options.go Outdated Show resolved Hide resolved
nsqd/http.go Outdated Show resolved Hide resolved
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())
Copy link
Member

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.

Copy link
Contributor Author

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.

@ploxiln
Copy link
Member

ploxiln commented Jan 17, 2023

I just disable ability to using https via unix socket

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
@telepenin
Copy link
Contributor Author

Anyway, this looks good to me, can you rebase+squash your commits down to a single commit?

Done.

@ploxiln
Copy link
Member

ploxiln commented Jan 18, 2023

Thanks!

@ploxiln ploxiln merged commit b28153e into nsqio:master Jan 18, 2023
@mreiferson mreiferson changed the title Ability to bind unix sockets for nsqd nsqd: ability to bind unix sockets Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants