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

(aws-lambda): specifying insightsVersion generates invalid CloudFormation #18789

Closed
wolverian opened this issue Feb 2, 2022 · 4 comments · Fixed by #18922
Closed

(aws-lambda): specifying insightsVersion generates invalid CloudFormation #18789

wolverian opened this issue Feb 2, 2022 · 4 comments · Fixed by #18922
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@wolverian
Copy link

wolverian commented Feb 2, 2022

What is the problem?

If I specify insightsVersion as so:

const l = new lambda.Function(this, 'CdkTsLambda', {
  code: lambda.AssetCode.fromAsset('../out/logger'),
  handler: 'bootstrap',
  runtime: lambda.Runtime.PROVIDED_AL2,
  insightsVersion: lambda.LambdaInsightsVersion.VERSION_1_0_119_0,
});

I get this error:

 ❌  CdkTsStack failed: Error [ValidationError]: Template format error: Mappings attribute name '1_0_119_0_x86_64' must contain only alphanumeric characters.

Reproduction Steps

https://github.com/wolverian/cdk-lambda-insights-issue/blob/main/lib/cdk-lambda-insights-issue-stack.ts#L18

This repo should reproduce the issue.

What did you expect to happen?

The generated CloudFormation is valid.

What actually happened?

The generated CloudFormation uses invalid keys in the Mapping section:

    "CloudwatchlambdainsightsversionMap": {
      "af-south-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:af-south-1:012438385374:layer:LambdaInsightsExtension:9"
      },
      "ap-east-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-east-1:519774774795:layer:LambdaInsightsExtension:9"
      },
      "ap-northeast-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-northeast-1:580247275435:layer:LambdaInsightsExtension:23"
      },
      "ap-northeast-2": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-northeast-2:580247275435:layer:LambdaInsightsExtension:16"
      },
      "ap-south-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-south-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "ap-southeast-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-southeast-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "ap-southeast-2": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ap-southeast-2:580247275435:layer:LambdaInsightsExtension:16"
      },
      "ca-central-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:ca-central-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "cn-north-1": {
        "1_0_119_0_x86_64": "arn:aws-cn:lambda:cn-north-1:488211338238:layer:LambdaInsightsExtension:9"
      },
      "cn-northwest-1": {
        "1_0_119_0_x86_64": "arn:aws-cn:lambda:cn-northwest-1:488211338238:layer:LambdaInsightsExtension:9"
      },
      "eu-central-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-central-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "eu-north-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-north-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "eu-south-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-south-1:339249233099:layer:LambdaInsightsExtension:9"
      },
      "eu-west-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-west-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "eu-west-2": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-west-2:580247275435:layer:LambdaInsightsExtension:16"
      },
      "eu-west-3": {
        "1_0_119_0_x86_64": "arn:aws:lambda:eu-west-3:580247275435:layer:LambdaInsightsExtension:16"
      },
      "me-south-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:me-south-1:285320876703:layer:LambdaInsightsExtension:9"
      },
      "sa-east-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:sa-east-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "us-east-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:us-east-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "us-east-2": {
        "1_0_119_0_x86_64": "arn:aws:lambda:us-east-2:580247275435:layer:LambdaInsightsExtension:16"
      },
      "us-west-1": {
        "1_0_119_0_x86_64": "arn:aws:lambda:us-west-1:580247275435:layer:LambdaInsightsExtension:16"
      },
      "us-west-2": {
        "1_0_119_0_x86_64": "arn:aws:lambda:us-west-2:580247275435:layer:LambdaInsightsExtension:16"
      }
    }

CDK CLI Version

2.10.0 (build e5b301f)

Framework Version

No response

Node.js Version

v16.13.2

OS

macOS Monterey 12.2 (21D49)

Language

Typescript, Go

Language Version

TypeScript 3.9.7 | go version go1.18beta1 darwin/arm64

Other information

No response

@wolverian wolverian added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 2, 2022
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Feb 2, 2022
@wolverian
Copy link
Author

I tested with both the TypeScript and Go CDK SDKs.

@ryparker ryparker added the p1 label Feb 2, 2022
@DmitryVarennikov
Copy link

I receive the same error, my lambda insights cdk code: insightsVersion: LambdaInsightsVersion.VERSION_1_0_98_0,

@kaizencc kaizencc added effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 9, 2022
@kaizencc
Copy link
Contributor

kaizencc commented Feb 9, 2022

Thanks for the report, @wolverian. This looks like a bug. Looks like the mapping was altered in this commit: https://github.com/aws/aws-cdk/pull/17984/files#diff-7d58dae78817493ec1119dc627f2ca5412971a0a6baea5a7c90fe69912e29fd6.

Since this is a deploy time error, I'd wager that this was an oversight, and has never worked since.

@rix0rrr what shall we do here? I think at a minimum we need to update this:

const factKey = factParam.replace(/[^a-zA-Z0-9]/g, '_');

since _ is not alphanumeric, but that's going to turn our mapping keys from 1_119_0_x86_64 to 11190x8664, which is ugly.

We also do some stuff that is specific to insights version here:

const suffix = version.split('.').join('_') + `_${arch ?? 'x86_64'}`;

All these replacements with _ seem like they are problematic.

@mergify mergify bot closed this as completed in #18922 Feb 11, 2022
mergify bot pushed a commit that referenced this issue Feb 11, 2022
In #17984, mappings were altered so that non-alphanumeric values were replaced with `_`. However, the names in the name-value pairs must be fully alphanumeric according to the [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/mappings-section-structure.html). The result was potentially invalid mappings generated at `cdk synth` that would fail at `cdk deploy`. One such example is the mappings generated for `lambda-insights` in #18789.

In this PR, the replacement value is updated from `_` to `x`. The mapping is not surfaced anywhere other than the template, so we just need a value that satisfies cloudformation. Thus we're okay with the slight loss of readability. In addition, `CfnMapping` is updated to validate the names in the name-value pair and ensure that it is alphanumeric.

Fixes #18789.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
In aws#17984, mappings were altered so that non-alphanumeric values were replaced with `_`. However, the names in the name-value pairs must be fully alphanumeric according to the [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/mappings-section-structure.html). The result was potentially invalid mappings generated at `cdk synth` that would fail at `cdk deploy`. One such example is the mappings generated for `lambda-insights` in aws#18789.

In this PR, the replacement value is updated from `_` to `x`. The mapping is not surfaced anywhere other than the template, so we just need a value that satisfies cloudformation. Thus we're okay with the slight loss of readability. In addition, `CfnMapping` is updated to validate the names in the name-value pair and ensure that it is alphanumeric.

Fixes aws#18789.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants