Merge pull request #20471 from calavera/userland_proxy_config_fix

Avoid setting default truthy values from flags that are not set.

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

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

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

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