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

Allow prefixes to URL #59

Closed
codyharris-h2o-ai opened this issue Aug 25, 2020 · 8 comments · Fixed by #1104
Closed

Allow prefixes to URL #59

codyharris-h2o-ai opened this issue Aug 25, 2020 · 8 comments · Fixed by #1104
Assignees
Labels
feature Feature request server Related to server
Milestone

Comments

@codyharris-h2o-ai
Copy link

Currently, when an app gets started, we expect that users will access the app via a path like /app// .
We’re rewriting the index path to update the references for the JS & CSS resources (ie, everything that comes from /static in Qd) to point to /app//static, and we proxy that directory to Qd. For the JS resources, we’re rewriting the websocket path from /_s to /app//ws. We’re also translating the websocket messages to rewrite /app/ to the route the app is currently listening on. We’re planning on adding a field to the toml file that allows developers to specify the route they’re using so we can do the correct proxy. We also rewrite /_f references to /app//_f.
It’s not ideal for us, particularly since the string sub can translate innocent code that breaks the page (we're already seeing this in some situations). We were wondering if it may be possible to have Qd know that it’s being loaded from a different path (using something like https://muffinman.io/react-router-subfolder-on-server/) that would allow us to avoid doing the translation.
Another option would be to serve apps from a subdomain, ie, .q8s.h2o.ai, and then it’s like it’s running from local. The concern is that this makes deployment on prem and local testing more difficult (but not impossible)

What we'd like to be able to do:
Allow the browser to fetch /app/<uuid>, and have the app function, and all we need to do is proxy /app/<uuid> to the container without any rewrites.

The HTML base page for the react app has a hardcoded path for /static where it loads the JS resources from, and the JS resources contain a hardcoded path to the websocket (/_s). Once the websocket connection is established, the browser sends its location to Qd. We can pass this path ahead of time as an environment variable when we launch the container so that the Qd knows to expect it and allow the app to load on the different path.

Apps might need to know about the prefix as well (ie, if they create a link to somewhere else in the app, they may need to know), so an API may be needed for this purpose.

We're open to other suggestions as well

@keznikl
Copy link
Contributor

keznikl commented Aug 26, 2020

Just to note we switched to host-based routing (using a subdomain for the app instance ID) but as @codyharris-h2o-ai said, we might prefer subroutes if it's sufficiently simple and safe to do

@lo5 lo5 added the server Related to server label Sep 9, 2020
@lo5 lo5 added this to the 2020 Q3 milestone Sep 9, 2020
@lo5 lo5 removed their assignment Sep 9, 2020
@geomodular geomodular self-assigned this Sep 28, 2020
@geomodular
Copy link
Contributor

geomodular commented Sep 30, 2020

but as @codyharris-h2o-ai said, we might prefer subroutes if it's sufficiently simple and safe to do

Quick answer: it's not simple change.

There are several ways how we can approach this. We need to alter both backend and frontend ofc:

On the backend side we can either:

  1. construct endpoints with a prefix: e.g. instead of having /_f endpoint we create /prefix/_f (H2O 3 has it that way) OR
  2. leave that to reverse proxy such as in nginx to handle /prefix part and have Qd server mostly untouched (DAI has it that way).

On the front end side we can either:

  1. use relative paths and base tag e.g. instead of downloading from /static we use static OR
  2. construct requests using prefix variable obtained from server so instead of calling /_f we would call /prefix/_f.

Needed changes

The main problem lies on frontend. We build it statically and no template system is used. We cannot use first approach as we seem are not able to build static part with a relative paths. And since we have no template involved, we cannot construct static paths the time we serve it.

The server part seems not hard to change (in case of the first point - H2O 3 way) but we do check url inside the codebase so several places need to be altered. The other thing is, the logs serve as the way to reconstruct Qd state and not sure what this would do if we change the endpoints.

I'd go with reverse proxy as a requirement (as long as it's feasible for k8s part - I assume there is such mechanism) and incorporate template for index.html to set up static paths according to some prefix variable. The same variable will be used to make the calls with prefix inside JS.

Is this ticket high priority btw? Not sure if it deserves so much attention since we have subdomain routing already implemented.

cc @lo5

@lo5 lo5 modified the milestones: 2020 Q3, 0.10 Nov 3, 2020
@lo5 lo5 modified the milestones: 0.10, 0.11 Nov 26, 2020
@lo5 lo5 modified the milestones: 0.11, 0.12 Jan 13, 2021
@lo5 lo5 modified the milestones: 0.12, 0.13 Feb 4, 2021
@lo5 lo5 removed this from the 0.13 milestone Mar 4, 2021
@lo5 lo5 added this to the 0.14 milestone Mar 15, 2021
@lo5 lo5 changed the title Allow Prefixes to Qd Allow prefixes to URL Mar 23, 2021
@lo5 lo5 mentioned this issue Mar 27, 2021
30 tasks
@lo5 lo5 modified the milestones: 0.14, 2021 Apr 30, 2021
@lo5 lo5 self-assigned this Oct 26, 2021
@keznikl
Copy link
Contributor

keznikl commented Oct 26, 2021

Again the issue is mostly with the client code; Appstore already implements a reverse proxy and it would be able to handle the trimming of a prefix

It is somewhat pressing since the host-based routing forces us to require wildcard TLS cert, which is often a problem; also the Appstore server needs to know its DNS ahead of time, which makes any deployment more problematic than any other webserver we have

mturoci added a commit that referenced this issue Oct 27, 2021
@lo5 lo5 closed this as completed in #1104 Nov 5, 2021
mturoci added a commit that referenced this issue Nov 8, 2021
@mturoci
Copy link
Collaborator

mturoci commented Dec 2, 2021

A few points that arose after discussion with @keznikl:

  • The auth paths (and their redirects) need to respect base URL as well, e.g. /base-url/auth. It currently returns 404.
    image
  • Minor: Assume leading / if not specified by the user, otherwise results in a python error:
    image
  • Minor: Consider renaming base-url to base-path as that's what it is actually.

cc @lo5 ^^

@lo5
Copy link
Member

lo5 commented Jan 11, 2022

The auth paths (and their redirects) need to respect base URL as well

Thanks for catching - fixed. There were hard-coded paths in the auth backend/frontend that caused this.

Minor: Assume leading / if not specified by the user

Is this for the @app() / site[...] in Python or -base-url? I don't think we have any examples in the docs that omit the leading /, do we?

Minor: Consider renaming base-url to base-path as that's what it is actually.

I thought through this before coming up with -base-url. I agree that technically it's a path and not a URL, but "base path" has other connotations and it's not immediately obvious that it's a "path that is part of the URL". Base URLs sans paths are employed by other projects, e.g. Typescript, Jekyll.

@mturoci
Copy link
Collaborator

mturoci commented Jan 17, 2022

Closing as HAC team confirmed this feature works as expected.

@mturoci mturoci closed this as completed Jan 17, 2022
@keznikl
Copy link
Contributor

keznikl commented Jan 17, 2022

Except the logout handling (which does not seem to work?)

@lo5
Copy link
Member

lo5 commented Jan 17, 2022

@keznikl The logout issue was fixed via 24d2a82

Are you still facing issues?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request server Related to server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants