Libbeat's HTTP Server can now listen to unix socket. #13655

ph · 2019-09-12T16:56:47Z

Allow to use a socket file using the unix:///tmp/hello.sock syntax
to define an HTTP server that will listen HTTP request and add support for windows named pipe using the npipe:///hello syntax.

Tasks:

Implements the host parsing.
Refactor the API handler to be reusable and testable.
Refactor the API Code so we can start and stop a server gracefully,
this is require to clean up the socket after usage.
Allow to mock monitoring.

NOTES: I need to verify the SecurityDescriptor permission for the named pipe.
Fixes: #13577

andrewkroh

Happy to see this enhancement. Left a few questions.

Also does Windows have anything similar? I saw this article and wonder if any of this could be made to work https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/.

andrewkroh · 2019-09-12T20:05:27Z

libbeat/api/server.go

@@ -51,16 +56,16 @@ func New(mux *http.ServeMux, config *common.Config) (*Server, error) {
 		return nil, err
 	}

-	return &Server{mux: mux, l: l, config: cfg}, nil
+	return &Server{mux: mux, l: l, config: cfg, log: log.Named("API")}, nil


Could we do lowercase on the logger name? I'd like to keep all logger names lowercase. This will create a new logger and append .API to the existing logger's name (if it had a name).

Would it be useful to include the listener address in logger statements? Something like log.Named("api").With("listener", l.Addr().String()) or log.Named("api").With("endpoint", l.Addr().String()).

I've downcased the api.

This will create a new logger and append .API to the existing logger's name (if it had a name).

Do you find the append problematic?

Not sure adding the listener or endpoint is useful, we only have a single responder?

andrewkroh · 2019-09-12T20:13:09Z

filebeat/filebeat.reference.yml

@@ -2034,7 +2034,8 @@ logging.files:
 # Defines if the HTTP endpoint is enabled.
 #http.enabled: false

-# The HTTP endpoint will bind to this hostname or IP address. It is recommended to use only localhost.
+# The HTTP endpoint will bind to this hostname, IP address or unix socket. It is recommended
+# to use only localhost.


Seems like we should reconsider this recommendation now.

I was about to change it, but I wanted to bring it up with @elastic/stack-monitoring,I know they wanted to make http.enabled by default at some point.

As far as the recommendation goes, what if it just said If binding to an IP address it is recommended to use only localhost.?

@cachedout I think the wording is fine, but I presume @andrewkroh question was more should we default to npipe or Unix socket instead of TCP for the HTTP endpoint.

My vote would be to default to the unix socket/npipe (starting 8.0.0 with deprecation warnings in 7.x, if possible) since its more secure (can only accept connections from local machine) and slightly faster than TCP. If users need the HTTP API to be callable from outside the local machine, they can take the deliberate step of configuring that.

@ycombinator @cachedout What about the "beta" warning when starting the API?

You mean this line?

2019-09-13T08:52:45.092-0700 WARN [cfgwarn] api/server.go:34 EXPERIMENTAL: Metrics endpoint is enabled.

If so, I vote for it to be taken out. As you said in another comment, this API has been around for a while now.

I've removed the beta label but I didn't add a deprecation or changed the defaults.

libbeat/api/server_test.go

CHANGELOG.next.asciidoc

libbeat/docs/http-endpoint.asciidoc

libbeat/api/server.go

ph · 2019-09-13T00:32:05Z

@andrewkroh Thanks for the review, concerning windows support, I just added support for windows named pipe.

I will address your concerns tomorrow 🤗

libbeat/api/npipe/pipe.go

ycombinator · 2019-09-13T15:55:00Z

I built Filebeat with this PR and then added the following lines to my filebeat.yml:

http.enabled: true
http.host: unix:///tmp/foo.sock

When I run Filebeat with filebeat -e, I get these messages in the log:

2019-09-13T08:52:45.092-0700    INFO    api/server.go:38        Starting stats endpoint
2019-09-13T08:52:45.093-0700    INFO    api/server.go:49        Metrics endpoint listening on: unix:///tmp/foo.sock:5066
2019-09-13T08:52:45.093-0700    INFO    api/server.go:51        finished starting stats endpoint: listen tcp: address unix:///tmp/foo.sock:5066: too many colons in address

Two questions:

Am I specifying the Unix socket incorrectly in my configuration?
Shouldn't the "too many colons in address" log message be of type ERROR instead of INFO?

ph · 2019-09-13T16:02:38Z

Am I specifying the Unix socket incorrectly in my configuration?
port should be ignored, I have a similar test config and it just pass. I will check if I can add a system test.

