fix(elasticloadbalancingv2): access logging fails on imported bucket #27948
Conversation
github-actions
bot
added
bug
aws-cdk-automation
added
the
pr/needs-maintainer-review
| bucket.addToResourcePolicy(putObjectStatement); | ||
| if (!bucket.policy) { | ||
| // Imported buckets have `autoCreatePolicy` disabled | ||
| bucket.policy = new s3.BucketPolicy(bucket, 'Policy', { bucket }); |
There was a problem hiding this comment.
Afaik this may fail if the imported bucket already has its bucket policy configured outside of cfn, because there can only be one bucket policy for each bucket. IMO we'd better leave the bucket policy as it is, at least for this PR.
There was a problem hiding this comment.
Thanks for the review.
Should we provide users some feedback about it or assume they know how to manage bucket policies?
There was a problem hiding this comment.
Given that there's been no issue related to that, I think we can assume they know it. Additional documents may help, but I found that this behavior is explicitly stated here, which may be already sufficient. I'll leave it to you after all 😄
There was a problem hiding this comment.
Ok, just wanted feedback on this.
I think that the current documentation is clear enough.
There was a problem hiding this comment.
Hi @lpizzinidev thanks for the quick fix for this issue! Just one comment, and also a general one: I would love if you described the issue you are solving in more detail in the pr description. It is quite hard to have to figure out what the actual problem is before looking into the code changes.
| const bucketPolicy = bucket.policy?.node.defaultChild; | ||
| if (lb && bucketPolicy && CfnResource.isCfnResource(lb) && CfnResource.isCfnResource(bucketPolicy)) { | ||
| lb.addDependency(bucketPolicy); | ||
| } |
There was a problem hiding this comment.
we started off with this (before #27558)
this.node.addDependency(bucket);Now, we are (among other changes) only adding the dependency if the policy exists. So I am concerned about the situation where the bucket does not have a policy. In this situation, it seems we used to add this.node.addDependency(bucket), but now skip adding the dependency altogether.
@lpizzinidev is that something you've considered?
There was a problem hiding this comment.
Thanks for the feedback.
If the bucket does not have a policy, then adding a dependency on it would not create a circular dependency.
Are there cases where we want a dependency on the bucket without the policy?
I assume that, if the bucket does not have a policy attached by CDK, the policy must already exist, otherwise logs would not be delivered to S3.
There was a problem hiding this comment.
Gotcha. We were depending on the bucket before but really only cared about depending on the policy. Thats the nuance I was missing.
|
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
removed
the
pr/needs-maintainer-review
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). |
…27948) #27558 fixed a circular dependency issue between the bucket and the ELB when using `logAccessLogs` by adding the dependency at the L1 level. The dependency was added on both the bucket and the bucket policy (which already refers to the bucket). This introduced a new bug when adding a dependency for imported buckets (as `bucket.node.defaultChild` is `undefined`). This PR fixes the issue by adding the dependency at the L1 level on the bucket policy directly. Closes #27928. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
#27558 fixed a circular dependency issue between the bucket and the ELB when using
logAccessLogsby adding the dependency at the L1 level.The dependency was added on both the bucket and the bucket policy (which already refers to the bucket).
This introduced a new bug when adding a dependency for imported buckets (as
bucket.node.defaultChildisundefined).This PR fixes the issue by adding the dependency at the L1 level on the bucket policy directly.
Closes #27928.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license