Move bootstrap checks to node start

This commit moves the execution of the bootstrap checks to after network
services are started. This gives us the flexibility to not merely check
if any of the network settings are set, but instead be smarter about it
and check if the network settings are set in a way that means that the
node will be communicating over an external network (either directly, or
via a proxy). As an bonanza, executing the bootstrap checks in this way
enables us to have the node name in the logs!

Closes #17570

core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -33,6 +33,7 @@ import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.LogConfigurator;
 import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.transport.BoundTransportAddress;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.monitor.jvm.JvmInfo;
 import org.elasticsearch.monitor.os.OsProbe;
@@ -184,9 +185,12 @@ final class Bootstrap {
                 .put(InternalSettingsPreparer.IGNORE_SYSTEM_PROPERTIES_SETTING.getKey(), true)
                 .build();
 
-        BootstrapCheck.check(nodeSettings);
+        node = new Node(nodeSettings) {
-
+            @Override
-        node = new Node(nodeSettings);
+            protected void validateNodeBeforeAcceptingRequests(Settings settings, BoundTransportAddress boundTransportAddress) {
+                BootstrapCheck.check(settings, boundTransportAddress);
+            }
+        };
     }
 
     private static Environment initialSettings(boolean foreground, String pidFile) {

core/src/main/java/org/elasticsearch/bootstrap/BootstrapCheck.java

@@ -22,20 +22,18 @@ package org.elasticsearch.bootstrap;
 import org.apache.lucene.util.Constants;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.Loggers;
-import org.elasticsearch.common.network.NetworkService;
-import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.transport.BoundTransportAddress;
+import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.discovery.zen.elect.ElectMasterService;
 import org.elasticsearch.monitor.process.ProcessProbe;
-import org.elasticsearch.transport.TransportSettings;
+import org.elasticsearch.node.Node;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
-import java.util.Set;
 
 /**
  * We enforce limits once any network host is configured. In this case we assume the node is running in production
@@ -55,9 +53,10 @@ final class BootstrapCheck {
      * checks
      *
      * @param settings              the current node settings
+     * @param boundTransportAddress the node network bindings
      */
-    public static void check(final Settings settings) {
+    static void check(final Settings settings, final BoundTransportAddress boundTransportAddress) {
-        check(enforceLimits(settings), checks(settings));
+        check(enforceLimits(boundTransportAddress), checks(settings), Node.NODE_NAME_SETTING.get(settings));
     }
 
     /**
@@ -67,10 +66,11 @@ final class BootstrapCheck {
      * @param enforceLimits true if the checks should be enforced or
      *                      warned
      * @param checks        the checks to execute
+     * @param nodeName      the node name to be used as a logging prefix
      */
     // visible for testing
-    static void check(final boolean enforceLimits, final List<Check> checks) {
+    static void check(final boolean enforceLimits, final List<Check> checks, final String nodeName) {
-        final ESLogger logger = Loggers.getLogger(BootstrapCheck.class);
+        final ESLogger logger = Loggers.getLogger(BootstrapCheck.class, nodeName);
 
         for (final Check check : checks) {
             final boolean fail = check.check();
@@ -84,33 +84,16 @@ final class BootstrapCheck {
         }
     }
 
-    /**
-     * The set of settings such that if any are set for the node, then
-     * the checks are enforced
-     *
-     * @return the enforcement settings
-     */
-    // visible for testing
-    static Set<Setting> enforceSettings() {
-        return Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
-            TransportSettings.BIND_HOST,
-            TransportSettings.HOST,
-            TransportSettings.PUBLISH_HOST,
-            NetworkService.GLOBAL_NETWORK_HOST_SETTING,
-            NetworkService.GLOBAL_NETWORK_BINDHOST_SETTING,
-            NetworkService.GLOBAL_NETWORK_PUBLISHHOST_SETTING
-        )));
-    }
-
     /**
      * Tests if the checks should be enforced
      *
-     * @param settings the current node settings
+     * @param boundTransportAddress the node network bindings
      * @return true if the checks should be enforced
      */
     // visible for testing
-    static boolean enforceLimits(final Settings settings) {
+    static boolean enforceLimits(BoundTransportAddress boundTransportAddress) {
-        return enforceSettings().stream().anyMatch(s -> s.exists(settings));
+        return !(Arrays.stream(boundTransportAddress.boundAddresses()).allMatch(TransportAddress::isLoopbackOrLinkLocalAddress) &&
+                boundTransportAddress.publishAddress().isLoopbackOrLinkLocalAddress());
     }
 
     // the list of checks to execute

core/src/main/java/org/elasticsearch/common/transport/DummyTransportAddress.java

@@ -44,6 +44,11 @@ public class DummyTransportAddress implements TransportAddress {
         return other == INSTANCE;
     }
 
+    @Override
+    public boolean isLoopbackOrLinkLocalAddress() {
+        return false;
+    }
+
     @Override
     public String getHost() {
         return "dummy";

core/src/main/java/org/elasticsearch/common/transport/InetSocketTransportAddress.java

@@ -74,6 +74,11 @@ public final class InetSocketTransportAddress implements TransportAddress {
                 address.getAddress().equals(((InetSocketTransportAddress) other).address.getAddress());
     }
 
+    @Override
+    public boolean isLoopbackOrLinkLocalAddress() {
+        return address.getAddress().isLinkLocalAddress() || address.getAddress().isLoopbackAddress();
+    }
+
     @Override
     public String getHost() {
        return getAddress(); // just delegate no resolving

core/src/main/java/org/elasticsearch/common/transport/LocalTransportAddress.java

@@ -55,6 +55,11 @@ public final class LocalTransportAddress implements TransportAddress {
         return other instanceof LocalTransportAddress && id.equals(((LocalTransportAddress) other).id);
     }
 
+    @Override
+    public boolean isLoopbackOrLinkLocalAddress() {
+        return false;
+    }
+
     @Override
     public String getHost() {
         return "local";

core/src/main/java/org/elasticsearch/common/transport/TransportAddress.java

@@ -46,5 +46,7 @@ public interface TransportAddress extends Writeable<TransportAddress> {
 
     boolean sameHost(TransportAddress other);
 
+    boolean isLoopbackOrLinkLocalAddress();
+
     public String toString();
 }

core/src/main/java/org/elasticsearch/node/Node.java

@@ -266,10 +266,6 @@ public class Node implements Closeable {
      * Start the node. If the node is already started, this method is no-op.
      */
     public Node start() {
-        if (!lifecycle.moveToStarted()) {
-            return this;
-        }
-
         ESLogger logger = Loggers.getLogger(Node.class, NODE_NAME_SETTING.get(settings));
         logger.info("starting ...");
         // hack around dependency injection problem (for now...)
@@ -308,10 +304,12 @@ public class Node implements Closeable {
         final TribeService tribeService = injector.getInstance(TribeService.class);
         tribeService.start();
 
-
         // Start the transport service now so the publish address will be added to the local disco node in ClusterService
         TransportService transportService = injector.getInstance(TransportService.class);
         transportService.start();
+
+        validateNodeBeforeAcceptingRequests(settings, transportService.boundAddress());
+
         DiscoveryNode localNode = injector.getInstance(DiscoveryNodeService.class)
                 .buildLocalNode(transportService.boundAddress().publishAddress());
 
@@ -381,6 +379,9 @@ public class Node implements Closeable {
     }
 
     private Node stop() {
+        if (lifecycle.moveToStarted()) {
+            return this;
+        }
         if (!lifecycle.moveToStopped()) {
             return this;
         }
@@ -521,6 +522,20 @@ public class Node implements Closeable {
         return this.injector;
     }
 
+    /**
+     * Hook for validating the node after network
+     * services are started but before the cluster service is started
+     * and before the network service starts accepting incoming network
+     * requests.
+     *
+     * @param settings              the fully-resolved settings
+     * @param boundTransportAddress the network addresses the node is
+     *                              bound and publishing to
+     */
+    @SuppressWarnings("unused")
+    protected void validateNodeBeforeAcceptingRequests(Settings settings, BoundTransportAddress boundTransportAddress) {
+    }
+
     /** Writes a file to the logs dir containing the ports for the given transport type */
     private void writePortsFile(String type, BoundTransportAddress boundAddress) {
         Path tmpPortsFile = environment.logsFile().resolve(type + ".ports.tmp");

core/src/test/java/org/elasticsearch/bootstrap/BootstrapCheckTests.java

@@ -20,26 +20,81 @@
 package org.elasticsearch.bootstrap;
 
 import org.apache.lucene.util.Constants;
-import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.transport.BoundTransportAddress;
+import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.Matchers.not;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class BootstrapCheckTests extends ESTestCase {
 
     public void testNonProductionMode() {
         // nothing should happen since we are in non-production mode
-        BootstrapCheck.check(Settings.EMPTY);
+        final List<TransportAddress> transportAddresses = new ArrayList<>();
+        for (int i = 0; i < randomIntBetween(1, 8); i++) {
+            TransportAddress localTransportAddress = mock(TransportAddress.class);
+            when(localTransportAddress.isLoopbackOrLinkLocalAddress()).thenReturn(true);
+            transportAddresses.add(localTransportAddress);
+        }
+
+        TransportAddress publishAddress = mock(TransportAddress.class);
+        when(publishAddress.isLoopbackOrLinkLocalAddress()).thenReturn(true);
+        BoundTransportAddress boundTransportAddress = mock(BoundTransportAddress.class);
+        when(boundTransportAddress.boundAddresses()).thenReturn(transportAddresses.toArray(new TransportAddress[0]));
+        when(boundTransportAddress.publishAddress()).thenReturn(publishAddress);
+        BootstrapCheck.check(Settings.EMPTY, boundTransportAddress);
+    }
+
+    public void testEnforceLimitsWhenBoundToNonLocalAddress() {
+        final List<TransportAddress> transportAddresses = new ArrayList<>();
+        final TransportAddress nonLocalTransportAddress = mock(TransportAddress.class);
+        when(nonLocalTransportAddress.isLoopbackOrLinkLocalAddress()).thenReturn(false);
+        transportAddresses.add(nonLocalTransportAddress);
+
+        for (int i = 0; i < randomIntBetween(0, 7); i++) {
+            final TransportAddress randomTransportAddress = mock(TransportAddress.class);
+            when(randomTransportAddress.isLoopbackOrLinkLocalAddress()).thenReturn(randomBoolean());
+            transportAddresses.add(randomTransportAddress);
+        }
+
+        final TransportAddress publishAddress = mock(TransportAddress.class);
+        when(publishAddress.isLoopbackOrLinkLocalAddress()).thenReturn(randomBoolean());
+
+        final BoundTransportAddress boundTransportAddress = mock(BoundTransportAddress.class);
+        Collections.shuffle(transportAddresses, random());
+        when(boundTransportAddress.boundAddresses()).thenReturn(transportAddresses.toArray(new TransportAddress[0]));
+        when(boundTransportAddress.publishAddress()).thenReturn(publishAddress);
+
+        assertTrue(BootstrapCheck.enforceLimits(boundTransportAddress));
+    }
+
+    public void testEnforceLimitsWhenPublishingToNonLocalAddress() {
+        final List<TransportAddress> transportAddresses = new ArrayList<>();
+
+        for (int i = 0; i < randomIntBetween(1, 8); i++) {
+            final TransportAddress randomTransportAddress = mock(TransportAddress.class);
+            when(randomTransportAddress.isLoopbackOrLinkLocalAddress()).thenReturn(false);
+            transportAddresses.add(randomTransportAddress);
+        }
+
+        final TransportAddress publishAddress = mock(TransportAddress.class);
+        when(publishAddress.isLoopbackOrLinkLocalAddress()).thenReturn(true);
+
+        final BoundTransportAddress boundTransportAddress = mock(BoundTransportAddress.class);
+        when(boundTransportAddress.boundAddresses()).thenReturn(transportAddresses.toArray(new TransportAddress[0]));
+        when(boundTransportAddress.publishAddress()).thenReturn(publishAddress);
+
+        assertTrue(BootstrapCheck.enforceLimits(boundTransportAddress));
     }
 
     public void testFileDescriptorLimits() {
@@ -64,7 +119,7 @@ public class BootstrapCheckTests extends ESTestCase {
         }
 
         try {
-            BootstrapCheck.check(true, Collections.singletonList(check));
+            BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits");
             fail("should have failed due to max file descriptors too low");
         } catch (final RuntimeException e) {
             assertThat(e.getMessage(), containsString("max file descriptors"));
@@ -72,12 +127,12 @@ public class BootstrapCheckTests extends ESTestCase {
 
         maxFileDescriptorCount.set(randomIntBetween(limit + 1, Integer.MAX_VALUE));
 
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits");
 
         // nothing should happen if current file descriptor count is
         // not available
         maxFileDescriptorCount.set(-1);
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimits");
     }
 
     public void testFileDescriptorLimitsThrowsOnInvalidLimit() {
@@ -119,7 +174,7 @@ public class BootstrapCheckTests extends ESTestCase {
 
             if (testCase.shouldFail) {
                 try {
-                    BootstrapCheck.check(true, Collections.singletonList(check));
+                    BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimitsThrowsOnInvalidLimit");
                     fail("should have failed due to memory not being locked");
                 } catch (final RuntimeException e) {
                     assertThat(
@@ -128,7 +183,7 @@ public class BootstrapCheckTests extends ESTestCase {
                 }
             } else {
                 // nothing should happen
-                BootstrapCheck.check(true, Collections.singletonList(check));
+                BootstrapCheck.check(true, Collections.singletonList(check), "testFileDescriptorLimitsThrowsOnInvalidLimit");
             }
         }
     }
@@ -144,7 +199,7 @@ public class BootstrapCheckTests extends ESTestCase {
         };
 
         try {
-            BootstrapCheck.check(true, Collections.singletonList(check));
+            BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck");
             fail("should have failed due to max number of threads too low");
         } catch (final RuntimeException e) {
             assertThat(e.getMessage(), containsString("max number of threads"));
@@ -152,12 +207,12 @@ public class BootstrapCheckTests extends ESTestCase {
 
         maxNumberOfThreads.set(randomIntBetween(limit + 1, Integer.MAX_VALUE));
 
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck");
 
         // nothing should happen if current max number of threads is
         // not available
         maxNumberOfThreads.set(-1);
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testMaxNumberOfThreadsCheck");
     }
 
     public void testMaxSizeVirtualMemory() {
@@ -176,7 +231,7 @@ public class BootstrapCheckTests extends ESTestCase {
         };
 
         try {
-            BootstrapCheck.check(true, Collections.singletonList(check));
+            BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory");
             fail("should have failed due to max size virtual memory too low");
         } catch (final RuntimeException e) {
             assertThat(e.getMessage(), containsString("max size virtual memory"));
@@ -184,19 +239,12 @@ public class BootstrapCheckTests extends ESTestCase {
 
         maxSizeVirtualMemory.set(rlimInfinity);
 
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory");
 
         // nothing should happen if max size virtual memory is not
         // available
         maxSizeVirtualMemory.set(Long.MIN_VALUE);
-        BootstrapCheck.check(true, Collections.singletonList(check));
+        BootstrapCheck.check(true, Collections.singletonList(check), "testMaxSizeVirtualMemory");
-    }
-
-    public void testEnforceLimits() {
-        final Set<Setting> enforceSettings = BootstrapCheck.enforceSettings();
-        final Setting setting = randomFrom(Arrays.asList(enforceSettings.toArray(new Setting[enforceSettings.size()])));
-        final Settings settings = Settings.builder().put(setting.getKey(), randomAsciiOfLength(8)).build();
-        assertTrue(BootstrapCheck.enforceLimits(settings));
     }
 
     public void testMinMasterNodes() {
@@ -205,6 +253,7 @@ public class BootstrapCheckTests extends ESTestCase {
         assertThat(check.check(), not(equalTo(isSet)));
         List<BootstrapCheck.Check> defaultChecks = BootstrapCheck.checks(Settings.EMPTY);
 
-        expectThrows(RuntimeException.class, () -> BootstrapCheck.check(true, defaultChecks));
+        expectThrows(RuntimeException.class, () -> BootstrapCheck.check(true, defaultChecks, "testMinMasterNodes"));
     }
+
 }