-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix(requests): use relative request paths #1175
Conversation
4435248
to
f6169a2
Compare
f6169a2
to
582adca
Compare
/build_test |
Test image available:
|
/build_test |
Test image available:
|
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.
LGTM
not sure quite how relevant/important this is, but should this line be cryostat-web/src/mirage/index.ts Line 402 in 23a2ec9
|
Good catch. That's only for the "live demo" mode where the frontend runs on its own and that file provides a mock API, rather than the client talking to an actual server, so I think it doesn't end up mattering, but it may as well be consistent. |
What does your browser devtools network tab say? And are there any corresponding server logs? That looks the same as what happens when the |
Okay, the 404s on every other file looks like it's trying to resolve them relative to the root If you check the raw response of the first |
there's no |
There definitely should be one - https://github.com/cryostatio/cryostat-web/blame/23a2ec9d1e0886b4b6528e10e2d5b456dbf3dcc5/src/index.html#L24 . It has been there since the beginning :-) |
I got |
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.
LGTM
What did you do to get it working? |
ef1f173
to
3fde721
Compare
oh I just realized I was being dumb and wasn't using the right -web branch 😁 |
* use relative authority * use relative request path (cherry picked from commit 991011f)
Welcome to Cryostat! 👋
Before contributing, make sure you have:
main
branch[chore, ci, docs, feat, fix, test]
To recreate commits with GPG signature
git fetch upstream && git rebase --force --gpg-sign upstream/main
Fixes https://github.com/cryostatio/cryostat/issues/1810
Description of the change:
Sets up the
index.html
to use a<base href="./">
, which indicates to the user's browser that all of the other assets referenced by the index should be resolved relative to the location where the browser found the index on the server.In the usual case where the Cryostat server responds to requests like
GET /
withindex.html
, the browser will see the associated resources likefavicon.ico
and look for it with aGET /favicon.ico
. Likewise, the web-client JavaScript resolves API requests assuming that the paths start from the server root location, ex./api/v1/recordings
.If a user were to deploy Cryostat such that it is not on the root path, for example using a proxy like in the linked issue where
/cryostat/foo
on the proxy gets mapped to/foo
on Cryostat, then this setup is broken. Currently, this results inindex.html
being loaded, but instructing the browser to look for/favicon.ico
whereas the browser must now instead request/cryostat/favicon.ico
. Likewise, the JavaScript requests should go to ex./cryostat/api/v1/recordings
, but will instead incorrectly go to/api/v1/recordings
.Motivation for the change:
Without this change, the
<base href="/">
used means that theindex.html
tells the browser to always look for the associated assets at the server root path, rather than look for them relative to whereindex.html
was found.If a user deploys Cryostat behind a proxy that places it on a non-root path, for example, then this breaks the browser's ability to load resources other than
index.html
- see https://github.com/cryostatio/cryostat/issues/1810 .Similarly, the JavaScript of the web-client makes requests to the Cryostat server assuming that it should be reached at the absolute path
/
, so all API requests resolve to ex./api/v1/recordings
. This change also allows these requests to be relative to the document location, ex./test/api/v1/recordings
, so that the API requests would be handled by the proxy and routed back to the Cryostat server, back through the router, then back to the browser.How to manually test:
./build.bash && ./run.bash
https://localhost:8181
and ensure Cryostat still works as expected in the normal case where it is not behind any proxy and is located on the root pathhttp://localhost:7878
and ensure an Nginx default page appears, indicating that the proxy is running.http://localhost:7878/test
- the reverse proxy will accept requests here and pass them to Cryostat, but with the/test/
portion at the beginning of the path removed. To be sure, do a hard refresh here or use a private/incognito window so that your browser is not using cached assets. You should still be able to load the Cryostat web-client UI and click around.Note: There is a bug in the proxy setup where it unencodes URI segments before passing them to the server, so some views will look broken due to HTTP 404s - verify that if these occur, the browser's devtools network tab shows that the correct request with the encoded path was made, but the proxy and server logs show that the server received a request for an unencoded path:
You will probably hit the web-client's 404 page when first visiting
/test/
on the proxy - this seems like a relatively minor bug and goes away once you click around anywhere else in the UI. There might be something to do to fix this but it seems minor enough to just live with for now.To compare against the existing behaviour, run the same steps as above, but with a plain upstream
main
image rather than the CI-built image here. The usualhttps://localhost:8181
should be the normal baseline for comparison. Running the proxy and going tohttp://localhost:7878/test
should illustrate the broken behaviour: the browser will loadindex.html
, but fail to load all other CSS, JS, etc. resources.