Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

fix: Comment out Lambda return value to avoid nodejs14.x #637

Merged
merged 2 commits into from
Feb 24, 2024

Conversation

bmorrissirromb
Copy link
Contributor

@bmorrissirromb bmorrissirromb commented Feb 23, 2024

Issue #, if available:
Closes #624

Description of Changes

Background

Lambda Functions in CDK, by default, do not create Log Groups at CDK deploy time. Log Groups are typically find-or-created when the Lambda executes for the first time.

HOWEVER, there are certain scenarios where the CloudFormation stack needs to know what the Log Group will be. For example, if the Function's log retention needs to be set to a specific value, or if some other resource needs to know the Log Group's properties.

In THESE cases, CDK will generate a Custom::LogRetention resource in the background, intended to set retention and return metadata about a Lambda's Log Group. You guessed it, this Custom Resource is itself a Lambda with a runtime of...drumroll...nodejs14.x (at least in CDK 2.50.0). So that's where our problems lies.

Diving Deep

The question is...why does this get created in this case at all? There's no mention of log retention ANYWHERE in this CDK application. However, careful readers may have picked up on this already: the custom resource also gets created if we're ever reading Log Group metadata. Which we're doing in each of the 4 engine constructs (eg.

). That seemingly inoffensive line of code is causing huge deployment issues down the line.

Solutioning

The next question is what would happen if we just...removed these offending lines of code. And the answer is...pretty much nothing. Setting this.adapterLogGroup = lambda.logGroup is effectively unnecessary because the only time that adapterLogGroup property is used is as a CfnOutput (which already has error handling if the value is undefined!!).

Therefore, my proposal is to comment out these 4 lines of code. If someone wants to see the Log Group name, they can look it up from the Lambda at Lambda-execute time. I will be submitting a PR for this.

Validation

Validated using cdk synth locally.

@wleepang
Copy link
Contributor

After some testing:
This fix restores the ability to deploy contexts, but introduces a bug where the CLI client can no longer retrieve adapter logs via:

agc logs adapter -c myContext

@wleepang
Copy link
Contributor

The agc logs adapter command just needs the name of the LogGroup for the WesAdapterLambda. These LogGroup names can be interpolated as:

'/aws/lambda/' + lambda.functionName

So a fix that also maintains the ability to get adapter logs is to replace the offending lines with something like:

this.adapterLogGroup = LogGroup.fromLogGroupName(this, 'NextflowAdapterLogGroup', '/aws/lambda/' + lambda.functionName)

I just verified this with a Nextflow context.

@wleepang wleepang merged commit b5b04f5 into aws:main Feb 24, 2024
4 of 5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node.js 14
2 participants