Gin/mux support for aws alb

hiteshshahjee commented 5 years ago

Gin/mux doesnt work with aws alb. With reinvent 2018, lambda can we invoked from aws alb. Do you have plan to fix/support this?

sapessi commented 5 years ago

Hi @hiteshshahjee - we definitely plan to add support for ALB. Stay tuned.

hiteshshahjee commented 5 years ago

@sapessi Thanks, will wait for the update

opalmer commented 5 years ago

Hey @sapessi, I came across issue this while trying to figure out how to integrate ALB requests with mux on one of my own projects. I didn't see an open PR or branch anywhere so I threw this together:

https://github.com/awslabs/aws-lambda-go-api-proxy/compare/master...opalmer:alb-support

I'm not sure if this is the right approach or not but it seems to work conceptually. I'm also not sure when or if I'll be able to finish this into a full PR but if you end up having the time take work on it I'd be happy to open one so you can iterate on it.

sapessi commented 5 years ago

Thanks @opalmer. I've had this in my to-do list of a while. The proxy events for API Gateway and ALB are fundamentally the same - the two services are unlikely to diverge in the event format going forward. The only place where they are different is the Context. I think the request generation can remain basically the same. The question is how do we inform the framework whether it's integrating with ALB or API Gateway. I see two options:

Add it as a parameter to the New method of the adapter
Deduce it automatically based on the values from the request context

While I think the first option is nice and clean, it is a breaking change to the APIs and would require a code deployment whenever the integration changes. Option two requires a bit more work on the framework side but makes switching between the two seamless as far as the Lambda function is concerned. Looking at the diff, I think we could keep the ProxyEventToHTTPRequest function the same, perhaps rename the type from APIGatewayProxyRequest to ProxyEvent.

The framework could automatically add a special header Lambda-Event-Source to tell the application whether the request came from API GW or ALB. We could create an additional method to extract the ALB context in the RequestAccessor.

Thoughts?

opalmer commented 5 years ago

The second option would probably be the direction I would take and was the way I was headed in my branch too.

As for the first approach, I initially started with making a second set of functions specific to ALB events. That turned out to require quite a bit of work for someone to take advantage of so I tried using interfaces instead so you could implement all the handling for specific types outside of the framework if you wished: https://gist.github.com/opalmer/044b076b3f0e80b8e0da2b43de98bc5a

The main problem is this was also a breaking change even though it was cleaner and allow someone to implement more customized handling. Given the only tag in this repository was 22 days ago there's a decent chance releasing a new version with an API change could break quite a few builds out there assuming consumers of this library were not vendoring. The other way to look at it I guess is deducing how to handle the event automatically means future changes to the event structure, or the addition of new ones, shouldn't require more breaking changes potentially.

So basically, I think I agree the second approach is probably the right one even if it may not be the cleanest. Make sense?

carlosbaraza commented 5 years ago

Hello @sapessi @opalmer, did you guys make any progress on adding ALB support?

opalmer commented 5 years ago

I have not unfortunately, sorry.

sapessi commented 5 years ago

I'll have some time to get back to this next week. Apologies for the delay @carlosbaraza, @opalmer

NicBuihner commented 5 years ago

I spent a bit of time going through the code base trying to come up with some way to solve the event agnosticism issue as I have a project where I want to use an existing Go code base via an ALB Target Group. I hammered together the following proof of concept and have simple working examples with both API Gateway and ALB Target Group events.

https://github.com/NicBuihner/aws-lambda-adapter

The API is breaking, but I think there are a couple of aspects that are advantageous from a maintainability standpoint:

Use a superset JSON tagged struct as the event entrypoint to get rid of the event type coupling inside the code base. If you need more, the RequestContext could be type switched inside the code base.
Consume the http.Handler from the various frameworks eliminating the need to write, test, and maintain a shim for each framework.
Use the httptest.Recorder to ensure a synchronous response from http.ServeHTTP. The deprecation warning on http.CloseNotifier made me nervous. I figure that the core maintainers will make sure that httptest.Recorder keeps working rather than rolling my own implementation.

This is just a proof of concept so far for solving the problem, so no testing or anything yet. I want to gauge whether this might be a worth while approach or not.

kyleyost19 commented 4 years ago

@NicBuihner great POC. I personally feel that writing a small pkg for each invocation type of is the most sane way to go. As for your second two bullet points, i totally agree with them. I made a PR here to get rid of all the dependencies on other packages ( #65 ), but I realize this is a stretch.

Not sure if you have seen this pkg for ALB invocations, but I think it is written similar to yours (using httptest.ResponseRecorder and using http.Handler instead of adding dependencies on other routers

NicBuihner commented 4 years ago

@kyleyost19 thank you for the feedback! I appreciate it. And for your package suggestion, that looks like exactly what I need!

Ahh I think I see the pkg per invocation reasoning. I think, at the time anyway, I was thinking that the underlying JSON for each event type was so similar that it would be possible for future event types to actually be identical, differing only because the API code generator created a new type name. This would open up the possibility of being future compatible without having to add a new event type consumer just because the type name was different.

A pkg per invocation is certainly easier to read and comprehend, no space for confusion between the event type name and the JSON schema if you just handle each invocation type directly. I certainly agree that's a much better trade off than gambling comprehension for a chance at not having to write more code XD

marcelomd commented 2 years ago

Hi! Any new developments here? Thanks!

KSDhillon commented 2 years ago

^ bumping the previous comment, would love this functionality to be added.

OranShuster commented 1 year ago

ALB functionallity has been added for echo,gin,gorilla,httpadapter and handlerfunc in this commit https://github.com/awslabs/aws-lambda-go-api-proxy/commit/10d7bab6671382da9443e8aa59cf0eb420526d3e

awslabs / aws-lambda-go-api-proxy

Gin/mux support for aws alb #22