Merge pull request #43509 from thaJeztah/daemon_fix_hosts_validation_step1a

cmd/dockerd: improve validation to allow early exit

cmd/dockerd/daemon.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"strings"
 	"time"
 
@@ -75,7 +76,7 @@ func NewDaemonCli() *DaemonCli {
 }
 
 func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
-	opts.SetDefaultOptions(opts.flags)
+	opts.setDefaultOptions()
 
 	if cli.Config, err = loadDaemonCliConfig(opts); err != nil {
 		return err
@@ -84,6 +85,11 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
 		return err
 	}
 
+	serverConfig, err := newAPIServerConfig(cli.Config)
+	if err != nil {
+		return err
+	}
+
 	if opts.Validate {
 		// If config wasn't OK we wouldn't have made it this far.
 		fmt.Fprintln(os.Stderr, "configuration OK")
@@ -91,10 +97,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
 	}
 
 	configureProxyEnv(cli.Config)
-
-	if err := configureDaemonLogs(cli.Config); err != nil {
-		return err
-	}
+	configureDaemonLogs(cli.Config)
 
 	logrus.Info("Starting up")
 
@@ -161,10 +164,6 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
 		}
 	}
 
-	serverConfig, err := newAPIServerConfig(cli)
-	if err != nil {
-		return errors.Wrap(err, "failed to create API server")
-	}
 	cli.api = apiserver.New(serverConfig)
 
 	hosts, err := loadListeners(cli, serverConfig)
@@ -393,21 +392,26 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
 	conf.Hosts = opts.Hosts
 	conf.LogLevel = opts.LogLevel
 
