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

feat: add support for ALB #1521

Merged
merged 8 commits into from
Nov 28, 2022
Merged

feat: add support for ALB #1521

merged 8 commits into from
Nov 28, 2022

Conversation

ihendriks
Copy link
Contributor

Description

AWS ALB support lambdas, serverless supports them as well. As such, supporting them in serverless-offline seems logical.

Based on:

Motivation and Context

Support for this has been requested by several people.

This would close #598 (which is already closed, but the feature is not supported).

How Has This Been Tested?

It's tested on our own project with success.

I was unable to get the testsuite to run at this time, but wanted to open the PR for feedback regardless.

@ihendriks ihendriks force-pushed the albsupport branch 2 times, most recently from f91a18a to 00575e0 Compare August 2, 2022 12:01
@ihendriks
Copy link
Contributor Author

@dherault I've rebased the PR and resolved the conflict.

Let me know if this needs further changes from my end. I've been using it locally for our usecase and haven't run into any issues so far.

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Aug 11, 2022

@ihendriks thank you for fixing up the PR. at a quick glance, it looks great. it would be great if we could add some tests. even if you have some examples on usage, I could guide you on how to add those.

@ihendriks ihendriks force-pushed the albsupport branch 2 times, most recently from 21ed009 to f33b042 Compare August 15, 2022 17:38
@ihendriks
Copy link
Contributor Author

ihendriks commented Aug 15, 2022

@dnalborczyk I've added an example and I've created some unittests. They're mostly duplicated from integrations/handlers as that made the most sense to me.

I didn't copy over everything, but figured this was a good starting point.

Let me know if there is anything that needs to be done.

Edit: Almost forgot to mention, but I noticed an issue where if you only define ALB endpoints (and no HTTP) in the serverless.yml, then it gets stuck during startup.
I'm not sure if that's an issue in my ALB code, or something else. Perhaps you know?

For now I've added a dummy HTTP endpoint in the unittests for ALB, but it's not the best solution.

@ihendriks
Copy link
Contributor Author

@dnalborczyk I fixed some issues with CORS and with wildcard parameters for the path.

There's a lot of code that is similar or outright duplicate of http/HttpServer.js but I think refactoring is best left to you, since you know better what you want.

I'm certain that there are features missing, but I don't have enough time to work on this. I think any other features can be taken care off at a later stage.

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Aug 25, 2022

thank you very much @ihendriks ! I'll try to make some time to have a deeper look very soon. stay tuned. I just wanna iron out some outstanding kinks in v9.

There's a lot of code that is similar or outright duplicate of http/HttpServer.js but I think refactoring is best left to you, since you know better what you want.

unfortunately there's a lot of duplicate code everywhere 😉 and often times with very few or no tests, so refactoring without breaking stuff isn't easy. it's just how the project grew organically over time. that's why I'm trying to pull new stuff in (ideally) only with tests. same applies for bug fixes. at least those cases won't break [again] in any future refactoring efforts.

@dnalborczyk dnalborczyk changed the title Implemented ALB support feat: add support for ALB Aug 25, 2022
@dnalborczyk dnalborczyk force-pushed the master branch 2 times, most recently from d5ea86d to 22fd667 Compare October 9, 2022 02:13
@dnalborczyk dnalborczyk force-pushed the master branch 2 times, most recently from fdd1699 to 51a30e9 Compare October 15, 2022 19:39
@ihendriks
Copy link
Contributor Author

Hi @dnalborczyk, I was wondering what the status of your refactoring is?

If I were to get this PR up to date with the current state of the repo, do you think this could be merged?

I'm reluctant to work on this while you're refactoring at the same time, but I would like to get this merged sooner rather than later, if that's possible.

Not to pressure you or anything, just curious about what the plan/timeline is here.

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Oct 26, 2022

thank you for your patience @ihendriks !

yes, if you could fix the conflicts that would be great. In the meantime I will wait to change any files related to your PR.

If needed, we can flag the ALB event support as experimental if there's anything to be fleshed out further.

@ihendriks
Copy link
Contributor Author

Alright, I will try to make some time for it this week.

've fixed some issues that we found ourselves, but I can imagine there's a couple of things I've missed still. So Experimental flag might be a good idea until there's a bit more feedback from other users.

@ihendriks
Copy link
Contributor Author

@dnalborczyk Sorry for the delay, was hard to find time for this.

I've resolved all conflicts, fixed a few issues and updated the code to match your refactoring a bit better.

Let me know if there is anything else that needs to be done.

@dnalborczyk
Copy link
Collaborator

thank you for fixing up the PR @ihendriks !!

we should get it merged in the next coming days. I'm thinking to release this as semver major, in case someone had an own/or third party offline plugin for application load balancer support, which in turn would likely break things or cause potentially weird behavior - although one could make that argument for any new features.

@dnalborczyk dnalborczyk merged commit bfcc4de into dherault:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for ALB to Lambda Integration
3 participants