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

Logging shutdown hack #20389

Merged
merged 6 commits into from Sep 9, 2016

Conversation

Projects
None yet
4 participants
@jasontedor
Copy link
Member

commented Sep 8, 2016

Log4j has a bug where on shutdown it ignores that its use of JMX might
be disabled. This leads to Log4j attempting to access JMX leading to a
security exception. This pull request intentionally introduces jar hell
with the Server class to work around this bug until a fix is a
released. We also move Log4j shutdown from Node#stop to Node#close as
the former was too early (but was masked by the above Log4j issue) given
that we also try to log in Node#close.

Relates #20304

jasontedor added some commits Sep 8, 2016

Prepare for Log4j hack
This commit copies Server from Log4j and exempts Server and its inner
classes from jar hell checks. This is to prepare for a hack to work
around a bug in Log4j.
Hack around Log4j bug on shutdown
Log4j has a bug where on shutdown it ignores that JMX might be disabled;
since it does not respect this on shutdown, it proceeds to attempt to
access JMX leading to a security exception that should have otherwise
not occurred had it respected that JMX is disabled. This commit
intentionally introduces jar hell with the Server class to work around
this bug until a fix is released.
Move Log4j shutdown
This commit moves the Log4j shutdown from Node#stop to Node#close as the
former was too early given that we also try to log in Node#close.
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2016

And finally there are clean shutdowns again:

^C[2016-09-08T12:55:47,698][INFO ][node                     ] [kpxKW1m] stopping ...
[2016-09-08T12:55:47,711][INFO ][node                     ] [kpxKW1m] stopped
[2016-09-08T12:55:47,711][INFO ][node                     ] [kpxKW1m] closing ...
[2016-09-08T12:55:47,719][INFO ][node                     ] [kpxKW1m] closed
$
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2016

retest this please

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2016

retest this please

* All instrumentation for Log4j 2 classes can be disabled by setting system property {@code -Dlog4j2.disable.jmx=true}.
* </p>
*/
public final class Server {

This comment has been minimized.

Copy link
@jaymode

jaymode Sep 9, 2016

Member

maybe we can also link to the issue / version this affects here?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 9, 2016

Author Member

There's a link the jar hell exemption, is that sufficient?

This comment has been minimized.

Copy link
@jaymode
@jaymode

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

LGTM

@abeyad

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

LGTM2

jasontedor added some commits Sep 9, 2016

Merge branch 'master' into logging-shutdown-hack
* master:
  Disable console logging
  [TEST] make BaseXContentTestCase platform independent (bis)
  Update search-template.asciidoc
  Added warning messages about the dangers of pathological regexes to: * pattern-replace charfilter * pattern-capture and pattern-replace token filters * pattern tokenizer * pattern analyzer
  Update painless.asciidoc
  [TEST] make BaseXContentTestCase platform independent
  Remove FORCE version_type
  id to name
  Add "version" field to Templates
  Remove unreleased version, these versons should be added once they are released
  Remove allow unquoted JSON
  Remove assertion for cluster name in data path
  Use a comment block to comment out release notes
  Bumped doc versions to 6.0.0-alpha1
  Add breaking changes for 6.0
Add test for Log4j shutdown hack
This  commit adds  a  test  that the  Log4j  shutdown hack  successfully
prevents Log4j from  trying to create an MBean server,  which requires a
permission that we do not grant.
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2016

Thanks for reviewing @jaymode and @abeyad. I added a small test in 6313096; can someone review?

@abeyad

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@jasontedor the test LGTM

@jaymode

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

test LGTM2

Add forbidden APIs suppression to Log4j hack
This commit adds a forbidden APIs suppression to the Log4j Server class
hack as the only modification to this class is to work around the bug in
the class for when Log4j's usage of JMX is disabled.

@jasontedor jasontedor merged commit 55a2f26 into elastic:master Sep 9, 2016

1 check passed

elasticsearch-ci Build finished.
Details

@jasontedor jasontedor deleted the jasontedor:logging-shutdown-hack branch Sep 9, 2016

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2016

Thanks again @abeyad and @jaymode.

jasontedor added a commit that referenced this pull request Sep 9, 2016

Logging shutdown hack
Log4j has a bug where on shutdown it ignores that JMX might be disabled;
since it does not respect this on shutdown, it proceeds to attempt to
access JMX leading to a security exception that should have otherwise
not occurred had it respected that JMX is disabled. This commit
intentionally introduces jar hell with the Server class to work around
this bug until a fix is released.

Relates #20389

jasontedor added a commit that referenced this pull request Sep 9, 2016

Logging shutdown hack
Log4j has a bug where on shutdown it ignores that JMX might be disabled;
since it does not respect this on shutdown, it proceeds to attempt to
access JMX leading to a security exception that should have otherwise
not occurred had it respected that JMX is disabled. This commit
intentionally introduces jar hell with the Server class to work around
this bug until a fix is released.

Relates #20389
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.