aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

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

Closed msimpsonnz closed 4 years ago

msimpsonnz commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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: https://github.com/awslabs/jsii/issues/446

igilham commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

RomainMuller commented 4 years ago

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:

RomainMuller commented 4 years ago

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

SomayaB commented 4 years ago

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