diff --git a/daemon/config.go b/daemon/config.go index 04931b24e9..ee0d41694c 100644 --- a/daemon/config.go +++ b/daemon/config.go @@ -56,7 +56,6 @@ type CommonConfig struct { GraphDriver string `json:"storage-driver,omitempty"` GraphOptions []string `json:"storage-opts,omitempty"` Labels []string `json:"labels,omitempty"` - LogConfig LogConfig `json:"log-config,omitempty"` Mtu int `json:"mtu,omitempty"` Pidfile string `json:"pidfile,omitempty"` Root string `json:"graph,omitempty"` @@ -76,12 +75,16 @@ type CommonConfig struct { // reachable by other hosts. ClusterAdvertise string `json:"cluster-advertise,omitempty"` - Debug bool `json:"debug,omitempty"` - Hosts []string `json:"hosts,omitempty"` - LogLevel string `json:"log-level,omitempty"` - TLS bool `json:"tls,omitempty"` - TLSVerify bool `json:"tls-verify,omitempty"` - TLSOptions CommonTLSOptions `json:"tls-opts,omitempty"` + Debug bool `json:"debug,omitempty"` + Hosts []string `json:"hosts,omitempty"` + LogLevel string `json:"log-level,omitempty"` + TLS bool `json:"tls,omitempty"` + TLSVerify bool `json:"tlsverify,omitempty"` + + // Embedded structs that allow config + // deserialization without the full struct. + CommonTLSOptions + LogConfig reloadLock sync.Mutex valuesSet map[string]interface{} @@ -215,16 +218,44 @@ func configValuesSet(config map[string]interface{}) map[string]interface{} { } // findConfigurationConflicts iterates over the provided flags searching for -// duplicated configurations. It returns an error with all the conflicts if +// duplicated configurations and unknown keys. It returns an error with all the conflicts if // it finds any. func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagSet) error { - var conflicts []string + // 1. Search keys from the file that we don't recognize as flags. + unknownKeys := make(map[string]interface{}) + for key, value := range config { + flagName := "-" + key + if flag := flags.Lookup(flagName); flag == nil { + unknownKeys[key] = value + } + } + // 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()) + } + } + } + flags.VisitAll(unknownNamedConflicts) + + if len(unknownKeys) > 0 { + var unknown []string + for key := range unknownKeys { + unknown = append(unknown, key) + } + return fmt.Errorf("the following directives don't match any configuration option: %s", strings.Join(unknown, ", ")) + } + + var conflicts []string printConflict := func(name string, flagValue, fileValue interface{}) string { return fmt.Sprintf("%s: (from flag: %v, from file: %v)", name, flagValue, fileValue) } - collectConflicts := func(f *flag.Flag) { + // 3. Search keys that are present as a flag and as a file option. + duplicatedConflicts := func(f *flag.Flag) { // search option name in the json configuration payload if the value is a named option if namedOption, ok := f.Value.(opts.NamedOption); ok { if optsValue, ok := config[namedOption.Name()]; ok { @@ -243,7 +274,7 @@ func findConfigurationConflicts(config map[string]interface{}, flags *flag.FlagS } } - flags.Visit(collectConflicts) + flags.Visit(duplicatedConflicts) if len(conflicts) > 0 { return fmt.Errorf("the following directives are specified both as a flag and in the configuration file: %s", strings.Join(conflicts, ", ")) diff --git a/daemon/config_test.go b/daemon/config_test.go index 69a199e162..dc1c3bc7da 100644 --- a/daemon/config_test.go +++ b/daemon/config_test.go @@ -89,21 +89,16 @@ func TestFindConfigurationConflicts(t *testing.T) { config := map[string]interface{}{"authorization-plugins": "foobar"} flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.String([]string{"-authorization-plugins"}, "", "") + if err := flags.Set("-authorization-plugins", "asdf"); err != nil { + t.Fatal(err) + } + err := findConfigurationConflicts(config, flags) - if err != nil { - t.Fatal(err) - } - - flags.String([]string{"authorization-plugins"}, "", "") - if err := flags.Set("authorization-plugins", "asdf"); err != nil { - t.Fatal(err) - } - - err = findConfigurationConflicts(config, flags) if err == nil { t.Fatal("expected error, got nil") } - if !strings.Contains(err.Error(), "authorization-plugins") { + if !strings.Contains(err.Error(), "authorization-plugins: (from flag: asdf, from file: foobar)") { t.Fatalf("expected authorization-plugins conflict, got %v", err) } } @@ -175,3 +170,41 @@ func TestDaemonConfigurationMergeConflictsWithInnerStructs(t *testing.T) { t.Fatalf("expected tlscacert conflict, got %v", err) } } + +func TestFindConfigurationConflictsWithUnknownKeys(t *testing.T) { + config := map[string]interface{}{"tls-verify": "true"} + flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + + flags.Bool([]string{"-tlsverify"}, false, "") + err := findConfigurationConflicts(config, flags) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "the following directives don't match any configuration option: tls-verify") { + t.Fatalf("expected tls-verify conflict, got %v", err) + } +} + +func TestFindConfigurationConflictsWithMergedValues(t *testing.T) { + var hosts []string + config := map[string]interface{}{"hosts": "tcp://127.0.0.1:2345"} + base := mflag.NewFlagSet("base", mflag.ContinueOnError) + base.Var(opts.NewNamedListOptsRef("hosts", &hosts, nil), []string{"H", "-host"}, "") + + flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + mflag.Merge(flags, base) + + err := findConfigurationConflicts(config, flags) + if err != nil { + t.Fatal(err) + } + + flags.Set("-host", "unix:///var/run/docker.sock") + err = findConfigurationConflicts(config, flags) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "hosts: (from flag: [unix:///var/run/docker.sock], from file: tcp://127.0.0.1:2345)") { + t.Fatalf("expected hosts conflict, got %v", err) + } +} diff --git a/docker/common.go b/docker/common.go index 5652424ed2..6028f79da0 100644 --- a/docker/common.go +++ b/docker/common.go @@ -18,6 +18,7 @@ const ( defaultCaFile = "ca.pem" defaultKeyFile = "key.pem" defaultCertFile = "cert.pem" + tlsVerifyKey = "tlsverify" ) var ( @@ -60,7 +61,7 @@ func postParseCommon() { // Regardless of whether the user sets it to true or false, if they // specify --tlsverify at all then we need to turn on tls // TLSVerify can be true even if not set due to DOCKER_TLS_VERIFY env var, so we need to check that here as well - if cmd.IsSet("-tlsverify") || commonFlags.TLSVerify { + if cmd.IsSet("-"+tlsVerifyKey) || commonFlags.TLSVerify { commonFlags.TLS = true } diff --git a/docker/daemon.go b/docker/daemon.go index ba4c9e41dd..0f85419215 100644 --- a/docker/daemon.go +++ b/docker/daemon.go @@ -204,9 +204,9 @@ func (cli *DaemonCli) CmdDaemon(args ...string) error { defaultHost := opts.DefaultHost if cli.Config.TLS { tlsOptions := tlsconfig.Options{ - CAFile: cli.Config.TLSOptions.CAFile, - CertFile: cli.Config.TLSOptions.CertFile, - KeyFile: cli.Config.TLSOptions.KeyFile, + CAFile: cli.Config.CommonTLSOptions.CAFile, + CertFile: cli.Config.CommonTLSOptions.CertFile, + KeyFile: cli.Config.CommonTLSOptions.KeyFile, } if cli.Config.TLSVerify { @@ -338,12 +338,12 @@ func loadDaemonCliConfig(config *daemon.Config, daemonFlags *flag.FlagSet, commo config.LogLevel = commonConfig.LogLevel config.TLS = commonConfig.TLS config.TLSVerify = commonConfig.TLSVerify - config.TLSOptions = daemon.CommonTLSOptions{} + config.CommonTLSOptions = daemon.CommonTLSOptions{} if commonConfig.TLSOptions != nil { - config.TLSOptions.CAFile = commonConfig.TLSOptions.CAFile - config.TLSOptions.CertFile = commonConfig.TLSOptions.CertFile - config.TLSOptions.KeyFile = commonConfig.TLSOptions.KeyFile + config.CommonTLSOptions.CAFile = commonConfig.TLSOptions.CAFile + config.CommonTLSOptions.CertFile = commonConfig.TLSOptions.CertFile + config.CommonTLSOptions.KeyFile = commonConfig.TLSOptions.KeyFile } if configFile != "" { @@ -362,7 +362,7 @@ func loadDaemonCliConfig(config *daemon.Config, daemonFlags *flag.FlagSet, commo // Regardless of whether the user sets it to true or false, if they // specify TLSVerify at all then we need to turn on TLS - if config.IsValueSet("tls-verify") { + if config.IsValueSet(tlsVerifyKey) { config.TLS = true } diff --git a/docker/daemon_test.go b/docker/daemon_test.go index 992172de4f..cd5d100d1a 100644 --- a/docker/daemon_test.go +++ b/docker/daemon_test.go @@ -51,8 +51,8 @@ func TestLoadDaemonCliConfigWithTLS(t *testing.T) { if loadedConfig == nil { t.Fatalf("expected configuration %v, got nil", c) } - if loadedConfig.TLSOptions.CAFile != "/tmp/ca.pem" { - t.Fatalf("expected /tmp/ca.pem, got %s: %q", loadedConfig.TLSOptions.CAFile, loadedConfig) + if loadedConfig.CommonTLSOptions.CAFile != "/tmp/ca.pem" { + t.Fatalf("expected /tmp/ca.pem, got %s: %q", loadedConfig.CommonTLSOptions.CAFile, loadedConfig) } } @@ -105,10 +105,11 @@ func TestLoadDaemonCliConfigWithTLSVerify(t *testing.T) { } configFile := f.Name() - f.Write([]byte(`{"tls-verify": true}`)) + f.Write([]byte(`{"tlsverify": true}`)) f.Close() flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.Bool([]string{"-tlsverify"}, false, "") loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) if err != nil { t.Fatal(err) @@ -136,10 +137,11 @@ func TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse(t *testing.T) { } configFile := f.Name() - f.Write([]byte(`{"tls-verify": false}`)) + f.Write([]byte(`{"tlsverify": false}`)) f.Close() flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.Bool([]string{"-tlsverify"}, false, "") loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) if err != nil { t.Fatal(err) @@ -198,6 +200,7 @@ func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) { f.Close() flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.String([]string{"-log-level"}, "", "") loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) if err != nil { t.Fatal(err) @@ -213,3 +216,34 @@ func TestLoadDaemonCliConfigWithLogLevel(t *testing.T) { t.Fatalf("expected warn log level, got %v", logrus.GetLevel()) } } + +func TestLoadDaemonConfigWithEmbeddedOptions(t *testing.T) { + c := &daemon.Config{} + common := &cli.CommonFlags{} + flags := mflag.NewFlagSet("test", mflag.ContinueOnError) + flags.String([]string{"-tlscacert"}, "", "") + flags.String([]string{"-log-driver"}, "", "") + + f, err := ioutil.TempFile("", "docker-config-") + if err != nil { + t.Fatal(err) + } + + configFile := f.Name() + f.Write([]byte(`{"tlscacert": "/etc/certs/ca.pem", "log-driver": "syslog"}`)) + f.Close() + + loadedConfig, err := loadDaemonCliConfig(c, flags, common, configFile) + if err != nil { + t.Fatal(err) + } + if loadedConfig == nil { + t.Fatalf("expected configuration %v, got nil", c) + } + if loadedConfig.CommonTLSOptions.CAFile != "/etc/certs/ca.pem" { + t.Fatalf("expected CA file path /etc/certs/ca.pem, got %v", loadedConfig.CommonTLSOptions.CAFile) + } + if loadedConfig.LogConfig.Type != "syslog" { + t.Fatalf("expected LogConfig type syslog, got %v", loadedConfig.LogConfig.Type) + } +} diff --git a/docs/reference/commandline/daemon.md b/docs/reference/commandline/daemon.md index f94f6b72e9..e59208033f 100644 --- a/docs/reference/commandline/daemon.md +++ b/docs/reference/commandline/daemon.md @@ -838,10 +838,8 @@ This is a full example of the allowed configuration options in the file: "storage-driver": "", "storage-opts": "", "labels": [], - "log-config": { - "log-driver": "", - "log-opts": [] - }, + "log-driver": "", + "log-opts": [], "mtu": 0, "pidfile": "", "graph": "", @@ -852,12 +850,10 @@ This is a full example of the allowed configuration options in the file: "hosts": [], "log-level": "", "tls": true, - "tls-verify": true, - "tls-opts": { - "tlscacert": "", - "tlscert": "", - "tlskey": "" - }, + "tlsverify": true, + "tlscacert": "", + "tlscert": "", + "tlskey": "", "api-cors-headers": "", "selinux-enabled": false, "userns-remap": "", diff --git a/pkg/mflag/flag.go b/pkg/mflag/flag.go index 0468329958..e2a0c42257 100644 --- a/pkg/mflag/flag.go +++ b/pkg/mflag/flag.go @@ -1223,11 +1223,27 @@ func (v mergeVal) IsBoolFlag() bool { return false } +// Name returns the name of a mergeVal. +// If the original value had a name, return the original name, +// otherwise, return the key asinged to this mergeVal. +func (v mergeVal) Name() string { + type namedValue interface { + Name() string + } + if nVal, ok := v.Value.(namedValue); ok { + return nVal.Name() + } + return v.key +} + // Merge is an helper function that merges n FlagSets into a single dest FlagSet // In case of name collision between the flagsets it will apply // the destination FlagSet's errorHandling behavior. func Merge(dest *FlagSet, flagsets ...*FlagSet) error { for _, fset := range flagsets { + if fset.formal == nil { + continue + } for k, f := range fset.formal { if _, ok := dest.formal[k]; ok { var err error @@ -1249,6 +1265,9 @@ func Merge(dest *FlagSet, flagsets ...*FlagSet) error { } newF := *f newF.Value = mergeVal{f.Value, k, fset} + if dest.formal == nil { + dest.formal = make(map[string]*Flag) + } dest.formal[k] = &newF } } diff --git a/pkg/mflag/flag_test.go b/pkg/mflag/flag_test.go index c28deda896..138355546e 100644 --- a/pkg/mflag/flag_test.go +++ b/pkg/mflag/flag_test.go @@ -514,3 +514,14 @@ func TestSortFlags(t *testing.T) { t.Fatalf("NFlag (%d) != fs.NFlag() (%d) of elements visited", nflag, fs.NFlag()) } } + +func TestMergeFlags(t *testing.T) { + base := NewFlagSet("base", ContinueOnError) + base.String([]string{"f"}, "", "") + + fs := NewFlagSet("test", ContinueOnError) + Merge(fs, base) + if len(fs.formal) != 1 { + t.Fatalf("FlagCount (%d) != number (1) of elements merged", len(fs.formal)) + } +}