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

(EvaluateExpression): The logical id of lambda created by EvaluateExpression changed after nodejs upgrade. #29212

Closed
weididai opened this issue Feb 22, 2024 · 8 comments
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort needs-review p1 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@weididai
Copy link

Describe the bug

We recently encountered a issue that the logical id of lambda created by EvaluateExpression changed after nodejs upgrade. This resulted in a new lambda got created and the old lambda got deleted which caused some impact. Upon some investigation, we discovered that is because the logical id of lambda created by EvaluateExpression is determined by nodejs version -- https://github.com/aws/aws-cdk/blob/v2.120.0/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/evaluate-expression.ts#L114. Is there a workaround here to do a nodejs upgrade without changing the logical id of the lambda.

We already tried using the overrideLogicalId but it seems the overrideLogicalId does not support for lambda created by EvaluateExpression

Expected Behavior

The logical id of lambda created by EvaluateExpression should not change after nodejs upgrade.

Current Behavior

The logical id of lambda created by EvaluateExpression is changed after nodejs upgrade.

Reproduction Steps

  1. Use nodejs 14.x for the EvaluateExpression and check the logical id of the lambda generated.
  2. Change the nodejs version to 18.x and then check the logical id of the lambda generated.
  3. The logical id generated from step 2 is different from step 1

Possible Solution

  1. Support overrideLogicalId of the lambda created by EvaluateExpression
  2. Only use the nodejs version to generate logical id if two EvaluateExpression have different nodejs version under the same path.

Additional Information/Context

No response

CDK CLI Version

2.120.0

Framework Version

No response

Node.js Version

14.x

OS

linux

Language

TypeScript

Language Version

No response

Other information

No response

@weididai weididai added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 22, 2024
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Feb 22, 2024
@pahud
Copy link
Contributor

pahud commented Feb 23, 2024

Yes I can confirm it does change when updating the runtime. This could be related to #28251. Making it a p1.

@pahud pahud added p1 needs-review effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 23, 2024
@colifran colifran self-assigned this Feb 23, 2024
@colifran
Copy link
Contributor

I don't think this is related to #28251. To me it appears that this is the intended behavior. We have an object nodeJsGuids which contains nodejs runtimes as keys and a uuid as a value. A SingletonFunction looks through the construct tree to see if a function with the same uuid already exists in the same scope and won't create a new one if it does. Since we're updating the uuid when we update the runtime I would expect a new function to be created and the old one to be deleted. That's not to say we couldn't support the proposed behavior, though.

@weididai
Copy link
Author

Thanks colifran, I understand this might be the intended behavior since we want to have different lambda created for different nodejs version. Currently, there is no way that I realize that could upgrade the nodejs version without updating the logical id of lambda. So I think team could give a workaround here by supporting overrideLogicalId of the lambda created by EvaluateExpression.

@pahud
Copy link
Contributor

pahud commented Feb 26, 2024

From my test, simply updating the runtime would not change the lambda.Function logicalId.

For example, change the runtime from NODEJS_16_X to NODEJS_18_X would only have in-place update:

    new lambda.Function(this, 'Func', {
        code: lambda.Code.fromInline('mock'),
        handler: 'index.handler',
        runtime: lambda.Runtime.NODEJS_16_X,
    });
image

But changing the runtime for EvaluateExpression would trigger Function and Role replacement:

const task = new tasks.EvaluateExpression(this, 'Task', {
  expression: '$.a + $.b',
  runtime: lambda.Runtime.NODEJS_16_X,
});
image

I am guessing it's EvalNodejsSingletonFunction that returns new resources.

Wondering it might because EvalNodejsSingletonFunction is actually a SingletonFunction which always ensureLambda based on given props and generate different constructName here, hence generating a new lambda.Function with different id from here.

I guess it's by design how SingletonFunction works when we pass a new uuid, CDK would always create a new lambda.Function by replacing the existing one and it's how it works now.

@weididai What is the impact if the lambda.Function would be replaced with a new logicalId? Can you share more about that?

@weididai
Copy link
Author

The logical id of the lambda created by EvaluateExpression is changed during a long running step function. Thus the step function fails as it tries to invoke the old lambda but the old lambda was already destroyed.

@pahud
Copy link
Contributor

pahud commented Feb 27, 2024

@weididai Thank you for your report I think we should figure out a way to avoid that as updating a SingletonFunction would always change the logicalId.

@pahud
Copy link
Contributor

pahud commented Feb 29, 2024

Hi @weididai

Consider this sample:

export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const expression = new tasks.EvaluateExpression(this, 'Task', {
      expression: '$.a + $.b',
      runtime: lambda.Runtime.NODEJS_16_X
    });

    this.overrideLambdaLogicalId('Evalda2d1181604e4a4586941a6abd7fe42dF371675D')

  }

  private overrideLambdaLogicalId(logicalId: string) {
    this.node.findAll().filter((construct) => 
    construct.node.id.startsWith('Eval') && (construct.node.defaultChild instanceof lambda.CfnFunction))
    .forEach(x => {
      (x.node.defaultChild as lambda.CfnFunction).overrideLogicalId(logicalId);
    })
  }

}

If you change the runtime and run npx cdk diff you should see this, indicating the lambda Function would not be replaced.

image

Let me know if it works for you.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Feb 29, 2024
@pahud pahud assigned pahud and unassigned colifran Feb 29, 2024
Copy link

github-actions bot commented Mar 2, 2024

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 2, 2024
@github-actions github-actions bot added closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Mar 7, 2024
@github-actions github-actions bot closed this as completed Mar 7, 2024
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. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort needs-review p1 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants