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

Conversation

beiske
Copy link
Member

@beiske beiske commented Apr 25, 2018

These are changes for running the rest tests against a cluster on Cloud.

@romseygeek romseygeek added the :Delivery/Build Build or test infrastructure label Apr 25, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented Apr 25, 2018

As far as I can see this change makes it possible to configure '/' as a pathPrefix in the low-level REST client. I wonder if this is how we want to make this change. We never modified paths to start with '/' but Elasticsearch used to be lenient there and accept relative paths as they were absolute. Considering that Elasticsearch no longer makes relative paths work as if they were absolute, should we rather always add the '/' when missing on any path, rather than adding the ability to configure '/' as pathPrefix?

@beiske
Copy link
Member Author

beiske commented Apr 25, 2018

...should we rather always add the '/' when missing on any path, rather than adding the ability to configure '/' as pathPrefix?

Sounds good to me. That was one of my original proposals. Using the prefix path was done by proposal from @jasontedor. Maybe he was anticipating issues when backporting to 6.x or something else I don't remember?

@jasontedor
Copy link
Member

I really do not like manipulating user input. If the user says: send the request to a/b/c we should send the request to a/b/c.

@colings86 colings86 added the >test Issues or PRs that are addressing/adding tests label Apr 25, 2018
@javanna
Copy link
Member

javanna commented Apr 26, 2018

@jasontedor I agree on the sentiment. On my end, I would like to figure out what Elasticsearch accepts with the recent changes in master, what works and what doesn't, and make sure that we are heading in the right direction. I would find it a bit weird to allow users to provide relative paths if they don't work at all, so wondering if this is only a proxy problem or not. An alternative could be to consider to no longer accept relative paths in the client, assuming that recent changes in the Elasticsearch server code doesn't make them work well like it did before.

} else {
fullPath = path;
}

logger.debug("Building URI with fullpath: ["+ fullPath+"] for path: ["+path+"]");
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wonder if this new log line is necessary, given that it's called for every single request I'd try to do without it. If users want to log this though they can enable request tracing?

@javanna
Copy link
Member

javanna commented May 7, 2018

I had another look and I am ok with this approach. I can't comment on the build changes though.

@beiske beiske force-pushed the rest-tests-against-cloud branch from 3cd2fcf to 1abf9ce Compare May 9, 2018 19:35
@suyograo
Copy link
Contributor

I can't comment on the build changes though.

@jasontedor could you please review or assign this to someone who can review the build changes bits?

@jasontedor
Copy link
Member

This will be looked at soon; we are bottlenecked right now.

@jasontedor
Copy link
Member

I want to break this PR into three separate PRs as there are multiple changes bundled here.

@jasontedor
Copy link
Member

The first PR is #30885.

@jasontedor
Copy link
Member

A second PR is #31074.

@jasontedor
Copy link
Member

A third PR is #31235.

@beiske
Copy link
Member Author

beiske commented Jun 21, 2018

I have rebased this one after the merges of #30885, #31074 and #31235 so that it now only contains the path prefix changes.

@beiske beiske changed the title Rest tests against cloud Enable setting client.path.prefix='/' Jun 21, 2018
@beiske beiske changed the title Enable setting client.path.prefix='/' Enable setting client.path.prefix='/' Jun 21, 2018
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

