Skip to content
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

[dotnet] Lambda deployment fails trying to deploy additional Log Retention Lambda #2240

Closed
msimpsonnz opened this issue Apr 10, 2019 · 8 comments
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. language/dotnet Related to .NET bindings needs-reproduction This issue needs reproduction.

Comments

@msimpsonnz
Copy link
Contributor

msimpsonnz commented Apr 10, 2019

I'm trying to get a simple Lambda function deployed using the CDK Version="0.28.0" with dotnet

            var lambda = new Function(this, "mjsdemo-blog-api", new FunctionProps
            {
                Code = code,
                Handler = "BlogAPI::BlogAPI.Functions::GetBlogsAsync",
                Runtime = Runtime.DotNetCore21
            });

However the CDK deploy fails on creating a Lambda function with looks like it relates to CloudTrail or XRay logs using Lambda running Node

The error I get is:

7/9 | 9:52:44 AM | CREATE_FAILED        | AWS::Lambda::Function | LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a (LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8aFD4BFC8A) Uploaded file must be a non-empty zip (Service: AWSLambdaInternal; Status Code: 400; Error Code: InvalidParameterValueException; Request ID: f8d1ae2a-5bda-11e9-9e1a-ad41c0ac9018)

Sample project is here:
https://github.com/msimpsonnz/cdknet-lam-dyn

@msimpsonnz
Copy link
Contributor Author

If I run the "same" thing using Typescript, I don't get all these Logging handlers deployed as extras, looks like there is some difference in the defaults between the two languages

    const hello = new lambda.Function(this, 'HelloHandler', {
      runtime: lambda.Runtime.DotNetCore21,
      code: lambda.Code.asset('lambda'),
      handler: 'BlogAPI::BlogAPI.Functions::GetBlogsAsync'                
    });

@eladb
Copy link
Contributor

eladb commented Apr 11, 2019

Acknowledging this is a bug. For some reason the .NET bindings pass in a value instead of null when the logRetention property is undefined.

Tracking here: aws/jsii#446

@igilham
Copy link
Contributor

igilham commented Aug 14, 2019

I'm getting a similar problem in TypeScript.

const func = new lambda.Function(this, 'testBotAudioVideo', {
      functionName: 'my-lambda',
      code: this.lambdaCode,
      handler: 'index.handler',
      runtime: lambda.Runtime.NODEJS_10_X,
      memorySize: 128,
      timeout: cdk.Duration.seconds(5)
    });

This works fine, but if I add logRetention = logs.RetentionDays.ONE_MONTH to the constructor props, it tries to create extra Lambda Functions with names like LogRetentionaae0aa3c5b4d4f87b02d85b201efdd8a, using CfnParameters for the S3 bucket and object key path with similar names.

Overriding log retention for a Lambda seems to be really broken at present.

Tested in 1.3.0 and 1.4.0.

@thevladeffect
Copy link

Why doesn't the logRetention attribute just create a LogGroup instead of creating a lambda that creates a log group? I worked around this by explicitly making the log group and then granting write to the lambda for it:

const logGroup = new LogGroup(this, 'log-group', {
      retention: RetentionDays.ONE_MONTH,
});
logGroup.grantWrite(handler);

@igilham
Copy link
Contributor

igilham commented Sep 4, 2019

I did the same thing, @thevladeffect . CloudFormation has always been weird about Lambda Functions and their LogGroups. The long-standing work-around is to create your own LogGroup with the name that would ordinarily be assigned to the automatically created one, then setting up retention and permissions as required.

Allowing the auto-generated LogGroup to be created for a service that goes into production can be expensive and irreversible if you properly version your infrastructure. It seems CDK does not improve on this bad design at this time, nor make it any easier to work around it.

@SomayaB SomayaB self-assigned this Sep 7, 2019
@SomayaB SomayaB added needs-triage This issue or PR still needs to be triaged. language/dotnet Related to .NET bindings bug This issue is a bug. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 7, 2019
@SomayaB SomayaB assigned NGL321 and unassigned SomayaB Sep 17, 2019
@NGL321 NGL321 added the @aws-cdk/aws-lambda Related to AWS Lambda label Nov 14, 2019
@NGL321 NGL321 assigned RomainMuller and unassigned NGL321 Nov 14, 2019
@RomainMuller
Copy link
Contributor

For context - the reason the log retention is altered using a Lambda-backed CustomResource is because there is no way to create the LogGroup before the function exists if it does not have a physical name (and we want it to work the same way regardless of whether a physical name was given or not), as this would create a cyclic dependency (LogGroup needs function name, but function shouldn't be created before LogGroup).

The options at this stage were:

  • Not give Lambda permissions to create log groups, and create the log group in CloudFormation (thus, after the Function was provisioned & thus given a name)
    • The issue with this is that the function may be called before the log group gets created, and it's log would be lost forever
  • Use a lambda-backed custom resource to "fix it up"

@RomainMuller
Copy link
Contributor

RomainMuller commented Nov 21, 2019

@msimpsonnz In any case - does this problem still exist with the latest CDK?

@RomainMuller RomainMuller added the needs-reproduction This issue needs reproduction. label Nov 21, 2019
@SomayaB SomayaB added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 21, 2019
@SomayaB SomayaB added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Dec 2, 2019
@SomayaB
Copy link
Contributor

SomayaB commented Dec 9, 2019

Closing this issue since there hasn't been an update in a while. Feel free to reopen.

@SomayaB SomayaB closed this as completed Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. language/dotnet Related to .NET bindings needs-reproduction This issue needs reproduction.
Projects
None yet
Development

No branches or pull requests

7 participants