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

Fix uwsgi log file permissions (PP-1667) #2077

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Sep 19, 2024

Description

Updates our docker build to make sure that:

  • /var/log/uwsgi
    • owned by root:adm
  • /var/log/uwsgi/uwsgi.log
    • owned by simplified:adm
    • has 644 permissions, so cloudwatchd can read and upload the files

Motivation and Context

I'm not sure when this got broken, but I realized when trying to work on PP-1667 this morning that we were not geting logs uploaded from staging, and then I went looking and all recently deployed CMs are in the same state.

Since /var/log is mounted as a volume, this will fix things for any new CM, but existing ones will to have their permissions updated. I'm planning to make a follow up PR to do this.

How Has This Been Tested?

  • Ran a docker compose up locally, and checked things have the correct permissions

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen added the bug Something isn't working label Sep 19, 2024
@@ -2,7 +2,7 @@
missingok
daily
create 0660 simplified adm
rotate 30
rotate 13
Copy link
Member Author

Choose a reason for hiding this comment

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

Align this with the retention we have for our other logs

@@ -1,7 +1,6 @@
/var/log/simplified/*.log {
missingok
daily
create 0700 root root
Copy link
Member Author

Choose a reason for hiding this comment

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

Because we use copytruncate this line is actually ignored, so we might as well drop it.

@@ -13,8 +12,7 @@
/var/log/uwsgi/*.log {
missingok
daily
create 0660 simplified adm
rotate 30
rotate 13
Copy link
Member Author

Choose a reason for hiding this comment

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

Align this with the configuration we push out in our playbook.

@@ -1,5 +1,4 @@
[uwsgi]
log-format = [uwsgi] %(var.HTTP_X_FORWARDED_FOR) (%(addr)) - - [%(ltime)] "%(method) %(uri) %(proto)" %(status) %(size) "%(referer)" "%(uagent)" host_hdr=%(host) req_time_elapsed=%(msecs) process=%(pid) worker=%(wid)
logfile-chmod = 644
Copy link
Member Author

Choose a reason for hiding this comment

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

This configuration option isn't used when using logger, its only used when using the log-to configuration. So we might as well drop it, since its deceiving that it is here.

When we switched to using logger in this config is probably when it broke.

@jonathangreen jonathangreen requested a review from a team September 19, 2024 13:39
Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good! 🎸

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.79%. Comparing base (557663c) to head (1f4eb76).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2077   +/-   ##
=======================================
  Coverage   90.79%   90.79%           
=======================================
  Files         344      344           
  Lines       40601    40601           
  Branches     8796     8796           
=======================================
+ Hits        36862    36863    +1     
+ Misses       2485     2484    -1     
  Partials     1254     1254           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathangreen jonathangreen merged commit 1500983 into main Sep 19, 2024
20 checks passed
@jonathangreen jonathangreen deleted the bugfix/uwsgi-log-permissions branch September 19, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants