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.

Sign up for GitHub

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

Jump to bottom

Allow changing file permissions on rotate #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow changing file permissions on rotate #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c4a82b
Define defaultFileMode variable
lambdanis Oct 22, 2024
d926000
Allow changing file permissions on rotate
lambdanis Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion chown_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import (
var osChown = os.Chown

func chown(name string, info os.FileInfo) error {
f, err := os.OpenFile(name, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, info.Mode())
return chownWithMode(name, info, info.Mode())
}

func chownWithMode(name string, info os.FileInfo, mode os.FileMode) error {
f, err := os.OpenFile(name, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode)
if err != nil {
return err
}
Expand Down
73 changes: 69 additions & 4 deletions linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ func testMaintainMode(t *testing.T, fileMode fs.FileMode) {

filename := logFile(dir)

mode := os.FileMode(0600)
l := &Logger{
Filename: filename,
MaxBackups: 1,
Expand All @@ -27,6 +26,7 @@ func testMaintainMode(t *testing.T, fileMode fs.FileMode) {
}
defer l.Close()

mode := defaultFileMode
// If custom file mode is set then use it.
if l.fileModeIsSet() {
mode = l.FileMode
Expand Down Expand Up @@ -89,7 +89,7 @@ func TestMaintainOwner(t *testing.T) {

filename := logFile(dir)

f, err := os.OpenFile(filename, os.O_CREATE|os.O_RDWR, 0644)
f, err := os.OpenFile(filename, os.O_CREATE|os.O_RDWR, defaultFileMode)
isNil(err, t)
f.Close()

Expand Down Expand Up @@ -121,7 +121,6 @@ func testCompressMaintainMode(t *testing.T, fileMode fs.FileMode) {

filename := logFile(dir)

mode := os.FileMode(0600)
l := &Logger{
Compress: true,
Filename: filename,
Expand All @@ -131,6 +130,7 @@ func testCompressMaintainMode(t *testing.T, fileMode fs.FileMode) {
}
defer l.Close()

mode := defaultFileMode
// If custom file mode is set then use it.
if l.fileModeIsSet() {
mode = l.FileMode
Expand Down Expand Up @@ -199,7 +199,7 @@ func TestCompressMaintainOwner(t *testing.T) {

filename := logFile(dir)

f, err := os.OpenFile(filename, os.O_CREATE|os.O_RDWR, 0644)
f, err := os.OpenFile(filename, os.O_CREATE|os.O_RDWR, defaultFileMode)
isNil(err, t)
f.Close()

Expand Down Expand Up @@ -292,3 +292,68 @@ func TestFileModeIsSet(t *testing.T) {
equals(c.isSet, l.fileModeIsSet(), t)
}
}

func testChangeModeOnRotate(t *testing.T, oldMode fs.FileMode, newMode fs.FileMode) {
currentTime = fakeTime
dir := t.TempDir()
defer os.RemoveAll(dir)

// create logger
filename := logFile(dir)
l := &Logger{
Filename: filename,
MaxBackups: 1,
MaxSize: 100, // megabytes
FileMode: oldMode,
}
defer l.Close()

// create file
mode := defaultFileMode
if l.fileModeIsSet() {
mode = l.FileMode
}
f, err := os.OpenFile(filename, os.O_CREATE|os.O_RDWR, mode)
isNil(err, t)
f.Close()

// change logger file mode
l.FileMode = newMode
Copy link
Member

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

Copy link
Author

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.


// write something to the file
b := []byte("boo!")
n, err := l.Write(b)
isNil(err, t)
equals(len(b), n, t)

// check that file permissions didn't change
info, err := os.Stat(filename)
isNil(err, t)
equals(mode, info.Mode(), t)

// rotate the file
newFakeTime()
err = l.Rotate()
isNil(err, t)

// check permissions for the rotated file
rotatedFilename := backupFile(dir)
rotatedInfo, err := os.Stat(rotatedFilename)
isNil(err, t)
equals(mode, rotatedInfo.Mode(), t)
// check permissions for the new file
expected := oldMode
if l.fileModeIsSet() {
expected = l.FileMode
}
info, err = os.Stat(filename)
isNil(err, t)
equals(expected, info.Mode(), t)
}

func TestChangeModeOnRotate(t *testing.T) {
testChangeModeOnRotate(t, 0600, 0644)
testChangeModeOnRotate(t, 0644, 0600)
testChangeModeOnRotate(t, 0000, 0640)
testChangeModeOnRotate(t, 0640, 0000)
}
15 changes: 9 additions & 6 deletions lumberjack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ var (
// variable so tests can mock it out and not need to write megabytes of data
// to disk.
megabyte = 1024 * 1024

defaultFileMode = os.FileMode(0644)
)

// Write implements io.Writer. If a write would cause the log file to be larger
Expand Down Expand Up @@ -218,24 +220,25 @@ func (l *Logger) openNew() error {
}

name := l.filename()
mode := os.FileMode(0644)

mode := defaultFileMode
if l.fileModeIsSet() {
mode = l.FileMode
}

info, err := osStat(name)
if err == nil {
// Copy the mode off the old logfile.
mode = info.Mode()
// If file mode is not set, use the mode of the existing file.
if !l.fileModeIsSet() {
mode = info.Mode()
}
// move the existing file
newname := backupName(name, l.LocalTime)
if err := os.Rename(name, newname); err != nil {
return fmt.Errorf("can't rename log file: %s", err)
}

// this is a no-op anywhere but linux
if err := chown(name, info); err != nil {
if err := chownWithMode(name, info, mode); err != nil {
return err
}
}
Expand Down Expand Up @@ -288,7 +291,7 @@ func (l *Logger) openExistingOrNew(writeLen int) error {
return l.rotate()
}

file, err := os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0644)
file, err := os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, defaultFileMode)
if err != nil {
// if we fail to open the old log file for some reason, just ignore
// it and open a new log file.
Expand Down