search

logstash-plugins / logstash-input-log4j

Apache License 2.0
14 stars 18 forks source link

Add support for proxy protocol #34

Closed nneul closed 7 years ago

nneul commented 8 years ago

Adds proxy protocol support to log4j plugin

Related to:

https://github.com/logstash-plugins/logstash-input-tcp/pull/57 https://github.com/elastic/logstash/issues/4418 https://github.com/logstash-plugins/logstash-input-syslog/pull/30

nneul commented 8 years ago

Not sure what's up with the build failure - it doesn't seem to reference anything I touched other than the input itself.

nneul commented 8 years ago

@jordansissel I have absolutely no idea how to write the spec test case for this one as there is no similar example in the current tests. I made the switch to boolean in the settings though.

This plugin also fails existing tests without my changes.

nneul commented 7 years ago

@ph Can you take a look at this PR? Suggestions for the testing are welcome as well.

jordansissel commented 7 years ago

I didn't have time to review this last week, btw. If you would rather @ph review this, that is ok.

On Sat, Nov 19, 2016 at 5:16 PM Nathan Neulinger notifications@github.com wrote:

@ph https://github.com/ph Can you take a look at this PR? Suggestions for the testing are welcome as well.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/logstash-plugins/logstash-input-log4j/pull/34#issuecomment-261751237, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIC6oF1BlAeWtNsHPwZK6OphmicM2FDks5q_59RgaJpZM4KkaMl .

nneul commented 7 years ago

No preference... wasn't sure who was primary...

jordansissel commented 7 years ago

This is still on my todo list.

jordansissel commented 7 years ago

Sorry again for the delays. I'm working on making it so this plugin can work at all on Logstash 5.x -- Tracking here: https://github.com/elastic/logstash/pull/6309

jordansissel commented 7 years ago

I reviewed the code and it looks good to me.

I have ideas on how to write a test for this:

0) Run nc -l 4000 > log4j.capture to listen on port 4000 and capture the data sent 1) Configure a log4j1 app to use SocketAppender to localhost:4000 2) Send 1 log message, maybe something simple like `logger.info("Hello world") 3) Use this captured payload as the payload to send in the test suite

The above might be simpler to maintain than setting up a full log4j logger with SocketAppender, but I'd accept a log4j SOcketAppender test also.

nneul commented 7 years ago

I'll take at what I can come up with for 0-3 above. Can definitely get 0-2, just need to review the code for how to do #3.

nneul commented 7 years ago

Can you trigger a CI test for this package unmodified? I'd really like to start from a known working state, and right now, pretty sure the tests are failing unrelated to my changes.

ph commented 7 years ago

@nneul I've rekicked the job

nneul commented 7 years ago

I was meaning on the mainline - not on my branch.

ph commented 7 years ago

@nneul I've kicked both the master branch and yours, you were right the master branch has also the same problem. See https://travis-ci.org/logstash-plugins/logstash-input-log4j/builds/151618256

nneul commented 7 years ago

Thanks. I'll ignore - trying to figure out enough of this testing mechanism to write the requested test case.

nneul commented 7 years ago

This doesn't work, but I'm not fluent in the any of this test infrastructure (or ruby for that matter). Can you take a look at the wip test code and see if anything looks obviously the wrong way to go about it - it's just hanging for me on the 'collect' line.

nneul commented 7 years ago

Note to be clear - that test/wip isn't touching the proxy protocol stuff yet - figured it needed to test the base/default case first. That's what I'm stuck on.

nneul commented 7 years ago

Can you merge this - since the underlying proxy protocol code works (I've been running it on my environment for a while now live), and can follow up with working test code once there is time to figure out how to make it work?

jordansissel commented 7 years ago

Since this is a plugin covered by our support team , we really need to have tests before merging code in.

I will work on tests as I have time and merge it once I finish. Probably some time in January.

On Fri, Dec 30, 2016 at 7:09 AM Nathan Neulinger notifications@github.com wrote:

Can you merge this - since the underlying proxy protocol code works (I've been running it on my environment for a while now live), and can follow up with working test code once there is time to figure out how to make it work?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/logstash-plugins/logstash-input-log4j/pull/34#issuecomment-269781929, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIC6vNHyD5hzz9CkhvuZ33WWWcatooxks5rNR6dgaJpZM4KkaMl .

nneul commented 7 years ago

Ok. Whenever you have a chance to look at it - if you have any ideas on why what I was trying isn't working, let me know. If we can get the base case working, adding test for proxy proto in the same style should be trivial.

jordansissel commented 7 years ago

@nneul did you intend to have this not set proxy_host or proxy_port fields? The TCP input does this :

jordansissel commented 7 years ago

I have forked this PR to #38.