From 066d5c81f981b83d6accfcc40055581de4c62503 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 14:04:20 +0000 Subject: [PATCH 1/8] Ignore blank lines within file and stdin --- cmd/crowdsec-cli/explain.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 0c33a845e49..667be735c63 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -21,8 +21,15 @@ func GetLineCountForFile(filepath string) (int, error) { } defer f.Close() lc := 0 - fs := bufio.NewScanner(f) - for fs.Scan() { + fs := bufio.NewReader(f) + for { + input, err := fs.ReadBytes('\n') + if err != nil && err == io.EOF { + break + } + if len(input) <= 1 { + continue + } lc++ } return lc, nil @@ -121,6 +128,9 @@ func runExplain(cmd *cobra.Command, args []string) error { if err != nil && err == io.EOF { break } + if len(input) <= 1 { + continue + } _, err = f.Write(input) if err != nil { errCount++ @@ -145,6 +155,10 @@ func runExplain(cmd *cobra.Command, args []string) error { if err != nil { return err } + log.Debug("line count: ", lineCount) + if lineCount == 0 { + return fmt.Errorf("the log file is empty: %s", absolutePath) + } if lineCount > 100 { log.Warnf("The log file contains %d lines. This may take a lot of resources.", lineCount) } From 197f30b5cf426c5fe4cfddb5bbe935ee5411fc8f Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 14:32:12 +0000 Subject: [PATCH 2/8] change cleanup to be persistent postrun so if we exit early it always cleans --- cmd/crowdsec-cli/explain.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 667be735c63..ccf07434d0e 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -14,6 +14,10 @@ import ( "github.com/crowdsecurity/crowdsec/pkg/hubtest" ) +var ( + dir string +) + func GetLineCountForFile(filepath string) (int, error) { f, err := os.Open(filepath) if err != nil { @@ -102,7 +106,7 @@ func runExplain(cmd *cobra.Command, args []string) error { var f *os.File // using empty string fallback to /tmp - dir, err := os.MkdirTemp("", "cscli_explain") + dir, err = os.MkdirTemp("", "cscli_explain") if err != nil { return fmt.Errorf("couldn't create a temporary directory to store cscli explain result: %s", err) } @@ -180,12 +184,6 @@ func runExplain(cmd *cobra.Command, args []string) error { return fmt.Errorf("fail to run crowdsec for test: %v", err) } - // rm the temporary log file if only a log line/stdin was provided - if tmpFile != "" { - if err := os.Remove(tmpFile); err != nil { - return fmt.Errorf("unable to remove tmp log file '%s': %+v", tmpFile, err) - } - } parserDumpFile := filepath.Join(dir, hubtest.ParserResultFileName) bucketStateDumpFile := filepath.Join(dir, hubtest.BucketPourResultFileName) @@ -201,10 +199,6 @@ func runExplain(cmd *cobra.Command, args []string) error { hubtest.DumpTree(*parserDump, *bucketStateDump, opts) - if err := os.RemoveAll(dir); err != nil { - return fmt.Errorf("unable to delete temporary directory '%s': %s", dir, err) - } - return nil } @@ -224,6 +218,14 @@ tail -n 5 myfile.log | cscli explain --type nginx -f - Args: cobra.ExactArgs(0), DisableAutoGenTag: true, RunE: runExplain, + PersistentPostRunE: func(cmd *cobra.Command, args []string) error { + if _, err := os.Stat(dir); !os.IsNotExist(err) { + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("unable to delete temporary directory '%s': %s", dir, err) + } + } + return nil + }, } flags := cmdExplain.Flags() From 9c49ff0e3cc29b85aaa61395b408db73f22c1a65 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 14:41:51 +0000 Subject: [PATCH 3/8] When using log flag we should add a newline so we know where EOF is --- cmd/crowdsec-cli/explain.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index ccf07434d0e..2e7508340d1 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -120,7 +120,7 @@ func runExplain(cmd *cobra.Command, args []string) error { } if logLine != "" { - _, err = f.WriteString(logLine) + _, err = f.WriteString(logLine + "\n") if err != nil { return err } From 99f79d73f058be2c0648127766ae83f5aeab7f1e Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 14:56:56 +0000 Subject: [PATCH 4/8] Inverse the check for log line since we dont want to modify the line itself --- cmd/crowdsec-cli/explain.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 2e7508340d1..c6291c9c30e 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -28,13 +28,12 @@ func GetLineCountForFile(filepath string) (int, error) { fs := bufio.NewReader(f) for { input, err := fs.ReadBytes('\n') + if len(input) > 1 { + lc++ + } if err != nil && err == io.EOF { break } - if len(input) <= 1 { - continue - } - lc++ } return lc, nil } @@ -120,7 +119,7 @@ func runExplain(cmd *cobra.Command, args []string) error { } if logLine != "" { - _, err = f.WriteString(logLine + "\n") + _, err = f.WriteString(logLine) if err != nil { return err } From d6db8e53c6595e6321b4c9107f915581ed0c88fc Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 15:51:23 +0000 Subject: [PATCH 5/8] Wrap run explain with a function that returns the error after cleaning up --- cmd/crowdsec-cli/explain.go | 60 +++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index c6291c9c30e..2a7dd15d27e 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -89,19 +89,6 @@ func runExplain(cmd *cobra.Command, args []string) error { return err } - fileInfo, _ := os.Stdin.Stat() - - if logType == "" || (logLine == "" && logFile == "" && dsn == "") { - printHelp(cmd) - fmt.Println() - fmt.Printf("Please provide --type flag\n") - os.Exit(1) - } - - if logFile == "-" && ((fileInfo.Mode() & os.ModeCharDevice) == os.ModeCharDevice) { - return fmt.Errorf("the option -f - is intended to work with pipes") - } - var f *os.File // using empty string fallback to /tmp @@ -216,13 +203,54 @@ tail -n 5 myfile.log | cscli explain --type nginx -f - `, Args: cobra.ExactArgs(0), DisableAutoGenTag: true, - RunE: runExplain, - PersistentPostRunE: func(cmd *cobra.Command, args []string) error { + Run: func(cmd *cobra.Command, args []string) { + err := runExplain(cmd, args) + if err != nil { + log.Error(err) + } if _, err := os.Stat(dir); !os.IsNotExist(err) { if err := os.RemoveAll(dir); err != nil { - return fmt.Errorf("unable to delete temporary directory '%s': %s", dir, err) + log.Errorf("unable to delete temporary directory '%s': %s", dir, err) } } + }, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + flags := cmd.Flags() + + logFile, err := flags.GetString("file") + if err != nil { + return err + } + + dsn, err := flags.GetString("dsn") + if err != nil { + return err + } + + logLine, err := flags.GetString("log") + if err != nil { + return err + } + + logType, err := flags.GetString("type") + if err != nil { + return err + } + + if logLine == "" && logFile == "" && dsn == "" { + printHelp(cmd) + fmt.Println() + return fmt.Errorf("please provide --log, --file or --dsn flag") + } + if logType == "" { + printHelp(cmd) + fmt.Println() + return fmt.Errorf("please provide --type flag") + } + fileInfo, _ := os.Stdin.Stat() + if logFile == "-" && ((fileInfo.Mode() & os.ModeCharDevice) == os.ModeCharDevice) { + return fmt.Errorf("the option -f - is intended to work with pipes") + } return nil }, } From e44d4df176f72d6ccfd6e7b6a5f5f1adabdbbcd8 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 15:53:01 +0000 Subject: [PATCH 6/8] Wrap run explain with a function that returns the error after cleanup --- cmd/crowdsec-cli/explain.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 2a7dd15d27e..625f81048e8 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -203,16 +203,14 @@ tail -n 5 myfile.log | cscli explain --type nginx -f - `, Args: cobra.ExactArgs(0), DisableAutoGenTag: true, - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { err := runExplain(cmd, args) - if err != nil { - log.Error(err) - } if _, err := os.Stat(dir); !os.IsNotExist(err) { if err := os.RemoveAll(dir); err != nil { log.Errorf("unable to delete temporary directory '%s': %s", dir, err) } } + return err }, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { flags := cmd.Flags() From 7646a9d0886ef5399415f4471f678009a185afdc Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 16:06:09 +0000 Subject: [PATCH 7/8] Use a defer iif instead of global var --- cmd/crowdsec-cli/explain.go | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 625f81048e8..209a49e7133 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -14,10 +14,6 @@ import ( "github.com/crowdsecurity/crowdsec/pkg/hubtest" ) -var ( - dir string -) - func GetLineCountForFile(filepath string) (int, error) { f, err := os.Open(filepath) if err != nil { @@ -92,10 +88,17 @@ func runExplain(cmd *cobra.Command, args []string) error { var f *os.File // using empty string fallback to /tmp - dir, err = os.MkdirTemp("", "cscli_explain") + dir, err := os.MkdirTemp("", "cscli_explain") if err != nil { return fmt.Errorf("couldn't create a temporary directory to store cscli explain result: %s", err) } + defer func() { + if _, err := os.Stat(dir); !os.IsNotExist(err) { + if err := os.RemoveAll(dir); err != nil { + log.Errorf("unable to delete temporary directory '%s': %s", dir, err) + } + } + }() tmpFile := "" // we create a temporary log file if a log line/stdin has been provided if logLine != "" || logFile == "-" { @@ -203,15 +206,7 @@ tail -n 5 myfile.log | cscli explain --type nginx -f - `, Args: cobra.ExactArgs(0), DisableAutoGenTag: true, - RunE: func(cmd *cobra.Command, args []string) error { - err := runExplain(cmd, args) - if _, err := os.Stat(dir); !os.IsNotExist(err) { - if err := os.RemoveAll(dir); err != nil { - log.Errorf("unable to delete temporary directory '%s': %s", dir, err) - } - } - return err - }, + RunE: runExplain, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { flags := cmd.Flags() From 35759f5df3c8485ff0998a1714bb889007c8f3f1 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 30 Nov 2023 16:29:20 +0000 Subject: [PATCH 8/8] Add invalid len input to err count so it more obvious what is happening --- cmd/crowdsec-cli/explain.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cmd/crowdsec-cli/explain.go b/cmd/crowdsec-cli/explain.go index 209a49e7133..1a9d24b853e 100644 --- a/cmd/crowdsec-cli/explain.go +++ b/cmd/crowdsec-cli/explain.go @@ -121,16 +121,15 @@ func runExplain(cmd *cobra.Command, args []string) error { if err != nil && err == io.EOF { break } - if len(input) <= 1 { - continue + if len(input) > 1 { + _, err = f.Write(input) } - _, err = f.Write(input) - if err != nil { + if err != nil || len(input) <= 1 { errCount++ } } if errCount > 0 { - log.Warnf("Failed to write %d lines to tmp file", errCount) + log.Warnf("Failed to write %d lines to %s", errCount, tmpFile) } } f.Close() @@ -148,12 +147,12 @@ func runExplain(cmd *cobra.Command, args []string) error { if err != nil { return err } - log.Debug("line count: ", lineCount) + log.Debugf("file %s has %d lines", absolutePath, lineCount) if lineCount == 0 { return fmt.Errorf("the log file is empty: %s", absolutePath) } if lineCount > 100 { - log.Warnf("The log file contains %d lines. This may take a lot of resources.", lineCount) + log.Warnf("%s contains %d lines. This may take a lot of resources.", absolutePath, lineCount) } }