[Core]: addPropertyOverride not producing correct result after upgrade #19971

Rohit-nagbhidkar · 2022-04-19T17:08:48Z

Describe the bug

When adding a condition on the LambdaFunctionArn to be attached to Custom::S3BucketNotifications resource, cdk is appending the If condition rather than replacing it.
This was working with version 1.100.0, but due to a new requirement we bumped up the version to 1.152.0
PFB for code snippet

      config.uploadTempBucket.addObjectCreatedNotification(new LambdaDestination(config.s3DocUploadLambda));
       const conditionForDT = new CfnCondition(scope, 'EnvCheckDT', {
           expression: Fn.conditionEquals(config.envName, 'DT')
       });
       const fileSecurityScanLambda = Function.fromFunctionArn(scope, 'FileSecurityScan-Lambda', config.fileScanLambdaArnString);
       const applicableFunction = Fn.conditionIf(conditionForDT.logicalId, config.s3DocUploadLambda.functionArn, fileSecurityScanLambda.functionArn).toString();
       let cfnResource = Stack.of(scope).node.findAll(ConstructOrder.POSTORDER).find(child => child instanceof CfnResource && child.cfnResourceType == 'Custom::S3BucketNotifications') as CfnResource;
//issue in following code
       cfnResource.addPropertyOverride('NotificationConfiguration.LambdaFunctionConfigurations.0.LambdaFunctionArn',
           applicableFunction
       );

Expected Behavior

in earlier version 1.100.0 the cfn template produced from the code is as below

      NotificationConfiguration:
        LambdaFunctionConfigurations:
          - Events:
              - s3:ObjectCreated:*
            LambdaFunctionArn:
              Fn::If:
                - EnvCheckDT
                - Fn::GetAtt:
                    - AegonDocumentUploadv11F49C644
                    - Arn
                - Ref: FileScanLambdaArn

Current Behavior

after version upgrade to 1.152.0 the cfn template produced from the code is as below


      NotificationConfiguration:
        LambdaFunctionConfigurations:
          - Events:
              - s3:ObjectCreated:*
            LambdaFunctionArn:
          Fn::GetAtt:
                - AegonDocumentUploadv11F49C644
                - Arn
              Fn::If:
                - EnvCheckDT
                - Fn::GetAtt:
                    - AegonDocumentUploadv11F49C644
                    - Arn
                - Ref: FileScanLambdaArn

Code is getting synthesized without errors
but while deployment it's giving following error.

Received response status [FAILED] from custom resource. Message returned: Error: Parameter validation failed: Invalid type for parameter NotificationConfiguration.LambdaFunctionConfigurations[0].LambdaFunctionArn, value: {'Fn::GetAtt': ['DocumentUploadv11F49C644', 'Arn'], 'Fn::If': ['EnvCheckDT', 'arn:aws:lambda:ap-south-1:xxx:function:document-upload-v1-DT', 'arn:aws:lambda:ap-south-1:xxx:function:document-upload-v1-DT']}, type: <class 'dict'>, valid types: <class 'str'>.

Kindly look into this

Reproduction Steps

try writing similar kind of code to reproduce

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

1.152.0 (build 9487b39)

Framework Version

No response

Node.js Version

14.16.1

OS

Windows 10

Language

Typescript

Language Version

4.1.6

Other information

No response

The text was updated successfully, but these errors were encountered:

peterwoodworth · 2022-04-21T23:45:25Z

I've come across this recently as well @Rohit-nagbhidkar

When I called addPropertyOverride() on a resource that had a property set by the CDK, it appended my override to what was already on the specified property. I would have expected that override overrides any existing data

@rix0rrr is this the intended functionality?

sofcal · 2022-04-26T00:23:14Z

I've seen this too. I don't know the aws-cli project at all, but a quick play shows the behaviour seems to have changed with this commit: ebac8bc. The tokenised value is now being resolved before passing it to deepMerge. The tokenised value is a string, whereas the resolved value is an object. So now in deepMerge, rather than assigning the entire value to the property, it's recursing.

Whether it's intentional or not I'm not sure, but went in back in June last year.

