Merge branch 'bootstrapping-bootstrap'

* bootstrapping-bootstrap:
  Output all failing bootstrap checks
  Move bootstrap checks to node start

Closes #17595

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,19 @@ 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;
+import java.util.stream.Collectors;
 
 /**
  * We enforce limits once any network host is configured. In this case we assume the node is running in production
@@ -54,10 +53,11 @@ final class BootstrapCheck {
      * checks the current limits against the snapshot or release build
      * checks
      *
-     * @param settings the current node settings
+     * @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,50 +67,42 @@ 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 List<String> errors =
-            final boolean fail = check.check();
+                checks.stream()
-            if (fail) {
+                        .filter(BootstrapCheck.Check::check)
-                if (enforceLimits) {
+                        .map(BootstrapCheck.Check::errorMessage)
-                    throw new RuntimeException(check.errorMessage());
+                        .collect(Collectors.toList());
-                } else {
+
-                    logger.warn(check.errorMessage());
+        if (!errors.isEmpty()) {
-                }
+            final List<String> messages = new ArrayList<>(1 + errors.size());
+            messages.add("bootstrap checks failed");
+            messages.addAll(errors);
+            if (enforceLimits) {
+                final RuntimeException re = new RuntimeException(String.join("\n", messages));
+                errors.stream().map(IllegalStateException::new).forEach(re::addSuppressed);
+                throw re;
+            } else {
+                messages.forEach(message -> logger.warn(message));
             }
         }
     }
 
-    /**
-     * 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,121 @@
 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.allOf;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.Matchers.hasToString;
 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 testExceptionAggregation() {
+        final List<BootstrapCheck.Check> checks = Arrays.asList(
+                new BootstrapCheck.Check() {
+                    @Override
+                    public boolean check() {
+                        return true;
+                    }
+
+                    @Override
+                    public String errorMessage() {
+                        return "first";
+                    }
+                },
+                new BootstrapCheck.Check() {
+                    @Override
+                    public boolean check() {
+                        return true;
+                    }
+
+                    @Override
+                    public String errorMessage() {
+                        return "second";
+                    }
+                }
+        );
+        final RuntimeException e =
+                expectThrows(RuntimeException.class, () -> BootstrapCheck.check(true, checks, "testExceptionAggregation"));
+        assertThat(e, hasToString(allOf(containsString("bootstrap checks failed"), containsString("first"), containsString("second"))));
+        final Throwable[] suppressed = e.getSuppressed();
+        assertThat(suppressed.length, equalTo(2));
+        assertThat(suppressed[0], instanceOf(IllegalStateException.class));
+        assertThat(suppressed[0], hasToString(containsString("first")));
+        assertThat(suppressed[1], instanceOf(IllegalStateException.class));
+        assertThat(suppressed[1], hasToString(containsString("second")));
     }
 
     public void testFileDescriptorLimits() {
@@ -64,7 +159,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 +167,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 +214,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 +223,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 +239,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 +247,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 +271,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 +279,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 +293,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"));
     }
+
 }