Closed pierresouchay closed 7 months ago
This looks AWESOME and thank you very much!!
One annoying caveat though. CloudFront logs for Lambda@Edge functions are peculiar, they do not actually store the logs in the usual log group with name similar to the Lambda function name in the region of the Lambda function. Instead Lambda@Edge logs get created in the region where the edge function happens to run (can be any region, depends on which edge location the user's request ended up hitting), and the log group name will be prefixed with us-east-1
.
Example log group name: /aws/lambda/us-east-1.MyStackName-AuthAtEdge-CheckAuthHandler-923RGl34OrvM
Have you seen that behavior? What's your take here? This is pretty impossible to fix in a reasonable way using CloudFormation I guess.
Anyways, semi fixing the Lambda function names might still be a good idea! Maybe we should only do it if the parameter prefix is given, and use AWS::NoValue
otherwise. I like that better than a default of AWS::StackId
, what do you think?
Have you seen that behavior? What's your take here? This is pretty impossible to fix in a reasonable way using CloudFormation I guess.
Hum yes... this sounds complicated... damned CloudFormation... so likely, not possible :(
Anyways, semi fixing the Lambda function names might still be a good idea! Maybe we should only do it if the parameter prefix is given, and use AWS::NoValue otherwise. I like that better than a default of AWS::StackId, what do you think?
I took this approach as it allows to have a similar behavior as the existing one: if you deploy twice the stack, it simply work. Without StackId, it would not as it would create twice the same lambda for 2 different stacks
Because of Lambda@Edge log behaviors (as discussed) adding the log groups in this PR is not useful is it? So then we better take them out again.
Naming the lambda's with a ResourceSuffix is still useful but would you mind using the AWS::NoValue
if ResourceSuffix is not given.
I took this approach as it allows to have a similar behavior as the existing one: if you deploy twice the stack, it simply work. Without StackId, it would not as it would create twice the same lambda for 2 different stacks
Not sure what went wrong there but CloudFormation is (mostly) deterministic. If you don's provide a lambda function name it will generate one, but it uses a deterministic method for that, and next time you deploy it will be the same name. So using AWS::NoValue
should work (you basically make the current way of not providing a lambda function name explicit then).
Also, for exiting users in this stack, with AWS::NoValue
this PR would be transparant, it would not recreate Lambdas upon redeployment. Only if you start specifying a ResourceSuffix.
Does that all make sense? Thank you for contributing @pierresouchay
stackID is deterministic. So it would be the same accross deployments. But using no Value would mean that by default, there is a clash between 2 stacks. That is why using StackID is IMHO better
Why a clash? I can deploy as many auth@edge stacks in the same account as I want without a clash currently, there is no clash. Under the hood CloudFormation uses stack id to generate the lambda function name (if you don't set it, or, same, set it to AWS:NoValue). So you don't have to explicitly do that. Don't think there's harm in it, but no advantage eithe.
Why a clash?
Because of
Properties:
LogGroupName:
Fn::Join:
- ""
- ["/aws/lambda/", "CheckAuthHandler", !Ref: "ResourceSuffix"]
=> If Resource Suffix := NoValue => 2 stacks would have the same ID. Initializing with StackID would remove this issue
@ottokruse Pushed my changes without the logs in 2nd commit
Ah that's what you mean. We can solve that with:
Conditions:
UseResourceSuffix: !Not [!Equals [!Ref ResourceSuffix, ""]] # Use "" as default value for ResourceSuffix
...
Properties:
FunctionName:
!If
- UseResourceSuffix
- Fn::Join:
- ""
- ["/aws/lambda/", "CheckAuthHandler", !Ref: "ResourceSuffix"]
- AWS::NoValue
Do you want to make that change @pierresouchay ?
Do you want to make that change @pierresouchay ?
Done
Will release to SAR soon
Will release to SAR soon
thank you
Published v2.1.9 to the SAR. FYI had to do some additional work to make ResourceSuffix work in https://github.com/aws-samples/cloudfront-authorization-at-edge/pull/256
Logs and naming is currently problematic in CloudFront authorization@edge
As LogGroup sucks with CloudFormation, because LogGroup has to be created before the lambda which creates the log group if not present, this MR follows this strategy: