do not prompt for node name twice

We allow setting the node's name a few different ways: the `name` system
property, the setting `name`, and the setting `node.name`. There is an order
of preference to these settings that gets applied, which can copy values from the
system property or `node.name` setting to the `name` setting. When setting
only `node.name` to one of the prompt placeholders, the user would be
prompted twice as the value of `node.name` is copied to `name` prior to
prompting for input. Additionally, the value entered by the user for `node.name`
would not be used and only the value entered for `name` would be used.

This fix changes the behavior to only prompt once when `node.name is set` and
`name` is not set. This is accomplished by waiting until all values have been
prompted and replaced, then the logic for determining the node's name is
executed.

Closes #11564

src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java

@@ -50,8 +50,8 @@ public class InternalSettingsPreparer {
 
     /**
      * Prepares the settings by gathering all elasticsearch system properties, optionally loading the configuration settings,
-     * and then replacing all property placeholders. This method will not work with settings that have <code>__prompt__</code>
-     * as their value unless they have been resolved previously.
+     * and then replacing all property placeholders. This method will not work with settings that have <code>${prompt.text}</code>
+     * or <code>${prompt.secret}</code> as their value unless they have been resolved previously.
      * @param pSettings The initial settings to use
      * @param loadConfigSettings flag to indicate whether to load settings from the configuration directory/file
      * @return the {@link Settings} and {@link Environment} as a {@link Tuple}
@@ -63,7 +63,8 @@ public static Tuple<Settings, Environment> prepareSettings(Settings pSettings, b
     /**
      * Prepares the settings by gathering all elasticsearch system properties, optionally loading the configuration settings,
      * and then replacing all property placeholders. If a {@link Terminal} is provided and configuration settings are loaded,
-     * settings with the <code>__prompt__</code> value will result in a prompt for the setting to the user.
+     * settings with a value of <code>${prompt.text}</code> or <code>${prompt.secret}</code> will result in a prompt for
+     * the setting to the user.
      * @param pSettings The initial settings to use
      * @param loadConfigSettings flag to indicate whether to load settings from the configuration directory/file
      * @param terminal the Terminal to use for input/output
@@ -131,16 +132,9 @@ public static Tuple<Settings, Environment> prepareSettings(Settings pSettings, b
         }
         settingsBuilder.replacePropertyPlaceholders();
 
-        // generate the name
+        // check if name is set in settings, if not look for system property and set it
         if (settingsBuilder.get("name") == null) {
             String name = System.getProperty("name");
-            if (name == null || name.isEmpty()) {
-                name = settingsBuilder.get("node.name");
-                if (name == null || name.isEmpty()) {
-                    name = Names.randomNodeName(environment.resolveConfig("names.txt"));
-                }
-            }
-
             if (name != null) {
                 settingsBuilder.put("name", name);
             }
@@ -151,17 +145,32 @@ public static Tuple<Settings, Environment> prepareSettings(Settings pSettings, b
             settingsBuilder.put(ClusterName.SETTING, ClusterName.DEFAULT.value());
         }
 
-        Settings v1 = replacePromptPlaceholders(settingsBuilder.build(), terminal);
-        environment = new Environment(v1);
+        Settings settings = replacePromptPlaceholders(settingsBuilder.build(), terminal);
+        // all settings placeholders have been resolved. resolve the value for the name setting by checking for name,
+        // then looking for node.name, and finally generate one if needed
+        if (settings.get("name") == null) {
+            final String name = settings.get("node.name");
+            if (name == null || name.isEmpty()) {
+                settings = settingsBuilder().put(settings)
+                        .put("name", Names.randomNodeName(environment.resolveConfig("names.txt")))
+                        .build();
+            } else {
+                settings = settingsBuilder().put(settings)
+                        .put("name", name)
+                        .build();
+            }
+        }
+
+        environment = new Environment(settings);
 
         // put back the env settings
-        settingsBuilder = settingsBuilder().put(v1);
+        settingsBuilder = settingsBuilder().put(settings);
         // we put back the path.logs so we can use it in the logging configuration file
         settingsBuilder.put("path.logs", cleanPath(environment.logsFile().getAbsolutePath()));
 
-        v1 = settingsBuilder.build();
+        settings = settingsBuilder.build();
 
-        return new Tuple<>(v1, environment);
+        return new Tuple<>(settings, environment);
     }
 
     static Settings replacePromptPlaceholders(Settings settings, Terminal terminal) {

src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.common.cli.CliToolTestCase;
 import org.elasticsearch.common.cli.Terminal;
 import org.elasticsearch.common.collect.Tuple;
+import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.test.ElasticsearchTestCase;
@@ -31,22 +32,27 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.notNullValue;
 
 public class InternalSettingsPreparerTests extends ElasticsearchTestCase {
 
     @Before
     public void setupSystemProperties() {
         System.setProperty("es.node.zone", "foo");
+        System.setProperty("name", "sys-prop-name");
     }
 
     @After
     public void cleanupSystemProperties() {
         System.clearProperty("es.node.zone");
+        System.clearProperty("name");
     }
 
     @Test
@@ -145,4 +151,72 @@ public void testReplaceTextPromptPlaceholderWithNullTerminal() {
             assertThat(e.getMessage(), containsString("with value [" + InternalSettingsPreparer.TEXT_PROMPT_VALUE + "]"));
         }
     }
+
+    @Test
+    public void testNameSettingsPreference() {
+        // Test system property overrides node.name
+        Settings settings = settingsBuilder()
+                .put("node.name", "node-name")
+                .put("path.home", newTempDir())
+                .build();
+        Tuple<Settings, Environment> tuple = InternalSettingsPreparer.prepareSettings(settings, true);
+        assertThat(tuple.v1().get("name"), equalTo("sys-prop-name"));
+
+        // test name in settings overrides sys prop and node.name
+        settings = settingsBuilder()
+                .put("name", "name-in-settings")
+                .put("node.name", "node-name")
+                .put("path.home", newTempDir())
+                .build();
+        tuple = InternalSettingsPreparer.prepareSettings(settings, true);
+        assertThat(tuple.v1().get("name"), equalTo("name-in-settings"));
+
+        // test only node.name in settings
+        System.clearProperty("name");
+        settings = settingsBuilder()
+                .put("node.name", "node-name")
+                .put("path.home", newTempDir())
+                .build();
+        tuple = InternalSettingsPreparer.prepareSettings(settings, true);
+        assertThat(tuple.v1().get("name"), equalTo("node-name"));
+
+        // test no name at all results in name being set
+        settings = settingsBuilder()
+                .put("path.home", newTempDir())
+                .build();
+        tuple = InternalSettingsPreparer.prepareSettings(settings, true);
+        assertThat(tuple.v1().get("name"), not("name-in-settings"));
+        assertThat(tuple.v1().get("name"), not("sys-prop-name"));
+        assertThat(tuple.v1().get("name"), not("node-name"));
+        assertThat(tuple.v1().get("name"), notNullValue());
+    }
+
+    @Test
+    public void testPromptForNodeNameOnlyPromptsOnce() {
+        final AtomicInteger counter = new AtomicInteger();
+        final Terminal terminal = new CliToolTestCase.MockTerminal() {
+            @Override
+            public char[] readSecret(String message, Object... args) {
+                fail("readSecret should never be called by this test");
+                return null;
+            }
+
+            @Override
+            public String readText(String message, Object... args) {
+                int count = counter.getAndIncrement();
+                return "prompted name " + count;
+            }
+        };
+
+        System.clearProperty("name");
+        Settings settings = ImmutableSettings.builder()
+                .put("path.home", newTempDir())
+                .put("node.name", InternalSettingsPreparer.TEXT_PROMPT_VALUE)
+                .build();
+        Tuple<Settings, Environment> tuple = InternalSettingsPreparer.prepareSettings(settings, false, terminal);
+        settings = tuple.v1();
+        assertThat(counter.intValue(), is(1));
+        assertThat(settings.get("name"), is("prompted name 0"));
+        assertThat(settings.get("node.name"), is("prompted name 0"));
+    }
 }