From fee35171b7d89013dab590fe885204215f2dc345 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 22 Jul 2020 16:35:31 -0400 Subject: [PATCH 1/3] avoid mutating config while parsing -config.file --- cmd/loki/main.go | 40 ++++++++++++++++++++++++++++++--------- pkg/cfg/cfg.go | 2 +- pkg/cfg/files.go | 49 +++++++++++++++++++++++++++++------------------- 3 files changed, 62 insertions(+), 29 deletions(-) diff --git a/cmd/loki/main.go b/cmd/loki/main.go index ae5b7cf714f01..f7866c713a080 100644 --- a/cmd/loki/main.go +++ b/cmd/loki/main.go @@ -7,6 +7,7 @@ import ( "reflect" "strings" + "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" @@ -29,18 +30,39 @@ func init() { var lineReplacer = strings.NewReplacer("\n", "\\n ") -func main() { - printVersion := flag.Bool("version", false, "Print this builds version information") - printConfig := flag.Bool("print-config-stderr", false, "Dump the entire Loki config object to stderr") - logConfig := flag.Bool("log-config-reverse-order", false, "Dump the entire Loki config object at Info log "+ +type Config struct { + loki.Config `yaml:",inline"` + printVersion bool + printConfig bool + logConfig bool + configFile string +} + +func (c *Config) RegisterFlags(f *flag.FlagSet) { + f.BoolVar(&c.printVersion, "version", false, "Print this builds version information") + f.BoolVar(&c.printConfig, "print-config-stderr", false, "Dump the entire Loki config object to stderr") + f.BoolVar(&c.logConfig, "log-config-reverse-order", false, "Dump the entire Loki config object at Info log "+ "level with the order reversed, reversing the order makes viewing the entries easier in Grafana.") + f.StringVar(&c.configFile, "config.file", "", "yaml file to load") + c.Config.RegisterFlags(f) +} + +// Clone takes advantage of pass-by-value semantics to return a distinct *Config. +// This is primarily used to parse a different flag set without mutating the original *Config. +func (c *Config) Clone() flagext.Registerer { + return flagext.Registerer(func(c Config) *Config { + return &c + }(*c)) +} + +func main() { + var config Config - var config loki.Config if err := cfg.Parse(&config); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) os.Exit(1) } - if *printVersion { + if config.printVersion { fmt.Println(version.Print("loki")) os.Exit(0) } @@ -65,14 +87,14 @@ func main() { os.Exit(1) } - if *printConfig { + if config.printConfig { err := logutil.PrintConfig(os.Stderr, &config) if err != nil { level.Error(util.Logger).Log("msg", "failed to print config to stderr", "err", err.Error()) } } - if *logConfig { + if config.logConfig { err := logutil.LogConfig(&config) if err != nil { level.Error(util.Logger).Log("msg", "failed to log config object", "err", err.Error()) @@ -96,7 +118,7 @@ func main() { } // Start Loki - t, err := loki.New(config) + t, err := loki.New(config.Config) util.CheckFatal("initialising loki", err) level.Info(util.Logger).Log("msg", "Starting Loki", "version", version.Info()) diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go index dd4bd14af913c..d96f5eca4bc67 100644 --- a/pkg/cfg/cfg.go +++ b/pkg/cfg/cfg.go @@ -39,7 +39,7 @@ func Unmarshal(dst interface{}, sources ...Source) error { func Parse(dst interface{}) error { return dParse(dst, Defaults(), - YAMLFlag("config.file", "", "yaml file to load"), + YAMLFlag("config.file"), Flags(), ) } diff --git a/pkg/cfg/files.go b/pkg/cfg/files.go index c5ef8bbadb7db..b6212deb23873 100644 --- a/pkg/cfg/files.go +++ b/pkg/cfg/files.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "os" + "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/pkg/errors" "gopkg.in/yaml.v2" ) @@ -36,19 +37,15 @@ func dJSON(y []byte) Source { } // YAML returns a Source that opens the supplied `.yaml` file and loads it. -func YAML(f *string) Source { +func YAML(f string) Source { return func(dst interface{}) error { - if f == nil { - return nil - } - - y, err := ioutil.ReadFile(*f) + y, err := ioutil.ReadFile(f) if err != nil { return err } err = dYAML(y)(dst) - return errors.Wrap(err, *f) + return errors.Wrap(err, f) } } @@ -59,29 +56,43 @@ func dYAML(y []byte) Source { } } -// YAMLFlag defines a `config.file` flag and loads this file -func YAMLFlag(name, value, help string) Source { +func YAMLFlag(name string) Source { + type cloneable interface { + Clone() flagext.Registerer + } + return func(dst interface{}) error { - f := flag.String(name, value, help) + freshFlags := flag.NewFlagSet("config-file-loader", flag.ContinueOnError) - usage := flag.CommandLine.Usage - flag.CommandLine.Usage = func() { /* don't do anything by default, we will print usage ourselves, but only when requested. */ } + c, ok := dst.(cloneable) + if !ok { + return errors.New("dst does not satisfy cloneable") + } + + // Ensure we register flags on a copy of the config so as to not mutate it while + // parsing out the config file location. + c.Clone().RegisterFlags(freshFlags) + + usage := freshFlags.Usage + freshFlags.Usage = func() { /* don't do anything by default, we will print usage ourselves, but only when requested. */ } - flag.CommandLine.Init(flag.CommandLine.Name(), flag.ContinueOnError) - err := flag.CommandLine.Parse(os.Args[1:]) + err := freshFlags.Parse(os.Args[1:]) if err == flag.ErrHelp { // print available parameters to stdout, so that users can grep/less it easily - flag.CommandLine.SetOutput(os.Stdout) + freshFlags.SetOutput(os.Stdout) usage() os.Exit(2) } else if err != nil { - fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get list of available parameters") + fmt.Fprintln(freshFlags.Output(), "Run with -help to get list of available parameters") os.Exit(2) } - if *f == "" { - f = nil + f := freshFlags.Lookup(name) + if f == nil || f.Value.String() == "" { + return nil } - return YAML(f)(dst) + + return YAML(f.Value.String())(dst) + } } From 0d2821fdc0ca934a4cea8aaa8acc133cf95dbe5c Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Tue, 4 Aug 2020 13:22:27 -0400 Subject: [PATCH 2/3] minimal config mutation test case --- pkg/cfg/cfg.go | 3 ++- pkg/cfg/files.go | 4 ++-- pkg/cfg/files_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 pkg/cfg/files_test.go diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go index d96f5eca4bc67..1e89fee2f7afb 100644 --- a/pkg/cfg/cfg.go +++ b/pkg/cfg/cfg.go @@ -1,6 +1,7 @@ package cfg import ( + "os" "reflect" "github.com/pkg/errors" @@ -39,7 +40,7 @@ func Unmarshal(dst interface{}, sources ...Source) error { func Parse(dst interface{}) error { return dParse(dst, Defaults(), - YAMLFlag("config.file"), + YAMLFlag(os.Args[1:], "config.file"), Flags(), ) } diff --git a/pkg/cfg/files.go b/pkg/cfg/files.go index b6212deb23873..6e439add4cce1 100644 --- a/pkg/cfg/files.go +++ b/pkg/cfg/files.go @@ -56,7 +56,7 @@ func dYAML(y []byte) Source { } } -func YAMLFlag(name string) Source { +func YAMLFlag(args []string, name string) Source { type cloneable interface { Clone() flagext.Registerer } @@ -76,7 +76,7 @@ func YAMLFlag(name string) Source { usage := freshFlags.Usage freshFlags.Usage = func() { /* don't do anything by default, we will print usage ourselves, but only when requested. */ } - err := freshFlags.Parse(os.Args[1:]) + err := freshFlags.Parse(args) if err == flag.ErrHelp { // print available parameters to stdout, so that users can grep/less it easily freshFlags.SetOutput(os.Stdout) diff --git a/pkg/cfg/files_test.go b/pkg/cfg/files_test.go new file mode 100644 index 0000000000000..f697857ee2007 --- /dev/null +++ b/pkg/cfg/files_test.go @@ -0,0 +1,33 @@ +package cfg + +import ( + "flag" + "testing" + + "github.com/cortexproject/cortex/pkg/util/flagext" + "github.com/stretchr/testify/require" +) + +type testCfg struct { + v int +} + +func (cfg *testCfg) RegisterFlags(f *flag.FlagSet) { + cfg.v++ +} + +func (cfg *testCfg) Clone() flagext.Registerer { + return func(cfg testCfg) flagext.Registerer { + return &cfg + }(*cfg) +} + +func TestYAMLFlagDoesNotMutate(t *testing.T) { + cfg := &testCfg{} + err := YAMLFlag(nil, "something")(cfg) + require.Nil(t, err) + require.Equal(t, 0, cfg.v) + + cfg.RegisterFlags(nil) + require.Equal(t, 1, cfg.v) +} From 0c267f2d75228ddb80850abbe8a7bd463d64ce9d Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Tue, 4 Aug 2020 13:30:40 -0400 Subject: [PATCH 3/3] logcli local query config file compat --- pkg/logcli/query/query.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/logcli/query/query.go b/pkg/logcli/query/query.go index 54f5b94643d73..71ba0e874c96e 100644 --- a/pkg/logcli/query/query.go +++ b/pkg/logcli/query/query.go @@ -2,6 +2,7 @@ package query import ( "context" + "errors" "fmt" "log" "os" @@ -108,7 +109,10 @@ func (q *Query) DoLocalQuery(out output.LogOutput, statistics bool, orgID string if err := cfg.Defaults()(&conf); err != nil { return err } - if err := cfg.YAML(&q.LocalConfig)(&conf); err != nil { + if q.LocalConfig == "" { + return errors.New("no supplied config file") + } + if err := cfg.YAML(q.LocalConfig)(&conf); err != nil { return err }