From 32f6da45eb37d97c64d875f4adadedfe96736749 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 21 Aug 2024 12:52:22 +0200 Subject: [PATCH] Revert RequestHandler type refactoring Signed-off-by: Ryan Levick --- crates/trigger-http2/src/outbound_http.rs | 12 +- crates/trigger-http2/src/server.rs | 218 ++++++++---------- tests/conformance-tests/src/lib.rs | 9 +- tests/testing-framework/Cargo.toml | 6 +- .../src/runtimes/in_process_spin.rs | 5 +- 5 files changed, 104 insertions(+), 146 deletions(-) diff --git a/crates/trigger-http2/src/outbound_http.rs b/crates/trigger-http2/src/outbound_http.rs index 38219b8f7..45e63ab75 100644 --- a/crates/trigger-http2/src/outbound_http.rs +++ b/crates/trigger-http2/src/outbound_http.rs @@ -11,17 +11,17 @@ use spin_http::routes::RouteMatch; use spin_outbound_networking::parse_service_chaining_target; use wasmtime_wasi_http::types::IncomingResponse; -use crate::server::RequestHandler; +use crate::HttpServer; /// An outbound HTTP interceptor that handles service chaining requests. pub struct OutboundHttpInterceptor { - handler: Arc, + server: Arc, origin: SelfRequestOrigin, } impl OutboundHttpInterceptor { - pub fn new(handler: Arc, origin: SelfRequestOrigin) -> Self { - Self { handler, origin } + pub fn new(server: Arc, origin: SelfRequestOrigin) -> Self { + Self { server, origin } } } @@ -41,10 +41,10 @@ impl spin_factor_outbound_http::OutboundHttpInterceptor for OutboundHttpIntercep let route_match = RouteMatch::synthetic(&component_id, uri.path()); let req = std::mem::take(request); let between_bytes_timeout = config.between_bytes_timeout; - let server = self.handler.clone(); + let server = self.server.clone(); let resp_fut = async move { match server - .handle_trigger_route(req, &route_match, Scheme::HTTP, CHAINED_CLIENT_ADDR) + .handle_trigger_route(req, route_match, Scheme::HTTP, CHAINED_CLIENT_ADDR) .await { Ok(resp) => Ok(Ok(IncomingResponse { diff --git a/crates/trigger-http2/src/server.rs b/crates/trigger-http2/src/server.rs index acea0eb9e..c01327fda 100644 --- a/crates/trigger-http2/src/server.rs +++ b/crates/trigger-http2/src/server.rs @@ -38,9 +38,18 @@ use crate::{ /// An HTTP server which runs Spin apps. pub struct HttpServer { + /// The address the server is listening on. + listen_addr: SocketAddr, + /// The TLS configuration for the server. tls_config: Option, + /// Request router. router: Router, - handler: Arc, + /// The app being triggered. + trigger_app: TriggerApp, + // Component ID -> component trigger config + component_trigger_configs: HashMap, + // Component ID -> handler type + component_handler_types: HashMap, } impl HttpServer { @@ -52,27 +61,32 @@ impl HttpServer { router: Router, component_trigger_configs: HashMap, ) -> anyhow::Result { + let component_handler_types = component_trigger_configs + .keys() + .map(|component_id| { + let component = trigger_app.get_component(component_id)?; + let handler_type = HandlerType::from_component(trigger_app.engine(), component)?; + Ok((component_id.clone(), handler_type)) + }) + .collect::>()?; Ok(Self { + listen_addr, tls_config, router, - handler: Arc::new(RequestHandler::new( - listen_addr, - trigger_app, - component_trigger_configs, - )?), + trigger_app, + component_trigger_configs, + component_handler_types, }) } /// Serve incoming requests over the provided [`TcpListener`]. pub async fn serve(self: Arc) -> anyhow::Result<()> { - let listener = TcpListener::bind(self.handler.listen_addr) - .await - .with_context(|| { - format!( - "Unable to listen on {listen_addr}", - listen_addr = self.handler.listen_addr - ) - })?; + let listener = TcpListener::bind(self.listen_addr).await.with_context(|| { + format!( + "Unable to listen on {listen_addr}", + listen_addr = self.listen_addr + ) + })?; if let Some(tls_config) = self.tls_config.clone() { self.serve_https(listener, tls_config).await?; } else { @@ -112,7 +126,7 @@ impl HttpServer { /// /// This method handles well known paths and routes requests to the handler when the router /// matches the requests path. - async fn handle( + pub async fn handle( self: &Arc, mut req: Request, server_scheme: Scheme, @@ -150,15 +164,70 @@ impl HttpServer { /// Handles a successful route match. pub async fn handle_trigger_route( self: &Arc, - req: Request, + mut req: Request, route_match: RouteMatch, server_scheme: Scheme, client_addr: SocketAddr, ) -> anyhow::Result> { - let res = self - .handler - .handle_trigger_route(req, &route_match, server_scheme, client_addr) - .await; + set_req_uri(&mut req, server_scheme.clone())?; + let app_id = self + .trigger_app + .app() + .get_metadata(APP_NAME_KEY)? + .unwrap_or_else(|| "".into()); + + let component_id = route_match.component_id(); + + spin_telemetry::metrics::monotonic_counter!( + spin.request_count = 1, + trigger_type = "http", + app_id = app_id, + component_id = component_id + ); + + let mut instance_builder = self.trigger_app.prepare(component_id)?; + + // Set up outbound HTTP request origin and service chaining + let origin = SelfRequestOrigin::create(server_scheme, &self.listen_addr)?; + instance_builder + .factor_builders() + .outbound_http() + .set_request_interceptor(OutboundHttpInterceptor::new(self.clone(), origin))?; + + // Prepare HTTP executor + let trigger_config = self.component_trigger_configs.get(component_id).unwrap(); + let handler_type = self.component_handler_types.get(component_id).unwrap(); + let executor = trigger_config + .executor + .as_ref() + .unwrap_or(&HttpExecutorType::Http); + + let res = match executor { + HttpExecutorType::Http => match handler_type { + HandlerType::Spin => { + SpinHttpExecutor + .execute(instance_builder, &route_match, req, client_addr) + .await + } + HandlerType::Wasi0_2 + | HandlerType::Wasi2023_11_10 + | HandlerType::Wasi2023_10_18 => { + WasiHttpExecutor { + handler_type: *handler_type, + } + .execute(instance_builder, &route_match, req, client_addr) + .await + } + }, + HttpExecutorType::Wagi(wagi_config) => { + let executor = WagiHttpExecutor { + wagi_config: wagi_config.clone(), + }; + executor + .execute(instance_builder, &route_match, req, client_addr) + .await + } + }; match res { Ok(res) => Ok(MatchedRoute::with_response_extension( res, @@ -174,7 +243,7 @@ impl HttpServer { /// Returns spin status information. fn app_info(&self, route: String) -> anyhow::Result> { - let info = AppInfo::new(self.handler.trigger_app.app()); + let info = AppInfo::new(self.trigger_app.app()); let body = serde_json::to_vec_pretty(&info)?; Ok(MatchedRoute::with_response_extension( Response::builder() @@ -278,7 +347,7 @@ impl HttpServer { println!("Available Routes:"); for (route, component_id) in self.router.routes() { println!(" {}: {}{}", component_id, base_url, route); - if let Some(component) = self.handler.trigger_app.app().get_component(component_id) { + if let Some(component) = self.trigger_app.app().get_component(component_id) { if let Some(description) = component.get_metadata(APP_DESCRIPTION_KEY)? { println!(" {}", description); } @@ -288,111 +357,6 @@ impl HttpServer { } } -/// Handles a routed HTTP trigger request. -pub struct RequestHandler { - /// The address the server is listening on. - pub(crate) listen_addr: SocketAddr, - /// The app being triggered. - trigger_app: TriggerApp, - // Component ID -> component trigger config - component_trigger_configs: HashMap, - // Component ID -> handler type - component_handler_types: HashMap, -} - -impl RequestHandler { - /// Create a new [`RequestHandler`] - pub fn new( - listen_addr: SocketAddr, - trigger_app: TriggerApp, - component_trigger_configs: HashMap, - ) -> anyhow::Result { - let component_handler_types = component_trigger_configs - .keys() - .map(|component_id| { - let component = trigger_app.get_component(component_id)?; - let handler_type = HandlerType::from_component(trigger_app.engine(), component)?; - Ok((component_id.clone(), handler_type)) - }) - .collect::>()?; - Ok(Self { - listen_addr, - trigger_app, - component_trigger_configs, - component_handler_types, - }) - } - - /// Handle a routed request. - pub async fn handle_trigger_route( - self: &Arc, - mut req: Request, - route_match: &RouteMatch, - server_scheme: Scheme, - client_addr: SocketAddr, - ) -> anyhow::Result> { - set_req_uri(&mut req, server_scheme.clone())?; - let app_id = self - .trigger_app - .app() - .get_metadata(APP_NAME_KEY)? - .unwrap_or_else(|| "".into()); - - let component_id = route_match.component_id(); - - spin_telemetry::metrics::monotonic_counter!( - spin.request_count = 1, - trigger_type = "http", - app_id = app_id, - component_id = component_id - ); - - let mut instance_builder = self.trigger_app.prepare(component_id)?; - - // Set up outbound HTTP request origin and service chaining - let origin = SelfRequestOrigin::create(server_scheme, &self.listen_addr)?; - instance_builder - .factor_builders() - .outbound_http() - .set_request_interceptor(OutboundHttpInterceptor::new(self.clone(), origin))?; - - // Prepare HTTP executor - let trigger_config = self.component_trigger_configs.get(component_id).unwrap(); - let handler_type = self.component_handler_types.get(component_id).unwrap(); - let executor = trigger_config - .executor - .as_ref() - .unwrap_or(&HttpExecutorType::Http); - - match executor { - HttpExecutorType::Http => match handler_type { - HandlerType::Spin => { - SpinHttpExecutor - .execute(instance_builder, route_match, req, client_addr) - .await - } - HandlerType::Wasi0_2 - | HandlerType::Wasi2023_11_10 - | HandlerType::Wasi2023_10_18 => { - WasiHttpExecutor { - handler_type: *handler_type, - } - .execute(instance_builder, route_match, req, client_addr) - .await - } - }, - HttpExecutorType::Wagi(wagi_config) => { - let executor = WagiHttpExecutor { - wagi_config: wagi_config.clone(), - }; - executor - .execute(instance_builder, route_match, req, client_addr) - .await - } - } - } -} - /// The incoming request's scheme and authority /// /// The incoming request's URI is relative to the server, so we need to set the scheme and authority. diff --git a/tests/conformance-tests/src/lib.rs b/tests/conformance-tests/src/lib.rs index bed1488a4..a03084819 100644 --- a/tests/conformance-tests/src/lib.rs +++ b/tests/conformance-tests/src/lib.rs @@ -1,5 +1,4 @@ use anyhow::Context as _; -use test_environment::http::Request; use testing_framework::runtimes::spin_cli::{SpinCli, SpinConfig}; /// Run a single conformance test against the supplied spin binary. @@ -57,11 +56,9 @@ pub fn run_test( let conformance_tests::config::Invocation::Http(mut invocation) = invocation; invocation.request.substitute_from_env(&mut env)?; let spin = env.runtime_mut(); - let actual = invocation.request.send(|request| { - let request = - Request::full(request.method, request.path, request.headers, request.body); - spin.make_http_request(request) - })?; + let actual = invocation + .request + .send(|request| spin.make_http_request(request))?; conformance_tests::assertions::assert_response(&invocation.response, &actual) .with_context(|| { diff --git a/tests/testing-framework/Cargo.toml b/tests/testing-framework/Cargo.toml index cc388246f..2118f93b3 100644 --- a/tests/testing-framework/Cargo.toml +++ b/tests/testing-framework/Cargo.toml @@ -14,12 +14,12 @@ regex = "1.10.2" reqwest = { workspace = true } temp-dir = "0.1.11" test-environment = { workspace = true } -spin-factors-executor = { path = "../../crates/factors-executor" } spin-app = { path = "../../crates/app" } -spin-trigger-http2 = { path = "../../crates/trigger-http2" } +spin-factors-executor = { path = "../../crates/factors-executor" } spin-http = { path = "../../crates/http" } -spin-trigger2 = { path = "../../crates/trigger2" } spin-loader = { path = "../../crates/loader" } +spin-trigger2 = { path = "../../crates/trigger2" } +spin-trigger-http2 = { path = "../../crates/trigger-http2" } toml = "0.8.6" tokio = "1.23" wasmtime-wasi-http = { workspace = true } diff --git a/tests/testing-framework/src/runtimes/in_process_spin.rs b/tests/testing-framework/src/runtimes/in_process_spin.rs index 16ad654af..7580842a2 100644 --- a/tests/testing-framework/src/runtimes/in_process_spin.rs +++ b/tests/testing-framework/src/runtimes/in_process_spin.rs @@ -3,7 +3,6 @@ use std::{path::PathBuf, sync::Arc}; use anyhow::Context as _; -use spin_http::routes::RouteMatch; use spin_trigger2::cli::{TriggerAppBuilder, TriggerAppOptions}; use spin_trigger_http2::{HttpServer, HttpTrigger}; use test_environment::{ @@ -54,12 +53,10 @@ impl InProcessSpin { } // TODO(rylev): convert body as well let req = builder.body(spin_http::body::empty()).unwrap(); - let route_match = RouteMatch::synthetic("test", "/"); let response = self .server - .handle_trigger_route( + .handle( req, - route_match, http::uri::Scheme::HTTP, (std::net::Ipv4Addr::LOCALHOST, 7000).into(), )