fix(s3): bucket notifications in owning stack deletes bucket notifica…

…tions from other stacks (#31091)

### Issue # (if applicable)

Closes #30607.

### Reason for this change

There's a bug reported in the Github issue that bucket notifications in owing stack will remove all notifications added in imported stack. This is because we treated the bucket as `managed` hence we use bucket notifications in that stack as source of truth.

In the `unmanaged` path, we already filtered out external notifications it should handle both scenarios when the bucket is managed or unmanaged.

### Description of changes

Always set `Managed` property to false when the feature flag is enabled. Here we introduce a feature flag to prevent it breaking current customers. 

### Description of how you validated changes

Added unit tests. Integrations test can't validate this change because we need to deploy twice to actually see the change.
Also tested manually.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts

@@ -2,6 +2,7 @@ import { Construct, IConstruct } from 'constructs';
 import { NotificationsResourceHandler } from './notifications-resource-handler';
 import * as iam from '../../../aws-iam';
 import * as cdk from '../../../core';
+import * as cxapi from '../../../cx-api';
 import { Bucket, IBucket, EventType, NotificationKeyFilter } from '../bucket';
 import { BucketNotificationDestinationType, IBucketNotificationDestination } from '../destination';
 
@@ -124,7 +125,14 @@ export class BucketNotifications extends Construct {
         role: this.handlerRole,
       });
 
-      const managed = this.bucket instanceof Bucket;
+      let managed = this.bucket instanceof Bucket;
+
+      // We should treat buckets as unmanaged because it will not remove notifications added somewhere else
+      // Ading a feature flag to prevent it brings unexpected changes to customers
+      // Put it here because we still need to create the permission if it's unmanaged bucket.
+      if (cdk.FeatureFlags.of(this).isEnabled(cxapi.S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET)) {
+        managed = false;
+      }
 
       if (!managed) {
         handler.addToRolePolicy(new iam.PolicyStatement({

packages/aws-cdk-lib/aws-s3/test/notification.test.ts

@@ -1,6 +1,7 @@
 import { Match, Template } from '../../assertions';
 import * as iam from '../../aws-iam';
 import * as cdk from '../../core';
+import * as cxapi from '../../cx-api';
 import * as s3 from '../lib';
 
 describe('notification', () => {
@@ -226,6 +227,45 @@ describe('notification', () => {
       },
     });
   });
+
+  test('Notification custom resource uses always treat bucket as unmanaged', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    stack.node.setContext(cxapi.S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET, true);
+
+    // WHEN
+    new s3.Bucket(stack, 'MyBucket', {
+      eventBridgeEnabled: true,
+    });
+
+    // THEN
+    Template.fromStack(stack).resourceCountIs('AWS::S3::Bucket', 1);
+    Template.fromStack(stack).hasResourceProperties('Custom::S3BucketNotifications', {
+      NotificationConfiguration: {
+        EventBridgeConfiguration: {},
+      },
+      Managed: false,
+    });
+    Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
+      PolicyDocument: {
+        Statement: [
+          {
+            Action: 's3:PutBucketNotification',
+            Effect: 'Allow',
+            Resource: '*',
+          },
+          {
+            Action: 's3:GetBucketNotification',
+            Effect: 'Allow',
+            Resource: '*',
+          },
+        ],
+        Version: '2012-10-17',
+      },
+    });
+  });
+
   test('check notifications handler runtime version', () => {
     const stack = new cdk.Stack();
 

packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md

@@ -71,6 +71,7 @@ Flags come in three types:
 | [@aws-cdk/pipelines:reduceAssetRoleTrustScope](#aws-cdkpipelinesreduceassetroletrustscope) | Remove the root account principal from PipelineAssetsFileRole trust policy | 2.141.0 | (default) |
 | [@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm](#aws-cdkaws-ecsremovedefaultdeploymentalarm) | When enabled, remove default deployment alarm settings | 2.143.0 | (default) |
 | [@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault](#aws-cdkcustom-resourceslogapiresponsedatapropertytruedefault) | When enabled, the custom resource used for `AwsCustomResource` will configure the `logApiResponseData` property as true by default | 2.145.0 | (fix) |
+| [@aws-cdk/aws-s3:keepNotificationInImportedBucket](#aws-cdkaws-s3keepnotificationinimportedbucket) | When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack. | V2NEXT | (fix) |
 
 <!-- END table -->
 
@@ -1338,4 +1339,20 @@ property from the event object.
 | 2.145.0 | `false` | `false` |
 
 
+### @aws-cdk/aws-s3:keepNotificationInImportedBucket
+
+*When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack.* (fix)
+
+Currently, adding notifications to a bucket where it was created by ourselves will override notification added where it is imported.
+
+When this feature flag is enabled, adding notifications to a bucket in the current stack will only update notification defined in this stack.
+Other notifications that are not managed by this stack will be kept.
+
+
+| Since | Default | Recommended |
+| ----- | ----- | ----- |
+| (not in v1) |  |  |
+| V2NEXT | `false` | `false` |
+
+
 <!-- END details -->

packages/aws-cdk-lib/cx-api/lib/features.ts

@@ -105,6 +105,7 @@ export const EKS_NODEGROUP_NAME = '@aws-cdk/aws-eks:nodegroupNameAttribute';
 export const EBS_DEFAULT_GP3 = '@aws-cdk/aws-ec2:ebsDefaultGp3Volume';
 export const ECS_REMOVE_DEFAULT_DEPLOYMENT_ALARM = '@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm';
 export const LOG_API_RESPONSE_DATA_PROPERTY_TRUE_DEFAULT = '@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault';
+export const S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET = '@aws-cdk/aws-s3:keepNotificationInImportedBucket';
 
 export const FLAGS: Record<string, FlagInfo> = {
   //////////////////////////////////////////////////////////////////////
@@ -1092,6 +1093,20 @@ export const FLAGS: Record<string, FlagInfo> = {
     introducedIn: { v2: '2.145.0' },
     recommendedValue: false,
   },
+
+  //////////////////////////////////////////////////////////////////////
+  [S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET]: {
+    type: FlagType.BugFix,
+    summary: 'When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack.',
+    detailsMd: `
+      Currently, adding notifications to a bucket where it was created by ourselves will override notification added where it is imported.
+
+      When this feature flag is enabled, adding notifications to a bucket in the current stack will only update notification defined in this stack.
+      Other notifications that are not managed by this stack will be kept.
+    `,
+    introducedIn: { v2: 'V2NEXT' },
+    recommendedValue: false,
+  },
 };
 
 const CURRENT_MV = 'v2';

packages/aws-cdk-lib/cx-api/test/features.test.ts

@@ -37,9 +37,8 @@ test('feature flag defaults may not be changed anymore', () => {
     [feats.EFS_DEFAULT_ENCRYPTION_AT_REST]: true,
     [feats.LAMBDA_RECOGNIZE_VERSION_PROPS]: true,
     [feats.CLOUDFRONT_DEFAULT_SECURITY_POLICY_TLS_V1_2_2021]: true,
-    // Add new disabling feature flags below this line
     [feats.PIPELINE_REDUCE_ASSET_ROLE_TRUST_SCOPE]: true,
-
+    // Add new disabling feature flags below this line
   });
 });