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

Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements #13880

Closed

Conversation

@rmuir
Copy link
Contributor

commented Sep 30, 2015

Old ES 1.x startup scripts were buggy, adding empty classpath elements. But this can be horrible: it means "CWD" to java, and when starting from a service maybe that is /, and now JarHell is scanning your entire computer (like #13864).

It would be better to just fail hard on a bogus classpath, and hint at the possible cause (outdated shell script from a previous version?)

Additionally, users can still override ES_CLASSPATH, which caused the whole bug in the first place for the 1.x scripts, but we should fail on that too. Its not going to work and you will just get a securityexception. Instead we can tell the user how to do it better.

Maybe we want to clean this up for earlier versions (e.g. 2.1 or maybe even 2.0) too, as some of it is "our fault" (the old broken scripts) and possible still "our fault" (maybe packaging is not upgrading them properly?)

Relates to #13864
Closes #13812

rmuir added 3 commits Sep 30, 2015
Fail hard on empty classpath elements.
This can happen easily, if somehow old 1.x shellscripts survive and try to launch 2.x code.
I have the feeling this happens maybe because of packaging upgrades or something.
Either way: we can just fail hard and clear in this situation, rather than the current situation
where CWD might be /, and we might traverse the entire filesystem until we hit an error...

Relates to #13864
Nuke ES_CLASSPATH appending
Out of box, ES expects its stuff to be in particular places. We should not be appending to ES_CLASSPATH, allowing users to specify stuff there, like we do in elasticsearch.bin.sh

If the user sets it, its not going to work out of box.

Closes #13812
@rjernst

This comment has been minimized.

Copy link
Member

commented Sep 30, 2015

LGTM.

@rmuir rmuir closed this in 8c4bc7d Sep 30, 2015

rmuir added a commit that referenced this pull request Oct 1, 2015
Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements
Closes #13880

Squashed commit of the following:

commit 316a328
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:57:47 2015 -0400

    windows is terrible

commit 0406b56
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:43:09 2015 -0400

    Nuke ES_CLASSPATH appending

    Out of box, ES expects its stuff to be in particular places. We should not be appending to ES_CLASSPATH, allowing users to specify stuff there, like we do in elasticsearch.bin.sh

    If the user sets it, its not going to work out of box.

    Closes #13812

commit 415d897
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:26:35 2015 -0400

    Fail hard on empty classpath elements.

    This can happen easily, if somehow old 1.x shellscripts survive and try to launch 2.x code.
    I have the feeling this happens maybe because of packaging upgrades or something.
    Either way: we can just fail hard and clear in this situation, rather than the current situation
    where CWD might be /, and we might traverse the entire filesystem until we hit an error...

    Relates to #13864
rmuir added a commit that referenced this pull request Oct 1, 2015
Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements
Closes #13880

Squashed commit of the following:

commit 316a328
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:57:47 2015 -0400

    windows is terrible

commit 0406b56
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:43:09 2015 -0400

    Nuke ES_CLASSPATH appending

    Out of box, ES expects its stuff to be in particular places. We should not be appending to ES_CLASSPATH, allowing users to specify stuff there, like we do in elasticsearch.bin.sh

    If the user sets it, its not going to work out of box.

    Closes #13812

commit 415d897
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:26:35 2015 -0400

    Fail hard on empty classpath elements.

    This can happen easily, if somehow old 1.x shellscripts survive and try to launch 2.x code.
    I have the feeling this happens maybe because of packaging upgrades or something.
    Either way: we can just fail hard and clear in this situation, rather than the current situation
    where CWD might be /, and we might traverse the entire filesystem until we hit an error...

    Relates to #13864

Conflicts:
	core/src/main/java/org/elasticsearch/bootstrap/JarHell.java
rmuir added a commit that referenced this pull request Oct 1, 2015
Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements
Closes #13880

Squashed commit of the following:

commit 316a328
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:57:47 2015 -0400

    windows is terrible

commit 0406b56
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:43:09 2015 -0400

    Nuke ES_CLASSPATH appending

    Out of box, ES expects its stuff to be in particular places. We should not be appending to ES_CLASSPATH, allowing users to specify stuff there, like we do in elasticsearch.bin.sh

    If the user sets it, its not going to work out of box.

    Closes #13812

commit 415d897
Author: Robert Muir <rmuir@apache.org>
Date:   Wed Sep 30 16:26:35 2015 -0400

    Fail hard on empty classpath elements.

    This can happen easily, if somehow old 1.x shellscripts survive and try to launch 2.x code.
    I have the feeling this happens maybe because of packaging upgrades or something.
    Either way: we can just fail hard and clear in this situation, rather than the current situation
    where CWD might be /, and we might traverse the entire filesystem until we hit an error...

    Relates to #13864

Conflicts:
	core/src/main/java/org/elasticsearch/bootstrap/JarHell.java

Conflicts:
	core/src/main/java/org/elasticsearch/bootstrap/JarHell.java
	core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java
@rmuir rmuir referenced this pull request Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.