Allow keystore add-file to handle multiple settings (#54240)

Today the keystore add-file command can only handle adding a single
setting/file pair in a single invocation. This incurs the startup costs
of the JVM many times, which in some environments can be expensive. This
commit teaches the add-file keystore command to accept adding multiple
settings in a single invocation.

Author: jasontedor

docs/reference/commands/keystore.asciidoc

@@ -12,7 +12,7 @@ in the {es} keystore.
 --------------------------------------------------
 bin/elasticsearch-keystore
 ([add <settings>] [-f] [--stdin] |
-[add-file <setting> <path>] | [create] [-p] |
+[add-file (<setting> <path>)+] | [create] [-p] |
 [list] | [passwd] | [remove <setting>] | [upgrade])
 [-h, --help] ([-s, --silent] | [-v, --verbose])
 --------------------------------------------------
@@ -48,7 +48,7 @@ must confirm that you want to overwrite the current value. If the keystore does
 not exist, you must confirm that you want to create a keystore. To avoid these
 two confirmation prompts, use the `-f` parameter.
 
-`add-file <setting> <path>`:: Adds a file to the keystore.
+`add-file (<setting> <path>)+`:: Adds files to the keystore.
 
 `create`:: Creates the keystore.
 
@@ -173,14 +173,23 @@ Values for multiple settings must be separated by carriage returns or newlines.
 ==== Add files to the keystore
 
 You can add sensitive files, like authentication key files for Cloud plugins,
-using the `add-file` command. Be sure to include your file path as an argument
-after the setting name.
+using the `add-file` command. Settings and file paths are specified in pairs
+consisting of `setting path`.
 
 [source,sh]
 ----------------------------------------------------------------
 bin/elasticsearch-keystore add-file the.setting.name.to.set /path/example-file.json
 ----------------------------------------------------------------
 
+You can add multiple files with the `add-file` command:
+
+[source,sh]
+----------------------------------------------------------------
+bin/elasticsearch-keystore add-file \
+  the.setting.name.to.set /path/example-file.json \
+  the.other.setting.name.to.set /path/other-example-file.json
+----------------------------------------------------------------
+
 If the {es} keystore is password protected, you are prompted to enter the
 password.
 

server/src/main/java/org/elasticsearch/common/settings/AddFileKeyStoreCommand.java

@@ -19,11 +19,6 @@
 
 package org.elasticsearch.common.settings;
 
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.util.Arrays;
-import java.util.List;
-
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 import org.elasticsearch.cli.ExitCodes;
@@ -33,6 +28,11 @@
 import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.env.Environment;
 
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+
 /**
  * A subcommand for the keystore cli which adds a file setting.
  */
@@ -47,41 +47,45 @@ class AddFileKeyStoreCommand extends BaseKeyStoreCommand {
         // jopt simple has issue with multiple non options, so we just get one set of them here
         // and convert to File when necessary
         // see https://github.com/jopt-simple/jopt-simple/issues/103
-        this.arguments = parser.nonOptions("setting [filepath]");
+        this.arguments = parser.nonOptions("(setting path)+");
     }
 
     @Override
     protected void executeCommand(Terminal terminal, OptionSet options, Environment env) throws Exception {
-        List<String> argumentValues = arguments.values(options);
+        final List<String> argumentValues = arguments.values(options);
         if (argumentValues.size() == 0) {
             throw new UserException(ExitCodes.USAGE, "Missing setting name");
         }
-        String setting = argumentValues.get(0);
+        if (argumentValues.size() % 2 != 0) {
+            throw new UserException(ExitCodes.USAGE, "settings and filenames must come in pairs");
+        }
+
         final KeyStoreWrapper keyStore = getKeyStore();
-        if (keyStore.getSettingNames().contains(setting) && options.has(forceOption) == false) {
-            if (terminal.promptYesNo("Setting " + setting + " already exists. Overwrite?", false) == false) {
-                terminal.println("Exiting without modifying keystore.");
-                return;
+
+        for (int i = 0; i < argumentValues.size(); i += 2) {
+            final String setting = argumentValues.get(i);
+
+            if (keyStore.getSettingNames().contains(setting) && options.has(forceOption) == false) {
+                if (terminal.promptYesNo("Setting " + setting + " already exists. Overwrite?", false) == false) {
+                    terminal.println("Exiting without modifying keystore.");
+                    return;
+                }
             }
-        }
 
-        if (argumentValues.size() == 1) {
-            throw new UserException(ExitCodes.USAGE, "Missing file name");
-        }
-        Path file = getPath(argumentValues.get(1));
-        if (Files.exists(file) == false) {
-            throw new UserException(ExitCodes.IO_ERROR, "File [" + file.toString() + "] does not exist");
-        }
-        if (argumentValues.size() > 2) {
-            throw new UserException(ExitCodes.USAGE, "Unrecognized extra arguments [" +
-                String.join(", ", argumentValues.subList(2, argumentValues.size())) + "] after filepath");
+            final Path file = getPath(argumentValues.get(i + 1));
+            if (Files.exists(file) == false) {
+                throw new UserException(ExitCodes.IO_ERROR, "File [" + file.toString() + "] does not exist");
+            }
+
+            keyStore.setFile(setting, Files.readAllBytes(file));
         }
-        keyStore.setFile(setting, Files.readAllBytes(file));
+
         keyStore.save(env.configFile(), getKeyStorePassword().getChars());
     }
 
     @SuppressForbidden(reason = "file arg for cli")
     private Path getPath(String file) {
         return PathUtils.get(file);
     }
+
 }

server/src/test/java/org/elasticsearch/common/settings/AddFileKeyStoreCommandTests.java

@@ -19,16 +19,20 @@
 
 package org.elasticsearch.common.settings;
 
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.util.Map;
-
 import org.elasticsearch.cli.Command;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.cli.UserException;
+import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.env.Environment;
 
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+
 import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.containsString;
 
@@ -49,7 +53,7 @@ private Path createRandomFile() throws IOException {
         for (int i = 0; i < length; ++i) {
             bytes[i] = randomByte();
         }
-        Path file = env.configFile().resolve("randomfile");
+        Path file = env.configFile().resolve(randomAlphaOfLength(16));
         Files.write(file, bytes);
         return file;
     }
@@ -164,7 +168,7 @@ public void testMissingFileName() throws Exception {
         terminal.addSecretInput(password);
         UserException e = expectThrows(UserException.class, () -> execute("foo"));
         assertEquals(ExitCodes.USAGE, e.exitCode);
-        assertThat(e.getMessage(), containsString("Missing file name"));
+        assertThat(e.getMessage(), containsString("settings and filenames must come in pairs"));
     }
 
     public void testFileDNE() throws Exception {
@@ -183,7 +187,7 @@ public void testExtraArguments() throws Exception {
         terminal.addSecretInput(password);
         UserException e = expectThrows(UserException.class, () -> execute("foo", file.toString(), "bar"));
         assertEquals(e.getMessage(), ExitCodes.USAGE, e.exitCode);
-        assertThat(e.getMessage(), containsString("Unrecognized extra arguments [bar]"));
+        assertThat(e.getMessage(), containsString("settings and filenames must come in pairs"));
     }
 
     public void testIncorrectPassword() throws Exception {
@@ -216,4 +220,20 @@ public void testAddToUnprotectedKeystore() throws Exception {
         execute("foo", "path/dne");
         assertSecureFile("foo", file, password);
     }
+
+    public void testAddMultipleFiles() throws Exception {
+        final String password = "keystorepassword";
+        createKeystore(password);
+        final int n = randomIntBetween(1, 8);
+        final List<Tuple<String, Path>> settingFilePairs = new ArrayList<>(n);
+        for (int i = 0; i < n; i++) {
+            settingFilePairs.add(Tuple.tuple("foo" + i, createRandomFile()));
+        }
+        terminal.addSecretInput(password);
+        execute(settingFilePairs.stream().flatMap(t -> Stream.of(t.v1(), t.v2().toString())).toArray(String[]::new));
+        for (int i = 0; i < n; i++) {
+            assertSecureFile(settingFilePairs.get(i).v1(), settingFilePairs.get(i).v2(), password);
+        }
+    }
+
 }