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

HttpServer: Support relative plugin paths #10975

Conversation

spinscale
Copy link
Contributor

When specifying relative paths on startup, handling plugin
paths failed due to recently added security fix.

The HttpServer now handles this correctly

In addition a new matcher has been added to easily check for a
status code of an HTTP response likes this

assertThat(response, hasStatus(OK));

Closes #10958

@spinscale spinscale added review :Core/Infra/REST API REST infrastructure and utilities labels May 5, 2015
@rmuir
Copy link
Contributor

rmuir commented May 5, 2015

Why make things absolute? I really really dislike java code that makes paths absolute when its unnecessary. Seems its only necessary for that the little webserver paths.

@spinscale
Copy link
Contributor Author

My idea was to unify the code instead of having this one exception with the plugins.
I dont have a strong feeling about either. Do you think something can go wrong by using absolute paths or just not a clean approach?

@rmuir
Copy link
Contributor

rmuir commented May 5, 2015

I don't think we should add code to the codebase and justify the additional code with "well nothing can go wrong by adding it". Either it has a purpose or we don't add it.

If we want to do some processing up-front here, i cannot imagine a situation where i'd want to just do absolute+normalize, because its no correct "solution" to anything

On the other hand if the proposal was:

// these directories are heavily used, so reduce to canonical form for performance reasons 
// (resolving any symlinks, case normalization, and making absolute). 
Files.createDirectories(path); // ensure created
path = path.toRealPath(); // to canonical

Then i could see the advantages: we go full-throttle to the canonical file name for each path which can have a number of advantages and simplifications.

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch from afaef6e to 2c22333 Compare May 5, 2015 14:09
@spinscale spinscale changed the title Environment: Ensure all path configurations are absolute HttpServer: Support relative plugin paths May 5, 2015
@spinscale
Copy link
Contributor Author

I actually rethought the whole implementation, and changed only the necessary part in HttpServer instead of changing the Environment, resulting in a much smaller change.

Thx for the helping comment!

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch from 2c22333 to c44de97 Compare May 5, 2015 14:11
@rmuir
Copy link
Contributor

rmuir commented May 5, 2015

+1 looks great.

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch 4 times, most recently from 1090520 to b883a82 Compare May 10, 2015 15:31
@spinscale
Copy link
Contributor Author

just fyi, why this is not merged yet. Tests failed on windows, added a fix, but want to make sure this passes on windows before pushing

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch 2 times, most recently from 28330e9 to 71e3965 Compare May 11, 2015 10:34
@jaymode
Copy link
Member

jaymode commented May 12, 2015

@spinscale I looked at the Windows issue and what I saw was that the code was trying to get a path with the first being relative ..\..\..\ and the second argument being absolute C:\path\elasticsearch. This causes an issue since Windows is always expected C: first. I worked around the issue with the following patch and tests pass on Windows 7 and my OS X.

diff --git a/src/test/java/org/elasticsearch/plugins/SitePluginRelativePathConfigTests.java b/src/test/java/org/elasticsearch/plugins/SitePluginRelativePathConfigTests.java
index 99fb23a..b80980b 100644
--- a/src/test/java/org/elasticsearch/plugins/SitePluginRelativePathConfigTests.java
+++ b/src/test/java/org/elasticsearch/plugins/SitePluginRelativePathConfigTests.java
@@ -40,35 +40,40 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasStatus;
 @ClusterScope(scope = SUITE, numDataNodes = 1)
 public class SitePluginRelativePathConfigTests extends ElasticsearchIntegrationTest {

+    final Path root = PathUtils.get(".").toAbsolutePath().getRoot();
+
     @Override
     protected Settings nodeSettings(int nodeOrdinal) {
-        // windows is not supported at the moment, see the assumeTrue statement in the test
-        if (Constants.WINDOWS) {
-            return super.nodeSettings(nodeOrdinal);
-        }

-        Path pluginDir = PathUtils.get(getRelativePath(PathUtils.get(".").toAbsolutePath()), getDataPath("/org/elasticsearch/plugins").toString());
+        String cwdToRoot = getRelativePath(PathUtils.get(".").toAbsolutePath());
+        Path pluginDir = PathUtils.get(cwdToRoot, relativizeToRootIfNecessary(getDataPath("/org/elasticsearch/plugins")).toString());
         boolean useRelativePath = randomBoolean();
         if (useRelativePath) {
             Path tempDir = createTempDir();
             pluginDir = PathUtils.get(tempDir.toString(), getRelativePath(tempDir), pluginDir.toString());
         }

+        pluginDir = pluginDir.toAbsolutePath();
         return settingsBuilder()
                 .put(super.nodeSettings(nodeOrdinal))
-                .put("path.plugins", pluginDir.toAbsolutePath())
+                .put("path.plugins", pluginDir)
                 .put("force.http.enabled", true)
                 .build();
     }

     @Test
     public void testThatRelativePathsDontAffectPlugins() throws Exception {
-        // the path is constructed like ..\..\..\C:\foo\bar, which does not work, thus skipping for now
-        assumeTrue("Absolute path inbetween paths are not supported on windows, skipping this test", !Constants.WINDOWS);
         HttpResponse response = httpClient().method("GET").path("/_plugin/dummy/").execute();
         assertThat(response, hasStatus(OK));
     }

+    private Path relativizeToRootIfNecessary(Path path) {
+        if (Constants.WINDOWS) {
+            return root.relativize(path);
+        }
+        return path;
+    }
+
     private String getRelativePath(Path path) {
         StringBuilder sb = new StringBuilder();
         for (int i = 0; i < path.getNameCount(); i++) {

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch 2 times, most recently from 6d706dc to a5eaa49 Compare May 13, 2015 14:08

import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;
import org.apache.lucene.util.Constants;
Copy link
Member

Choose a reason for hiding this comment

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

minor: unused import now that WINDOWS is a static import below

@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch from a5eaa49 to 49ea17e Compare May 13, 2015 16:56
@spinscale
Copy link
Contributor Author

@jaymode incorporated review comments, thx for checking

@jaymode
Copy link
Member

jaymode commented May 13, 2015

tested again and all good on Windows. LGTM

When specifying relative paths on startup, handling plugin
paths failed due to recently added security fix. This fix
ensures normalization of the plugin path as well.

In addition a new matcher has been added to easily check for a
status code of an HTTP response likes this

assertThat(response, hasStatus(OK));

Closes elastic#10958
@spinscale spinscale force-pushed the 1505-issue-10958-site-plugins-relative-path branch from 49ea17e to f05808d Compare May 15, 2015 06:46
@spinscale spinscale merged commit f05808d into elastic:master May 15, 2015
@kevinkluge kevinkluge removed the review label May 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with _site plugin using 1.5.2
5 participants