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

fix(s3): bucket notifications in owning stack deletes bucket notifications from other stacks #31091

Merged
merged 8 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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({
Expand Down
40 changes: 40 additions & 0 deletions packages/aws-cdk-lib/aws-s3/test/notification.test.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -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: {
xazhao marked this conversation as resolved.
Show resolved Hide resolved
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();

Expand Down
17 changes: 17 additions & 0 deletions packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->

Expand Down Expand Up @@ -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 -->
15 changes: 15 additions & 0 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> = {
//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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';
Expand Down
3 changes: 1 addition & 2 deletions packages/aws-cdk-lib/cx-api/test/features.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
});

Expand Down