addPropertyOverride works fine for me if I use a string, but if I try to use a token it fails. My intention was to replace a TemplateUrl with an Fn::Sub that could resolve at deploy time.

peterwoodworth · 2022-04-27T19:54:24Z

Great find @sofcal, I think you're right about the source of the issue.

In case anyone comes across this looking for a short term fix, call addPropertyDeletionOverride() just before you addPropertyOverride() the same property

sofcal · 2022-04-27T20:51:45Z

Great find @sofcal, I think you're right about the source of the issue.

In case anyone comes across this looking for a short term fix, call addPropertyDeletionOverride() just before you addPropertyOverride() the same property

This didn't actually work for me. The second override was overwriting the first; I was testing on cdk v2, attempting to override the TemplateURL. The only option that worked for me was to override with the same structure as the original value; e.g. the TemplateUrl uses a Fn:Join, so I replaced it with my own Fn:join operation, which then correctly replaced.

peterwoodworth · 2022-04-27T21:16:32Z

Interesting, that had worked for me when I had ran into this. Thanks for the info

Rohit-nagbhidkar · 2022-04-28T05:47:29Z

Great find @sofcal, I think you're right about the source of the issue.

In case anyone comes across this looking for a short term fix, call addPropertyDeletionOverride() just before you addPropertyOverride() the same property

Yes, as faced by @sofcal, i am also not able to get desired output with addPropertyDeletionOverride() before addPropertyOverride(). it produces the same result.

rix0rrr · 2022-05-23T14:14:39Z

The fix would be to not recurse into intrinsics when merging these objects.

…20608) When you add a property override, we merge it with any properties set when the construct is initialized. The merge happens _after_ tokens are resolved. For example, if we created a resource: ```ts const resource = new CfnResource(this, 'Resource', { type: 'MyResourceType', properties: { prop1: Fn.ref('Param'), }, }); ``` And the added an override for `prop1`: ```ts resource.addPropertyOverride('prop1', Fn.ref('OtherParam')); ``` We would then perform a deep merge on the properties with a priority on the overrides. The above example would work fine, eventually the merge would get to the point where it was comparing two objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { Ref: 'OtherParam' } } result = { prop1: { Ref: 'OtherParam' } } ``` If we instead added an override using a different intrinsic: ```ts resource.addPropertyOverride('prop1', Fn.join('-', ['hello', Fn.ref('Param')])); ``` Then we would get to the point in the merge where we were comparing two different objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } } result = { prop1: { Ref: 'Param', 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] }, }} ``` This PR solves this issue by filtering out any CloudFormation intrinsics. If the merge finds an intrinsic key in the target it "drops" the intrinsic and takes whatever value is in the source (override). fix #19971, #19447 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions · 2022-06-03T14:09:01Z

⚠️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.

…ws#20608) When you add a property override, we merge it with any properties set when the construct is initialized. The merge happens _after_ tokens are resolved. For example, if we created a resource: ```ts const resource = new CfnResource(this, 'Resource', { type: 'MyResourceType', properties: { prop1: Fn.ref('Param'), }, }); ``` And the added an override for `prop1`: ```ts resource.addPropertyOverride('prop1', Fn.ref('OtherParam')); ``` We would then perform a deep merge on the properties with a priority on the overrides. The above example would work fine, eventually the merge would get to the point where it was comparing two objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { Ref: 'OtherParam' } } result = { prop1: { Ref: 'OtherParam' } } ``` If we instead added an override using a different intrinsic: ```ts resource.addPropertyOverride('prop1', Fn.join('-', ['hello', Fn.ref('Param')])); ``` Then we would get to the point in the merge where we were comparing two different objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } } result = { prop1: { Ref: 'Param', 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] }, }} ``` This PR solves this issue by filtering out any CloudFormation intrinsics. If the merge finds an intrinsic key in the target it "drops" the intrinsic and takes whatever value is in the source (override). fix aws#19971, aws#19447 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

YikaiHu · 2022-09-15T15:18:57Z

I still face the same issue as @sofcal and @Rohit-nagbhidkar.

