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

fea: add aws-lambda-alb support #2914

Draft
wants to merge 4 commits into
base: v2
Choose a base branch
from
Draft

fea: add aws-lambda-alb support #2914

wants to merge 4 commits into from

Conversation

marcogrcr
Copy link

πŸ”— Linked issue

#2072

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

WHAT?

Add support for ALB lambda function targets.

WHY?

ALBEvent is similar to APIGatewayProxyEvent. However, it has key differences such as:

  • Query string parameters are not URL-decoded automatically.
  • The events either have headers/queryStringParameters or multiValueHeaders/multiValueQueryStringParameters, but
    not both.
  • Headers without values are removed from the event's input.

Without this change, using nitro with Lambda + ALB yields unexpected behaviors like double URL encoding.

RESOLVES?

#2072

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

WHAT?

Add additional tests to `aws-lambda` runtime.

WHY?

Improve the thoroughness and accuracy of the tests.

HOW?

`pnpm build && pnpm test`
WHAT?

Add support for ALB lambda function targets.

WHY?

`ALBEvent` is similar to `APIGatewayProxyEvent`. However, it has key differences such as:

- Query string parameters are not URL-decoded automatically.
- The events either have `headers`/`queryStringParameters` or `multiValueHeaders`/`multiValueQueryStringParameters`, but
  not both.
- Headers without values are removed from the event's input.

Without this change, using `nitro` with Lambda + ALB yields unexpected behaviors like double URL encoding.

HOW?

`pnpm build && pnpm test`
WHAT?

Update `aws-lambda` preset's documentation to clearly indicate which
event types are supported.

WHY?

Avoid confusion from users when trying to use an unsupported event type.
@marcogrcr marcogrcr requested a review from pi0 as a code owner December 2, 2024 17:13
Copy link
Author

@marcogrcr marcogrcr Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Tests are based on the behaviors observed here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When work finished, would be nice if we can also setup for nitro-deploys to make sure preset works also in actual environment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When work finished, would be nice if we can also setup for nitro-deploys to make sure preset works also in actual environment.

I'm not sure how to action this since there doesn't seem to be any workflow in place that deploys to AWS (AWS account, credentials and infra would would be a pre-req). Also, it seems all deployments except alwaysdata.yml are currently disabled.

@pi0
Copy link
Member

pi0 commented Dec 3, 2024

Thanks for PR.

Considering several differences, i think it would be nicer if we develop a standalone subpreset + entry such as aws-lambda-alb and avoid changes to current aws-lambda (that as linked issue states is purpose built for gateway proxy)

@pi0 pi0 changed the title feat(aws-lambda-preset): add alb support fea: add aws-lambda-alb support Dec 3, 2024
@pi0 pi0 marked this pull request as draft December 3, 2024 11:20
@marcogrcr
Copy link
Author

Thanks for the reply @pi0!

Considering several differences, i think it would be nicer if we develop a standalone subpreset + entry such as aws-lambda-alb

Just to confirm, do you mean something like the following?

Update:

export interface AwsLambdaOptions {
streaming?: boolean;
}

to:

export type AwsLambdaOptions =
  | {
    // backwards compatibility, the presence of `type` overrides this
    streaming?: boolean;
  }
  | {
    /**
     * The Lambda function type:
     *
     * - `alb`: An ALB Lambda target group.
     * - `api-gateway`: An API Gateway REST API Lambda proxy integration, HTTP API Lambda integration, or Lambda function URL.
     * - `function-url`: A synonym of `api-gateway`.
     * - `function-url-stream`: A Lambda function URL with response streaming.
     *
     * @default "api-gateway"
     */
    type?: "api-gateway" | "alb" | "function-url" | "function-url-stream";
  };

And update:

hooks: {
"rollup:before": (nitro, rollupConfig) => {
if (nitro.options.awsLambda.streaming) {
(rollupConfig.input as string) += "-streaming";
}
},
},

to:

hooks: {
  "rollup:before": (nitro, rollupConfig) => {
    if ("type" in nitro.options.awsLambda) {
      if (nitro.options.awsLambda.type === "alb") {
        (rollupConfig.input as string) += "-alb";
      } else if (nitro.options.awsLambda.type === "function-url-stream") {
        (rollupConfig.input as string) += "-streaming";
      }
    } else if (nitro.options.awsLambda.streaming) {
      (rollupConfig.input as string) += "-streaming";
    }
  },
},

@pi0
Copy link
Member

pi0 commented Dec 4, 2024

I mean making new variant / export in presets (similar to cloudflare/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants