feat: support for server lambda_http::Request

[hugobast]

Motivation and Context

This PR replaces: #1338

Adds support for accepting lambda_http::Request.

Testing

Pokemon Service is updated with a lambda_http run example.

Checklist

• I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
• I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[crisidev]

LGTM. Do you mind to fix the conflict so we can merge.

[david-perez]

Sorry it took me so long to take a look at this, I was on holiday.

I believe the last thing that stands in the way is #1364

#1364 is unrelated to this change. What this change does is to allow smithy-rs to take in lambda_http:Requests as input, by adding a converter from Lambda's body error type into a smithy-rs RequestRejection. This will get returned when we fail to buffer non-streaming requests as bytes. You might want to update the PR description / commit message to better reflect the intent of this patch.

I could use some help devising a test; the way I've tested this is by changing the pokemon service to use lambda_http::run in this fashion and witnessing that the code compiles:

It'd be nice if we could showcase a working Lambda in the repo as an example project. #1338 includes a nice example you can copy over. You'll also need to copy over the stage stripping part from the request URI path. Do you want to tackle this in a separate PR?

I am unsure what to put under the CHANGELOG for this change; please advise.

A simple message along the lines of "Add support for accepting input lambda_http::Requests in the server" will do.

---

Thank you!

[hugobast]

Sorry it took me so long to take a look at this, I was on holiday.

I'm currently taking a week off too, I'll get to this on Monday. Thanks!

[hugobast]

👋 Back dusting this off and see if I can finish the last leg

#1364 is unrelated to this change. What this change does is to allow smithy-rs to take in lambda_http:Requests as input, by adding a converter from Lambda's body error type into a smithy-rs RequestRejection. This will get returned when we fail to buffer non-streaming requests as bytes. You might want to update the PR description / commit message to better reflect the intent of this patch.

I probably had the wrong thing to paste here; I believe I intended to link to this comment: #1338 (comment) which had a comment about a workaround to #1364. Sorry for the confusion.

You'll also need to copy over the stage stripping part from the request URI path. Do you want to tackle this in a separate PR?

I'd like to get this all done in this PR, so I'll update accordingly,

Thanks for the feedback!

[david-perez]

Nice!

Do you want to copy over Eduardo's cargo lambda config (see rust-runtime/aws-smithy-http-server/examples/{Makefile, README.md} in his PR). It would run the new Lambda executable example.

Can you also update the CHANGELOG.next.toml saying that there is now a canonical and easy way to run smithy-rs on Lambda, linking to the example? We have users doing the old request-response mapping + HTTP URI path stage stripping in their applications, and they'll be happy to know they can get rid of them. Call out in the changelog that there are now converters between SdkBody and lambda_http::Body.

[david-perez]

In your Lambdas, how are you handling usage of the Lambda context in your operation handlers? It's useful for X-Ray integration, logging a request ID etc.

Our users currently have to retrieve it out of the lambda_http::Request and insert it into the HTTP extensions with their state. It's not a great UX. I was wondering if you had any ideas to make it better.

[hugobast]

New information

You'll also need to copy over the stage stripping part from the request URI path. Do you want to tackle this in a separate PR?

I had to do some research yesterday since stage stripping wasn't giving me the expected result. It turns out that as implemented in #1338 we were not stripping anything.

let raw_path = request.raw_http_path()
/// raw_path = "" (empty)

The routing worked at a specific version (sha: eduardomourar@e05b302) because of how the server initializes RequestSpec regexes.

Example:

uri_path_regex: /orders/123/shipments$

It's subtle but it lacks the start of line assert ^ which is present in current versions of smithy-rs. That old regex will match a path like this /prod/orders/123/shipments and more relevant for me it will match a versioned path like this /prod/2022-01-01/orders/123/shipments.

The new regexes look like this:

uri_path_regex: ^/orders/123/shipments$

After stage stripping it will still not match the versioned path that I have /2022-01-01/orders/123/shipments.

I'm not sure what to do about stripping /2022-01-01 if any of this sparks a thought for anyone reading please let me know.

Edit

@david-perez thanks for the review, will work through it today. How are other users dealing with having versioning in their path?

Are they adding the version to the path like so?

@http(uri: "/2022-01-01/orders", method: "POST")
operation CreateOrder {
...
}

[rcoh]

we'll need to figure out how to make this work without adding an unconditional lambda dependency to aws-smithy-http

[hugobast]

@david-perez

In your Lambdas, how are you handling usage of the Lambda context in your operation handlers? It's useful for X-Ray integration, logging a request ID etc.

Our users currently have to retrieve it out of the lambda_http::Request and insert it into the HTTP extensions with their state. It's not a great UX. I was wondering if you had any ideas to make it better.

I've asked a team member about this; I don't think we do anything with the Lambda context yet. I wasn't involved with our X-Ray integration, but I know we have one, and it works.

Maybe this is related, for things like extensions on Request, we've made a RequestExt trait like this:

pub trait RequestExt {
    fn account_id(&self) -> Option<&str>;
    fn request_id(&self) -> Option<&str>;
}

And Request<T> implements RequestExt.

[rcoh]

Lgtm from my side thanks!!

[david-perez]

@david-perez thanks for the review, will work through it today. How are other users dealing with having versioning in their path?

Is the version an API Gateway stage? If so, they are stripping it using a similar function than convert_event from this PR, applying it as middleware. The Smithy model does not include the stage in the URI, since it's deployed to multiple stages.

[hugobast]

At what point in the request lifecycle do you get to use the account_id and request_id methods? Do you thread those strings to the operation handler functions? If yes, how?

Oh right, no, we don't have those in our operation handlers; sorry to mislead, they are only available in middlewares (layers/services)

[hugobast]

@david-perez I made a new set of changes (and rebased) to get the two jobs that were failing to pass

• https://github.com/awslabs/smithy-rs/runs/7903857949?check_suite_focus=true
• https://github.com/awslabs/smithy-rs/runs/7903858148?check_suite_focus=true

[david-perez]

@hugobast Thanks for your contribution.

Changes were made in order to avoid doing a body conversion from a lambda_http::Body to a hyper::Body it's unfortunate that we would have to do a body conversion here anyway for ByteStream?

We will try to address this, perhaps via #1125, so that convert_event does not have to make the body type conversion.