Skip to content

fix(startup): only modifying logrotate configs when they are writable#266

Merged
bokysan merged 3 commits into
bokysan:masterfrom
outofrange:fix-263/logrotate-config-template
Mar 5, 2026
Merged

fix(startup): only modifying logrotate configs when they are writable#266
bokysan merged 3 commits into
bokysan:masterfrom
outofrange:fix-263/logrotate-config-template

Conversation

@outofrange
Copy link
Copy Markdown
Contributor

@outofrange outofrange commented Feb 24, 2026

logrotate.conf might be mounted from a Kubernetes ConfigMap, in which case

  • /etc/logrotate.d/logrotate.conf
  • inline sed replacements for removing duplicate mail.log lines are skipped
  • responsibility for correctness is with user

For consistency, I added the same check for /etc/logrotate.d/rsyslog.

Fixes #263

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Feb 24, 2026

Need to test this before marking as ready for review, but this PR should take care of read-only mounted logrotate.confs.

@outofrange outofrange marked this pull request as ready for review February 24, 2026 17:05
@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Feb 24, 2026

Tested PR with locally built image in our Kubernetes environment, where it was crashing on logrotate.conf modifications before - now it's working!

@outofrange outofrange mentioned this pull request Feb 24, 2026
Closed
@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Feb 24, 2026

Another approach would be to mount the configmap logrotate.conf to something like /etc/logrotate.d/logrotate.conf.tpl, and then cp /etc/logrotate.d/logrotate.conf.tpl /etc/logrotate.d/logrotate.conf on container start to get a modifyable config.
But as far as I can tell, the sed duplication fix is the only place where it's modified, is it?

@bokysan what do you think/prefer?

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Feb 24, 2026 •
edited
Loading

Also, my unit test is named 029_logrotate_remove_duplicate_mail_log.bats, but #265 would already use 029_postfix_set_hostname.bats - should I use 030_...?

Edit: since #265 is now merged, I rebased this PR and updated unit test number prefix to 030_...

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Mar 3, 2026 •
edited
Loading

As a workaround, I also tried to just override functions.sh through a ConfigMap subPath mount using the Helm chart - but then it failed at chmod +x /scripts/*.sh (run.sh), because ConfigMap mounts are read-only.

I'm wondering if this is actually necessary?
As far as I see, those files already get their executable bit set during image creation: Dockerfile

@outofrange
fix(startup): only modifying logrotate configs when they are writable
36ed0f3 
logrotate.conf might be mounted from a Kubernetes ConfigMap, in which case
- /etc/logrotate.d/logrotate.conf
- inline sed replacements for removing duplicate mail.log lines are skipped
- responsibility for correctness is with user
@outofrange
test(logrotate): adding bats unit tests for logrotate replacement fun…
50ffc1f 
…ctions
@outofrange
chore(test): renumbered logrotate unit test
e016044
@outofrange outofrange force-pushed the fix-263/logrotate-config-template branch from 0ae0dd2 to e016044 Compare March 5, 2026 08:58
@bokysan
Copy link
Copy Markdown
Owner

bokysan commented Mar 5, 2026

Hi, sorry for the late reply -- I will check out your commit ASAP.

@bokysan
Copy link
Copy Markdown
Owner

bokysan commented Mar 5, 2026

Does it work on both Alpine and Debian? If so I think we can merge it.

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Mar 5, 2026

We're currently running the Debian based image built from this MR in our infra - I'll quickly test with Alpine as well!

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Mar 5, 2026

Yes, both Alpine and Debian based images work!

@bokysan bokysan merged commit 7cb845c into bokysan:master Mar 5, 2026
5 checks passed
@bokysan
Copy link
Copy Markdown
Owner

bokysan commented Mar 5, 2026

Merged. Let's see how it pans out.

Thank you so much for all the great work!

@outofrange
Copy link
Copy Markdown
Contributor Author

outofrange commented Mar 5, 2026

Can you trigger a 5.1.1 release for this, or do you want to wait for other features as well?
Would be happy to use the official image again as soon as possible ;)

@outofrange outofrange deleted the fix-263/logrotate-config-template branch March 5, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

logrotate device or resource busy

2 participants

@outofrange @bokysan