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

listen on unix domain socket #768

Merged
merged 10 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@graphaelli
Copy link
Member

commented Mar 22, 2018

with apm-server.host=unix:/path

closes #689

@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2018

still thinking about a test for this, might have to be in a separate integration test

@alvarolobato alvarolobato added this to the 6.4 milestone Mar 23, 2018

listen on unix domain socket
with apm-server.host=unix:/path

@graphaelli graphaelli force-pushed the graphaelli:accept-unix branch from 15417f7 to 0a5244c Apr 4, 2018

@graphaelli graphaelli requested review from roncohen, jalvz and simitt Apr 4, 2018

graphaelli added some commits Apr 4, 2018

test unix listener
Refactors test server startup to create beat so that tests use the same
path through beater.Run as under normal operation.

@graphaelli graphaelli force-pushed the graphaelli:accept-unix branch from 0a5244c to 4e7f90a Apr 4, 2018

@graphaelli graphaelli removed request for roncohen, jalvz and simitt Apr 5, 2018

assert.NoError(t, err)

// create a beat
apm, err := instance.NewBeat("test-apm-server", "", "")

This comment has been minimized.

Copy link
@graphaelli

graphaelli Apr 5, 2018

Author Member

spoke with beats team about this since it felt gross - suggested it was better to duplicate the logic than pull instance in

graphaelli added some commits Apr 5, 2018

@graphaelli graphaelli force-pushed the graphaelli:accept-unix branch from 92c84f7 to 7ba774f Apr 5, 2018

@jalvz

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2018

ip/ExtractIP will be foobar'ed with this, so you probably need handle the case of not existing IPs, maybe here: https://github.com/jalvz/apm-server/blob/640e667fcef6677287b9e5830cad88ec2a44f80b/utility/common.go#L31

@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

@jalvz we only extractIP from client requests right? I expect that wouldn't change, but I can add a test to be sure.

@jalvz

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2018

what do you mean with client requests? we do it for all the requests that come in, either backend or frontend.

@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

I mean as opposed to the listening address. I'm not sure where something that's not an IP would be exposed to extractIP?

@jalvz

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2018

if you make a request to an endpoint; that function gets called, right?

@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2018

resolved in chat, update needed to omit remote IP if empty. @jalvz reminds me we need docs for this.

@@ -56,11 +57,25 @@ func (bt *beater) Run(b *beat.Beat) error {
}
defer pub.Stop()

lis, err := net.Listen("tcp", bt.config.Host)
path := bt.config.Host

This comment has been minimized.

Copy link
@roncohen

roncohen Apr 9, 2018

Collaborator

Tiny nitpick: Could we extract this into its own function?

@watson

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Is the unix: prefix a common thing to use? I've always heard that it should always end in .sock to be valid. But it's all anecdotal, so just curious.

@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2018

That's what I've used in the past, probably because of apache. While it's not a standard, nginx also uses it so that seems like enough precedent to me.

graphaelli added some commits Apr 9, 2018

omit IP if empty
Remote Address is not set when connecting through a unix domain socket listener.  This produces a document like:

{
  "context": {
    "system": {
      "ip": ""
    }
  }
}

which triggers an error during indexing - illegal_argument_exception: "" is not an IP string literal.

@graphaelli graphaelli merged commit 4cc227b into elastic:master Apr 9, 2018

3 checks passed

CLA Commit author has signed the CLA
Details
apm-ci Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@graphaelli graphaelli deleted the graphaelli:accept-unix branch Apr 9, 2018

graphaelli added a commit to graphaelli/apm-server that referenced this pull request Apr 10, 2018

listen on unix domain socket (elastic#768)
* listen on unix domain socket

with apm-server.host=unix:/path

* test unix listener

Refactors test server startup to create beat so that tests use the same
path through beater.Run as under normal operation.

* use beater.Run for all server tests

* tidy imports

* address data race

* skip unix domain socket test on windows

* create beat directly

* omit IP if empty

Remote Address is not set when connecting through a unix domain socket listener.  This produces a document like:

{
  "context": {
    "system": {
      "ip": ""
    }
  }
}

which triggers an error during indexing - illegal_argument_exception: "" is not an IP string literal.

* document unix domain socket option usage

* refactor listener creation into separate function
@watson

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

@graphaelli Great work!

Did you ever look into supporting named pipes on Windows as well? (the equivalent of domain sockets in the Windows world).

I know I'm too late to the party here, but we might want to consider if the prefix unix: is good or bad in the context of allowing future support for named pipes. A more general purpose prefix could be ipc: - or we might consider not having a prefix at all (FYI: this is the regex I normally use to detect if the user have provided a named pipe on Windows: /^\\\\[?.]\\pipe/).

simitt added a commit that referenced this pull request Apr 11, 2018

listen on unix domain socket (#768) (#823)
* listen on unix domain socket

with apm-server.host=unix:/path

* test unix listener

Refactors test server startup to create beat so that tests use the same
path through beater.Run as under normal operation.

* use beater.Run for all server tests

* tidy imports

* address data race

* skip unix domain socket test on windows

* create beat directly

* omit IP if empty

Remote Address is not set when connecting through a unix domain socket listener.  This produces a document like:

{
  "context": {
    "system": {
      "ip": ""
    }
  }
}

which triggers an error during indexing - illegal_argument_exception: "" is not an IP string literal.

* document unix domain socket option usage

* refactor listener creation into separate function
@graphaelli

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2018

I did not consider windows named pipes support. We'd need separate logic for creating that kind of listener anyway so that should be a separate issue (and I'd love to hear a need before we spend time on it) - a quick look indicates http://github.com/Microsoft/go-winio (MIT licensed) would make this pretty straightforward, we'd just have to decide on how users would specify that type of listener.

That might all be moot as (news to me) it seems unix socket support is coming to windows, so I expect unix:c:\tmp\apm-server.sock will just work there eventually. Of course we'll have to fix up the tests to use that instead of skipping them on windows.

Again, would love to hear user demand for getting these done but they don't seem too challenging given the libraries already available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.