refact cmd/crowdsec: remove globals, lint, etc (#4163)

* remove global additionalLabels

* remove (unused) global lastProcessedItem

* metrics.go

* tighter var scope

* cmd/crowdsec: use flagset

Author: mmetc

cmd/crowdsec/flags.go

@@ -3,6 +3,7 @@ package main
 import (
 	"flag"
 	"fmt"
+	"os"
 	"runtime"
 	"strings"
 
@@ -18,7 +19,7 @@ type Flags struct {
 
 	PrintVersion   bool
 	SingleFileType string
-	Labels         map[string]string
+	Labels         labelsMap
 	OneShotDSN     string
 	TestMode       bool
 	DisableAgent   bool
@@ -54,36 +55,44 @@ func (l *labelsMap) Set(label string) error {
 	return nil
 }
 
-func (f *Flags) parse() {
-	flag.StringVar(&f.ConfigFile, "c", csconfig.DefaultConfigPath("config.yaml"), "configuration file")
+func parseFlags(argv []string) (Flags, error) {
+	var f Flags
 
-	var trace, debug, info, warn, erro, fatal bool
+	fs := flag.NewFlagSet("crowdsec", flag.ExitOnError)
+	fs.SetOutput(os.Stderr)
+
+	fs.StringVar(&f.ConfigFile, "c", csconfig.DefaultConfigPath("config.yaml"), "configuration file")
 
-	flag.BoolVar(&trace, "trace", false, "set log level to 'trace' (VERY verbose)")
-	flag.BoolVar(&debug, "debug", false, "set log level to 'debug'")
-	flag.BoolVar(&info, "info", false, "set log level to 'info'")
-	flag.BoolVar(&warn, "warning", false, "set log level to 'warning'")
-	flag.BoolVar(&erro, "error", false, "set log level to 'error'")
-	flag.BoolVar(&fatal, "fatal", false, "set log level to 'fatal'")
-
-	flag.BoolVar(&f.PrintVersion, "version", false, "display version")
-	flag.StringVar(&f.OneShotDSN, "dsn", "", "Process a single data source in time-machine")
-	flag.StringVar(&f.Transform, "transform", "", "expr to apply on the event after acquisition")
-	flag.StringVar(&f.SingleFileType, "type", "", "Labels.type for file in time-machine")
-	flag.Var(&additionalLabels, "label", "Additional Labels for file in time-machine")
-	flag.BoolVar(&f.TestMode, "t", false, "only test configs")
-	flag.BoolVar(&f.DisableAgent, "no-cs", false, "disable crowdsec agent")
-	flag.BoolVar(&f.DisableAPI, "no-api", false, "disable local API")
-	flag.BoolVar(&f.DisableCAPI, "no-capi", false, "disable communication with Central API")
-	flag.BoolVar(&f.OrderEvent, "order-event", false, "enforce event ordering with significant performance cost")
+	var trace, debug, info, warn, erro, fatal bool
+	fs.BoolVar(&trace, "trace", false, "set log level to 'trace' (VERY verbose)")
+	fs.BoolVar(&debug, "debug", false, "set log level to 'debug'")
+	fs.BoolVar(&info, "info", false, "set log level to 'info'")
+	fs.BoolVar(&warn, "warning", false, "set log level to 'warning'")
+	fs.BoolVar(&erro, "error", false, "set log level to 'error'")
+	fs.BoolVar(&fatal, "fatal", false, "set log level to 'fatal'")
+
+	fs.BoolVar(&f.PrintVersion, "version", false, "display version")
+	fs.StringVar(&f.OneShotDSN, "dsn", "", "Process a single data source in time-machine")
+	fs.StringVar(&f.Transform, "transform", "", "expr to apply on the event after acquisition")
+	fs.StringVar(&f.SingleFileType, "type", "", "Labels.type for file in time-machine")
+	f.Labels = make(labelsMap)
+	fs.Var(&f.Labels, "label", "Additional Labels for file in time-machine")
+	fs.BoolVar(&f.TestMode, "t", false, "only test configs")
+	fs.BoolVar(&f.DisableAgent, "no-cs", false, "disable crowdsec agent")
+	fs.BoolVar(&f.DisableAPI, "no-api", false, "disable local API")
+	fs.BoolVar(&f.DisableCAPI, "no-capi", false, "disable communication with Central API")
+	fs.BoolVar(&f.OrderEvent, "order-event", false, "enforce event ordering with significant performance cost")
 
 	if runtime.GOOS == "windows" {
-		flag.StringVar(&f.WinSvc, "winsvc", "", "Windows service Action: Install, Remove etc..")
+		fs.StringVar(&f.WinSvc, "winsvc", "", "Windows service Action: Install, Remove etc..")
 	}
 
-	flag.StringVar(&f.DumpDir, "dump-data", "", "dump parsers/buckets raw outputs")
-	flag.StringVar(&f.CPUProfile, "cpu-profile", "", "write cpu profile to file")
-	flag.Parse()
+	fs.StringVar(&f.DumpDir, "dump-data", "", "dump parsers/buckets raw outputs")
+	fs.StringVar(&f.CPUProfile, "cpu-profile", "", "write cpu profile to file")
+
+	if err := fs.Parse(argv); err != nil {
+		return f, err
+	}
 
 	// Set the log level selected by the --trace, --debug, --info, etc. flags,
 	// giving precedence to the most verbose flag if multiple are set. If no flag is specified,
@@ -102,4 +111,11 @@ func (f *Flags) parse() {
 	case fatal:
 		f.LogLevel = log.FatalLevel
 	}
+
+	if len(fs.Args()) > 0 {
+		return f, fmt.Errorf("argument provided but not defined: %s", fs.Args()[0])
+
+	}
+
+	return f, nil
 }

cmd/crowdsec/main.go

@@ -3,14 +3,14 @@ package main
 import (
 	"context"
 	"errors"
-	"flag"
 	"fmt"
 	_ "net/http/pprof"
 	"os"
 	"path/filepath"
 	"runtime/pprof"
 	"time"
 
+	"github.com/fatih/color"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/tomb.v2"
 
@@ -47,8 +47,6 @@ var (
 	inputLineChan   chan pipeline.Event
 	inputEventChan  chan pipeline.Event
 	outputEventChan chan pipeline.Event // the buckets init returns its own chan that is used for multiplexing
-	// settings
-	lastProcessedItem time.Time // keep track of last item timestamp in time-machine. it is used to GC buckets when we dump them.
 	pluginBroker      csplugin.PluginBroker
 )
 
@@ -77,7 +75,6 @@ func LoadBuckets(cConfig *csconfig.Config, hub *cwhub.Hub) error {
 
 func LoadAcquisition(ctx context.Context, cConfig *csconfig.Config, hub *cwhub.Hub) ([]acquisition.DataSource, error) {
 	if flags.SingleFileType != "" && flags.OneShotDSN != "" {
-		flags.Labels = additionalLabels
 		flags.Labels["type"] = flags.SingleFileType
 
 		ds, err := acquisition.LoadAcquisitionFromDSN(ctx, flags.OneShotDSN, flags.Labels, flags.Transform, hub)
@@ -100,10 +97,6 @@ func LoadAcquisition(ctx context.Context, cConfig *csconfig.Config, hub *cwhub.H
 	return dataSources, nil
 }
 
-var (
-	additionalLabels = make(labelsMap)
-)
-
 // LoadConfig returns a configuration parsed from configuration file
 func LoadConfig(configFile string, disableAgent bool, disableAPI bool, quiet bool) (*csconfig.Config, error) {
 	cConfig, _, err := csconfig.NewConfig(configFile, disableAgent, disableAPI, quiet)
@@ -199,7 +192,7 @@ func LoadConfig(configFile string, disableAgent bool, disableAPI bool, quiet boo
 // or uptime of the application
 var crowdsecT0 time.Time
 
-func run() error {
+func run(flags Flags) error {
 	if flags.CPUProfile != "" {
 		f, err := os.Create(flags.CPUProfile)
 		if err != nil {
@@ -245,22 +238,22 @@ func main() {
 
 	crowdsecT0 = time.Now()
 
-	// Handle command line arguments
-	flags.parse()
-
-	if len(flag.Args()) > 0 {
-		fmt.Fprintf(os.Stderr, "argument provided but not defined: %s\n", flag.Args()[0])
-		flag.Usage()
-		// the flag package exits with 2 in case of unknown flag
+	parsedFlags, err := parseFlags(os.Args[1:])
+	if err != nil {
+		fmt.Fprintln(os.Stderr, color.RedString("Error:"), err)
+		// the flag package exits with 2 in case of unknown flag,
+		// we do the same for extra arguments
 		os.Exit(2)
 	}
 
+	flags = parsedFlags
+
 	if flags.PrintVersion {
 		os.Stdout.WriteString(cwversion.FullString())
-		os.Exit(0)
+		return
 	}
 
-	if err := run(); err != nil {
+	if err := run(flags); err != nil {
 		log.Fatal(err)
 	}
 }

cmd/crowdsec/metrics.go

@@ -1,8 +1,9 @@
 package main
 
 import (
-	"fmt"
+	"net"
 	"net/http"
+	"strconv"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -19,8 +20,8 @@ import (
 
 func computeDynamicMetrics(next http.Handler, dbClient *database.Client) http.HandlerFunc {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// catch panics here because they are not handled by servePrometheus
 		defer trace.CatchPanic("crowdsec/computeDynamicMetrics")
+
 		// update cache metrics (stash)
 		cache.UpdateCacheMetrics()
 		// update cache metrics (regexp)
@@ -36,7 +37,7 @@ func computeDynamicMetrics(next http.Handler, dbClient *database.Client) http.Ha
 
 		decisions, err := dbClient.QueryDecisionCountByScenario(ctx)
 		if err != nil {
-			log.Errorf("Error querying decisions for metrics: %v", err)
+			log.WithError(err).Error("querying decisions for metrics")
 			next.ServeHTTP(w, r)
 
 			return
@@ -56,7 +57,7 @@ func computeDynamicMetrics(next http.Handler, dbClient *database.Client) http.Ha
 
 		alerts, err := dbClient.AlertsCountPerScenario(ctx, alertsFilter)
 		if err != nil {
-			log.Errorf("Error querying alerts for metrics: %v", err)
+			log.WithError(err).Error("querying alerts for metrics")
 			next.ServeHTTP(w, r)
 
 			return
@@ -76,7 +77,7 @@ func registerPrometheus(config *csconfig.PrometheusCfg) {
 	}
 
 	if err := metrics.RegisterMetrics(config.Level); err != nil {
-		log.Errorf("Error registering prometheus metrics: %v", err)
+		log.WithError(err).Error("registering prometheus metrics")
 		return
 	}
 }
@@ -92,10 +93,10 @@ func servePrometheus(config *csconfig.PrometheusCfg, dbClient *database.Client,
 
 	http.Handle("/metrics", computeDynamicMetrics(promhttp.Handler(), dbClient))
 
-	if err := http.ListenAndServe(fmt.Sprintf("%s:%d", config.ListenAddr, config.ListenPort), nil); err != nil {
+	if err := http.ListenAndServe(net.JoinHostPort(config.ListenAddr, strconv.Itoa(config.ListenPort)), nil); err != nil {
 		// in time machine, we most likely have the LAPI using the port
 		if !flags.haveTimeMachine() {
-			log.Warningf("prometheus: %s", err)
+			log.WithError(err).Warning("serving metrics")
 		}
 	}
 }

cmd/crowdsec/pour.go

@@ -66,12 +66,6 @@ func runPour(ctx context.Context, input chan pipeline.Event, holders []leaky.Buc
 			} else {
 				metrics.GlobalBucketPourKo.Inc()
 			}
-
-			if parsed.MarshaledTime != "" {
-				if err := lastProcessedItem.UnmarshalText([]byte(parsed.MarshaledTime)); err != nil {
-					log.Warningf("failed to parse time from event: %s", err)
-				}
-			}
 		}
 	}
 }

cmd/crowdsec/serve.go

@@ -235,11 +235,6 @@ func unregisterWatcher(ctx context.Context, cConfig *csconfig.Config) (bool, err
 }
 
 func HandleSignals(ctx context.Context, cConfig *csconfig.Config) error {
-	var (
-		newConfig *csconfig.Config
-		err       error
-	)
-
 	signalChan := make(chan os.Signal, 1)
 
 	// We add os.Interrupt mostly to ease windows development,
@@ -265,24 +260,23 @@ func HandleSignals(ctx context.Context, cConfig *csconfig.Config) error {
 			case syscall.SIGHUP:
 				log.Warning("SIGHUP received, reloading")
 
-				if err = shutdown(s, cConfig); err != nil {
+				if err := shutdown(s, cConfig); err != nil {
 					exitChan <- fmt.Errorf("failed shutdown: %w", err)
 					return
 				}
 
-				if newConfig, err = reloadHandler(ctx, s); err != nil {
+				newConfig, err := reloadHandler(ctx, s)
+				if err != nil {
 					exitChan <- fmt.Errorf("reload handler failure: %w", err)
 					return
 				}
 
-				if newConfig != nil {
-					cConfig = newConfig
-				}
+				cConfig = newConfig
 			// ctrl+C, kill -SIGINT XXXX, kill -SIGTERM XXXX
 			case os.Interrupt, syscall.SIGTERM:
 				log.Warning("SIGTERM received, shutting down")
 
-				if err = shutdown(s, cConfig); err != nil {
+				if err := shutdown(s, cConfig); err != nil {
 					exitChan <- fmt.Errorf("failed shutdown: %w", err)
 					return
 				}
@@ -292,7 +286,7 @@ func HandleSignals(ctx context.Context, cConfig *csconfig.Config) error {
 		}
 	}()
 
-	err = <-exitChan
+	err := <-exitChan
 	if err == nil {
 		log.Warning("Crowdsec service shutting down")
 	}