Add a HeaderMap to HttpError (#1193)

There are certain response headers which the HTTP standard specifies
that servers should send along with particular error responses or status
codes. For example, when returning a 405 Method Not Allowed status, 
[§ 15.5.0 RFC9110] states that:

> The origin server MUST generate an Allow header field in a 405
> response containing a list of the target resource's currently
> supported methods.

Currently, Dropshot does not do this, so we are technically not
compliant with RFC9110.

Similarly, some of Dropshot's extractors return error responses for
which we ought to emit headers. For example, the `TypedBody` extractor
will return a 415 Unsupported Media Type status, for which [§ 15.5.16
RFC9110] recommends returning an `Accept` or `Accept-Encoding` header
as appropriate:

> If the problem was caused by an unsupported content coding, the
> Accept-Encoding response header field (Section 12.5.3) ought to be
> used  to indicate which (if any) content codings would have been
> accepted in the request.
>
> On the other hand, if the cause was an unsupported media type, the
> Accept response header field (Section 12.5.1) can be used to indicate
> which media types would have been accepted in the request.

Note that, unlike the `Allow` header for 405 responses, this is just a
suggestion --- the RFC doesn't include normative language stating that
we MUST do this. But it's nice if we do.

Errors emitted internally by Dropshot are represented by the `HttpError`
type. Currently, there is no way for this type to provide headers which
should be added to responses generated from an `HttpError`. Therefore,
this branch does so by adding a `http::HeaderMap` to `HttpError`. When
the `HttpError` is converted into a response, any headers in the
`HeaderMap` are added to the response. Adding this field is a breaking
change, so I've added it to the changelog.

Because the `http::HeaderMap` type is fairly large even when empty, the
`HeaderMap` is stored in an `Option<Box<HeaderMap>>` to reduce the size
of `HttpError`s, especially in the common case where there is no header
map to add. Unlike containers such as `Vec` and `HashMap`, an empty
`http::HeaderMap` contains a fairly large number of fields, rather than
being a single pointer to a heap allocation containing the actual data.
Thus, we store an `Option<Box<HeaderMap>>` here, so that the empty case
is just a null pointer. Not doing this results in a clippy warning
on...every function returning a `Result<_, HttpError>`, which is
unfortunate because even if we ignored that lint in Dropshot, it would
still trigger for consumers of Dropshot's APIs.

As an initial proof of concept, I've added code to dropshot's router
module to add `Allow` headers with the list of allowed methods when
returning a 405 Method Not Allowed error. There are other places where
we should be returning headers in our error responses, but this felt
like one of the more important ones, since the RFC says we MUST do it.
We can add headers to other errors separately; my priority was to make
the breaking parts of the change first.

For example, note that we now return `Allow` headers in responses to
requests with invalid methods:

```console
eliza@noctis ~/Code/dropshot $ cargo run --example basic &
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/basic`
Dec 04 18:51:47.826 INFO listening, local_addr: 127.0.0.1:41751

eliza@noctis ~/Code/dropshot $ curl -v http://localhost:41751/counter -X DELETE
* Host localhost:41751 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:41751...
* connect to ::1 port 41751 from ::1 port 53992 failed: Connection refused
*   Trying 127.0.0.1:41751...
* Connected to localhost (127.0.0.1) port 41751
* using HTTP/1.x
> DELETE /counter HTTP/1.1
> Host: localhost:41751
> User-Agent: curl/8.11.0
> Accept: */*
>
* Request completely sent off
Dec 04 18:59:14.913 INFO accepted connection, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
Dec 04 18:59:14.917 INFO request completed, error_message_external: Method Not Allowed, error_message_internal: Method Not Allowed, latency_us: 834, response_code: 405, uri: /counter, method: DELETE, req_id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
< HTTP/1.1 405 Method Not Allowed
< allow: GET
< allow: PUT
< content-type: application/json
< x-request-id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793
< content-length: 93
< date: Wed, 04 Dec 2024 18:59:14 GMT
<
{
  "request_id": "e1d14d57-ca67-49cc-84b0-d1dc1e0e5793",
  "message": "Method Not Allowed"
* Connection #0 to host localhost left intact
}%

```

[§ 15.5.0 RFC9110]: https://httpwg.org/specs/rfc9110.html#status.405
[§ 15.5.16 RFC9110]: https://httpwg.org/specs/rfc9110.html#status.415

CHANGELOG.adoc

@@ -85,6 +85,11 @@ Additionally, `ErrorStatusCode` provides an `as_client_error` method that
 returns a `ClientErrorStatusCode` if the status code is a client error, or an
 error.
 
+* `HttpError` now contains an `Option<Box<http::HeaderMap>>` of headers to add
+to error responses constructed for the error.
++
+Code that constructs `HttpError` literals must now initialize this field.
+
 === Other notable changes
 
 * Endpoint handler functions may now return any error type that implements the

dropshot/src/error.rs

@@ -118,6 +118,14 @@ pub struct HttpError {
     pub external_message: String,
     /// Error message recorded in the log for this error
     pub internal_message: String,
+    /// Headers that will be added to responses generated from this
+    /// error.
+    // This is boxed in obeisance to Clippy's `result_large_err` lint
+    // (https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err).
+    // While we could allow that lint for Dropshot, `HttpError` will also be a
+    // common return type for consumers of Dropshot, so let's not force users to
+    // also allow the lint.
+    pub headers: Option<Box<http::HeaderMap>>,
 }
 
 /// Body of an HTTP response for an `HttpError`.  This type can be used to
@@ -212,6 +220,7 @@ impl HttpError {
             error_code,
             internal_message: message.clone(),
             external_message: message,
+            headers: None,
         }
     }
 
@@ -227,6 +236,7 @@ impl HttpError {
                 .unwrap()
                 .to_string(),
             internal_message,
+            headers: None,
         }
     }
 
@@ -245,6 +255,7 @@ impl HttpError {
                 .unwrap()
                 .to_string(),
             internal_message,
+            headers: None,
         }
     }
 
@@ -290,15 +301,108 @@ impl HttpError {
             error_code,
             internal_message,
             external_message,
+            headers: None,
         }
     }
 
+    /// Mutably borrow the `http::HeaderMap` associated with this error. If
+    /// there is no header map for this error, this method creates one.
+    pub fn headers_mut(&mut self) -> &mut http::HeaderMap {
+        self.headers.get_or_insert_with(|| Box::new(http::HeaderMap::new()))
+    }
+
+    /// Adds a header to the [`http::HeaderMap`] of headers to add to responses
+    /// generated from this error.
+    ///
+    /// If this error does not already have a header map (`self.header_map` is
+    /// `None`), this method creates one.
+    ///
+    /// # Returns
+    /// - [`Ok`]`(&mut Self)` if the provided `name` and `value` are a valid
+    ///   header name and value, respectively.
+    /// - [`Err`]`(`[`http::Error`]`)` if the header name or value is invalid,
+    ///   or the `HeaderMap` is full.
+    pub fn add_header<K, V>(
+        &mut self,
+        name: K,
+        value: V,
+    ) -> Result<&mut Self, http::Error>
+    where
+        http::HeaderName: TryFrom<K>,
+        <http::HeaderName as TryFrom<K>>::Error: Into<http::Error>,
+        http::HeaderValue: TryFrom<V>,
+        <http::HeaderValue as TryFrom<V>>::Error: Into<http::Error>,
+    {
+        let name = <http::HeaderName as TryFrom<K>>::try_from(name)
+            .map_err(Into::into)?;
+        let value = <http::HeaderValue as TryFrom<V>>::try_from(value)
+            .map_err(Into::into)?;
+        self.headers_mut().try_append(name, value)?;
+        Ok(self)
+    }
+
+    /// Adds a header to the [`http::HeaderMap`] of headers to add to responses
+    /// generated from this error, taking the error by value.
+    ///
+    /// If this error does not already have a header map (`self.header_map` is
+    /// `None`), this method creates one.
+    ///
+    /// Unlike [`HttpError::set_header`], this method takes `self` by value,
+    /// allowing it to be chained to form an expression that returns an
+    /// `HttpError`. However, because this takes `self` by value, returning an
+    /// error for an invalid header name or value will discard the `HttpError`.
+    /// To avoid this, use [`HttpError::set_header`] instead.
+    ///
+    /// # Returns
+    ///
+    /// - [`Ok`]`(`Self`)` if the provided `name` and `value` are a valid
+    ///   header name and value, respectively.
+    /// - [`Err`]`(`[`http::Error`]`)` if the header name or value is invalid,
+    ///   or the `HeaderMap` is full.
+    pub fn with_header<K, V>(
+        mut self,
+        name: K,
+        value: V,
+    ) -> Result<Self, http::Error>
+    where
+        http::HeaderName: TryFrom<K>,
+        <http::HeaderName as TryFrom<K>>::Error: Into<http::Error>,
+        http::HeaderValue: TryFrom<V>,
+        <http::HeaderValue as TryFrom<V>>::Error: Into<http::Error>,
+    {
+        let name = <http::HeaderName as TryFrom<K>>::try_from(name)
+            .map_err(Into::into)?;
+        let value = <http::HeaderValue as TryFrom<V>>::try_from(value)
+            .map_err(Into::into)?;
+        self.headers_mut().try_append(name, value)?;
+        Ok(self)
+    }
+
     /// Generates an HTTP response for the given `HttpError`, using `request_id`
     /// for the response's request id.
     pub fn into_response(
         self,
         request_id: &str,
     ) -> hyper::Response<crate::Body> {
+        let mut builder = hyper::Response::builder();
+
+        // Set the builder's initial `HeaderMap` to the headers specified by
+        // the error, if any exist.  Replacing the initial `HeaderMap` is more
+        // efficient than inserting each header from `self.headers` into the
+        // builder one-by-one, as we can just reuse the existing header map.
+        // It's fine to clobber the builder's header map, as we just created the
+        // builder and the header map is empty.
+        if let Some(headers) = self.headers {
+            let builder_headers = builder
+                .headers_mut()
+                // `headers_mut()` returns `None` in the case that the builder is in
+                // a failed state due to setting an invalid header or status code.
+                // However, we *just* constructed this builder, so it cannot be in
+                // an error state, as we haven't called any methods on it yet.
+                .expect("a newly created response builder cannot have failed");
+            *builder_headers = *headers;
+        }
+
         // TODO-hardening: consider handling the operational errors that the
         // Serde serialization fails or the response construction fails.  In
         // those cases, we should probably try to report this as a serious
@@ -307,7 +411,7 @@ impl HttpError {
         // there's only one possible set of input and we can test it.  We'll
         // probably have to use unwrap() there and make sure we've tested that
         // code at least once!)
-        hyper::Response::builder()
+        builder
             .status(self.status_code.as_status())
             .header(
                 http::header::CONTENT_TYPE,

dropshot/src/router.rs

@@ -532,10 +532,27 @@ impl<Context: ServerContext> HttpRouter<Context> {
         if node.methods.values().any(|handlers| {
             find_handler_matching_version(handlers, version).is_some()
         }) {
-            Err(HttpError::for_client_error_with_status(
+            let mut err = HttpError::for_client_error_with_status(
                 None,
                 ClientErrorStatusCode::METHOD_NOT_ALLOWED,
-            ))
+            );
+
+            // Add `Allow` headers for the methods that *are* acceptable for
+            // this path, as specified in § 15.5.0 RFC9110, which states:
+            //
+            // > The origin server MUST generate an Allow header field in a
+            // > 405 response containing a list of the target resource's
+            // > currently supported methods.
+            //
+            // See: https://httpwg.org/specs/rfc9110.html#status.405
+            if let Some(hdrs) = err.headers.as_deref_mut() {
+                hdrs.reserve(node.methods.len());
+            }
+            for allowed in node.methods.keys() {
+                err.add_header(http::header::ALLOW, allowed)
+                    .expect("method should be a valid allow header");
+            }
+            Err(err)
         } else {
             Err(HttpError::for_not_found(
                 None,