Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Factors] Change how incoming-request.authority is set. #2723

Merged
merged 3 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Change how incoming-request.authority is set.
Always rely on the `Host` header for authority incoming-reques.authority. To
determine where `self` requests should be routed, explicitly pass a
the listening address.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
  • Loading branch information
rylev committed Aug 19, 2024
commit 5bf5cea18774f4bb4a1cd58caf9ebd60ac324e0f
13 changes: 10 additions & 3 deletions crates/trigger-http2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ pub(crate) type InstanceState = ();

/// The Spin HTTP trigger.
pub struct HttpTrigger {
listen_addr: SocketAddr,
/// The address the server will listen on.
addr_to_bind: SocketAddr,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't change this; the CLI flag is --listen...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear ya. I wanted to draw attention to the fact that this might not be the actual SocketAddr the socket is listening on (in the case of a 0 port), but perhaps that's not worth the potential confusion in the other direction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the CLI arg is actually --address... But I'll still change it back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it is a trick:

#[clap(long = "listen", ...)]
pub address: SocketAddr,

tls_config: Option<TlsConfig>,
router: Router,
// Component ID -> component trigger config
Expand Down Expand Up @@ -112,7 +113,7 @@ impl Trigger for HttpTrigger {
);

Ok(Self {
listen_addr: cli_args.address,
addr_to_bind: cli_args.address,
tls_config: cli_args.into_tls_config(),
router,
component_trigger_configs,
Expand All @@ -121,7 +122,7 @@ impl Trigger for HttpTrigger {

async fn run(self, trigger_app: TriggerApp) -> anyhow::Result<()> {
let Self {
listen_addr,
addr_to_bind: listen_addr,
tls_config,
router,
component_trigger_configs,
Expand All @@ -131,6 +132,12 @@ impl Trigger for HttpTrigger {
.await
.with_context(|| format!("Unable to listen on {listen_addr}"))?;

// Get the address the server is actually listening on
// We can't use `self.listen_addr` because it might not
// be fully resolved (e.g, port 0).
let listen_addr = listener
.local_addr()
.context("failed to retrieve address server is listening on")?;
let server = Arc::new(HttpServer::new(
listen_addr,
trigger_app,
Expand Down
3 changes: 2 additions & 1 deletion crates/trigger-http2/src/outbound_http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{
sync::Arc,
};

use http::uri::Scheme;
use spin_factor_outbound_http::{
HostFutureIncomingResponse, InterceptOutcome, OutgoingRequestConfig, Request, SelfRequestOrigin,
};
Expand Down Expand Up @@ -42,7 +43,7 @@ impl spin_factor_outbound_http::OutboundHttpInterceptor for OutboundHttpIntercep
let server = self.server.clone();
let resp_fut = async move {
match server
.handle_trigger_route(req, route_match, CHAINED_CLIENT_ADDR)
.handle_trigger_route(req, route_match, Scheme::HTTP, CHAINED_CLIENT_ADDR)
.await
{
Ok(resp) => Ok(Ok(IncomingResponse {
Comment on lines 43 to 49
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should be setting the SelfRequestOrigin unconditionally in this interceptor. It seems like self requests ought to work in service-chained handlers? 🤔

Expand Down
53 changes: 40 additions & 13 deletions crates/trigger-http2/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use crate::{
};

pub struct HttpServer {
/// The address the server is listening on.
listen_addr: SocketAddr,
trigger_app: TriggerApp,
router: Router,
Expand Down Expand Up @@ -74,7 +75,8 @@ impl HttpServer {
self.print_startup_msgs("http", &listener)?;
loop {
let (stream, client_addr) = listener.accept().await?;
self.clone().serve_connection(stream, client_addr);
self.clone()
.serve_connection(stream, Scheme::HTTP, client_addr);
}
}

Expand All @@ -88,7 +90,9 @@ impl HttpServer {
loop {
let (stream, client_addr) = listener.accept().await?;
match acceptor.accept(stream).await {
Ok(stream) => self.clone().serve_connection(stream, client_addr),
Ok(stream) => self
.clone()
.serve_connection(stream, Scheme::HTTPS, client_addr),
Err(err) => tracing::error!(?err, "Failed to start TLS session"),
}
}
Expand All @@ -98,10 +102,10 @@ impl HttpServer {
async fn handle(
self: &Arc<Self>,
mut req: Request<Body>,
scheme: Scheme,
server_scheme: Scheme,
client_addr: SocketAddr,
) -> anyhow::Result<Response<Body>> {
set_req_uri(&mut req, scheme, self.listen_addr)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be done at the top of instrumented_service_fn? The http_span! there emits the scheme for o11y. As a bonus it would avoid needing to thread the server_scheme param through here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting it in instrumented_service_fn moves the logic up the call stack where we need to move it down the call stack.

The call graph is: instrumented_service_fn -> handle -> handle_trigger_route. The handler for service chained calls calls handle_trigger_route which is why we moved set_req_uri to that function - the service chained request also needs its uri manipulated. If we were to move that logic up to instrumented_service_fn, the issue this change is trying to address would persist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha. Perhaps it would make sense to split the scheme and host updates? 🤔

set_req_uri(&mut req, server_scheme.clone())?;
strip_forbidden_headers(&mut req);

spin_telemetry::extract_trace_context(&req);
Expand All @@ -124,7 +128,7 @@ impl HttpServer {

match self.router.route(&path) {
Ok(route_match) => {
self.handle_trigger_route(req, route_match, client_addr)
self.handle_trigger_route(req, route_match, server_scheme, client_addr)
.await
}
Err(_) => Self::not_found(NotFoundRouteKind::Normal(path.to_string())),
Expand All @@ -135,6 +139,7 @@ impl HttpServer {
self: &Arc<Self>,
req: Request<Body>,
route_match: RouteMatch,
server_scheme: Scheme,
client_addr: SocketAddr,
) -> anyhow::Result<Response<Body>> {
let app_id = self
Expand All @@ -155,9 +160,15 @@ impl HttpServer {
let mut instance_builder = self.trigger_app.prepare(component_id)?;

// Set up outbound HTTP request origin and service chaining
let uri = req.uri();
let origin = SelfRequestOrigin::from_uri(uri)
.with_context(|| format!("invalid request URI {uri:?}"))?;
let origin = SelfRequestOrigin {
scheme: server_scheme,
authority: self.listen_addr.to_string().parse().with_context(|| {
format!(
"server address '{}' is not a valid authority",
self.listen_addr
)
})?,
};
rylev marked this conversation as resolved.
Show resolved Hide resolved
instance_builder
.factor_builders()
.outbound_http()
Expand Down Expand Up @@ -259,6 +270,7 @@ impl HttpServer {
fn serve_connection<S: AsyncRead + AsyncWrite + Unpin + Send + 'static>(
self: Arc<Self>,
stream: S,
server_scheme: Scheme,
client_addr: SocketAddr,
) {
task::spawn(async move {
Expand All @@ -267,7 +279,11 @@ impl HttpServer {
.serve_connection(
TokioIo::new(stream),
service_fn(move |request| {
self.clone().instrumented_service_fn(client_addr, request)
self.clone().instrumented_service_fn(
server_scheme.clone(),
client_addr,
request,
)
}),
)
.await
Expand All @@ -279,6 +295,7 @@ impl HttpServer {

async fn instrumented_service_fn(
self: Arc<Self>,
server_scheme: Scheme,
client_addr: SocketAddr,
request: Request<Incoming>,
) -> anyhow::Result<Response<HyperOutgoingBody>> {
Expand All @@ -291,7 +308,7 @@ impl HttpServer {
body.map_err(wasmtime_wasi_http::hyper_response_error)
.boxed()
}),
Scheme::HTTP,
server_scheme,
client_addr,
)
.await;
Expand Down Expand Up @@ -322,11 +339,21 @@ impl HttpServer {

/// 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
fn set_req_uri(req: &mut Request<Body>, scheme: Scheme, addr: SocketAddr) -> anyhow::Result<()> {
/// The incoming request's URI is relative to the server, so we need to set the scheme and authority.
/// The `Host` header is used to set the authority. This function will error if no `Host` header is
/// present or if it is not parsable as an `Authority`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on hyperium/hyper#1612 I believe this should also consider the req.uri().authority(), verifying that they are identical if both are present.

fn set_req_uri(req: &mut Request<Body>, scheme: Scheme) -> anyhow::Result<()> {
let uri = req.uri().clone();
let mut parts = uri.into_parts();
let authority = format!("{}:{}", addr.ip(), addr.port()).parse().unwrap();
let headers = req.headers();
let host_header = headers
.get(http::header::HOST)
.context("missing 'Host' header")?
.to_str()
.context("'Host' header is not valid UTF-8")?;
let authority = host_header
.parse()
.context("'Host' header contains an invalid authority")?;
parts.scheme = Some(scheme);
parts.authority = Some(authority);
*req.uri_mut() = Uri::from_parts(parts).unwrap();
Expand Down