Aws-cdk: S3 Bucket Policy Changes Not Recognized As A Change on CDK Deploy

[bensoer]

When making changes to a bucket policy from a pre-existing bucket, applying changes to its Policy are not applied. The CDK seems to act as if no changes are needed

Reproduction Steps

Note that I have changed the names of things in this example to simplify and avoid disclosing

Adding the following code to my application to edit a pre-existing bucket's bucket policy so that other resources may get to it which may or may not have been created with the CDK

const myPreExistingBucket = s3.Bucket.fromBucketName(this, 'MyPreExistingBucket-Lookup-ID', "mypreexistingbucket");
myPreExistingBucket.addToResourcePolicy(new iam.PolicyStatement({
            actions:[
                "s3:*"
            ],
            resources:[
                "arn:aws:s3:::mypreexistingbucket",
                "arn:aws:s3:::mypreexistingbucket/*"
            ],
            principals:[
                new iam.AccountPrincipal("arn:aws:iam::XXXXXXXXXXXX:root")
            ]
        }));

Then deploy with the CDK:
cdk -i --region us-east-1 --app 'npx --quiet ts-node app.ts' deploy --profile datascience

Error Log

Error message is not an error but a false positive in that there are no changes needing to be applied, when there are. Checking the account as well shows no updates in the cloud formation templates and the Bucket Policy not being applied to the Bucket

Environment

• CLI Version : Attempted with v1.26.0 and v1.18.0
• Framework Version: Nodejs - v12.16.1, NPM - v6.13.4
• OS : MAC OS Mojave
• Language : Typescript - v3.7.4

Other

---

This is :bug: Bug Report

[iliapolo]

Hi @bensoer

This is actually the desired behavior. Imported/Existing resources are not strictly speaking a part of the CloudFormation stack. Notice that if you simply do:

s3.Bucket.fromBucketName(this, 'MyPreExistingBucket-Lookup-ID', 
  "mypreexistingbucket");

you will get an empty stack. Existing resources can be used for referencing by other stack resources, but they cannot be mutated. Since this might break its previous, unmanaged consumers. Having said that, we acknowledge the use case for this, and actually CloudFormation have added support for explicitly importing existing resources, but this isn't yet supported in the CDK.

There are ways around this, like using the AwsCustomResource construct.

We encourage you to open a feature request that details your use case :)

[BenoitDel]

If i am not mistaken, the ressource mutation fails silently. IMHO this should trigger en error message

[iliapolo]

@BenoitDel You're right. Actually, the correct behavior would be to not even expose the addToResourcePolicy method in this case.

However, i want to make a small correction to a statement i previously made:

Existing resources can be used for referencing by other stack resources, but they cannot be mutated.

Thats not always the case, it actually depends on CloudFormation capabilities, but in principal, CDK does not enforce this (i.e, some imported resources can be updated).

So in reality, we should either properly support this scenario (in which case, this is just a bug), or, if this scenario is not supported, we should hide the addToResourcePolicy method altogether.

Thanks for reporting this, we will take a look.

[skinny85]

So in reality, we should either properly support this scenario (in which case, this is just a bug), or, if this scenario is not supported, we should hide the addToResourcePolicy method altogether.

Remember that a Bucket is also an IBucket, which means just having an IBucket, we don't know whether it's mutable or not. In many cases, it makes sense to have a method like addToResourcePolicy on IBucket, even if an imported bucket doesn't support it. One of the patterns we use is to return a boolean value indicating whether a change was actually made.

This will become even more important, as we move to our improved CfnInclude experience (#7304), and we will want to have an L1-to-L2 transition, which will result in mutable IBuckets.

[iliapolo]

@skinny85 Do you think it makes sense to specifically have addToResourcePolicy on IBucket at the moment? even though always returns statementAdded: false?

[skinny85]

@skinny85 Do you think it makes sense to specifically have addToResourcePolicy on IBucket at the moment? even though always returns statementAdded: false?

Yes. Not sure what you mean by "it always returns statementAdded: false"...? It doesn't: https://github.com/aws/aws-cdk/blob/f5dbed6c2615cff4c7c868d5f7073e42665e07ca/packages/%40aws-cdk/aws-s3/lib/bucket.ts#L459-L459

[iliapolo]

@skinny85 Notice that the full code reads:

https://github.com/aws/aws-cdk/blob/745922aa5a5a0195869830b54d7e529bec83e37c/packages/%40aws-cdk/aws-s3/lib/bucket.ts#L452-L462

When the bucket is an Import, policy is undefined, and autoCreatePolicy is always false:

https://github.com/aws/aws-cdk/blob/745922aa5a5a0195869830b54d7e529bec83e37c/packages/%40aws-cdk/aws-s3/lib/bucket.ts#L1155-L1156

So we end up here:

https://github.com/aws/aws-cdk/blob/745922aa5a5a0195869830b54d7e529bec83e37c/packages/%40aws-cdk/aws-s3/lib/bucket.ts#L462

[skinny85]

That is true that today, but might not be in the future. For example, we might have a a fromCfnBucket(): IBucket in the future (https://github.com/aws/aws-cdk/issues/9719) that returns a mutable implementation of IBucket.

[philhorrocks]

just experienced this issue. An error would be nice as cdk fails silently :(