fix(cloudtrail): addS3EventSelector does not expose all options (#1854)

RomainMuller · web-flow · commit 5c3431b08b54 · 2019-02-28T10:00:56.000+01:00
The documentation suggested one could pass an object as the second argument to `CloudTrail.addS3EventSelector`, however the real argument was `ReadWriteType`. Additionally the documentation suggested incorrect uses of the API that would lead to invalid templates being generated. BREAKING CHANGE: The `CloudTrail.addS3EventSelector` accepts an options object instead of only a `ReadWriteType` value. Fixes #1841
diff --git a/packages/@aws-cdk/aws-cloudtrail/README.md b/packages/@aws-cdk/aws-cloudtrail/README.md
@@ -49,10 +49,10 @@ const trail = new cloudtrail.CloudTrail(this, 'MyAmazingCloudTrail');
 
 // Adds an event selector to the bucket magic-bucket.
 // By default, this includes management events and all operations (Read + Write)
-trail.addS3EventSelector(["arn:aws:s3:::magic-bucket/"]); 
+trail.addS3EventSelector(["arn:aws:s3:::magic-bucket/"]);
 
 // Adds an event selector to the bucket foo, with a specific configuration
-trail.addS3EventSelector(["arn:aws:s3:::foo"], { 
+trail.addS3EventSelector(["arn:aws:s3:::foo/"], {
   includeManagementEvents: false,
   readWriteType: ReadWriteType.All,
 });
diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts
@@ -191,19 +191,20 @@ export class CloudTrail extends cdk.Construct {
    *
    * Data events: These events provide insight into the resource operations performed on or within a resource.
    * These are also known as data plane operations.
-   * @param readWriteType the configuration type to log for this data event
-   * Eg, ReadWriteType.ReadOnly will only log "read" events for S3 objects that match a filter)
+   *
+   * @param prefixes the list of object ARN prefixes to include in logging (maximum 250 entries).
+   * @param options the options to configure logging of management and data events.
    */
-  public addS3EventSelector(prefixes: string[], readWriteType: ReadWriteType) {
+  public addS3EventSelector(prefixes: string[], options: AddS3EventSelectorOptions = {}) {
     if (prefixes.length > 250) {
       throw new Error("A maximum of 250 data elements can be in one event selector");
     }
     if (this.eventSelectors.length > 5) {
       throw new Error("A maximum of 5 event selectors are supported per trail.");
     }
     this.eventSelectors.push({
-      includeManagementEvents: false,
-      readWriteType,
+      includeManagementEvents: options.includeManagementEvents,
+      readWriteType: options.readWriteType,
       dataResources: [{
         type: "AWS::S3::Object",
         values: prefixes
@@ -212,6 +213,25 @@ export class CloudTrail extends cdk.Construct {
   }
 }
 
+/**
+ * Options for adding an S3 event selector.
+ */
+export interface AddS3EventSelectorOptions {
+  /**
+   * Specifies whether to log read-only events, write-only events, or all events.
+   *
+   * @default ReadWriteType.All
+   */
+  readWriteType?: ReadWriteType;
+
+  /**
+   * Specifies whether the event selector includes management events for the trail.
+   *
+   * @default true
+   */
+  includeManagementEvents?: boolean;
+}
+
 interface EventSelector {
   readonly includeManagementEvents?: boolean;
   readonly readWriteType?: ReadWriteType;
diff --git a/packages/@aws-cdk/aws-cloudtrail/package.json b/packages/@aws-cdk/aws-cloudtrail/package.json
@@ -35,7 +35,8 @@
     "pkglint": "pkglint -f",
     "package": "cdk-package",
     "awslint": "cdk-awslint",
-    "cfn2ts": "cfn2ts"
+    "cfn2ts": "cfn2ts",
+    "integ": "cdk-integ"
   },
   "cdk-build": {
     "cloudformation": "AWS::CloudTrail"
@@ -58,7 +59,8 @@
     "cdk-build-tools": "^0.24.1",
     "cfn2ts": "^0.24.1",
     "colors": "^1.2.1",
-    "pkglint": "^0.24.1"
+    "pkglint": "^0.24.1",
+    "cdk-integ-tools": "^0.24.1"
   },
   "dependencies": {
     "@aws-cdk/aws-iam": "^0.24.1",
diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.expected.json b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.expected.json
@@ -0,0 +1,107 @@
+{
+  "Resources": {
+    "Bucket83908E77": {
+      "Type": "AWS::S3::Bucket"
+    },
+    "TrailS30071F172": {
+      "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Retain"
+    },
+    "TrailS3PolicyE42170FE": {
+      "Type": "AWS::S3::BucketPolicy",
+      "Properties": {
+        "Bucket": {
+          "Ref": "TrailS30071F172"
+        },
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": "s3:GetBucketAcl",
+              "Effect": "Allow",
+              "Principal": {
+                "Service": "cloudtrail.amazonaws.com"
+              },
+              "Resource": {
+                "Fn::GetAtt": [
+                  "TrailS30071F172",
+                  "Arn"
+                ]
+              }
+            },
+            {
+              "Action": "s3:PutObject",
+              "Condition": {
+                "StringEquals": {
+                  "s3:x-amz-acl": "bucket-owner-full-control"
+                }
+              },
+              "Effect": "Allow",
+              "Principal": {
+                "Service": "cloudtrail.amazonaws.com"
+              },
+              "Resource": {
+                "Fn::Join": [
+                  "",
+                  [
+                    {
+                      "Fn::GetAtt": [
+                        "TrailS30071F172",
+                        "Arn"
+                      ]
+                    },
+                    "/AWSLogs/",
+                    {
+                      "Ref": "AWS::AccountId"
+                    },
+                    "/*"
+                  ]
+                ]
+              }
+            }
+          ],
+          "Version": "2012-10-17"
+        }
+      }
+    },
+    "Trail022F0CF2": {
+      "Type": "AWS::CloudTrail::Trail",
+      "Properties": {
+        "IsLogging": true,
+        "S3BucketName": {
+          "Ref": "TrailS30071F172"
+        },
+        "EnableLogFileValidation": true,
+        "EventSelectors": [
+          {
+            "DataResources": [
+              {
+                "Type": "AWS::S3::Object",
+                "Values": [
+                  {
+                    "Fn::Join": [
+                      "",
+                      [
+                        {
+                          "Fn::GetAtt": [
+                            "Bucket83908E77",
+                            "Arn"
+                          ]
+                        },
+                        "/"
+                      ]
+                    ]
+                  }
+                ]
+              }
+            ]
+          }
+        ],
+        "IncludeGlobalServiceEvents": true,
+        "IsMultiRegionTrail": true
+      },
+      "DependsOn": [
+        "TrailS3PolicyE42170FE"
+      ]
+    }
+  }
+}
diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.ts b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.ts
@@ -0,0 +1,13 @@
+import s3 = require('@aws-cdk/aws-s3');
+import cdk = require('@aws-cdk/cdk');
+import cloudtrail = require('../lib');
+
+const app = new cdk.App();
+const stack = new cdk.Stack(app, 'integ-cloudtrail');
+
+const bucket = new s3.Bucket(stack, 'Bucket', { removalPolicy: cdk.RemovalPolicy.Destroy });
+
+const trail = new cloudtrail.CloudTrail(stack, 'Trail');
+trail.addS3EventSelector([bucket.arnForObjects('')]);
+
+app.run();
diff --git a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts
@@ -119,7 +119,7 @@ export = {
         const stack = getTestStack();
 
         const cloudTrail = new CloudTrail(stack, 'MyAmazingCloudTrail');
-        cloudTrail.addS3EventSelector(["arn:aws:s3:::"], ReadWriteType.All);
+        cloudTrail.addS3EventSelector(["arn:aws:s3:::"]);
 
         expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
         expect(stack).to(haveResource("AWS::S3::Bucket"));
@@ -130,7 +130,33 @@ export = {
         const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
         test.equals(trail.Properties.EventSelectors.length, 1);
         const selector = trail.Properties.EventSelectors[0];
-        test.equals(selector.ReadWriteType, "All", "Expected selector read write type to be All");
+        test.equals(selector.ReadWriteType, null, "Expected selector read write type to be undefined");
+        test.equals(selector.IncludeManagementEvents, null, "Expected management events to be undefined");
+        test.equals(selector.DataResources.length, 1, "Expected there to be one data resource");
+        const dataResource = selector.DataResources[0];
+        test.equals(dataResource.Type, "AWS::S3::Object", "Expected the data resrouce type to be AWS::S3::Object");
+        test.equals(dataResource.Values.length, 1, "Expected there to be one value");
+        test.equals(dataResource.Values[0], "arn:aws:s3:::", "Expected the first type value to be the S3 type");
+        test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
+        test.done();
+      },
+
+      'with hand-specified props'(test: Test) {
+        const stack = getTestStack();
+
+        const cloudTrail = new CloudTrail(stack, 'MyAmazingCloudTrail');
+        cloudTrail.addS3EventSelector(["arn:aws:s3:::"], { includeManagementEvents: false, readWriteType: ReadWriteType.ReadOnly });
+
+        expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
+        expect(stack).to(haveResource("AWS::S3::Bucket"));
+        expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties));
+        expect(stack).to(not(haveResource("AWS::Logs::LogGroup")));
+        expect(stack).to(not(haveResource("AWS::IAM::Role")));
+
+        const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
+        test.equals(trail.Properties.EventSelectors.length, 1);
+        const selector = trail.Properties.EventSelectors[0];
+        test.equals(selector.ReadWriteType, "ReadOnly", "Expected selector read write type to be Read");
         test.equals(selector.IncludeManagementEvents, false, "Expected management events to be false");
         test.equals(selector.DataResources.length, 1, "Expected there to be one data resource");
         const dataResource = selector.DataResources[0];
diff --git a/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.ts b/packages/@aws-cdk/aws-codepipeline/test/integ.lambda-pipeline.ts
@@ -17,7 +17,7 @@ const bucket = new s3.Bucket(stack, 'PipelineBucket', {
 });
 const key = 'key';
 const trail = new cloudtrail.CloudTrail(stack, 'CloudTrail');
-trail.addS3EventSelector([bucket.arnForObjects(key)], cloudtrail.ReadWriteType.WriteOnly);
+trail.addS3EventSelector([bucket.arnForObjects(key)], { readWriteType: cloudtrail.ReadWriteType.WriteOnly, includeManagementEvents: false });
 sourceStage.addAction(new s3.PipelineSourceAction({
   actionName: 'Source',
   outputArtifactName: 'SourceArtifact',
diff --git a/tools/decdk/package.json b/tools/decdk/package.json
@@ -142,4 +142,4 @@
   "engines": {
     "node": ">= 8.10.0"
   }
-}
+}

Original file line number	Diff line number	Diff line change
`@@ -142,4 +142,4 @@`
`142`	`142`	`"engines": {`
`143`	`143`	`"node": ">= 8.10.0"`
`144`	`144`	`}`
`145`		`-}`
	`145`	`+}`