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

[4naesthetic]

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:

• Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

• This PR adds new construct runtime dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • 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

[gitpod-io]

[HBobertz]

looks good overall. I'd just like to see a minor change to a test

[HBobertz]

good catch

[HBobertz]

For this test can we specify the error instead of just "throws", i.e. using toThrowError() and then specify the exception in the test description here. I think the exception is InvalidEventSelectorsException, but whatever the error we expect is.

[4naesthetic]

Is it just the description you want updated? As far as I understand, exceptions thrown using node.addValidation(...) use a generic throw new Error(...) rather than a custom exception class.

Pull request has been modified.

[HBobertz]

Gotcha I think I misunderstood exactly what toThrow and what toThrowError did. I just wanted to ensure it wouldn't just capture any error, and it would look for that specific one. But it may have been doing that with toThrow anyway. Title is also more descriptive which always helps. Looks good to me

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

[TheRealAmazonKendra]

Mergify's gonna Mergify. Not sure why it's not working right now but I'm re-approving after manually updating.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 5c26a9e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).