support both apigw and alb events as http::{Request,Response} #64
Conversation
|
I'm not sure I'd worry too much about portability between API Gateway and ALB. The reason is that if I choose API Gateway, I'll likely offload AuthN/AuthZ tasks to the Gateway and my Lambda function will rely on the context object to validate this. Especially if I use AWS_IAM as the auth strategy. On the other hand, with ALB I will only support bearer tokens. My point is, if they have decided to only rely on properties of the request and bearer tokens for auth, then their code is easily portable. If they have chosen API Gateway and rely on its context, they probably expect the migration to take some time and require code changes. Having said that, I think the most important feature to abstract is the auth state. I just went through this in my |
|
Thanks for the feedback @sapessi. I don't want to make this any more opinionated that it needs to be for now. For now we have an abstraction that let's rust developers that know the http crate be immediately familiar and productive but to also not restrict access to some of the decorations that come along with the request context/{querystring/path} parameters ect. In practice I've used two methods for auth with lambda custom authorizer and auth0. I don't want to restrict what folks can and can't do. I think higher level abstractions could come from the http crate ecosystem and "just work" here. I think for Meetup and potentially others the interest is more in the ability to flexibility experiment with one of the other for potential cost savings an integration needs. I'm really excited to see the potential for those with existing albs to have lambdas that can simply "plug into" exciting albs to help more easily break down legacy applications into smaller parts. We've being experimenting with api gateway for that with api gateway previously being the only option but some use cases may benefit more from the alb since we're already paying for that. Ideally we could switch back and forth with minimal code changes to facilitate experimentation. I'm mainly looking for feedback on on the suggestion about the cargo feature to trim off the bits you know you'll likely not need if you're heavily invested in one or the other. I could do that in a followup but I think it'd also be straight forward to do in this pull. I wanted to wait on some feedback before I took a stab at that. |
|
I’m not entirely sure that I agree with you, @sapessi. ALBs and API Gateway have been getting closer to one another over the years (i.e., both support OAuth authentication) and I’ve had two separate conversations inside of Amazon where folks wanted to swap out API Gateway with an ALB (and vice versa). I look forward to the day that we have a single story for L7 load balancing at Amazon. @softprops Personally, I like the idea of a feature flag toggling between an ALB and API Gateway, as it allows—like you said—incremental migration of legacy applications and potential cost savings for higher-traffic applications. |
|
Perhaps I wasn't clear - late night writing... I agree that feature flags are a good idea to make it easy to switch between versions of the crate. For AuthN/AuthZ, it may be a good idea to start thinking about a standard that could be included in the http crate. In Java this is the servlet specs and Jax-RS. If you can create a generic object such as the security context linked above, then you can implement it for both API Gateway and ALB as well as include it in regular http servers. The difference is that http servers will parse the bearer tokens and create it from scratch, with ALB/API GW you can populate it using the context. The only "unusual" case there is when API Gateway is configured with IAM auth, where it might be helpful to include more AWS-specific properties. |
|
awesome 👍 let me know if that's enough to be a blocker for this or could be a follow up. I'd be happy to do either. Im into fast iteration. |
|
Im actually starting to convince myself that conditional compilation may not be worth it for the added maintainability. I'll post another pull shortly to demonstrate for comparison and keep this branch clean |
No, don't think it's a blocker - just food for thought. I find the servlet and jax rs specification to be very good at what they do but even better at giving the community a set of common tools to build around. |
|
example with feature flags #65 ( just for comparison ) |
agreed. that's essentially what the http crate in the rust ecosystem today, a least common denominator for rust ecosystem libraries to build on top of |
|
@davidbarsky / @sapessi can you two take a peek over #65. I wanted to drop up a picture of what a cargo feature flag impl might look like before making more changes in this branch. #65 just builds on this an makes a best effort attempt at feature flagging the two http-like triggers |
| #[serde(default, deserialize_with = "nullable_default")] | ||
| pub(crate) multi_value_query_string_parameters: StrMap, | ||
| #[serde(deserialize_with = "nullable_default")] | ||
| /// alb events do not have path parameters | ||
| #[serde(default, deserialize_with = "nullable_default")] |
There was a problem hiding this comment.
a nit and a thought:
- in doc comments, can we use sentence casing? I'm not sure how to programmatically enforce this...
- Situations like these—“ALB events don't have path parameters.“—make me think compile-time feature flags are the correct option, but I also don't see why API Gateway and ALBs can't converge on supported values. I don't have a good answer for this.
There was a problem hiding this comment.
Will fix. Note: this is a internal crate so its more like doc for maintainers than it is for users
There was a problem hiding this comment.
As for programatic enforcement, there's nothing that can enforce that today but it would be an interesting thought experiment to sent the rustdoc team's way.
There was a problem hiding this comment.
Will fix. Note: this is a internal crate so its more like doc for maintainers than it is for users
Fair point!
| pub authorizer: HashMap<String, Value>, | ||
| pub api_id: String, | ||
| pub identity: Identity, | ||
| pub struct Elb { |
There was a problem hiding this comment.
Small nit, and I know this is what Amazon calls it, maybe we shouldn't propagate that mistake here. Do you think this struct be called something LoadBalancerContext with a #[serde(rename = "elb")] attribute plopped on?
There was a problem hiding this comment.
@davidbarsky Do you mean the type of the field name?
Aside from letter casing rustc warnings, I'm a bit hesitant to rename these provided fields because these are what I users will find in documentation that comes from aws (across runtimes) in examples and other forms of documentation. My empathy here goes out to engineers that work across aws projects (and runtimes). I have the feeling renaming fields would require an extra overhead of how these map back to the source in developer heads.
I'm open to renaming types here but I'm going to try to debate the value of keeping the field names that represent the "shape" of data as close to the source as Rust will allow. Rustc will warn on not rusty letter casings so that I'm open to at least that much wiggle room.
Thoughts? If you mean the type. I'm down with that.
There was a problem hiding this comment.
Aside from letter casing rustc warnings, I'm a bit hesitant to rename these provided fields because these are what I users will find in documentation that comes from aws (across runtimes) in examples and other forms of documentation. My empathy here goes out to engineers that work across aws projects (and runtimes). I have the feeling renaming fields would require an extra overhead of how these map back to the source in developer heads.
That's a good point to be honest. I don't have very strong opinions on that front. I think it's fine as is!
|
@softprops this looks good to me. Do you want to push any additional changes? |
|
Let's go! I'm all for iterations |
Issue #, if available:
Description of changes:
This is a follow up to some commentary from the initial lambda-http pull about supporting alb events. Below are some notes and some potential changes I considered
There's a great series of posts I lifted my knowledge of the differences between the two ( there are a few ) here. I'd give that a quick read before looking at this.
There is one case where knowledge needs transfered about the request in order to serialize the response for alb requests
statusDescription.stage variables and path parameters are features specific to apigateway so these request ext methods will be empty for alb requests ( have some thoughts about this below ).
the request context is uber rich for api gateway but slim for alb. Im supporting both as an enumeration but not sure about this as a design choice ( have some thoughts on this below )
as a user of lambda I want the flexibility to switch between both alb and api gateway with minimal fuss. as a general goal here, if lambdas tag line is "write code without thinking about servers" I want this crate to represent "write (correct) http code without thinking about the inconsistencies between http lambda triggers ". I have some ideas for potential amendments made to choices made in this pull.
One may argue that is awkward to to work with the request_context if you know your application will always be triggered by an alb or always by an api gateway. Similarly it may feel awkward to have stage_variables and path_parameters as available methods in an alb only context. One possible solution is to amend some of these choices with cargo feature flags, One for apigw support and one for alb support. When provided in combination you get the interfaces in this pull. When provided separately you get just the features specific to that type of http event.
@davidbarsky let me know what you think.
By submitting this pull request