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.Function.addEnvironment appears to only accept string type values #3337

Closed
2 of 5 tasks
stevegoossens opened this issue Jul 18, 2019 · 4 comments · Fixed by #3858
Closed
2 of 5 tasks

lambda.Function.addEnvironment appears to only accept string type values #3337

stevegoossens opened this issue Jul 18, 2019 · 4 comments · Fixed by #3858
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug.

Comments

@stevegoossens
Copy link

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • 🪲 bug report
    • 🚀 feature request
    • 📚 construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    If the current behavior is a 🪲bug🪲: Please provide the steps to reproduce

// fn is a lambda.Function
const tableTtl = 300;
fn.addEnvironment('TABLE_TTL', tableTtl)

attempting a cdk synth results in an error:

      throw new CfnSynthesisError(message);
            ^
Error: Resolution error: Supplied properties not correct for "CfnFunctionProps"
  environment: supplied properties not correct for "EnvironmentProperty"
    variables: element 'TABLE_TTL': 300 should be a string.
  • What is the expected behavior (or behavior of feature suggested)?
    TABLE_TTL should be added as a Lambda environment variable as "300" (presumably Lambda env vars are only ever string type values)

  • What is the motivation / use case for changing the behavior or adding this feature?
    I had to do the following as a workaround:

fn.addEnvironment('TABLE_TTL', tableTtl.toString())

the "hacky" string conversion is a bit (code-)smelly

  • Please tell us about your environment:

    • CDK CLI Version: 1.0.0 (build d89592e)
    • Module Version: 1.0.0
    • OS: [ OSX High Sierra ]
    • Language: [ TypeScript ]
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

I think .addEnvironment should handle the string conversion, as the value arg/param is documented as any type. I think a number to string conversion should be an easy quick fix, without having to necessarily cater for more difficult type to string conversions.

if (typeof value !== 'string') value = value.toString()

maybe that would be "good enough" to handle "any" (sane) values passed?

@stevegoossens stevegoossens added the needs-triage This issue or PR still needs to be triaged. label Jul 18, 2019
@nmussy
Copy link
Contributor

nmussy commented Jul 18, 2019

Your issue is mentioned in #744

It's not really a construct library gap, but a fairly common CloudFormation quirk. It gets even more frustrating with ECS, in which you have the Container Cpu number property, and the Task Cpu string property. You end up with the following:

const cpuReservation = 256;
const memoryReservation = 256;

const taskDefinition = new TaskDefinition(this, 'task', {
    memoryMiB: memoryReservation.toString(),
    cpu: cpuReservation.toString(),
    compatibility: Compatibility.EC2,
});

const container = taskDefinition.addContainer('container', {
    logging: LogDriver.awsLogs({streamPrefix: 'ecs'}),
    image: ContainerImage.fromEcrRepository(ecr, 'latest'),
    cpu: cpuReservation,
    memoryLimitMiB: memoryReservation,
});

@stevegoossens
Copy link
Author

stevegoossens commented Jul 18, 2019

My issue is that the AWS CDK documentation doesn't mention that the value type needs to be a string.

In https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ecs.TaskDefinition.html, the cpu and memoryMIB properties/params are defined as string type.

Whereas, https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html#add-environmentkey-value, has the value as any type. Why bother documenting it in the AWS CDK documentation (i.e. not CloudFormation) that this value can be anything other than a string, when even a number fails?

Even easier suggested fix

If the AWS CDK documentation above was updated to value: string instead of value: any, that would be fine.

@nmussy
Copy link
Contributor

nmussy commented Jul 18, 2019

I see, my bad. Your issue is that the environment property is defined as a map of any, while the CloudFormation Function Environment reference explicitly types it as Map of String:

readonly environment?: { [key: string]: any };

@stevegoossens
Copy link
Author

Yes, either the AWS CDK documentation (for the type of value) is wrong, or there is a missing type conversion (to string) in .addEnvironment

Going by the CDK docs, I wrongly assumed it would take care of any type conversions necessary.

@RomainMuller RomainMuller added bug This issue is a bug. @aws-cdk/aws-lambda Related to AWS Lambda and removed needs-triage This issue or PR still needs to be triaged. labels Aug 26, 2019
eladb pushed a commit that referenced this issue Aug 29, 2019
AWS Lambda requires that all environment variables will be
strings but the API indicated `any` as the type of the value.
If users would pass in a non-string value, they would see an
error only during deployment.

Fixes #3337
@mergify mergify bot closed this as completed in #3858 Aug 29, 2019
mergify bot pushed a commit that referenced this issue Aug 29, 2019
* fix(lambda): environment var values are strings

AWS Lambda requires that all environment variables will be
strings but the API indicated `any` as the type of the value.
If users would pass in a non-string value, they would see an
error only during deployment.

Fixes #3337

* remove redundant null-check on this.environment

* add breaking change excludes
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants