Skip to content

nomad ui: 403 error handling #25631

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ygersie
Copy link
Contributor

@ygersie ygersie commented Apr 9, 2025

Currently the Nomad UI will always display an error without any body content. The Nomad CLI does handle this scenario nicely by outputting the body content coming from the API, see:

c.Ui.Error(fmt.Sprintf("Error submitting job: %s", err))

The reason this is important for us is because we have a deployment approval handler in between clients and Nomad. Our policy API returns a 403 when approvals haven't been gathered with a descriptive message. Currently the Nomad UI unfortunately doesn't return any response content, it just raises a static error.

This change returns the ForbiddenError message if its set and doesn't end with string permission denied, which is the body content returned on Nomad API calls when ACL errors occur.

Currently the Nomad UI will always display an error without any body content. The Nomad CLI does handle
this scenario nicely by outputting the body content coming from the API, see:
https://github.com/hashicorp/nomad/blob/95c914624eaa5bc8f8942bbcbea0b54b207efe99/command/job_run.go#L299

The reason this is for us is because we have a deployment approval handler in between clients and Nomad.
Our policy API returns a 403 when approvals haven't been gathered with a descriptive message. Currently
the Nomad UI unfortunately doesn't return any response content, it just raises a static error.

This change returns the ForbiddenError message if its set and doesn't end with `permission denied`, which
is the body content Nomad returns on ACL errors.
@philrenaud
Copy link
Contributor

Hi @ygersie, thanks for the PR! It looks good, but I wonder if there's an alternate approach:

https://github.com/hashicorp/nomad/blob/main/ui/app/templates/application.hbs#L95-L97
^--- this provides error output directly in the UI, and is currently hidden behind an env === dev flag. Looks like this:

image

This is to say, if we were to remove the env === dev condition, we could see the message passed forward without having to store it as a ForbiddenError.detail / without the extra adapter work here. I don't think I'm opposed to removing that conditional check, as a 403 error message is probably important enough to include by default here.

Happy to hear your opinion on the matter, though; do you think it would make sense to have it generally available here?

@ygersie
Copy link
Contributor Author

ygersie commented Apr 30, 2025

Hey @philrenaud, sorry for the late reply, I was out on PTO. The problem there is that it doesn't cover all use cases. The result would be that it works for the error page but not for errors embedded on others like job reverts/job stops/job updates:
Screenshot 2025-04-30 at 12 42 18

@tgross tgross moved this from Needs Triage to Triaging in Nomad - Community Issues Triage May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants