From 31cb96dcfaaebe3f807e7c7bf82a48b5995c743b Mon Sep 17 00:00:00 2001 From: David Calavera Date: Thu, 18 Feb 2016 16:55:03 -0500 Subject: [PATCH] Avoid setting default truthy values from flags that are not set. When the value for a configuration option in the file is `false`, and the default value for a flag is `true`, we should not take the value from the later as final value for the option, because the user explicitly set `false`. This change overrides the default value in the flagSet with the value in the configuration file so we get the correct result when we merge the two configurations together. Signed-off-by: David Calavera --- daemon/config.go | 56 ++++++++++++++++++++++++----- docker/daemon_test.go | 77 ++++++++++++++++++++++++++++++++++++++++ docker/daemon_unix.go | 5 ++- docker/daemon_windows.go | 4 ++- 4 files changed, 131 insertions(+), 11 deletions(-) diff --git a/daemon/config.go b/daemon/config.go index fb46f26667..50e814ba4e 100644 --- a/daemon/config.go +++ b/daemon/config.go @@ -154,14 +154,20 @@ func parseClusterAdvertiseSettings(clusterStore, clusterAdvertise string) (strin } // ReloadConfiguration reads the configuration in the host and reloads the daemon and server. -func ReloadConfiguration(configFile string, flags *flag.FlagSet, reload func(*Config)) { +func ReloadConfiguration(configFile string, flags *flag.FlagSet, reload func(*Config)) error { logrus.Infof("Got signal to reload configuration, reloading from: %s", configFile) newConfig, err := getConflictFreeConfiguration(configFile, flags) if err != nil { - logrus.Error(err) - } else { - reload(newConfig) + return err } + reload(newConfig) + return nil +} + +// boolValue is an interface that boolean value flags implement +// to tell the command line how to make -name equivalent to -name=true. +type boolValue interface { + IsBoolFlag() bool } // MergeDaemonConfigurations reads a configuration file, @@ -206,6 +212,36 @@ func getConflictFreeConfiguration(configFile string, flags *flag.FlagSet) (*Conf return nil, err } + // Override flag values to make sure the values set in the config file with nullable values, like `false`, + // are not overriden by default truthy values from the flags that were not explicitly set. + // See https://github.com/docker/docker/issues/20289 for an example. + // + // TODO: Rewrite configuration logic to avoid same issue with other nullable values, like numbers. + namedOptions := make(map[string]interface{}) + for key, value := range configSet { + f := flags.Lookup("-" + key) + if f == nil { // ignore named flags that don't match + namedOptions[key] = value + continue + } + + if _, ok := f.Value.(boolValue); ok { + f.Value.Set(fmt.Sprintf("%v", value)) + } + } + if len(namedOptions) > 0 { + // set also default for mergeVal flags that are boolValue at the same time. + flags.VisitAll(func(f *flag.Flag) { + if opt, named := f.Value.(opts.NamedOption); named { + v, set := namedOptions[opt.Name()] + _, boolean := f.Value.(boolValue) + if set && boolean { + f.Value.Set(fmt.Sprintf("%v", v)) + } + } + }) + } + config.valuesSet = configSet } @@ -245,14 +281,16 @@ func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagS // 2. Discard values that implement NamedOption. // Their configuration name differs from their flag name, like `labels` and `label`. - unknownNamedConflicts := func(f *flag.Flag) { - if namedOption, ok := f.Value.(opts.NamedOption); ok { - if _, valid := unknownKeys[namedOption.Name()]; valid { - delete(unknownKeys, namedOption.Name()) + if len(unknownKeys) > 0 { + unknownNamedConflicts := func(f *flag.Flag) { + if namedOption, ok := f.Value.(opts.NamedOption); ok { + if _, valid := unknownKeys[namedOption.Name()]; valid { + delete(unknownKeys, namedOption.Name()) + } } } + flags.VisitAll(unknownNamedConflicts) } - flags.VisitAll(unknownNamedConflicts) if len(unknownKeys) > 0 { var unknown []string diff --git a/docker/daemon_test.go b/docker/daemon_test.go index 5afdfb3bde..1be2ab8164 100644 --- a/docker/daemon_test.go +++ b/docker/daemon_test.go @@ -291,3 +291,80 @@ func TestLoadDaemonConfigWithMapOptions(t *testing.T) { t.Fatalf("expected log tag `test`, got %s", tag) } } + +func TestLoadDaemonConfigWithTrueDefaultValues(t *testing.T) { + c := &daemon.Config{} + common := &cli.CommonFlags{} + flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.BoolVar(&c.EnableUserlandProxy, []string{"-userland-proxy"}, true, "") + + f, err := ioutil.TempFile("", "docker-config-") + if err != nil { + t.Fatal(err) + } + + if err := flags.ParseFlags([]string{}, false); err != nil { + t.Fatal(err) + } + + configFile := f.Name() + f.Write([]byte(`{ + "userland-proxy": false +}`)) + f.Close() + + loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) + if err != nil { + t.Fatal(err) + } + if loadedConfig == nil { + t.Fatal("expected configuration, got nil") + } + + if loadedConfig.EnableUserlandProxy { + t.Fatal("expected userland proxy to be disabled, got enabled") + } + + // make sure reloading doesn't generate configuration + // conflicts after normalizing boolean values. + err = daemon.ReloadConfiguration(configFile, flags, func(reloadedConfig *daemon.Config) { + if reloadedConfig.EnableUserlandProxy { + t.Fatal("expected userland proxy to be disabled, got enabled") + } + }) + if err != nil { + t.Fatal(err) + } +} + +func TestLoadDaemonConfigWithTrueDefaultValuesLeaveDefaults(t *testing.T) { + c := &daemon.Config{} + common := &cli.CommonFlags{} + flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.BoolVar(&c.EnableUserlandProxy, []string{"-userland-proxy"}, true, "") + + f, err := ioutil.TempFile("", "docker-config-") + if err != nil { + t.Fatal(err) + } + + if err := flags.ParseFlags([]string{}, false); err != nil { + t.Fatal(err) + } + + configFile := f.Name() + f.Write([]byte(`{}`)) + f.Close() + + loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) + if err != nil { + t.Fatal(err) + } + if loadedConfig == nil { + t.Fatal("expected configuration, got nil") + } + + if !loadedConfig.EnableUserlandProxy { + t.Fatal("expected userland proxy to be enabled, got disabled") + } +} diff --git a/docker/daemon_unix.go b/docker/daemon_unix.go index a89bdc73bb..c76700f014 100644 --- a/docker/daemon_unix.go +++ b/docker/daemon_unix.go @@ -8,6 +8,7 @@ import ( "os/signal" "syscall" + "github.com/Sirupsen/logrus" apiserver "github.com/docker/docker/api/server" "github.com/docker/docker/daemon" "github.com/docker/docker/pkg/mflag" @@ -58,7 +59,9 @@ func setupConfigReloadTrap(configFile string, flags *mflag.FlagSet, reload func( signal.Notify(c, syscall.SIGHUP) go func() { for range c { - daemon.ReloadConfiguration(configFile, flags, reload) + if err := daemon.ReloadConfiguration(configFile, flags, reload); err != nil { + logrus.Error(err) + } } }() } diff --git a/docker/daemon_windows.go b/docker/daemon_windows.go index 307bbcc39b..52649daf0b 100644 --- a/docker/daemon_windows.go +++ b/docker/daemon_windows.go @@ -50,7 +50,9 @@ func setupConfigReloadTrap(configFile string, flags *mflag.FlagSet, reload func( logrus.Debugf("Config reload - waiting signal at %s", ev) for { syscall.WaitForSingleObject(h, syscall.INFINITE) - daemon.ReloadConfiguration(configFile, flags, reload) + if err := daemon.ReloadConfiguration(configFile, flags, reload); err != nil { + logrus.Error(err) + } } } }()