Shouldn't the "too many colons in address" log message be of type ERROR instead of INFO?
Good point, I think the statement was before the refactoring.

ph · 2019-09-13T21:34:27Z

@ycombinator lets pair on monday, I was not able to reproduce your error, are you using the latest commits?

ph · 2019-09-13T21:36:20Z

I need to add support for the helper.NewHTTP to connect to both unix socket and named pipe.

ycombinator · 2019-09-13T21:38:05Z

@ycombinator lets pair on monday, I was not able to reproduce your error, are you using the latest commits?

I will try again and if I can still reproduce the same error, lets pair on Monday.

michalpristas

LGTM so far

filebeat/filebeat.yml

ycombinator · 2019-09-16T13:28:09Z

I will try again and if I can still reproduce the same error, lets pair on Monday.

I checked out this PR again and now things are working as expected.

libbeat/api/make_listener_windows.go

ph · 2019-09-16T13:32:31Z

@ycombinator do you mind if I split server and client changes in another PR? That will make it smaller to review.

libbeat/api/server.go

ycombinator · 2019-09-16T13:35:56Z

do you mind if I split server and client changes in another PR? That will make it smaller to review.

Not at all, even better! Thanks @ph.

ph · 2019-09-16T15:23:28Z

I believe I have addressed the concerns of @ycombinator and @andrewkroh.

Concerning the changing the default and adding a deprecation warning, I would prefer we have this out of the door with a few usage from users. So we can do it in 7.6

Do a final review and I am finishing a PR that that support for the http client to support both unix socket and named pipe.

ycombinator · 2019-09-16T15:45:39Z

Concerning the changing the default and adding a deprecation warning, I would prefer we have this out of the door with a few usage from users. So we can do it in 7.6

++ Also, we might want to separately discuss whether this needs to go into 8.0.0 only since we're changing the default behavior. It's not a breaking change per se but it may catch users by surprise. Anyway, let's discuss that in a separate issue/PR from this one.

libbeat/api/npipe/listener_windows.go

auditbeat/auditbeat.reference.yml

libbeat/api/make_listener_posix.go

libbeat/api/npipe/listerner_posix.go

libbeat/api/npipe/listener_windows.go

libbeat/api/make_listener_windows.go

ph · 2019-10-09T18:55:58Z

The failures on TravisCI seems to be related to #13987
Beats-ci is mostly green, please do a final review so we can merge this. Thanks.

ycombinator · 2019-10-10T12:51:52Z

auditbeat/auditbeat.reference.yml

 #http.host: localhost

 # Port on which the HTTP endpoint will bind. Default is 5066.
 #http.port: 5066

+# Define which user should be owning the named pipe.
+#http.user:


Would it make sense to place this setting and the one right below it under a http.named_pipe (or similar) namespace?

Make sense, I think it will make it more obvious for the user.

libbeat/api/make_listener_posix.go

ycombinator

Functionally LGTM, works as expected. Left a couple of minor comments.

ycombinator · 2019-10-10T16:33:43Z

libbeat/api/make_listener_posix.go

