From 23ec3d189c2ce719fe37474a75bcee98bf171c87 Mon Sep 17 00:00:00 2001 From: jvstme <36324149+jvstme@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:28:07 +0000 Subject: [PATCH] Configuring if service path prefix is stripped (#2254) - Add the `strip_prefix` setting to allow configuring whether the path prefix is stripped when forwarding requests to services without a gateway. - Update `URLReplacer` to not add the prefix to URLs that already have it. If a web app is configured to run with a prefix, it can log its URLs with the prefix, so `dstack` shouldn't add the prefix for the second time. - Add a Dash example to docs. --- docs/docs/concepts/services.md | 48 +++++++++++++++++-- .../_internal/core/models/configurations.py | 11 +++++ src/dstack/_internal/core/services/logs.py | 5 +- src/dstack/_internal/proxy/lib/models.py | 1 + .../_internal/proxy/lib/testing/common.py | 2 + .../_internal/server/services/proxy/repo.py | 1 + .../services/proxy/services/service_proxy.py | 4 ++ src/dstack/_internal/server/services/runs.py | 2 +- src/dstack/api/server/_runs.py | 10 ++++ .../_internal/core/services/test_logs.py | 22 ++++++--- .../proxy/routers/test_service_proxy.py | 39 +++++++++++++++ 11 files changed, 134 insertions(+), 11 deletions(-) diff --git a/docs/docs/concepts/services.md b/docs/docs/concepts/services.md index 9d02d4a9d..a860e9ae9 100644 --- a/docs/docs/concepts/services.md +++ b/docs/docs/concepts/services.md @@ -112,6 +112,48 @@ port: 8000 +### Path prefix { #path-prefix } + +If your `dstack` project doesn't have a [gateway](gateways.md), services are hosted with the +`/proxy/services///` path prefix in the URL. +When running web apps, you may need to set some app-specific settings +so that browser-side scripts and CSS work correctly with the path prefix. + +
+ +```yaml +type: service +name: dash +gateway: false + +# Disable authorization +auth: false +# Do not strip the path prefix +strip_prefix: false + +env: + # Configure Dash to work with a path prefix + # Replace `main` with your dstack project name + - DASH_ROUTES_PATHNAME_PREFIX=/proxy/services/main/dash/ + +commands: + - pip install dash + # Assuming the Dash app is in your repo at app.py + - python app.py + +port: 8050 +``` + +
+ +By default, `dstack` strips the prefix before forwarding requests to your service, +so to the service it appears as if the prefix isn't there. This allows some apps +to work out of the box. If your app doesn't expect the prefix to be stripped, +set [`strip_prefix`](../reference/dstack.yml/service.md#strip_prefix) to `false`. + +If your app cannot be configured to work with a path prefix, you can host it +on a dedicated domain name by setting up a [gateway](gateways.md). + ### Model If the service is running a chat model with an OpenAI-compatible interface, @@ -345,9 +387,9 @@ via the [`spot_policy`](../reference/dstack.yml/service.md#spot_policy) property Running services doesn't require [gateways](gateways.md) unless you need to enable auto-scaling or want the endpoint to use HTTPS and map it to your domain. -!!! info "Websockets and base path" - A [gateway](gateways.md) may also be required if the service needs Websockets or cannot be used with - a base path. +!!! info "Websockets and path prefix" + A gateway may also be required if the service needs Websockets or cannot be used with + a [path prefix](#path-prefix). > If you're using [dstack Sky :material-arrow-top-right-thin:{ .external }](https://sky.dstack.ai){:target="_blank"}, > a gateway is already pre-configured for you. diff --git a/src/dstack/_internal/core/models/configurations.py b/src/dstack/_internal/core/models/configurations.py index 87c80be8c..4c8f3b2b3 100644 --- a/src/dstack/_internal/core/models/configurations.py +++ b/src/dstack/_internal/core/models/configurations.py @@ -21,6 +21,7 @@ CommandsList = List[str] ValidPort = conint(gt=0, le=65536) SERVICE_HTTPS_DEFAULT = True +STRIP_PREFIX_DEFAULT = True class RunConfigurationType(str, Enum): @@ -246,6 +247,16 @@ class ServiceConfigurationParams(CoreModel): ), ), ] = None + strip_prefix: Annotated[ + bool, + Field( + description=( + "Strip the `/proxy/services///` path prefix" + " when forwarding requests to the service. Only takes effect" + " when running the service without a gateway" + ) + ), + ] = STRIP_PREFIX_DEFAULT model: Annotated[ Optional[Union[AnyModel, str]], Field( diff --git a/src/dstack/_internal/core/services/logs.py b/src/dstack/_internal/core/services/logs.py index cafcc24af..24eca579b 100644 --- a/src/dstack/_internal/core/services/logs.py +++ b/src/dstack/_internal/core/services/logs.py @@ -42,11 +42,14 @@ def _replace_url(self, match: re.Match) -> bytes: qs = {k: v[0] for k, v in urllib.parse.parse_qs(url.query).items()} if app_spec and app_spec.url_query_params is not None: qs.update({k.encode(): v.encode() for k, v in app_spec.url_query_params.items()}) + path = url.path + if not path.startswith(self.path_prefix.removesuffix(b"/")): + path = concat_url_path(self.path_prefix, path) url = url._replace( scheme=("https" if self.secure else "http").encode(), netloc=(self.hostname if omit_port else f"{self.hostname}:{local_port}").encode(), - path=concat_url_path(self.path_prefix, url.path), + path=path, query=urllib.parse.urlencode(qs).encode(), ) return url.geturl() diff --git a/src/dstack/_internal/proxy/lib/models.py b/src/dstack/_internal/proxy/lib/models.py index 5ec08f00a..8079a0868 100644 --- a/src/dstack/_internal/proxy/lib/models.py +++ b/src/dstack/_internal/proxy/lib/models.py @@ -32,6 +32,7 @@ class Service(ImmutableModel): https: Optional[bool] # only used on gateways auth: bool client_max_body_size: int # only enforced on gateways + strip_prefix: bool = True # only used in-server replicas: tuple[Replica, ...] @property diff --git a/src/dstack/_internal/proxy/lib/testing/common.py b/src/dstack/_internal/proxy/lib/testing/common.py index 6b335a339..7cd5722e3 100644 --- a/src/dstack/_internal/proxy/lib/testing/common.py +++ b/src/dstack/_internal/proxy/lib/testing/common.py @@ -29,6 +29,7 @@ def make_service( domain: Optional[str] = None, https: Optional[bool] = None, auth: bool = False, + strip_prefix: bool = True, ) -> Service: return Service( project_name=project_name, @@ -37,6 +38,7 @@ def make_service( https=https, auth=auth, client_max_body_size=2**20, + strip_prefix=strip_prefix, replicas=( Replica( id="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", diff --git a/src/dstack/_internal/server/services/proxy/repo.py b/src/dstack/_internal/server/services/proxy/repo.py index 8b7ef4577..0f63cb76f 100644 --- a/src/dstack/_internal/server/services/proxy/repo.py +++ b/src/dstack/_internal/server/services/proxy/repo.py @@ -98,6 +98,7 @@ async def get_service(self, project_name: str, run_name: str) -> Optional[Servic https=None, auth=run_spec.configuration.auth, client_max_body_size=DEFAULT_SERVICE_CLIENT_MAX_BODY_SIZE, + strip_prefix=run_spec.configuration.strip_prefix, replicas=tuple(replicas), ) diff --git a/src/dstack/_internal/server/services/proxy/services/service_proxy.py b/src/dstack/_internal/server/services/proxy/services/service_proxy.py index 3bbf9c136..24af3e4a5 100644 --- a/src/dstack/_internal/server/services/proxy/services/service_proxy.py +++ b/src/dstack/_internal/server/services/proxy/services/service_proxy.py @@ -12,6 +12,7 @@ ServiceConnectionPool, get_service_replica_client, ) +from dstack._internal.utils.common import concat_url_path from dstack._internal.utils.logging import get_logger logger = get_logger(__name__) @@ -37,6 +38,9 @@ async def proxy( client = await get_service_replica_client(service, repo, service_conn_pool) + if not service.strip_prefix: + path = concat_url_path(request.scope.get("root_path", "/"), request.url.path) + try: upstream_request = await build_upstream_request(request, path, client) except ClientDisconnect: diff --git a/src/dstack/_internal/server/services/runs.py b/src/dstack/_internal/server/services/runs.py index bcd83dc3e..dea0ff257 100644 --- a/src/dstack/_internal/server/services/runs.py +++ b/src/dstack/_internal/server/services/runs.py @@ -967,7 +967,7 @@ def _validate_run_spec_and_set_defaults(run_spec: RunSpec): _UPDATABLE_SPEC_FIELDS = ["repo_code_hash", "configuration"] # Most service fields can be updated via replica redeployment. # TODO: Allow updating other fields when a rolling deployment is supported. -_UPDATABLE_CONFIGURATION_FIELDS = ["replicas", "scaling"] +_UPDATABLE_CONFIGURATION_FIELDS = ["replicas", "scaling", "strip_prefix"] def _can_update_run_spec(current_run_spec: RunSpec, new_run_spec: RunSpec) -> bool: diff --git a/src/dstack/api/server/_runs.py b/src/dstack/api/server/_runs.py index dd8728188..2c2586d42 100644 --- a/src/dstack/api/server/_runs.py +++ b/src/dstack/api/server/_runs.py @@ -4,6 +4,11 @@ from pydantic import parse_obj_as +from dstack._internal.core.models.common import is_core_model_instance +from dstack._internal.core.models.configurations import ( + STRIP_PREFIX_DEFAULT, + ServiceConfiguration, +) from dstack._internal.core.models.pools import Instance from dstack._internal.core.models.profiles import Profile from dstack._internal.core.models.runs import ( @@ -146,6 +151,11 @@ def _get_run_spec_excludes(run_spec: RunSpec) -> Optional[dict]: if profile is not None and profile.stop_duration is None: profile_excludes.add("stop_duration") # client >= 0.18.40 / server <= 0.18.39 compatibility tweak + if ( + is_core_model_instance(configuration, ServiceConfiguration) + and configuration.strip_prefix == STRIP_PREFIX_DEFAULT + ): + configuration_excludes.add("strip_prefix") if configuration.single_branch is None: configuration_excludes.add("single_branch") diff --git a/src/tests/_internal/core/services/test_logs.py b/src/tests/_internal/core/services/test_logs.py index d3c74420d..6c966a3ae 100644 --- a/src/tests/_internal/core/services/test_logs.py +++ b/src/tests/_internal/core/services/test_logs.py @@ -1,3 +1,5 @@ +import pytest + from dstack._internal.core.models.runs import AppSpec from dstack._internal.core.services.logs import URLReplacer @@ -126,7 +128,18 @@ def test_omit_https_default_port(self): ) assert replacer(b"http://0.0.0.0:8000/qwerty") == b"https://secure.host.com/qwerty" - def test_in_server_proxy(self): + @pytest.mark.parametrize( + ("in_path", "out_path"), + [ + ("", "/proxy/services/main/service/"), + ("/", "/proxy/services/main/service/"), + ("/a/b/c", "/proxy/services/main/service/a/b/c"), + ("/proxy/services/main/service", "/proxy/services/main/service"), + ("/proxy/services/main/service/", "/proxy/services/main/service/"), + ("/proxy/services/main/service/a/b/c", "/proxy/services/main/service/a/b/c"), + ], + ) + def test_adds_prefix_unless_already_present(self, in_path: str, out_path: str) -> None: replacer = URLReplacer( ports={8888: 3000}, app_specs=[], @@ -135,9 +148,6 @@ def test_in_server_proxy(self): path_prefix="/proxy/services/main/service/", ) assert ( - replacer(b"http://0.0.0.0:8888") == b"http://0.0.0.0:3000/proxy/services/main/service/" - ) - assert ( - replacer(b"http://0.0.0.0:8888/qwerty") - == b"http://0.0.0.0:3000/proxy/services/main/service/qwerty" + replacer(f"http://0.0.0.0:8888{in_path}".encode()) + == f"http://0.0.0.0:3000{out_path}".encode() ) diff --git a/src/tests/_internal/server/services/proxy/routers/test_service_proxy.py b/src/tests/_internal/server/services/proxy/routers/test_service_proxy.py index 7892c9b70..70ff9fc82 100644 --- a/src/tests/_internal/server/services/proxy/routers/test_service_proxy.py +++ b/src/tests/_internal/server/services/proxy/routers/test_service_proxy.py @@ -4,6 +4,7 @@ import httpx import pytest from fastapi import FastAPI +from fastapi.responses import PlainTextResponse from dstack._internal.proxy.gateway.repo.repo import GatewayProxyRepo from dstack._internal.proxy.lib.auth import BaseProxyAuthProvider @@ -25,6 +26,8 @@ @pytest.fixture def mock_replica_client_httpbin(httpbin) -> Generator[None, None, None]: + """Mocks deployed services. Replaces them with httpbin""" + with patch( "dstack._internal.proxy.lib.services.service_connection.ServiceConnectionPool.get_or_add" ) as add_connection_mock: @@ -34,6 +37,20 @@ def mock_replica_client_httpbin(httpbin) -> Generator[None, None, None]: yield +@pytest.fixture +def mock_replica_client_path_reporter() -> Generator[None, None, None]: + """Mocks deployed services. Replaces them with an app that returns the requested path""" + + app = FastAPI() + app.get("{path:path}")(lambda path: PlainTextResponse(path)) + client = ServiceClient(base_url="http://test/", transport=httpx.ASGITransport(app)) + with patch( + "dstack._internal.proxy.lib.services.service_connection.ServiceConnectionPool.get_or_add" + ) as add_connection_mock: + add_connection_mock.return_value.client.return_value = client + yield + + def make_app( repo: BaseProxyRepo, auth: BaseProxyAuthProvider = ProxyTestAuthProvider() ) -> FastAPI: @@ -200,3 +217,25 @@ async def test_auth(mock_replica_client_httpbin, token: Optional[str], status: i url = "http://test-host/proxy/services/test-proj/httpbin/" resp = await client.get(url, headers=headers) assert resp.status_code == status + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("strip", "downstream_path", "upstream_path"), + [ + (True, "/proxy/services/my-proj/my-run/", "/"), + (True, "/proxy/services/my-proj/my-run/a/b", "/a/b"), + (False, "/proxy/services/my-proj/my-run/", "/proxy/services/my-proj/my-run/"), + (False, "/proxy/services/my-proj/my-run/a/b", "/proxy/services/my-proj/my-run/a/b"), + ], +) +async def test_strip_prefix( + mock_replica_client_path_reporter, strip: bool, downstream_path: str, upstream_path: str +) -> None: + repo = ProxyTestRepo() + await repo.set_project(make_project("my-proj")) + await repo.set_service(make_service("my-proj", "my-run", strip_prefix=strip)) + _, client = make_app_client(repo) + resp = await client.get(f"http://test-host{downstream_path}") + assert resp.status_code == 200 + assert resp.text == upstream_path