From 77e0384c9a24b9ecdc1167b54d0e96670a99f0f8 Mon Sep 17 00:00:00 2001 From: Konrad Beiske Date: Tue, 10 Apr 2018 11:35:31 +0200 Subject: [PATCH 1/4] Enable setting `client.path.prefix='/'` to ensure all request paths have a leading '/'. Some proxies require all requests to have paths starting with `/` since there are no relative paths at the http connection level. Elasticsearch oth just assumes paths are absolute. In order to run rest tests against a cluster behind such a proxy, specify `-Dtests.rest.client.path.prefix='/'`. --- .../org/elasticsearch/client/RestClient.java | 8 ++------ .../client/RestClientBuilder.java | 15 +++++++------- .../client/RestClientBuilderTests.java | 1 - .../elasticsearch/client/RestClientTests.java | 20 +++++++++++++++++++ .../test/rest/ESRestTestCase.java | 10 +++++++++- ...CoreWithSecurityClientYamlTestSuiteIT.java | 1 + 6 files changed, 40 insertions(+), 15 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index d07e9e3c1cab7..915c1c7678f77 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -796,12 +796,8 @@ static URI buildUri(String pathPrefix, String path, Map params) Objects.requireNonNull(path, "path must not be null"); try { String fullPath; - if (pathPrefix != null) { - if (path.startsWith("/")) { - fullPath = pathPrefix + path; - } else { - fullPath = pathPrefix + "/" + path; - } + if (pathPrefix != null && !pathPrefix.isEmpty()) { + fullPath = (pathPrefix + "/" + path).replaceAll("//+", "/"); } else { fullPath = path; } diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index fb61f4f17c483..2bbf89f58c35a 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java @@ -143,22 +143,26 @@ public RestClientBuilder setRequestConfigCallback(RequestConfigCallback requestC * For example, if this is set to "/my/path", then any client request will become "/my/path/" + endpoint. *

* In essence, every request's {@code endpoint} is prefixed by this {@code pathPrefix}. The path prefix is useful for when - * Elasticsearch is behind a proxy that provides a base path; it is not intended for other purposes and it should not be supplied in - * other scenarios. + * Elasticsearch is behind a proxy that provides a base path or a proxy that requires all paths to start with '/'; + * it is not intended for other purposes and it should not be supplied in other scenarios. * * @throws NullPointerException if {@code pathPrefix} is {@code null}. - * @throws IllegalArgumentException if {@code pathPrefix} is empty, only '/', or ends with more than one '/'. + * @throws IllegalArgumentException if {@code pathPrefix} is empty, or ends with more than one '/'. */ public RestClientBuilder setPathPrefix(String pathPrefix) { Objects.requireNonNull(pathPrefix, "pathPrefix must not be null"); String cleanPathPrefix = pathPrefix; + if (cleanPathPrefix.isEmpty()) { + throw new IllegalArgumentException("pathPrefix must not be empty: [" + pathPrefix + "]"); + } + if (cleanPathPrefix.startsWith("/") == false) { cleanPathPrefix = "/" + cleanPathPrefix; } // best effort to ensure that it looks like "/base/path" rather than "/base/path/" - if (cleanPathPrefix.endsWith("/")) { + if (cleanPathPrefix.endsWith("/") && cleanPathPrefix.length() > 1) { cleanPathPrefix = cleanPathPrefix.substring(0, cleanPathPrefix.length() - 1); if (cleanPathPrefix.endsWith("/")) { @@ -166,9 +170,6 @@ public RestClientBuilder setPathPrefix(String pathPrefix) { } } - if (cleanPathPrefix.isEmpty() || "/".equals(cleanPathPrefix)) { - throw new IllegalArgumentException("pathPrefix must not be empty or '/': [" + pathPrefix + "]"); - } this.pathPrefix = cleanPathPrefix; return this; diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java index 9fcb4978e28a7..834748d65de34 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java @@ -180,7 +180,6 @@ public void testSetPathPrefixNull() { } public void testSetPathPrefixEmpty() { - assertSetPathPrefixThrows("/"); assertSetPathPrefixThrows(""); } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java index 30289ec84c9df..e8fb69153c27b 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java @@ -223,6 +223,26 @@ public void testBuildUriLeavesPathUntouched() { URI uri = RestClient.buildUri("/foo$bar", "/index/type/id", Collections.emptyMap()); assertEquals("/foo$bar/index/type/id", uri.getPath()); } + { + URI uri = RestClient.buildUri("/", "/*", Collections.emptyMap()); + assertEquals("/*", uri.getPath()); + } + { + URI uri = RestClient.buildUri("/", "*", Collections.emptyMap()); + assertEquals("/*", uri.getPath()); + } + { + URI uri = RestClient.buildUri(null, "*", Collections.emptyMap()); + assertEquals("*", uri.getPath()); + } + { + URI uri = RestClient.buildUri("", "*", Collections.emptyMap()); + assertEquals("*", uri.getPath()); + } + { + URI uri = RestClient.buildUri(null, "/*", Collections.emptyMap()); + assertEquals("/*", uri.getPath()); + } { URI uri = RestClient.buildUri(null, "/foo$bar/ty/pe/i/d", Collections.emptyMap()); assertEquals("/foo$bar/ty/pe/i/d", uri.getPath()); diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java index 495df4aa461a9..b31b78a3e10de 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java @@ -90,6 +90,7 @@ public abstract class ESRestTestCase extends ESTestCase { public static final String TRUSTSTORE_PASSWORD = "truststore.password"; public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout"; public static final String CLIENT_SOCKET_TIMEOUT = "client.socket.timeout"; + public static final String CLIENT_PATH_PREFIX = "client.path.prefix"; /** * Convert the entity from a {@link Response} into a map of maps. @@ -383,7 +384,11 @@ private void waitForClusterStateUpdatesToFinish() throws Exception { * Used to obtain settings for the REST client that is used to send REST requests. */ protected Settings restClientSettings() { - return Settings.EMPTY; + Settings.Builder builder = Settings.builder(); + if (System.getProperty("tests.rest.client.path.prefix") != null) { + builder.put(CLIENT_PATH_PREFIX, System.getProperty("tests.rest.client.path.prefix")); + } + return builder.build(); } /** @@ -454,6 +459,9 @@ protected static void configureClient(RestClientBuilder builder, Settings settin final TimeValue socketTimeout = TimeValue.parseTimeValue(socketTimeoutString, CLIENT_SOCKET_TIMEOUT); builder.setRequestConfigCallback(conf -> conf.setSocketTimeout(Math.toIntExact(socketTimeout.getMillis()))); } + if (settings.hasValue(CLIENT_PATH_PREFIX)) { + builder.setPathPrefix(settings.get(CLIENT_PATH_PREFIX)); + } } @SuppressWarnings("unchecked") diff --git a/x-pack/qa/core-rest-tests-with-security/src/test/java/org/elasticsearch/xpack/security/CoreWithSecurityClientYamlTestSuiteIT.java b/x-pack/qa/core-rest-tests-with-security/src/test/java/org/elasticsearch/xpack/security/CoreWithSecurityClientYamlTestSuiteIT.java index e5c82a23a59a5..212a342479d3f 100644 --- a/x-pack/qa/core-rest-tests-with-security/src/test/java/org/elasticsearch/xpack/security/CoreWithSecurityClientYamlTestSuiteIT.java +++ b/x-pack/qa/core-rest-tests-with-security/src/test/java/org/elasticsearch/xpack/security/CoreWithSecurityClientYamlTestSuiteIT.java @@ -39,6 +39,7 @@ public static Iterable parameters() throws Exception { protected Settings restClientSettings() { String token = basicAuthHeaderValue(USER, new SecureString(PASS.toCharArray())); return Settings.builder() + .put(super.restClientSettings()) .put(ThreadContext.PREFIX + ".Authorization", token) .build(); } From 1f5410ee8afbc4bb6215962e51cdd05de13a6e87 Mon Sep 17 00:00:00 2001 From: Konrad Beiske Date: Thu, 28 Jun 2018 10:47:38 +0200 Subject: [PATCH 2/4] Changes after review --- .../main/java/org/elasticsearch/client/RestClient.java | 9 ++++++++- .../java/org/elasticsearch/test/rest/ESRestTestCase.java | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 915c1c7678f77..9e9d4cd727b64 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -796,8 +796,15 @@ static URI buildUri(String pathPrefix, String path, Map params) Objects.requireNonNull(path, "path must not be null"); try { String fullPath; - if (pathPrefix != null && !pathPrefix.isEmpty()) { + if (pathPrefix != null && pathPrefix.isEmpty() == false) { fullPath = (pathPrefix + "/" + path).replaceAll("//+", "/"); + if (pathPrefix.endsWith("/") && path.startsWith("/")) { + fullPath = pathPrefix.substring(0, pathPrefix.length() - 1) + path; + } else if (pathPrefix.endsWith("/") || path.startsWith("/")) { + fullPath = pathPrefix + path; + } else { + fullPath = pathPrefix + "/" + path; + } } else { fullPath = path; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java index b31b78a3e10de..8737378dbd715 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java @@ -385,8 +385,8 @@ private void waitForClusterStateUpdatesToFinish() throws Exception { */ protected Settings restClientSettings() { Settings.Builder builder = Settings.builder(); - if (System.getProperty("tests.rest.client.path.prefix") != null) { - builder.put(CLIENT_PATH_PREFIX, System.getProperty("tests.rest.client.path.prefix")); + if (System.getProperty("tests.rest.client_path_prefix") != null) { + builder.put(CLIENT_PATH_PREFIX, System.getProperty("tests.rest.client_path_prefix")); } return builder.build(); } From a45c41407668fe3b0b6c5634cd7554cc7223992b Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 29 Jun 2018 15:38:49 -0400 Subject: [PATCH 3/4] Remove superfluous line --- .../rest/src/main/java/org/elasticsearch/client/RestClient.java | 1 - 1 file changed, 1 deletion(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java index 9e9d4cd727b64..d85bdb86d021f 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClient.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClient.java @@ -797,7 +797,6 @@ static URI buildUri(String pathPrefix, String path, Map params) try { String fullPath; if (pathPrefix != null && pathPrefix.isEmpty() == false) { - fullPath = (pathPrefix + "/" + path).replaceAll("//+", "/"); if (pathPrefix.endsWith("/") && path.startsWith("/")) { fullPath = pathPrefix.substring(0, pathPrefix.length() - 1) + path; } else if (pathPrefix.endsWith("/") || path.startsWith("/")) { From 3b59d9f84fa5f7da2a79000774cd464faa913ecb Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Sat, 30 Jun 2018 15:06:23 -0400 Subject: [PATCH 4/4] Cleanup --- .../elasticsearch/client/RestClientBuilder.java | 6 +++--- .../org/elasticsearch/client/RestClientTests.java | 15 ++++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index 2bbf89f58c35a..dd3f5ad5a7274 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java @@ -151,12 +151,12 @@ public RestClientBuilder setRequestConfigCallback(RequestConfigCallback requestC */ public RestClientBuilder setPathPrefix(String pathPrefix) { Objects.requireNonNull(pathPrefix, "pathPrefix must not be null"); - String cleanPathPrefix = pathPrefix; - if (cleanPathPrefix.isEmpty()) { - throw new IllegalArgumentException("pathPrefix must not be empty: [" + pathPrefix + "]"); + if (pathPrefix.isEmpty()) { + throw new IllegalArgumentException("pathPrefix must not be empty"); } + String cleanPathPrefix = pathPrefix; if (cleanPathPrefix.startsWith("/") == false) { cleanPathPrefix = "/" + cleanPathPrefix; } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java index e8fb69153c27b..741fae68eba3e 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java @@ -219,32 +219,33 @@ public void onFailure(Exception exception) { } public void testBuildUriLeavesPathUntouched() { + final Map emptyMap = Collections.emptyMap(); { - URI uri = RestClient.buildUri("/foo$bar", "/index/type/id", Collections.emptyMap()); + URI uri = RestClient.buildUri("/foo$bar", "/index/type/id", emptyMap); assertEquals("/foo$bar/index/type/id", uri.getPath()); } { - URI uri = RestClient.buildUri("/", "/*", Collections.emptyMap()); + URI uri = RestClient.buildUri("/", "/*", emptyMap); assertEquals("/*", uri.getPath()); } { - URI uri = RestClient.buildUri("/", "*", Collections.emptyMap()); + URI uri = RestClient.buildUri("/", "*", emptyMap); assertEquals("/*", uri.getPath()); } { - URI uri = RestClient.buildUri(null, "*", Collections.emptyMap()); + URI uri = RestClient.buildUri(null, "*", emptyMap); assertEquals("*", uri.getPath()); } { - URI uri = RestClient.buildUri("", "*", Collections.emptyMap()); + URI uri = RestClient.buildUri("", "*", emptyMap); assertEquals("*", uri.getPath()); } { - URI uri = RestClient.buildUri(null, "/*", Collections.emptyMap()); + URI uri = RestClient.buildUri(null, "/*", emptyMap); assertEquals("/*", uri.getPath()); } { - URI uri = RestClient.buildUri(null, "/foo$bar/ty/pe/i/d", Collections.emptyMap()); + URI uri = RestClient.buildUri(null, "/foo$bar/ty/pe/i/d", emptyMap); assertEquals("/foo$bar/ty/pe/i/d", uri.getPath()); } {