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

S3 LambdaDestination creates a cyclic reference #5760

Closed
rinfield opened this issue Jan 11, 2020 · 17 comments · Fixed by #10426
Closed

S3 LambdaDestination creates a cyclic reference #5760

rinfield opened this issue Jan 11, 2020 · 17 comments · Fixed by #10426
Assignees
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. cross-stack Related to cross-stack resource sharing effort/medium Medium work item – several days of effort in-progress This issue is being actively worked on. p1

Comments

@rinfield
Copy link

rinfield commented Jan 11, 2020

Setting S3 Bucket notification to a Lambda function in the other stack creates a cyclic reference.

Reproduction Steps

import * as cdk from "@aws-cdk/core";
import * as lambda from "@aws-cdk/aws-lambda";
import * as s3 from "@aws-cdk/aws-s3";
import * as s3n from "@aws-cdk/aws-s3-notifications";

const app = new cdk.App();
const stack1 = new cdk.Stack(app, "stack1");
const stack2 = new cdk.Stack(app, "stack2");

const lambdaFunction = new lambda.Function(stack1, "lambdaFunction", {
  code: lambda.Code.fromInline("whatever"),
  handler: "index.handler",
  runtime: lambda.Runtime.NODEJS_10_X
});

const bucket = new s3.Bucket(stack2, "bucket");
bucket.addObjectCreatedNotification(new s3n.LambdaDestination(lambdaFunction));
$ cdk synth

Error Log

/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/stack.ts:639
        throw new Error(`'${target.node.path}' depends on '${this.node.path}' (${cycle.join(', ')}). Adding this dependency (${reason}) would create a cyclic reference.`);
              ^
Error: 'stack2' depends on 'stack1' (stack2 -> stack1/lambdaFunction/Resource.Arn, "stack2/bucket/Notifications/Resource" depends on "stack1/lambdaFunction/AllowBucketNotificationsFromstack2bucketA161360C"). Adding this dependency (stack1 -> stack2/bucket/Resource.Arn) would create a cyclic reference.
    at Stack._addAssemblyDependency (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/stack.ts:639:15)
    at Object.addDependency (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/deps.ts:44:20)
    at Stack.addDependency (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/stack.ts:343:5)
    at Stack.prepareCrossReference (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/stack.ts:906:24)
    at Stack.prepare (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/stack.ts:749:37)
    at Function.prepare (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/construct.ts:81:28)
    at Function.synth (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/construct.ts:40:10)
    at App.synth (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/app.ts:141:36)
    at process.<anonymous> (/workspaces/cdk-sample/node_modules/@aws-cdk/core/lib/app.ts:120:45)
    at Object.onceWrapper (events.js:300:26)

Environment

  • CLI Version : 1.20.0
  • Framework Version: 1.20.0
  • OS : All
  • Language : TypeScript

Other

The resources for the notification seem to have been attached to the stack for Lambda Function and it is causing cyclic reference. Instead, it should be attached to the stack for S3 Bucket.

Passing explicit scope at fn.addPermission() would solve the problem, but I'm not sure it is an appropriate fix or not.


This is 🐛 Bug Report

@rinfield rinfield added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 11, 2020
@SomayaB SomayaB added cross-stack Related to cross-stack resource sharing @aws-cdk/aws-s3 Related to Amazon S3 labels Jan 13, 2020
@eladb eladb added the p1 label Jan 14, 2020
@SomayaB
Copy link
Contributor

SomayaB commented Jan 15, 2020

Hi @rinfield, thanks for reporting this. We will update this issue when there is movement.

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jan 15, 2020
@eladb eladb assigned iliapolo and unassigned eladb Jan 22, 2020
@muellerk22
Copy link

I'm having the same issue. Is there a work around?

@patbassing
Copy link

I too am having this issue. I haven't been able to find any work around, other than creating bucket and function in the same stack.
+1 from me.

@civilizeddev
Copy link
Contributor

+1
I'm having the same issue when using SqsDestination.

@aleskozina
Copy link

+1
same here

@iliapolo iliapolo added the effort/medium Medium work item – several days of effort label Aug 5, 2020
@comsky
Copy link

comsky commented Sep 14, 2020

+1
same here
CDK 1.62

@aleskozina
Copy link

anyone tested if this happens in nestedStack too?

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Sep 18, 2020
@mergify mergify bot closed this as completed in #10426 Sep 21, 2020
mergify bot pushed a commit that referenced this issue Sep 21, 2020
…cy when bucket and lambda are in different stacks (#10426)

When the bucket and function are in two different stacks, the following stacks are created:

### Bucket Stack

- `s3.Bucket`
- `s3.BucketNotificationHandler` (creates a dependency on **lambda stack** since it configures the target of the trigger)

### Lambda Stack

- `lambda.Function`
- `lambda.Permission` (creates a dependency on the **bucket stack** since it configures the lambda to allow invocations from that specific bucket)

The solution is to switch up the `lambda.Permission` scope and use the bucket instead of the function, so that it is added to the bucket stack, leaving the lambda stack independent.

Fixes #5760

----

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

jmfiola commented Apr 23, 2021

+1

@jappyjan
Copy link

same here

@miekassu
Copy link

Looks like this is still an issue!

Cyclic dependency error as described in issue description.
CDK Version 2.8.0.

This relates to #11245.
I did workaround using this solution #11245 (comment)

@ShivamJoker
Copy link

Hello any updates? why is it closed?

@gfteix
Copy link

gfteix commented Nov 29, 2022

One possible workaround is to use AwsCustomResource. You basically specify an AWS SDK call to be executed onCreate, onUpdate or onDelete - internally this creates a Lambda that will do the real work:
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources-readme.html#custom-resources-for-aws-apis

Note: This replaces the existing notification configuration with the configuration you include in the parameter. Check: https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketNotificationConfiguration.html

lambda.addPermission(`AllowS3Invocation`, {
    action: 'lambda:InvokeFunction',
    principal: new ServicePrincipal('s3.amazonaws.com'),
    sourceArn: bucket.bucketArn
  })

  const notificationResource = new AwsCustomResource(this, `NotificationCustomResource`, {
    logRetention: RetentionDays.THREE_DAYS,
    policy: AwsCustomResourcePolicy.fromStatements([
      new PolicyStatement({
        effect: Effect.ALLOW,
        actions: ['s3:PutBucketNotification'],
        resources: [bucket.bucketArn, `${ bucket.bucketArn }/*`],
      })
    ]),
    onCreate: {
      service: 'S3',
      action: 'putBucketNotificationConfiguration',
      parameters: {
        Bucket: bucket.bucketName,
        NotificationConfiguration: {
          LambdaFunctionConfigurations: [
            {
              Events:['s3:ObjectCreated:*'],
              LambdaFunctionArn: lambda.functionArn,
            }
          ]
        }
      },
      physicalResourceId: PhysicalResourceId.of(`${ id + Date.now().toString() }`),
    },
  })

  notificationResource.node.addDependency(lambda.permissionsNode.findChild('AllowS3Invocation')

@MatthewMSaucedo
Copy link

+1 - this is still an issue.

@Brody8TY
Copy link

+1 - still an issue

@kellan-cartledge
Copy link

+1 - this is still an issue.

@HaydenFaulkner
Copy link

+1

1 similar comment
@tejad
Copy link

tejad commented Aug 5, 2024

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. cross-stack Related to cross-stack resource sharing effort/medium Medium work item – several days of effort in-progress This issue is being actively worked on. p1
Projects
None yet