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

[cfn-include] Ref'd Parameters in Conditions are not replaced when parameters are set to "" #10107

Closed
ds17f opened this issue Sep 1, 2020 · 4 comments · Fixed by #10195
Closed
Assignees
Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2

Comments

@ds17f
Copy link

ds17f commented Sep 1, 2020

I have a CloudFormation template which has both Parameters and Conditions. The Conditions Ref the Parameters. When I set the parameters field on the props while instantiating a CfnInclude construct, and then run cdk synth (or deploy or diff), I see an exception if I set the value of the parameter to the empty string.

If the parameter is named SomeParameter, The error message reads:

Element used in Ref expression with logical ID: 'SomeParameter' not found

Reproduction Steps

Bug.json - A cfn template with a parameter and condition

{
    "AWSTemplateFormatVersion": "2010-09-09",
    "Description": "Example of bug with conditions ref'ing parameters",
    "Parameters": {
        "SomeParameter": {
            "Type": "String",
            "Description": "Some Value"
        }
    },
    "Conditions": {
        "SomeParameterIsNotEmpty": {
            "Fn::Not": [
                {
                    "Fn::Equals": [
                        {
                            "Ref": "SomeParameter"
                        },
                        ""
                    ]
                }
            ]
        }
    },
    "Resources": {
        "SSMValue": {
            "Type": "AWS::SSM:Parameter",
            "Properties": {
                "Type": "String",
                "Name": "SomeValue",
                "Value": {
                    "Ref": "SomeParameter"
                }
            }
        }
    }
}

A CfnInclude construct to add to a stack

        // An example of the bug
        const template = new cfn_inc.CfnInclude(this, 'BugConstruct', {
            templateFile: `${__dirname}/Bug.json`,
            parameters: {
                "SomeParameter": ""
            }
        });

What did you expect to happen?

I'd expect to see the value "" replaced wherever a Ref points to SomeParameter. This works as expected with a non-empty string value.

What actually happened?

The cdk synth command failed producing the following error:

Element used in Ref expression with logical ID: 'SomeParameter' not found

Environment

  • CLI Version : 1.61.1
  • Framework Version: 1.61.1
  • Node.js Version: v10.16.3
  • OS : macOS 10.12.6
  • Language (Version): TypeScript (3.8.3)

Other

I did some digging and I'm fairly sure that the issue lies here:
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/core/lib/cfn-parse.ts#L435

if (specialRef) {

and is a result of the value returned from this.specialCaseRefs here:
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/core/lib/cfn-parse.ts#L651-L666

this.specialCaseRefs appears to have 3 classes of return value:

  1. the value of the parameter
  2. an enum related to the AWS::* flag
  3. undefined if it's none of these.

if the value of the parameter is falsey (such as an empty string) then the returned value will be seen as NOT a specialRef and will not be replaced.

I think the fix is simple, but I haven't tested it. Will try to open a PR if possible. The fix should just be:

if (specialRef !== undefined) {

This is 🐛 Bug Report

@ds17f ds17f added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 1, 2020
@github-actions github-actions bot added the @aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package label Sep 1, 2020
@skinny85
Copy link
Contributor

skinny85 commented Sep 1, 2020

Hey @ds17f ,

thanks a lot for opening the issue. Yes, I'm fairly certain the fix is exactly to change

if (specialRef) {

to

if (specialRef !== undefined) {

Would appreciate a PR :) Check out our Contributing docs for some tips on how to do that!

Thanks,
Adam

@skinny85 skinny85 added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 1, 2020
@skinny85 skinny85 added the in-progress This issue is being actively worked on. label Sep 2, 2020
@skinny85
Copy link
Contributor

skinny85 commented Sep 3, 2020

@ds17f how's it going? Do you need something for the PR? I'm here for you 🙂.

@ds17f
Copy link
Author

ds17f commented Sep 4, 2020

@skinny85 Work wasn't in scope for the story. :) As simple as the fix is I don't have an env setup for cdk build work. I made a gitpod but it started to eat up time that I didn't have allotted.

I will get to this ASAP, but honestly I'm guessing that you could probably wrap this before I actually got it done. It's really just the test that's holding me up here too. I know the fix is stupid easy.

@skinny85
Copy link
Contributor

skinny85 commented Sep 4, 2020

OK, I'll take this one then, no worries 🙂

skinny85 added a commit to skinny85/aws-cdk that referenced this issue Sep 4, 2020
We were using a falsy check for the provided parameter values,
which meant passing an empty string or 0 there would fail.
Correctly change the check to explicitly test for 'undefined'.

Fixes aws#10107
@mergify mergify bot closed this as completed in #10195 Sep 8, 2020
mergify bot pushed a commit that referenced this issue Sep 8, 2020
We were using a falsy check for the provided parameter values,
which meant passing an empty string or 0 there would fail.
Correctly change the check to explicitly test for 'undefined'.

Fixes #10107

----

*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/cloudformation-include Issues related to the "CFN include v.20" package bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants