-
Notifications
You must be signed in to change notification settings - Fork 214
Migrate service model base url #3560
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
Changes from all commits
74ee2f4
345b2c9
3f1520e
e4c53c5
da461fe
b0dd7ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,17 @@ server { | |
|
|
||
| {% for location in locations %} | ||
| location {{ location.prefix }} { | ||
| {% if cors_enabled %} | ||
| # Handle CORS preflight before auth (rewrite phase runs before access phase) | ||
| if ($request_method = 'OPTIONS') { | ||
| add_header 'Access-Control-Allow-Origin' '*' always; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, PATCH, OPTIONS, HEAD' always; | ||
| add_header 'Access-Control-Allow-Headers' '*' always; | ||
| add_header 'Access-Control-Max-Age' '600' always; | ||
| return 204; | ||
| } | ||
| {% endif %} | ||
|
|
||
| {% if auth %} | ||
| auth_request /_dstack_auth; | ||
| {% endif %} | ||
|
|
@@ -46,6 +57,15 @@ server { | |
| location @websocket { | ||
| set $dstack_replica_hit 1; | ||
| {% if replicas %} | ||
| {% if cors_enabled %} | ||
| proxy_hide_header 'Access-Control-Allow-Origin'; | ||
| proxy_hide_header 'Access-Control-Allow-Methods'; | ||
| proxy_hide_header 'Access-Control-Allow-Headers'; | ||
| proxy_hide_header 'Access-Control-Allow-Credentials'; | ||
| add_header 'Access-Control-Allow-Origin' '*' always; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, PATCH, OPTIONS, HEAD' always; | ||
| add_header 'Access-Control-Allow-Headers' '*' always; | ||
| {% endif %} | ||
| proxy_pass http://{{ domain }}.upstream; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header Host $host; | ||
|
|
@@ -60,6 +80,15 @@ server { | |
| location @ { | ||
| set $dstack_replica_hit 1; | ||
| {% if replicas %} | ||
| {% if cors_enabled %} | ||
| proxy_hide_header 'Access-Control-Allow-Origin'; | ||
| proxy_hide_header 'Access-Control-Allow-Methods'; | ||
| proxy_hide_header 'Access-Control-Allow-Headers'; | ||
| proxy_hide_header 'Access-Control-Allow-Credentials'; | ||
|
Comment on lines
+84
to
+87
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this prevents users from configuring their own CORS policy for the service. For example, users may want to set a stricter But I'm not sure if there's an easy way to set headers conditionally, based on whether a header was returned by the upstream. Maybe this limitation won't have any realistic impact if we only set our CORS policy on |
||
| add_header 'Access-Control-Allow-Origin' '*' always; | ||
| add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, PATCH, OPTIONS, HEAD' always; | ||
| add_header 'Access-Control-Allow-Headers' '*' always; | ||
| {% endif %} | ||
| proxy_pass http://{{ domain }}.upstream; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header Host $host; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ class Service(ImmutableModel): | |
| strip_prefix: bool = True # only used in-server | ||
| replicas: tuple[Replica, ...] | ||
| router: Optional[AnyRouterConfig] = None | ||
| cors_enabled: bool = False # only used on gateways; enabled for openai-format models | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (nit) Whether CORS is enabled is a decision the gateway takes based on existing data points, not a new data point, so I'd prefer not to persist it in the repo. But if there isn't a different and easy way to implement that decision, I don't mind keeping it as is |
||
|
|
||
| @property | ||
| def domain_safe(self) -> str: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This CORS policy is more permissive than what we have on the
gateway.*endpoint, because this policy allows cross-origin requests to the entire service, not just the model. Consider setting a stricter policy by returning the CORS headers only from/v1(or a different configured OpenAI API prefix). Should be relatively easy to implement with anif ($request_uri ~ ^/<prefix>)blockThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if you don't strongly insist, I'd prefer to implement it separately from the main PR.