S3 SqsDestination and SnsDestination creates a cyclic reference

mickog commented 4 years ago

Reproduction Steps

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

const topic = new Topic(stack2, 'Topic');

const bucket = new s3.Bucket(stack1, "bucket");
bucket.addEventNotification(      EventType.OBJECT_CREATED_PUT, new s3n.SnsDestination(topic));

$ cdk synth

What actually happened?

Error: 'stack1' depends on 'stack2' ("stack1/bucket/Notifications/Resource" depends on "stack2/Topic/Resource", "stack1/bucket/Notifications/Resource" depends on "stack2/Topic/Policy/Resource", stack1 -> stack2/Topic/Resource.Ref). Adding this dependency (stack2 -> stack1/bucket/Resource.Arn) would create a cyclic reference.

mickog commented 3 years ago

For reference https://github.com/aws/aws-cdk/issues/5760, the fix may be similar

dreamitsk commented 3 years ago

Apparently there is some kind of what-was-first-chicken-or-egg situation and topic's stack can't leave without the bucket's Custom::S3BucketNotifications resource for some reason. Does anyone know any obvious workaround (except for placing both the bucket and the notification target in the same node) for this or any estimate on a fix (if it is even possible)?

ezeev commented 3 years ago

+1

jmfiola commented 3 years ago

+1

pulkit83 commented 3 years ago

+1

jaspotter commented 3 years ago

+1

awsAmanda commented 3 years ago

I'm adding my issue to this to bump awareness.

I create all of my buckets in a single class, and made each of the buckets an attribute of the class. I imported the MyBuckets class into my main stack, which passed the necessary buckets to the sub stacks. In one of the stacks, I need to add an S3 event trigger onto a Lambda, and that's where I'm getting the cyclic reference. Is there a workaround? I also get the cyclic reference error if I attempt to add the S3 bucket as an event source to the lambda function.

What's odd is I followed the same process for an SQS queue and did not have an issue (create the queue in its own class, pass the object to the constructs/stacks that need it, use it as event source on a Lambda)

`

MyBuckets Class

self.bucket1 = s3.Bucket(self, "Bucket1") self.bucket2 = s3.Bucket(self, "Bucket2")

Main Stack

import MyBuckets

buckets = MyBuckets(...)

subStack = Stack(self, "SubStack", a_bucket=buckets.bucket1)

Sub Stack

def init(..., a_bucket: Bucket)

myLambda = lambda.Function(....) a_bucket.add_event_notification(...) or myLambda.add_event_source(S3EventSource(abucket, .....)) `

sekhavati commented 3 years ago

I'm also hitting this when using SNS as an event destination, any workaround or movement on this issue?

sekhavati commented 3 years ago

So thought I’d share the workaround I’ve put in place as it may help someone else who comes across this thread.

In my situation I was configuring a basic fan-out pattern across stacks as shown below but essentially to fix it I had to create the resources that resulted in a cyclic reference within the same stack.

Before (broken):

Stack 1
- creates an s3 bucket
- exposes s3 bucket as property of stack
Stack 2
- accepts s3 bucket as a prop (ie: from stack 1)
- adds s3 event notification with SNS topic as a destination _<--- ** cyclic reference error _**
- add a lambda subscription to the SNS topic above

After (working):

Stack 1
- creates an s3 bucket
- add s3 event notification with SNS topic as a destination _<--- ** cyclic reference fix _**
- expose SNS topics as property of stack
Stack 2
- accepts SNS topic as a prop (ie: from stack 1)
- add a lambda subscription to the SNS topic above

This works but clouds the separations of concerns for each of the stacks in question in my case.

@SomayaB Do you think this issue is worthy of bumping to a P1 given it’s Lambda counterpart was also a P1?

larsskaug commented 2 years ago

+1

laguna-rotem commented 2 years ago

Same issue, a fix would be great!

owengage commented 2 years ago

Low effort fix: rather than accept the IBucket as a property, accept the bucket name and use Bucket.fromBucketName in the stack requiring the S3 notification. This breaks the cycle.

IsaiahJTurner commented 2 years ago

This issue is due to the fact that addObjectCreatedNotification is trying to modify the policy document of the destination to allow access. Ideally, it would accept a parameter that allows me to explicitly say I do NOT want the policy updated. Since that is not possible currently, you can do this to trick it:

// SQS stack
    const sqsQueue = new sqs.Queue(this, 'Queue', {
      retentionPeriod: Duration.days(14),
      receiveMessageWaitTime: Duration.seconds(20),
      visibilityTimeout: Duration.hours(1),
    })
    sqsQueue.addToResourcePolicy(new iam.PolicyStatement({
      actions: ['sqs:SendMessage'],
      effect: Effect.ALLOW,
      principals: [new ServicePrincipal('s3.amazonaws.com')],
      resources: [sqsQueue.queueArn],
      conditions: {
        StringEquals: {
          "aws:SourceAccount": this.account,
        },
        ArnLike: {
           // Allows all buckets to send notifications since we haven't created the bucket yet.
          "aws:SourceArn": "arn:aws:s3:*:*:*"
        }
      }
    }))
    sqsQueue.addToResourcePolicy(new iam.PolicyStatement({
      actions: ['sqs:SendMessage'],
      effect: Effect.ALLOW,
      principals: [new ServicePrincipal('sns.amazonaws.com')],
      resources: [sqsQueue.queueArn],
      conditions: {
        ArnLike: {
           // Allows all sns topics to send notifications since we haven't created the topic yet.
          "aws:SourceArn": `arn:aws:sns:*:${this.account}:*` 
        }
      }
    }))

// S3 stack
       const bucket = new s3.Bucket(this, 'Bucket', {
      versioned: true,
    })
// Importing the queue tricks the dependency validator and causes it to not try and modify the policy.
    const queue = sqs.Queue.fromQueueArn(this, `Queue`, props.beanstalkPrepStack.sqsQueue.queueArn)
    bucket.addObjectCreatedNotification(new s3n.SqsDestination(queue))

This should provide adequate security for most use cases since it is scoped to the account level but it's not perfect.

gfteix commented 1 year ago

Other 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

// bucket created in stack1

// queue and notification in stack2
queue.addPermission(`AllowS3Invocation`, {
    action: 'sqs:SendMessage',
    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: {
          QueueConfigurations: [
            {
              Events:['s3:ObjectCreated:*'],
              QueueArn: queue.queueArn,
            }
          ]
        }
      },
      physicalResourceId: PhysicalResourceId.of(`${ id + Date.now().toString() }`),
    },
  })

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

MatthewMSaucedo commented 1 year ago

+1 to this, as issue 5760 has been erroneously closed.

knziiy commented 1 year ago

+1

awmcc90 commented 1 year ago

Any update on this? It's still an issue in the latest CDK version.

github-actions[bot] commented 5 months ago

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

scorbiere commented 4 months ago

The usual way to deal with the cross Stack cyclic references issue is to use the "export attribute and import resources" mechanism (see CloudFormation Output and Fn::ImportValue). For now the L2 constructs of Bucket, Topic and so on, don't support this cross Stack setup.
So as a workaround, and to extend the comment from @owengage, here is what I would recommend for now:

import * as s3 from 'aws-cdk-lib/aws-s3';
import * as sns from 'aws-cdk-lib/aws-sns';
import * as cdk from 'aws-cdk-lib';
import * as s3n from 'aws-cdk-lib/aws-s3-notifications';

import { Construct } from 'constructs';

class BucketStack extends cdk.Stack {
  private readonly bucket: s3.Bucket;
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.bucket = new s3.Bucket(this, 'my-bucket');
  }

  /**
   * 
   * @param scope Construct which will interact with the bucket. If the scope's Stack is not the same as the BucketStack,
   * a cross stack reference will be created using the `Stack.exportValue()` method.
   * 
   * @returns the bucket that was created with this stack.
   */
  public getBucket(scope?: Construct): s3.IBucket {
    // return this.bucket; // --> does not support cross stacks scenario

    if (scope === undefined || cdk.Stack.of(scope) === this) {
      return this.bucket;
    } else {
      const exportedBucketName = this.exportValue(this.bucket.bucketName);
      return s3.Bucket.fromBucketName(scope, 'my-imported-bucket', exportedBucketName);
    }
  }
}

class TopicStack extends cdk.Stack {
  private readonly topic: sns.Topic;
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.topic = new sns.Topic(this, 'my-topic');
  }

  /**
   * 
   * @param scope Construct which will interact with the topic. If the scope's Stack is not the same as the TopicStack,
   * a cross stack reference will be created using the `Stack.exportValue()` method.
   * 
   * @returns the topic that was created with this stack.
   */
  public getTopic(scope?: Construct): sns.ITopic {
    // return this.topic; // --> does not support cross stack scenario

    if (scope === undefined || cdk.Stack.of(scope) === this) {
      return this.topic;
    } else {
      const exportedTopicArn = this.exportValue(this.topic.topicArn);
      return sns.Topic.fromTopicArn(scope, 'my-imported-topic', exportedTopicArn);
    }
  }
}

export class S3EventNotifications {
  private readonly bucket: s3.IBucket;
  private readonly topic: sns.ITopic;

  constructor(app: cdk.App) {

    const bucketStack = new BucketStack(app, 'MyBucketStack');
    const topicStack = new TopicStack(app, 'MyTopicStack');

    this.bucket = bucketStack.getBucket(topicStack);
    this.topic = topicStack.getTopic();

    this.bucket.addEventNotification(s3.EventType.OBJECT_CREATED_PUT, new s3n.SnsDestination(this.topic));
  }
}

In this example the responsibility of creating an export/import resource is handled by the stack which own the resource.

Another option would be to move this logic into the S3EventNotifications (from my example) as you may want to keep the control of which resource attribute is exported and independently decide of its usage (with a <RESOURCE>.from<ATTRIBUTE>(...) method). Which will make your code easier to change in the case of a future architecture change/migration (see this discussion for more context: https://github.com/aws/aws-cdk/issues/27420)

Here is the result of the 2 stacks synthesize:

cdk synth MyBucketStack

Resources:
  mybucket15E130AF:
    Type: AWS::S3::Bucket
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
Outputs:
  ExportsOutputRefmybucket15E130AFA0000000:
    Value:
      Ref: mybucket15E130AF
    Export:
      Name: MyBucketStack:ExportsOutputRefmybucket15E130AFA0000000

cdk synth MyTopicStack

Resources:
  mytopicA51900AA:
    Type: AWS::SNS::Topic
  mytopicPolicy0AEB5F49:
    Type: AWS::SNS::TopicPolicy
    Properties:
      PolicyDocument:
        Statement:
          - Action: sns:Publish
            Condition:
              ArnLike:
                aws:SourceArn:
                  Fn::Join:
                    - ""
                    - - "arn:"
                      - Ref: AWS::Partition
                      - ":s3:::"
                      - Fn::ImportValue: MyBucketStack:ExportsOutputRefmybucket15E130AFA0000000
            Effect: Allow
            Principal:
              Service: s3.amazonaws.com
            Resource:
              Ref: mytopicA51900AA
            Sid: "0"
        Version: "2012-10-17"
      Topics:
        - Ref: mytopicA51900AA
  myimportedbucketNotificationsAC5303A0:
    Type: Custom::S3BucketNotifications
    Properties:
      ServiceToken:
        Fn::GetAtt:
          - BucketNotificationsHandler000a0087b7544547bf325f094a3db8347ECC3691
          - Arn
      BucketName:
        Fn::ImportValue: MyBucketStack:ExportsOutputRefmybucket15E130AFA0000000
      NotificationConfiguration:
        TopicConfigurations:
          - Events:
              - s3:ObjectCreated:Put
            TopicArn:
              Ref: mytopicA51900AA
      Managed: false
    DependsOn:
      - mytopicPolicy0AEB5F49
      - mytopicA51900AA
  BucketNotificationsHandler000a0087b7544547bf325f094a3db834RoleB6FB88EC:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              Service: lambda.amazonaws.com
        Version: "2012-10-17"
      ManagedPolicyArns:
        - Fn::Join:
            - ""
            - - "arn:"
              - Ref: AWS::Partition
              - :iam::aws:policy/service-role/AWSLambdaBasicExecutionRole
  BucketNotificationsHandler000a0087b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - s3:GetBucketNotification
              - s3:PutBucketNotification
            Effect: Allow
            Resource: "*"
        Version: "2012-10-17"
      PolicyName: BucketNotificationsHandler000a0087b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36
      Roles:
        - Ref: BucketNotificationsHandler000a0087b7544547bf325f094a3db834RoleB6FB88EC
  BucketNotificationsHandler000a0087b7544547bf325f094a3db8347ECC3691:
    Type: AWS::Lambda::Function
    Properties:
      Description: AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)
      Code: ...

Tested with cdk deploy MyBucketStack MyTopicStack which successfully created the 2 stacks with the associated resources.

This being said, I am interested in cases where this kind of workaround is not possible. Please, feel free to share some examples.

github-actions[bot] commented 2 months ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

aws / aws-cdk