+	}
+
+	if network == "unix" {
+		if _, err := os.Stat(path); !os.IsNotExist(err) {


The double negative if predicate, !os.IsNotExist(err) is a bit hard to parse. Any reason we couldn't do os.IsExist(err)?

Just chatted with @ph off-PR about this change so wanted to summarize our discussion here. Apparently, if the double-negative check here is replaced with os.IsExist(err), things don't work as expected:

$ touch /tmp/foo $ ./filebeat -c filebeat.dev.yml -E http.host=unix:///tmp/foo Exiting: could not start the HTTP server for the API: listen unix /tmp/foo: bind: address already in use

But if we keep the double-negative check, everything works as expected. So we are bringing it back.

Allow to use a socket file using the `unix:///tmp/hello.sock` syntax to define an HTTP server that will listen HTTP request. Tasks: - Implements the host parsing. - Refactor the API handler to be reusable and testable. - Refactor the API Code so we can start and stop a server gracefully, this is require to clean up the socket after usage.

ph · 2019-10-10T16:36:57Z

@ycombinator updated with your comments.

ycombinator

LGTM (again). WFG.

ruflin · 2019-10-16T09:32:16Z

Just stumbled over this, great to see this implemented. I wonder if this could allow us to have the endpoint enabled by default (if not already the case)?

ph · 2019-10-16T12:05:35Z

@ruflin I think it could be on by default but maybe it should be included in the 8.0 deprecation or changes issue? wdyt @ycombinator.

ycombinator · 2019-10-16T14:05:52Z

So if we turn it on by default, I assume the default would be to listen on the socket. If so, what would the default socket name/path be? Maybe something including the PID so we don't have collisions between multiple beats running on the same host?

ph · 2019-10-16T14:45:50Z

Lets move the discussion in #12918

ph added the libbeat label Sep 12, 2019

ph requested review from ycombinator, cachedout and michalpristas September 12, 2019 16:56

ph requested review from a team as code owners September 12, 2019 16:56

andrewkroh reviewed Sep 12, 2019

View reviewed changes

ph force-pushed the monitoring/support-socket branch from 1974a2f to 88e1232 Compare September 13, 2019 00:31

michalpristas reviewed Sep 13, 2019

View reviewed changes

libbeat/api/npipe/pipe.go Outdated Show resolved Hide resolved

michalpristas approved these changes Sep 16, 2019

View reviewed changes

ycombinator reviewed Sep 16, 2019

View reviewed changes

filebeat/filebeat.yml Outdated Show resolved Hide resolved

ycombinator reviewed Sep 16, 2019

View reviewed changes

libbeat/api/make_listener_windows.go Outdated Show resolved Hide resolved

ycombinator reviewed Sep 16, 2019

View reviewed changes

libbeat/api/server.go Outdated Show resolved Hide resolved

ph force-pushed the monitoring/support-socket branch from 4e61a64 to f9975b9 Compare September 16, 2019 15:23

ycombinator reviewed Sep 16, 2019

View reviewed changes

libbeat/api/npipe/listener_windows.go Outdated Show resolved Hide resolved

cachedout reviewed Sep 18, 2019

View reviewed changes

auditbeat/auditbeat.reference.yml Outdated Show resolved Hide resolved

libbeat/api/make_listener_posix.go Outdated Show resolved Hide resolved

libbeat/api/npipe/listerner_posix.go Show resolved Hide resolved

michalpristas reviewed Sep 23, 2019

View reviewed changes

libbeat/api/npipe/listener_windows.go Show resolved Hide resolved

urso reviewed Oct 2, 2019

View reviewed changes

libbeat/api/make_listener_windows.go Outdated Show resolved Hide resolved

ph force-pushed the monitoring/support-socket branch from d44857f to 0938e0c Compare October 3, 2019 15:08

ph force-pushed the monitoring/support-socket branch from 2a536d5 to a53a2cb Compare October 8, 2019 17:48

ycombinator reviewed Oct 10, 2019

View reviewed changes

libbeat/api/make_listener_posix.go Show resolved Hide resolved

ycombinator approved these changes Oct 10, 2019

View reviewed changes

ycombinator reviewed Oct 10, 2019

View reviewed changes

ph added 7 commits October 10, 2019 12:35

trace

5c70b54

cleanup vendor.json

7792540

capitalize

32c6093

change version

7ab4bcf

missing microsoft

9b4981f

review comments

3a9e984

ph force-pushed the monitoring/support-socket branch from bcc6f06 to 3a9e984 Compare October 10, 2019 16:35

ph force-pushed the monitoring/support-socket branch from 69f3063 to 3a9e984 Compare October 10, 2019 16:52

ycombinator approved these changes Oct 10, 2019

View reviewed changes

ph merged commit 5d9aeb7 into elastic:master Oct 11, 2019

urso added v7.5.0 release-highlight labels Oct 22, 2019

Libbeat's HTTP Server can now listen to unix socket. #13655

Libbeat's HTTP Server can now listen to unix socket. #13655

Conversation

ph commented Sep 12, 2019 • edited Loading

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Sep 13, 2019

ycombinator commented Sep 13, 2019

ph commented Sep 13, 2019

ph commented Sep 13, 2019

ph commented Sep 13, 2019

ycombinator commented Sep 13, 2019

michalpristas left a comment

Choose a reason for hiding this comment

ycombinator commented Sep 16, 2019

ph commented Sep 16, 2019

ycombinator commented Sep 16, 2019

ph commented Sep 16, 2019 • edited Loading

ycombinator commented Sep 16, 2019

ph commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Oct 10, 2019

ycombinator left a comment

Choose a reason for hiding this comment

ruflin commented Oct 16, 2019

ph commented Oct 16, 2019

ycombinator commented Oct 16, 2019

ph commented Oct 16, 2019

ph commented Sep 12, 2019 •

edited

Loading

ph commented Sep 16, 2019 •

edited

Loading