Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable setting client.path.prefix='/' #30119

Merged
merged 4 commits into from
Jul 1, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -796,8 +796,11 @@ static URI buildUri(String pathPrefix, String path, Map<String, String> params)
Objects.requireNonNull(path, "path must not be null");
try {
String fullPath;
if (pathPrefix != null) {
if (path.startsWith("/")) {
if (pathPrefix != null && pathPrefix.isEmpty() == false) {
fullPath = (pathPrefix + "/" + path).replaceAll("//+", "/");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is to address the possibility of pathPrefix ending with / and us ending up with ${pathPrefix}//${path}? Rather, I think we should reject a pathPrefix that ends in / and then we don't have to do any of this funny business? Then this whole if block can disappear?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And alternative is to allow the trailing / and only add another / if it's not there?

I am ambivalent between the two approaches, but either is better than the //+ replacement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pathPrefix that is a single slash, would also be a pathPrefix that ends with a slash. So a naturalæ consequence of using the pathPrefix to ensure there always is a leading slash is allowing pathPrefix with a trailing slash. Of course one could make a special case for trailing slash when it is the entire prefix, but that seemed like very circumstantial logic to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is immediately written over in any one of the following if, else if, or else blocks so this line does not do anything?

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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,32 +143,33 @@ public RestClientBuilder setRequestConfigCallback(RequestConfigCallback requestC
* For example, if this is set to "/my/path", then any client request will become <code>"/my/path/" + endpoint</code>.
* <p>
* 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("/")) {
throw new IllegalArgumentException("pathPrefix is malformed. too many trailing slashes: [" + pathPrefix + "]");
}
}

if (cleanPathPrefix.isEmpty() || "/".equals(cleanPathPrefix)) {
throw new IllegalArgumentException("pathPrefix must not be empty or '/': [" + pathPrefix + "]");
}

this.pathPrefix = cleanPathPrefix;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,6 @@ public void testSetPathPrefixNull() {
}

public void testSetPathPrefixEmpty() {
assertSetPathPrefixThrows("/");
assertSetPathPrefixThrows("");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,26 @@ public void testBuildUriLeavesPathUntouched() {
URI uri = RestClient.buildUri("/foo$bar", "/index/type/id", Collections.<String, String>emptyMap());
assertEquals("/foo$bar/index/type/id", uri.getPath());
}
{
URI uri = RestClient.buildUri("/", "/*", Collections.<String, String>emptyMap());
assertEquals("/*", uri.getPath());
}
{
URI uri = RestClient.buildUri("/", "*", Collections.<String, String>emptyMap());
assertEquals("/*", uri.getPath());
}
{
URI uri = RestClient.buildUri(null, "*", Collections.<String, String>emptyMap());
assertEquals("*", uri.getPath());
}
{
URI uri = RestClient.buildUri("", "*", Collections.<String, String>emptyMap());
assertEquals("*", uri.getPath());
}
{
URI uri = RestClient.buildUri(null, "/*", Collections.<String, String>emptyMap());
assertEquals("/*", uri.getPath());
}
{
URI uri = RestClient.buildUri(null, "/foo$bar/ty/pe/i/d", Collections.<String, String>emptyMap());
assertEquals("/foo$bar/ty/pe/i/d", uri.getPath());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
}

/**
Expand Down Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public static Iterable<Object[]> 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();
}
Expand Down