From 51800132cd831146dab155b0c7839881dc0ce05c Mon Sep 17 00:00:00 2001 From: mmetc <92726601+mmetc@users.noreply.github.com> Date: Fri, 13 Jan 2023 13:42:42 +0100 Subject: [PATCH] improve feature flag logging (#1986) For cscli: it should provide a terse output, not nag users with configuration details. Although it's usually important that cscli and crowdsec have the same enabled features, having it list them every time the command is invoked can be too much. For crowdsec: when features are set from the environment, it's too early to log where we should. So we can use log.Debug at activation time, and list them again once logging is configured. - wrap some functions in csconfig for convenience and DRY - for each enabled feature, log.Debug - log all enabled features once as Info (crowdsec) or Debug (cscli) - file does not exist -> log.Trace --- cmd/crowdsec-cli/main.go | 17 +++++++++------ cmd/crowdsec/main.go | 34 ++++++----------------------- pkg/csconfig/fflag.go | 44 ++++++++++++++++++++++++++++++++++++++ pkg/fflag/features.go | 6 +++--- pkg/fflag/features_test.go | 6 +++--- 5 files changed, 67 insertions(+), 40 deletions(-) create mode 100644 pkg/csconfig/fflag.go diff --git a/cmd/crowdsec-cli/main.go b/cmd/crowdsec-cli/main.go index d8d45b5a2..b1092565e 100644 --- a/cmd/crowdsec-cli/main.go +++ b/cmd/crowdsec-cli/main.go @@ -66,9 +66,14 @@ func initConfig() { csConfig = csconfig.NewDefaultConfig() } - featurePath := filepath.Join(csConfig.ConfigPaths.ConfigDir, "feature.yaml") - if err = fflag.Crowdsec.SetFromYamlFile(featurePath, log.StandardLogger()); err != nil { - log.Fatalf("File %s: %s", featurePath, err) + if err := csconfig.LoadFeatureFlagsFile(csConfig, log.StandardLogger()); err != nil { + log.Fatal(err) + } + + // recap of the enabled feature flags, because logging + // was not enabled when we set them from envvars + if fflist := csconfig.ListFeatureFlags(); fflist != "" { + log.Debugf("Enabled feature flags: %s", fflist) } if csConfig.Cscli == nil { @@ -141,10 +146,8 @@ func main() { log.Fatalf("failed to register features: %s", err) } - // some features can require configuration or command-line options, - // so we need to parse them asap. we'll load from feature.yaml later. - if err := fflag.Crowdsec.SetFromEnv(log.StandardLogger()); err != nil { - log.Fatalf("failed to set features from environment: %s", err) + if err := csconfig.LoadFeatureFlagsEnv(log.StandardLogger()); err != nil { + log.Fatalf("failed to set feature flags from env: %s", err) } var rootCmd = &cobra.Command{ diff --git a/cmd/crowdsec/main.go b/cmd/crowdsec/main.go index 3544857ba..05d206168 100644 --- a/cmd/crowdsec/main.go +++ b/cmd/crowdsec/main.go @@ -5,7 +5,6 @@ import ( "fmt" _ "net/http/pprof" "os" - "path/filepath" "runtime" "strings" "time" @@ -261,35 +260,16 @@ func LoadConfig(cConfig *csconfig.Config) error { return err } - err := LoadFeatureFlags(cConfig, log.StandardLogger()) - if err != nil { + if err := csconfig.LoadFeatureFlagsFile(cConfig, log.StandardLogger()); err != nil { return err } - return nil -} - - -// LoadFeatureFlags parses {ConfigDir}/feature.yaml to enable/disable features. -// -// Since CROWDSEC_FEATURE_ envvars are parsed before config.yaml, -// when the logger is not yet initialized, we also log here a recap -// of what has been enabled. -func LoadFeatureFlags(cConfig *csconfig.Config, logger *log.Logger) error { - featurePath := filepath.Join(cConfig.ConfigPaths.ConfigDir, "feature.yaml") - - if err := fflag.Crowdsec.SetFromYamlFile(featurePath, logger); err != nil { - return fmt.Errorf("file %s: %s", featurePath, err) + // recap of the enabled feature flags, because logging + // was not enabled when we set them from envvars + if fflist := csconfig.ListFeatureFlags(); fflist != "" { + log.Infof("Enabled feature flags: %s", fflist) } - enabledFeatures := fflag.Crowdsec.GetEnabledFeatures() - - msg := "" - if len(enabledFeatures) > 0 { - msg = strings.Join(enabledFeatures, ", ") - } - logger.Infof("Enabled features: %s", msg) - return nil } @@ -324,8 +304,8 @@ func main() { // some features can require configuration or command-line options, // so wwe need to parse them asap. we'll load from feature.yaml later. - if err := fflag.Crowdsec.SetFromEnv(log.StandardLogger()); err != nil { - log.Fatalf("failed set features from environment: %s", err) + if err := csconfig.LoadFeatureFlagsEnv(log.StandardLogger()); err != nil { + log.Fatalf("failed to set feature flags from environment: %s", err) } crowdsecT0 = time.Now() diff --git a/pkg/csconfig/fflag.go b/pkg/csconfig/fflag.go new file mode 100644 index 000000000..3419669bb --- /dev/null +++ b/pkg/csconfig/fflag.go @@ -0,0 +1,44 @@ +package csconfig + +import ( + "fmt" + "path/filepath" + "strings" + + log "github.com/sirupsen/logrus" + + "github.com/crowdsecurity/crowdsec/pkg/fflag" +) + + +// LoadFeatureFlagsEnv parses the environment variables to enable feature flags. +func LoadFeatureFlagsEnv(logger *log.Logger) error { + if err := fflag.Crowdsec.SetFromEnv(logger); err != nil { + return err + } + return nil +} + + +// LoadFeatureFlags parses {ConfigDir}/feature.yaml to enable feature flags. +func LoadFeatureFlagsFile(cConfig *Config, logger *log.Logger) error { + featurePath := filepath.Join(cConfig.ConfigPaths.ConfigDir, "feature.yaml") + + if err := fflag.Crowdsec.SetFromYamlFile(featurePath, logger); err != nil { + return fmt.Errorf("file %s: %s", featurePath, err) + } + return nil +} + + +// ListFeatureFlags returns a list of the enabled feature flags. +func ListFeatureFlags() string { + enabledFeatures := fflag.Crowdsec.GetEnabledFeatures() + + msg := "" + if len(enabledFeatures) > 0 { + msg = strings.Join(enabledFeatures, ", ") + } + + return msg +} diff --git a/pkg/fflag/features.go b/pkg/fflag/features.go index 5373c4210..e6a49a38d 100644 --- a/pkg/fflag/features.go +++ b/pkg/fflag/features.go @@ -181,7 +181,7 @@ func (fr *FeatureRegister) SetFromEnv(logger *logrus.Logger) error { return err } - logger.Infof("Feature flag: %s=%t (from envvar). %s", featureName, enable, feat.Description) + logger.Debugf("Feature flag: %s=%t (from envvar). %s", featureName, enable, feat.Description) } return nil @@ -224,7 +224,7 @@ func (fr *FeatureRegister) SetFromYaml(r io.Reader, logger *logrus.Logger) error return err } - logger.Infof("Feature flag: %s=true (from config file). %s", k, feat.Description) + logger.Debugf("Feature flag: %s=true (from config file). %s", k, feat.Description) } return nil @@ -234,7 +234,7 @@ func (fr *FeatureRegister) SetFromYamlFile(path string, logger *logrus.Logger) e f, err := os.Open(path) if err != nil { if os.IsNotExist(err) { - logger.Debugf("Feature flags config file '%s' does not exist", path) + logger.Tracef("Feature flags config file '%s' does not exist", path) return nil } diff --git a/pkg/fflag/features_test.go b/pkg/fflag/features_test.go index 8d153931b..aac8c079a 100644 --- a/pkg/fflag/features_test.go +++ b/pkg/fflag/features_test.go @@ -286,7 +286,7 @@ func TestSetFromEnv(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { logger, hook := logtest.NewNullLogger() - logger.SetLevel(logrus.InfoLevel) + logger.SetLevel(logrus.DebugLevel) t.Setenv(tc.envvar, tc.value) err := fr.SetFromEnv(logger) cstest.RequireErrorMessage(t, err, tc.expectedErr) @@ -346,7 +346,7 @@ func TestSetFromYaml(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { logger, hook := logtest.NewNullLogger() - logger.SetLevel(logrus.InfoLevel) + logger.SetLevel(logrus.DebugLevel) err := fr.SetFromYaml(strings.NewReader(tc.yml), logger) cstest.RequireErrorMessage(t, err, tc.expectedErr) for _, expectedMessage := range tc.expectedLog { @@ -369,7 +369,7 @@ func TestSetFromYamlFile(t *testing.T) { fr := setUp(t) logger, hook := logtest.NewNullLogger() - logger.SetLevel(logrus.InfoLevel) + logger.SetLevel(logrus.DebugLevel) err = fr.SetFromYamlFile(tmpfile.Name(), logger) require.NoError(t, err)