Skip to content

feat(lambda-http): accept http_body::Body in responses #491

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 13, 2022
Merged

feat(lambda-http): accept http_body::Body in responses #491

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e68561b
feat!(lambda-http): accept http_body::Body in responses
nmoutschen Apr 14, 2022
418ee4c
fix(lambda-http): fix examples and docs
nmoutschen Apr 19, 2022
44edec4
chore: try adapt makefile to m1 mac
hugobast Jun 15, 2022
b56e759
fix: notify waker to process resp future
hugobast Jun 20, 2022
d58a1bc
refactor: turn struct into enum
hugobast Jun 20, 2022
e9111a7
feat: impl into_response for bytes
hugobast Jun 21, 2022
9e053ef
fix: mispelling
hugobast Jun 22, 2022
7d37ace
Merge remote-tracking branch 'upstream/main'
hugobast Jun 24, 2022
9cd8f3f
feat: respond based on content headers
hugobast Jul 6, 2022
cf049d4
Merge remote-tracking branch 'upstream/main'
hugobast Jul 6, 2022
2632aad
feat: consider content type charset
hugobast Jul 7, 2022
7b00ea7
feat: adjust examples + code review comments
hugobast Jul 7, 2022
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
9 changes: 6 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ INTEG_EXTENSIONS := extension-fn extension-trait logs-trait
# Using musl to run extensions on both AL1 and AL2
INTEG_ARCH := x86_64-unknown-linux-musl

define uppercase
$(shell sed -r 's/(^|-)(\w)/\U\2/g' <<< $(1))
endef

pr-check:
cargo +1.54.0 check --all
cargo +stable fmt --all -- --check
Expand All @@ -15,7 +19,7 @@ pr-check:

integration-tests:
# Build Integration functions
cross build --release --target $(INTEG_ARCH) -p lambda_integration_tests
cargo zigbuild --release --target $(INTEG_ARCH) -p lambda_integration_tests
rm -rf ./build
mkdir -p ./build
${MAKE} ${MAKEOPTS} $(foreach function,${INTEG_FUNCTIONS_BUILD}, build-integration-function-${function})
Expand All @@ -37,7 +41,7 @@ build-integration-function-%:

build-integration-extension-%:
mkdir -p ./build/$*/extensions
cp -v ./target/$(INTEG_ARCH)/release/$* ./build/$*/extensions/$*
cp -v ./target/$(INTEG_ARCH)/release/$* ./build/$*/extensions/$(call uppercase,$*)