I am also not able to get desired output with addPropertyDeletionOverride() before addPropertyOverride().
It produces the same result.

The cdk appended my override to what was already on the specified property.

My cdk version is 2.20.0 (build 738ef49).

YikaiHu · 2022-09-15T15:25:32Z

Could you please have a check on this? @rix0rrr Thanks!

My code is :

import {
  Aws, Stack, StackProps, aws_cloudfront as cloudfront, Aspects,
  IAspect, CfnResource, CfnCondition, Fn
} from 'aws-cdk-lib';
import { Construct, IConstruct } from 'constructs';
import { CloudFrontToS3 } from "@aws-solutions-constructs/aws-cloudfront-s3";

export class CloudfrontLoggingDevStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const isOpsInRegion = new CfnCondition(this, "isOpsInRegion", {
      expression: Fn.conditionEquals(Aws.REGION, "us-east-1"),
    });

    // Use cloudfrontToS3 solution contructs
    const portal = new CloudFrontToS3(this, "UI", {
      bucketProps: {
        versioned: true,
        enforceSSL: true,
        autoDeleteObjects: false,
      },
      cloudFrontDistributionProps: {
        priceClass: cloudfront.PriceClass.PRICE_CLASS_ALL,
        minimumProtocolVersion: cloudfront.SecurityPolicyProtocol.TLS_V1_2_2019,
        enableIpv6: false,
        enableLogging: true, //Enable access logging for the distribution.
        comment: `${Aws.STACK_NAME} - Kervin Dev Distribution (${Aws.REGION})`,
        errorResponses: [
          {
            httpStatus: 403,
            responseHttpStatus: 200,
            responsePagePath: "/index.html",
          },
        ],
      },
      insertHttpSecurityHeaders: false,
    });

    const loggingBucket = portal.cloudFrontLoggingBucket

    const isEnableLogging = Fn.conditionIf(isOpsInRegion.logicalId, {
      "Ref": "AWS::NoValue"
    }, {
      "Bucket": loggingBucket?.bucketDomainName
    });
    const cfnCloudFrontWebDistribution = portal.cloudFrontWebDistribution.node.defaultChild as cloudfront.CfnDistribution;
    cfnCloudFrontWebDistribution.addPropertyDeletionOverride("DistributionConfig.Logging")
    
    cfnCloudFrontWebDistribution.addPropertyOverride("DistributionConfig.Logging", isEnableLogging)

  }
}

The key part output of the cdk synth is:

     "Logging": {
      "Bucket": {
       "Fn::GetAtt": [
        "UICloudfrontLoggingBucket4E929DEC",
        "RegionalDomainName"
       ]
      },
      "Fn::If": [
       "isOpsInRegion",
       {
        "Ref": "AWS::NoValue"
       },
       {
        "Bucket": {
         "Fn::GetAtt": [
          "UICloudfrontLoggingBucket4E929DEC",
          "DomainName"
         ]
        }
       }
      ]
     },

My expectation is:

     "Logging": {
      "Fn::If": [
       "isOpsInRegion",
       {
        "Ref": "AWS::NoValue"
       },
       {
        "Bucket": {
         "Fn::GetAtt": [
          "UICloudfrontLoggingBucket4E929DEC",
          "DomainName"
         ]
        }
       }
      ]
     },

corymhall · 2022-09-15T17:26:03Z

It looks like the linked PR was only a partial fix for when the original property and the override are both intrinsics. It also needs to handle when only one or the other is.

YikaiHu · 2022-09-16T05:07:46Z

Here is a workaround

    const cfnCloudFrontWebDistribution = portal.cloudFrontWebDistribution.node.defaultChild as cloudfront.CfnDistribution;

    (cfnCloudFrontWebDistribution.distributionConfig as any).logging = Fn.conditionIf(isOpsInRegion.logicalId, Aws.NO_VALUE,
      {
      Bucket: portal.cloudFrontLoggingBucket!.bucketRegionalDomainName,
    })

The key point is to use defaultChild to modify (true override) the specific property.

