Skip to content

fix: access logs bucket policy applied after ALB update#4169

Merged
mergify[bot] merged 2 commits intoaws:mainlinefrom
dannyrandall:fix/access-logs
Nov 11, 2022
Merged

fix: access logs bucket policy applied after ALB update#4169
mergify[bot] merged 2 commits intoaws:mainlinefrom
dannyrandall:fix/access-logs

Conversation

@dannyrandall
Copy link
Copy Markdown
Contributor

Previously, turning on access logs for the ALB would result in the error like:

Access Denied for bucket: app-env-elbaccesslogsbucket-1234. Please check S3bucket permission (Service: AmazonElasticLoadBalancing; Status Code: 400; Error Code: InvalidConfigurationRequest; Request ID: id; Proxy: null)

This is because Cloudformation would update the ALB's settings before creating the bucket policy that allows the ALB to send logs to the bucket.

Now, we add an explicit dependency to the ALB on the bucket policy if access logs is enabled so that the policy is created/applied before attempting to put objects in the bucket.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@dannyrandall dannyrandall requested a review from a team as a code owner November 11, 2022 00:52
@dannyrandall dannyrandall requested review from huanjani and removed request for a team November 11, 2022 00:52
Copy link
Copy Markdown
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 11, 2022

🍕 Here are the new binary sizes!

Name New size (kiB) v1.23.0 size (kiB) Delta (%)
macOS (amd) 47572 47548 +0.05
macOS (arm) 47572 48200 ❤️ -1.30
linux (amd) 41836 41812 +0.06
linux (arm) 41836 41220 🥺 +1.49
windows (amd) 38684 38664 +0.05

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #4169 (fd184e7) into mainline (b956464) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           mainline    #4169   +/-   ##
=========================================
  Coverage     69.24%   69.24%           
=========================================
  Files           250      250           
  Lines         35927    35927           
  Branches        264      264           
=========================================
+ Hits          24876    24878    +2     
+ Misses         9856     9853    -3     
- Partials       1195     1196    +1     
Impacted Files Coverage Δ
...ternal/pkg/deploy/cloudformation/cloudformation.go 73.02% <0.00%> (+0.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mergify mergify Bot merged commit 5845a2a into aws:mainline Nov 11, 2022
@dannyrandall dannyrandall deleted the fix/access-logs branch November 11, 2022 01:49
KollaAdithya pushed a commit to KollaAdithya/copilot-cli that referenced this pull request Nov 21, 2022
Previously, turning on [access logs](https://aws.github.io/copilot-cli/docs/manifest/environment/#http-public-access-logs) for the ALB would result in the error like:
```
Access Denied for bucket: app-env-elbaccesslogsbucket-1234. Please check S3bucket permission (Service: AmazonElasticLoadBalancing; Status Code: 400; Error Code: InvalidConfigurationRequest; Request ID: id; Proxy: null)
```

This is because Cloudformation would update the ALB's settings _before_ creating the bucket policy that allows the ALB to send logs to the bucket.

Now, we add an explicit dependency to the ALB on the bucket policy if access logs is enabled so that the policy is created/applied before attempting to put objects in the bucket. 

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants