error reporting (#1501)

* unified error reporting, removed redundancy, tests

cmd/crowdsec-cli/alerts.go

@@ -18,6 +18,7 @@ import (
 	"github.com/crowdsecurity/crowdsec/pkg/models"
 	"github.com/go-openapi/strfmt"
 	"github.com/olekukonko/tablewriter"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"gopkg.in/yaml.v2"
@@ -197,20 +198,14 @@ func NewAlertsCmd() *cobra.Command {
 		Short:             "Manage alerts",
 		Args:              cobra.MinimumNArgs(1),
 		DisableAutoGenTag: true,
-		PersistentPreRun: func(cmd *cobra.Command, args []string) {
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			var err error
 			if err := csConfig.LoadAPIClient(); err != nil {
-				log.Fatalf("loading api client: %s", err.Error())
-			}
-			if csConfig.API.Client == nil {
-				log.Fatalln("There is no configuration on 'api_client:'")
-			}
-			if csConfig.API.Client.Credentials == nil {
-				log.Fatalf("Please provide credentials for the API in '%s'", csConfig.API.Client.CredentialsFilePath)
+				return errors.Wrap(err, "loading api client")
 			}
 			apiURL, err := url.Parse(csConfig.API.Client.Credentials.URL)
 			if err != nil {
-				log.Fatalf("parsing api url: %s", apiURL)
+				return errors.Wrapf(err, "parsing api url %s", apiURL)
 			}
 			Client, err = apiclient.NewClient(&apiclient.Config{
 				MachineID:     csConfig.API.Client.Credentials.Login,
@@ -221,8 +216,9 @@ func NewAlertsCmd() *cobra.Command {
 			})
 
 			if err != nil {
-				log.Fatalf("new api client: %s", err.Error())
+				return errors.Wrap(err, "new api client")
 			}
+			return nil
 		},
 	}
 

cmd/crowdsec-cli/decisions.go

@@ -19,6 +19,7 @@ import (
 	"github.com/go-openapi/strfmt"
 	"github.com/jszwec/csvutil"
 	"github.com/olekukonko/tablewriter"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
@@ -147,20 +148,14 @@ func NewDecisionsCmd() *cobra.Command {
 		/*TBD example*/
 		Args:              cobra.MinimumNArgs(1),
 		DisableAutoGenTag: true,
-		PersistentPreRun: func(cmd *cobra.Command, args []string) {
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			if err := csConfig.LoadAPIClient(); err != nil {
-				log.Fatalf(err.Error())
-			}
-			if csConfig.API.Client == nil {
-				log.Fatalln("There is no configuration on 'api_client:'")
-			}
-			if csConfig.API.Client.Credentials == nil {
-				log.Fatalf("Please provide credentials for the API in '%s'", csConfig.API.Client.CredentialsFilePath)
+				return errors.Wrap(err, "loading api client")
 			}
 			password := strfmt.Password(csConfig.API.Client.Credentials.Password)
 			apiurl, err := url.Parse(csConfig.API.Client.Credentials.URL)
 			if err != nil {
-				log.Fatalf("parsing api url ('%s'): %s", csConfig.API.Client.Credentials.URL, err)
+				return errors.Wrapf(err, "parsing api url %s", csConfig.API.Client.Credentials.URL)
 			}
 			Client, err = apiclient.NewClient(&apiclient.Config{
 				MachineID:     csConfig.API.Client.Credentials.Login,
@@ -170,8 +165,9 @@ func NewDecisionsCmd() *cobra.Command {
 				VersionPrefix: "v1",
 			})
 			if err != nil {
-				log.Fatalf("creating api client : %s", err)
+				return errors.Wrap(err, "creating api client")
 			}
+			return nil
 		},
 	}
 

cmd/crowdsec-cli/lapi.go

@@ -13,6 +13,7 @@ import (
 	"github.com/crowdsecurity/crowdsec/pkg/cwversion"
 	"github.com/crowdsecurity/crowdsec/pkg/models"
 	"github.com/go-openapi/strfmt"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"gopkg.in/yaml.v2"
@@ -29,13 +30,7 @@ func NewLapiCmd() *cobra.Command {
 		DisableAutoGenTag: true,
 		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			if err := csConfig.LoadAPIClient(); err != nil {
-				return fmt.Errorf("loading api client: %s", err.Error())
-			}
-			if csConfig.API.Client == nil {
-				log.Fatalln("There is no API->client configuration")
-			}
-			if csConfig.API.Client.Credentials == nil {
-				log.Fatalf("no configuration for Local API (LAPI) in '%s'", *csConfig.FilePath)
+				return errors.Wrap(err, "loading api client")
 			}
 			return nil
 		},

cmd/crowdsec-cli/main.go

@@ -183,6 +183,6 @@ It is meant to allow you to manage bans, parsers/scenarios/etc, api and generall
 	rootCmd.AddCommand(NewHubTestCmd())
 	rootCmd.AddCommand(NewNotificationsCmd())
 	if err := rootCmd.Execute(); err != nil {
-		log.Fatalf("While executing root command : %s", err)
+		log.Fatal(err)
 	}
 }

cmd/crowdsec/api.go

@@ -14,7 +14,7 @@ import (
 func initAPIServer(cConfig *csconfig.Config) (*apiserver.APIServer, error) {
 	apiServer, err := apiserver.NewServer(cConfig.API.Server)
 	if err != nil {
-		return nil, fmt.Errorf("unable to run local API: %s", err)
+		return nil, errors.Wrap(err, "unable to run local API")
 	}
 
 	if hasPlugins(cConfig.API.Server.Profiles) {

cmd/crowdsec/pour.go

@@ -42,7 +42,7 @@ func runPour(input chan types.Event, holders []leaky.BucketFactory, buckets *lea
 			//here we can bucketify with parsed
 			poured, err := leaky.PourItemToHolders(parsed, holders, buckets)
 			if err != nil {
-				log.Fatalf("bucketify failed for: %v", parsed)
+				log.Errorf("bucketify failed for: %v", parsed)
 				return fmt.Errorf("process of event failed : %v", err)
 			}
 			if poured {

cmd/crowdsec/run_in_svc.go

@@ -51,7 +51,11 @@ func StartRunSvc() {
 
 	if exitCode, err := Serve(cConfig); err != nil {
 		if err != nil {
-			log.Errorf(err.Error())
+			// this method of logging a fatal error does not
+			// trigger a program exit (as stated by the authors, it
+			// is not going to change in logrus to keep backward
+			// compatibility), and allows us to report coverage.
+			log.NewEntry(log.StandardLogger()).Log(log.FatalLevel, err)
 			if !bincoverTesting {
 				os.Exit(exitCode)
 			}

pkg/apiserver/apiserver.go

@@ -94,7 +94,7 @@ func NewServer(config *csconfig.LocalApiServerCfg) (*APIServer, error) {
 	var flushScheduler *gocron.Scheduler
 	dbClient, err := database.NewClient(config.DbConfig)
 	if err != nil {
-		return &APIServer{}, fmt.Errorf("unable to init database client: %s", err)
+		return &APIServer{}, errors.Wrap(err, "unable to init database client")
 	}
 
 	if config.DbConfig.Flush != nil {

pkg/apiserver/apiserver_test.go

@@ -289,7 +289,7 @@ func TestWithWrongDBConfig(t *testing.T) {
 	apiServer, err := NewServer(config.API.Server)
 
 	assert.Equal(t, apiServer, &APIServer{})
-	assert.Equal(t, "unable to init database client: unknown database type", err.Error())
+	assert.Equal(t, "unable to init database client: unknown database type 'test'", err.Error())
 }
 
 func TestWithWrongFlushConfig(t *testing.T) {

pkg/csconfig/api.go

@@ -58,18 +58,20 @@ func (l *LocalApiClientCfg) Load() error {
 	patcher := yamlpatch.NewPatcher(l.CredentialsFilePath, ".local")
 	fcontent, err := patcher.MergedPatchContent()
 	if err != nil {
-		return errors.Wrapf(err, "failed to read api client credential configuration file '%s'", l.CredentialsFilePath)
+		return err
 	}
 	err = yaml.UnmarshalStrict(fcontent, &l.Credentials)
 	if err != nil {
 		return errors.Wrapf(err, "failed unmarshaling api client credential configuration file '%s'", l.CredentialsFilePath)
 	}
+	if l.Credentials == nil || l.Credentials.URL == "" {
+		return fmt.Errorf("no credentials or URL found in api client configuration '%s'", l.CredentialsFilePath)
+	}
+
 	if l.Credentials != nil && l.Credentials.URL != "" {
 		if !strings.HasSuffix(l.Credentials.URL, "/") {
 			l.Credentials.URL = l.Credentials.URL + "/"
 		}
-	} else {
-		log.Warningf("no credentials or URL found in api client configuration '%s'", l.CredentialsFilePath)
 	}
 	if l.InsecureSkipVerify == nil {
 		apiclient.InsecureSkipVerify = false
@@ -177,13 +179,13 @@ func (c *Config) LoadAPIServer() error {
 }
 
 func (c *Config) LoadAPIClient() error {
-	if c.API != nil && c.API.Client != nil && c.API.Client.CredentialsFilePath != "" && !c.DisableAgent {
-		if err := c.API.Client.Load(); err != nil {
-			return err
-		}
-	} else {
+	if c.API == nil || c.API.Client == nil || c.API.Client.CredentialsFilePath == "" || c.DisableAgent {
 		return fmt.Errorf("no API client section in configuration")
 	}
 
+	if err := c.API.Client.Load(); err != nil {
+		return err
+	}
+
 	return nil
 }

pkg/csconfig/config_test.go

@@ -18,9 +18,9 @@ func TestNormalLoad(t *testing.T) {
 
 	_, err = NewConfig("./tests/xxx.yaml", false, false)
 	if runtime.GOOS != "windows" {
-		assert.EqualError(t, err, "while reading ./tests/xxx.yaml: open ./tests/xxx.yaml: no such file or directory")
+		assert.EqualError(t, err, "while reading yaml file: open ./tests/xxx.yaml: no such file or directory")
 	} else {
-		assert.EqualError(t, err, "while reading ./tests/xxx.yaml: open ./tests/xxx.yaml: The system cannot find the file specified.")
+		assert.EqualError(t, err, "while reading yaml file: open ./tests/xxx.yaml: The system cannot find the file specified.")
 	}
 
 	_, err = NewConfig("./tests/simulation.yaml", false, false)

pkg/csconfig/simulation_test.go

@@ -89,7 +89,7 @@ func TestSimulationLoading(t *testing.T) {
 				},
 				Crowdsec: &CrowdsecServiceCfg{},
 			},
-			err: fmt.Sprintf("while reading %s: open %s: The system cannot find the file specified.", testXXFullPath, testXXFullPath),
+			err: fmt.Sprintf("while reading yaml file: open %s: The system cannot find the file specified.", testXXFullPath),
 		})
 	} else {
 		tests = append(tests, struct {
@@ -106,7 +106,7 @@ func TestSimulationLoading(t *testing.T) {
 				},
 				Crowdsec: &CrowdsecServiceCfg{},
 			},
-			err: fmt.Sprintf("while reading %s: open %s: no such file or directory", testXXFullPath, testXXFullPath),
+			err: fmt.Sprintf("while reading yaml file: open %s: no such file or directory", testXXFullPath),
 		})
 	}
 

pkg/database/database.go

@@ -100,7 +100,7 @@ func NewClient(config *csconfig.DatabaseCfg) (*Client, error) {
 		}
 		client = ent.NewClient(ent.Driver(drv), entOpt)
 	default:
-		return &Client{}, fmt.Errorf("unknown database type")
+		return &Client{}, fmt.Errorf("unknown database type '%s'", config.Type)
 	}
 
 	if config.LogLevel != nil && *config.LogLevel >= log.DebugLevel {

pkg/types/dataset_test.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"io/ioutil"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -10,6 +11,9 @@ import (
 )
 
 func TestDownladFile(t *testing.T) {
+	examplePath := "./example.txt"
+	defer os.Remove(examplePath)
+
 	httpmock.Activate()
 	defer httpmock.DeactivateAndReset()
 	//OK
@@ -23,16 +27,16 @@ func TestDownladFile(t *testing.T) {
 		"https://example.com/x",
 		httpmock.NewStringResponder(404, "not found"),
 	)
-	err := downloadFile("https://example.com/xx", "./example.txt")
+	err := downloadFile("https://example.com/xx", examplePath)
 	assert.NoError(t, err)
-	content, err := ioutil.ReadFile("./example.txt")
+	content, err := ioutil.ReadFile(examplePath)
 	assert.Equal(t, "example content oneoneone", string(content))
 	assert.NoError(t, err)
 	//bad uri
-	err = downloadFile("https://zz.com", "./example.txt")
+	err = downloadFile("https://zz.com", examplePath)
 	assert.Error(t, err)
 	//404
-	err = downloadFile("https://example.com/x", "./example.txt")
+	err = downloadFile("https://example.com/x", examplePath)
 	assert.Error(t, err)
 	//bad target
 	err = downloadFile("https://example.com/xx", "")

pkg/yamlpatch/patcher.go

@@ -29,7 +29,7 @@ func readYAML(filePath string) ([]byte, error) {
 	var err error
 
 	if content, err = os.ReadFile(filePath); err != nil {
-		return nil, errors.Wrapf(err, "while reading %s", filePath)
+		return nil, errors.Wrap(err, "while reading yaml file")
 	}
 
 	var yamlMap map[interface{}]interface{}

tests/bats/01_base.bats

@@ -26,14 +26,14 @@ declare stderr
 
 #----------
 
-@test "$FILE cscli - usage" {
+@test "${FILE} cscli - usage" {
     run -0 cscli
     assert_output --partial "Usage:"
     assert_output --partial "cscli [command]"
     assert_output --partial "Available Commands:"
 }
 
-@test "$FILE cscli version" {
+@test "${FILE} cscli version" {
     run -0 cscli version
     assert_output --partial "version:"
     assert_output --partial "Codename:"
@@ -51,7 +51,7 @@ declare stderr
     assert_output --partial "version:"
 }
 
-@test "$FILE cscli help" {
+@test "${FILE} cscli help" {
     run -0 cscli help
     assert_line "Available Commands:"
     assert_line --regexp ".* help .* Help about any command"
@@ -62,7 +62,7 @@ declare stderr
     assert_line "Available Commands:"
 }
 
-@test "$FILE cscli alerts list: at startup returns at least one entry: community pull" {
+@test "${FILE} cscli alerts list: at startup returns at least one entry: community pull" {
     is_db_postgres && skip
     # it should have been received while preparing the fixture
     run -0 cscli alerts list -o json
@@ -80,7 +80,7 @@ declare stderr
     # refute_output 0
 }
 
-@test "$FILE cscli capi status" {
+@test "${FILE} cscli capi status" {
     run -0 cscli capi status
     assert_output --partial "Loaded credentials from"
     assert_output --partial "Trying to authenticate with username"
@@ -88,7 +88,7 @@ declare stderr
     assert_output --partial "You can successfully interact with Central API (CAPI)"
 }
 
-@test "$FILE cscli config show -o human" {
+@test "${FILE} cscli config show -o human" {
     run -0 cscli config show -o human
     assert_output --partial "Global:"
     assert_output --partial "Crowdsec:"
@@ -96,7 +96,7 @@ declare stderr
     assert_output --partial "Local API Server:"
 }
 
-@test "$FILE cscli config show -o json" {
+@test "${FILE} cscli config show -o json" {
     run -0 cscli config show -o json
     assert_output --partial '"API":'
     assert_output --partial '"Common":'
@@ -109,7 +109,7 @@ declare stderr
     assert_output --partial '"Prometheus":'
 }
 
-@test "$FILE cscli config show -o raw" {
+@test "${FILE} cscli config show -o raw" {
     run -0 cscli config show -o raw
     assert_line "api:"
     assert_line "common:"
@@ -121,45 +121,112 @@ declare stderr
     assert_line "prometheus:"
 }
 
-@test "$FILE cscli config show --key" {
+@test "${FILE} cscli config show --key" {
     run -0 cscli config show --key Config.API.Server.ListenURI
     assert_output "127.0.0.1:8080"
 }
 
-@test "$FILE cscli config backup" {
+@test "${FILE} cscli config backup" {
     backupdir=$(TMPDIR="${BATS_TEST_TMPDIR}" mktemp -u)
     run -0 cscli config backup "${backupdir}"
     assert_output --partial "Starting configuration backup"
     run -1 --separate-stderr cscli config backup "${backupdir}"
 
-    run -0 echo "$stderr"
+    run -0 echo "${stderr}"
     assert_output --partial "Failed to backup configurations"
     assert_output --partial "file exists"
     rm -rf -- "${backupdir:?}"
 }
 
-@test "$FILE cscli lapi status" {
+@test "${FILE} cscli lapi status" {
     if is_db_postgres; then sleep 4; fi
     run -0 --separate-stderr cscli lapi status
 
-    run -0 echo "$stderr"
+    run -0 echo "${stderr}"
     assert_output --partial "Loaded credentials from"
     assert_output --partial "Trying to authenticate with username"
     assert_output --partial " on http://127.0.0.1:8080/"
     assert_output --partial "You can successfully interact with Local API (LAPI)"
 }
 
-@test "$FILE cscli metrics" {
+@test "${FILE} cscli - missing LAPI credentials file" {
+    LOCAL_API_CREDENTIALS=$(config_yq '.api.client.credentials_path')
+    rm -f "${LOCAL_API_CREDENTIALS}"
+    run -1 --separate-stderr cscli lapi status
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: while reading yaml file: open ${LOCAL_API_CREDENTIALS}: no such file or directory"
+
+    run -1 --separate-stderr cscli alerts list
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: while reading yaml file: open ${LOCAL_API_CREDENTIALS}: no such file or directory"
+
+    run -1 --separate-stderr cscli decisions list
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: while reading yaml file: open ${LOCAL_API_CREDENTIALS}: no such file or directory"
+}
+
+@test "${FILE} cscli - empty LAPI credentials file" {
+    LOCAL_API_CREDENTIALS=$(config_yq '.api.client.credentials_path')
+    truncate -s 0 "${LOCAL_API_CREDENTIALS}"
+    run -1 --separate-stderr cscli lapi status
+    run -0 echo "${stderr}"
+    assert_output --partial "no credentials or URL found in api client configuration '${LOCAL_API_CREDENTIALS}'"
+
+    run -1 --separate-stderr cscli alerts list
+    run -0 echo "${stderr}"
+    assert_output --partial "no credentials or URL found in api client configuration '${LOCAL_API_CREDENTIALS}'"
+
+    run -1 --separate-stderr cscli decisions list
+    run -0 echo "${stderr}"
+    assert_output --partial "no credentials or URL found in api client configuration '${LOCAL_API_CREDENTIALS}'"
+}
+
+@test "${FILE} cscli - missing LAPI client settings" {
+    yq e 'del(.api.client)' -i "${CONFIG_YAML}"
+    run -1 --separate-stderr cscli lapi status
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: no API client section in configuration"
+
+    run -1 --separate-stderr cscli alerts list
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: no API client section in configuration"
+
+    run -1 --separate-stderr cscli decisions list
+    run -0 echo "${stderr}"
+    assert_output --partial "loading api client: no API client section in configuration"
+}
+
+@test "${FILE} cscli - malformed LAPI url" {
+    LOCAL_API_CREDENTIALS=$(config_yq '.api.client.credentials_path')
+    yq e '.url="https://127.0.0.1:-80"' -i "${LOCAL_API_CREDENTIALS}"
+
+    run -1 --separate-stderr cscli lapi status
+    run -0 echo "${stderr}"
+    assert_output --partial 'parsing api url'
+    assert_output --partial 'invalid port \":-80\" after host'
+
+    run -1 --separate-stderr cscli alerts list
+    run -0 echo "${stderr}"
+    assert_output --partial 'parsing api url'
+    assert_output --partial 'invalid port ":-80" after host'
+
+    run -1 --separate-stderr cscli decisions list
+    run -0 echo "${stderr}"
+    assert_output --partial 'parsing api url'
+    assert_output --partial 'invalid port ":-80" after host'
+}
+
+@test "${FILE} cscli metrics" {
     run -0 cscli lapi status
     run -0 --separate-stderr cscli metrics
     assert_output --partial "ROUTE"
     assert_output --partial '/v1/watchers/login'
 
-    run -0 echo "$stderr"
+    run -0 echo "${stderr}"
     assert_output --partial "Local Api Metrics:"
 }
 
-@test "$FILE 'cscli completion' with or without configuration file" {
+@test "${FILE} 'cscli completion' with or without configuration file" {
     run -0 cscli completion bash
     assert_output --partial "# bash completion for cscli"
     run -0 cscli completion zsh
@@ -172,7 +239,7 @@ declare stderr
     assert_output --partial "# zsh completion for cscli"
 }
 
-@test "$FILE cscli hub list" {
+@test "${FILE} cscli hub list" {
     # we check for the presence of some objects. There may be others when we
     # use $PACKAGE_TESTING, so the order is not important.
 

tests/bats/06_crowdsec.bats

@@ -0,0 +1,36 @@
+#!/usr/bin/env bats
+# vim: ft=bats:list:ts=8:sts=4:sw=4:et:ai:si:
+
+set -u
+
+setup_file() {
+    load "../lib/setup_file.sh"
+}
+
+teardown_file() {
+    load "../lib/teardown_file.sh"
+}
+
+setup() {
+    load "../lib/setup.sh"
+    ./instance-data load
+}
+
+teardown() {
+    ./instance-crowdsec stop
+}
+
+# to silence shellcheck
+declare stderr
+
+#----------
+
+@test "${FILE} crowdsec - print error on exit" {
+    # errors that cause program termination are printed to stderr, not only logs
+    yq e '.db_config.type="meh"' -i "${CONFIG_YAML}"
+    run -1 --separate-stderr "${BIN_DIR}/crowdsec"
+    refute_output
+    run -0 echo "${stderr}"
+    assert_output --partial "api server init: unable to run local API: unable to init database client: unknown database type 'meh'"
+}
+