-
Notifications
You must be signed in to change notification settings - Fork 2k
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 UI to query client directly for task logs/state #6669
Conversation
Nomad web UI currently fails when querying client nodes for allocation state end endpoints, due to CORS policy. The issue is that CORS requests that are marked `withCredentials` need the http server to include a `Access-Control-Allow-Credentials` [1]. But Nomad Task Logs and filesystem requests include authenticating information and thus marked with `credentials=true`[2][3]. It's worth noting that the browser currently sends credentials and authentication token to servers anyway; it's just that the response is not made available to caller nomad ui javascript. For task logs specifically, nomad ui retries again by querying the web ui address (typically pointing to a nomad server) which will forward the request to the nomad client agent appropriately. [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials [2] https://github.com/hashicorp/nomad/blob/101d0373eec5d58761d05e67e03f38916997a6d2/ui/app/components/task-log.js#L50 [3] https://github.com/hashicorp/nomad/blob/101d0373eec5d58761d05e67e03f38916997a6d2/ui/app/services/token.js#L25-L39
I'm sure this is a good patch and it works as described, but it's puzzling to me. I swear @schmichael and I worked through this when we put CORS in place originally. Alas I don't remember any of that pairing session. Maybe @schmichael does? |
Is it possible to have a test assertion that this header exists, to prevent a regression happening again? |
@backspace What would you suggest? Is there a way to have mirage or frontend tests assert that browsers load the page given those headers? I feel like adding a backend test simply asserting which headers are set isn't quite meaningful and wouldn't have caught the regression here considering its due to ui changing the request parameters? IMO, an effective test would be probably an integration or an e2e test, but we lack this class of tests now. |
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.
I think this is safe to merge. It would be super cool to do a UI e2e test, but that's not realistic at the moment.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Nomad web UI currently fails when querying client nodes for allocation
state end endpoints, due to CORS policy.
The issue is that CORS requests that are marked
withCredentials
needthe http server to include a
Access-Control-Allow-Credentials
[1].But Nomad Task Logs and filesystem requests include authenticating
information and thus marked with
credentials=true
[2][3].It's worth noting that the browser currently sends credentials and
authentication token to servers anyway; it's just that the response is
not made available to caller nomad ui javascript. For task logs
specifically, nomad ui retries again by querying the web ui address
(typically pointing to a nomad server) which will forward the request
to the nomad client agent appropriately.
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials
[2]
nomad/ui/app/components/task-log.js
Line 50 in 101d037
[3]
nomad/ui/app/services/token.js
Lines 25 to 39 in 101d037