Browse code

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 <david.calavera@gmail.com>

David Calavera authored on 2016/02/19 06:55:03
Showing 4 changed files
... ...
@@ -154,14 +154,20 @@ func parseClusterAdvertiseSettings(clusterStore, clusterAdvertise string) (strin
154 154
 }
155 155
 
156 156
 // ReloadConfiguration reads the configuration in the host and reloads the daemon and server.
157
-func ReloadConfiguration(configFile string, flags *flag.FlagSet, reload func(*Config)) {
157
+func ReloadConfiguration(configFile string, flags *flag.FlagSet, reload func(*Config)) error {
158 158
 	logrus.Infof("Got signal to reload configuration, reloading from: %s", configFile)
159 159
 	newConfig, err := getConflictFreeConfiguration(configFile, flags)
160 160
 	if err != nil {
161
-		logrus.Error(err)
162
-	} else {
163
-		reload(newConfig)
161
+		return err
164 162
 	}
163
+	reload(newConfig)
164
+	return nil
165
+}
166
+
167
+// boolValue is an interface that boolean value flags implement
168
+// to tell the command line how to make -name equivalent to -name=true.
169
+type boolValue interface {
170
+	IsBoolFlag() bool
165 171
 }
166 172
 
167 173
 // MergeDaemonConfigurations reads a configuration file,
... ...
@@ -206,6 +212,36 @@ func getConflictFreeConfiguration(configFile string, flags *flag.FlagSet) (*Conf
206 206
 			return nil, err
207 207
 		}
208 208
 
209
+		// Override flag values to make sure the values set in the config file with nullable values, like `false`,
210
+		// are not overriden by default truthy values from the flags that were not explicitly set.
211
+		// See https://github.com/docker/docker/issues/20289 for an example.
212
+		//
213
+		// TODO: Rewrite configuration logic to avoid same issue with other nullable values, like numbers.
214
+		namedOptions := make(map[string]interface{})
215
+		for key, value := range configSet {
216
+			f := flags.Lookup("-" + key)
217
+			if f == nil { // ignore named flags that don't match
218
+				namedOptions[key] = value
219
+				continue
220
+			}
221
+
222
+			if _, ok := f.Value.(boolValue); ok {
223
+				f.Value.Set(fmt.Sprintf("%v", value))
224
+			}
225
+		}
226
+		if len(namedOptions) > 0 {
227
+			// set also default for mergeVal flags that are boolValue at the same time.
228
+			flags.VisitAll(func(f *flag.Flag) {
229
+				if opt, named := f.Value.(opts.NamedOption); named {
230
+					v, set := namedOptions[opt.Name()]
231
+					_, boolean := f.Value.(boolValue)
232
+					if set && boolean {
233
+						f.Value.Set(fmt.Sprintf("%v", v))
234
+					}
235
+				}
236
+			})
237
+		}
238
+
209 239
 		config.valuesSet = configSet
210 240
 	}
211 241
 
... ...
@@ -245,14 +281,16 @@ func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagS
245 245
 
246 246
 	// 2. Discard values that implement NamedOption.
247 247
 	// Their configuration name differs from their flag name, like `labels` and `label`.
