From 02013f639a129092b50fa903aee9fbea87b2ff2f Mon Sep 17 00:00:00 2001
From: Alex Courouble <courouba@amazon.com>
Date: Mon, 2 Nov 2020 10:46:39 -0800
Subject: [PATCH] Output to stderr when no log file path is passed

---
 README.md                       |  4 +++-
 pkg/utils/logger/logger_test.go | 25 +++++++++++++++++++++++++
 pkg/utils/logger/zaplogger.go   | 23 ++++++++++++++++-------
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/README.md b/README.md
index 7847106e76..26fb06cc6c 100644
--- a/README.md
+++ b/README.md
@@ -318,7 +318,9 @@ Default: `/var/log/aws-routed-eni/plugin.log`
 Valid Values: `stderr` or a file path
 
 Specifies where to write the logging output for `aws-cni` plugin. Either to `stderr` or to override the default file (i.e., `/var/log/aws-routed-eni/plugin.log`).
-`Stdout` cannot be supported for plugin log, please refer to #1248 for more details.
+`Stdout` cannot be supported for plugin log, please refer to [#1248](https://github.com/aws/amazon-vpc-cni-k8s/issues/1248) for more details.
+
+Note: If chaining an external plugin (i.e Cilium) that does not provide a `pluginLogFile` in its config file, the CNI plugin will by default write to `os.Stderr`. The output of `cmdAdd` are available in the Kubelet logs.
 
 ---
 
diff --git a/pkg/utils/logger/logger_test.go b/pkg/utils/logger/logger_test.go
index d488f9932d..0e93ab5888 100644
--- a/pkg/utils/logger/logger_test.go
+++ b/pkg/utils/logger/logger_test.go
@@ -19,6 +19,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"go.uber.org/zap/zapcore"
+	"gopkg.in/natefinch/lumberjack.v2"
 )
 
 func TestEnvLogFilePath(t *testing.T) {
@@ -58,3 +59,27 @@ func TestLogLevelReturnsDefaultLevelWhenEnvSetToInvalidValue(t *testing.T) {
 	expectedLogLevel = zapcore.DebugLevel
 	assert.Equal(t, expectedLogLevel, getZapLevel(inputLogLevel))
 }
+
+func TestGetPluginLogFilePathEmpty(t *testing.T) {
+	expectedWriter := zapcore.Lock(os.Stderr)
+	inputPluginLogFile := ""
+	assert.Equal(t, expectedWriter, getPluginLogFilePath(inputPluginLogFile))
+}
+
+func TestGetPluginLogFilePathStdout(t *testing.T) {
+	expectedWriter := zapcore.Lock(os.Stdout)
+	inputPluginLogFile := "stdout"
+	assert.Equal(t, expectedWriter, getPluginLogFilePath(inputPluginLogFile))
+}
+
+func TestGetPluginLogFilePath(t *testing.T) {
+	inputPluginLogFile := "/var/log/aws-routed-eni/plugin.log"
+	expectedLumberJackLogger := &lumberjack.Logger{
+		Filename:   "/var/log/aws-routed-eni/plugin.log",
+		MaxSize:    100,
+		MaxBackups: 5,
+		MaxAge:     30,
+		Compress:   true,
+	}
+	assert.Equal(t, zapcore.AddSync(expectedLumberJackLogger), getPluginLogFilePath(inputPluginLogFile))
+}
diff --git a/pkg/utils/logger/zaplogger.go b/pkg/utils/logger/zaplogger.go
index 306736b3fd..7b4801687b 100644
--- a/pkg/utils/logger/zaplogger.go
+++ b/pkg/utils/logger/zaplogger.go
@@ -105,17 +105,11 @@ func getEncoder() zapcore.Encoder {
 
 func (logConfig *Configuration) newZapLogger() *structuredLogger {
 	var cores []zapcore.Core
-	var writer zapcore.WriteSyncer
 
 	logLevel := getZapLevel(logConfig.LogLevel)
 
-	logFilePath := logConfig.LogLocation
+	writer := getPluginLogFilePath(logConfig.LogLocation)
 
-	if logFilePath != "" && strings.ToLower(logFilePath) != "stdout" {
-		writer = getLogWriter(logFilePath)
-	} else {
-		writer = zapcore.Lock(os.Stdout)
-	}
 	cores = append(cores, zapcore.NewCore(getEncoder(), writer, logLevel))
 
 	combinedCore := zapcore.NewTee(cores...)
@@ -132,6 +126,21 @@ func (logConfig *Configuration) newZapLogger() *structuredLogger {
 	}
 }
 
+// getPluginLogFilePath returns the writer
+func getPluginLogFilePath(logFilePath string) zapcore.WriteSyncer {
+	var writer zapcore.WriteSyncer
+
+	if logFilePath == "" {
+		writer = zapcore.Lock(os.Stderr)
+	} else if strings.ToLower(logFilePath) != "stdout" {
+		writer = getLogWriter(logFilePath)
+	} else {
+		writer = zapcore.Lock(os.Stdout)
+	}
+
+	return writer
+}
+
 //getLogWriter is for lumberjack
 func getLogWriter(logFilePath string) zapcore.WriteSyncer {
 	lumberJackLogger := &lumberjack.Logger{