Merge pull request #11672 from vespa-engine/bratseth/validate-hostnames

Validate hostnames on setup

6 files changed, 88 insertions, 48 deletions

diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java
index e95044ce6d2..0e7488c8927 100644
--- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java
+++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java
@@ -27,6 +27,8 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -75,7 +77,7 @@ class ApacheGatewayConnection implements GatewayConnection {
             HttpClientFactory httpClientFactory,
             String clientId) {
         SUPPORTED_VERSIONS.add(3);
-        this.endpoint = endpoint;
+        this.endpoint = validate(endpoint);
         this.feedParams = feedParams;
         this.clusterSpecificRoute = clusterSpecificRoute;
         this.httpClientFactory = httpClientFactory;
@@ -90,8 +92,17 @@ class ApacheGatewayConnection implements GatewayConnection {
             endOfFeed = END_OF_FEED_XML;
         }
         this.clientId = clientId;
-        if (this.clientId == null) {
-            throw new RuntimeException("Got no client Id.");
+        if (this.clientId == null)
+            throw new IllegalArgumentException("Got no client Id.");
+    }
+
+    private static Endpoint validate(Endpoint endpoint) {
+        try {
+            InetAddress.getByName(endpoint.getHostname());
+            return endpoint;
+        }
+        catch (UnknownHostException e) {
+            throw new IllegalArgumentException("Unknown host: " + endpoint);
         }
     }
 
@@ -390,7 +401,7 @@ class ApacheGatewayConnection implements GatewayConnection {
         final ConnectionParams connectionParams;
         final boolean useSsl;
 
-        public HttpClientFactory(final ConnectionParams connectionParams, final boolean useSsl) {
+        public HttpClientFactory(ConnectionParams connectionParams, boolean useSsl) {
             this.connectionParams = connectionParams;
             this.useSsl = useSsl;
         }
@@ -426,14 +437,12 @@ class ApacheGatewayConnection implements GatewayConnection {
             clientBuilder.disableContentCompression();
             // Try to disable the disabling to see if system tests become stable again.
             // clientBuilder.disableAutomaticRetries();
-            {
-                RequestConfig.Builder requestConfigBuilder = RequestConfig.custom();
-                requestConfigBuilder.setSocketTimeout(0);
-                if (connectionParams.getProxyHost() != null) {
-                    requestConfigBuilder.setProxy(new HttpHost(connectionParams.getProxyHost(), connectionParams.getProxyPort()));
-                }
-                clientBuilder.setDefaultRequestConfig(requestConfigBuilder.build());
+            RequestConfig.Builder requestConfigBuilder = RequestConfig.custom();
+            requestConfigBuilder.setSocketTimeout(0);
+            if (connectionParams.getProxyHost() != null) {
+                requestConfigBuilder.setProxy(new HttpHost(connectionParams.getProxyHost(), connectionParams.getProxyPort()));
             }
+            clientBuilder.setDefaultRequestConfig(requestConfigBuilder.build());
 
             log.fine("Creating HttpClient: " + " ConnectionTimeout "
                             + " SocketTimeout 0 secs "
diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java
index 643b7641e68..77ed8464284 100644
--- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java
+++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/IOThread.java
@@ -298,6 +298,7 @@ class IOThread implements Runnable, AutoCloseable {
         return processResponse;
     }
 
+    /** Given a current thread state, take the appropriate action and return the resulting new thread state */
     private ThreadState cycle(ThreadState threadState) {
         switch(threadState) {
             case DISCONNECTED:
diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/FeedClientTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/FeedClientTest.java
index 88deaa07e12..aa47128f436 100644
--- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/FeedClientTest.java
+++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/FeedClientTest.java
@@ -27,7 +27,7 @@ public class FeedClientTest {
 
     SessionParams sessionParams = new SessionParams.Builder()
             .addCluster(new Cluster.Builder()
-                    .addEndpoint(Endpoint.create("hostname"))
+                    .addEndpoint(Endpoint.create("localhost"))
                     .build())
             .setConnectionParams(new ConnectionParams.Builder()
                     .setDryRun(true)
diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/SyncFeedClientTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/SyncFeedClientTest.java
index 5a93fdbf9e1..6e2c6cafc9b 100644
--- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/SyncFeedClientTest.java
+++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/SyncFeedClientTest.java
@@ -30,7 +30,7 @@ public class SyncFeedClientTest {
     public void testFeedJson() {
         SessionParams sessionParams = new SessionParams.Builder()
                                               .addCluster(new Cluster.Builder()
-                                                                  .addEndpoint(Endpoint.create("hostname"))
+                                                                  .addEndpoint(Endpoint.create("localhost"))
                                                                   .build())
                                               .setConnectionParams(new ConnectionParams.Builder()
                                                                            .setDryRun(true)
diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnectionTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnectionTest.java
index d49f4093a06..59a8b613e67 100644
--- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnectionTest.java
+++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnectionTest.java
@@ -50,14 +50,14 @@ public class ApacheGatewayConnectionTest {
 
     @Test
     public void testProtocolV3() throws Exception {
-        final Endpoint endpoint = Endpoint.create("hostname", 666, false);
+        final Endpoint endpoint = Endpoint.create("localhost", 666, false);
         final FeedParams feedParams = new FeedParams.Builder().setDataFormat(FeedParams.DataFormat.JSON_UTF8).build();
         final String clusterSpecificRoute = "";
         final ConnectionParams connectionParams = new ConnectionParams.Builder()
                 .build();
         final List<Document> documents = new ArrayList<>();
 
-        final String vespaDocContent ="Hello, I a JSON doc.";
+        final String vespaDocContent = "Hello, I a JSON doc.";
         final String docId = "42";
 
         final AtomicInteger requestsReceived = new AtomicInteger(0);
@@ -84,7 +84,7 @@ public class ApacheGatewayConnectionTest {
 
     @Test(expected=IllegalArgumentException.class)
     public void testServerReturnsBadSessionInV3() throws Exception {
-        final Endpoint endpoint = Endpoint.create("hostname", 666, false);
+        final Endpoint endpoint = Endpoint.create("localhost", 666, false);
         final FeedParams feedParams = new FeedParams.Builder().setDataFormat(FeedParams.DataFormat.JSON_UTF8).build();
         final String clusterSpecificRoute = "";
         final ConnectionParams connectionParams = new ConnectionParams.Builder()
@@ -108,7 +108,7 @@ public class ApacheGatewayConnectionTest {
 
     @Test(expected=RuntimeException.class)
     public void testBadConfigParameters() throws Exception {
-            final Endpoint endpoint = Endpoint.create("hostname", 666, false);
+            final Endpoint endpoint = Endpoint.create("localhost", 666, false);
         final FeedParams feedParams = new FeedParams.Builder().setDataFormat(FeedParams.DataFormat.JSON_UTF8).build();
         final String clusterSpecificRoute = "";
         final ConnectionParams connectionParams = new ConnectionParams.Builder()
@@ -128,7 +128,7 @@ public class ApacheGatewayConnectionTest {
 
     @Test
     public void testJsonDocumentHeader() throws Exception {
-        final Endpoint endpoint = Endpoint.create("hostname", 666, false);
+        final Endpoint endpoint = Endpoint.create("localhost", 666, false);
         final FeedParams feedParams = new FeedParams.Builder().setDataFormat(FeedParams.DataFormat.JSON_UTF8).build();
         final String clusterSpecificRoute = "";
         final ConnectionParams connectionParams = new ConnectionParams.Builder()
@@ -187,7 +187,7 @@ public class ApacheGatewayConnectionTest {
      */
     @Test
     public void testCompressedWriteOperations() throws Exception {
-        final Endpoint endpoint = Endpoint.create("hostname", 666, false);
+        final Endpoint endpoint = Endpoint.create("localhost", 666, false);
         final FeedParams feedParams = new FeedParams.Builder().setDataFormat(FeedParams.DataFormat.XML_UTF8).build();
         final String clusterSpecificRoute = "";
         final ConnectionParams connectionParams = new ConnectionParams.Builder()
@@ -237,7 +237,7 @@ public class ApacheGatewayConnectionTest {
     }
 
     @Test
-    public void dynamic_headers_are_added_to_the_response() throws IOException, ServerResponseException, InterruptedException {
+    public void dynamic_headers_are_added_to_the_response() throws IOException, ServerResponseException {
         ConnectionParams.HeaderProvider headerProvider = mock(ConnectionParams.HeaderProvider.class);
         when(headerProvider.getHeaderValue())
                 .thenReturn("v1")
@@ -260,7 +260,7 @@ public class ApacheGatewayConnectionTest {
 
         ApacheGatewayConnection apacheGatewayConnection =
             new ApacheGatewayConnection(
-                    Endpoint.create("hostname", 666, false),
+                    Endpoint.create("localhost", 666, false),
                     new FeedParams.Builder().build(),
                     "",
                     connectionParams,
@@ -288,7 +288,7 @@ public class ApacheGatewayConnectionTest {
 
         ApacheGatewayConnection apacheGatewayConnection =
             new ApacheGatewayConnection(
-                    Endpoint.create("hostname", 666, false),
+                    Endpoint.create("localhost", 666, false),
                     new FeedParams.Builder().build(),
                     "",
                     new ConnectionParams.Builder().build(),
@@ -379,4 +379,5 @@ public class ApacheGatewayConnectionTest {
         when(response.getEntity()).thenReturn(httpEntity);
         return response;
     }
+
 }
diff --git a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessorTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessorTest.java
index 78f616d0e36..0c636ba804e 100644
--- a/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessorTest.java
+++ b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessorTest.java
@@ -19,8 +19,11 @@ import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.mock;
@@ -40,10 +43,10 @@ public class OperationProcessorTest {
     @Test
     public void testBasic() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .build();
 
         OperationProcessor q = new OperationProcessor(
@@ -53,26 +56,26 @@ public class OperationProcessorTest {
 
 
         q.resultReceived(new EndpointResult("foo", new Result.Detail(null)), 0);
-        assertThat(queue.size(), is(0));
+        assertEquals(0, queue.size());
 
 
         q.sendDocument(doc1);
-        assertThat(queue.size(), is(0));
+        assertEquals(0, queue.size());
 
         q.resultReceived(new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("a"))), 0);
-        assertThat(queue.size(), is(0));
+        assertEquals(0, queue.size());
 
         q.resultReceived(new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("b"))), 1);
-        assertThat(queue.size(), is(0));
+        assertEquals(0, queue.size());
 
         q.resultReceived(new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("c"))), 2);
-        assertThat(queue.size(), is(0));
+        assertEquals(0, queue.size());
 
         q.resultReceived(new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("d"))), 3);
-        assertThat(queue.size(), is(1));
+        assertEquals(1, queue.size());
 
         q.resultReceived(new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("e"))), 0);
-        assertThat(queue.size(), is(1));
+        assertEquals(1, queue.size());
 
         //check a, b, c, d
         Result aggregated = queue.poll();
@@ -121,8 +124,8 @@ public class OperationProcessorTest {
     @Test
     public void testBlockingOfOperationsTwoEndpoints() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .setConnectionParams(new ConnectionParams.Builder().build())
                 .build();
         OperationProcessor operationProcessor = new OperationProcessor(
@@ -159,7 +162,7 @@ public class OperationProcessorTest {
     @Test
     public void testBlockingOfOperationsToSameDocIdWithTwoOperations() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .setConnectionParams(new ConnectionParams.Builder().build())
                 .build();
 
@@ -192,7 +195,7 @@ public class OperationProcessorTest {
     @Test
     public void testBlockingOfOperationsToSameDocIdMany() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .setConnectionParams(new ConnectionParams.Builder().build())
                 .build();
 
@@ -225,7 +228,7 @@ public class OperationProcessorTest {
 
     @Test
     public void testMixOfBlockingAndNonBlocking() {
-        Endpoint endpoint = Endpoint.create("host");
+        Endpoint endpoint = Endpoint.create("localhost");
         SessionParams sessionParams = new SessionParams.Builder()
                 .addCluster(new Cluster.Builder().addEndpoint(endpoint).build())
                 .setConnectionParams(new ConnectionParams.Builder().build())
@@ -266,9 +269,9 @@ public class OperationProcessorTest {
     @Test
     public void assertThatDuplicateResultsFromOneClusterWorks() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .build();
 
         OperationProcessor q = new OperationProcessor(
@@ -292,9 +295,9 @@ public class OperationProcessorTest {
     @Test
     public void testMultipleDuplicateDocIds() {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .build();
 
         OperationProcessor q = new OperationProcessor(
@@ -354,7 +357,7 @@ public class OperationProcessorTest {
     @Test
     public void testWaitBlocks() throws InterruptedException {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("host")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .build();
 
         OperationProcessor operationProcessor = new OperationProcessor(
@@ -376,17 +379,17 @@ public class OperationProcessorTest {
         started.await();
         // We want the test to pass fast so we only wait 40mS to see that it is blocking. This might lead to
         // some false positives, but that is ok.
-        assertThat(done.await(40, TimeUnit.MILLISECONDS), is(false));
+        assertFalse(done.await(40, TimeUnit.MILLISECONDS));
         operationProcessor.resultReceived(
                 new EndpointResult(doc1.getOperationId(), new Result.Detail(Endpoint.create("d"))), 0);
-        assertThat(done.await(120, TimeUnit.SECONDS), is(true));
+        assertTrue(done.await(120, TimeUnit.SECONDS));
 
     }
 
     @Test
     public void testSendsResponseToQueuedDocumentOnClose() throws InterruptedException {
         SessionParams sessionParams = new SessionParams.Builder()
-                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("#$#")).build())
+                .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
                 .build();
 
         ScheduledThreadPoolExecutor executor = mock(ScheduledThreadPoolExecutor.class);
@@ -410,4 +413,30 @@ public class OperationProcessorTest {
         operationProcessor.close();
         countDownLatch.await();
     }
+
+    @Test
+    public void unknownHostThrowsExceptionAtConstructionTime() {
+        try {
+            SessionParams sessionParams = new SessionParams.Builder()
+                    .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build())
+                    .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("unknown.invalid")).build())
+                    .build();
+            ScheduledThreadPoolExecutor executor = mock(ScheduledThreadPoolExecutor.class);
+
+            CountDownLatch countDownLatch = new CountDownLatch(3);
+
+            OperationProcessor operationProcessor = new OperationProcessor(
+                    new IncompleteResultsThrottler(19, 19, null, null),
+                    (docId, documentResult) -> {
+                        countDownLatch.countDown();
+                    },
+                    sessionParams, executor);
+
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("Unknown host: unknown.invalid:4080 ssl=false", e.getMessage());
+        }
+    }
+
 }