Nuke ES_CLASSPATH appending, JarHell fail on empty classpath elements #13880
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 elastic#13864
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 elastic#13812
|
LGTM. |
rmuir
added a commit
that referenced
this pull request
Oct 1, 2015
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
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
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
clintongormley
added
>enhancement
:Delivery/Packaging
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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