invoke-integration-function-%:
aws lambda invoke --function-name $$(aws cloudformation describe-stacks --stack-name $(INTEG_STACK_NAME) \
Expand All @@ -56,4 +60,3 @@ invoke-integration-api-%:
curl -X POST -d '{"command": "hello"}' $(API_URL)/trait/post
curl -X POST -d '{"command": "hello"}' $(API_URL)/al2/post
curl -X POST -d '{"command": "hello"}' $(API_URL)/al2-trait/post

2 changes: 1 addition & 1 deletion examples/http-cors/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ async fn main() -> Result<(), Error> {

async fn func(event: Request) -> Result<Response<Body>, Error> {
Ok(match event.query_string_parameters().first("first_name") {
Some(first_name) => format!("Hello, {}!", first_name).into_response(),
Some(first_name) => format!("Hello, {}!", first_name).into_response().await,
_ => Response::builder()
.status(400)
.body("Empty first name".into())
Expand Down
9 changes: 6 additions & 3 deletions examples/http-shared-resource/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ async fn main() -> Result<(), Error> {
// Define a closure here that makes use of the shared client.
let handler_func_closure = move |event: Request| async move {
Result::<Response<Body>, Error>::Ok(match event.query_string_parameters().first("first_name") {
Some(first_name) => shared_client_ref
.response(event.lambda_context().request_id, first_name)
.into_response(),
Some(first_name) => {
shared_client_ref
.response(event.lambda_context().request_id, first_name)
.into_response()
.await
}
_ => Response::builder()
.status(400)
.body("Empty first name".into())
Expand Down
1 change: 1 addition & 0 deletions lambda-http/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ base64 = "0.13.0"
bytes = "1"
http = "0.2"
http-body = "0.4"
hyper = "0.14"
lambda_runtime = { path = "../lambda-runtime", version = "0.5" }
serde = { version = "^1", features = ["derive"] }
serde_json = "^1"
Expand Down
29 changes: 20 additions & 9 deletions lambda-http/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ extern crate maplit;
pub use http::{self, Response};
use lambda_runtime::LambdaEvent;
pub use lambda_runtime::{self, service_fn, tower, Context, Error, Service};
use request::RequestFuture;
use response::ResponseFuture;

pub mod ext;
pub mod request;
Expand All @@ -91,9 +93,9 @@ pub type Request = http::Request<Body>;
///
/// This is used by the `Adapter` wrapper and is completely internal to the `lambda_http::run` function.
#[doc(hidden)]
pub struct TransformResponse<'a, R, E> {
request_origin: RequestOrigin,
fut: Pin<Box<dyn Future<Output = Result<R, E>> + 'a>>,
pub enum TransformResponse<'a, R, E> {
Request(RequestOrigin, RequestFuture<'a, R, E>),
Response(RequestOrigin, ResponseFuture),
}

impl<'a, R, E> Future for TransformResponse<'a, R, E>
Expand All @@ -103,11 +105,19 @@ where
type Output = Result<LambdaResponse, E>;

fn poll(mut self: Pin<&mut Self>, cx: &mut TaskContext) -> Poll<Self::Output> {
match self.fut.as_mut().poll(cx) {
Poll::Ready(result) => Poll::Ready(
result.map(|resp| LambdaResponse::from_response(&self.request_origin, resp.into_response())),
),
Poll::Pending => Poll::Pending,
match *self {
TransformResponse::Request(ref mut origin, ref mut request) => match request.as_mut().poll(cx) {
Poll::Ready(Ok(resp)) => {
*self = TransformResponse::Response(origin.clone(), resp.into_response());
self.poll(cx)
}
Poll::Ready(Err(err)) => Poll::Ready(Err(err)),
Poll::Pending => Poll::Pending,
},
TransformResponse::Response(ref mut origin, ref mut response) => match response.as_mut().poll(cx) {
Poll::Ready(resp) => Poll::Ready(Ok(LambdaResponse::from_response(origin, resp))),
Poll::Pending => Poll::Pending,
},
}
}
}
Expand Down Expand Up @@ -153,7 +163,8 @@ where
let request_origin = req.payload.request_origin();
let event: Request = req.payload.into();
let fut = Box::pin(self.service.call(event.with_lambda_context(req.context)));
TransformResponse { request_origin, fut }

TransformResponse::Request(request_origin, fut)
}
}

Expand Down
16 changes: 16 additions & 0 deletions lambda-http/src/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ use http::header::HeaderName;
use query_map::QueryMap;
use serde::Deserialize;
use serde_json::error::Error as JsonError;
use std::future::Future;
use std::pin::Pin;
use std::{io::Read, mem};

/// Internal representation of an Lambda http event from
Expand Down Expand Up @@ -45,6 +47,9 @@ impl LambdaRequest {
}
}

/// RequestFuture type
pub type RequestFuture<'a, R, E> = Pin<Box<dyn Future<Output = Result<R, E>> + 'a>>;

/// Represents the origin from which the lambda was requested from.
#[doc(hidden)]
#[derive(Debug)]
Expand All @@ -59,6 +64,17 @@ pub enum RequestOrigin {
WebSocket,
}

impl RequestOrigin {
pub(crate) fn clone(&self) -> RequestOrigin {
match self {
RequestOrigin::ApiGatewayV1 => RequestOrigin::ApiGatewayV1,
RequestOrigin::ApiGatewayV2 => RequestOrigin::ApiGatewayV2,
RequestOrigin::Alb => RequestOrigin::Alb,
RequestOrigin::WebSocket => RequestOrigin::WebSocket,
}
}
}

fn into_api_gateway_v2_request(ag: ApiGatewayV2httpRequest) -> http::Request<Body> {
let http_method = ag.request_context.http.method.clone();
let raw_path = ag.raw_path.unwrap_or_default();
Expand Down
133 changes: 88 additions & 45 deletions lambda-http/src/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,16 @@ use http::{
header::{CONTENT_TYPE, SET_COOKIE},
Response,
};
use http_body::Body as HttpBody;
use hyper::body::to_bytes;
use serde::Serialize;
use std::future::ready;
use std::{
any::{Any, TypeId},
fmt,
future::Future,
pin::Pin,
};

/// Representation of Lambda response
#[doc(hidden)]
Expand All @@ -20,14 +29,11 @@ pub enum LambdaResponse {
Alb(AlbTargetGroupResponse),
}

/// tranformation from http type to internal type
/// Transformation from http type to internal type
impl LambdaResponse {
pub(crate) fn from_response<T>(request_origin: &RequestOrigin, value: Response<T>) -> Self
where
T: Into<Body>,
{
pub(crate) fn from_response(request_origin: &RequestOrigin, value: Response<Body>) -> Self {
let (parts, bod) = value.into_parts();
let (is_base64_encoded, body) = match bod.into() {
let (is_base64_encoded, body) = match bod {
Body::Empty => (false, None),
b @ Body::Text(_) => (false, Some(b)),
b @ Body::Binary(_) => (true, Some(b)),
Expand Down Expand Up @@ -87,71 +93,99 @@ impl LambdaResponse {
}
}

/// A conversion of self into a `Response<Body>` for various types.
///
/// Implementations for `Response<B> where B: Into<Body>`,
/// `B where B: Into<Body>` and `serde_json::Value` are provided
/// by default.
///
/// # Example
///
/// ```rust
/// use lambda_http::{Body, IntoResponse, Response};
/// Trait for generating responses
///
/// assert_eq!(
/// "hello".into_response().body(),
/// Response::new(Body::from("hello")).body()
/// );
/// ```
/// Types that implement this trait can be used as return types for handler functions.
pub trait IntoResponse {
/// Return a translation of `self` into a `Response<Body>`
fn into_response(self) -> Response<Body>;
/// Transform into a Response<Body> Future
fn into_response(self) -> ResponseFuture;
}

impl<B> IntoResponse for Response<B>
where
B: Into<Body>,
B: IntoBody + 'static,
{
fn into_response(self) -> Response<Body> {
fn into_response(self) -> ResponseFuture {
let (parts, body) = self.into_parts();
Response::from_parts(parts, body.into())

let fut = async { Response::from_parts(parts, body.into_body().await) };

Box::pin(fut)
}
}

impl IntoResponse for String {
fn into_response(self) -> Response<Body> {
Response::new(Body::from(self))
fn into_response(self) -> ResponseFuture {
Box::pin(ready(Response::new(Body::from(self))))
}
}

impl IntoResponse for &str {
fn into_response(self) -> Response<Body> {
Response::new(Body::from(self))
fn into_response(self) -> ResponseFuture {
Box::pin(ready(Response::new(Body::from(self))))
}
}

impl IntoResponse for &[u8] {
fn into_response(self) -> ResponseFuture {
Box::pin(ready(Response::new(Body::from(self))))
}
}

impl IntoResponse for Vec<u8> {
fn into_response(self) -> ResponseFuture {
Box::pin(ready(Response::new(Body::from(self))))
}
}

impl IntoResponse for serde_json::Value {
fn into_response(self) -> Response<Body> {
Response::builder()
.header(CONTENT_TYPE, "application/json")
.body(
serde_json::to_string(&self)
.expect("unable to serialize serde_json::Value")
.into(),
)
.expect("unable to build http::Response")
fn into_response(self) -> ResponseFuture {
Box::pin(async move {
Response::builder()
.header(CONTENT_TYPE, "application/json")
.body(
serde_json::to_string(&self)
.expect("unable to serialize serde_json::Value")
.into(),
)
.expect("unable to build http::Response")
})
}
}

pub type ResponseFuture = Pin<Box<dyn Future<Output = Response<Body>>>>;

pub trait IntoBody {
fn into_body(self) -> BodyFuture;
}

impl<B> IntoBody for B
where
B: HttpBody + Unpin + 'static,
B::Error: fmt::Debug,
{
fn into_body(self) -> BodyFuture {
if TypeId::of::<Body>() == self.type_id() {
let any_self = Box::new(self) as Box<dyn Any + 'static>;
// Can safely unwrap here as we do type validation in the 'if' statement
Box::pin(ready(*any_self.downcast::<Body>().unwrap()))
} else {
Box::pin(async move { Body::from(to_bytes(self).await.expect("unable to read bytes from body").to_vec()) })
Copy link
Contributor

Choose a reason for hiding this comment

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

I've experimented with this patch and the more I think about this issue, the more I think that it's impossible for aws-lambda-rust-runtime to provide a response body conversion that will work for all cases.

The issue I have with this line in particular is that it's effectively saying "if the user's tower::Service has a Response whose Body is not of type lambda_http::Body, then convert the response body to_bytes, then to Vec<u8>, and use lambda_http::Body::from to yield a response whose body is of the lambda_http::Body::Binary variant".

This is problematic, because lambda_http::Body::Binary is signalling to API Gateway that the response contains binary data, and API Gateway does all kinds of fun stuff before relaying the reponse to the client depending on several factors, including whether we signalled our payload contains binary data or not. For example, it might base64 encode the body before returning the response.

I deployed some Lambdas and was able to reproduce clearly undersirable behavior when the service returns a response body with Data = Bytes that contains a UTF-8 encoded string. Now, one might argue that the user should then use Data = String, or have their Response type be String directly (which would leverage the impl IntoResponse for String in response.rs, that correctly uses lambda_http::Body::Text). The problem is the user might not know or be in control of what body type they're returning because they are relying on a library. For example, Axum uses axum_core::body::BoxBody as the unified response body type of all the routes, regardless of what data type your handler returns. BoxBody is a type-erased boxed body type that holds Bytes.

Here is an example repository that reproduces the problem: https://github.com/david-perez/lambda-pr-491. It has instructions to deploy a Rust Lambda function fronted by both a HTTP API Gateway and a REST API Gateway using CDK.

The Lambda function uses Axum and contains a route that returns JSON:

#[tokio::main]
async fn main() -> Result<(), Error> {
    let lambda_handler = Router::new().route("/prod/hello", get(hello));

    lambda_http::run(lambda_handler)
        .await
        .expect("something went wrong in the Lambda runtime");

    Ok(())
}

async fn hello() -> Json<Value> {
    Json(json!({ "message": "hello stranger!"}))
}

As explained above, this setup makes aws-lambda-rust-runtime return a lambda_http::Body::Binary body to the internal AWS Lambda service, signalling a binary payload.

When the response gets sent to the client, it seems like the behavior of API Gateway is dfferent depending on whether the API type is HTTP or REST.

$ curl https://<http-api-id>.execute-api.eu-west-1.amazonaws.com/prod/hello                             
{"message":"hello stranger!"}%
$ curl https://<rest-api-id>.execute-api.eu-west-1.amazonaws.com/prod/hello
eyJtZXNzYWdlIjoiaGVsbG8gc3RyYW5nZXIhIn0=%

The HTTP API works as we would expect, but the the REST API has "helpfully" base64 encoded the JSON text. In both cases the response has the Content-Type: application/json header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll continue with the information you've provided, I wasn't able to get to the core of the issue and was focusing too much on the issues I had with coming up with a test setup for the response conversion table, what you provided is straightforward and really helpful. Thank you, David!

Copy link
Contributor Author

@hugobast hugobast Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@david-perez After a few hours of playing with this, the problem as I see it is that axum::body::BoxBody is a downcast (is that the correct term?) of http_body::Body and I don't see how we can get anything more out of it than the bytes it's holding. In other words, is it correct to say that it is essentially "just bytes" at this point and we have no choice but the Body::Binary variant?

That said I had an idea maybe this is what we are looking for

Suggested change
Box::pin(async move { Body::from(to_bytes(self).await.expect("unable to read bytes from body").to_vec()) })
Box::pin(async move {
let bytes = to_bytes(self).await.expect("unable to read bytes from body");
match from_utf8(&bytes) {
Ok(s) => Body::from(s),
Err(_) => Body::from(bytes.to_vec()),
}
})

To summarize the code we are commenting on

  1. lambda_http::Body pass through as they are
  2. Other bytes::Bytes types (of which BoxBody is)
    a. utf8 from bytes -> Body::Text
    b. if not -> Body::Binary

Copy link
Contributor

Choose a reason for hiding this comment

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

@hugobast That's a neat idea. Unfortunately, the fact that the payload is valid UTF-8 does not imply that its contents are text. A priori the user could be sending a PNG that is valid UTF-8 text, for example.

Looking at the Content-Type header of the response is another approach. But again, it's not an infallible solution. The user might not have set the header. It's also easy to map application/json or text/plain to Body::Text, but we can't possibly handle all standard content types, let alone user-defined custom ones.

There are no "one size fits all" solutions unfortunately. All in all, this is why I think it's impossible to land this feature. We can't guess the user's intention. I think any kind of solution will have to ultimately explicitly involve the user communicating his intention to the runtime. Inserting an http::Extensions in the response that the runtime can then read, for example. That will severely limit the user though.

Copy link
Contributor

@calavera calavera Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think any kind of solution will have to ultimately explicitly involve the user communicating his intention to the runtime

I don't think this is necessarily bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bnusunny I think your point needs to be documented in the runtime documentation, but we should not make users depend on specific configurations in another service. binaryMediaTypes are for example not available when you call a function with invoke. People that use that should still be able to use http responses as expected.

Copy link
Contributor

@bnusunny bnusunny Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For the rules, I would suggest two updates:

2.i.a If it is equal to text/*, application/json, application/javascript or application/xml, leave the body as is and set isBase64Encoded to false.

content-type may have suffixes, such as 'application/json; charset=UTF-8'. So it is better to use:

2.i.a If the content-type begins with text/*, application/json, application/javascript or application/xml, leave the body as is and set isBase64Encoded to false.

2.ii If the content-type header is not present, leave the body as is and set isBase64Encoded to false.

This is problematic. If a bad client sent a binary http response without the content-type header, we would treate it as text and Lambda service would throw an error. We should default to binary here. ALB also uses binary as default.

2.ii If the content-type header is not present, base64 encode the body and set isBase64Encoded to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bnusunny I think your point needs to be documented in the runtime documentation, but we should not make users depend on specific configurations in another service. binaryMediaTypes are for example not available when you call a function with invoke. People that use that should still be able to use http responses as expected.

Yes. We definitely need to document this configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

2.ii If the content-type header is not present, leave the body as is and set isBase64Encoded to false.

This is problematic. If a bad client sent a binary http response without the content-type header, we would treate it as text and Lambda service would throw an error.

However, if they send a text HTTP response without the content-type header, the client sends e.g. Accept: text/plain and the user has not configured contentHandling (which they shouldn't need to, since they are returning text payloads) we would treat it as binary and return a base64-encoded response. That is, the client would receive eyJtZXNzYWdlIjoiaGVsbG8gc3RyYW5nZXIhIn0= instead of {"message":"hello stranger!"}.

I think APIs that return text are far more common than those that return binary payloads and as such we should default to text in this case where we don't have any information. I also prefer the Lambda service throwing a loud error than the client being puzzled at why they received base64-encoded text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense. Let's default to text.

By the way. lambda_http crate only supports Lambda Proxy integration with API GW. contentHandling doesn't apply to Lambda Proxy integration. APIGW Developer Guide has the details.

}
}
}

pub type BodyFuture = Pin<Box<dyn Future<Output = Body>>>;

#[cfg(test)]
mod tests {
use super::{Body, IntoResponse, LambdaResponse, RequestOrigin};
use http::{header::CONTENT_TYPE, Response};
use serde_json::{self, json};

#[test]
fn json_into_response() {
let response = json!({ "hello": "lambda"}).into_response();
#[tokio::test]
async fn json_into_response() {
let response = json!({ "hello": "lambda"}).into_response().await;
match response.body() {
Body::Text(json) => assert_eq!(json, r#"{"hello":"lambda"}"#),
_ => panic!("invalid body"),
Expand All @@ -165,15 +199,24 @@ mod tests {
)
}

#[test]
fn text_into_response() {
let response = "text".into_response();
#[tokio::test]
async fn text_into_response() {
let response = "text".into_response().await;
match response.body() {
Body::Text(text) => assert_eq!(text, "text"),
_ => panic!("invalid body"),
}
}

#[tokio::test]
async fn bytes_into_response() {
let response = "text".as_bytes().into_response().await;
match response.body() {
Body::Binary(data) => assert_eq!(data, "text".as_bytes()),
_ => panic!("invalid body"),
}
}

#[test]
fn serialize_multi_value_headers() {
let res = LambdaResponse::from_response(
Expand Down
7 changes: 5 additions & 2 deletions lambda-integration-tests/src/bin/http-fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
use lambda_http::{service_fn, Error, IntoResponse, Request, RequestExt, Response};
use lambda_http::{service_fn, Body, Error, IntoResponse, Request, RequestExt, Response};
use tracing::info;

async fn handler(event: Request) -> Result<impl IntoResponse, Error> {
let _context = event.lambda_context();
info!("[http-fn] Received event {} {}", event.method(), event.uri().path());

Ok(Response::builder().status(200).body("Hello, world!").unwrap())
Ok(Response::builder()
.status(200)
.body(Body::from("Hello, world!"))
.unwrap())
}

#[tokio::main]
Expand Down
Loading