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

(aws-cloudtrail): stack not updated to single-region #17179

Closed
rantoniuk opened this issue Oct 27, 2021 · 6 comments
Closed

(aws-cloudtrail): stack not updated to single-region #17179

rantoniuk opened this issue Oct 27, 2021 · 6 comments
Assignees
Labels
@aws-cdk/aws-cloudtrail Related to AWS CloudTrail bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@rantoniuk
Copy link

What is the problem?

Creating Cloud Trail with default options results in a multi-region trail. Changing the options to single region does not have any effect.

Workaround:

  • remove the trail definition, deploy, re-add with a multiRegion set to false

Reproduction Steps

Create Trail with:

const trail = new Trail(this, 'Trail', { });

and deploy.

Then update the CDK to:

    const trail = new Trail(this, 'Trail', {
      isMultiRegionTrail: false,
    });

and check cdk diff, it will report no diff.

What did you expect to happen?

Trail to be changed to single-region by replacement or options change.

What actually happened?

Nothing.

CDK CLI Version

1.129.0

Framework Version

No response

Node.js Version

12

OS

MacOS

Language

Typescript

Language Version

4.0

Other information

No response

@rantoniuk rantoniuk added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 27, 2021
@github-actions github-actions bot added the @aws-cdk/aws-cloudtrail Related to AWS CloudTrail label Oct 27, 2021
@rantoniuk
Copy link
Author

rantoniuk commented Oct 27, 2021

Another thing:

  • the default stack creates an S3 bucket for writing CloudTrail logs. I think that this bucket should have by default the policy of BLOCK_ALL_PUBLIC_ACCESS but instead I see it has "Objects can be public".
  • when the CloudTrail is removed, the bucked is orphaned and stays there. It would be great to have it cleaned up by default.
  • the created CloudTrail has empty tags - while it should have CloudFormation tags I guess?

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 27, 2021

Does not repro for me:

image

  • You can turn off BPA in your account.
  • Yes, this could be a feature request.
  • Depends on how you added tags, I suppose.

@rantoniuk
Copy link
Author

If BPA is this what you mean then while it would indeed result in the desired behaviour, I don't think that's actually a solution for CDK.
BPA is a platform feature and here we are talking about IaaC, which is a separate way of managing infra as code on top of CloudFormation. I think that CDK should implement AWS best practices and to limit permissions on the created resources by default.

I would even say that turning on BPA is not recommended when using CDK because then CDK does not reflect the actual configuration since: These settings apply account-wide for all current and future buckets.

Depends on how you added tags, I suppose.

I did not add any tags and I should not add any.
I'm talking about CloudFormation tags, that are there by default for S3 bucket, e.g.:

aws:cloudformation:stack-id | arn:aws:cloudformation:eu-west-1:000000:stack/stack/00000-cccc-11eb-95d0-06f8c06e7e67

but they are not there (or they are not visible in the UI) for the created CloudTrail.

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 27, 2021

I don't disagree with you, I'm just saying practically that you can achieve the same effect today if you must. BPA on a per-bucket basis can and should probably be implemented as an Aspect. We don't have this available readily, but you can implement it yourself.

Bucket deletion cannot be implemented as an Aspect, but would be covered by this: aws/aws-cdk-rfcs#25. In the mean time, you are going to have to create a bucket that is configured for proper cleanup yourself and pass it to the trail, rather than rely on the automatically created bucket. In the same way you can set up BPA for the one bucket, rather than implement it as an Aspect.

Tags are an implementation detail of CloudFormation. I do not know why CloudFormation is not tagging the trail appropriately, but unfortunately we can not do anything about that. Get in touch with CloudFormation to report this to them: https://github.com/aws-cloudformation/cloudformation-coverage-roadmap

I cannot reproduce the original issue around isMultiRegionTrail not producing any diffs. It does for me.

I don't think there's anything actionable for us in this issue that is not covered elsewhere, right? And if you want to open a feature request for a BPA Aspect, I feel that should be a separate issue.

Are you alright with me closing this one out?

@rantoniuk
Copy link
Author

Got it, so:

  • for isMultiRegionTrail, I'll try to reproduce it again and comment later if I succeed
  • for Tags of CloudTrail, I will open a CF Support Request

Thanks for feedback, feel free to close it for now.

@rix0rrr rix0rrr closed this as completed Oct 27, 2021
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudtrail Related to AWS CloudTrail bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants