fix(aws-s3): log delivery may be incorrectly configured when target bucket is imported #23552
Conversation
|
|
github-actions
bot
added
admired-contributor
github-actions
bot
added
bug
laurelmay
changed the title
|
Edit: Done |
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
|
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). |
|
@Mergifyio update |
❌ Base branch update has failedrefusing to allow a GitHub App to create or update workflow |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
I am waiting for this fix release.Looks like this fix didn't make it to v2.60.0 [https://github.com/aws/aws-cdk/releases/tag/v2.60.0].Any idea which release of CDK version this fix would be in?. |
|
It was merged in master... so probably in the next one v2.61.0 |
|
I wonder if release automation didn't pick it up because it's not following the convention |
|
I take that back... it literally happened 9hrs after they cut the v2.60.0 release. However... I hope it ends up in the release changelog section (see my previous comment on following the naming convention). In any case, @kylelaker thanks for fixing this. 👍 |
Ahh rats! I totally missed getting the convention wrong (too used to using This might be a decent addition to |
The AWS CDK uses conventional commits which encourage a scope. In nearly all cases, contributors omit the `aws-` prefix; though there are some situations where they're included (such as aws#23552, aws#23225, and others). Interestingly, it seems that most often the `aws-` prefix is used for chore/doc commits that don't end up in changelogs anyway. This actually results in inconsistencies for users in the changelog. Because the changelog sorts entries alphabetically by scope, changes that were contributed with `aws-s3` as the scope are listed at the top of the changelog whereas changes that just used `s3` are sorted further down. This means it's harder for users to review one (or I suppose 2 with Feature/Fix being separate sections) spots in the changelog to identify specific modules they care about. It's also possible that rather than potentially annoying contributors who have to edit their titles, this should be automatically fixed up in the changelog generation. The current behavior may also accidentally encourage users to scan the whole changelog rather than just a few bits of it. So it's very understandable if the potential inconvenience from this change it's worth the minor formatting consistency gain.
The AWS CDK uses conventional commits which encourage a scope. In nearly all cases, contributors omit the `aws-` prefix; though there are some situations where they're included (such as aws#23552, aws#23225, and others). Interestingly, it seems that most often the `aws-` prefix is used for chore/doc commits that don't end up in changelogs anyway. This actually results in inconsistencies for users in the changelog. Because the changelog sorts entries alphabetically by scope, changes that were contributed with `aws-s3` as the scope are listed at the top of the changelog whereas changes that just used `s3` are sorted further down. This means it's harder for users to review one (or I suppose 2 with Feature/Fix being separate sections) spots in the changelog to identify specific modules they care about. It's also possible that rather than potentially annoying contributors who have to edit their titles, this should be automatically fixed up in the changelog generation. The current behavior may also accidentally encourage users to scan the whole changelog rather than just a few bits of it. So it's very understandable if the potential inconvenience from this change it's worth the minor formatting consistency gain.
The AWS CDK uses conventional commits which encourage a scope. In nearly all cases, contributors omit the `aws-` prefix; though there are some situations where they're included (such as #23552, #23225, and others). Interestingly, it seems that most often the `aws-` prefix is used for chore/doc commits that don't end up in changelogs anyway. This actually results in inconsistencies for users in the changelog. Because the changelog sorts entries alphabetically by scope, changes that were contributed with `aws-s3` as the scope are listed at the top of the changelog whereas changes that just used `s3` are sorted further down. This means it's harder for users to review one (or I suppose 2 with Feature/Fix being separate sections) spots in the changelog to identify specific modules they care about. It's also possible that rather than potentially annoying contributors who have to edit their titles, this should be automatically fixed up in the changelog generation. The current behavior may also accidentally encourage users to scan the whole changelog rather than just a few bits of it. So it's very understandable if the potential inconvenience from this change it's worth the minor formatting consistency gain. ---- ### 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
|
Thanks @kylelaker that fix made it to v2.61.0, which resolved the s3 issue noticed. |
This resolves issues with log delivery for situations where the target bucket for S3 Access Logs is in another stack. This situation was previously missing from unit and integration tests. This adds additional checks to make sure that this behavior works. There are two primary issues here:
@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy, if the target bucket was imported (not a concreteBucket), the grant would be applied to the source bucket for logs improperly. This resulted in the ACL or Bucket Policy being added to the wrong stack.@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicywas enabled the created bucket policy resulted in a cyclical dependency because of the conditions; these conditions are not necessary for successful delivery. This omits them unless the bucket is concrete and in the same stack.Unit tests and integration tests now cover importing a bucket between stacks.
Closes: #23547
Closes: #23588
All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license