feat(log): consolidate log file handling in waflog

Signed-off-by: Felipe Zipitria <[email protected]>

check/base.go

@@ -78,7 +78,7 @@ func (c *FTWCheck) ForcedFail(id string) bool {
 
 // CloudMode returns true if we are running in cloud mode
 func (c *FTWCheck) CloudMode() bool {
-	return c.overrides.Mode == config.CloudMode
+	return config.FTWConfig.RunMode == config.CloudRunMode
 }
 
 // SetCloudMode alters the values for expected logs and status code

check/base_test.go

@@ -20,8 +20,7 @@ testoverride:
 `
 
 var yamlCloudConfig = `---
-testoverride:
-  mode: "cloud"
+mode: "cloud"
 `
 
 func TestNewCheck(t *testing.T) {

cmd/root.go

@@ -60,6 +60,6 @@ func initConfig() {
 		}
 	}
 	if cloud {
-		config.FTWConfig.TestOverride.Mode = config.CloudMode
+		config.FTWConfig.RunMode = config.CloudRunMode
 	}
 }

config/config.go

@@ -95,7 +95,7 @@ func loadDefaults() {
 	if FTWConfig.LogMarkerHeaderName == "" {
 		FTWConfig.LogMarkerHeaderName = DefaultLogMarkerHeaderName
 	}
-	if FTWConfig.TestOverride.Mode == "" {
-		FTWConfig.TestOverride.Mode = DefaultMode
+	if FTWConfig.RunMode == "" {
+		FTWConfig.RunMode = DefaultRunMode
 	}
 }

config/config_test.go

@@ -10,7 +10,6 @@ import (
 
 var yamlConfig = `---
 logfile: 'tests/logs/modsec2-apache/apache2/error.log'
-cloudmode: True
 testoverride:
   input:
     dest_addr: 'httpbin.org'
@@ -19,6 +18,10 @@ testoverride:
     '920400-1': 'This test result must be ignored'
 `
 
+var yamlCloudConfig = `---
+mode: 'cloud'
+`
+
 var yamlBadConfig = `
 ---
 logfile: 'tests/logs/modsec2-apache/apache2/error.log'
@@ -130,8 +133,8 @@ func TestNewConfigFromEnvHasDefaults(t *testing.T) {
 		t.Error(err)
 	}
 
-	if FTWConfig.TestOverride.Mode != DefaultMode {
-		t.Errorf("unexpected default value '%s' for testoverride.mode", FTWConfig.TestOverride.Mode)
+	if FTWConfig.RunMode != DefaultRunMode {
+		t.Errorf("unexpected default value '%s' for run mode", FTWConfig.RunMode)
 	}
 	if FTWConfig.LogMarkerHeaderName != DefaultLogMarkerHeaderName {
 		t.Errorf("unexpected default value '%s' for logmarkerheadername", FTWConfig.LogMarkerHeaderName)
@@ -146,8 +149,8 @@ func TestNewConfigFromFileHasDefaults(t *testing.T) {
 		t.Error(err)
 	}
 
-	if FTWConfig.TestOverride.Mode != DefaultMode {
-		t.Errorf("unexpected default value '%s' for testoverride.mode", FTWConfig.TestOverride.Mode)
+	if FTWConfig.RunMode != DefaultRunMode {
+		t.Errorf("unexpected default value '%s' for run mode", FTWConfig.RunMode)
 	}
 	if FTWConfig.LogMarkerHeaderName != DefaultLogMarkerHeaderName {
 		t.Errorf("unexpected default value '%s' for logmarkerheadername", FTWConfig.LogMarkerHeaderName)
@@ -159,10 +162,23 @@ func TestNewConfigFromStringHasDefaults(t *testing.T) {
 		t.Error(err)
 	}
 
-	if FTWConfig.TestOverride.Mode != DefaultMode {
-		t.Errorf("unexpected default value '%s' for testoverride.mode", FTWConfig.TestOverride.Mode)
+	if FTWConfig.RunMode != DefaultRunMode {
+		t.Errorf("unexpected default value '%s' for run mode", FTWConfig.RunMode)
 	}
 	if FTWConfig.LogMarkerHeaderName != DefaultLogMarkerHeaderName {
 		t.Errorf("unexpected default value '%s' for logmarkerheadername", FTWConfig.LogMarkerHeaderName)
 	}
 }
+
+func TestNewConfigFromFileRunMode(t *testing.T) {
+	filename, _ := utils.CreateTempFileWithContent(yamlCloudConfig, "test-*.yaml")
+	defer os.Remove(filename)
+
+	if err := NewConfigFromFile(filename); err != nil {
+		t.Error(err)
+	}
+
+	if FTWConfig.RunMode != CloudRunMode {
+		t.Errorf("unexpected value '%s' for run mode, expected '%s;", FTWConfig.RunMode, CloudRunMode)
+	}
+}

config/types.go

@@ -1,10 +1,14 @@
 package config
 
+// RunMode represents the mode of the test run
+type RunMode string
+
 const (
-	// CloudMode is the string that will be used to override the mode of execution to cloud
-	CloudMode string = "cloud"
-	// DefaultMode is the default execution mode
-	DefaultMode                string = "default"
+	// CloudRunMode is the string that will be used to override the run mode of execution to cloud
+	CloudRunMode RunMode = "cloud"
+	// DefaultRunMode is the default execution run mode
+	DefaultRunMode RunMode = "default"
+	// DefaultLogMarkerHeaderName is the default log marker header name
 	DefaultLogMarkerHeaderName string = "X-CRS-Test"
 )
 
@@ -16,6 +20,7 @@ type FTWConfiguration struct {
 	LogFile             string          `koanf:"logfile"`
 	TestOverride        FTWTestOverride `koanf:"testoverride"`
 	LogMarkerHeaderName string          `koanf:"logmarkerheadername"`
+	RunMode             RunMode         `koanf:"mode"`
 }
 
 // FTWTestOverride holds four lists:
@@ -28,5 +33,4 @@ type FTWTestOverride struct {
 	Ignore    map[string]string `koanf:"ignore"`
 	ForcePass map[string]string `koanf:"forcepass"`
 	ForceFail map[string]string `koanf:"forcefail"`
-	Mode      string            `koanf:"mode"`
 }

runner/run.go

@@ -3,7 +3,6 @@ package runner
 import (
 	"errors"
 	"fmt"
-	"os"
 	"regexp"
 	"strconv"
 	"time"
@@ -27,19 +26,7 @@ import (
 func Run(include string, exclude string, showTime bool, output bool, ftwtests []test.FTWTest) TestRunContext {
 	printUnlessQuietMode(output, ":rocket:Running go-ftw!\n")
 
-	// TODO: this is a hack, there should only be one instance of FTWLogLines
-	logLines := &waflog.FTWLogLines{
-		FileName: config.FTWConfig.LogFile,
-	}
-	var logFile *os.File
-	if logLines.FileName != "" {
-		var err error
-		logFile, err = os.Open(logLines.FileName)
-		if err != nil {
-			log.Panic().Caller().Err(err).Msg("failed to open file")
-		}
-		defer logFile.Close()
-	}
+	logLines := waflog.NewFTWLogLines(waflog.WithLogFile(config.FTWConfig.LogFile))
 
 	client := ftwhttp.NewClient()
 	runContext := TestRunContext{
@@ -49,7 +36,7 @@ func Run(include string, exclude string, showTime bool, output bool, ftwtests []
 		Output:   output,
 		Client:   client,
 		LogLines: logLines,
-		LogFile:  logFile,
+		RunMode:  config.FTWConfig.RunMode,
 	}
 
 	for _, test := range ftwtests {
@@ -58,6 +45,8 @@ func Run(include string, exclude string, showTime bool, output bool, ftwtests []
 
 	printSummary(output, runContext.Stats)
 
+	defer cleanLogs(logLines)
+
 	return runContext
 }
 
@@ -128,11 +117,13 @@ func RunStage(runContext *TestRunContext, ftwCheck *check.FTWCheck, testCase tes
 		Protocol: testRequest.GetProtocol(),
 	}
 
-	startMarker, err := markAndFlush(runContext, dest, stageID)
-	if err != nil && !expectedOutput.ExpectError {
-		log.Fatal().Caller().Err(err).Msg("Failed to find start marker")
+	if notRunningInCloudMode(ftwCheck) {
+		startMarker, err := markAndFlush(runContext, dest, stageID)
+		if err != nil && !expectedOutput.ExpectError {
+			log.Fatal().Caller().Err(err).Msg("Failed to find start marker")
+		}
+		ftwCheck.SetStartMarker(startMarker)
 	}
-	ftwCheck.SetStartMarker(startMarker)
 
 	req = getRequestFromTest(testRequest)
 
@@ -150,12 +141,14 @@ func RunStage(runContext *TestRunContext, ftwCheck *check.FTWCheck, testCase tes
 		log.Fatal().Caller().Err(err).Msgf("can't connect to destination %+v - unexpected error found. Is your waf running?", dest)
 	}
 
-	endMarker, err := markAndFlush(runContext, dest, stageID)
-	if err != nil && !expectedOutput.ExpectError {
-		log.Fatal().Caller().Err(err).Msg("Failed to find end marker")
+	if notRunningInCloudMode(ftwCheck) {
+		endMarker, err := markAndFlush(runContext, dest, stageID)
+		if err != nil && !expectedOutput.ExpectError {
+			log.Fatal().Caller().Err(err).Msg("Failed to find end marker")
 
+		}
+		ftwCheck.SetEndMarker(endMarker)
 	}
-	ftwCheck.SetEndMarker(endMarker)
 
 	// Set expected test output in check
 	ftwCheck.SetExpectTestOutput(&expectedOutput)
@@ -206,7 +199,7 @@ func markAndFlush(runContext *TestRunContext, dest *ftwhttp.Destination, stageID
 			return nil, fmt.Errorf("ftw/run: failed sending request to %+v - unexpected error found. Is your waf running?", dest)
 		}
 
-		marker := runContext.LogLines.CheckLogForMarker(runContext.LogFile, stageID)
+		marker := runContext.LogLines.CheckLogForMarker(stageID)
 		if marker != nil {
 			return marker, nil
 		}
@@ -383,3 +376,13 @@ func applyInputOverride(testRequest *test.Input) error {
 	}
 	return retErr
 }
+
+func notRunningInCloudMode(c *check.FTWCheck) bool {
+	return !c.CloudMode()
+}
+
+func cleanLogs(logLines *waflog.FTWLogLines) {
+	if err := logLines.Cleanup(); err != nil {
+		log.Fatal().Err(err).Msg("Failed to cleanup log file")
+	}
+}

runner/run_test.go

@@ -12,7 +12,6 @@ import (
 	"github.com/fzipi/go-ftw/config"
 	"github.com/fzipi/go-ftw/ftwhttp"
 	"github.com/fzipi/go-ftw/test"
-	"github.com/fzipi/go-ftw/waflog"
 )
 
 var yamlConfig = `
@@ -44,8 +43,7 @@ testoverride:
 
 var yamlCloudConfig = `
 ---
-testoverride:
-  mode: cloud
+mode: cloud
 `
 
 var logText = `
@@ -248,7 +246,7 @@ func newTestServer(t *testing.T, logLines string) (destination *ftwhttp.Destinat
 	logFilePath = setUpLogFileForTestServer(t)
 
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(200)
+		w.WriteHeader(http.StatusOK)
 		_, _ = w.Write([]byte("Hello, client"))
 
 		writeTestServerLog(t, logLines, logFilePath, r)
@@ -266,14 +264,10 @@ func newTestServer(t *testing.T, logLines string) (destination *ftwhttp.Destinat
 }
 
 // Error checking omitted for brevity
-func newTestServerForCloudTest(t *testing.T, responseStatus int, logLines string) (server *httptest.Server, destination *ftwhttp.Destination, logFilePath string) {
-	logFilePath = setUpLogFileForTestServer(t)
-
+func newTestServerForCloudTest(t *testing.T, responseStatus int, logLines string) (server *httptest.Server, destination *ftwhttp.Destination) {
 	server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(responseStatus)
 		_, _ = w.Write([]byte("Hello, client"))
-
-		writeTestServerLog(t, logLines, logFilePath, r)
 	}))
 
 	// close server after test
@@ -284,12 +278,12 @@ func newTestServerForCloudTest(t *testing.T, responseStatus int, logLines string
 		t.Error(err)
 		t.FailNow()
 	}
-	return server, dest, logFilePath
+	return server, dest
 }
 
 func setUpLogFileForTestServer(t *testing.T) (logFilePath string) {
 	// log to the configured file
-	if config.FTWConfig != nil {
+	if config.FTWConfig != nil && config.FTWConfig.RunMode == config.DefaultRunMode {
 		logFilePath = config.FTWConfig.LogFile
 	}
 	// if no file has been configured, create one and handle cleanup
@@ -416,7 +410,7 @@ func TestOverrideRun(t *testing.T) {
 	// setup test webserver (not a waf)
 	err := config.NewConfigFromString(yamlConfigOverride)
 	if err != nil {
-		t.Errorf("Failed!")
+		t.Error(err)
 	}
 
 	dest, logFilePath := newTestServer(t, logText)
@@ -555,18 +549,9 @@ func TestCloudRun(t *testing.T) {
 				} else if stage.Output.NoLogContains != "" {
 					responseStatus = 405
 				}
-				server, dest, logFilePath := newTestServerForCloudTest(t, responseStatus, logText)
+				server, dest := newTestServerForCloudTest(t, responseStatus, logText)
 
 				replaceDestinationInConfiguration(*dest)
-				config.FTWConfig.LogFile = logFilePath
-				logLines := &waflog.FTWLogLines{
-					FileName: logFilePath,
-				}
-				logFile, err := os.Open(logFilePath)
-				if err != nil {
-					t.Fatal(err)
-				}
-				t.Cleanup(func() { logFile.Close() })
 
 				replaceDestinationInTest(&ftwTest, *dest)
 				if err != nil {
@@ -578,8 +563,7 @@ func TestCloudRun(t *testing.T) {
 					ShowTime: false,
 					Output:   true,
 					Client:   ftwhttp.NewClient(),
-					LogLines: logLines,
-					LogFile:  logFile,
+					LogLines: nil,
 				}
 
 				RunStage(&runContext, ftwCheck, *testCase, *stage)

runner/types.go

@@ -1,9 +1,9 @@
 package runner
 
 import (
-	"os"
 	"time"
 
+	"github.com/fzipi/go-ftw/config"
 	"github.com/fzipi/go-ftw/ftwhttp"
 	"github.com/fzipi/go-ftw/waflog"
 )
@@ -21,5 +21,5 @@ type TestRunContext struct {
 	Duration time.Duration
 	Client   *ftwhttp.Client
 	LogLines *waflog.FTWLogLines
-	LogFile  *os.File
+	RunMode  config.RunMode
 }