From 00245403d958fa7bd345db0e7745daf77813dc00 Mon Sep 17 00:00:00 2001 From: sh0rez Date: Thu, 26 Sep 2019 16:26:27 +0200 Subject: [PATCH] feat: configuration source precedence (#980) This implements more sophisticated handling of our configuration: - Correct precedence: Flags set on the command line now precede values from YAML, which in turn precede the defaults from the `flag` package - Sticks to the `flagext.Registerer` pattern, as it is partly vendored from Cortex / Weaveworks code we cannot change easily --- cmd/loki/main.go | 42 ++++++++--------------- cmd/promtail/main.go | 27 ++++----------- pkg/cfg/cfg.go | 61 +++++++++++++++++++++++++++++++++ pkg/cfg/cfg_test.go | 43 +++++++++++++++++++++++ pkg/cfg/data_test.go | 33 ++++++++++++++++++ pkg/cfg/files.go | 68 ++++++++++++++++++++++++++++++++++++ pkg/cfg/flag.go | 43 +++++++++++++++++++++++ pkg/cfg/flag_test.go | 58 +++++++++++++++++++++++++++++++ pkg/cfg/precedence_test.go | 70 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 397 insertions(+), 48 deletions(-) create mode 100644 pkg/cfg/cfg.go create mode 100644 pkg/cfg/cfg_test.go create mode 100644 pkg/cfg/data_test.go create mode 100644 pkg/cfg/files.go create mode 100644 pkg/cfg/flag.go create mode 100644 pkg/cfg/flag_test.go create mode 100644 pkg/cfg/precedence_test.go diff --git a/cmd/loki/main.go b/cmd/loki/main.go index c061280360..abea6d2751 100644 --- a/cmd/loki/main.go +++ b/cmd/loki/main.go @@ -7,7 +7,7 @@ import ( "reflect" "github.com/go-kit/kit/log/level" - "github.com/grafana/loki/pkg/helpers" + "github.com/grafana/loki/pkg/cfg" "github.com/grafana/loki/pkg/loki" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/version" @@ -15,8 +15,6 @@ import ( "github.com/weaveworks/common/tracing" "github.com/cortexproject/cortex/pkg/util" - "github.com/cortexproject/cortex/pkg/util/flagext" - "github.com/grafana/loki/pkg/util/validation" ) @@ -25,48 +23,35 @@ func init() { } func main() { - var ( - cfg loki.Config - configFile = "" - ) - flag.StringVar(&configFile, "config.file", "", "Configuration file to load.") - flagext.RegisterFlags(&cfg) - printVersion := flag.Bool("version", false, "Print this builds version information") - flag.Parse() + var config loki.Config + if err := cfg.Parse(&config); err != nil { + level.Error(util.Logger).Log("msg", "parsing config", "error", err) + os.Exit(1) + } if *printVersion { fmt.Print(version.Print("loki")) os.Exit(0) } - // LimitsConfig has a customer UnmarshalYAML that will set the defaults to a global. // This global is set to the config passed into the last call to `NewOverrides`. If we don't // call it atleast once, the defaults are set to an empty struct. // We call it with the flag values so that the config file unmarshalling only overrides the values set in the config. - if _, err := validation.NewOverrides(cfg.LimitsConfig); err != nil { - level.Error(util.Logger).Log("msg", "error loading limits", "err", err) + if _, err := validation.NewOverrides(config.LimitsConfig); err != nil { + level.Error(util.Logger).Log("msg", "setting up overrides", "error", err) os.Exit(1) } - util.InitLogger(&cfg.Server) - - if configFile != "" { - if err := helpers.LoadConfig(configFile, &cfg); err != nil { - level.Error(util.Logger).Log("msg", "error loading config", "filename", configFile, "err", err) - os.Exit(1) - } - } - - // Re-init the logger which will now honor a different log level set in cfg.Server - if reflect.DeepEqual(&cfg.Server.LogLevel, &logging.Level{}) { + // Init the logger which will honor the log level set in config.Server + if reflect.DeepEqual(&config.Server.LogLevel, &logging.Level{}) { level.Error(util.Logger).Log("msg", "invalid log level") os.Exit(1) } - util.InitLogger(&cfg.Server) + util.InitLogger(&config.Server) // Setting the environment variable JAEGER_AGENT_HOST enables tracing - trace := tracing.NewFromEnv(fmt.Sprintf("loki-%s", cfg.Target)) + trace := tracing.NewFromEnv(fmt.Sprintf("loki-%s", config.Target)) defer func() { if err := trace.Close(); err != nil { level.Error(util.Logger).Log("msg", "error closing tracing", "err", err) @@ -74,7 +59,8 @@ func main() { } }() - t, err := loki.New(cfg) + // Start Loki + t, err := loki.New(config) if err != nil { level.Error(util.Logger).Log("msg", "error initialising loki", "err", err) os.Exit(1) diff --git a/cmd/promtail/main.go b/cmd/promtail/main.go index 4933ebc824..bda02a679d 100644 --- a/cmd/promtail/main.go +++ b/cmd/promtail/main.go @@ -7,13 +7,12 @@ import ( "reflect" "github.com/cortexproject/cortex/pkg/util" - "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/go-kit/kit/log/level" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/version" "github.com/weaveworks/common/logging" - "github.com/grafana/loki/pkg/helpers" + "github.com/grafana/loki/pkg/cfg" "github.com/grafana/loki/pkg/logentry/stages" "github.com/grafana/loki/pkg/promtail" "github.com/grafana/loki/pkg/promtail/config" @@ -24,31 +23,19 @@ func init() { } func main() { - var ( - configFile = "cmd/promtail/promtail-local-config.yaml" - config config.Config - ) - flag.StringVar(&configFile, "config.file", "promtail.yml", "The config file.") - flagext.RegisterFlags(&config) - printVersion := flag.Bool("version", false, "Print this builds version information") - flag.Parse() + var config config.Config + if err := cfg.Parse(&config); err != nil { + level.Error(util.Logger).Log("msg", "parsing config", "error", err) + os.Exit(1) + } if *printVersion { fmt.Print(version.Print("promtail")) os.Exit(0) } - util.InitLogger(&config.ServerConfig.Config) - - if configFile != "" { - if err := helpers.LoadConfig(configFile, &config); err != nil { - level.Error(util.Logger).Log("msg", "error loading config", "filename", configFile, "err", err) - os.Exit(1) - } - } - - // Re-init the logger which will now honor a different log level set in ServerConfig.Config + // Init the logger which will honor the log level set in cfg.Server if reflect.DeepEqual(&config.ServerConfig.Config.LogLevel, &logging.Level{}) { level.Error(util.Logger).Log("msg", "invalid log level") os.Exit(1) diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go new file mode 100644 index 0000000000..789c733a80 --- /dev/null +++ b/pkg/cfg/cfg.go @@ -0,0 +1,61 @@ +package cfg + +import ( + "reflect" + + "github.com/pkg/errors" +) + +// Source is a generic configuration source. This function may do whatever is +// required to obtain the configuration. It is passed a pointer to the +// destination, which will be something compatible to `json.Unmarshal`. The +// obtained configuration may be written to this object, it may also contain +// data from previous sources. +type Source func(interface{}) error + +var ( + ErrNotPointer = errors.New("dst is not a pointer") +) + +// Unmarshal merges the values of the various configuration sources and sets them on +// `dst`. The object must be compatible with `json.Unmarshal`. +func Unmarshal(dst interface{}, sources ...Source) error { + if len(sources) == 0 { + panic("No sources supplied to cfg.Unmarshal(). This is most likely a programming issue and should never happen. Check the code!") + } + if reflect.ValueOf(dst).Kind() != reflect.Ptr { + return ErrNotPointer + } + + for _, source := range sources { + if err := source(dst); err != nil { + return errors.Wrap(err, "sourcing") + } + } + return nil +} + +// Parse is a higher level wrapper for Unmarshal that automatically parses flags and a .yaml file +func Parse(dst interface{}) error { + return dParse(dst, + Defaults(), + YAMLFlag("config.file", "", "yaml file to load"), + Flags(), + ) +} + +// dParse is the same as Parse, but with dependency injection for testing +func dParse(dst interface{}, defaults, yaml, flags Source) error { + // check dst is a pointer + v := reflect.ValueOf(dst) + if v.Kind() != reflect.Ptr { + return ErrNotPointer + } + + // unmarshal config + return Unmarshal(dst, + defaults, + yaml, + flags, + ) +} diff --git a/pkg/cfg/cfg_test.go b/pkg/cfg/cfg_test.go new file mode 100644 index 0000000000..b0de7e4309 --- /dev/null +++ b/pkg/cfg/cfg_test.go @@ -0,0 +1,43 @@ +package cfg + +import ( + "flag" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParse(t *testing.T) { + yamlSource := dYAML([]byte(` +server: + port: 2000 + timeout: 60h +tls: + key: YAML +`)) + + fs := flag.NewFlagSet(t.Name(), flag.PanicOnError) + flagSource := dFlags(fs, []string{"-verbose", "-server.port=21"}) + + data := Data{} + err := dParse(&data, + dDefaults(fs), + yamlSource, + flagSource, + ) + require.NoError(t, err) + + assert.Equal(t, Data{ + Verbose: true, // flag + Server: Server{ + Port: 21, // flag + Timeout: 60 * time.Hour, // defaults + }, + TLS: TLS{ + Cert: "DEFAULTCERT", // defaults + Key: "YAML", // yaml + }, + }, data) +} diff --git a/pkg/cfg/data_test.go b/pkg/cfg/data_test.go new file mode 100644 index 0000000000..b262c492b1 --- /dev/null +++ b/pkg/cfg/data_test.go @@ -0,0 +1,33 @@ +package cfg + +import ( + "flag" + "time" +) + +// Data is a test Data structure +type Data struct { + Verbose bool `yaml:"verbose"` + Server Server `yaml:"server"` + TLS TLS `yaml:"tls"` +} + +type Server struct { + Port int `yaml:"port"` + Timeout time.Duration `yaml:"timeout"` +} + +type TLS struct { + Cert string `yaml:"cert"` + Key string `yaml:"key"` +} + +// RegisterFlags makes Data implement flagext.Registerer for using flags +func (d *Data) RegisterFlags(fs *flag.FlagSet) { + fs.BoolVar(&d.Verbose, "verbose", false, "") + fs.IntVar(&d.Server.Port, "server.port", 80, "") + fs.DurationVar(&d.Server.Timeout, "server.timeout", 60*time.Second, "") + + fs.StringVar(&d.TLS.Cert, "tls.cert", "DEFAULTCERT", "") + fs.StringVar(&d.TLS.Key, "tls.key", "DEFAULTKEY", "") +} diff --git a/pkg/cfg/files.go b/pkg/cfg/files.go new file mode 100644 index 0000000000..7cedbc81fd --- /dev/null +++ b/pkg/cfg/files.go @@ -0,0 +1,68 @@ +package cfg + +import ( + "encoding/json" + "flag" + "io/ioutil" + + yaml "gopkg.in/yaml.v2" +) + +// JSON returns a Source that opens the supplied `.json` file and loads it. +func JSON(f *string) Source { + return func(dst interface{}) error { + if f == nil { + return nil + } + + j, err := ioutil.ReadFile(*f) + if err != nil { + return err + } + + return dJSON(j)(dst) + } +} + +// dJSON returns a JSON source and allows dependency injection +func dJSON(y []byte) Source { + return func(dst interface{}) error { + return json.Unmarshal(y, dst) + } +} + +// YAML returns a Source that opens the supplied `.yaml` file and loads it. +func YAML(f *string) Source { + return func(dst interface{}) error { + if f == nil { + return nil + } + + y, err := ioutil.ReadFile(*f) + if err != nil { + return err + } + + return dYAML(y)(dst) + } +} + +// dYAML returns a YAML source and allows dependency injection +func dYAML(y []byte) Source { + return func(dst interface{}) error { + return yaml.Unmarshal(y, dst) + } +} + +// YAMLFlag defines a `config.file` flag and loads this file +func YAMLFlag(name, value, help string) Source { + return func(dst interface{}) error { + f := flag.String(name, value, help) + flag.Parse() + + if *f == "" { + f = nil + } + return YAML(f)(dst) + } +} diff --git a/pkg/cfg/flag.go b/pkg/cfg/flag.go new file mode 100644 index 0000000000..b895a7b408 --- /dev/null +++ b/pkg/cfg/flag.go @@ -0,0 +1,43 @@ +package cfg + +import ( + "flag" + "os" + + "github.com/cortexproject/cortex/pkg/util/flagext" + "github.com/pkg/errors" +) + +// Defaults registers flags to the command line using dst as the +// flagext.Registerer +func Defaults() Source { + return dDefaults(flag.CommandLine) +} + +// dDefaults registers flags to the flagSet using dst as the flagext.Registerer +func dDefaults(fs *flag.FlagSet) Source { + return func(dst interface{}) error { + r, ok := dst.(flagext.Registerer) + if !ok { + return errors.New("dst does not satisfy flagext.Registerer") + } + + // already sets the defaults on r + r.RegisterFlags(fs) + return nil + } +} + +// Flags parses the flag from the command line, setting only user-supplied +// values on the flagext.Registerer passed to Defaults() +func Flags() Source { + return dFlags(flag.CommandLine, os.Args[1:]) +} + +// dFlags parses the flagset, applying all values set on the slice +func dFlags(fs *flag.FlagSet, args []string) Source { + return func(dst interface{}) error { + // parse the final flagset + return fs.Parse(args) + } +} diff --git a/pkg/cfg/flag_test.go b/pkg/cfg/flag_test.go new file mode 100644 index 0000000000..8c45a7846e --- /dev/null +++ b/pkg/cfg/flag_test.go @@ -0,0 +1,58 @@ +package cfg + +import ( + "flag" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestDefaults checks that defaults are correctly obtained from a +// flagext.Registerer +func TestDefaults(t *testing.T) { + data := Data{} + fs := flag.NewFlagSet(t.Name(), flag.PanicOnError) + + err := Unmarshal(&data, + dDefaults(fs), + ) + + require.NoError(t, err) + assert.Equal(t, Data{ + Verbose: false, + Server: Server{ + Port: 80, + Timeout: 60 * time.Second, + }, + TLS: TLS{ + Cert: "DEFAULTCERT", + Key: "DEFAULTKEY", + }, + }, data) +} + +// TestFlags checks that defaults and flag values (they can't be separated) are +// correctly obtained from the command line +func TestFlags(t *testing.T) { + data := Data{} + fs := flag.NewFlagSet(t.Name(), flag.PanicOnError) + err := Unmarshal(&data, + dDefaults(fs), + dFlags(fs, []string{"-server.timeout=10h", "-verbose"}), + ) + require.NoError(t, err) + + assert.Equal(t, Data{ + Verbose: true, + Server: Server{ + Port: 80, + Timeout: 10 * time.Hour, + }, + TLS: TLS{ + Cert: "DEFAULTCERT", + Key: "DEFAULTKEY", + }, + }, data) +} diff --git a/pkg/cfg/precedence_test.go b/pkg/cfg/precedence_test.go new file mode 100644 index 0000000000..7232b6ff7b --- /dev/null +++ b/pkg/cfg/precedence_test.go @@ -0,0 +1,70 @@ +package cfg + +import ( + "flag" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// This file checks precedence rules are correctly working +// The default precedence recommended by this package is the following: +// flag defaults < yaml < user-set flags +// +// The following tests make sure that this is indeed correct + +const y = ` +verbose: true +tls: + cert: YAML +server: + port: 1234 +` + +func TestYAMLOverDefaults(t *testing.T) { + data := Data{} + fs := flag.NewFlagSet(t.Name(), flag.PanicOnError) + err := Unmarshal(&data, + dDefaults(fs), + dYAML([]byte(y)), + ) + + require.NoError(t, err) + assert.Equal(t, Data{ + Verbose: true, // yaml + Server: Server{ + Port: 1234, // yaml + Timeout: 60 * time.Second, // default + }, + TLS: TLS{ + Cert: "YAML", // yaml + Key: "DEFAULTKEY", // default + }, + }, data) +} + +func TestFlagOverYAML(t *testing.T) { + data := Data{} + fs := flag.NewFlagSet(t.Name(), flag.PanicOnError) + + err := Unmarshal(&data, + dDefaults(fs), + dYAML([]byte(y)), + dFlags(fs, []string{"-verbose=false", "-tls.cert=CLI"}), + ) + + require.NoError(t, err) + assert.Equal(t, Data{ + Verbose: false, // flag + Server: Server{ + Port: 1234, // yaml + Timeout: 60 * time.Second, // default + }, + TLS: TLS{ + Cert: "CLI", // flag + Key: "DEFAULTKEY", // default + }, + }, data) +}