-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow changing file permissions on rotate #5
base: master
Are you sure you want to change the base?
Conversation
Just to make it clearer what's the default. Also use it in tests. Signed-off-by: Anna Kapuscinska <[email protected]>
Since 4d704c9 lumberjack allows to set the permission bits on log files via Logger.FileMode field. However, if the file already existed then its permissions were preserved even after log rotation or when a new Logger was created, so the FileMode field was effectively ignored. This commit changes that behaviour. If FileMode field is set to a different value than permissions on the existing file, then file permissions are not changed immediately, but after log rotation the new file has new permissions. This allows to change permissions on the log file programatically, without a need for manual operation. Signed-off-by: Anna Kapuscinska <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor question, but looks fine
f.Close() | ||
|
||
// change logger file mode | ||
l.FileMode = newMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you set this? I would have expected a simple test, you write a file with default mode, no need to log to it, then you create the new Logger with corresponding FileMode: newMode, you trigger a write which should trigger a rotate, then at least: equals(newMode, info.Mode(), t)
? am I missing something obvious?
On another note what happens to the main tetragon.log file, do we explicitly chown that file with new mode or not? I can't recall. If not while you are it please ensure if the main non-rotated file exist then we do a proper chown on it using the permission passed from tetragon so we can do 0644<=>0600
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added tests for four transitions (L355-358): default->640, 640->default (this one is a no-op), 600->644 and 644->600. I wanted to check that the permissions change doesn't happen to early (e.g. on write), only on rotate, that's why I first change the logger permissions then write to the file. Does it make sense?
On another note what happens to the main tetragon.log file, do we explicitly chown that file with new mode or not? I can't recall.
Yes, we explicitly chown in openNew()
. So if the non-rotated file exists then we do a proper chown on it using given permissions when it gets rotated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright much appreciated
Since 4d704c9 lumberjack allows to set the permission bits on log files
via Logger.FileMode field. However, if the file already existed then its
permissions were preserved even after log rotation or when a new Logger was
created, so the FileMode field was effectively ignored.
This commit changes that behaviour. If FileMode field is set to a different
value than permissions on the existing file, then file permissions are not
changed immediately, but after log rotation the new file has new permissions.
This allows to change permissions on the log file programatically, without
a need for manual operation.