From e90778f645a36d504a9c5f5e0b0b79d39dd5cfbb Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Tue, 31 Oct 2023 16:56:05 -0700 Subject: [PATCH 1/7] Migrate to a more specific error-code variant in wasi-http Co-authored-by: Pat Hickey --- .../src/bin/api_proxy_streaming.rs | 2 +- .../http_outbound_request_invalid_dnsname.rs | 13 +-- .../http_outbound_request_invalid_version.rs | 17 ++-- .../http_outbound_request_unknown_method.rs | 16 ++-- ...ttp_outbound_request_unsupported_scheme.rs | 16 ++-- crates/test-programs/src/http.rs | 9 +- crates/wasi-http/src/body.rs | 28 +++--- crates/wasi-http/src/http_impl.rs | 37 ++++---- crates/wasi-http/src/lib.rs | 73 ++++------------ crates/wasi-http/src/types.rs | 72 +++++++-------- crates/wasi-http/src/types_impl.rs | 21 +++-- crates/wasi-http/tests/all/main.rs | 6 +- crates/wasi-http/wit/deps/http/handler.wit | 6 +- crates/wasi-http/wit/deps/http/types.wit | 87 ++++++++++++++++--- crates/wasi/wit/deps/http/handler.wit | 6 +- crates/wasi/wit/deps/http/types.wit | 87 ++++++++++++++++--- src/commands/serve.rs | 31 ++++++- 17 files changed, 327 insertions(+), 200 deletions(-) diff --git a/crates/test-programs/src/bin/api_proxy_streaming.rs b/crates/test-programs/src/bin/api_proxy_streaming.rs index 92ad6fb0283c..9c54eb474b49 100644 --- a/crates/test-programs/src/bin/api_proxy_streaming.rs +++ b/crates/test-programs/src/bin/api_proxy_streaming.rs @@ -451,7 +451,7 @@ mod executor { pub fn outgoing_request_send( request: OutgoingRequest, - ) -> impl Future> { + ) -> impl Future> { future::poll_fn({ let response = outgoing_handler::handle(request, None); diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs index 100f57d6d022..94fe91405d61 100644 --- a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs +++ b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, Method, Scheme}; fn main() { let res = test_programs::http::request( @@ -10,9 +10,10 @@ fn main() { None, ); - let error = res.unwrap_err().to_string(); - assert!( - error.starts_with("Error::InvalidUrl(\""), - "bad error: {error}" - ); + assert!(matches!( + res.unwrap_err() + .downcast::() + .expect("expected a wasi-http ErrorCode"), + ErrorCode::DnsError(_) + )); } diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs index 59c652fc2b9a..568026859a13 100644 --- a/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs +++ b/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs @@ -1,17 +1,14 @@ -use test_programs::wasi::http::types::{Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, Method, Scheme}; fn main() { let addr = std::env::var("HTTP_SERVER").unwrap(); let res = test_programs::http::request(Method::Connect, Scheme::Http, &addr, "/", None, Some(&[])); - let error = res.unwrap_err().to_string(); - if !error.starts_with("Error::ProtocolError(\"") { - panic!( - r#"assertion failed: `(left == right)` - left: `"{error}"`, - right: `"Error::ProtocolError(\"invalid HTTP version parsed\")"` - or `"Error::ProtocolError(\"operation was canceled\")"`)"# - ) - } + assert!(matches!( + res.unwrap_err() + .downcast::() + .expect("expected a wasi-http ErrorCode"), + ErrorCode::HttpProtocolError, + )); } diff --git a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs index 6211bf0c8b99..8de9af2ca3ec 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, HttpRequestErrorPayload, Method, Scheme}; fn main() { let res = test_programs::http::request( @@ -10,9 +10,13 @@ fn main() { None, ); - let error = res.unwrap_err(); - assert_eq!( - error.to_string(), - "Error::InvalidUrl(\"unknown method OTHER\")" - ); + assert!(matches!( + res.unwrap_err() + .downcast::() + .expect("expected a wasi-http ErrorCode"), + ErrorCode::HttpRequestError(HttpRequestErrorPayload { + status_code: 405, + .. + }), + )); } diff --git a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs index 262cb11e62f0..d7b59d891131 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, HttpRequestErrorPayload, Method, Scheme}; fn main() { let res = test_programs::http::request( @@ -10,9 +10,13 @@ fn main() { None, ); - let error = res.unwrap_err(); - assert_eq!( - error.to_string(), - "Error::InvalidUrl(\"unsupported scheme WS\")" - ); + assert!(matches!( + res.unwrap_err() + .downcast::() + .expect("expected a wasi-http ErrorCode"), + ErrorCode::HttpRequestError(HttpRequestErrorPayload { + status_code: 400, + .. + }), + )); } diff --git a/crates/test-programs/src/http.rs b/crates/test-programs/src/http.rs index 3dbcb6846b84..64a182ac8e09 100644 --- a/crates/test-programs/src/http.rs +++ b/crates/test-programs/src/http.rs @@ -113,19 +113,16 @@ pub fn request( http_types::OutgoingBody::finish(outgoing_body, None); let incoming_response = match future_response.get() { - Some(result) => result.map_err(|_| anyhow!("incoming response errored"))?, + Some(result) => result.map_err(|()| anyhow!("response already taken"))?, None => { let pollable = future_response.subscribe(); pollable.block(); future_response .get() .expect("incoming response available") - .map_err(|_| anyhow!("incoming response errored"))? + .map_err(|()| anyhow!("response already taken"))? } - } - // TODO: maybe anything that appears in the Result<_, E> position should impl - // Error? anyway, just use its Debug here: - .map_err(|e| anyhow!("{e:?}"))?; + }?; drop(future_response); diff --git a/crates/wasi-http/src/body.rs b/crates/wasi-http/src/body.rs index 03cc695bcdfd..c4ada9ef908f 100644 --- a/crates/wasi-http/src/body.rs +++ b/crates/wasi-http/src/body.rs @@ -14,7 +14,7 @@ use wasmtime_wasi::preview2::{ Subscribe, }; -pub type HyperIncomingBody = BoxBody; +pub type HyperIncomingBody = BoxBody; /// Small wrapper around `BoxBody` which adds a timeout to every frame. struct BodyWithTimeout { @@ -45,12 +45,12 @@ impl BodyWithTimeout { impl Body for BodyWithTimeout { type Data = Bytes; - type Error = types::Error; + type Error = types::ErrorCode; fn poll_frame( self: Pin<&mut Self>, cx: &mut Context<'_>, - ) -> Poll, types::Error>>> { + ) -> Poll, types::ErrorCode>>> { let me = Pin::into_inner(self); // If the timeout timer needs to be reset, do that now relative to the @@ -66,9 +66,7 @@ impl Body for BodyWithTimeout { // Register interest in this context on the sleep timer, and if the // sleep elapsed that means that we've timed out. if let Poll::Ready(()) = me.timeout.as_mut().poll(cx) { - return Poll::Ready(Some(Err(types::Error::TimeoutError( - "frame timed out".to_string(), - )))); + return Poll::Ready(Some(Err(types::ErrorCode::ConnectionReadTimeout))); } // Without timeout business now handled check for the frame. If a frame @@ -222,7 +220,7 @@ impl Subscribe for HostIncomingBodyStream { } impl HostIncomingBodyStream { - fn record_frame(&mut self, frame: Option, types::Error>>) { + fn record_frame(&mut self, frame: Option, types::ErrorCode>>) { match frame { Some(Ok(frame)) => match frame.into_data() { // A data frame was received, so queue up the buffered data for @@ -300,7 +298,7 @@ pub enum HostFutureTrailers { /// /// Note that `Ok(None)` means that there were no trailers for this request /// while `Ok(Some(_))` means that trailers were found in the request. - Done(Result, types::Error>), + Done(Result, types::ErrorCode>), } #[async_trait::async_trait] @@ -328,9 +326,7 @@ impl Subscribe for HostFutureTrailers { // this just in case though. Err(_) => { debug_assert!(false, "should be unreachable"); - *self = HostFutureTrailers::Done(Err(types::Error::ProtocolError( - "stream hung up before trailers were received".to_string(), - ))); + *self = HostFutureTrailers::Done(Err(types::ErrorCode::ConnectionTerminated)); } } } @@ -362,7 +358,7 @@ impl Subscribe for HostFutureTrailers { } } -pub type HyperOutgoingBody = BoxBody; +pub type HyperOutgoingBody = BoxBody; pub enum FinishMessage { Finished, @@ -384,7 +380,7 @@ impl HostOutgoingBody { } impl Body for BodyImpl { type Data = Bytes; - type Error = types::Error; + type Error = types::ErrorCode; fn poll_frame( mut self: Pin<&mut Self>, cx: &mut Context<'_>, @@ -406,9 +402,9 @@ impl HostOutgoingBody { FinishMessage::Trailers(trailers) => { Poll::Ready(Some(Ok(Frame::trailers(trailers)))) } - FinishMessage::Abort => Poll::Ready(Some(Err( - types::Error::ProtocolError("response corrupted".into()), - ))), + FinishMessage::Abort => { + Poll::Ready(Some(Err(types::ErrorCode::HttpProtocolError))) + } }, Poll::Ready(Err(RecvError { .. })) => Poll::Ready(None), } diff --git a/crates/wasi-http/src/http_impl.rs b/crates/wasi-http/src/http_impl.rs index 47ff08930647..a3070ed99877 100644 --- a/crates/wasi-http/src/http_impl.rs +++ b/crates/wasi-http/src/http_impl.rs @@ -1,22 +1,23 @@ -use crate::bindings::http::{ - outgoing_handler, - types::{self as http_types, Scheme}, +use crate::{ + bindings::http::{ + outgoing_handler, + types::{self, Scheme}, + }, + http_request_error, + types::{HostFutureIncomingResponse, HostOutgoingRequest, OutgoingRequest}, + WasiHttpView, }; -use crate::types::{self, HostFutureIncomingResponse, OutgoingRequest}; -use crate::WasiHttpView; use bytes::Bytes; use http_body_util::{BodyExt, Empty}; use hyper::Method; -use types::HostOutgoingRequest; use wasmtime::component::Resource; impl outgoing_handler::Host for T { fn handle( &mut self, request_id: Resource, - options: Option>, - ) -> wasmtime::Result, outgoing_handler::Error>> - { + options: Option>, + ) -> wasmtime::Result, types::ErrorCode>> { let opts = options.and_then(|opts| self.table().get(&opts).ok()); let connect_timeout = opts @@ -44,9 +45,10 @@ impl outgoing_handler::Host for T { crate::bindings::http::types::Method::Trace => Method::TRACE, crate::bindings::http::types::Method::Patch => Method::PATCH, crate::bindings::http::types::Method::Other(method) => { - return Ok(Err(outgoing_handler::Error::InvalidUrl(format!( - "unknown method {method}" - )))); + return Ok(Err(http_request_error( + 405, + format!("unknown method {method}"), + ))); } }; @@ -54,9 +56,10 @@ impl outgoing_handler::Host for T { Scheme::Http => (false, "http://", 80), Scheme::Https => (true, "https://", 443), Scheme::Other(scheme) => { - return Ok(Err(outgoing_handler::Error::InvalidUrl(format!( - "unsupported scheme {scheme}" - )))) + return Ok(Err(http_request_error( + 400, + format!("unsupported scheme {scheme}"), + ))) } }; @@ -86,7 +89,9 @@ impl outgoing_handler::Host for T { .body .unwrap_or_else(|| Empty::::new().map_err(|_| unreachable!()).boxed()); - let request = builder.body(body).map_err(types::http_protocol_error)?; + let request = builder + .body(body) + .map_err(|_| types::ErrorCode::HttpProtocolError)?; Ok(Ok(self.send_request(OutgoingRequest { use_tls, diff --git a/crates/wasi-http/src/lib.rs b/crates/wasi-http/src/lib.rs index 936c48a1f4db..faa983c3d031 100644 --- a/crates/wasi-http/src/lib.rs +++ b/crates/wasi-http/src/lib.rs @@ -37,64 +37,21 @@ pub mod bindings { pub use wasi::http; } -impl From for crate::bindings::http::types::Error { - fn from(err: wasmtime_wasi::preview2::TableError) -> Self { - Self::UnexpectedError(err.to_string()) - } +pub(crate) fn dns_error(rcode: String, info_code: u16) -> bindings::http::types::ErrorCode { + bindings::http::types::ErrorCode::DnsError(bindings::http::types::DnsErrorPayload { + rcode, + info_code, + }) } -impl From for crate::bindings::http::types::Error { - fn from(err: anyhow::Error) -> Self { - Self::UnexpectedError(err.to_string()) - } -} - -impl From for crate::bindings::http::types::Error { - fn from(err: std::io::Error) -> Self { - let message = err.to_string(); - match err.kind() { - std::io::ErrorKind::InvalidInput => Self::InvalidUrl(message), - std::io::ErrorKind::AddrNotAvailable => Self::InvalidUrl(message), - _ => { - if message.starts_with("failed to lookup address information") { - Self::InvalidUrl("invalid dnsname".to_string()) - } else { - Self::ProtocolError(message) - } - } - } - } -} - -impl From for crate::bindings::http::types::Error { - fn from(err: hyper::Error) -> Self { - let message = err.message().to_string(); - if err.is_timeout() { - Self::TimeoutError(message) - } else if err.is_parse_status() || err.is_user() { - Self::InvalidUrl(message) - } else if err.is_body_write_aborted() - || err.is_canceled() - || err.is_closed() - || err.is_incomplete_message() - || err.is_parse() - { - Self::ProtocolError(message) - } else { - Self::UnexpectedError(message) - } - } -} - -impl From for crate::bindings::http::types::Error { - fn from(err: tokio::time::error::Elapsed) -> Self { - Self::TimeoutError(err.to_string()) - } -} - -#[cfg(not(any(target_arch = "riscv64", target_arch = "s390x")))] -impl From for crate::bindings::http::types::Error { - fn from(_err: rustls::client::InvalidDnsNameError) -> Self { - Self::InvalidUrl("invalid dnsname".to_string()) - } +pub(crate) fn http_request_error( + status_code: u16, + status_phrase: String, +) -> bindings::http::types::ErrorCode { + bindings::http::types::ErrorCode::HttpRequestError( + bindings::http::types::HttpRequestErrorPayload { + status_code, + status_phrase, + }, + ) } diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs index c7041fcad5c0..a7ec00b9ed4f 100644 --- a/crates/wasi-http/src/types.rs +++ b/crates/wasi-http/src/types.rs @@ -4,6 +4,7 @@ use crate::{ bindings::http::types::{self, Method, Scheme}, body::{HostIncomingBody, HyperIncomingBody, HyperOutgoingBody}, + dns_error, }; use http_body_util::BodyExt; use hyper::header::HeaderName; @@ -50,7 +51,7 @@ pub trait WasiHttpView: Send { fn new_response_outparam( &mut self, result: tokio::sync::oneshot::Sender< - Result, types::Error>, + Result, types::ErrorCode>, >, ) -> wasmtime::Result> { let id = self.table().push(HostResponseOutparam { result })?; @@ -108,15 +109,29 @@ async fn handler( first_byte_timeout: Duration, request: http::Request, between_bytes_timeout: Duration, -) -> Result { +) -> Result { let tcp_stream = TcpStream::connect(authority.clone()) .await - .map_err(invalid_url)?; + .map_err(|e| match e.kind() { + std::io::ErrorKind::AddrNotAvailable => { + dns_error("address not available".to_string(), 0) + } + + _ => { + if e.to_string() + .starts_with("failed to lookup address information") + { + dns_error("address not available".to_string(), 0) + } else { + types::ErrorCode::ConnectionRefused + } + } + })?; let (mut sender, worker) = if use_tls { #[cfg(any(target_arch = "riscv64", target_arch = "s390x"))] { - return Err(crate::bindings::http::types::Error::UnexpectedError( + return Err(crate::bindings::http::types::ErrorCode::UnexpectedError( "unsupported architecture for SSL".to_string(), )); } @@ -141,18 +156,20 @@ async fn handler( let connector = tokio_rustls::TlsConnector::from(std::sync::Arc::new(config)); let mut parts = authority.split(":"); let host = parts.next().unwrap_or(&authority); - let domain = rustls::ServerName::try_from(host)?; + let domain = rustls::ServerName::try_from(host) + .map_err(|_| dns_error("invalid dns name".to_string(), 0))?; let stream = connector .connect(domain, tcp_stream) .await - .map_err(|e| crate::bindings::http::types::Error::ProtocolError(e.to_string()))?; + .map_err(|_| types::ErrorCode::TlsProtocolError)?; let (sender, conn) = timeout( connect_timeout, hyper::client::conn::http1::handshake(stream), ) .await - .map_err(|_| timeout_error("connection"))??; + .map_err(|_| types::ErrorCode::ConnectionTimeout)? + .map_err(|_| types::ErrorCode::ConnectionTimeout)?; let worker = preview2::spawn(async move { match conn.await { @@ -172,7 +189,8 @@ async fn handler( hyper::client::conn::http1::handshake(tcp_stream), ) .await - .map_err(|_| timeout_error("connection"))??; + .map_err(|_| types::ErrorCode::ConnectionTimeout)? + .map_err(|_| types::ErrorCode::HttpProtocolError)?; let worker = preview2::spawn(async move { match conn.await { @@ -187,9 +205,12 @@ async fn handler( let resp = timeout(first_byte_timeout, sender.send_request(request)) .await - .map_err(|_| timeout_error("first byte"))? - .map_err(hyper_protocol_error)? - .map(|body| body.map_err(|e| e.into()).boxed()); + .map_err(|_| types::ErrorCode::ConnectionReadTimeout)? + .map_err(|_| types::ErrorCode::HttpProtocolError)? + .map(|body| { + body.map_err(|_| types::ErrorCode::HttpProtocolError) + .boxed() + }); Ok(IncomingResponseInternal { resp, @@ -197,25 +218,6 @@ async fn handler( between_bytes_timeout, }) } - -pub fn timeout_error(kind: &str) -> types::Error { - types::Error::TimeoutError(format!("{kind} timed out")) -} - -pub fn http_protocol_error(e: http::Error) -> types::Error { - types::Error::ProtocolError(e.to_string()) -} - -pub fn hyper_protocol_error(e: hyper::Error) -> types::Error { - types::Error::ProtocolError(e.to_string()) -} - -fn invalid_url(e: std::io::Error) -> types::Error { - // TODO: DNS errors show up as a Custom io error, what subset of errors should we consider for - // InvalidUrl here? - types::Error::InvalidUrl(e.to_string()) -} - impl From for types::Method { fn from(method: http::Method) -> Self { if method == http::Method::GET { @@ -268,7 +270,7 @@ pub struct HostIncomingRequest { pub struct HostResponseOutparam { pub result: - tokio::sync::oneshot::Sender, types::Error>>, + tokio::sync::oneshot::Sender, types::ErrorCode>>, } pub struct HostOutgoingRequest { @@ -347,11 +349,11 @@ pub struct IncomingResponseInternal { } type FutureIncomingResponseHandle = - AbortOnDropJoinHandle>>; + AbortOnDropJoinHandle>>; pub enum HostFutureIncomingResponse { Pending(FutureIncomingResponseHandle), - Ready(anyhow::Result>), + Ready(anyhow::Result>), Consumed, } @@ -364,7 +366,9 @@ impl HostFutureIncomingResponse { matches!(self, Self::Ready(_)) } - pub fn unwrap_ready(self) -> anyhow::Result> { + pub fn unwrap_ready( + self, + ) -> anyhow::Result> { match self { Self::Ready(res) => res, Self::Pending(_) | Self::Consumed => { diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs index 00400a07c860..f8b692754d06 100644 --- a/crates/wasi-http/src/types_impl.rs +++ b/crates/wasi-http/src/types_impl.rs @@ -1,5 +1,5 @@ use crate::{ - bindings::http::types::{self, Error, Headers, Method, Scheme, StatusCode, Trailers}, + bindings::http::types::{self, Headers, Method, Scheme, StatusCode, Trailers}, body::{FinishMessage, HostFutureTrailers, HostIncomingBody, HostOutgoingBody}, types::{ FieldMap, HostFields, HostFutureIncomingResponse, HostIncomingRequest, @@ -17,7 +17,14 @@ use wasmtime_wasi::preview2::{ Pollable, Table, }; -impl crate::bindings::http::types::Host for T {} +impl crate::bindings::http::types::Host for T { + fn http_error( + &mut self, + _err: wasmtime::component::Resource, + ) -> wasmtime::Result> { + todo!() + } +} /// Take ownership of the underlying [`FieldMap`] associated with this fields resource. If the /// fields resource references another fields, the returned [`FieldMap`] will be cloned. @@ -525,7 +532,7 @@ impl crate::bindings::http::types::HostResponseOutparam for T { fn set( &mut self, id: Resource, - resp: Result, Error>, + resp: Result, types::ErrorCode>, ) -> wasmtime::Result<()> { let val = match resp { Ok(resp) => Ok(self.table().delete(resp)?.try_into()?), @@ -620,7 +627,7 @@ impl crate::bindings::http::types::HostFutureTrailers for T { fn get( &mut self, id: Resource, - ) -> wasmtime::Result>, Error>>> { + ) -> wasmtime::Result>, types::ErrorCode>>> { let trailers = self.table().get_mut(&id)?; match trailers { HostFutureTrailers::Waiting(_) => return Ok(None), @@ -773,7 +780,9 @@ impl crate::bindings::http::types::HostFutureIncomingResponse f fn get( &mut self, id: Resource, - ) -> wasmtime::Result, Error>, ()>>> { + ) -> wasmtime::Result< + Option, types::ErrorCode>, ()>>, + > { let resp = self.table().get_mut(&id)?; match resp { @@ -786,7 +795,7 @@ impl crate::bindings::http::types::HostFutureIncomingResponse f match std::mem::replace(resp, HostFutureIncomingResponse::Consumed).unwrap_ready() { Err(e) => { // Trapping if it's not possible to downcast to an wasi-http error - let e = e.downcast::()?; + let e = e.downcast::()?; return Ok(Some(Ok(Err(e)))); } diff --git a/crates/wasi-http/tests/all/main.rs b/crates/wasi-http/tests/all/main.rs index 45094d52e621..323d41892feb 100644 --- a/crates/wasi-http/tests/all/main.rs +++ b/crates/wasi-http/tests/all/main.rs @@ -15,7 +15,7 @@ use wasmtime_wasi::preview2::{ self, pipe::MemoryOutputPipe, Table, WasiCtx, WasiCtxBuilder, WasiView, }; use wasmtime_wasi_http::{ - bindings::http::types::Error, + bindings::http::types::ErrorCode, body::HyperIncomingBody, types::{self, HostFutureIncomingResponse, IncomingResponseInternal, OutgoingRequest}, WasiHttpCtx, WasiHttpView, @@ -128,7 +128,7 @@ async fn run_wasi_http( component_filename: &str, req: hyper::Request, send_request: Option, -) -> anyhow::Result>, Error>> { +) -> anyhow::Result>, ErrorCode>> { let stdout = MemoryOutputPipe::new(4096); let stderr = MemoryOutputPipe::new(4096); let table = Table::new(); @@ -457,7 +457,7 @@ async fn do_wasi_http_echo(uri: &str, url_header: Option<&str>) -> Result<()> { let request = request.body(BoxBody::new(StreamBody::new(stream::iter( body.chunks(16 * 1024) - .map(|chunk| Ok::<_, Error>(Frame::data(Bytes::copy_from_slice(chunk)))) + .map(|chunk| Ok::<_, ErrorCode>(Frame::data(Bytes::copy_from_slice(chunk)))) .collect::>(), ))))?; diff --git a/crates/wasi-http/wit/deps/http/handler.wit b/crates/wasi-http/wit/deps/http/handler.wit index 21b97a354cc7..a34a0649d5b0 100644 --- a/crates/wasi-http/wit/deps/http/handler.wit +++ b/crates/wasi-http/wit/deps/http/handler.wit @@ -22,7 +22,9 @@ interface incoming-handler { /// This interface defines a handler of outgoing HTTP Requests. It should be /// imported by components which wish to make HTTP Requests. interface outgoing-handler { - use types.{outgoing-request, request-options, future-incoming-response, error}; + use types.{ + outgoing-request, request-options, future-incoming-response, error-code + }; /// This function is invoked with an outgoing HTTP Request, and it returns /// a resource `future-incoming-response` which represents an HTTP Response @@ -37,5 +39,5 @@ interface outgoing-handler { handle: func( request: outgoing-request, options: option - ) -> result; + ) -> result; } diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit index 4662f3fd6ba8..500ee711e83d 100644 --- a/crates/wasi-http/wit/deps/http/types.wit +++ b/crates/wasi-http/wit/deps/http/types.wit @@ -3,7 +3,7 @@ /// their headers, trailers, and bodies. interface types { use wasi:clocks/monotonic-clock@0.2.0-rc-2023-11-05.{duration}; - use wasi:io/streams@0.2.0-rc-2023-11-05.{input-stream, output-stream}; + use wasi:io/streams@0.2.0-rc-2023-11-05.{input-stream, output-stream, error as stream-error}; use wasi:io/poll@0.2.0-rc-2023-11-05.{pollable}; /// This type corresponds to HTTP standard Methods. @@ -27,14 +27,65 @@ interface types { other(string) } - /// TODO: perhaps better align with HTTP semantics? - /// This type enumerates the different kinds of errors that may occur when - /// initially returning a response. - variant error { - invalid-url(string), - timeout-error(string), - protocol-error(string), - unexpected-error(string) + // The cases of this variant correspond to the IANA HTTP Proxy Error Types: + // https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types + variant error-code { + DNS-timeout, + DNS-error(DNS-error-payload), + destination-not-found, + destination-unavailable, + destination-IP-prohibited, + destination-IP-unroutable, + connection-refused, + connection-terminated, + connection-timeout, + connection-read-timeout, + connection-write-timeout, + connection-limit-reached, + TLS-protocol-error, + TLS-certificate-error, + TLS-alert-received(TLS-alert-received-payload), + HTTP-request-error(HTTP-request-error-payload), + HTTP-request-denied, + HTTP-response-incomplete, + HTTP-response-header-section-size(u32), + HTTP-response-header-size(field-size-payload), + HTTP-response-body-size(u32), + HTTP-response-trailer-section-size(u32), + HTTP-response-trailer-size(field-size-payload), + HTTP-response-transfer-coding(string), + HTTP-response-content-coding(string), + HTTP-response-timeout, + HTTP-upgrade-failed, + HTTP-protocol-error, + proxy-internal-response, + proxy-internal-error, + proxy-configuration-error, + proxy-loop-detected + } + + // Defines the case payload type for `DNS-error` above: + record DNS-error-payload { + rcode: string, + info-code: u16 + } + + // Defines the case payload type for `TLS-alert-received` above: + record TLS-alert-received-payload { + alert-ID: u8, + alert-message: string + } + + // Defines the case payload type for `HTTP-request-error` above: + record HTTP-request-error-payload { + status-code: u16, + status-phrase: string + } + + // Defines the case payload type for `HTTP-response-{header,trailer}-size` above: + record field-size-payload { + field-name: string, + field-size: u32 } /// This tyep enumerates the different kinds of errors that may occur when @@ -45,6 +96,18 @@ interface types { immutable } + /// Attempts to extract a http-related `error` from the stream `error` + /// provided. + /// + /// Stream operations which return `stream-error::last-operation-failed` have + /// a payload with more information about the operation that failed. This + /// payload can be passed through to this function to see if there's + /// http-related information about the error to return. + /// + /// Note that this function is fallible because not all stream-related errors + /// are http-related errors. + http-error: func(err: borrow) -> option; + /// Field keys are always strings. type field-key = string; @@ -263,7 +326,7 @@ interface types { /// implementation determine how to respond with an HTTP error response. set: static func( param: response-outparam, - response: result, + response: result, ); } @@ -338,7 +401,7 @@ interface types { /// as well as any trailers, were received successfully, or that an error /// occured receiving them. The optional `trailers` indicates whether or not /// trailers were present in the body. - get: func() -> option, error>>; + get: func() -> option, error-code>>; } /// Represents an outgoing HTTP Response. @@ -434,7 +497,7 @@ interface types { /// occured. Errors may also occur while consuming the response body, /// but those will be reported by the `incoming-body` and its /// `output-stream` child. - get: func() -> option>>; + get: func() -> option>>; } } diff --git a/crates/wasi/wit/deps/http/handler.wit b/crates/wasi/wit/deps/http/handler.wit index 21b97a354cc7..a34a0649d5b0 100644 --- a/crates/wasi/wit/deps/http/handler.wit +++ b/crates/wasi/wit/deps/http/handler.wit @@ -22,7 +22,9 @@ interface incoming-handler { /// This interface defines a handler of outgoing HTTP Requests. It should be /// imported by components which wish to make HTTP Requests. interface outgoing-handler { - use types.{outgoing-request, request-options, future-incoming-response, error}; + use types.{ + outgoing-request, request-options, future-incoming-response, error-code + }; /// This function is invoked with an outgoing HTTP Request, and it returns /// a resource `future-incoming-response` which represents an HTTP Response @@ -37,5 +39,5 @@ interface outgoing-handler { handle: func( request: outgoing-request, options: option - ) -> result; + ) -> result; } diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit index 4662f3fd6ba8..500ee711e83d 100644 --- a/crates/wasi/wit/deps/http/types.wit +++ b/crates/wasi/wit/deps/http/types.wit @@ -3,7 +3,7 @@ /// their headers, trailers, and bodies. interface types { use wasi:clocks/monotonic-clock@0.2.0-rc-2023-11-05.{duration}; - use wasi:io/streams@0.2.0-rc-2023-11-05.{input-stream, output-stream}; + use wasi:io/streams@0.2.0-rc-2023-11-05.{input-stream, output-stream, error as stream-error}; use wasi:io/poll@0.2.0-rc-2023-11-05.{pollable}; /// This type corresponds to HTTP standard Methods. @@ -27,14 +27,65 @@ interface types { other(string) } - /// TODO: perhaps better align with HTTP semantics? - /// This type enumerates the different kinds of errors that may occur when - /// initially returning a response. - variant error { - invalid-url(string), - timeout-error(string), - protocol-error(string), - unexpected-error(string) + // The cases of this variant correspond to the IANA HTTP Proxy Error Types: + // https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types + variant error-code { + DNS-timeout, + DNS-error(DNS-error-payload), + destination-not-found, + destination-unavailable, + destination-IP-prohibited, + destination-IP-unroutable, + connection-refused, + connection-terminated, + connection-timeout, + connection-read-timeout, + connection-write-timeout, + connection-limit-reached, + TLS-protocol-error, + TLS-certificate-error, + TLS-alert-received(TLS-alert-received-payload), + HTTP-request-error(HTTP-request-error-payload), + HTTP-request-denied, + HTTP-response-incomplete, + HTTP-response-header-section-size(u32), + HTTP-response-header-size(field-size-payload), + HTTP-response-body-size(u32), + HTTP-response-trailer-section-size(u32), + HTTP-response-trailer-size(field-size-payload), + HTTP-response-transfer-coding(string), + HTTP-response-content-coding(string), + HTTP-response-timeout, + HTTP-upgrade-failed, + HTTP-protocol-error, + proxy-internal-response, + proxy-internal-error, + proxy-configuration-error, + proxy-loop-detected + } + + // Defines the case payload type for `DNS-error` above: + record DNS-error-payload { + rcode: string, + info-code: u16 + } + + // Defines the case payload type for `TLS-alert-received` above: + record TLS-alert-received-payload { + alert-ID: u8, + alert-message: string + } + + // Defines the case payload type for `HTTP-request-error` above: + record HTTP-request-error-payload { + status-code: u16, + status-phrase: string + } + + // Defines the case payload type for `HTTP-response-{header,trailer}-size` above: + record field-size-payload { + field-name: string, + field-size: u32 } /// This tyep enumerates the different kinds of errors that may occur when @@ -45,6 +96,18 @@ interface types { immutable } + /// Attempts to extract a http-related `error` from the stream `error` + /// provided. + /// + /// Stream operations which return `stream-error::last-operation-failed` have + /// a payload with more information about the operation that failed. This + /// payload can be passed through to this function to see if there's + /// http-related information about the error to return. + /// + /// Note that this function is fallible because not all stream-related errors + /// are http-related errors. + http-error: func(err: borrow) -> option; + /// Field keys are always strings. type field-key = string; @@ -263,7 +326,7 @@ interface types { /// implementation determine how to respond with an HTTP error response. set: static func( param: response-outparam, - response: result, + response: result, ); } @@ -338,7 +401,7 @@ interface types { /// as well as any trailers, were received successfully, or that an error /// occured receiving them. The optional `trailers` indicates whether or not /// trailers were present in the body. - get: func() -> option, error>>; + get: func() -> option, error-code>>; } /// Represents an outgoing HTTP Response. @@ -434,7 +497,7 @@ interface types { /// occured. Errors may also occur while consuming the response body, /// but those will be reported by the `incoming-body` and its /// `output-stream` child. - get: func() -> option>>; + get: func() -> option>>; } } diff --git a/src/commands/serve.rs b/src/commands/serve.rs index 927cc755b728..e6e02bd97e41 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -14,7 +14,9 @@ use wasmtime::{Engine, Store, StoreLimits}; use wasmtime_wasi::preview2::{ self, StreamError, StreamResult, Table, WasiCtx, WasiCtxBuilder, WasiView, }; -use wasmtime_wasi_http::{body::HyperOutgoingBody, WasiHttpCtx, WasiHttpView}; +use wasmtime_wasi_http::{ + bindings::http::types as http_types, body::HyperOutgoingBody, WasiHttpCtx, WasiHttpView, +}; #[cfg(feature = "wasi-nn")] use wasmtime_wasi_nn::WasiNnCtx; @@ -363,9 +365,30 @@ impl hyper::service::Service for ProxyHandler { let mut store = inner.cmd.new_store(&inner.engine, req_id)?; - let req = store - .data_mut() - .new_incoming_request(req.map(|body| body.map_err(|e| e.into()).boxed()))?; + let req = store.data_mut().new_incoming_request(req.map(|body| { + body.map_err(|err| { + if err.is_timeout() { + http_types::ErrorCode::HttpResponseTimeout + } else if err.is_parse_status() || err.is_user() { + http_types::ErrorCode::HttpRequestError( + http_types::HttpRequestErrorPayload { + status_code: 400, + status_phrase: "".to_string(), + }, + ) + } else if err.is_parse_too_large() { + http_types::ErrorCode::HttpRequestError( + http_types::HttpRequestErrorPayload { + status_code: 413, + status_phrase: "".to_string(), + }, + ) + } else { + http_types::ErrorCode::HttpProtocolError + } + }) + .boxed() + }))?; let out = store.data_mut().new_response_outparam(sender)?; From 045ebb250fcf851635a89a87e301e602d1fad675 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Tue, 7 Nov 2023 14:27:26 -0800 Subject: [PATCH 2/7] Optional fields, and align with upstream pr --- .../http_outbound_request_unknown_method.rs | 2 +- ...ttp_outbound_request_unsupported_scheme.rs | 2 +- crates/wasi-http/src/lib.rs | 8 ++-- crates/wasi-http/wit/deps/http/types.wit | 45 +++++++++++-------- crates/wasi/wit/deps/http/types.wit | 45 +++++++++++-------- src/commands/serve.rs | 8 ++-- 6 files changed, 62 insertions(+), 48 deletions(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs index 8de9af2ca3ec..96101f9005a0 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs @@ -15,7 +15,7 @@ fn main() { .downcast::() .expect("expected a wasi-http ErrorCode"), ErrorCode::HttpRequestError(HttpRequestErrorPayload { - status_code: 405, + status_code: Some(405), .. }), )); diff --git a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs index d7b59d891131..ffbf14f339cb 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs @@ -15,7 +15,7 @@ fn main() { .downcast::() .expect("expected a wasi-http ErrorCode"), ErrorCode::HttpRequestError(HttpRequestErrorPayload { - status_code: 400, + status_code: Some(400), .. }), )); diff --git a/crates/wasi-http/src/lib.rs b/crates/wasi-http/src/lib.rs index faa983c3d031..60b9dba1e750 100644 --- a/crates/wasi-http/src/lib.rs +++ b/crates/wasi-http/src/lib.rs @@ -39,8 +39,8 @@ pub mod bindings { pub(crate) fn dns_error(rcode: String, info_code: u16) -> bindings::http::types::ErrorCode { bindings::http::types::ErrorCode::DnsError(bindings::http::types::DnsErrorPayload { - rcode, - info_code, + rcode: Some(rcode), + info_code: Some(info_code), }) } @@ -50,8 +50,8 @@ pub(crate) fn http_request_error( ) -> bindings::http::types::ErrorCode { bindings::http::types::ErrorCode::HttpRequestError( bindings::http::types::HttpRequestErrorPayload { - status_code, - status_phrase, + status_code: Some(status_code), + status_phrase: Some(status_phrase), }, ) } diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit index 500ee711e83d..0f99e5a20d0a 100644 --- a/crates/wasi-http/wit/deps/http/types.wit +++ b/crates/wasi-http/wit/deps/http/types.wit @@ -27,8 +27,8 @@ interface types { other(string) } - // The cases of this variant correspond to the IANA HTTP Proxy Error Types: - // https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types + /// The cases of this variant correspond to the IANA HTTP Proxy Error Types: + /// https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types variant error-code { DNS-timeout, DNS-error(DNS-error-payload), @@ -48,13 +48,13 @@ interface types { HTTP-request-error(HTTP-request-error-payload), HTTP-request-denied, HTTP-response-incomplete, - HTTP-response-header-section-size(u32), + HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), - HTTP-response-body-size(u32), - HTTP-response-trailer-section-size(u32), + HTTP-response-body-size(option), + HTTP-response-trailer-section-size(option), HTTP-response-trailer-size(field-size-payload), - HTTP-response-transfer-coding(string), - HTTP-response-content-coding(string), + HTTP-response-transfer-coding(option), + HTTP-response-content-coding(option), HTTP-response-timeout, HTTP-upgrade-failed, HTTP-protocol-error, @@ -64,28 +64,28 @@ interface types { proxy-loop-detected } - // Defines the case payload type for `DNS-error` above: + /// Defines the case payload type for `DNS-error` above: record DNS-error-payload { - rcode: string, - info-code: u16 + rcode: option, + info-code: option } - // Defines the case payload type for `TLS-alert-received` above: + /// Defines the case payload type for `TLS-alert-received` above: record TLS-alert-received-payload { - alert-ID: u8, - alert-message: string + alert-ID: option, + alert-message: option } - // Defines the case payload type for `HTTP-request-error` above: + /// Defines the case payload type for `HTTP-request-error` above: record HTTP-request-error-payload { - status-code: u16, - status-phrase: string + status-code: option, + status-phrase: option } - // Defines the case payload type for `HTTP-response-{header,trailer}-size` above: + /// Defines the case payload type for `HTTP-response-{header,trailer}-size` above: record field-size-payload { - field-name: string, - field-size: u32 + field-name: option, + field-size: option } /// This tyep enumerates the different kinds of errors that may occur when @@ -108,6 +108,13 @@ interface types { /// are http-related errors. http-error: func(err: borrow) -> option; + /// This tyep enumerates the different kinds of errors that may occur when + /// setting or appending to a `fields` resource. + variant header-error { + invalid-syntax, + forbidden, + } + /// Field keys are always strings. type field-key = string; diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit index 500ee711e83d..0f99e5a20d0a 100644 --- a/crates/wasi/wit/deps/http/types.wit +++ b/crates/wasi/wit/deps/http/types.wit @@ -27,8 +27,8 @@ interface types { other(string) } - // The cases of this variant correspond to the IANA HTTP Proxy Error Types: - // https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types + /// The cases of this variant correspond to the IANA HTTP Proxy Error Types: + /// https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types variant error-code { DNS-timeout, DNS-error(DNS-error-payload), @@ -48,13 +48,13 @@ interface types { HTTP-request-error(HTTP-request-error-payload), HTTP-request-denied, HTTP-response-incomplete, - HTTP-response-header-section-size(u32), + HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), - HTTP-response-body-size(u32), - HTTP-response-trailer-section-size(u32), + HTTP-response-body-size(option), + HTTP-response-trailer-section-size(option), HTTP-response-trailer-size(field-size-payload), - HTTP-response-transfer-coding(string), - HTTP-response-content-coding(string), + HTTP-response-transfer-coding(option), + HTTP-response-content-coding(option), HTTP-response-timeout, HTTP-upgrade-failed, HTTP-protocol-error, @@ -64,28 +64,28 @@ interface types { proxy-loop-detected } - // Defines the case payload type for `DNS-error` above: + /// Defines the case payload type for `DNS-error` above: record DNS-error-payload { - rcode: string, - info-code: u16 + rcode: option, + info-code: option } - // Defines the case payload type for `TLS-alert-received` above: + /// Defines the case payload type for `TLS-alert-received` above: record TLS-alert-received-payload { - alert-ID: u8, - alert-message: string + alert-ID: option, + alert-message: option } - // Defines the case payload type for `HTTP-request-error` above: + /// Defines the case payload type for `HTTP-request-error` above: record HTTP-request-error-payload { - status-code: u16, - status-phrase: string + status-code: option, + status-phrase: option } - // Defines the case payload type for `HTTP-response-{header,trailer}-size` above: + /// Defines the case payload type for `HTTP-response-{header,trailer}-size` above: record field-size-payload { - field-name: string, - field-size: u32 + field-name: option, + field-size: option } /// This tyep enumerates the different kinds of errors that may occur when @@ -108,6 +108,13 @@ interface types { /// are http-related errors. http-error: func(err: borrow) -> option; + /// This tyep enumerates the different kinds of errors that may occur when + /// setting or appending to a `fields` resource. + variant header-error { + invalid-syntax, + forbidden, + } + /// Field keys are always strings. type field-key = string; diff --git a/src/commands/serve.rs b/src/commands/serve.rs index e6e02bd97e41..4dad926df7a5 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -372,15 +372,15 @@ impl hyper::service::Service for ProxyHandler { } else if err.is_parse_status() || err.is_user() { http_types::ErrorCode::HttpRequestError( http_types::HttpRequestErrorPayload { - status_code: 400, - status_phrase: "".to_string(), + status_code: Some(400), + status_phrase: Some("".to_string()), }, ) } else if err.is_parse_too_large() { http_types::ErrorCode::HttpRequestError( http_types::HttpRequestErrorPayload { - status_code: 413, - status_phrase: "".to_string(), + status_code: Some(413), + status_phrase: Some("".to_string()), }, ) } else { From 620b99347463485ce9084f79fcb551891455d6f1 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Wed, 8 Nov 2023 17:22:34 -0800 Subject: [PATCH 3/7] Update for upstream changes to the error-code variant --- .../http_outbound_request_unknown_method.rs | 7 ++-- ...ttp_outbound_request_unsupported_scheme.rs | 7 ++-- crates/wasi-http/src/http_impl.rs | 36 ++++++++----------- crates/wasi-http/src/lib.rs | 12 ------- crates/wasi-http/wit/deps/http/types.wit | 12 +++---- crates/wasi/wit/deps/http/types.wit | 12 +++---- src/commands/serve.rs | 14 +------- 7 files changed, 30 insertions(+), 70 deletions(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs index 96101f9005a0..149ee076766f 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{ErrorCode, HttpRequestErrorPayload, Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, Method, Scheme}; fn main() { let res = test_programs::http::request( @@ -14,9 +14,6 @@ fn main() { res.unwrap_err() .downcast::() .expect("expected a wasi-http ErrorCode"), - ErrorCode::HttpRequestError(HttpRequestErrorPayload { - status_code: Some(405), - .. - }), + ErrorCode::HttpProtocolError, )); } diff --git a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs index ffbf14f339cb..84ce1a5ba92c 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs @@ -1,4 +1,4 @@ -use test_programs::wasi::http::types::{ErrorCode, HttpRequestErrorPayload, Method, Scheme}; +use test_programs::wasi::http::types::{ErrorCode, Method, Scheme}; fn main() { let res = test_programs::http::request( @@ -14,9 +14,6 @@ fn main() { res.unwrap_err() .downcast::() .expect("expected a wasi-http ErrorCode"), - ErrorCode::HttpRequestError(HttpRequestErrorPayload { - status_code: Some(400), - .. - }), + ErrorCode::HttpProtocolError, )); } diff --git a/crates/wasi-http/src/http_impl.rs b/crates/wasi-http/src/http_impl.rs index a3070ed99877..ce98c659266b 100644 --- a/crates/wasi-http/src/http_impl.rs +++ b/crates/wasi-http/src/http_impl.rs @@ -3,7 +3,6 @@ use crate::{ outgoing_handler, types::{self, Scheme}, }, - http_request_error, types::{HostFutureIncomingResponse, HostOutgoingRequest, OutgoingRequest}, WasiHttpView, }; @@ -35,32 +34,27 @@ impl outgoing_handler::Host for T { let req = self.table().delete(request_id)?; let method = match req.method { - crate::bindings::http::types::Method::Get => Method::GET, - crate::bindings::http::types::Method::Head => Method::HEAD, - crate::bindings::http::types::Method::Post => Method::POST, - crate::bindings::http::types::Method::Put => Method::PUT, - crate::bindings::http::types::Method::Delete => Method::DELETE, - crate::bindings::http::types::Method::Connect => Method::CONNECT, - crate::bindings::http::types::Method::Options => Method::OPTIONS, - crate::bindings::http::types::Method::Trace => Method::TRACE, - crate::bindings::http::types::Method::Patch => Method::PATCH, - crate::bindings::http::types::Method::Other(method) => { - return Ok(Err(http_request_error( - 405, - format!("unknown method {method}"), - ))); + types::Method::Get => Method::GET, + types::Method::Head => Method::HEAD, + types::Method::Post => Method::POST, + types::Method::Put => Method::PUT, + types::Method::Delete => Method::DELETE, + types::Method::Connect => Method::CONNECT, + types::Method::Options => Method::OPTIONS, + types::Method::Trace => Method::TRACE, + types::Method::Patch => Method::PATCH, + types::Method::Other(_) => { + // We can't support arbitrary methods + return Ok(Err(types::ErrorCode::HttpProtocolError)); } }; let (use_tls, scheme, port) = match req.scheme.unwrap_or(Scheme::Https) { Scheme::Http => (false, "http://", 80), Scheme::Https => (true, "https://", 443), - Scheme::Other(scheme) => { - return Ok(Err(http_request_error( - 400, - format!("unsupported scheme {scheme}"), - ))) - } + + // We can only support http/https + Scheme::Other(_) => return Ok(Err(types::ErrorCode::HttpProtocolError)), }; let authority = if let Some(authority) = req.authority { diff --git a/crates/wasi-http/src/lib.rs b/crates/wasi-http/src/lib.rs index 60b9dba1e750..486c6b776d96 100644 --- a/crates/wasi-http/src/lib.rs +++ b/crates/wasi-http/src/lib.rs @@ -43,15 +43,3 @@ pub(crate) fn dns_error(rcode: String, info_code: u16) -> bindings::http::types: info_code: Some(info_code), }) } - -pub(crate) fn http_request_error( - status_code: u16, - status_phrase: String, -) -> bindings::http::types::ErrorCode { - bindings::http::types::ErrorCode::HttpRequestError( - bindings::http::types::HttpRequestErrorPayload { - status_code: Some(status_code), - status_phrase: Some(status_phrase), - }, - ) -} diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit index 0f99e5a20d0a..364b0c4316c9 100644 --- a/crates/wasi-http/wit/deps/http/types.wit +++ b/crates/wasi-http/wit/deps/http/types.wit @@ -45,8 +45,12 @@ interface types { TLS-protocol-error, TLS-certificate-error, TLS-alert-received(TLS-alert-received-payload), - HTTP-request-error(HTTP-request-error-payload), HTTP-request-denied, + HTTP-request-length-required, + HTTP-request-content-too-large, + HTTP-request-URI-too-long, + HTTP-request-header-section-size(option), + HTTP-request-header-size(option), HTTP-response-incomplete, HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), @@ -76,12 +80,6 @@ interface types { alert-message: option } - /// Defines the case payload type for `HTTP-request-error` above: - record HTTP-request-error-payload { - status-code: option, - status-phrase: option - } - /// Defines the case payload type for `HTTP-response-{header,trailer}-size` above: record field-size-payload { field-name: option, diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit index 0f99e5a20d0a..364b0c4316c9 100644 --- a/crates/wasi/wit/deps/http/types.wit +++ b/crates/wasi/wit/deps/http/types.wit @@ -45,8 +45,12 @@ interface types { TLS-protocol-error, TLS-certificate-error, TLS-alert-received(TLS-alert-received-payload), - HTTP-request-error(HTTP-request-error-payload), HTTP-request-denied, + HTTP-request-length-required, + HTTP-request-content-too-large, + HTTP-request-URI-too-long, + HTTP-request-header-section-size(option), + HTTP-request-header-size(option), HTTP-response-incomplete, HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), @@ -76,12 +80,6 @@ interface types { alert-message: option } - /// Defines the case payload type for `HTTP-request-error` above: - record HTTP-request-error-payload { - status-code: option, - status-phrase: option - } - /// Defines the case payload type for `HTTP-response-{header,trailer}-size` above: record field-size-payload { field-name: option, diff --git a/src/commands/serve.rs b/src/commands/serve.rs index 4dad926df7a5..cac97c0305d4 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -369,20 +369,8 @@ impl hyper::service::Service for ProxyHandler { body.map_err(|err| { if err.is_timeout() { http_types::ErrorCode::HttpResponseTimeout - } else if err.is_parse_status() || err.is_user() { - http_types::ErrorCode::HttpRequestError( - http_types::HttpRequestErrorPayload { - status_code: Some(400), - status_phrase: Some("".to_string()), - }, - ) } else if err.is_parse_too_large() { - http_types::ErrorCode::HttpRequestError( - http_types::HttpRequestErrorPayload { - status_code: Some(413), - status_phrase: Some("".to_string()), - }, - ) + http_types::ErrorCode::HttpRequestContentTooLarge } else { http_types::ErrorCode::HttpProtocolError } From c1b02904581b4fab8f8ad03c5b492b0c92df73a6 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Thu, 9 Nov 2023 17:22:27 -0800 Subject: [PATCH 4/7] Sync with the upstream implementation --- .../http_outbound_request_unknown_method.rs | 12 ++-- crates/wasi-http/src/http_impl.rs | 41 +++++++------ crates/wasi-http/src/types_impl.rs | 2 +- crates/wasi-http/wit/deps/http/types.wit | 57 ++++++++++--------- crates/wasi/wit/deps/http/types.wit | 57 ++++++++++--------- src/commands/serve.rs | 4 +- 6 files changed, 90 insertions(+), 83 deletions(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs index 149ee076766f..50d5751f5458 100644 --- a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs +++ b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs @@ -1,8 +1,8 @@ -use test_programs::wasi::http::types::{ErrorCode, Method, Scheme}; +use test_programs::wasi::http::types::{Method, Scheme}; fn main() { let res = test_programs::http::request( - Method::Other("OTHER".to_owned()), + Method::Other("bad\nmethod".to_owned()), Scheme::Http, "localhost:3000", "/", @@ -10,10 +10,6 @@ fn main() { None, ); - assert!(matches!( - res.unwrap_err() - .downcast::() - .expect("expected a wasi-http ErrorCode"), - ErrorCode::HttpProtocolError, - )); + // This error arises from input validation in the `set_method` function on `OutgoingRequest`. + assert_eq!(res.unwrap_err().to_string(), "failed to set method"); } diff --git a/crates/wasi-http/src/http_impl.rs b/crates/wasi-http/src/http_impl.rs index ce98c659266b..040348a51226 100644 --- a/crates/wasi-http/src/http_impl.rs +++ b/crates/wasi-http/src/http_impl.rs @@ -32,8 +32,9 @@ impl outgoing_handler::Host for T { .unwrap_or(std::time::Duration::from_millis(600 * 1000)); let req = self.table().delete(request_id)?; + let mut builder = hyper::Request::builder(); - let method = match req.method { + builder = builder.method(match req.method { types::Method::Get => Method::GET, types::Method::Head => Method::HEAD, types::Method::Post => Method::POST, @@ -43,15 +44,15 @@ impl outgoing_handler::Host for T { types::Method::Options => Method::OPTIONS, types::Method::Trace => Method::TRACE, types::Method::Patch => Method::PATCH, - types::Method::Other(_) => { - // We can't support arbitrary methods - return Ok(Err(types::ErrorCode::HttpProtocolError)); - } - }; + types::Method::Other(m) => match hyper::Method::from_bytes(m.as_bytes()) { + Ok(method) => method, + Err(_) => return Ok(Err(types::ErrorCode::HttpRequestMethodInvalid)), + }, + }); let (use_tls, scheme, port) = match req.scheme.unwrap_or(Scheme::Https) { - Scheme::Http => (false, "http://", 80), - Scheme::Https => (true, "https://", 443), + Scheme::Http => (false, http::uri::Scheme::HTTP, 80), + Scheme::Https => (true, http::uri::Scheme::HTTPS, 443), // We can only support http/https Scheme::Other(_) => return Ok(Err(types::ErrorCode::HttpProtocolError)), @@ -66,14 +67,20 @@ impl outgoing_handler::Host for T { } else { String::new() }; + builder = builder.header(hyper::header::HOST, &authority); + + let mut uri = http::Uri::builder() + .scheme(scheme) + .authority(authority.clone()); + + if let Some(path) = req.path_with_query { + uri = uri.path_and_query(path); + } - let mut builder = hyper::Request::builder() - .method(method) - .uri(format!( - "{scheme}{authority}{}", - req.path_with_query.as_ref().map_or("", String::as_ref) - )) - .header(hyper::header::HOST, &authority); + builder = builder.uri(uri.build().map_err(|e| { + eprintln!("uri build error: {e:#?}"); + types::ErrorCode::HttpRequestUriInvalid + })?); for (k, v) in req.headers.iter() { builder = builder.header(k, v); @@ -81,11 +88,11 @@ impl outgoing_handler::Host for T { let body = req .body - .unwrap_or_else(|| Empty::::new().map_err(|_| unreachable!()).boxed()); + .unwrap_or_else(|| Empty::::new().map_err(|_| todo!("thing")).boxed()); let request = builder .body(body) - .map_err(|_| types::ErrorCode::HttpProtocolError)?; + .map_err(|err| types::ErrorCode::InternalError(Some(err.to_string())))?; Ok(Ok(self.send_request(OutgoingRequest { use_tls, diff --git a/crates/wasi-http/src/types_impl.rs b/crates/wasi-http/src/types_impl.rs index f8b692754d06..cf35b5b3b21f 100644 --- a/crates/wasi-http/src/types_impl.rs +++ b/crates/wasi-http/src/types_impl.rs @@ -18,7 +18,7 @@ use wasmtime_wasi::preview2::{ }; impl crate::bindings::http::types::Host for T { - fn http_error( + fn http_error_code( &mut self, _err: wasmtime::component::Resource, ) -> wasmtime::Result> { diff --git a/crates/wasi-http/wit/deps/http/types.wit b/crates/wasi-http/wit/deps/http/types.wit index 364b0c4316c9..41226894eb2c 100644 --- a/crates/wasi-http/wit/deps/http/types.wit +++ b/crates/wasi-http/wit/deps/http/types.wit @@ -27,7 +27,7 @@ interface types { other(string) } - /// The cases of this variant correspond to the IANA HTTP Proxy Error Types: + /// These cases are inspired by the IANA HTTP Proxy Error Types: /// https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types variant error-code { DNS-timeout, @@ -47,14 +47,18 @@ interface types { TLS-alert-received(TLS-alert-received-payload), HTTP-request-denied, HTTP-request-length-required, - HTTP-request-content-too-large, + HTTP-request-body-size(option), + HTTP-request-method-invalid, + HTTP-request-URI-invalid, HTTP-request-URI-too-long, HTTP-request-header-section-size(option), HTTP-request-header-size(option), + HTTP-request-trailer-section-size(option), + HTTP-request-trailer-size(field-size-payload), HTTP-response-incomplete, HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), - HTTP-response-body-size(option), + HTTP-response-body-size(option), HTTP-response-trailer-section-size(option), HTTP-response-trailer-size(field-size-payload), HTTP-response-transfer-coding(option), @@ -62,10 +66,14 @@ interface types { HTTP-response-timeout, HTTP-upgrade-failed, HTTP-protocol-error, - proxy-internal-response, - proxy-internal-error, - proxy-configuration-error, - proxy-loop-detected + loop-detected, + configuration-error, + /// This is a catch-all error for anything that doesn't fit cleanly into a + /// more specific case. It also includes an optional string for an + /// unstructured description of the error. Users should not depend on the + /// string for diagnosing errors, as it's not required to be consistent + /// between implementations. + internal-error(option) } /// Defines the case payload type for `DNS-error` above: @@ -76,7 +84,7 @@ interface types { /// Defines the case payload type for `TLS-alert-received` above: record TLS-alert-received-payload { - alert-ID: option, + alert-id: option, alert-message: option } @@ -86,14 +94,6 @@ interface types { field-size: option } - /// This tyep enumerates the different kinds of errors that may occur when - /// setting or appending to a `fields` resource. - variant header-error { - invalid-syntax, - forbidden, - immutable - } - /// Attempts to extract a http-related `error` from the stream `error` /// provided. /// @@ -104,13 +104,23 @@ interface types { /// /// Note that this function is fallible because not all stream-related errors /// are http-related errors. - http-error: func(err: borrow) -> option; + http-error-code: func(err: borrow) -> option; - /// This tyep enumerates the different kinds of errors that may occur when + /// This type enumerates the different kinds of errors that may occur when /// setting or appending to a `fields` resource. variant header-error { + /// This error indicates that a `field-key` or `field-value` was + /// syntactically invalid when used with an operation that sets headers in a + /// `fields`. invalid-syntax, + + /// This error indicates that a forbidden `field-key` was used when trying + /// to set a header in a `fields`. forbidden, + + /// This error indicates that the operation on the `fields` was not + /// permitted because the fields are immutable. + immutable, } /// Field keys are always strings. @@ -151,21 +161,14 @@ interface types { /// Set all of the values for a key. Clears any existing values for that /// key, if they have been set. - /// - /// The operation can fail if the name or value arguments are invalid, or if - /// the name is forbidden. set: func(name: field-key, value: list) -> result<_, header-error>; /// Delete all values for a key. Does nothing if no values for the key - /// exist. Returns and error if the `field-key` is syntactically invalid, or - /// if the `field-key` is forbidden. + /// exist. delete: func(name: field-key) -> result<_, header-error>; /// Append a value for a key. Does not change or delete any existing /// values for that key. - /// - /// The operation can fail if the name or value arguments are invalid, or if - /// the name is forbidden. append: func(name: field-key, value: field-value) -> result<_, header-error>; /// Retrieve the full set of keys and values in the Fields. Like the @@ -218,7 +221,7 @@ interface types { resource outgoing-request { /// Construct a new `outgoing-request` with a default `method` of `GET`, and - /// default values for `path-with-query`, `scheme`, and `authority. + /// `none` values for `path-with-query`, `scheme`, and `authority`. /// /// * `headers` is the HTTP Headers for the Request. /// diff --git a/crates/wasi/wit/deps/http/types.wit b/crates/wasi/wit/deps/http/types.wit index 364b0c4316c9..41226894eb2c 100644 --- a/crates/wasi/wit/deps/http/types.wit +++ b/crates/wasi/wit/deps/http/types.wit @@ -27,7 +27,7 @@ interface types { other(string) } - /// The cases of this variant correspond to the IANA HTTP Proxy Error Types: + /// These cases are inspired by the IANA HTTP Proxy Error Types: /// https://www.iana.org/assignments/http-proxy-status/http-proxy-status.xhtml#table-http-proxy-error-types variant error-code { DNS-timeout, @@ -47,14 +47,18 @@ interface types { TLS-alert-received(TLS-alert-received-payload), HTTP-request-denied, HTTP-request-length-required, - HTTP-request-content-too-large, + HTTP-request-body-size(option), + HTTP-request-method-invalid, + HTTP-request-URI-invalid, HTTP-request-URI-too-long, HTTP-request-header-section-size(option), HTTP-request-header-size(option), + HTTP-request-trailer-section-size(option), + HTTP-request-trailer-size(field-size-payload), HTTP-response-incomplete, HTTP-response-header-section-size(option), HTTP-response-header-size(field-size-payload), - HTTP-response-body-size(option), + HTTP-response-body-size(option), HTTP-response-trailer-section-size(option), HTTP-response-trailer-size(field-size-payload), HTTP-response-transfer-coding(option), @@ -62,10 +66,14 @@ interface types { HTTP-response-timeout, HTTP-upgrade-failed, HTTP-protocol-error, - proxy-internal-response, - proxy-internal-error, - proxy-configuration-error, - proxy-loop-detected + loop-detected, + configuration-error, + /// This is a catch-all error for anything that doesn't fit cleanly into a + /// more specific case. It also includes an optional string for an + /// unstructured description of the error. Users should not depend on the + /// string for diagnosing errors, as it's not required to be consistent + /// between implementations. + internal-error(option) } /// Defines the case payload type for `DNS-error` above: @@ -76,7 +84,7 @@ interface types { /// Defines the case payload type for `TLS-alert-received` above: record TLS-alert-received-payload { - alert-ID: option, + alert-id: option, alert-message: option } @@ -86,14 +94,6 @@ interface types { field-size: option } - /// This tyep enumerates the different kinds of errors that may occur when - /// setting or appending to a `fields` resource. - variant header-error { - invalid-syntax, - forbidden, - immutable - } - /// Attempts to extract a http-related `error` from the stream `error` /// provided. /// @@ -104,13 +104,23 @@ interface types { /// /// Note that this function is fallible because not all stream-related errors /// are http-related errors. - http-error: func(err: borrow) -> option; + http-error-code: func(err: borrow) -> option; - /// This tyep enumerates the different kinds of errors that may occur when + /// This type enumerates the different kinds of errors that may occur when /// setting or appending to a `fields` resource. variant header-error { + /// This error indicates that a `field-key` or `field-value` was + /// syntactically invalid when used with an operation that sets headers in a + /// `fields`. invalid-syntax, + + /// This error indicates that a forbidden `field-key` was used when trying + /// to set a header in a `fields`. forbidden, + + /// This error indicates that the operation on the `fields` was not + /// permitted because the fields are immutable. + immutable, } /// Field keys are always strings. @@ -151,21 +161,14 @@ interface types { /// Set all of the values for a key. Clears any existing values for that /// key, if they have been set. - /// - /// The operation can fail if the name or value arguments are invalid, or if - /// the name is forbidden. set: func(name: field-key, value: list) -> result<_, header-error>; /// Delete all values for a key. Does nothing if no values for the key - /// exist. Returns and error if the `field-key` is syntactically invalid, or - /// if the `field-key` is forbidden. + /// exist. delete: func(name: field-key) -> result<_, header-error>; /// Append a value for a key. Does not change or delete any existing /// values for that key. - /// - /// The operation can fail if the name or value arguments are invalid, or if - /// the name is forbidden. append: func(name: field-key, value: field-value) -> result<_, header-error>; /// Retrieve the full set of keys and values in the Fields. Like the @@ -218,7 +221,7 @@ interface types { resource outgoing-request { /// Construct a new `outgoing-request` with a default `method` of `GET`, and - /// default values for `path-with-query`, `scheme`, and `authority. + /// `none` values for `path-with-query`, `scheme`, and `authority`. /// /// * `headers` is the HTTP Headers for the Request. /// diff --git a/src/commands/serve.rs b/src/commands/serve.rs index cac97c0305d4..5aad87decdbb 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -369,10 +369,8 @@ impl hyper::service::Service for ProxyHandler { body.map_err(|err| { if err.is_timeout() { http_types::ErrorCode::HttpResponseTimeout - } else if err.is_parse_too_large() { - http_types::ErrorCode::HttpRequestContentTooLarge } else { - http_types::ErrorCode::HttpProtocolError + http_types::ErrorCode::InternalError(Some(err.message().to_string())) } }) .boxed() From 52a445519a11d7b0042be723942a2a43e1f0217f Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 10 Nov 2023 13:45:44 -0800 Subject: [PATCH 5/7] Missed updating an error for riscv64 and s390x --- crates/wasi-http/src/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/wasi-http/src/types.rs b/crates/wasi-http/src/types.rs index a7ec00b9ed4f..a5bc098dada1 100644 --- a/crates/wasi-http/src/types.rs +++ b/crates/wasi-http/src/types.rs @@ -131,8 +131,8 @@ async fn handler( let (mut sender, worker) = if use_tls { #[cfg(any(target_arch = "riscv64", target_arch = "s390x"))] { - return Err(crate::bindings::http::types::ErrorCode::UnexpectedError( - "unsupported architecture for SSL".to_string(), + return Err(crate::bindings::http::types::ErrorCode::InternalError( + Some("unsupported architecture for SSL".to_string()), )); } From ebabc9f906cf0eb02c1146232c42a37fc8ebdee4 Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 10 Nov 2023 16:06:28 -0800 Subject: [PATCH 6/7] More debuggable error prtest:full --- .../bin/http_outbound_request_invalid_dnsname.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs index 94fe91405d61..a37b2753619e 100644 --- a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs +++ b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs @@ -10,10 +10,13 @@ fn main() { None, ); - assert!(matches!( - res.unwrap_err() - .downcast::() - .expect("expected a wasi-http ErrorCode"), - ErrorCode::DnsError(_) - )); + let e = res.unwrap_err(); + assert!( + matches!( + e.downcast_ref::() + .expect("expected a wasi-http ErrorCode"), + ErrorCode::DnsError(_) + ), + "Unexpected error: {e:#?}" + ); } From 2c275801a468a0325dcac584b31c68e42532c9da Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 10 Nov 2023 16:41:48 -0800 Subject: [PATCH 7/7] Try to stabilize the test on windows --- .../src/bin/http_outbound_request_invalid_dnsname.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs index a37b2753619e..0b6deddef755 100644 --- a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs +++ b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs @@ -15,7 +15,7 @@ fn main() { matches!( e.downcast_ref::() .expect("expected a wasi-http ErrorCode"), - ErrorCode::DnsError(_) + ErrorCode::DnsError(_) | ErrorCode::ConnectionRefused, ), "Unexpected error: {e:#?}" );