[serve] Use longest prefix matching for path routing (#15041)

python/ray/serve/BUILD

@@ -41,6 +41,14 @@ py_test(
     deps = [":serve_lib"],
 )
 
+py_test(
+    name = "test_http_routes",
+    size = "medium",
+    srcs = serve_tests_srcs,
+    tags = ["exclusive"],
+    deps = [":serve_lib"],
+)
+
 py_test(
     name = "test_advanced",
     size = "medium",

python/ray/serve/api.py

@@ -1102,6 +1102,10 @@ def ingress(
             # the fast api routes here.
             make_fastapi_class_based_view(app, cls)
         if path_prefix is not None:
+            if not path_prefix.startswith("/"):
+                raise ValueError("path_prefix must start with '/'")
+            if "{" in path_prefix or "}" in path_prefix:
+                raise ValueError("path_prefix may not contain wildcards")
             cls._serve_path_prefix = path_prefix
 
         return cls
@@ -1111,7 +1115,7 @@ def ingress(
 
 class ServeDeployment(ABC):
     @classmethod
-    def deploy(self, *init_args) -> None:
+    def deploy(cls, *init_args) -> None:
         """Deploy this deployment.
 
         Args:
@@ -1122,17 +1126,17 @@ class ServeDeployment(ABC):
         raise NotImplementedError()
 
     @classmethod
-    def delete(self) -> None:
+    def delete(cls) -> None:
         """Delete this deployment."""
         raise NotImplementedError()
 
     @classmethod
-    def get_handle(self, sync: Optional[bool] = True
+    def get_handle(cls, sync: Optional[bool] = True
                    ) -> Union[RayServeHandle, RayServeSyncHandle]:
         raise NotImplementedError()
 
     @classmethod
-    def options(self,
+    def options(cls,
                 backend_def: Optional[Callable] = None,
                 name: Optional[str] = None,
                 version: Optional[str] = None,

python/ray/serve/constants.py

@@ -41,3 +41,13 @@ BACKEND_RECONFIGURE_METHOD = "reconfigure"
 #: Internally reserved version tag that cannot be used by applications.
 # TODO(edoakes): this should be removed when we remove the old codepath.
 RESERVED_VERSION_TAG = "__serve_version__"
+
+#: All defined HTTP methods.
+# https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
+ALL_HTTP_METHODS = [
+    "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE",
+    "PATCH"
+]
+
+#: Path suffix used to wildcard all subpaths.
+WILDCARD_PATH_SUFFIX = "/{wildcard:path}"

python/ray/serve/controller.py

@@ -18,7 +18,11 @@ from ray.serve.common import (
     TrafficPolicy,
 )
 from ray.serve.config import BackendConfig, HTTPOptions, ReplicaConfig
-from ray.serve.constants import RESERVED_VERSION_TAG
+from ray.serve.constants import (
+    ALL_HTTP_METHODS,
+    RESERVED_VERSION_TAG,
+    WILDCARD_PATH_SUFFIX,
+)
 from ray.serve.endpoint_state import EndpointState
 from ray.serve.http_state import HTTPState
 from ray.serve.kv_store import RayInternalKVStore
@@ -266,18 +270,23 @@ class ServeController:
             # Backend config should be synchronized so the backend worker
             # is aware of it.
             backend_config.internal_metadata.path_prefix = f"/{name}"
+        else:
+            if ("{" in replica_config.path_prefix
+                    or "}" in replica_config.path_prefix):
+                raise ValueError(
+                    "Wildcard routes are not supported for deployment paths. "
+                    "Please use @serve.ingress with FastAPI instead.")
 
         if replica_config.is_asgi_app:
             # When the backend is asgi application, we want to proxy it
             # with a prefixed path as well as proxy all HTTP methods.
             # {wildcard:path} is used so HTTPProxy's Starlette router can match
             # arbitrary path.
-            http_route = f"{replica_config.path_prefix}" + "/{wildcard:path}"
-            # https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
-            http_methods = [
-                "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS",
-                "TRACE", "PATCH"
-            ]
+            path_prefix = replica_config.path_prefix
+            if path_prefix.endswith("/"):
+                path_prefix = path_prefix[:-1]
+            http_route = path_prefix + WILDCARD_PATH_SUFFIX
+            http_methods = ALL_HTTP_METHODS
         else:
             http_route = replica_config.path_prefix
             # Generic endpoint should support a limited subset of HTTP methods.

python/ray/serve/http_proxy.py

@@ -10,6 +10,7 @@ import ray
 from ray import serve
 from ray.exceptions import RayTaskError
 from ray.serve.common import EndpointTag
+from ray.serve.constants import WILDCARD_PATH_SUFFIX
 from ray.serve.long_poll import LongPollNamespace
 from ray.util import metrics
 from ray.serve.utils import logger
@@ -109,10 +110,13 @@ class HTTPProxy:
         logger.debug(f"HTTP Proxy: Get updated route table: {route_table}.")
         self.route_table = route_table
 
+        # Routes are sorted in order of descending length to enable longest
+        # prefix matching (Starlette evaluates the routes in order).
         routes = [
             starlette.routing.Route(
                 route, ServeStarletteEndpoint(endpoint_tag), methods=methods)
-            for route, (endpoint_tag, methods) in route_table.items()
+            for route, (endpoint_tag, methods) in sorted(
+                route_table.items(), key=lambda x: len(x[0]), reverse=True)
             if not self._is_headless(route)
         ]
 
@@ -130,7 +134,16 @@ class HTTPProxy:
         await response.send(scope, receive, send)
 
     async def _display_route_table(self, request):
-        return starlette.responses.JSONResponse(self.route_table)
+        # Strip out the wildcard suffix added to the routes in the controller.
+        # TODO(edoakes): once we deprecate the old ingress support, we could
+        # just add the wildcard when we add routes to the Router instead of in
+        # the route table.
+        stripped = {}
+        for path, (endpoint, methods) in self.route_table.items():
+            if path.endswith(WILDCARD_PATH_SUFFIX):
+                path = path[:-len(WILDCARD_PATH_SUFFIX)]
+            stripped[path] = (endpoint, methods)
+        return starlette.responses.JSONResponse(stripped)
 
     def _is_headless(self, route: str):
         """Returns True if `route` corresponds to a headless endpoint."""
@@ -146,10 +159,8 @@ class HTTPProxy:
         assert self.route_table is not None, (
             "Route table must be set via set_route_table.")
         assert scope["type"] == "http"
-        current_path = scope["path"]
-
-        self.request_counter.inc(tags={"route": current_path})
 
+        self.request_counter.inc(tags={"route": scope["path"]})
         await self.router(scope, receive, send)
 
 

python/ray/serve/tests/test_http_routes.py

@@ -0,0 +1,139 @@
+from fastapi import FastAPI
+import pytest
+import requests
+
+from ray import serve
+from ray.serve.constants import ALL_HTTP_METHODS
+
+
+def test_path_validation(serve_instance):
+    # Path prefix must start with /.
+    with pytest.raises(ValueError):
+
+        @serve.ingress(path_prefix="hello")
+        class D1:
+            pass
+
+    # Wildcards not allowed with new ingress support.
+    with pytest.raises(ValueError):
+
+        @serve.ingress(path_prefix="/{wildcard}")
+        class D2:
+            pass
+
+    @serve.deployment("test")
+    @serve.ingress(path_prefix="/duplicate")
+    class D3:
+        pass
+
+    D3.deploy()
+
+    # Reject duplicate route.
+    with pytest.raises(ValueError):
+        D3.options(name="test2").deploy()
+
+
+def test_routes_endpoint(serve_instance):
+    @serve.deployment("D1")
+    @serve.ingress
+    class D1:
+        pass
+
+    @serve.deployment("D2")
+    @serve.ingress(path_prefix="/hello/world")
+    class D2:
+        pass
+
+    D1.deploy()
+    D2.deploy()
+
+    routes = requests.get("http://localhost:8000/-/routes").json()
+
+    assert len(routes) == 2
+    assert routes["/D1"] == ["D1", ["GET", "POST"]]
+    assert routes["/hello/world"] == ["D2", ["GET", "POST"]]
+
+    D1.delete()
+
+    routes = requests.get("http://localhost:8000/-/routes").json()
+    assert len(routes) == 1
+    assert routes["/hello/world"] == ["D2", ["GET", "POST"]]
+
+    D2.delete()
+    routes = requests.get("http://localhost:8000/-/routes").json()
+    assert len(routes) == 0
+
+    app = FastAPI()
+
+    @serve.deployment("D3")
+    @serve.ingress(app, path_prefix="/hello")
+    class D3:
+        pass
+
+    D3.deploy()
+
+    routes = requests.get("http://localhost:8000/-/routes").json()
+    assert len(routes) == 1
+    assert routes["/hello"] == ["D3", ALL_HTTP_METHODS]
+
+
+def test_path_prefixing(serve_instance):
+    def req(subpath):
+        return requests.get(f"http://localhost:8000{subpath}").text
+
+    @serve.deployment("D1")
+    @serve.ingress(path_prefix="/")
+    class D1:
+        def __call__(self, *args):
+            return "1"
+
+    D1.deploy()
+    assert req("/") == "1"
+    assert req("/a") != "1"
+
+    @serve.deployment("D2")
+    @serve.ingress(path_prefix="/hello")
+    class D2:
+        def __call__(self, *args):
+            return "2"
+
+    D2.deploy()
+    assert req("/") == "1"
+    assert req("/hello") == "2"
+
+    @serve.deployment("D3")
+    @serve.ingress(path_prefix="/hello/world")
+    class D3:
+        def __call__(self, *args):
+            return "3"
+
+    D3.deploy()
+    assert req("/") == "1"
+    assert req("/hello") == "2"
+    assert req("/hello/world") == "3"
+
+    app = FastAPI()
+
+    @serve.deployment("D4")
+    @serve.ingress(app, path_prefix="/hello/world/again")
+    class D4:
+        @app.get("/")
+        def root(self):
+            return 4
+
+        @app.get("/{p}")
+        def subpath(self, p: str):
+            return p
+
+    D4.deploy()
+    assert req("/") == "1"
+    assert req("/hello") == "2"
+    assert req("/hello/world") == "3"
+    assert req("/hello/world/again") == "4"
+    assert req("/hello/world/again/") == "4"
+    assert req("/hello/world/again/hi") == '"hi"'
+
+
+if __name__ == "__main__":
+    import sys
+    sys.exit(pytest.main(["-v", "-s", __file__]))