-	if opts.flags.Changed(FlagTLS) {
+	if flags.Changed("graph") && flags.Changed("data-root") {
+		return nil, errors.New(`cannot specify both "--graph" and "--data-root" option`)
+	}
+	if flags.Changed(FlagTLS) {
 		conf.TLS = &opts.TLS
 	}
-	if opts.flags.Changed(FlagTLSVerify) {
+	if flags.Changed(FlagTLSVerify) {
 		conf.TLSVerify = &opts.TLSVerify
 		v := true
 		conf.TLS = &v
 	}
 
-	conf.CommonTLSOptions = config.CommonTLSOptions{}
-
 	if opts.TLSOptions != nil {
-		conf.CommonTLSOptions.CAFile = opts.TLSOptions.CAFile
-		conf.CommonTLSOptions.CertFile = opts.TLSOptions.CertFile
-		conf.CommonTLSOptions.KeyFile = opts.TLSOptions.KeyFile
+		conf.CommonTLSOptions = config.CommonTLSOptions{
+			CAFile:   opts.TLSOptions.CAFile,
+			CertFile: opts.TLSOptions.CertFile,
+			KeyFile:  opts.TLSOptions.KeyFile,
+		}
+	} else {
+		conf.CommonTLSOptions = config.CommonTLSOptions{}
 	}
 
 	if conf.TrustKeyPath == "" {
@@ -418,10 +422,6 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
 		conf.TrustKeyPath = filepath.Join(daemonConfDir, defaultTrustKeyFile)
 	}
 
-	if flags.Changed("graph") && flags.Changed("data-root") {
-		return nil, errors.New(`cannot specify both "--graph" and "--data-root" option`)
-	}
-
 	if opts.configFile != "" {
 		c, err := config.MergeDaemonConfigurations(conf, flags, opts.configFile)
 		if err != nil {
@@ -437,6 +437,10 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
 		}
 	}
 
+	if err := normalizeHosts(conf); err != nil {
+		return nil, err
+	}
+
 	if err := config.Validate(conf); err != nil {
 		return nil, err
 	}
@@ -471,6 +475,41 @@ func loadDaemonCliConfig(opts *daemonOptions) (*config.Config, error) {
 	return conf, nil
 }
 
+// normalizeHosts normalizes the configured config.Hosts and remove duplicates.
+// It returns an error if it fails to parse a host.
+func normalizeHosts(config *config.Config) error {
+	if len(config.Hosts) == 0 {
+		// if no hosts are configured, create a single entry slice, so that the
+		// default is used.
+		//
+		// TODO(thaJeztah) implement a cleaner way for this; this depends on a
+		//                 side-effect of how we parse empty/partial hosts.
+		config.Hosts = make([]string, 1)
+	}
+	hosts := make([]string, 0, len(config.Hosts))
+	seen := make(map[string]struct{}, len(config.Hosts))
+
+	useTLS := DefaultTLSValue
+	if config.TLS != nil {
+		useTLS = *config.TLS
+	}
+
+	for _, h := range config.Hosts {
+		host, err := dopts.ParseHost(useTLS, honorXDG, h)
+		if err != nil {
+			return err
+		}
+		if _, ok := seen[host]; ok {
+			continue
+		}
+		seen[host] = struct{}{}
+		hosts = append(hosts, host)
+	}
+	sort.Strings(hosts)
+	config.Hosts = hosts
+	return nil
+}
+
 func checkDeprecatedOptions(config *config.Config) error {
 	// Overlay networks with external k/v stores have been deprecated
 	if config.ClusterAdvertise != "" || len(config.ClusterOpts) > 0 || config.ClusterStore != "" {
@@ -567,36 +606,32 @@ func (cli *DaemonCli) getContainerdDaemonOpts() ([]supervisor.DaemonOpt, error)
 	return opts, nil
 }
 
-func newAPIServerConfig(cli *DaemonCli) (*apiserver.Config, error) {
+func newAPIServerConfig(config *config.Config) (*apiserver.Config, error) {
 	serverConfig := &apiserver.Config{
-		SocketGroup: cli.Config.SocketGroup,
+		SocketGroup: config.SocketGroup,
 		Version:     dockerversion.Version,
-		CorsHeaders: cli.Config.CorsHeaders,
+		CorsHeaders: config.CorsHeaders,
 	}
 
-	if cli.Config.TLS != nil && *cli.Config.TLS {
+	if config.TLS != nil && *config.TLS {
 		tlsOptions := tlsconfig.Options{
-			CAFile:             cli.Config.CommonTLSOptions.CAFile,
-			CertFile:           cli.Config.CommonTLSOptions.CertFile,
-			KeyFile:            cli.Config.CommonTLSOptions.KeyFile,
+			CAFile:             config.CommonTLSOptions.CAFile,
+			CertFile:           config.CommonTLSOptions.CertFile,
+			KeyFile:            config.CommonTLSOptions.KeyFile,
 			ExclusiveRootPools: true,
 		}
 
-		if cli.Config.TLSVerify == nil || *cli.Config.TLSVerify {
+		if config.TLSVerify == nil || *config.TLSVerify {
 			// server requires and verifies client's certificate
 			tlsOptions.ClientAuth = tls.RequireAndVerifyClientCert
 		}
 		tlsConfig, err := tlsconfig.Server(tlsOptions)
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrap(err, "invalid TLS configuration")
 		}
 		serverConfig.TLSConfig = tlsConfig
 	}
 
-	if len(cli.Config.Hosts) == 0 {
-		cli.Config.Hosts = make([]string, 1)
-	}
-
 	return serverConfig, nil
 }
 
@@ -626,32 +661,19 @@ func checkTLSAuthOK(c *config.Config) bool {
 }
 
 func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, error) {
-	var hosts []string
-	seen := make(map[string]struct{}, len(cli.Config.Hosts))
-
-	useTLS := DefaultTLSValue
-	if cli.Config.TLS != nil {
-		useTLS = *cli.Config.TLS
+	if len(cli.Config.Hosts) == 0 {
+		return nil, errors.New("no hosts configured")
 	}
+	var hosts []string
 
 	for i := 0; i < len(cli.Config.Hosts); i++ {
-		var err error
-		if cli.Config.Hosts[i], err = dopts.ParseHost(useTLS, honorXDG, cli.Config.Hosts[i]); err != nil {
-			return nil, errors.Wrapf(err, "error parsing -H %s", cli.Config.Hosts[i])
-		}
-		if _, ok := seen[cli.Config.Hosts[i]]; ok {
-			continue
-		}
-		seen[cli.Config.Hosts[i]] = struct{}{}
-
 		protoAddr := cli.Config.Hosts[i]
-		protoAddrParts := strings.SplitN(protoAddr, "://", 2)
+		protoAddrParts := strings.SplitN(cli.Config.Hosts[i], "://", 2)
 		if len(protoAddrParts) != 2 {
 			return nil, fmt.Errorf("bad format %s, expected PROTO://ADDR", protoAddr)
 		}
 
-		proto := protoAddrParts[0]
-		addr := protoAddrParts[1]
+		proto, addr := protoAddrParts[0], protoAddrParts[1]
 
 		// It's a bad idea to bind to TCP without tlsverify.
 		authEnabled := serverConfig.TLSConfig != nil && serverConfig.TLSConfig.ClientAuth == tls.RequireAndVerifyClientCert
@@ -764,14 +786,14 @@ func systemContainerdRunning(honorXDG bool) (string, bool, error) {
 	return addr, err == nil, nil
 }
 
-// configureDaemonLogs sets the logrus logging level and formatting
-func configureDaemonLogs(conf *config.Config) error {
+// configureDaemonLogs sets the logrus logging level and formatting. It expects
+// the passed configuration to already be validated, and ignores invalid options.
+func configureDaemonLogs(conf *config.Config) {
 	if conf.LogLevel != "" {
 		lvl, err := logrus.ParseLevel(conf.LogLevel)
-		if err != nil {
-			return fmt.Errorf("unable to parse logging level: %s", conf.LogLevel)
+		if err == nil {
+			logrus.SetLevel(lvl)
 		}
-		logrus.SetLevel(lvl)
 	} else {
 		logrus.SetLevel(logrus.InfoLevel)
 	}
@@ -780,7 +802,6 @@ func configureDaemonLogs(conf *config.Config) error {
 		DisableColors:   conf.RawLogs,
 		FullTimestamp:   true,
 	})
-	return nil
 }
 
 func configureProxyEnv(conf *config.Config) {

cmd/dockerd/daemon_test.go

@@ -14,7 +14,7 @@ import (
 func defaultOptions(t *testing.T, configFile string) *daemonOptions {
 	opts := newDaemonOptions(&config.Config{})
 	opts.flags = &pflag.FlagSet{}
-	opts.InstallFlags(opts.flags)
+	opts.installFlags(opts.flags)
 	if err := installConfigFlags(opts.daemonConfig, opts.flags); err != nil {
 		t.Fatal(err)
 	}
@@ -186,19 +186,15 @@ func TestLoadDaemonConfigWithRegistryOptions(t *testing.T) {
 
 func TestConfigureDaemonLogs(t *testing.T) {
 	conf := &config.Config{}
-	err := configureDaemonLogs(conf)
-	assert.NilError(t, err)
+	configureDaemonLogs(conf)
 	assert.Check(t, is.Equal(logrus.InfoLevel, logrus.GetLevel()))
 
 	conf.LogLevel = "warn"
-	err = configureDaemonLogs(conf)
-	assert.NilError(t, err)
+	configureDaemonLogs(conf)
 	assert.Check(t, is.Equal(logrus.WarnLevel, logrus.GetLevel()))
 
+	// log level should not be changed when passing an invalid value
 	conf.LogLevel = "foobar"
-	err = configureDaemonLogs(conf)
-	assert.Error(t, err, "unable to parse logging level: foobar")
-
-	// log level should not be changed after a failure
+	configureDaemonLogs(conf)
 	assert.Check(t, is.Equal(logrus.WarnLevel, logrus.GetLevel()))
 }

cmd/dockerd/docker.go

@@ -46,7 +46,7 @@ func newDaemonCommand() (*cobra.Command, error) {
 	}
 	flags.StringVar(&opts.configFile, "config-file", defaultDaemonConfigFile, "Daemon configuration file")
 	configureCertsDir()
-	opts.InstallFlags(flags)
+	opts.installFlags(flags)
 	if err := installConfigFlags(opts.daemonConfig, flags); err != nil {
 		return nil, err
 	}

cmd/dockerd/options.go

@@ -51,8 +51,8 @@ func newDaemonOptions(config *config.Config) *daemonOptions {
 	}
 }
 
-// InstallFlags adds flags for the common options on the FlagSet
-func (o *daemonOptions) InstallFlags(flags *pflag.FlagSet) {
+// installFlags adds flags for the common options on the FlagSet
+func (o *daemonOptions) installFlags(flags *pflag.FlagSet) {
 	if dockerCertPath == "" {
 		// cliconfig.Dir returns $DOCKER_CONFIG or ~/.docker.
 		// cliconfig.Dir does not look up $XDG_CONFIG_HOME
@@ -77,18 +77,18 @@ func (o *daemonOptions) InstallFlags(flags *pflag.FlagSet) {
 	flags.VarP(hostOpt, "host", "H", "Daemon socket(s) to connect to")
 }
 
-// SetDefaultOptions sets default values for options after flag parsing is
+// setDefaultOptions sets default values for options after flag parsing is
 // complete
-func (o *daemonOptions) SetDefaultOptions(flags *pflag.FlagSet) {
+func (o *daemonOptions) setDefaultOptions() {
 	// 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 flags.Changed(FlagTLSVerify) || o.TLSVerify {
+	if o.flags.Changed(FlagTLSVerify) || o.TLSVerify {
 		o.TLS = true
 	}
 
-	if o.TLS && !flags.Changed(FlagTLSVerify) {
+	if o.TLS && !o.flags.Changed(FlagTLSVerify) {
 		// Enable tls verification unless explicitly disabled
 		o.TLSVerify = true
 	}
@@ -96,19 +96,18 @@ func (o *daemonOptions) SetDefaultOptions(flags *pflag.FlagSet) {
 	if !o.TLS {
 		o.TLSOptions = nil
 	} else {
-		tlsOptions := o.TLSOptions
-		tlsOptions.InsecureSkipVerify = !o.TLSVerify
+		o.TLSOptions.InsecureSkipVerify = !o.TLSVerify
 
 		// Reset CertFile and KeyFile to empty string if the user did not specify
 		// the respective flags and the respective default files were not found.
-		if !flags.Changed("tlscert") {
-			if _, err := os.Stat(tlsOptions.CertFile); os.IsNotExist(err) {
-				tlsOptions.CertFile = ""
+		if !o.flags.Changed("tlscert") {
+			if _, err := os.Stat(o.TLSOptions.CertFile); os.IsNotExist(err) {
+				o.TLSOptions.CertFile = ""
 			}
 		}
-		if !flags.Changed("tlskey") {
-			if _, err := os.Stat(tlsOptions.KeyFile); os.IsNotExist(err) {
-				tlsOptions.KeyFile = ""
+		if !o.flags.Changed("tlskey") {
+			if _, err := os.Stat(o.TLSOptions.KeyFile); os.IsNotExist(err) {
+				o.TLSOptions.KeyFile = ""
 			}
 		}
 	}

cmd/dockerd/options_test.go

@@ -14,7 +14,7 @@ import (
 func TestCommonOptionsInstallFlags(t *testing.T) {
 	flags := pflag.NewFlagSet("testing", pflag.ContinueOnError)
 	opts := newDaemonOptions(&config.Config{})
-	opts.InstallFlags(flags)
+	opts.installFlags(flags)
 
 	err := flags.Parse([]string{
 		"--tlscacert=/foo/cafile",
@@ -34,7 +34,7 @@ func defaultPath(filename string) string {
 func TestCommonOptionsInstallFlagsWithDefaults(t *testing.T) {
 	flags := pflag.NewFlagSet("testing", pflag.ContinueOnError)
 	opts := newDaemonOptions(&config.Config{})
-	opts.InstallFlags(flags)
+	opts.installFlags(flags)
 
 	err := flags.Parse([]string{})
 	assert.Check(t, err)

daemon/config/config.go

@@ -558,6 +558,13 @@ func findConfigurationConflicts(config map[string]interface{}, flags *pflag.Flag
 // such as config.DNS, config.Labels, config.DNSSearch,
 // as well as config.MaxConcurrentDownloads, config.MaxConcurrentUploads and config.MaxDownloadAttempts.
 func Validate(config *Config) error {
+	// validate log-level
+	if config.LogLevel != "" {
+		if _, err := logrus.ParseLevel(config.LogLevel); err != nil {
+			return fmt.Errorf("invalid logging level: %s", config.LogLevel)
+		}
+	}
+
 	// validate DNS
 	for _, dns := range config.DNS {
 		if _, err := opts.ValidateIPAddress(dns); err != nil {

daemon/config/config_test.go

@@ -345,6 +345,15 @@ func TestValidateConfigurationErrors(t *testing.T) {
 			},
 			expectedErr: "invalid bind address (127.0.0.1:2375/path): should not contain a path element",
 		},
+		{
+			name: "with invalid log-level",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					LogLevel: "foobar",
+				},
+			},
+			expectedErr: "invalid logging level: foobar",
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -437,6 +446,14 @@ func TestValidateConfiguration(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "with log-level warn",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					LogLevel: "warn",
+				},
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {