Deprecate fallback to java on PATH (#37990)

Finding java on the path is sometimes confusing for users and unexpected, as well as leading to a different java being used than a user expects. This commit adds warning messages when starting elasticsearch (or any tools like the plugin cli) and using java found on the PATH instead of via JAVA_HOME.
jmazanec15 · Feb 21, 2019 · 6e7b643 · 6e7b643
1 parent a96df5d
commit 6e7b643
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 17 deletions.
diff --git a/Vagrantfile b/Vagrantfile
@@ -346,6 +346,11 @@ def sh_install_deps(config,
       echo "==> Java is not installed"
       return 1
     }
+    cat \<\<JAVA > /etc/profile.d/java_home.sh
+if [ -z "\\\$JAVA_HOME" ]; then
+  export JAVA_HOME=$(dirname $(dirname $(readlink -f $(which java))))
+fi
+JAVA
     ensure tar
     ensure curl
     ensure unzip
@@ -382,6 +387,7 @@ Defaults   env_keep += "BATS_UTILS"
 Defaults   env_keep += "BATS_TESTS"
 Defaults   env_keep += "PACKAGING_ARCHIVES"
 Defaults   env_keep += "PACKAGING_TESTS"
+Defaults   env_keep += "JAVA_HOME"
 SUDOERS_VARS
     chmod 0440 /etc/sudoers.d/elasticsearch_vars
   SHELL

diff --git a/distribution/packages/src/common/scripts/preinst b/distribution/packages/src/common/scripts/preinst
@@ -25,7 +25,7 @@ else
 fi
 
 if [ -z "$JAVA" ]; then
-    err_exit "could not find java; set JAVA_HOME or ensure java is in PATH"
+    err_exit "could not find java; set JAVA_HOME"
 fi
 
 case "$1" in

diff --git a/distribution/src/bin/elasticsearch-env b/distribution/src/bin/elasticsearch-env
@@ -41,11 +41,12 @@ if [ -x "$JAVA_HOME/bin/java" ]; then
 else
   set +e
   JAVA=`which java`
+  echo "warning: Falling back to java on path. This behavior is deprecated. Specify JAVA_HOME"
   set -e
 fi
 
 if [ ! -x "$JAVA" ]; then
-  echo "could not find java; set JAVA_HOME or ensure java is in PATH" >&2
+  echo "could not find java; set JAVA_HOME" >&2
   exit 1
 fi
 

diff --git a/distribution/src/bin/elasticsearch-env.bat b/distribution/src/bin/elasticsearch-env.bat
@@ -20,11 +20,12 @@ rem now set the path to java
 if defined JAVA_HOME (
   set JAVA="%JAVA_HOME%\bin\java.exe"
 ) else (
+  echo warning: Falling back to java on path. This behavior is deprecated. Specify JAVA_HOME
   for %%I in (java.exe) do set JAVA="%%~$PATH:I"
 )
 
 if not exist %JAVA% (
-  echo could not find java; set JAVA_HOME or ensure java is in PATH 1>&2
+  echo could not find java; set JAVA_HOME 1>&2
   exit /b 1
 )
 

diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java
@@ -102,7 +102,7 @@ public void test30AbortWhenJavaMissing() {
             );
 
             assertThat(runResult.exitCode, is(1));
-            assertThat(runResult.stderr, containsString("could not find java; set JAVA_HOME or ensure java is in PATH"));
+            assertThat(runResult.stderr, containsString("could not find java; set JAVA_HOME"));
         });
 
         Platforms.onLinux(() -> {
@@ -112,7 +112,7 @@ public void test30AbortWhenJavaMissing() {
                 sh.run("chmod -x '" + javaPath + "'");
                 final Result runResult = sh.runIgnoreExitCode(bin.elasticsearch.toString());
                 assertThat(runResult.exitCode, is(1));
-                assertThat(runResult.stderr, containsString("could not find java; set JAVA_HOME or ensure java is in PATH"));
+                assertThat(runResult.stderr, containsString("could not find java; set JAVA_HOME"));
             } finally {
                 sh.run("chmod +x '" + javaPath + "'");
             }

diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackageTestCase.java
@@ -62,15 +62,16 @@ public void onlyCompatibleDistributions() {
 
     public void test05InstallFailsWhenJavaMissing() {
         final Shell sh = new Shell();
-        final Result java = sh.run("command -v java");
+        final Result javaHomeOutput = sh.run("echo $JAVA_HOME");
 
-        final Path originalJavaPath = Paths.get(java.stdout.trim());
-        final Path relocatedJavaPath = originalJavaPath.getParent().resolve("java.relocated");
+        final Path javaHome = Paths.get(javaHomeOutput.stdout.trim());
+        final Path originalJavaPath = javaHome.resolve("bin").resolve("java");
+        final Path relocatedJavaPath = javaHome.resolve("bin").resolve("java.relocated");
         try {
             mv(originalJavaPath, relocatedJavaPath);
             final Result installResult = runInstallCommand(distribution());
             assertThat(installResult.exitCode, is(1));
-            assertThat(installResult.stderr, containsString("could not find java; set JAVA_HOME or ensure java is in PATH"));
+            assertThat(installResult.stderr, containsString("could not find java; set JAVA_HOME"));
         } finally {
             mv(relocatedJavaPath, originalJavaPath);
         }

diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/WindowsServiceTestCase.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/test/WindowsServiceTestCase.java
@@ -135,7 +135,7 @@ public void test12InstallService() {
     public void test13InstallMissingJava() throws IOException {
         Result result = runWithoutJava(serviceScript + " install");
         assertThat(result.exitCode, equalTo(1));
-        assertThat(result.stderr, containsString("could not find java; set JAVA_HOME or ensure java is in PATH"));
+        assertThat(result.stderr, containsString("could not find java; set JAVA_HOME"));
     }
 
     public void test14RemoveNotInstalled() {

diff --git a/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash b/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash
@@ -171,7 +171,7 @@ fi
   sudo chmod +x $JAVA
 
   [ "$status" -eq 1 ]
-  local expected="could not find java; set JAVA_HOME or ensure java is in PATH"
+  local expected="could not find java; set JAVA_HOME"
   [[ "$output" == *"$expected"* ]] || {
     echo "Expected error message [$expected] but found: $output"
     false

diff --git a/qa/vagrant/src/test/resources/packaging/utils/packages.bash b/qa/vagrant/src/test/resources/packaging/utils/packages.bash
@@ -30,6 +30,14 @@
 # specific language governing permissions and limitations
 # under the License.
 
+env_file() {
+    if is_dpkg; then
+        echo "/etc/default/elasticsearch"
+    fi
+    if is_rpm; then
+        echo "/etc/sysconfig/elasticsearch"
+    fi
+}
 
 # Export some useful paths.
 export_elasticsearch_paths() {
@@ -40,15 +48,11 @@ export_elasticsearch_paths() {
     export ESDATA="/var/lib/elasticsearch"
     export ESLOG="/var/log/elasticsearch"
     export ESPIDDIR="/var/run/elasticsearch"
-    if is_dpkg; then
-        export ESENVFILE="/etc/default/elasticsearch"
-    fi
-    if is_rpm; then
-        export ESENVFILE="/etc/sysconfig/elasticsearch"
-    fi
+    export ESENVFILE=$(env_file)
     export PACKAGE_NAME=${PACKAGE_NAME:-"elasticsearch-oss"}
 }
 
+
 # Install the rpm or deb package.
 # -u upgrade rather than install. This only matters for rpm.
 # -v the version to upgrade to. Defaults to the version under test.
@@ -94,6 +98,9 @@ install_package() {
     else
         skip "Only rpm or deb supported"
     fi
+
+    # pass through java home to package
+    echo "JAVA_HOME=\"$JAVA_HOME\"" >> $(env_file)
 }
 
 # Checks that all directories & files are correctly installed after a deb or