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

core: CfnParser can't correctly parse conditional Tags #27594

Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p1

Comments

@jaredlundell
Copy link

Describe the bug

The CfnParser class (used by the CfnInclude construct) cannot correctly parse Tag declarations that conditionally include tags. Using intrinsic functions inside the Key or Value of a Tag is fine, but if you try to declare a tag using a top-level Fn::If (which is allowed by CloudFormation), parsing using CfnParser fails.

Expected Behavior

CfnParser should be able to correctly parse Tag declarations that use Fn::If.

Current Behavior

Tag declarations that use top-level Fn::If get turned into an empty {} by CfnParser, which causes failure during synthesis when {} is rejected as an invalid Tag by validateCfnTag()

The error message looks like this:

CfnSynthesisError: Resolution error: Supplied properties not correct for "CfnLoadBalancerProps"
  tags: element 0: {} should have a 'key' and a 'value' property.
    at ValidationResult.assertSuccess (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/runtime.js:11:1271)
    at convertCfnLoadBalancerPropsToCloudFormation (/workplace/cdkApp/node_modules/aws-cdk-lib/aws-elasticloadbalancingv2/lib/elasticloadbalancingv2.generated.js:26:80)
    at CfnLoadBalancer.renderProperties (/workplace/cdkApp/node_modules/aws-cdk-lib/aws-elasticloadbalancingv2/lib/elasticloadbalancingv2.generated.js:20:2083)
    at PostResolveToken.Resources [as processor] (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/cfn-resource.js:1:7100)
    at PostResolveToken.postProcess (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/util.js:1:1565)
    at Object.postProcess (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:1018)
    at DefaultTokenResolver.resolveToken (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/resolvable.js:1:1320)
    at resolve (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:2510)
    at Object.resolve (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:892)
    at resolve (/workplace/cdkApp/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:2787) {
  type: 'CfnSynthesisError'

Reproduction Steps

Create a CFN template with a resource containing a tag specification that looks like this:

"Tags" : [
  {
    "Fn::If" : [
      "ShouldIncludeTag",
      {"Key" : "TagName", "Value" : "TagValue"},
      {"Ref" :  "AWS::NoValue"}
    ]
  }
]

If you try to include that template into a CDK application using CfnInclude, it results in a synthesis failure.

Possible Solution

There are two places that are causing problems here. The first is the FromCloudFormation.getCfnTag() function from cfn-parse.ts. It assumes that a parsed tag will always be an object with Key and Value properties. Tags declared using Fn::If get turned into an IResolvable object instead. Adding if (isResolvableObject(tag)) { return tag } to getCfnTag() should solve that part of the problem.

The second problem is the cfnTagToCloudFormation() function from runtime.ts. It also assumes that tags are always objects with Key and Value properties. My recommended solution here would be to add if (!canInspect(x)) { return x; }, similar to what the listMapper and hashMapper functions do.

Additional Information/Context

No response

CDK CLI Version

2.94

Framework Version

No response

Node.js Version

18.16.0

OS

Linux

Language

TypeScript

Language Version

No response

Other information

No response

@jaredlundell jaredlundell added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2023
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Oct 18, 2023
@peterwoodworth peterwoodworth added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2023
@peterwoodworth
Copy link
Contributor

Thanks for the bug report and deep dive into the code @jaredlundell, since you seem to be familiar with it would you be willing to help us by submitting a PR?

@mergify mergify bot closed this as completed in #30515 Sep 10, 2024
mergify bot pushed a commit that referenced this issue Sep 10, 2024
### Issue # (if applicable)

Closes #27594.

### Reason for this change

Templates that use intrinsics in resource Tags cannot be used with CFN Include. 

### Description of changes

Modifed the CFN Parser to not choke on Intrinsics found in resource Tags. 

### Description of how you validated changes

Unit tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2024
xazhao pushed a commit to xazhao/aws-cdk that referenced this issue Sep 12, 2024
)

### Issue # (if applicable)

Closes aws#27594.

### Reason for this change

Templates that use intrinsics in resource Tags cannot be used with CFN Include. 

### Description of changes

Modifed the CFN Parser to not choke on Intrinsics found in resource Tags. 

### Description of how you validated changes

Unit tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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 subscribe to this conversation on GitHub. Already have an account? Sign in.