fix(cloudtrail): Trail fails during resource creation due to invalid template properties when management events are 'None' #16387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
jaresuth
approved these changes
Sep 28, 2021
jaresuth
approved these changes
Sep 28, 2021
RomainMuller
force-pushed
the
master
branch
from
924c117 to
ebfd5f2
Compare
4naesthetic
changed the title
jaresuth
approved these changes
Sep 30, 2021
peterwoodworth
changed the title
rix0rrr
requested changes
Oct 27, 2021
There was a problem hiding this comment.
I like what you're doing, but I think it's generally simpler to just do the following:
- If
includeManagementEventsneeds to betrue, add an event selector just for the management events. - For all subsequent event selectors that are added, unconditionally set
includeManagementEvents: false.
From what I'm reading here, this should have the exact same effect and is much easier to understand. Wdyt?
rix0rrr
added
bug
|
This PR has been in CHANGES REQUESTED for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
github-actions
bot
added
the
closed-for-staleness
4 tasks
mergify bot
pushed a commit
that referenced
this pull request
Jan 12, 2023
…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*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #15488.
Have done as much as I could to preserve backwards compatibility through this fix. It implements different behaviour than what was originally written which I would appreciate feedback on. The new implementation defaults to adding
includeManagementEvents: falseto event selectors when theTrailis constructed withmanagementEvents: 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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license