[Core]: addPropertyOverride not producing correct result after upgrade

[Rohit-nagbhidkar]

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

[peterwoodworth]

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]

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]

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]

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]

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

[Rohit-nagbhidkar]

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]

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

[github-actions]

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

[YikaiHu]

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]

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]

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]

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.

[github-actions]

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