There was a previous fix in #20608 that attempted to fix addPropertyOverride when intrinisics were involved. This PR fixes an edge case where overrides do not work correctly an object is being replaced with an intrinsic. For example, there might be the case where the override is an intrinsic and it is overriding an object, not a value. ``` original: { Type: 'MyResourceType', Properties: { prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } } } } override: { Properties: { prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }} } } ``` The previous fix only handled cases where the original had an intrinsic, but in the above example the override is the first to hit an intrinsic. This PR adds logic to handle cases where we hit an intrinsic in the original _or_ the override. fixes #19971 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions · 2022-09-30T21:00:11Z

⚠️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.

…2294) There was a previous fix in aws#20608 that attempted to fix addPropertyOverride when intrinisics were involved. This PR fixes an edge case where overrides do not work correctly an object is being replaced with an intrinsic. For example, there might be the case where the override is an intrinsic and it is overriding an object, not a value. ``` original: { Type: 'MyResourceType', Properties: { prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } } } } override: { Properties: { prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }} } } ``` The previous fix only handled cases where the original had an intrinsic, but in the above example the override is the first to hit an intrinsic. This PR adds logic to handle cases where we hit an intrinsic in the original _or_ the override. fixes aws#19971 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Rohit-nagbhidkar added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 19, 2022

github-actions bot added the @aws-cdk/core Related to core CDK functionality label Apr 19, 2022

github-actions bot assigned rix0rrr Apr 19, 2022

peterwoodworth added p1 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 Apr 21, 2022

rix0rrr removed their assignment May 23, 2022

corymhall self-assigned this Jun 2, 2022

corymhall mentioned this issue Jun 3, 2022

fix(core): property overrides sometimes don't work with intrinsics #20608

Merged

4 tasks

mergify bot closed this as completed in #20608 Jun 3, 2022

YikaiHu mentioned this issue Sep 15, 2022

[Core]: addPropertyOverride still not producing correct result after upgrade #22057

Closed

corymhall reopened this Sep 15, 2022

corymhall mentioned this issue Sep 29, 2022

fix(core): addPropertyOverride doesn't work for all intrinsics #22294

Merged

4 tasks

mergify bot closed this as completed in #22294 Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core]: addPropertyOverride not producing correct result after upgrade #19971

[Core]: addPropertyOverride not producing correct result after upgrade #19971

Rohit-nagbhidkar commented Apr 19, 2022

peterwoodworth commented Apr 21, 2022

sofcal commented Apr 26, 2022 •

edited

Loading

peterwoodworth commented Apr 27, 2022

sofcal commented Apr 27, 2022

peterwoodworth commented Apr 27, 2022

Rohit-nagbhidkar commented Apr 28, 2022

rix0rrr commented May 23, 2022

github-actions bot commented Jun 3, 2022

YikaiHu commented Sep 15, 2022

YikaiHu commented Sep 15, 2022

corymhall commented Sep 15, 2022

YikaiHu commented Sep 16, 2022

github-actions bot commented Sep 30, 2022

[Core]: addPropertyOverride not producing correct result after upgrade #19971

[Core]: addPropertyOverride not producing correct result after upgrade #19971

Comments

Rohit-nagbhidkar commented Apr 19, 2022

Describe the bug

Expected Behavior

Current Behavior

Reproduction Steps

Possible Solution

Additional Information/Context

CDK CLI Version

Framework Version

Node.js Version

OS

Language

Language Version

Other information

peterwoodworth commented Apr 21, 2022

sofcal commented Apr 26, 2022 • edited Loading

peterwoodworth commented Apr 27, 2022

sofcal commented Apr 27, 2022

peterwoodworth commented Apr 27, 2022

Rohit-nagbhidkar commented Apr 28, 2022

rix0rrr commented May 23, 2022

github-actions bot commented Jun 3, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

YikaiHu commented Sep 15, 2022

YikaiHu commented Sep 15, 2022

corymhall commented Sep 15, 2022

YikaiHu commented Sep 16, 2022

github-actions bot commented Sep 30, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

sofcal commented Apr 26, 2022 •

edited

Loading