Looks good in general but I left some comments.

} else {
fullPath = pathPrefix + "/" + path;
}
if (pathPrefix != null && !pathPrefix.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

!pathPrefix.isEmpty() -> pathPrefix.isEmpty() == false

Like it or not, it's our style.

fullPath = pathPrefix + "/" + path;
}
if (pathPrefix != null && !pathPrefix.isEmpty()) {
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.

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be tests.rest.client_path_prefix. We do not need to namespace under path under client.

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"));
Copy link
Member

Choose a reason for hiding this comment

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

tests.rest.client.path.prefix -> tests.rest.client_path_prefix.

…ave 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='/'`.
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a comment.

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.

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?

@jasontedor jasontedor merged commit 2971dd5 into elastic:master Jul 1, 2018
jasontedor pushed a commit to jasontedor/elasticsearch that referenced this pull request Jul 1, 2018
Some proxies require all requests to have paths starting with / since
there are no relative paths at the HTTP connection level. Elasticsearch
assumes paths are absolute. In order to run rest tests against a cluster
behind such a proxy, set the system property
tests.rest.client_path_prefix to /.
jasontedor added a commit that referenced this pull request Jul 1, 2018
* elastic/master:
  Enable setting client path prefix to / (#30119)
  [DOCS] Secure settings specified per node (#31621)
  has_parent builder: exception message/param fix (#31182)
jasontedor added a commit that referenced this pull request Jul 1, 2018
* elastic/6.x:
  Enable setting client path prefix to / (#30119)
  [DOCS] Secure settings specified per node (#31621)
  Build test: Thread linger
  Build: Fix naming conventions task   (#31681)
  Introduce a Hashing Processor (#31087)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Jul 1, 2018
* elastic/ccr: (30 commits)
  Enable setting client path prefix to / (elastic#30119)
  [DOCS] Secure settings specified per node (elastic#31621)
  has_parent builder: exception message/param fix (elastic#31182)
  TEST: Randomize soft-deletes settings (elastic#31585)
  Mute 'Test typed keys parameter for suggesters' as we await a fix.
  Build test: Thread linger
  Fix gradle4.8 deprecation warnings (elastic#31654)
  Mute FileRealmTests#testAuthenticateCaching with an @AwaitsFix.
  Mute TransportChangePasswordActionTests#testIncorrectPasswordHashingAlgorithm with an @AwaitsFix.
  Build: Fix naming conventions task   (elastic#31681)
  Introduce a Hashing Processor (elastic#31087)
  Do not check for object existence when deleting repository index files (elastic#31680)
  Remove extra check for object existence in repository-gcs read object (elastic#31661)
  Support multiple system store types (elastic#31650)
  [Test] Clean up some repository-s3 tests (elastic#31601)
  [Docs] Use capital letters in section headings (elastic#31678)
  muted tests that will be replaced by the shard follow task refactoring: elastic#31581
  [DOCS] Add PQL language Plugin (elastic#31237)
  Merge AzureStorageService and AzureStorageServiceImpl and clean up tests (elastic#31607)
  TEST: Fix test task invocation (elastic#31657)
  ...
@beiske
Copy link
Member Author

beiske commented Jul 5, 2018

@jasontedor Thank you!

@aug24
Copy link

aug24 commented Aug 13, 2018

I've done some more digging, and I would like to discuss this further if that's OK? I believe the current position is inconsistent.

In RestClientBuilder.setPathPrefix, there is the following code:

    if (cleanPathPrefix.startsWith("/") == false) {
        cleanPathPrefix = "/" + cleanPathPrefix;
    }

ie paths are forced to have a leading slash.

There is also this:

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

ie you cannot simply handle the trivial case of "always prepend a slash" because the code actually doesn't let you.

As far as I can tell, there is no correct use case for the user providing a relative path URI without a pathPrefix. If we don't automatically fix this error (per the fix I suggested) then we should throw an exception with advice that either a pathPrefix or a leading slash is required.

@beiske
Copy link
Member Author

beiske commented Aug 13, 2018

Hi @aug24
not sure I quite understand what you're getting at. First:

    if (cleanPathPrefix.startsWith("/") == false) {
        cleanPathPrefix = "/" + cleanPathPrefix;
    }

Is code from RestClientBuilder.setPathPrefix(String pathPrefix), but this method isn't required, so paths are not forced to have a leading slash, just the prefix if you set one via the builder.

About:

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

this is code that was removed from RestClient in this pull request or did you find the same lines somewhere else?

If I've misunderstood the code locations you were thinking of, then please show me with links to the exact source on Github so we can clear up the misunderstanding.

Thanks for your contribution!

@aug24
Copy link

aug24 commented Aug 13, 2018

Apologies: more context is at #32714. Perhaps this discussion should continue there. I'll cross-post.

I clearly have a slightly old client as the second block, which was removed on July 1st, is present.

In short: the rest client builder permits the user to build a request which is, as I understand it, invalid.

I suggest adding a leading slash if one was not provided, or throwing an exception, as I think a request without a leading is slash is invalid (and so do AWS ELBs, which leads to a hard to debug error).

I notice that we do force a leading slash if a pathPrefix without one is given. In fact the comment on this PR:

Some proxies require all requests to have paths starting with / since
there are no relative paths at the HTTP connection level. Elasticsearch
assumes paths are absolute.

is precisely the reason why I think the other PR should be accepted.

@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants