-
Notifications
You must be signed in to change notification settings - Fork 239
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
CloudFrontToS3
doesn't honor cloudFrontDistributionProps
#1108
Comments
The issue seems to lie within the The To resolve this issue, we could modify the |
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. |
Change
to
To allow clients to specify any subset of props that they wanted, we defined all the CDK props object as 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. |
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. |
@biffgaut Thanks for the detailed explanation about the design choices behind the Property validation would have been a lifesaver here, hopefully that can be added in the future. |
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? |
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 Thanks |
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. |
Now I'm a little confused, two questions:
|
Actually, I was wrong, please disregard my report. 🤦🏻♂️ I was under misconception that Short story – the CDK app is in Typescript, and I was deploying without rebuilding changes. Even Now that I am running the build step,
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. |
But the defaultCachePolicy is still an error, isn't it? |
(You mean 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
|
The props you define in the
CloudFrontToS3
construct get ignored. Here's an example:As you can see, you end up with the following configuration for the CloudFront distribution:
Distribution.priceClass
: set toPriceClass.PRICE_CLASS_ALL
instead ofPriceClass.PRICE_CLASS_100
Distribution.enableLogging
: set totrue
instead offalse
Reproduction Steps
cdk deploy
Error Log
Environment
Other
This is 🐛 Bug Report
The text was updated successfully, but these errors were encountered: