Merge pull request #197 from minrk/trailing-slash-route

make sure that `/prefix` and `/prefix/` are handled the same

Author: GeorgianaElena

jupyterhub_traefik_proxy/traefik_utils.py

@@ -22,14 +22,46 @@ def validate(self, obj, value):
 
 
 def generate_rule(routespec):
+    """Generate a traefik routing rule for a jupyterhub routespec
+
+
+    - if a routespec doesn't start with a `/`, the first part is a hostname.
+    - routespecs always end with `/`
+    - routespecs are a path _prefix_, and should match anything under them
+    - the root of the route without the trailing slash should match the rule,
+      e.g. the routespec `/prefix/` should match `/prefix` and `/prefix/tree`
+
+    Traefik rule documentation: https://doc.traefik.io/traefik/routing/routers/#rule
+    """
+
+    # validate assumption that routespecs always end with '/'
+    if not routespec.endswith("/"):
+        raise ValueError("routespec must end with /")
     routespec = unquote(routespec)
+
+    # traefik won't match /proxy/path to /proxy/path/
+    # so strip trailing slash for consistent behavior
     if routespec.startswith("/"):
         # Path-based route, e.g. /proxy/path/
-        rule = f"PathPrefix(`{routespec}`)"
+        host = ""
+        path = routespec
     else:
         # Host-based routing, e.g. host.tld/proxy/path/
-        host, path_prefix = routespec.split("/", 1)
-        rule = f"Host(`{host}`) && PathPrefix(`/{path_prefix}`)"
+        host, slash, path = routespec.partition("/")
+        path = slash + path
+
+    path_no_slash = path.rstrip("/")
+
+    path_rule = f"PathPrefix(`{path}`)"
+    if path_no_slash:
+        # include exact Path('/prefix') so that both /prefix/ and /prefix
+        # are served correctly
+        path_rule = f"( {path_rule} || Path(`{path_no_slash}`) )"
+
+    if host:
+        rule = f"Host(`{host}`) && {path_rule}"
+    else:
+        rule = path_rule
     return rule
 
 

tests/test_proxy.py

@@ -336,6 +336,27 @@ async def _wait_for_deletion():
         await test_route_exist(spec, extra_backends[i])
 
 
+async def test_trailing_slash(proxy, launch_backends):
+    proxy_url = proxy.public_url.rstrip("/")
+    routespec = "/path/prefix/"
+    default_backend, target_backend = await launch_backends(2)
+    target_port = urlparse(target_backend).port
+    default_port = urlparse(default_backend).port
+
+    await proxy.add_route("/", default_backend, {})
+    await proxy.add_route(routespec, target_backend, {})
+
+    port = await utils.get_responding_backend_port(proxy_url, "/path/prefix/")
+    assert port == target_port
+    port = await utils.get_responding_backend_port(proxy_url, "/path/prefix")
+    assert port == target_port
+    port = await utils.get_responding_backend_port(proxy_url, "/path/prefixnoslash")
+    assert port == default_port
+
+    for route in ["/", routespec]:
+        await proxy.delete_route(route)
+
+
 async def test_get_all_routes(proxy, launch_backends):
     # initial state: no routes
     routes = await proxy.get_all_routes()

tests/test_traefik_utils.py

@@ -76,3 +76,20 @@ def test_atomic_writing_recovery(tmpdir):
 
     # didn't leave any residue
     assert tmpdir.listdir() == [testfile]
+
+
+@pytest.mark.parametrize(
+    "routespec, expected_rule",
+    [
+        ("/", "PathPrefix(`/`)"),
+        ("/path/prefix/", "( PathPrefix(`/path/prefix/`) || Path(`/path/prefix`) )"),
+        ("host/", "Host(`host`) && PathPrefix(`/`)"),
+        (
+            "host/path/prefix/",
+            "Host(`host`) && ( PathPrefix(`/path/prefix/`) || Path(`/path/prefix`) )",
+        ),
+    ],
+)
+def test_generate_rule(routespec, expected_rule):
+    rule = traefik_utils.generate_rule(routespec)
+    assert rule == expected_rule

tests/utils.py

@@ -58,18 +58,20 @@ async def check_host_up_http(url):
 async def get_responding_backend_port(traefik_url, path):
     """Check if traefik followed the configuration and routed the
     request to the right backend"""
-    if not path.endswith("/"):
-        path += "/"
+
+    headers = {}
 
     if not path.startswith("/"):
-        req = HTTPRequest(
-            traefik_url + "".join("/" + path.split("/", 1)[1]),
-            method="GET",
-            headers={"Host": path.split("/")[0]},
-            validate_cert=False,
-        )
-    else:
-        req = HTTPRequest(traefik_url + path, validate_cert=False)
+        host, slash, path = path.partition("/")
+        path = slash + path
+        headers["Host"] = host
+
+    req = HTTPRequest(
+        traefik_url + path,
+        headers=headers,
+        validate_cert=False,
+        follow_redirects=True,
+    )
 
     try:
         resp = await AsyncHTTPClient().fetch(req)