fix(cloudtrail): Trail fails during resource creation due to invalid template properties when management events are 'None' (#23569)

#### Overview 

Currently CDK produces invalid CloudFormation that fails validation by the CloudTrail API upon deployment when setting the `managementEvents` parameter to `ReadWriteType.NONE`.

Although this is a contribution to a stable module, I consider this change to not have breaking changes as the original implementation generates CloudFormation that would result in stack deployment failure so is currently broken.

#### Behaviour changes

*Setting `managementEvents` to `ReadWriteType.NONE`*

**Old behaviour:** Successfully synthesises but produces CloudFormation that fails to deploy.
**New behaviour:** Fails synthesis with a validation error if no additional event selectors are added to the trail, as the default behaviour of CloudTrail is to enable management events if no event selectors are provided. Sets `includeManagementEvents` to `false` by default when new event selectors are added unless overridden explicitly by the user.

#### Other options considered

The previous PR had a suggestion to just always set `includeManagementEvents` to `false` when adding additional event selectors rather than only setting it to `false` when the Trail was created with `ReadWriteType.NONE`. However, this would break backwards compatibility for some scenarios (such as where users don't set `managementEvents` when creating the trail and later add an event selector, as well as a bunch of other esoteric edge cases).

#### Related

Previous PR that was closed for staleness (#16387).

Closes #15488

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

Author: 4naesthetic

packages/@aws-cdk/aws-cloudtrail/lib/cloudtrail.ts

@@ -235,6 +235,7 @@ export class Trail extends Resource {
   public readonly logGroup?: logs.ILogGroup;
 
   private s3bucket: s3.IBucket;
+  private managementEvents: ReadWriteType | undefined;
   private eventSelectors: EventSelector[] = [];
   private topic: sns.ITopic | undefined;
   private insightTypeValues: InsightSelector[] | undefined;
@@ -289,20 +290,14 @@ export class Trail extends Resource {
       }));
     }
 
-    if (props.managementEvents) {
-      let managementEvent;
-      if (props.managementEvents === ReadWriteType.NONE) {
-        managementEvent = {
-          includeManagementEvents: false,
-        };
-      } else {
-        managementEvent = {
-          includeManagementEvents: true,
-          readWriteType: props.managementEvents,
-        };
-      }
-      this.eventSelectors.push(managementEvent);
+    this.managementEvents = props.managementEvents;
+    if (this.managementEvents && this.managementEvents !== ReadWriteType.NONE) {
+      this.eventSelectors.push({
+        includeManagementEvents: true,
+        readWriteType: props.managementEvents,
+      });
     }
+    this.node.addValidation({ validate: () => this.validateEventSelectors() });
 
     if (props.kmsKey && props.encryptionKey) {
       throw new Error('Both kmsKey and encryptionKey must not be specified. Use only encryptionKey');
@@ -373,12 +368,17 @@ export class Trail extends Resource {
       throw new Error('A maximum of 5 event selectors are supported per trail.');
     }
 
+    let includeAllManagementEvents;
+    if (this.managementEvents === ReadWriteType.NONE) {
+      includeAllManagementEvents = false;
+    }
+
     this.eventSelectors.push({
       dataResources: [{
         type: dataResourceType,
         values: dataResourceValues,
       }],
-      includeManagementEvents: options.includeManagementEvents,
+      includeManagementEvents: options.includeManagementEvents ?? includeAllManagementEvents,
       excludeManagementEventSources: options.excludeManagementEventSources,
       readWriteType: options.readWriteType,
     });
@@ -403,7 +403,7 @@ export class Trail extends Resource {
   }
 
   /**
-   * Log all Lamda data events for all lambda functions the account.
+   * Log all Lambda data events for all lambda functions the account.
    * @see https://docs.aws.amazon.com/awscloudtrail/latest/userguide/logging-data-events-with-cloudtrail.html
    * @default false
    */
@@ -451,6 +451,15 @@ export class Trail extends Resource {
   public onCloudTrailEvent(id: string, options: events.OnEventOptions = {}): events.Rule {
     return Trail.onEvent(this, id, options);
   }
+
+  private validateEventSelectors(): string[] {
+    const errors: string[] = [];
+    // Ensure that there is at least one event selector when management events are set to None
+    if (this.managementEvents === ReadWriteType.NONE && this.eventSelectors.length === 0) {
+      errors.push('At least one event selector must be added when management event recording is set to None');
+    }
+    return errors;
+  }
 }
 
 /**

packages/@aws-cdk/aws-cloudtrail/test/cloudtrail.test.ts

@@ -625,19 +625,77 @@ describe('cloudtrail', () => {
         });
       });
 
-      test('managementEvents set to None correctly turns off management events', () => {
+      test('not provided and managementEvents set to None throws missing event selectors error', () => {
         const stack = getTestStack();
 
         new Trail(stack, 'MyAmazingCloudTrail', {
           managementEvents: ReadWriteType.NONE,
         });
 
+        expect(() => {
+          Template.fromStack(stack);
+        }).toThrowError(/At least one event selector must be added when management event recording is set to None/);
+      });
+
+      test('defaults to not include management events when managementEvents set to None', () => {
+        const stack = getTestStack();
+
+        const cloudTrail = new Trail(stack, 'MyAmazingCloudTrail', {
+          managementEvents: ReadWriteType.NONE,
+        });
+
+        const bucket = new s3.Bucket(stack, 'testBucket', { bucketName: 'test-bucket' });
+        cloudTrail.addS3EventSelector([{ bucket }]);
+
         Template.fromStack(stack).hasResourceProperties('AWS::CloudTrail::Trail', {
-          EventSelectors: [
-            {
-              IncludeManagementEvents: false,
-            },
-          ],
+          EventSelectors: [{
+            DataResources: [{
+              Type: 'AWS::S3::Object',
+              Values: [{
+                'Fn::Join': [
+                  '',
+                  [
+                    { 'Fn::GetAtt': ['testBucketDF4D7D1A', 'Arn'] },
+                    '/',
+                  ],
+                ],
+              }],
+            }],
+            IncludeManagementEvents: false,
+          }],
+        });
+      });
+
+      test('includeManagementEvents can be overridden when managementEvents set to None', () => {
+        const stack = getTestStack();
+
+        const cloudTrail = new Trail(stack, 'MyAmazingCloudTrail', {
+          managementEvents: ReadWriteType.NONE,
+        });
+
+        const bucket = new s3.Bucket(stack, 'testBucket', { bucketName: 'test-bucket' });
+        cloudTrail.addS3EventSelector([{ bucket }], {
+          includeManagementEvents: true,
+          readWriteType: ReadWriteType.WRITE_ONLY,
+        });
+
+        Template.fromStack(stack).hasResourceProperties('AWS::CloudTrail::Trail', {
+          EventSelectors: [{
+            DataResources: [{
+              Type: 'AWS::S3::Object',
+              Values: [{
+                'Fn::Join': [
+                  '',
+                  [
+                    { 'Fn::GetAtt': ['testBucketDF4D7D1A', 'Arn'] },
+                    '/',
+                  ],
+                ],
+              }],
+            }],
+            IncludeManagementEvents: true,
+            ReadWriteType: 'WriteOnly',
+          }],
         });
       });
 

packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-data-events-only.js.snapshot/CloudTrailDataEventsOnlyTestDefaultTestDeployAssertA7E52868.assets.json

@@ -0,0 +1,19 @@
+{
+  "version": "22.0.0",
+  "files": {
+    "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
+      "source": {
+        "path": "CloudTrailDataEventsOnlyTestDefaultTestDeployAssertA7E52868.template.json",
+        "packaging": "file"
+      },
+      "destinations": {
+        "current_account-current_region": {
+          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
+          "objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
+          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
+        }
+      }
+    }
+  },
+  "dockerImages": {}
+}

packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-data-events-only.js.snapshot/CloudTrailDataEventsOnlyTestDefaultTestDeployAssertA7E52868.template.json

@@ -0,0 +1,36 @@
+{
+ "Parameters": {
+  "BootstrapVersion": {
+   "Type": "AWS::SSM::Parameter::Value<String>",
+   "Default": "/cdk-bootstrap/hnb659fds/version",
+   "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
+  }
+ },
+ "Rules": {
+  "CheckBootstrapVersion": {
+   "Assertions": [
+    {
+     "Assert": {
+      "Fn::Not": [
+       {
+        "Fn::Contains": [
+         [
+          "1",
+          "2",
+          "3",
+          "4",
+          "5"
+         ],
+         {
+          "Ref": "BootstrapVersion"
+         }
+        ]
+       }
+      ]
+     },
+     "AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
+    }
+   ]
+  }
+ }
+}

packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-data-events-only.js.snapshot/cdk.out

@@ -0,0 +1 @@
+{"version":"22.0.0"}

packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-data-events-only.js.snapshot/integ-cloudtrail-data-events.assets.json

@@ -0,0 +1,19 @@
+{
+  "version": "22.0.0",
+  "files": {
+    "1e43b2272a716f06e79a67fe7810bd64d2c4f198ec606c15bac1ce856e05dbbc": {
+      "source": {
+        "path": "integ-cloudtrail-data-events.template.json",
+        "packaging": "file"
+      },
+      "destinations": {
+        "current_account-current_region": {
+          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
+          "objectKey": "1e43b2272a716f06e79a67fe7810bd64d2c4f198ec606c15bac1ce856e05dbbc.json",
+          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
+        }
+      }
+    }
+  },
+  "dockerImages": {}
+}