-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(cloudfront): function URL
origin access control L2 construct
#31339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
function URL
origin access control L2 construct
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this so quickly @watany-dev! :)
* An Origin for a Lambda Function URL with OAC. | ||
*/ | ||
export class FunctionUrlOriginWithOAC extends cloudfront.OriginBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this class private as it's an implementation detail and not intended for public usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
a43bf40
new cloudfront.Distribution(stack, 'MyDistribution', { | ||
defaultBehavior: { | ||
origin: FunctionUrlOrigin.withOriginAccessControl(fnUrl, { | ||
originAccessControl: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is undefined
passed here? Could we just leave out the props altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
f660a83
} | ||
|
||
new lambda.CfnPermission(scope, `InvokeFromApiFor${options.originId}`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into a private function? It would make it clearer and easier to understand why a new permission is getting created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is it possible to use any existing grant or addPermission methods here instead? And how do we handle permissions for imported lambda functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there might still be a potential bug with addPermission, so I’m using cfnPermission instead. It’s not too complex, and since the cross-stack test is passing, I don't have any concerns. Here’s a reference (apologies, it's in Japanese!) https://dev.classmethod.jp/articles/cdk-over-21-lambda-create-error/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...dk-testing/framework-integ/test/aws-cloudfront-origins/test/integ.function-url-origin-oac.ts
Show resolved
Hide resolved
|
||
const template = Template.fromStack(stack); | ||
|
||
template.hasResourceProperties('AWS::CloudFront::Distribution', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this test is checking for the correct permissions, can we also check the AWS::Lambda::Permission
resource exists in the template and has the expected policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
7226680
packages/aws-cdk-lib/aws-cloudfront-origins/lib/function-url-origin.ts
Outdated
Show resolved
Hide resolved
Just to confirm, will this proposal work well with Lambda function aliases and versions? Background: Lambda provisioned concurrency only works with Lambda function versions or aliases. Given that, we have all of our function URLs pointing to aliases instead of "normal" Lambda functions. Before this was merged, just wanted to make sure this PR would also accommodate function URLs backed by versions/aliases. From updated README looks like the answer is yes and it would be something like:
Might be worth adding to README updates given this may be a pretty common pattern. |
body: 'Forbidden', | ||
})); | ||
|
||
app.synth(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we no longer need app.synth()
in integ tests so this line can be removed from the new integ tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
baddb09
Thanks for pointing out this use case! I tested setting up a Lambda function alias with OAC with this API and it deployed/setup successfully. @watany-dev could you please add an integration test and README example for this use case? |
@rhermes62 @gracelu0 |
Thank you @watany-dev ! We are still pending the security review and service team confirmation, we appreciate your effort and patience in getting this reviewed. |
packages/aws-cdk-lib/aws-cloudfront-origins/lib/function-url-origin.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
@gracelu0 |
Hi @watany-dev , I think it looks good! Just waiting on confirmation from security reviewer before I approve (should be very soon!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this @watany-dev !
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31339 +/- ##
=======================================
Coverage 77.18% 77.18%
=======================================
Files 105 105
Lines 7161 7161
Branches 1312 1312
=======================================
Hits 5527 5527
Misses 1454 1454
Partials 180 180
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
#31629
Reason for this change
This change introduces support for Lambda Function URLs with custom Origin Access Control (OAC) in CloudFront distributions, enhancing security and control over CloudFront-Lambda integration.
Description of changes
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license