248
-	unknownNamedConflicts := func(f *flag.Flag) {
249
-		if namedOption, ok := f.Value.(opts.NamedOption); ok {
250
-			if _, valid := unknownKeys[namedOption.Name()]; valid {
251
-				delete(unknownKeys, namedOption.Name())
248
+	if len(unknownKeys) > 0 {
249
+		unknownNamedConflicts := func(f *flag.Flag) {
250
+			if namedOption, ok := f.Value.(opts.NamedOption); ok {
251
+				if _, valid := unknownKeys[namedOption.Name()]; valid {
252
+					delete(unknownKeys, namedOption.Name())
253
+				}
252 254
 			}
253 255
 		}
256
+		flags.VisitAll(unknownNamedConflicts)
254 257
 	}
255
-	flags.VisitAll(unknownNamedConflicts)
256 258
 
257 259
 	if len(unknownKeys) > 0 {
258 260
 		var unknown []string
... ...
@@ -291,3 +291,80 @@ func TestLoadDaemonConfigWithMapOptions(t *testing.T) {
291 291
 		t.Fatalf("expected log tag `test`, got %s", tag)
292 292
 	}
293 293
 }
294
+
295
+func TestLoadDaemonConfigWithTrueDefaultValues(t *testing.T) {
296
+	c := &daemon.Config{}
297
+	common := &cli.CommonFlags{}
298
+	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
299
+	flags.BoolVar(&c.EnableUserlandProxy, []string{"-userland-proxy"}, true, "")
300
+
301
+	f, err := ioutil.TempFile("", "docker-config-")
302
+	if err != nil {
303
+		t.Fatal(err)
304
+	}
305
+
306
+	if err := flags.ParseFlags([]string{}, false); err != nil {
307
+		t.Fatal(err)
308
+	}
309
+
310
+	configFile := f.Name()
311
+	f.Write([]byte(`{
312
+		"userland-proxy": false
313
+}`))
314
+	f.Close()
315
+
316
+	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
317
+	if err != nil {
318
+		t.Fatal(err)
319
+	}
320
+	if loadedConfig == nil {
321
+		t.Fatal("expected configuration, got nil")
322
+	}
323
+
324
+	if loadedConfig.EnableUserlandProxy {
325
+		t.Fatal("expected userland proxy to be disabled, got enabled")
326
+	}
327
+
328
+	// make sure reloading doesn't generate configuration
329
+	// conflicts after normalizing boolean values.
330
+	err = daemon.ReloadConfiguration(configFile, flags, func(reloadedConfig *daemon.Config) {
331
+		if reloadedConfig.EnableUserlandProxy {
332
+			t.Fatal("expected userland proxy to be disabled, got enabled")
333
+		}
334
+	})
335
+	if err != nil {
336
+		t.Fatal(err)
337
+	}
338
+}
339
+
340
+func TestLoadDaemonConfigWithTrueDefaultValuesLeaveDefaults(t *testing.T) {
341
+	c := &daemon.Config{}
342
+	common := &cli.CommonFlags{}
343
+	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
344
+	flags.BoolVar(&c.EnableUserlandProxy, []string{"-userland-proxy"}, true, "")
345
+
346
+	f, err := ioutil.TempFile("", "docker-config-")
347
+	if err != nil {
348
+		t.Fatal(err)
349
+	}
350
+
351
+	if err := flags.ParseFlags([]string{}, false); err != nil {
352
+		t.Fatal(err)
353
+	}
354
+
355
+	configFile := f.Name()
356
+	f.Write([]byte(`{}`))
357
+	f.Close()
358
+
359
+	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
360
+	if err != nil {
361
+		t.Fatal(err)
362
+	}
363
+	if loadedConfig == nil {
364
+		t.Fatal("expected configuration, got nil")
365
+	}
366
+
367
+	if !loadedConfig.EnableUserlandProxy {
368
+		t.Fatal("expected userland proxy to be enabled, got disabled")
369
+	}
370
+}
... ...
@@ -8,6 +8,7 @@ import (
8 8
 	"os/signal"
9 9
 	"syscall"
10 10
 
11
+	"github.com/Sirupsen/logrus"
11 12
 	apiserver "github.com/docker/docker/api/server"
12 13
 	"github.com/docker/docker/daemon"
13 14
 	"github.com/docker/docker/pkg/mflag"
... ...
@@ -58,7 +59,9 @@ func setupConfigReloadTrap(configFile string, flags *mflag.FlagSet, reload func(
58 58
 	signal.Notify(c, syscall.SIGHUP)
59 59
 	go func() {
60 60
 		for range c {
61
-			daemon.ReloadConfiguration(configFile, flags, reload)
61
+			if err := daemon.ReloadConfiguration(configFile, flags, reload); err != nil {
62
+				logrus.Error(err)
63
+			}
62 64
 		}
63 65
 	}()
64 66
 }
... ...
@@ -50,7 +50,9 @@ func setupConfigReloadTrap(configFile string, flags *mflag.FlagSet, reload func(
50 50
 			logrus.Debugf("Config reload - waiting signal at %s", ev)
51 51
 			for {
52 52
 				syscall.WaitForSingleObject(h, syscall.INFINITE)
53
-				daemon.ReloadConfiguration(configFile, flags, reload)
53
+				if err := daemon.ReloadConfiguration(configFile, flags, reload); err != nil {
54
+					logrus.Error(err)
55
+				}
54 56
 			}
55 57
 		}
56 58
 	}()