CodeGenieApp / serverless-express

Run Express and other Node.js frameworks on AWS Serverless technologies such as Lambda, API Gateway, Lambda@Edge, and more.
https://codegenie.codes
Apache License 2.0
5.18k stars 672 forks source link

sourceIP missing with ALB? #331

Closed alsonkemp closed 3 years ago

alsonkemp commented 3 years ago

We've got a fairly generic ALB -> Lambda config and are seeing the following issue. sourceIp is not defined on event.requestContext.identity (or rather identity is not defined).

{
    "errorType": "Runtime.UnhandledPromiseRejection",
    "errorMessage": "TypeError: Cannot read property 'sourceIp' of undefined",
    "reason": {
        "errorType": "TypeError",
        "errorMessage": "Cannot read property 'sourceIp' of undefined",
        "stack": [
            "TypeError: Cannot read property 'sourceIp' of undefined",
            "    at getRequestValuesFromEvent (/var/task/api/_dist/node_modules/@vendia/serverless-express/src/event-sources/utils.js:52:50)",

Usage is as follows (app is in a Promise so we need to await it and then proxy it?):

module.exports.handler = async (event, context, callback): Promise<any> => {
    console.debug('index.handler: running');
    console.debug(` event: ${util.inspect(event)}`);
    console.debug(` context: ${util.inspect(context)}`);

    // Start a server (if one doesn't exist), then memoize it.
    if (!_server) {
        console.debug('index.handler: creating AWSServerlessExpress');
        const app = await __setup();
        _server = configure({ app, binaryMimeTypes });
        dog_log('api', 'boot', 'complete');
    }

    return _server.proxy({ event, context, callback });
};
brettstack commented 3 years ago

Hi @alsonkemp, are you running on V3 or V4 (just published an RC for that to the @vendia/serverless-express npm package). If it's not working on V4 I'll take a look (or you can send a PR if you discover a problem). We don't plan on publishing anything but bug fixes to V3 (which doesn't natively support ALB though people have hacked around it)

alsonkemp commented 3 years ago

4.0.0-rc2

"dependencies": {
    "@vendia/serverless-express": "4.0.0-rc.2",

Hi Alison, are you running on V3 or V4 (just published an RC for that to the @vendia/serverless-express npm package). If it's not working on V4 I'll take a look (or you can send a PR if you discover a problem). We don't plan on publishing anything but bug fixes to V3 (which doesn't natively support ALB though people have hacked around it)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

brettstack commented 3 years ago

Cool. I'll check this out as soon as I can and get back to you

alsonkemp commented 3 years ago

Also (as you probably know), annoyingly the health checker doesn't send in much info to the Lambda so it seems SE fails 100% of health checks.

brettstack commented 3 years ago

Install RC 4 or later. Please let me know any other issues you encounter with ALB as I haven't done thorough testing of this event source.

I'm also curious why you decided to go with ALB over the other event sources

alsonkemp commented 3 years ago

I'll check RC4 tomorrow.

Other event source? e.g. APIG? APIG is seriously complex for our use case and had some weird behavior with deployments via Terraform. ALB seemed simper (though it's proving not to be so) since we do all routing and documentation via OpenAPI....

brettstack commented 3 years ago

APIGW HTTP API is a simpler alternative to APIGW REST API. V4 has experimental support for that also, as well as Lambda@Edge if you need Edge computing (and your app can fit within the limits of Edge)

alsonkemp commented 3 years ago

Indeed V2 looks much simpler. Still, it didn't work (some routes, it did; some routes, it didn't). Nearly impossible to debug. I think I'm going to write a quick, custom ALB-Lambda integration...

brettstack commented 3 years ago

What are you having trouble with for debugging? You should be able to run with an offline tool such as Serverless or SAM to work through any initial issues.

alsonkemp commented 3 years ago

I can't figure out why I'm getting a LambdaUnhandled error. More details are at https://serverfault.com/questions/1049284/aws-alb-lambda-502. Thank you for the quick responses.

brettstack commented 3 years ago

Were you able to figure this out?

alsonkemp commented 3 years ago

@brettstack Thanks for checking in. Yes, I just wrote a straightforward adapter from ALB<->Lambda. Ripped off a bunch of your code... ;) I'll look into open sourcing it and will drop a link to the code in here. Was kind of a pain in the ass to debug but it works well.

brettstack commented 3 years ago

I'd love to see it and get it fixed in this package.j

alsonkemp commented 3 years ago

@brettstack Gist here: https://gist.github.com/alsonkemp/cd133900bbc664095ee852cd20a5e9f5

AWS ALB has an undocumented bug (or at least I couldn't find docs) where it rejects certain header values in responses. Through an unimaginable amount of painful debugging, I found this out... See:

// THE ALB SEEMS TO REJECT REQUESTS WITH CERTAIN HEADERS SO ONLY SEND WHITE LISTED HEADERS
const VALID_HEADERS = [ ... ];
alsonkemp commented 3 years ago

@brettstack To be honest, serverless-express was a bit too abstract/overfactored for my tastes. While I appreciated abstraction, this was a little too much for me and I felt like I was chasing execution path back/forth through files:

const getResponseToAlb = ({
  statusCode,
  body,
  headers,
  isBase64Encoded
}) => getResponseToService({
  statusCode,
  body,
  headers,
  isBase64Encoded
})

Also, we're mostly Typescript so the missing types were ... missing.

brettstack commented 3 years ago

Thanks! I should be able to take that gist and apply some of that header whitelisting logic.

brettstack commented 3 years ago

@alsonkemp what headers were causing you issue? I'd rather go with a deny list over an allow list.

alsonkemp commented 3 years ago

Deny: agreed... If I knew what to deny. I didn't feel like binary searching with 5 minute latencies...

I think it was a partial download header which was sent by express-static. 'accept-ranges: bytes' but I couldn't find any documentation and AWS Support was unenthusiastic.

I went with a white list not only because I didn't want to fiddle with identifying the bad header but also because I didn't want to accidentally discover new ones and I didn't mind controlling which headers are sent (it's a pretty standard, static list).

I may have mentioned this already but besides copying your code, I did next to nothing to verify query string functionality.

On Thu, Jan 28, 2021, 3:58 AM Brett Andrews notifications@github.com wrote:

@alsonkemp https://github.com/alsonkemp what headers were causing you issue? I'd rather go with a deny list over an allow list.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vendia/serverless-express/issues/331#issuecomment-769005099, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIPBO4XB6EY6ZLYVG3GT3S4FGMTANCNFSM4V247DQA .