From fa126fd853444bed3edaf162cb2a7d62a9a59774 Mon Sep 17 00:00:00 2001
From: Andy Bristol <andy.bristol@elastic.co>
Date: Tue, 29 May 2018 15:17:11 -0700
Subject: [PATCH] [test] packaging: use shell when running commands (#30852)

When subprocesses are started with ProcessBuilder, they're forked by the
java process directly rather than from a shell, which can be surprising
for our use case here in the packaging tests which is similar to
scripting.

This commit changes the tests to run their subprocess commands in a
shell, using the bash -c <script> syntax for commands on linux and using
the powershell.exe -Command <script> syntax for commands on windows.
This syntax on windows is essentially what the tests were already doing.
---
 qa/vagrant/README.md                          | 40 ++++++----------
 .../packaging/util/Archives.java              | 45 ++++++++---------
 .../elasticsearch/packaging/util/Cleanup.java | 24 +++++-----
 .../packaging/util/Platforms.java             | 12 ++---
 .../elasticsearch/packaging/util/Shell.java   | 48 ++++++++++++++++++-
 5 files changed, 101 insertions(+), 68 deletions(-)

diff --git a/qa/vagrant/README.md b/qa/vagrant/README.md
index ce253a2e3e002..801c3e52673a3 100644
--- a/qa/vagrant/README.md
+++ b/qa/vagrant/README.md
@@ -82,38 +82,26 @@ In general it's probably best to avoid running external commands when a good
 Java alternative exists. For example most filesystem operations can be done with
 the java.nio.file APIs. For those that aren't, use an instance of [Shell](src/main/java/org/elasticsearch/packaging/util/Shell.java)
 
-Despite the name, commands run with this class are not run in a shell, and any
-familiar features of shells like variables or expansion won't work.
-
-If you do need the shell, you must explicitly invoke the shell's command. For
-example to run a command with Bash, use the `bash -c command` syntax. Note that
-the entire script must be in a single string argument
+This class runs scripts in either bash with the `bash -c <script>` syntax,
+or in powershell with the `powershell.exe -Command <script>` syntax.
 
 ```java
 Shell sh = new Shell();
-sh.run("bash", "-c", "echo $foo; echo $bar");
-```
 
-Similary for powershell - again, the entire powershell script must go in a
-single string argument
+// equivalent to `bash -c 'echo $foo; echo $bar'`
+sh.bash("echo $foo; echo $bar");
 
-```java
-sh.run("powershell.exe", "-Command", "Write-Host $foo; Write-Host $bar");
+// equivalent to `powershell.exe -Command 'Write-Host $foo; Write-Host $bar'`
+sh.powershell("Write-Host $foo; Write-Host $bar");
 ```
 
-On Linux, most commands you'll want to use will be executable files and will
-work fine without a shell
-
-```java
-sh.run("tar", "-xzpf", "elasticsearch-6.1.0.tar.gz");
-```
+### Notes about powershell
 
-On Windows you'll mostly want to use powershell as it can do a lot more and
-gives much better feedback than Windows' legacy command line. Unfortunately that
-means that you'll need to use the `powershell.exe -Command` syntax as
-powershell's [Cmdlets](https://msdn.microsoft.com/en-us/library/ms714395.aspx)
-don't correspond to executable files and are not runnable by `Runtime` directly.
+Powershell scripts for the most part have backwards compatibility with legacy
+cmd.exe commands and their syntax. Most of the commands you'll want to use
+in powershell are [Cmdlets](https://msdn.microsoft.com/en-us/library/ms714395.aspx)
+which generally don't have a one-to-one mapping with an executable file.
 
-When writing powershell commands this way, make sure to test them as some types
-of formatting can cause it to return a successful exit code but not run
-anything.
+When writing powershell commands in this project it's worth testing them by
+hand, as sometimes when a script can't be interpreted correctly it will
+fail silently.
diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java
index 4a00570bf30f9..c4d3655a55398 100644
--- a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java
+++ b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Archives.java
@@ -64,7 +64,7 @@ public static Installation installArchive(Distribution distribution, Path fullIn
         if (distribution.packaging == Distribution.Packaging.TAR) {
 
             if (Platforms.LINUX) {
-                sh.run("tar", "-C", baseInstallPath.toString(), "-xzpf", distributionFile.toString());
+                sh.bash("tar -C " + baseInstallPath + " -xzpf " + distributionFile);
             } else {
                 throw new RuntimeException("Distribution " + distribution + " is not supported on windows");
             }
@@ -72,11 +72,12 @@ public static Installation installArchive(Distribution distribution, Path fullIn
         } else if (distribution.packaging == Distribution.Packaging.ZIP) {
 
             if (Platforms.LINUX) {
-                sh.run("unzip", distributionFile.toString(), "-d", baseInstallPath.toString());
+                sh.bash("unzip " + distributionFile + " -d " + baseInstallPath);
             } else {
-                sh.run("powershell.exe", "-Command",
+                sh.powershell(
                     "Add-Type -AssemblyName 'System.IO.Compression.Filesystem'; " +
-                    "[IO.Compression.ZipFile]::ExtractToDirectory('" + distributionFile + "', '" + baseInstallPath + "')");
+                    "[IO.Compression.ZipFile]::ExtractToDirectory('" + distributionFile + "', '" + baseInstallPath + "')"
+                );
             }
 
         } else {
@@ -102,35 +103,35 @@ public static Installation installArchive(Distribution distribution, Path fullIn
     private static void setupArchiveUsersLinux(Path installPath) {
         final Shell sh = new Shell();
 
-        if (sh.runIgnoreExitCode("getent", "group", "elasticsearch").isSuccess() == false) {
+        if (sh.bashIgnoreExitCode("getent group elasticsearch").isSuccess() == false) {
             if (isDPKG()) {
-                sh.run("addgroup", "--system", "elasticsearch");
+                sh.bash("addgroup --system elasticsearch");
             } else {
-                sh.run("groupadd", "-r", "elasticsearch");
+                sh.bash("groupadd -r elasticsearch");
             }
         }
 
-        if (sh.runIgnoreExitCode("id", "elasticsearch").isSuccess() == false) {
+        if (sh.bashIgnoreExitCode("id elasticsearch").isSuccess() == false) {
             if (isDPKG()) {
-                sh.run("adduser",
-                    "--quiet",
-                    "--system",
-                    "--no-create-home",
-                    "--ingroup", "elasticsearch",
-                    "--disabled-password",
-                    "--shell", "/bin/false",
+                sh.bash("adduser " +
+                    "--quiet " +
+                    "--system " +
+                    "--no-create-home " +
+                    "--ingroup elasticsearch " +
+                    "--disabled-password " +
+                    "--shell /bin/false " +
                     "elasticsearch");
             } else {
-                sh.run("useradd",
-                    "--system",
-                    "-M",
-                    "--gid", "elasticsearch",
-                    "--shell", "/sbin/nologin",
-                    "--comment", "elasticsearch user",
+                sh.bash("useradd " +
+                    "--system " +
+                    "-M " +
+                    "--gid elasticsearch " +
+                    "--shell /sbin/nologin " +
+                    "--comment 'elasticsearch user' " +
                     "elasticsearch");
             }
         }
-        sh.run("chown", "-R", "elasticsearch:elasticsearch", installPath.toString());
+        sh.bash("chown -R elasticsearch:elasticsearch " + installPath);
     }
 
     public static void verifyArchiveInstallation(Installation installation, Distribution distribution) {
diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Cleanup.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Cleanup.java
index 9e9150c9c1814..a775a23a19490 100644
--- a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Cleanup.java
+++ b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Cleanup.java
@@ -59,16 +59,16 @@ public static void cleanEverything() {
         if (Platforms.WINDOWS) {
 
             // the view of processes returned by Get-Process doesn't expose command line arguments, so we use WMI here
-            sh.runIgnoreExitCode("powershell.exe", "-Command",
+            sh.powershellIgnoreExitCode(
                 "Get-WmiObject Win32_Process | " +
                 "Where-Object { $_.CommandLine -Match 'org.elasticsearch.bootstrap.Elasticsearch' } | " +
-                "ForEach-Object { $_.Terminate() }");
+                "ForEach-Object { $_.Terminate() }"
+            );
 
         } else {
 
-            sh.runIgnoreExitCode("pkill", "-u", "elasticsearch");
-            sh.runIgnoreExitCode("bash", "-c",
-                "ps aux | grep -i 'org.elasticsearch.bootstrap.Elasticsearch' | awk {'print $2'} | xargs kill -9");
+            sh.bashIgnoreExitCode("pkill -u elasticsearch");
+            sh.bashIgnoreExitCode("ps aux | grep -i 'org.elasticsearch.bootstrap.Elasticsearch' | awk {'print $2'} | xargs kill -9");
 
         }
 
@@ -78,8 +78,8 @@ public static void cleanEverything() {
 
         // remove elasticsearch users
         if (Platforms.LINUX) {
-            sh.runIgnoreExitCode("userdel", "elasticsearch");
-            sh.runIgnoreExitCode("groupdel", "elasticsearch");
+            sh.bashIgnoreExitCode("userdel elasticsearch");
+            sh.bashIgnoreExitCode("groupdel elasticsearch");
         }
 
         // delete files that may still exist
@@ -95,7 +95,7 @@ public static void cleanEverything() {
         // disable elasticsearch service
         // todo add this for windows when adding tests for service intallation
         if (Platforms.LINUX && isSystemd()) {
-            sh.run("systemctl", "unmask", "systemd-sysctl.service");
+            sh.bash("systemctl unmask systemd-sysctl.service");
         }
     }
 
@@ -103,19 +103,19 @@ private static void purgePackagesLinux() {
         final Shell sh = new Shell();
 
         if (isRPM()) {
-            sh.runIgnoreExitCode("rpm", "--quiet", "-e", "elasticsearch", "elasticsearch-oss");
+            sh.bashIgnoreExitCode("rpm --quiet -e elasticsearch elasticsearch-oss");
         }
 
         if (isYUM()) {
-            sh.runIgnoreExitCode("yum", "remove", "-y", "elasticsearch", "elasticsearch-oss");
+            sh.bashIgnoreExitCode("yum remove -y elasticsearch elasticsearch-oss");
         }
 
         if (isDPKG()) {
-            sh.runIgnoreExitCode("dpkg", "--purge", "elasticsearch", "elasticsearch-oss");
+            sh.bashIgnoreExitCode("dpkg --purge elasticsearch elasticsearch-oss");
         }
 
         if (isAptGet()) {
-            sh.runIgnoreExitCode("apt-get", "--quiet", "--yes", "purge", "elasticsearch", "elasticsearch-oss");
+            sh.bashIgnoreExitCode("apt-get --quiet --yes purge elasticsearch elasticsearch-oss");
         }
     }
 }
diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java
index 230af8efc2dbb..202c025ae8a55 100644
--- a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java
+++ b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Platforms.java
@@ -28,41 +28,41 @@ public static boolean isDPKG() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "dpkg").isSuccess();
+        return new Shell().bashIgnoreExitCode("which dpkg").isSuccess();
     }
 
     public static boolean isAptGet() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "apt-get").isSuccess();
+        return new Shell().bashIgnoreExitCode("which apt-get").isSuccess();
     }
 
     public static boolean isRPM() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "rpm").isSuccess();
+        return new Shell().bashIgnoreExitCode("which rpm").isSuccess();
     }
 
     public static boolean isYUM() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "yum").isSuccess();
+        return new Shell().bashIgnoreExitCode("which yum").isSuccess();
     }
 
     public static boolean isSystemd() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "systemctl").isSuccess();
+        return new Shell().bashIgnoreExitCode("which systemctl").isSuccess();
     }
 
     public static boolean isSysVInit() {
         if (WINDOWS) {
             return false;
         }
-        return new Shell().runIgnoreExitCode("which", "service").isSuccess();
+        return new Shell().bashIgnoreExitCode("which service").isSuccess();
     }
 }
diff --git a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Shell.java b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Shell.java
index 3adc0b62e04ea..9a908e2d680eb 100644
--- a/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Shell.java
+++ b/qa/vagrant/src/main/java/org/elasticsearch/packaging/util/Shell.java
@@ -29,6 +29,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.stream.Stream;
 
 import static java.util.Collections.emptyMap;
 
@@ -57,7 +58,47 @@ public Shell(Map<String, String> env, Path workingDirectory) {
         this.workingDirectory = workingDirectory;
     }
 
-    public Result run(String... command) {
+    /**
+     * Runs a script in a bash shell, throwing an exception if its exit code is nonzero
+     */
+    public Result bash(String script) {
+        return run(bashCommand(script));
+    }
+
+    /**
+     * Runs a script in a bash shell
+     */
+    public Result bashIgnoreExitCode(String script) {
+        return runIgnoreExitCode(bashCommand(script));
+    }
+
+    private static String[] bashCommand(String script) {
+        return Stream.concat(Stream.of("bash", "-c"), Stream.of(script)).toArray(String[]::new);
+    }
+
+    /**
+     * Runs a script in a powershell shell, throwing an exception if its exit code is nonzero
+     */
+    public Result powershell(String script) {
+        return run(powershellCommand(script));
+    }
+
+    /**
+     * Runs a script in a powershell shell
+     */
+    public Result powershellIgnoreExitCode(String script) {
+        return runIgnoreExitCode(powershellCommand(script));
+    }
+
+    private static String[] powershellCommand(String script) {
+        return Stream.concat(Stream.of("powershell.exe", "-Command"), Stream.of(script)).toArray(String[]::new);
+    }
+
+    /**
+     * Runs an executable file, passing all elements of {@code command} after the first as arguments. Throws an exception if the process'
+     * exit code is nonzero
+     */
+    private Result run(String[] command) {
         Result result = runIgnoreExitCode(command);
         if (result.isSuccess() == false) {
             throw new RuntimeException("Command was not successful: [" + String.join(" ", command) + "] result: " + result.toString());
@@ -65,7 +106,10 @@ public Result run(String... command) {
         return result;
     }
 
-    public Result runIgnoreExitCode(String... command) {
+    /**
+     * Runs an executable file, passing all elements of {@code command} after the first as arguments
+     */
+    private Result runIgnoreExitCode(String[] command) {
         ProcessBuilder builder = new ProcessBuilder();
         builder.command(command);