From cf98aea5e47f823fe3efc87073010e39d1048956 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Tue, 7 Jan 2020 12:36:14 +0100 Subject: Validate hostnames on setup --- .../communication/ApacheGatewayConnection.java | 31 ++++++---- .../http/client/core/communication/IOThread.java | 1 + .../operationProcessor/OperationProcessorTest.java | 68 +++++++++++++--------- 3 files changed, 63 insertions(+), 37 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..2faa6d86426 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 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/core/operationProcessor/OperationProcessorTest.java b/vespa-http-client/src/test/java/com/yahoo/vespa/http/client/core/operationProcessor/OperationProcessorTest.java index 78f616d0e36..54293495d5c 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,6 +19,8 @@ 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.mockito.ArgumentMatchers.any; @@ -40,10 +42,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 +55,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 +123,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 +161,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 +194,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 +227,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 +268,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 +294,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 +356,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 +378,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 +412,18 @@ public class OperationProcessorTest { operationProcessor.close(); countDownLatch.await(); } + + @Test + public void testUnknownHost() { + try { + new SessionParams.Builder() + .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build()) + .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("unknown")).build()) + .build(); + } + catch (IllegalArgumentException e) { + assertEquals("", e.getMessage()); + } + } + } -- cgit v1.2.3 From b0d801daed411fa114223a5e6f0663b595334de5 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Tue, 7 Jan 2020 13:15:14 +0100 Subject: Use 'localhost' as hostname --- .../com/yahoo/vespa/http/client/FeedClientTest.java | 2 +- .../yahoo/vespa/http/client/SyncFeedClientTest.java | 2 +- .../communication/ApacheGatewayConnectionTest.java | 19 ++++++++++--------- 3 files changed, 12 insertions(+), 11 deletions(-) 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 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; } + } -- cgit v1.2.3 From b92b5c31216fb331d9bc6b63c13c99424e7991d9 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Tue, 7 Jan 2020 14:23:19 +0100 Subject: Test properly --- .../operationProcessor/OperationProcessorTest.java | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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 54293495d5c..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 @@ -23,6 +23,7 @@ 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; @@ -414,15 +415,27 @@ public class OperationProcessorTest { } @Test - public void testUnknownHost() { + public void unknownHostThrowsExceptionAtConstructionTime() { try { - new SessionParams.Builder() + SessionParams sessionParams = new SessionParams.Builder() .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("localhost")).build()) - .addCluster(new Cluster.Builder().addEndpoint(Endpoint.create("unknown")).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("", e.getMessage()); + assertEquals("Unknown host: unknown.invalid:4080 ssl=false", e.getMessage()); } } -- cgit v1.2.3 From a11412b2f0184c2cc0c102c38d9701aff1bb27ae Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Tue, 7 Jan 2020 14:31:04 +0100 Subject: Make static --- .../vespa/http/client/core/communication/ApacheGatewayConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 2faa6d86426..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 @@ -96,7 +96,7 @@ class ApacheGatewayConnection implements GatewayConnection { throw new IllegalArgumentException("Got no client Id."); } - private Endpoint validate(Endpoint endpoint) { + private static Endpoint validate(Endpoint endpoint) { try { InetAddress.getByName(endpoint.getHostname()); return endpoint; -- cgit v1.2.3