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

fix(requests): use relative request paths #1175

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Dec 6, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

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 / with index.html, the browser will see the associated resources like favicon.ico and look for it with a GET /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 in index.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 the index.html tells the browser to always look for the associated assets at the server root path, rather than look for them relative to where index.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:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. Clone https://github.com/andrewazores/proxy-test, ./build.bash && ./run.bash
  3. Open web browser to 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 path
  4. Go to http://localhost:7878 and ensure an Nginx default page appears, indicating that the proxy is running.
  5. Go to 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:

Screenshot_2023-12-07_09-50-30

INFO: 10.0.2.100 - - [Thu, 7 Dec 2023 14:50:13 GMT] 4ms "GET /api/v1/targets HTTP/1.0" 200 1 KB "http://localhost:7878/test/" "Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/119.0"
Dec 07, 2023 2:50:13 PM org.slf4j.impl.JDK14LoggerAdapter fillCallerData
INFO: 10.0.2.100 - - [Thu, 7 Dec 2023 14:50:13 GMT] 6ms "GET /api/v1/recordings HTTP/1.0" 200 2 bytes "http://localhost:7878/test/" "Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/119.0"
Dec 07, 2023 2:50:13 PM org.slf4j.impl.JDK14LoggerAdapter fillCallerData
INFO: 10.0.2.100 - - [Thu, 7 Dec 2023 14:50:13 GMT] 1ms "GET /api/v1/recordings HTTP/1.0" 200 2 bytes "http://localhost:7878/test/" "Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/119.0"
Dec 07, 2023 2:50:17 PM org.slf4j.impl.JDK14LoggerAdapter fillCallerData
WARNING: 10.0.2.100 - - [Thu, 7 Dec 2023 14:50:17 GMT] 0ms "GET /api/v1/targets/service:jmx:rmi:/jndi/rmi:/localhost:0/jmxrmi/recordings HTTP/1.0" 404 53 bytes "http://localhost:7878/recordings" "Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/119.0"

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.

Screenshot_2023-12-07_10-01-54

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 usual https://localhost:8181 should be the normal baseline for comparison. Running the proxy and going to http://localhost:7878/test should illustrate the broken behaviour: the browser will load index.html, but fail to load all other CSS, JS, etc. resources.

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Dec 6, 2023
@mergify mergify bot added the safe-to-test label Dec 6, 2023
@andrewazores andrewazores added backport and removed needs-triage Needs thorough attention from code reviewers labels Dec 6, 2023
@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Dec 6, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1175-582adcac442281bc8986e48a24a5991aeb209d70 sh smoketest.sh

@andrewazores andrewazores marked this pull request as ready for review December 7, 2023 14:53
@andrewazores
Copy link
Member Author

/build_test

Copy link

github-actions bot commented Dec 7, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1175-ee0efb503ede2b97cfce72a653c118be43119229 sh smoketest.sh

aali309
aali309 previously approved these changes Dec 7, 2023
Copy link
Contributor

@aali309 aali309 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mwangggg
Copy link
Member

mwangggg commented Dec 8, 2023

not sure quite how relevant/important this is, but should this line be api/beta/matchExpressions

this.post('/api/beta/matchExpressions', (_, request) => {

@andrewazores
Copy link
Member Author

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.

@mwangggg
Copy link
Member

mwangggg commented Dec 8, 2023

I get a blank page when I go to http://localhost:7878/test
Screenshot from 2023-12-08 16-30-43

@andrewazores
Copy link
Member Author

What does your browser devtools network tab say? And are there any corresponding server logs?

That looks the same as what happens when the index.html gets loaded, but it either fails to load the JS/CSS/etc. assets or the server erroneously sends the index.html again instead of the actual asset that was requested.

@mwangggg
Copy link
Member

mwangggg commented Dec 8, 2023

this is the network tab
Screenshot from 2023-12-08 16-47-57

@andrewazores
Copy link
Member Author

Okay, the 404s on every other file looks like it's trying to resolve them relative to the root / instead of relative to /test - it should be doing GET /test/favicon.ico instead of GET /favicon.ico since you loaded index.html from /test.

If you check the raw response of the first GET /test/ you should see the index.html that was sent back to your browser - could you check for the <base> element and see what the href attribute is?

@mwangggg
Copy link
Member

mwangggg commented Dec 8, 2023

there's no <base> element? Perhaps I'm running the wrong -web branch...

@andrewazores
Copy link
Member Author

There definitely should be one - https://github.com/cryostatio/cryostat-web/blame/23a2ec9d1e0886b4b6528e10e2d5b456dbf3dcc5/src/index.html#L24 . It has been there since the beginning :-)

@mwangggg
Copy link
Member

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 http://localhost:7878/test working! Turns out I couldn't see the <base> element because it was off the screen 😁

Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewazores
Copy link
Member Author

What did you do to get it working?

@mwangggg
Copy link
Member

What did you do to get it working?

oh I just realized I was being dumb and wasn't using the right -web branch 😁

@andrewazores andrewazores merged commit 991011f into cryostatio:main Dec 11, 2023
18 checks passed
@andrewazores andrewazores deleted the web-root-path branch December 11, 2023 17:13
mergify bot pushed a commit that referenced this pull request Dec 11, 2023
* use relative authority

* use relative request path

(cherry picked from commit 991011f)
andrewazores added a commit that referenced this pull request Dec 11, 2023
* use relative authority

* use relative request path

(cherry picked from commit 991011f)

Co-authored-by: Andrew Azores <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Request] Provide a param that can set the web context root path of cryostat-web
3 participants