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.

Sign up for GitHub

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

Jump to bottom

server: return better HTTP errors when not ensured #649

Merged
merged 4 commits into from
Apr 10, 2024
Merged

server: return better HTTP errors when not ensured #649

merged 4 commits into from
Apr 10, 2024

Commits on Apr 10, 2024

  1. server: return better HTTP errors when not ensured 

    Currently, if the `propolis-server` APIs that require a running instance
are called when the server has not received an `InstanceEnsure` request,
we return a 404. Sled-agent and Nexus handle this status code by
assuming *they* have done something wrong, such as attempting to call an
API route that the server doesn't support. This results in issues where
sled-agent assumes that it's the one that made a mistake when it tries
to call APIs on a `propolis-server` that has lost its VM instance due to
being restarted after a panic.

To make it easier for callers distinguish between errors that indicate
"this server doesn't know about a running instance" and errors that
indicate "you attempted to call an API route that I don't know about",
I've changed the status code returned by `propolis-server`s that haven't
created an instance to HTTP [424 Failed Dependency]. The best thing
about returning this status code is that it's "not 404", but it does
arguably have the most correct semantics for this specific error:

> The HTTP 424 Failed Dependency client error response code indicates
> that the method could not be performed on the resource because the
> requested action depended on another action, and that action failed.

On the other hand, this is kind of a weird status code that's mostly
used by WebDAV servers, so I'm open to using something else. We could
alternatively return 410 Gone in these cases, but that status is already
returned by servers whose VM instance has been deleted. I thought it
might be nice to be able to distinguish between "not ensured" and
"ensured, and then deleted"...but we could switch to 410 Gone here, too.

[424 Failed Dependency]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/424
    @hawkw
    hawkw committed Apr 10, 2024
    Configuration menu
    Copy the full SHA
    5719c28 View commit details
    Browse the repository at this point in the history

  2. more descriptive names

    @hawkw
    hawkw committed Apr 10, 2024
    Configuration menu
    Copy the full SHA
    6a97df7 View commit details
    Browse the repository at this point in the history

  3. update expected statuses in test

    @hawkw
    hawkw committed Apr 10, 2024
    Configuration menu
    Copy the full SHA
    686d48b View commit details
    Browse the repository at this point in the history

  4. move error codes to an enum propolis-api-types

    @hawkw
    hawkw committed Apr 10, 2024
    Configuration menu
    Copy the full SHA
    3ad59e9 View commit details
    Browse the repository at this point in the history