From 937a414fd0f30dcc77d95479e07f69c45fa9fc93 Mon Sep 17 00:00:00 2001
From: Hans Hasselberg <me@hans.io>
Date: Wed, 18 Dec 2019 22:31:22 +0100
Subject: [PATCH] log: handle discard all logfiles properly (#6945)

* Handle discard all logfiles properly

Fixes https://github.com/hashicorp/consul/issues/6892.

The [docs](https://www.consul.io/docs/agent/options.html#_log_rotate_max_files) are stating:

> -log-rotate-max-files - to specify the maximum number of older log
> file archives to keep. Defaults to 0 (no files are ever deleted). Set to
> -1 to disable rotation and discard all log files.

But the `-1` case was not implemented and led to a panic when being
used.

Co-Authored-By: Freddy <freddygv@users.noreply.github.com>
---
 logger/logfile.go                         | 15 +++++++++----
 logger/logfile_test.go                    | 27 ++++++++++++++++++++++-
 website/source/docs/agent/options.html.md |  2 +-
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/logger/logfile.go b/logger/logfile.go
index 8b6ad4aa2adb..7effe7a95754 100644
--- a/logger/logfile.go
+++ b/logger/logfile.go
@@ -62,10 +62,11 @@ func (l *LogFile) fileNamePattern() string {
 
 func (l *LogFile) openNew() error {
 	fileNamePattern := l.fileNamePattern()
-	// New file name has the format : filename-timestamp.extension
+
 	createTime := now()
 	newfileName := fmt.Sprintf(fileNamePattern, strconv.FormatInt(createTime.UnixNano(), 10))
 	newfilePath := filepath.Join(l.logPath, newfileName)
+
 	// Try creating a file. We truncate the file because we are the only authority to write the logs
 	filePointer, err := os.OpenFile(newfilePath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0640)
 	if err != nil {
@@ -104,8 +105,14 @@ func (l *LogFile) pruneFiles() error {
 	if err != nil {
 		return err
 	}
-	// Prune if there are more files stored than the configured max
-	stale := len(matches) - l.MaxFiles
+	var stale int
+	if l.MaxFiles <= -1 {
+		// Prune everything
+		stale = len(matches)
+	} else {
+		// Prune if there are more files stored than the configured max
+		stale = len(matches) - l.MaxFiles
+	}
 	for i := 0; i < stale; i++ {
 		if err := os.Remove(matches[i]); err != nil {
 			return err
@@ -123,7 +130,7 @@ func (l *LogFile) Write(b []byte) (n int, err error) {
 
 	l.acquire.Lock()
 	defer l.acquire.Unlock()
-	//Create a new file if we have no file to write to
+	// Create a new file if we have no file to write to
 	if l.FileInfo == nil {
 		if err := l.openNew(); err != nil {
 			return 0, err
diff --git a/logger/logfile_test.go b/logger/logfile_test.go
index 44d55a245f31..95c6c9d0dc4f 100644
--- a/logger/logfile_test.go
+++ b/logger/logfile_test.go
@@ -136,7 +136,7 @@ func TestLogFile_deleteArchives(t *testing.T) {
 
 func TestLogFile_deleteArchivesDisabled(t *testing.T) {
 	t.Parallel()
-	tempDir := testutil.TempDir(t, "LogWriteDeleteArchivesDisabled")
+	tempDir := testutil.TempDir(t, t.Name())
 	defer os.Remove(tempDir)
 	filt := LevelFilter()
 	filt.MinLevel = logutils.LogLevel("INFO")
@@ -158,3 +158,28 @@ func TestLogFile_deleteArchivesDisabled(t *testing.T) {
 		return
 	}
 }
+
+func TestLogFile_rotationDisabled(t *testing.T) {
+	t.Parallel()
+	tempDir := testutil.TempDir(t, t.Name())
+	defer os.Remove(tempDir)
+	filt := LevelFilter()
+	filt.MinLevel = logutils.LogLevel("INFO")
+	logFile := LogFile{
+		logFilter: filt,
+		fileName:  testFileName,
+		logPath:   tempDir,
+		MaxBytes:  testBytes,
+		duration:  24 * time.Hour,
+		MaxFiles:  -1,
+	}
+	logFile.Write([]byte("[INFO] Hello World"))
+	logFile.Write([]byte("[INFO] Second File"))
+	logFile.Write([]byte("[INFO] Third File"))
+	want := 1
+	tempFiles, _ := ioutil.ReadDir(tempDir)
+	if got := tempFiles; len(got) != want {
+		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
+		return
+	}
+}
diff --git a/website/source/docs/agent/options.html.md b/website/source/docs/agent/options.html.md
index 0c3228a073ec..064f946135c5 100644
--- a/website/source/docs/agent/options.html.md
+++ b/website/source/docs/agent/options.html.md
@@ -283,7 +283,7 @@ The options below are all specified on the command-line.
 
 * <a name="_log_rotate_duration"></a><a href="#_log_rotate_duration">`-log-rotate-duration`</a> - to specify the maximum duration a log should be written to before it needs to be rotated. Must be a duration value such as 30s. Defaults to 24h.
 
-* <a name="_log_rotate_max_files"></a><a href="#_log_rotate_max_files">`-log-rotate-max-files`</a> - to specify the maximum number of older log file archives to keep. Defaults to 0 (no files are ever deleted). Set to -1 to disable rotation and discard all log files.
+* <a name="_log_rotate_max_files"></a><a href="#_log_rotate_max_files">`-log-rotate-max-files`</a> - to specify the maximum number of older log file archives to keep. Defaults to 0 (no files are ever deleted). Set to -1 to discard old log files when a new one is created.
 
 * <a name="_join"></a><a href="#_join">`-join`</a> - Address of another agent
   to join upon starting up. This can be