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

Update Serverless Framework's AWS integration #3755

Conversation

virtuoushub
Copy link
Collaborator

@virtuoushub virtuoushub commented Nov 19, 2021

Work related to #3284 / #3686

  • target latest node runtime
  • improve CORS defaults
  • remove deprecation warnings
  • clean up packaging (unrelated prisma binaries not included)

see also: #3284 (comment)

@@ -16,6 +16,7 @@ service: app

plugins:
- serverless-dotenv-plugin
- serverless-api-cloudfront
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, use of this plugin might not be what we want. We currently use httpApi events and not http; which this plugin expects. The plugin also does not seem to be updated much, so more of a roll our own approach seems warranted.


Shout out to @ajcwebdev for helping smoke testing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, bummer. I'm trying to get some help from folks at Serverless. Let's keep this top of the list for them to look at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! Serverless Framework team member here 👋

Could you clarify why CloudFront is used? Is it used:

  • as a CDN for static assets/static website hosting (i.e. in front of S3)
  • or as a CDN/cache for HTTP responses returned by Lambda (i.e. in front of API Gateway)

From what I understand it seems to be the second case, but I just want to make sure.

AFAIK here are your option to set up CloudFront with API Gateway in serverless.yml:

  • deploy and configure CloudFront via CloudFormation code in serverless.yml (in the resources section) -> you should expect ~100 lines of YAML (I've done this a lot, this can be painful) -> I can help you with that if you choose to go that way
  • or use a plugin, but AFAIK there are not many options - I've worked on a plugin that may help here: https://github.com/getlift/lift

Lift offers several "constructs" to deploy high-level components. One of them is "server-side website": that construct deploys a CloudFront distribution that serves responses from API Gateway.

You can read more about it here: https://github.com/getlift/lift/blob/master/docs/server-side-website.md

One piece of caution: that construct was built with backend frameworks in mind, like Laravel, Ruby on Rails, Django, Symfony… Not really for server-side rendering of frontend websites (even though one user mentioned that it seemed to work fine too for these use-cases). From what I've read about Redwood, it's the GraphQL API running on Lambda? If that's the case, then it could work well here.

Also that leads me to one question: why use CloudFront in front of the GraphQL API?

By the way we also have one for static websites served with CloudFront + S3: https://github.com/getlift/lift/blob/master/docs/static-website.md (but it may not be what you need here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mnapoli Thanks for jumping in here. Redwood has two considerations during deployment:

  1. API functions
  2. Web assets

We use a Jamstack-like deployment flow, which you can read about in the first two sections of this Doc:

Our goal here is to deploy:

  1. API Functions --> AWS Lambdas
  2. Web assets --> CDN

The latter should be Cloudfront, correct?

Bonus Time: Caching

Lastly, Redwood's GraphQL API Function is "cache-ready" out of the box: either mem or DB (eg. Redis). For serverless deployments, we normally reach for Redis, which might not be the only option on AWS Lambdas.

It's not a hard requirement for this PR, but it would 100% be something that gets people excited to use this Serverless setup. Any immediate reactions/ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's clearer. I see 2 scenarios:

Scenario A (simplest):

  • website.com -> CloudFront -> S3
  • api.website.com (i.e. another domain) -> API Gateway -> Lambda

In that case, Lift's static website might be perfect here. If you don't want to use the Lift plugin, it is also perfectly possible to write the CloudFormation code manually (or use "eject" to generate that code). Note that Lift provides a few helpers as well to quickly deploy asset changes, and clear the CDN cache on deployment.

You get CDN for the static website, but CloudFront would not cover the API. One could use API Gateway's caching feature though (API Gateway has a caching system).

Scenario B:

  • website.com/* -> CloudFront
    • /* -> S3
    • /api/* -> API Gateway -> Lambda

In other words, all requests go through CloudFront. Then CloudFront routes based on the path. CloudFront caching could then apply to web assets and API responses. Watch out: this scenario also involves extra CloudFront costs, because all API calls go through CloudFront.

In that case, you may need to write the CloudFront configuration manually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mnapoli Lift's static-website construct is great, thanks for pointing me in this direction!

If we did decide to go with scenario B, do you know of a similar construct for spinning up that setup, or where I might start looking for CDK snippets/constructs? I'd like to avoid manual CloudFront if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@virtuoushub For scenario B I'm wondering if https://github.com/getlift/lift/blob/master/docs/server-side-website.md could actually be a solution here 🤔

Maybe a config like this would be worth testing:

service: my-app
provider:
    name: aws

functions:
    function1:
        handler: home.handler
        events:
            -   httpApi: 'GET /.redwood/functions/function1/'
    # ...

constructs:
    website:
        type: server-side-website
        assets:
            '/index.html': dir-of-static-website/index.html
            '/css/*': dir-of-static-website/css
            '/js/*': dir-of-static-website/js

plugins:
    - serverless-lift

The downsides:

  • the name of that construct (server-side-website) isn't really the best name here, but it's just a name 🤷
  • with that construct, since /* is directed by default to API URLs, you need to specify explicitly every root file in the constructs.website.assets section (you cannot do '/*': dir-of-static-website/, or at least I don't expect it to work)

@virtuoushub virtuoushub force-pushed the feat/update-serverless-framework-integration branch from 797e824 to b8bd6c2 Compare November 26, 2021 15:25
@@ -24,19 +25,26 @@ custom:

provider:
name: aws
runtime: nodejs12.x
runtime: nodejs14.x
region: us-east-2 # This is the AWS region where the service will be deployed.
httpApi: # HTTP API is used by default. To learn about the available options in API Gateway, see https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-vs-rest.html
cors: true
Copy link
Contributor

@Irev-Dev Irev-Dev Dec 7, 2021

Choose a reason for hiding this comment

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

cors: true is serverless shorthand for wild card origin and a bunch of headers, the biggest problem is the wildcard as that it's not allow by browsers for requests with credentials. Here's my suggestion for what we should add instead.

Suggested change
cors: true
cors:
allowedOrigins:
- '*' # the * wildcard will need to be replaced with your domain to allow requests with credentials.
allowCredentials: true
allowedHeaders:
- authorization
- auth-provider
- content-Type

Though we could also add these common aws headers that could be useful depending on what other aws services a Redwood user might employ.

              - X-Amz-Date
              - X-Api-Key
              - X-Amz-Security-Token
              - X-Amz-User-Agent

See also #3812 (note my suggestion is a little different from the screen shot in that issue, that because we're using httpApi events here, where as I was using restApi in my experiments)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it possible to use something like ${config.web.host}, e.g.:

...
      allowedOrigins:
        - ${config.web.host}
...

so no manual replacing is necissary?

Copy link
Contributor

@Irev-Dev Irev-Dev Dec 10, 2021

Choose a reason for hiding this comment

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

Sorry all this cors talk is coming from the perspective of me trying to deploy the api only with serverless (since frontend/cloudfront isn't working less). and so my points about credentials and CORS are mute if the API is on the same domain.

Otherwise what you suggesting sounds good if such an option exists.

Comment on lines 16 to 18

plugins:
- serverless-dotenv-plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

dot env files are going to be included by default in the next serverless version, but the functionality can be opted into now, plus helps remove a dependency.

Suggested change
plugins:
- serverless-dotenv-plugin
useDotenv: true
plugins:

if this suggestion is accepted it also needs to be removed from the setup command install, see:

// packages/cli/src/commands/setup/deploy/providers/aws-serverless.js:87
export const apiDevPackages = [
  '@netlify/zip-it-and-ship-it',
  'serverless-dotenv-plugin', // should be removed too!
]

Copy link
Contributor

@Irev-Dev Irev-Dev Dec 7, 2021

Choose a reason for hiding this comment

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

Note that in #3782 I added a stage argument to the deploy command. Related to this as when deploying serverless will look for a .env.{stage} file before falling back to the .env file if non exists.

https://www.serverless.com/framework/docs/environment-variables/#support-for-env-files

Copy link
Contributor

Choose a reason for hiding this comment

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

dot env files are going to be included by default in the next serverless version

Hi, quick note: useDotenv: true will work in Serverless Framework v2 and v3 (future next major) in the same way.

That means that you will need to keep useDotenv: true in serverless.yml if you want .env files to be loaded.

Note that the serverless-dotenv-plugin plugin works slightly differently from the built-in feature, so pick the one you prefer based on your requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Irev-Dev In general, we want to stay far away from recommending people including secrets (a la the file .env) in their repo. Not a best practice.

But, yes, the exception for us is the .env.defaults file we have in the template for non-secret vars, e.g. config and setting related.

Just to confirm, how are you thinking people will use this? Also, given this is a local deployment at this time, maybe I'm misunderstanding and this will 100% be required.

@mnapoli do you have suggestions about best practices here? Especially for the case of multiple people working on a project and 1) keeping secrets secure while 2) multiple people having "permission" to deploy?


@Irev-Dev however we handle this, we need to give a strong WARNING about this in the docs and setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, secrets are indeed better stored outside of the filesystem (because it makes sharing them across the team hard & risky, and there's always the risk of committing them/publishing them somewhere).

Common approaches with AWS (and Serverless Framework) are:

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding either of those options into the setup tutorial is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's action should be taken from this conversation @thedavidprice?

I think maybe updating https://redwoodjs.com/docs/deploy#aws-serverless-deploy to include setting up the serverless dashboard with ci/cd and secrets/env-vars? should I make an issue? (and then resolve that issue 😝 )

@ajcwebdev
Copy link
Collaborator

Danny's notes, lots of good stuff we'll want to incorporate into this draft: #3782 (comment)

virtuoushub added a commit to virtuoushub/redwood that referenced this pull request Dec 9, 2021
virtuoushub added a commit to virtuoushub/redwood that referenced this pull request Dec 9, 2021
@virtuoushub virtuoushub force-pushed the feat/update-serverless-framework-integration branch from b62f50c to f0567a3 Compare December 9, 2021 23:00
@virtuoushub virtuoushub force-pushed the feat/update-serverless-framework-integration branch 2 times, most recently from 221a10e to 86ad7aa Compare December 10, 2021 22:18
@thedavidprice thedavidprice changed the base branch from main to serverless-deploy-3686 December 17, 2021 18:05
@thedavidprice
Copy link
Contributor

thedavidprice commented Dec 17, 2021

Update

I’ve changed this PR to merge against a new branch:

Also, moving and consolidating conversation to:

@thedavidprice
Copy link
Contributor

Please see discussion over here:

@dac09
Copy link
Contributor

dac09 commented Dec 21, 2021

@virtuoushub do you think you could please update the title and description to explain the changes here? I can see what they are but when reviewing later on it might be helpful

@virtuoushub virtuoushub changed the title [WIP] update serverless framework integration [WIP] Update Serverless Framework's AWS integration (target latest node runtime, improve CORS defaults, remove deprecation warnings, clean up packaging) Dec 21, 2021
@virtuoushub
Copy link
Collaborator Author

@dac09 / @jtoar re: @thedavidprice's comment, I'll defer to you on what we want to do with this PR.


package:
individually: true
patterns:
Copy link
Contributor

Choose a reason for hiding this comment

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

@virtuoushub could you give me a clue what this is required for? Everything else in this PR looks good.

Copy link
Collaborator Author

@virtuoushub virtuoushub Dec 22, 2021

Choose a reason for hiding this comment

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

@dac09 -

The Serverless configuration file includes a package pattern that excludes all Prisma Engine binaries but the one relevant for the Lambda runtime, so only 1 binary will be included when Serverless packages your app for upload:

they stem from reading through @thedavidprice's comment here: #3284 (comment) and more specifically here as that first one is a rather large info dump: https://gist.github.com/thedavidprice/ac67656b1a3e2b32f31799cd4fb7cbec#gistcomment-3897679

I have not tested this theory, but it is very possible #3782 does most of this cleanup for us already?

see also:

Copy link
Contributor

Choose a reason for hiding this comment

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

I see makes sense. Let me take it for a spin, and will merge in if it deploys all ok!

@dac09 dac09 marked this pull request as ready for review December 22, 2021 15:47
@dac09 dac09 changed the title [WIP] Update Serverless Framework's AWS integration (target latest node runtime, improve CORS defaults, remove deprecation warnings, clean up packaging) Update Serverless Framework's AWS integration Dec 22, 2021
@dac09 dac09 assigned dac09 and unassigned thedavidprice Dec 22, 2021
….com:virtuoushub/redwood into feat/update-serverless-framework-integration

* 'feat/update-serverless-framework-integration' of github.com:virtuoushub/redwood:
  feat: update `cors` definitions for `httpApi` in yaml template
  chore: avoid deprecation warnings
  feat: bump to node 14
  fix: remove the duplicate Prisma binary being deployed
@dac09 dac09 merged commit cc84f2e into redwoodjs:serverless-deploy-3686 Jan 4, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jan 4, 2022
@virtuoushub virtuoushub deleted the feat/update-serverless-framework-integration branch January 4, 2022 16:30
dac09 added a commit to dac09/redwood that referenced this pull request Jan 6, 2022
@jtoar jtoar added release:feature This PR introduces a new feature and removed triage/processing labels Jan 20, 2022
@thedavidprice thedavidprice modified the milestones: next-release, v0.42.0 Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature topic/deployment
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants