sam-goodwin / punchcard

Type-safe AWS infrastructure.
Apache License 2.0
507 stars 20 forks source link

Lambda.Function doesn't seem to be shipping the code #57

Closed pocesar closed 4 years ago

pocesar commented 4 years ago

although this is being triggered by the wrapper in https://github.com/punchcard/punchcard/blob/d0b6fe43d5e9ef5994c96929b28405bf7ca6cc45/packages/punchcard/lib/core/code.ts#L90-L111

error returned by AWS::CloudFront::Distribution step:

The function cannot have environment variables. Function: arn:aws:lambda:us-east-1:xxxx:function:xxxx:1 (Service: AmazonCloudFront; Status Code: 
400; Error Code: InvalidLambdaFunctionAssociation; Request ID: xxxx)

some kind of heuristic analysis on AWS side I guess. the code below is enough to trigger this when adding to a CloudFrontWebDistribution

import {
  CloudFrontResponseEvent,
  CloudFrontResponseResult,
} from 'aws-lambda'

// hide frontend from search engines / bots
const noIndex = new Lambda.Function(stack, 'app-noindex-lambda', {
  handle: async (event: CloudFrontResponseEvent): Promise<CloudFrontResponseResult> => {
    const response = event.Records[0].cf.response
    const headers = response.headers

    headers['x-robots-tag'] = [{
      key: 'X-Robots-Tag',
      value: 'noindex,nocache,nofollow'
    }]

    return response
  }
})

stack.map((s) => {
  /* ... */
  const noIndexVersion = new Version(s, 'no-index-version', {
    lambda: Build.resolve(noIndex.resource)
  })

  new CloudFrontWebDistribution(s, 'frontend', {
    /* ... */
    originConfigs: [
      {
        behaviors: [{
          /* ... */
          lambdaFunctionAssociations: [
            {
              eventType: LambdaEdgeEventType.VIEWER_RESPONSE,
              lambdaFunction: noIndexVersion
            }
          ]
        }],
      },
    ]
  });
  /* ... */
})

EDIT: just noticed that the build step isn't happening for the "handler" property and the code isn't being packaged. the .punchcard has the unbuilt (aka, just the wrapper) app.js file and the app.js doesn't exist, or I'm doing something wrong

image image

sam-goodwin commented 4 years ago

Oh no, sorry about the bugs!

Looks like that CloudFront problem is caused by a restriction for Lambda@Edge functions - that is a case I was not anticipating. PC currently relies on environment variables for bootstrapping lambda functions with things like ARNs. For this case, we'll need to come up with another solution, I'll need to look into that more.

RE: the code packaging issue, I'll need to dig deeper in. Can you provide a working code snippet I can debug for you?

sam-goodwin commented 4 years ago

This seems relevant: https://aws.amazon.com/blogs/networking-and-content-delivery/leveraging-external-data-in-lambdaedge/

sam-goodwin commented 4 years ago

Lambda@Edge functions will have to fetch their configuration data from a remote source. Perhaps we need to write a Custom Resource to collect things like Table ARN at deploy time and store them in a S3 object for the edge function to grab at container boot time.

This could also be a part of a larger solution to passing arbitrarily large data to a function as configuration, where the data doesn't fit in an environment variable.

pocesar commented 4 years ago

yeah, about Lamda@Edge, if you need to do something more advanced than just setting / retrieving values, it might get nasty without an opinionated way to do this. fetching config using json files should be a good bet like the article proposes! fetch, parse then map the resources (although we are doing AWS job here haha)

About the "not packaging", I'll try to do a minimum repro. I was about to debug punchcard with my current CDK stack, but the cloudfront was failing (not an issue with punchcard), and I was wasting ~45 minutes per try because of a bug (aws/aws-cdk#5269).

pocesar commented 4 years ago

@sam-goodwin found the issue, it's trying to compile the TS file without the typescript plugin (I'm using ts-node in cdk.json). in core/app.ts there's a call to core/code.ts, and it's failing inside "compile.run". the funny thing is that the "err" variable is null, which makes it even more confusing. second variable, that is currently omitted is the "Stats" class, and the "compilation.errors" member isn't empty:

[
  ModuleParseError: Module parse failed: Unexpected token (77:23)
  You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
  | const certificateArn = 'arn:aws:acm:us-east-1:xxxx:certificate/xxxx'
  |
  > const viewerCertificate: ViewerCertificate = {
  |   aliases: ['xxxx'],
  |   props: {
      at handleParseError (G:\www\reader\deploy\node_modules\webpack\lib\NormalModule.js:469:19)
      at G:\www\reader\deploy\node_modules\webpack\lib\NormalModule.js:503:5
      at G:\www\reader\deploy\node_modules\webpack\lib\NormalModule.js:358:12
      at G:\www\reader\deploy\node_modules\loader-runner\lib\LoaderRunner.js:373:3
      at iterateNormalLoaders (G:\www\reader\deploy\node_modules\loader-runner\lib\LoaderRunner.js:214:10)
      at Array.<anonymous> (G:\www\reader\deploy\node_modules\loader-runner\lib\LoaderRunner.js:205:4)
      at Storage.finished (G:\www\reader\deploy\node_modules\enhanced-resolve\lib\CachedInputFileSystem.js:55:16)     
      at G:\www\reader\deploy\node_modules\enhanced-resolve\lib\CachedInputFileSystem.js:91:9
      at G:\www\reader\deploy\node_modules\graceful-fs\graceful-fs.js:115:16
      at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
]

so basically, besides checking for "err" there

https://github.com/punchcard/punchcard/blob/3dc3e5ad19bc3734998c6caf330e156d9cd252d7/packages/punchcard/lib/core/code.ts#L113-L120

you'd need to check for the length of the "compilation.errors" array. so, in the end, I need to compile my app.ts to .js before trying to do a cdk synth

EDIT: did a tsc -p . before cdk synth, and it worked as expected. I tried programmatically adding ts-loader as a plugin using app.addPlugin(), but it doesn't work since it's not a plugin...

EDIT2: A good approach would to, instead of adding addExternal and addPlugin methods to the App class, would be to decouple them and append as strategies, that would decide and merge where it would go: PluginStrategy would add them to webpack { plugins: [] }, LoaderStrategy would add them to { module: { rules: [ ] } }, or maybe I'm overthinking, and should provide a POJO to the app class and just merge the configuration as-is, recursively (since most settings are nested arrays in objects)

EDIT3: since PC is a TS only module, wouldn't hurt to add ts-loader in the webpack configuration out-of-the-box and expect the entrypoint to be TS ❤️

sam-goodwin commented 4 years ago

This change also helped this problem: https://github.com/punchcard/punchcard/pull/84

Before: if the webpack compilation failed, then the error was swallowed silently, so synth would succeed but there'd be no code (app.js).

pocesar commented 4 years ago

the initial issue is gone along with #84, so I guess this could be closed 👍 for the lambda@edge issue, I'll open a new one, since I mixed a lot of stuff in here haha