Browse code

Verify that the configuration keys in the file are valid.

- Return an error if any of the keys don't match valid flags.
- Fix an issue ignoring merged values as named values.
- Fix tlsverify configuration key.
- Fix bug in mflag to avoid panics when one of the flag set doesn't have any flag.

Signed-off-by: David Calavera <david.calavera@gmail.com>

David Calavera authored on 2016/01/21 07:16:49
Showing 8 changed files
... ...
@@ -80,7 +80,7 @@ type CommonConfig struct {
80 80
 	Hosts      []string         `json:"hosts,omitempty"`
81 81
 	LogLevel   string           `json:"log-level,omitempty"`
82 82
 	TLS        bool             `json:"tls,omitempty"`
83
-	TLSVerify  bool             `json:"tls-verify,omitempty"`
83
+	TLSVerify  bool             `json:"tlsverify,omitempty"`
84 84
 	TLSOptions CommonTLSOptions `json:"tls-opts,omitempty"`
85 85
 
86 86
 	reloadLock sync.Mutex
... ...
@@ -215,16 +215,43 @@ func configValuesSet(config map[string]interface{}) map[string]interface{} {
215 215
 }
216 216
 
217 217
 // findConfigurationConflicts iterates over the provided flags searching for
218
-// duplicated configurations. It returns an error with all the conflicts if
218
+// duplicated configurations and unknown keys. It returns an error with all the conflicts if
219 219
 // it finds any.
220 220
 func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagSet) error {
221
-	var conflicts []string
221
+	// 1. Search keys from the file that we don't recognize as flags.
222
+	unknownKeys := make(map[string]interface{})
223
+	for key, value := range config {
224
+		flagName := "-" + key
225
+		if flag := flags.Lookup(flagName); flag == nil {
226
+			unknownKeys[key] = value
227
+		}
228
+	}
229
+
230
+	// 2. Discard keys that might have a given name, like `labels`.
231
+	unknownNamedConflicts := func(f *flag.Flag) {
232
+		if namedOption, ok := f.Value.(opts.NamedOption); ok {
233
+			if _, valid := unknownKeys[namedOption.Name()]; valid {
234
+				delete(unknownKeys, namedOption.Name())
235
+			}
236
+		}
237
+	}
238
+	flags.VisitAll(unknownNamedConflicts)
239
+
240
+	if len(unknownKeys) > 0 {
241
+		var unknown []string
242
+		for key := range unknownKeys {
243
+			unknown = append(unknown, key)
244
+		}
245
+		return fmt.Errorf("the following directives don't match any configuration option: %s", strings.Join(unknown, ", "))
246
+	}
222 247
 
248
+	var conflicts []string
223 249
 	printConflict := func(name string, flagValue, fileValue interface{}) string {
224 250
 		return fmt.Sprintf("%s: (from flag: %v, from file: %v)", name, flagValue, fileValue)
225 251
 	}
226 252
 
227
-	collectConflicts := func(f *flag.Flag) {
253
+	// 3. Search keys that are present as a flag and as a file option.
254
+	duplicatedConflicts := func(f *flag.Flag) {
228 255
 		// search option name in the json configuration payload if the value is a named option
229 256
 		if namedOption, ok := f.Value.(opts.NamedOption); ok {
230 257
 			if optsValue, ok := config[namedOption.Name()]; ok {
... ...
@@ -243,7 +270,7 @@ func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagS
243 243
 		}
244 244
 	}
245 245
 
246
-	flags.Visit(collectConflicts)
246
+	flags.Visit(duplicatedConflicts)
247 247
 
248 248
 	if len(conflicts) > 0 {
249 249
 		return fmt.Errorf("the following directives are specified both as a flag and in the configuration file: %s", strings.Join(conflicts, ", "))
... ...
@@ -89,21 +89,16 @@ func TestFindConfigurationConflicts(t *testing.T) {
89 89
 	config := map[string]interface{}{"authorization-plugins": "foobar"}
90 90
 	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
91 91
 
92
-	err := findConfigurationConflicts(config, flags)
93
-	if err != nil {
94
-		t.Fatal(err)
95
-	}
96
-
97
-	flags.String([]string{"authorization-plugins"}, "", "")
98
-	if err := flags.Set("authorization-plugins", "asdf"); err != nil {
92
+	flags.String([]string{"-authorization-plugins"}, "", "")
93
+	if err := flags.Set("-authorization-plugins", "asdf"); err != nil {
99 94
 		t.Fatal(err)
100 95
 	}
101 96
 
102
-	err = findConfigurationConflicts(config, flags)
97
+	err := findConfigurationConflicts(config, flags)
103 98
 	if err == nil {
104 99
 		t.Fatal("expected error, got nil")
105 100
 	}
106
-	if !strings.Contains(err.Error(), "authorization-plugins") {
101
+	if !strings.Contains(err.Error(), "authorization-plugins: (from flag: asdf, from file: foobar)") {
107 102
 		t.Fatalf("expected authorization-plugins conflict, got %v", err)
108 103
 	}
109 104
 }
... ...
@@ -175,3 +170,41 @@ func TestDaemonConfigurationMergeConflictsWithInnerStructs(t *testing.T) {
175 175
 		t.Fatalf("expected tlscacert conflict, got %v", err)
176 176
 	}
177 177
 }
178
+
179
+func TestFindConfigurationConflictsWithUnknownKeys(t *testing.T) {
180
+	config := map[string]interface{}{"tls-verify": "true"}
181
+	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
182
+
183
+	flags.Bool([]string{"-tlsverify"}, false, "")
184
+	err := findConfigurationConflicts(config, flags)
185
+	if err == nil {
186
+		t.Fatal("expected error, got nil")
187
+	}
188
+	if !strings.Contains(err.Error(), "the following directives don't match any configuration option: tls-verify") {
189
+		t.Fatalf("expected tls-verify conflict, got %v", err)
190
+	}
191
+}
192
+
193
+func TestFindConfigurationConflictsWithMergedValues(t *testing.T) {
194
+	var hosts []string
195
+	config := map[string]interface{}{"hosts": "tcp://127.0.0.1:2345"}
196
+	base := mflag.NewFlagSet("base", mflag.ContinueOnError)
197
+	base.Var(opts.NewNamedListOptsRef("hosts", &hosts, nil), []string{"H", "-host"}, "")
198
+
199
+	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
200
+	mflag.Merge(flags, base)
201
+
202
+	err := findConfigurationConflicts(config, flags)
203
+	if err != nil {
204
+		t.Fatal(err)
205
+	}
206
+
207
+	flags.Set("-host", "unix:///var/run/docker.sock")
208
+	err = findConfigurationConflicts(config, flags)
209
+	if err == nil {
210
+		t.Fatal("expected error, got nil")
211
+	}
212
+	if !strings.Contains(err.Error(), "hosts: (from flag: [unix:///var/run/docker.sock], from file: tcp://127.0.0.1:2345)") {
213
+		t.Fatalf("expected hosts conflict, got %v", err)
214
+	}
215
+}
... ...
@@ -18,6 +18,7 @@ const (
18 18
 	defaultCaFile       = "ca.pem"
19 19
 	defaultKeyFile      = "key.pem"
20 20
 	defaultCertFile     = "cert.pem"
21
+	tlsVerifyKey        = "tlsverify"
21 22
 )
22 23
 
23 24
 var (
... ...
@@ -60,7 +61,7 @@ func postParseCommon() {
60 60
 	// Regardless of whether the user sets it to true or false, if they
61 61
 	// specify --tlsverify at all then we need to turn on tls
62 62
 	// TLSVerify can be true even if not set due to DOCKER_TLS_VERIFY env var, so we need to check that here as well
63
-	if cmd.IsSet("-tlsverify") || commonFlags.TLSVerify {
63
+	if cmd.IsSet("-"+tlsVerifyKey) || commonFlags.TLSVerify {
64 64
 		commonFlags.TLS = true
65 65
 	}
66 66
 
... ...
@@ -362,7 +362,7 @@ func loadDaemonCliConfig(config *daemon.Config, daemonFlags *flag.FlagSet, commo
362 362
 
363 363
 	// Regardless of whether the user sets it to true or false, if they
364 364
 	// specify TLSVerify at all then we need to turn on TLS
365
-	if config.IsValueSet("tls-verify") {
365
+	if config.IsValueSet(tlsVerifyKey) {
366 366
 		config.TLS = true
367 367
 	}
368 368
 
... ...
@@ -105,10 +105,11 @@ func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) {
105 105
 	}
106 106
 
107 107
 	configFile := f.Name()
108
-	f.Write([]byte(`{"tls-verify": true}`))
108
+	f.Write([]byte(`{"tlsverify": true}`))
109 109
 	f.Close()
110 110
 
111 111
 	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
112
+	flags.Bool([]string{"-tlsverify"}, false, "")
112 113
 	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
113 114
 	if err != nil {
114 115
 		t.Fatal(err)
... ...
@@ -136,10 +137,11 @@ func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) {
136 136
 	}
137 137
 
138 138
 	configFile := f.Name()
139
-	f.Write([]byte(`{"tls-verify": false}`))
139
+	f.Write([]byte(`{"tlsverify": false}`))
140 140
 	f.Close()
141 141
 
142 142
 	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
143
+	flags.Bool([]string{"-tlsverify"}, false, "")
143 144
 	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
144 145
 	if err != nil {
145 146
 		t.Fatal(err)
... ...
@@ -198,6 +200,7 @@ func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) {
198 198
 	f.Close()
199 199
 
200 200
 	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
201
+	flags.String([]string{"-log-level"}, "", "")
201 202
 	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
202 203
 	if err != nil {
203 204
 		t.Fatal(err)
... ...
@@ -213,3 +216,30 @@ func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) {
213 213
 		t.Fatalf("expected warn log level, got %v", logrus.GetLevel())
214 214
 	}
215 215
 }
216
+
217
+func TestLoadDaemonCliConfigWithTLSOptions(t *testing.T) {
218
+	c := &daemon.Config{}
219
+	common := &cli.CommonFlags{}
220
+
221
+	f, err := ioutil.TempFile("", "docker-config-")
222
+	if err != nil {
223
+		t.Fatal(err)
224
+	}
225
+
226
+	configFile := f.Name()
227
+	f.Write([]byte(`{"tls-opts": {"tlscacert": "/etc/certs/ca.pem"}}`))
228
+	f.Close()
229
+
230
+	flags := mflag.NewFlagSet("test", mflag.ContinueOnError)
231
+	flags.String([]string{"-tlscacert"}, "", "")
232
+	loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile)
233
+	if err != nil {
234
+		t.Fatal(err)
235
+	}
236
+	if loadedConfig == nil {
237
+		t.Fatalf("expected configuration %v, got nil", c)
238
+	}
239
+	if loadedConfig.TLSOptions.CAFile != "/etc/certs/ca.pem" {
240
+		t.Fatalf("expected CA file path /etc/certs/ca.pem, got %v", loadedConfig.TLSOptions.CAFile)
241
+	}
242
+}
... ...
@@ -852,7 +852,7 @@ This is a full example of the allowed configuration options in the file:
852 852
 	"hosts": [],
853 853
 	"log-level": "",
854 854
 	"tls": true,
855
-	"tls-verify": true,
855
+	"tlsverify": true,
856 856
 	"tls-opts": {
857 857
 		"tlscacert": "",
858 858
 		"tlscert": "",
... ...
@@ -1223,11 +1223,27 @@ func (v mergeVal) IsBoolFlag() bool {
1223 1223
 	return false
1224 1224
 }
1225 1225
 
1226
+// Name returns the name of a mergeVal.
1227
+// If the original value had a name, return the original name,
1228
+// otherwise, return the key asinged to this mergeVal.
1229
+func (v mergeVal) Name() string {
1230
+	type namedValue interface {
1231
+		Name() string
1232
+	}
1233
+	if nVal, ok := v.Value.(namedValue); ok {
1234
+		return nVal.Name()
1235
+	}
1236
+	return v.key
1237
+}
1238
+
1226 1239
 // Merge is an helper function that merges n FlagSets into a single dest FlagSet
1227 1240
 // In case of name collision between the flagsets it will apply
1228 1241
 // the destination FlagSet's errorHandling behavior.
1229 1242
 func Merge(dest *FlagSet, flagsets ...*FlagSet) error {
1230 1243
 	for _, fset := range flagsets {
1244
+		if fset.formal == nil {
1245
+			continue
1246
+		}
1231 1247
 		for k, f := range fset.formal {
1232 1248
 			if _, ok := dest.formal[k]; ok {
1233 1249
 				var err error
... ...
@@ -1249,6 +1265,9 @@ func Merge(dest *FlagSet, flagsets ...*FlagSet) error {
1249 1249
 			}
1250 1250
 			newF := *f
1251 1251
 			newF.Value = mergeVal{f.Value, k, fset}
1252
+			if dest.formal == nil {
1253
+				dest.formal = make(map[string]*Flag)
1254
+			}
1252 1255
 			dest.formal[k] = &newF
1253 1256
 		}
1254 1257
 	}
... ...
@@ -514,3 +514,14 @@ func TestSortFlags(t *testing.T) {
514 514
 		t.Fatalf("NFlag (%d) != fs.NFlag() (%d) of elements visited", nflag, fs.NFlag())
515 515
 	}
516 516
 }
517
+
518
+func TestMergeFlags(t *testing.T) {
519
+	base := NewFlagSet("base", ContinueOnError)
520
+	base.String([]string{"f"}, "", "")
521
+
522
+	fs := NewFlagSet("test", ContinueOnError)
523
+	Merge(fs, base)
524
+	if len(fs.formal) != 1 {
525
+		t.Fatalf("FlagCount (%d) != number (1) of elements merged", len(fs.formal))
526
+	}
527
+}