Watcher: Always get HTTP response body independent from error code

When an HTTP input returns an error body, right now we check if the
error code is below 400 and only then we include the body.

However using another method from URLConnection, the body can be
access always.

Closes elastic/elasticsearch#1550

Original commit: elastic/x-pack-elasticsearch@1743fd0a77

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/http/HttpClient.java

@@ -43,8 +43,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import static java.util.Collections.unmodifiableMap;
-
 /**
  * Client class to wrap http connections
  */
@@ -211,16 +209,20 @@ public class HttpClient extends AbstractLifecycleComponent<HttpClient> {
             }
         }
         logger.debug("http status code [{}]", statusCode);
-        if (statusCode < 400) {
-            final byte[] body;
-            try (InputStream inputStream = urlConnection.getInputStream();
-                 ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
-                Streams.copy(inputStream, outputStream);
-                body = outputStream.toByteArray();
+        final byte[] body;
+        try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
+            try (InputStream is = urlConnection.getInputStream()) {
+                Streams.copy(is, outputStream);
+            } catch (Exception e) {
+                if (urlConnection.getErrorStream() != null) {
+                    try (InputStream is = urlConnection.getErrorStream()) {
+                        Streams.copy(is, outputStream);
+                    }
+                }
             }
-            return new HttpResponse(statusCode, body, responseHeaders);
+            body = outputStream.toByteArray();
         }
-        return new HttpResponse(statusCode, responseHeaders);
+        return new HttpResponse(statusCode, body, responseHeaders);
     }
 
     /** SSL Initialization **/

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/http/HttpResponse.java

@@ -52,11 +52,11 @@ public class HttpResponse implements ToXContent {
     }
 
     public HttpResponse(int status, @Nullable byte[] body) {
-        this(status, body != null ? new BytesArray(body) : null, emptyMap());
+        this(status, body != null && body.length > 0 ? new BytesArray(body) : null, emptyMap());
     }
 
     public HttpResponse(int status, @Nullable byte[] body, Map<String, String[]> headers) {
-        this(status, body != null ? new BytesArray(body) : null, headers);
+        this(status, body != null && body.length > 0 ? new BytesArray(body) : null, headers);
     }
 
     public HttpResponse(int status, @Nullable BytesReference body, Map<String, String[]> headers) {

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/http/HttpClientTests.java

@@ -5,12 +5,14 @@
  */
 package org.elasticsearch.watcher.support.http;
 
+import com.carrotsearch.randomizedtesting.generators.RandomStrings;
 import com.squareup.okhttp.mockwebserver.MockResponse;
 import com.squareup.okhttp.mockwebserver.MockWebServer;
 import com.squareup.okhttp.mockwebserver.RecordedRequest;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.junit.annotations.Network;
@@ -56,7 +58,7 @@ public class HttpClientTests extends ESTestCase {
     public void init() throws Exception {
         secretService = SecretService.Insecure.INSTANCE;
         authRegistry = new HttpAuthRegistry(singletonMap(BasicAuth.TYPE, new BasicAuthFactory(secretService)));
-        webServer = startWebServer(9200, 9300);
+        webServer = startWebServer();
         webPort = webServer.getPort();
         httpClient = new HttpClient(Settings.EMPTY, authRegistry, environment).start();
     }
@@ -265,17 +267,28 @@ public class HttpClientTests extends ESTestCase {
         }
     }
 
-    public void test400Code() throws Exception {
-        webServer.enqueue(new MockResponse().setResponseCode(400));
+    public void testHttpResponseWithAnyStatusCodeCanReturnBody() throws Exception {
+        int statusCode = randomFrom(200, 201, 400, 401, 403, 404, 405, 409, 413, 429, 500, 503);
+        String body = RandomStrings.randomAsciiOfLength(getRandom(), 100);
+        boolean hasBody = usually();
+        MockResponse mockResponse = new MockResponse().setResponseCode(statusCode);
+        if (hasBody) {
+            mockResponse.setBody(body);
+        }
+        webServer.enqueue(mockResponse);
         HttpRequest.Builder request = HttpRequest.builder("localhost", webPort)
                 .method(HttpMethod.POST)
                 .path("/test")
                 .auth(new BasicAuth("user", "pass".toCharArray()))
-                .body("body");
+                .body("body")
+                .connectionTimeout(TimeValue.timeValueMillis(500))
+                .readTimeout(TimeValue.timeValueMillis(500));
         HttpResponse response = httpClient.execute(request.build());
-        assertThat(response.status(), equalTo(400));
-        assertThat(response.hasContent(), is(false));
-        assertThat(response.body(), nullValue());
+        assertThat(response.status(), equalTo(statusCode));
+        assertThat(response.hasContent(), is(hasBody));
+        if (hasBody) {
+            assertThat(response.body().toUtf8(), is(body));
+        }
     }
 
     @Network
@@ -312,7 +325,7 @@ public class HttpClientTests extends ESTestCase {
 
     public void testThatProxyCanBeConfigured() throws Exception {
         // this test fakes a proxy server that sends a response instead of forwarding it to the mock web server
-        MockWebServer proxyServer = startWebServer(62000, 63000);
+        MockWebServer proxyServer = startWebServer();
         proxyServer.enqueue(new MockResponse().setResponseCode(200).setBody("fullProxiedContent"));
 
         try {
@@ -340,7 +353,7 @@ public class HttpClientTests extends ESTestCase {
 
     public void testThatProxyCanBeOverriddenByRequest() throws Exception {
         // this test fakes a proxy server that sends a response instead of forwarding it to the mock web server
-        MockWebServer proxyServer = startWebServer(62000, 63000);
+        MockWebServer proxyServer = startWebServer();
         proxyServer.enqueue(new MockResponse().setResponseCode(200).setBody("fullProxiedContent"));
 
         try {
@@ -382,17 +395,15 @@ public class HttpClientTests extends ESTestCase {
         assertThat(recordedRequest.getPath(), equalTo(path));
     }
 
-    private MockWebServer startWebServer(int startPort, int endPort) throws IOException {
-        for (int port = startPort; port < endPort; port++) {
-            try {
-                MockWebServer mockWebServer = new MockWebServer();
-                mockWebServer.start(port);
-                return mockWebServer;
-            } catch (BindException be) {
-                logger.warn("port [{}] was already in use trying next port", webPort);
-            }
+    private MockWebServer startWebServer() throws IOException {
+        try {
+            MockWebServer mockWebServer = new MockWebServer();
+            mockWebServer.start();
+            return mockWebServer;
+        } catch (BindException be) {
+            logger.warn("port [{}] was already in use trying next port", webPort);
         }
-        throw new ElasticsearchException("unable to find open port between 9200 and 9300");
+        throw new ElasticsearchException("unable to find open port: none specified, free one should have been chosed automatically");
     }
 
     static class ClientAuthRequiringSSLSocketFactory extends SSLSocketFactory {