avoid mutating config while parsing -config.file (#2392)

* avoid mutating config while parsing -config.file

* minimal config mutation test case

* logcli local query config file compat

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())

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", "", "yaml file to load"),
+		YAMLFlag(os.Args[1:], "config.file"),
 		Flags(),
 	)
 }

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(args []string, 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(args)
 		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)
+
 	}
 }

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)
+}

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
 	}