`CloudFrontToS3` doesn't honor `cloudFrontDistributionProps`

[garysassano]

The props you define in the CloudFrontToS3 construct get ignored. Here's an example:

const cloudfront = new CloudFrontToS3(this, "CloudFrontToS3", {
  existingBucketObj: websiteBucket,
  cloudFrontDistributionProps: {
    PriceClass: PriceClass.PRICE_CLASS_100,
  },
  logS3AccessLogs: false,
});

As you can see, you end up with the following configuration for the CloudFront distribution:

• Distribution.priceClass: set to PriceClass.PRICE_CLASS_ALL instead of PriceClass.PRICE_CLASS_100
• Distribution.enableLogging: set to true instead of false

Reproduction Steps

cdk deploy

Error Log

Environment

• CDK CLI Version :
• CDK Framework Version:
• AWS Solutions Constructs Version :
• OS :
• Language :

Other

---

This is :bug: Bug Report

[garysassano]

The issue seems to lie within the consolidateProps function from the @aws-solutions-constructs/core library. This function is used to merge default properties with user-provided properties for the CloudFront distribution.

The consolidateProps function performs a deep merge, which can lead to unintended consequences. In this case, the default PriceClass property within the construct is overwriting the user-provided PriceClass due to the deep merge behavior.

To resolve this issue, we could modify the consolidateProps function to prioritize user-provided properties over defaults.

[biffgaut]

Thanks - sorry for the delay getting to this, I'm taking a look now. The logging behavior is correct - logS3AccessLogs defines whether access logs are turned on for the S3 bucket created. To control logging for the distribution, use cloudfront.DistributionProps.enableLogging.

That said, the other issue seems of concern and may affect your ability to use DistributionProps to set enableLogging.

[biffgaut]

Change

PriceClass: PriceClass.PRICE_CLASS_100,

to

priceClass: PriceClass.PRICE_CLASS_100,

To allow clients to specify any subset of props that they wanted, we defined all the CDK props object as _propstype_ | any. The upside of this is that you can set any individual property without having to specify all the required properties. The downside is that we lose type safety and we are vulnerable to typos like this.

We've explored using Typescript Partial classes, but the JSII library we use to provide Python and Java interfaces doesn't support Partial classes - I assume because of limitations in Python and/or Java (JSII is a library the CDK team wrote and uses to provide multilanguage support for the CDK).

On wish list is to set up a step where we look at all incoming props objects and throw an error if someone specifies a property not found in the CDK defined type - that would have caught this. There is no ETA on such functionality at the moment.

[biffgaut]

FWIW - Consolidate Props does prioritize client values over default values. It accepts up to 3 sets of props as arguments - defaults, client props and construct props. Construct props are settings that are required for the construct to operate - DNS settings in a VPC when using endpoints for instance. Consolidate props conducts 2 deep merges, client and default - prioritizing client. The output of that is then merged with construct props - with construct props being given priority as they are essential to the operation of the Solutions Construct.

[garysassano]

@biffgaut Thanks for the detailed explanation about the design choices behind the CloudFrontToS3 construct. That clarifies why I encountered issues like the priceClass typo and the lack of IntelliSense suggestions for enableLogging, which lead me to believe the prop wasn't present (I hadn't realized the cloudFrontDistributionProps interface used the any type for flexibility). The insights into consolidateProps were also very valuable.

Property validation would have been a lifesaver here, hopefully that can be added in the future.

[ameotoko]

I've encountered similar problem using https://github.com/aws-solutions/video-on-demand-on-aws-foundation, which uses this construct.

At least following properties are ignored by the construct:

cloudFrontDistributionProps: {
    defaultBehavior: {
        allowedMethods: AllowedMethods.ALLOW_GET_HEAD_OPTIONS,
        compress: false,
        responseHeadersPolicy: ResponseHeadersPolicy.CORS_ALLOW_ALL_ORIGINS_WITH_PREFLIGHT
    },
}

Is there a way around this for now?

[biffgaut]

This is an issue internal to the VOD Solution code (and how they invoke Solutions Constructs).

Please enter an issue on their github repo pointing out that they are using the wrong attribute name on

this line (should be defaultBehavior), and this line (should be compress)

Thanks

[ameotoko]

Hey @biffgaut, thanks for the quick response! Sorry if I haven't been clear – the code snippet in my previous message is exactly what I tried, it contains correctly named properties. I already figured out that they used wrong attribute names (honestly, their whole code is a mess but that's a different story) and I fixed them. Correct properties still have no effect on deployment.

[biffgaut]

Correct properties still have no effect on deployment.

Now I'm a little confused, two questions:

• How did you "fix" their internal code?
• Now you're launching CloudFrontToS3 with correct attribute values that are then being ignored?

[ameotoko]

Actually, I was wrong, please disregard my report. 🤦🏻‍♂️ I was under misconception that cdk deploy automatically builds the app and synthesizes the template. Even if that's the case, either their repo is misconfigured or I need to study the CDK better.

Short story – the CDK app is in Typescript, and I was deploying without rebuilding changes. Even cdk deploy --watch did not pick it up.

Now that I am running the build step, cdk diff and cdk deploy steps manually - everything actually works. I was even able to set cachePolicy to my custom policy that already pre-existed in my account. Sorry to waste your time @biffgaut.

How did you "fix" their internal code?

Their code is a public repo on GitHub. It's just a CDK app, I cloned and customized it, which is actually the way they recommend.

[biffgaut]

But the defaultCachePolicy is still an error, isn't it?

[ameotoko]

But the defaultCachePolicy is still an error, isn't it?

(You mean defaultCacheBehavior). That's right. Here's my real config that I just deployed and can confirm that it works:

const cloudFront = new CloudFrontToS3(this, 'CloudFront', {
    existingBucketObj: destination,
    insertHttpSecurityHeaders: false,
    cloudFrontDistributionProps: {
        comment:`${cdk.Aws.STACK_NAME} Video on Demand Foundation`,
        defaultBehavior: {
            allowedMethods: AllowedMethods.ALLOW_GET_HEAD_OPTIONS,
            compress: false,
            forwardedValues: {
              queryString: false,
              headers: [ 'Origin', 'Access-Control-Request-Method','Access-Control-Request-Headers' ],
              cookies: { forward: 'none' }
            },
            viewerProtocolPolicy: ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
            responseHeadersPolicy: ResponseHeadersPolicy.CORS_ALLOW_ALL_ORIGINS_WITH_PREFLIGHT,

            // custom policy "CachingOptimizedAllowOrigins"
            cachePolicy: { cachePolicyId: '41b58241-399f-4200-8914-465e7aac65b9' },
        },
        logBucket: logsBucket,
        logFilePrefix: 'cloudfront-logs/'
    }
});

That defaultCacheBehavior cost me a couple hours of lurking through the docs and source code. I can easily see where the confusion comes from, just check this out:

• https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-defaultcachebehavior.html#cfn-cloudfront-distribution-defaultcachebehavior-responseheaderspolicyid
• https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_cloudfront.CfnDistribution.DefaultCacheBehaviorProperty.html