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

fix(lambda): circular dependencies when EFS and Lambda are deployed in separate stacks #28560

Merged
merged 5 commits into from
Jan 13, 2024

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented Jan 3, 2024

This PR fixed an error when deploying EFS and Lambda in separate stacks.

Cause of the bug

Currently, when using EFS from Lambda, deploying EFS and Lambda in separate stacks creates incorrect resource dependencies and cannot be deployed correctly.
This error is caused by adding a security group setting in the Function construct to allow EFS and Lambda to communicate correctly.
By calling the Connections.allowDefaultPortFrom method of the Filesystem in the LambdaStack, IngressRule is created in the scope of the EfsStack.
Note that the remoteRule flag is false when calling SecurityGroupBase.addIngressRule at this time.

props.filesystem.config.connections.allowDefaultPortFrom(this);

securityGroup.addIngressRule(rule, portRange, description, remoteRule);

public addIngressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {

Here is the minimal code to reproduce this error without EFS and Lambda.

#!/usr/bin/env node
import 'source-map-support/register';
import { App, Stack, StackProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as ec2 from 'aws-cdk-lib/aws-ec2';

export class EfsStack extends Stack {
    public vpc: ec2.Vpc;
    public efsSg: ec2.SecurityGroup;

    constructor(scope: Construct, id: string, props?: StackProps) {
      super(scope, id, props);

      this.vpc = new ec2.Vpc(this, 'Vpc');

      this.efsSg = new ec2.SecurityGroup(this, 'SecurityGroup', {
        vpc: this.vpc,
        allowAllOutbound: true,
      });
    }
}

interface LambdaStackProps extends StackProps {
    vpc: ec2.Vpc;
    efsSg: ec2.SecurityGroup;
}

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

      const lambdaSg = new ec2.SecurityGroup(this, 'SecurityGroup', {
        vpc: props.vpc,
        allowAllOutbound: true,
      });

      // Since `remoteRule` flag is set to false here, IngressRule is deployed in EfsStack scope.
      props.efsSg.addIngressRule(lambdaSg, ec2.Port.tcp(2049), '', false);
    }
}

const app = new App();
const efsStack = new EfsStack(app, 'EfsStack');
const lambdaStack = new LambdaStack(app, 'LambdaStack', {
    vpc: efsStack.vpc,
    efsSg: efsStack.efsSg,
});

By calling the SecurityGroupBase.addIngressRule method with the remoteRule flag true, the IngressRule will be deployed in the scope of the Lambda stack and the deployment will complete successfully.

Changes

Fixed the SecurityGroup Rule configuration part in the Function construct to fix this error.
By changing the Function construct to call the Connections.allowTo method, the remoteRule flag is set to true when allowTo method calls allowFrom method and the EFS Security Group Ingress Rule will be correctly created in the scope of the Lambda stack.

other.connections.remoteRule = true;

other.connections.allowFrom(this, portRange, description);

Closes #18759


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Jan 3, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team January 3, 2024 15:49
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 3, 2024
paulhcsun
paulhcsun previously approved these changes Jan 12, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for detailing out the issue and solution @sakurai-ryo, this will be another helpful win for the community!

Copy link
Contributor

mergify bot commented Jan 12, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 12, 2024
Copy link
Contributor

mergify bot commented Jan 12, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed paulhcsun’s stale review January 13, 2024 01:02

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 478094c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2024
Copy link
Contributor

mergify bot commented Jan 13, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(lambda): EFS and Lambda create cyclic reference when deployed in separate stacks
3 participants