From 1e18c9a7b6623db0100d93e92919a4a7d29e7231 Mon Sep 17 00:00:00 2001
From: Simon Willnauer <simonw@apache.org>
Date: Fri, 5 Feb 2016 11:21:00 +0100
Subject: [PATCH] Apply system properties after all arguemnts are parsed in
 BootstrapCLIParser

One of our tests leaked a system property here since we failed after appling some
system properties in BootstrapCLIParser. This is not a huge deal in production since
we exit the JVM if we fail on that. Yet for correctnes we should only apply them if
we manage to parse them all.
This also caused a test failure lately on CI but on an unrelated test:
  https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+periodic/314/console
---
 .../elasticsearch/bootstrap/BootstrapCLIParser.java  | 11 ++++++++---
 .../bootstrap/BootstrapCliParserTests.java           | 12 ++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java
index 9cae3b8cb15..ca67fc91132 100644
--- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java
+++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapCLIParser.java
@@ -32,10 +32,12 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 
 import static org.elasticsearch.common.cli.CliToolConfig.Builder.cmd;
 import static org.elasticsearch.common.cli.CliToolConfig.Builder.optionBuilder;
@@ -131,6 +133,7 @@ final class BootstrapCLIParser extends CliTool {
 
             // hacky way to extract all the fancy extra args, there is no CLI tool helper for this
             Iterator<String> iterator = cli.getArgList().iterator();
+            final Map<String, String> properties = new HashMap<>();
             while (iterator.hasNext()) {
                 String arg = iterator.next();
                 if (!arg.startsWith("--")) {
@@ -148,20 +151,22 @@ final class BootstrapCLIParser extends CliTool {
                     String[] splitArg = arg.split("=", 2);
                     String key = splitArg[0];
                     String value = splitArg[1];
-                    System.setProperty("es." + key, value);
+                    properties.put("es." + key, value);
                 } else {
                     if (iterator.hasNext()) {
                         String value = iterator.next();
                         if (value.startsWith("--")) {
                             throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "] needs value");
                         }
-                        System.setProperty("es." + arg, value);
+                        properties.put("es." + arg, value);
                     } else {
                         throw new UserError(ExitStatus.USAGE, "Parameter [" + arg + "] needs value");
                     }
                 }
             }
-
+            for (Map.Entry<String, String> entry : properties.entrySet()) {
+                System.setProperty(entry.getKey(), entry.getValue());
+            }
             return new Start(terminal);
         }
 
diff --git a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java
index f417fe70d8b..012af99cef0 100644
--- a/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java
+++ b/qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/BootstrapCliParserTests.java
@@ -29,11 +29,15 @@ import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 import org.hamcrest.Matcher;
 import org.junit.After;
+import org.junit.Before;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
+import java.util.Properties;
 
 import static org.elasticsearch.common.cli.CliTool.ExitStatus.OK;
 import static org.elasticsearch.common.cli.CliTool.ExitStatus.OK_AND_EXIT;
@@ -48,6 +52,12 @@ public class BootstrapCliParserTests extends CliToolTestCase {
 
     private CaptureOutputTerminal terminal = new CaptureOutputTerminal();
     private List<String> propertiesToClear = new ArrayList<>();
+    private Map<Object, Object> properties;
+
+    @Before
+    public void before() {
+        this.properties = new HashMap<>(System.getProperties());
+    }
 
     @After
     public void clearProperties() {
@@ -55,6 +65,7 @@ public class BootstrapCliParserTests extends CliToolTestCase {
             System.clearProperty(property);
         }
         propertiesToClear.clear();
+        assertEquals("properties leaked", properties, new HashMap<>(System.getProperties()));
     }
 
     public void testThatVersionIsReturned() throws Exception {
@@ -235,6 +246,7 @@ public class BootstrapCliParserTests extends CliToolTestCase {
                 parser.parse("start", new String[]{"--foo=bar", "-Dbaz=qux"});
         });
         assertThat(e.getMessage(), containsString("must be before any parameters starting with --"));
+        assertNull(System.getProperty("es.foo"));
     }
 
     private void registerProperties(String ... systemProperties) {