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

[lambda] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. #10228

Closed
knihit opened this issue Sep 8, 2020 · 7 comments · Fixed by #10422
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 p2

Comments

@knihit
Copy link

knihit commented Sep 8, 2020

I am using CfnParameter to read various parameters. One of the parameter types is a 'Number' with min value 1 and max value as 100. This value of this parameter is then passed as the lambda environment key value pairs. When I use 'valueAsNumber' method in conjunction with the lambda environment variables, cdk synth does not synthesize this code for lazy resolution through 'Ref'. It rather assigns a value to the lambda environment variable during synth process. 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not.

Reproduction Steps

    const numberOfTopics = new CfnParameter(this, 'NumberOfTopics', {
        type: 'Number',
        default: 10,
        minValue: 1,
        maxValue: 100,
        description: 'The number of topics to be discovered by Topic analysis. The min value is 1 and maximum value is 100'
    });

In another section of the code where I read the CfnParameter values

        lambdaFunctionProps: {
            runtime: Runtime.NODEJS_12_X,
            handler: 'index.handler',
            code: Code.fromAsset(`${__dirname}/../../lambda/wf-submit-topic-model`),
            environment: {
                INGESTION_WINDOW: props.ingestionWindow,
                RAW_BUCKET_FEED: props.rawBucket.bucketName,
                **NUMBER_OF_TOPICS: props.numberofTopics,**
                STACK_NAME: props.stackName
            },
            timeout: Duration.minutes(5),
            memorySize: 512
        },

What did you expect to happen?

    "NUMBER_OF_TOPICS": {
      "Ref": "NumberOfTopics"
    },

What actually happened?

Environment

  • CLI Version: aws-cli/2.0.44 Python/3.8.5 Darwin/19.6.0 source/x86_64
  • Framework Version: 1.62.0
  • Node.js Version: 14.9.0
  • OS: MacOS
  • Language (Version): 4.0.2

This is 🐛 Bug Report

@knihit knihit added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 8, 2020
@SomayaB SomayaB changed the title [aws-cdk-core] [aws-cdk/core] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. Sep 8, 2020
@SomayaB SomayaB changed the title [aws-cdk/core] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. [core] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. Sep 8, 2020
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Sep 8, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 9, 2020

It's because stringification happens in TypeScript without regard to whether it's a Token or not.

Should have been Tokenization.stringify().

@rix0rrr rix0rrr added @aws-cdk/aws-lambda Related to AWS Lambda and removed @aws-cdk/core Related to core CDK functionality labels Sep 9, 2020
@rix0rrr rix0rrr changed the title [core] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. [lambda] 'valueAsString' works well with lazy resolution with lambda environment variables but the type 'Number' does not. Sep 9, 2020
@knihit
Copy link
Author

knihit commented Sep 9, 2020

@rix0rrr, can you please help me understand how should I be reading CfnParameter Number types for lambda functions, is there another way to read them. As a workaround, I am using the 'valueAsString' to get the parameters.

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Sep 9, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 11, 2020

Yes that will do it.

@rix0rrr rix0rrr removed their assignment Sep 11, 2020
@nija-at
Copy link
Contributor

nija-at commented Sep 17, 2020

What language are you using here? typescript or javascript?

The type for the environment property is { [key: string]: string }, however numberOfTopics is of type CfnParameter. So typescript should fail at the compile step if you do the assignment you've claimed as problematic.

If you're on javascript, I believe it's up to you to ensure that you're performing the correct assignments.

As noted, using valueAsString() will get you the string representation of the token

@nija-at nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 17, 2020
@knihit
Copy link
Author

knihit commented Sep 17, 2020

The language for the runtime is typescript.

@nija-at agree that the lambda environment variable is a string. Traditionally if I were to use CFN directly, I can define the parameter as a Number and have it Ref into the Lambda environment variables. This way I get appropriate parameter validation. In CDK if I can still use it as 'Number' and then be able to Ref the variable when assigning to Lambda still works fine. So there are two problems that I see

  1. Having CfnParameter as a Number does not synthesize into the json template. So I cannot do valueAsNumber().toString() when assigning it to lambda environment variables
  2. If I use CfnParameter as a String, it does synthesize correctly and I can use it as valueAsString() but I need to add a regex instead of having to provide a min/ max value. So I lose the capability to perform appropriate validation. The code where I have the problem is here. https://github.com/awslabs/discovering-hot-topics-using-machine-learning/blob/master/source/lib/discovering-hot-topics-stack.ts (refer line number: 63).

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 18, 2020
@nija-at
Copy link
Contributor

nija-at commented Sep 18, 2020

Thanks for the explanation @knihit. I see the problem.

@nija-at
Copy link
Contributor

nija-at commented Sep 18, 2020

You can workaround this using Token.asString(numberOfTopics.value)

@nija-at nija-at added p2 effort/small Small work item – less than a day of effort labels Sep 18, 2020
nija-at pushed a commit that referenced this issue Sep 18, 2020
CloudFormation allows for parameters of type 'Number' to be referenced,
using the 'Ref' keyword, into properties that are of type 'String'.

This will let customers now use CloudFormation maximum and minimum
constraints on the number parameter type, and still use the resulting
value where a string is expected.

fixes #10228
@mergify mergify bot closed this as completed in #10422 Sep 18, 2020
mergify bot pushed a commit that referenced this issue Sep 18, 2020
…0422)

CloudFormation allows for parameters of type 'Number' to be referenced,
using the 'Ref' keyword, into properties that are of type 'String'.

This will let customers now use CloudFormation maximum and minimum
constraints on the number parameter type, and still use the resulting
value in a property of string type.

fixes #10228


----

*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 p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants