aws-cdk/aws-s3: cyclic dependency created when adding bucket inventory config cross-stack in a stage

[cloventt]

Describe the bug

A cyclic dependency is created when:

• a stage is created, with two stacks (A and B)
• a bucket (A) is created in stack A
• another bucket (B) is created in stack B
• bucket B is configured to write inventory reports to bucket A

Expected Behavior

Buckets should be able to add an inventory config referencing another bucket cross-stack.

Current Behavior

Adding a cross-stack bucket inventory config inside a stage creates a cyclic dependency, causing the build to fail.

Reproduction Steps

• create a stage
• create a stack containing a bucket (A) inside the stage
• create another stack containing a bucket (B) inside the stage
• call bucket.addInventory() on bucket B, setting bucket A as the destination

Possible Solution

I think the issue occurs because of this code here:

aws-cdk/packages/aws-cdk-lib/aws-s3/lib/bucket.ts

Lines 2357 to 2372 in b885ece

	if (inventory.destination.bucket instanceof Bucket) {
	inventory.destination.bucket.addToResourcePolicy(new iam.PolicyStatement({
	effect: iam.Effect.ALLOW,
	actions: ['s3:PutObject'],
	resources: [
	inventory.destination.bucket.bucketArn,
	inventory.destination.bucket.arnForObjects(`${inventory.destination.prefix ?? ''}*`),
	],
	principals: [new iam.ServicePrincipal('s3.amazonaws.com')],
	conditions: {
	ArnLike: {
	'aws:SourceArn': this.bucketArn,
	},
	},
	}));
	}

Essentially, the lib is trying to be helpful and add a policy on the destination bucket to make sure the source bucket can write to the destination correctly. However, there is no way to disable this behaviour if for example you want to manage your own bucket policies in the stacks separately. Doing so would allow me to remove the cyclic dependency.

Alternatively, allowing the user to optionally pass a bucket name instead of the entire Bucket object to the s3.InventoryDestination would allow this cycle to be broken.

Additional Information/Context

A workaround is to move the buckets to both be on the same stack.

I found another workaround which was to do something like this:

    // this silly lookup avoids a cyclic dependency between stacks
    const inventoryBucketArn = s3.Bucket.fromBucketArn(this, 'inventory-bucket', inventoryBucket.bucketArn);

    this.bucket.addInventory({
      destination: {
        bucket: inventoryBucketArn,
        bucketOwner: '<account-id>',
      },
      frequency: s3.InventoryFrequency.DAILY,
      includeObjectVersions: s3.InventoryObjectVersion.ALL,
    });

The Bucket.fromBucketArn() static method returns an IBucket which does not trigger the addition of the inline policy on the destination bucket.

CDK CLI Version

2.79.1

Framework Version

No response

Node.js Version

19

OS

Linux

Language

Typescript

Language Version

No response

Other information

No response

[peterwoodworth]

Could add a check in the code you linked to make sure the scope of the Bucket is in the same stack, and throwing an error otherwise telling the user to import the Bucket like you've done in the workaround. Maybe there's a better solution though

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[cloventt]

I was finally able to recreate the issue in a unit test, and additionally I can confirm the problem only occurs if the two stacks are both defined on the same cdk.Stage.

The unit test demonstrating the behaviour is here: https://github.com/cloventt/aws-cdk/blob/b5b0025d00ee7087c291b27399d3530a37f8fc6d/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts#L2868-L2922

The test fails with the following output:

  ● bucket › Cross-stack destination inventory bucket does not introduce cyclic dependency

    'parent-stage/bucket-stack' depends on 'parent-stage/inv-stack' (parent-stage/bucket-stack -> parent-stage/inv-stack/InventoryBucket/Resource.Arn). Adding this dependency (parent-stage/inv-stack -> parent-stage/bucket-stack/MyBucket/Resource.Arn) would create a cyclic reference.

      906 |       }).join(', ');
      907 |       // eslint-disable-next-line max-len
    > 908 |       throw new Error(`'${target.node.path}' depends on '${this.node.path}' (${cycleDescription}). Adding this dependency (${reason.description}) would create a cyclic reference.`);
          |             ^
      909 |     }
      910 |
      911 |     let dep = this._stackDependencies[Names.uniqueId(target)];

      at Stack._addAssemblyDependency (core/lib/stack.ts:908:13)
      at _addAssemblyDependency (core/lib/deps.ts:90:24)
      at operateOnDependency (core/lib/deps.ts:26:3)
      at Stack.addDependency (core/lib/stack.ts:615:18)
      at addDependency (core/lib/private/refs.ts:130:12)
      at resolveValue (core/lib/private/refs.ts:34:24)
      at prepareApp (core/lib/private/prepare-app.ts:30:20)
      at synthesize (core/lib/private/synthesis.ts:45:13)
      at App.synth (core/lib/stage.ts:217:33)
      at Object.synth (aws-s3/test/bucket.test.ts:2908:15)

It may be relevant to note that the cyclic dependency is only created when using a cdk.Stage. For example, the following test setup does not create a cyclic dependency:

    const parentApp = new cdk.App();
    const parentStack = new cdk.Stack(parentApp, 'parent-stage');
    const inventoryStack = new cdk.Stack(parentStack, 'inv-stack');
    const bucketStack = new cdk.Stack(parentStack, 'bucket-stack');

[cloventt]

@peterwoodworth would you be able to re-triage this issue? It might be that this is the intended behaviour when constructing stacks inside stages.

[peterwoodworth]

Thanks @cloventt, I'm finding the same behavior. I hadn't noticed this only occurred on synth when using a stage. It's a bug on our end that we're not throwing this on synth without a Stage (I also tested this out with nested stacks just now, it only seems to throw with a Stage as the parent). If you attempt to deploy what gets synthesized, you'll find that the inv-stack template is invalid, as it attempts to directly reference the Bucket from bucket-stack.

      {
       "Action": "s3:PutObject",
       "Condition": {
        "ArnLike": {
         "aws:SourceArn": {
          "Fn::GetAtt": [
           "MyBucketF68F3FF0", <------------- Doesn't exist in template
           "Arn"
          ]
         }
        }
       },

[peterwoodworth]

Could add a check in the code you linked to make sure the scope of the Bucket is in the same stack, and throwing an error otherwise telling the user to import the Bucket like you've done in the workaround

This is a decent preventative solution for the problem still, but there's this other issue you've brought up that it's not properly generating the cross-stack references for some reason when not using a stage. There's probably elsewhere in the codebase that's affected by this bug

[peterwoodworth]

I created a new tracking issue for the problem I described #25621