Merge pull request moby#48979 from thaJeztah/execopts_parse

daemon/config: add validation of exec-config options

daemon/config/config.go

@@ -343,6 +343,17 @@ func New() (*Config, error) {
 	return cfg, nil
 }
 
+// GetExecOpt looks up a user-configured exec-opt. It returns a boolean
+// if found, and an error if the configuration has invalid options set.
+func (conf *Config) GetExecOpt(name string) (val string, found bool, _ error) {
+	o, err := parseExecOptions(conf.ExecOptions)
+	if err != nil {
+		return "", false, err
+	}
+	val, found = o[name]
+	return val, found, nil
+}
+
 // GetConflictFreeLabels validates Labels for conflict
 // In swarm the duplicates for labels are removed
 // so we only take same values here, no conflict values
@@ -739,10 +750,36 @@ func Validate(config *Config) error {
 		return errors.New(`DEPRECATED: The "api-cors-header" config parameter and the dockerd "--api-cors-header" option have been removed; use a reverse proxy if you need CORS headers`)
 	}
 
+	if _, err := parseExecOptions(config.ExecOptions); err != nil {
+		return err
+	}
+
 	// validate platform-specific settings
 	return validatePlatformConfig(config)
 }
 
+// parseExecOptions parses the given exec-options into a map. It returns an
+// error if the exec-options are formatted incorrectly, or when options are
+// used that are not supported on this platform.
+//
+// TODO(thaJeztah): consider making this more strict: make options case-sensitive and disallow whitespace around "=".
+func parseExecOptions(execOptions []string) (map[string]string, error) {
+	o := make(map[string]string)
+	for _, keyValue := range execOptions {
+		k, v, ok := strings.Cut(keyValue, "=")
+		k = strings.ToLower(strings.TrimSpace(k))
+		v = strings.TrimSpace(v)
+		if !ok || k == "" || v == "" {
+			return nil, fmt.Errorf("invalid exec-opt (%s): must be formatted 'opt=value'", keyValue)
+		}
+		if err := validatePlatformExecOpt(k, v); err != nil {
+			return nil, fmt.Errorf("invalid exec-opt (%s): %w", keyValue, err)
+		}
+		o[k] = v
+	}
+	return o, nil
+}
+
 // MaskCredentials masks credentials that are in an URL.
 func MaskCredentials(rawURL string) string {
 	parsedURL, err := url.Parse(rawURL)

daemon/config/config_linux.go

@@ -245,6 +245,20 @@ func validatePlatformConfig(conf *Config) error {
 	return verifyDefaultCgroupNsMode(conf.CgroupNamespaceMode)
 }
 
+// validatePlatformExecOpt validates if the given exec-opt and value are valid
+// for the current platform.
+func validatePlatformExecOpt(opt, value string) error {
+	switch opt {
+	case "isolation":
+		return fmt.Errorf("option '%s' is only supported on windows", opt)
+	case "native.cgroupdriver":
+		// TODO(thaJeztah): add validation that's currently in daemon.verifyCgroupDriver
+		return nil
+	default:
+		return fmt.Errorf("unknown option: '%s'", opt)
+	}
+}
+
 // verifyUserlandProxyConfig verifies if a valid userland-proxy path
 // is configured if userland-proxy is enabled.
 func verifyUserlandProxyConfig(conf *Config) error {

daemon/config/config_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -221,6 +222,7 @@ func TestValidateConfigurationErrors(t *testing.T) {
 		name        string
 		field       string
 		config      *Config
+		platform    string
 		expectedErr string
 	}{
 		{
@@ -352,6 +354,62 @@ func TestValidateConfigurationErrors(t *testing.T) {
 			},
 			expectedErr: "invalid logging level: foobar",
 		},
+		{
+			name: "exec-opt without value",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"no-value"},
+				},
+			},
+			expectedErr: "invalid exec-opt (no-value): must be formatted 'opt=value'",
+		},
+		{
+			name: "exec-opt with empty value",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"empty-value="},
+				},
+			},
+			expectedErr: "invalid exec-opt (empty-value=): must be formatted 'opt=value'",
+		},
+		{
+			name: "exec-opt without key",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"=empty-key"},
+				},
+			},
+			expectedErr: "invalid exec-opt (=empty-key): must be formatted 'opt=value'",
+		},
+		{
+			name: "exec-opt unknown option",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"unknown-option=any-value"},
+				},
+			},
+			expectedErr: "invalid exec-opt (unknown-option=any-value): unknown option: 'unknown-option'",
+		},
+		{
+			name: "exec-opt invalid on linux",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"isolation=default"},
+				},
+			},
+			platform:    "linux",
+			expectedErr: "invalid exec-opt (isolation=default): option 'isolation' is only supported on windows",
+		},
+		{
+			name: "exec-opt invalid on windows",
+			config: &Config{
+				CommonConfig: CommonConfig{
+					ExecOptions: []string{"native.cgroupdriver=systemd"},
+				},
+			},
+			platform:    "windows",
+			expectedErr: "invalid exec-opt (native.cgroupdriver=systemd): option 'native.cgroupdriver' is only supported on linux",
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -363,7 +421,11 @@ func TestValidateConfigurationErrors(t *testing.T) {
 				assert.Check(t, mergo.Merge(cfg, tc.config, mergo.WithOverride))
 			}
 			err = Validate(cfg)
-			assert.Error(t, err, tc.expectedErr)
+			if tc.platform != "" && tc.platform != runtime.GOOS {
+				assert.NilError(t, err)
+			} else {
+				assert.Error(t, err, tc.expectedErr)
+			}
 		})
 	}
 }

daemon/config/config_windows.go

@@ -2,6 +2,7 @@ package config // import "github.com/docker/docker/daemon/config"
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path/filepath"
 
@@ -89,3 +90,17 @@ func validatePlatformConfig(conf *Config) error {
 	}
 	return nil
 }
+
+// validatePlatformExecOpt validates if the given exec-opt and value are valid
+// for the current platform.
+func validatePlatformExecOpt(opt, value string) error {
+	switch opt {
+	case "isolation":
+		// TODO(thaJeztah): add validation that's currently in Daemon.setDefaultIsolation()
+		return nil
+	case "native.cgroupdriver":
+		return fmt.Errorf("option '%s' is only supported on linux", opt)
+	default:
+		return fmt.Errorf("unknown option: '%s'", opt)
+	}
+}

daemon/daemon_unix.go

@@ -585,32 +585,25 @@ func cgroupDriver(cfg *config.Config) string {
 	return cgroupFsDriver
 }
 
-// getCD gets the raw value of the native.cgroupdriver option, if set.
-func getCD(config *config.Config) string {
-	for _, option := range config.ExecOptions {
-		key, val, ok := strings.Cut(option, "=")
-		if ok && strings.EqualFold(strings.TrimSpace(key), "native.cgroupdriver") {
-			return strings.TrimSpace(val)
-		}
-	}
-	return ""
-}
-
 // verifyCgroupDriver validates native.cgroupdriver
 func verifyCgroupDriver(config *config.Config) error {
-	cd := getCD(config)
-	if cd == "" || cd == cgroupFsDriver || cd == cgroupSystemdDriver {
-		return nil
+	cd, _, err := config.GetExecOpt("native.cgroupdriver")
+	if err != nil {
+		return err
 	}
-	if cd == cgroupNoneDriver {
+	switch cd {
+	case "", cgroupFsDriver, cgroupSystemdDriver:
+		return nil
+	case cgroupNoneDriver:
 		return fmt.Errorf("native.cgroupdriver option %s is internally used and cannot be specified manually", cd)
+	default:
+		return fmt.Errorf("native.cgroupdriver option %s not supported", cd)
 	}
-	return fmt.Errorf("native.cgroupdriver option %s not supported", cd)
 }
 
 // UsingSystemd returns true if cli option includes native.cgroupdriver=systemd
 func UsingSystemd(config *config.Config) bool {
-	cd := getCD(config)
+	cd, _, _ := config.GetExecOpt("native.cgroupdriver")
 
 	if cd == cgroupSystemdDriver {
 		return true

daemon/daemon_windows.go

@@ -25,7 +25,6 @@ import (
 	"github.com/docker/docker/libnetwork/options"
 	"github.com/docker/docker/libnetwork/scope"
 	"github.com/docker/docker/pkg/idtools"
-	"github.com/docker/docker/pkg/parsers"
 	"github.com/docker/docker/pkg/parsers/operatingsystem"
 	"github.com/docker/docker/pkg/sysinfo"
 	"github.com/docker/docker/pkg/system"
@@ -523,26 +522,20 @@ func (daemon *Daemon) setDefaultIsolation(config *config.Config) error {
 	} else {
 		daemon.defaultIsolation = containertypes.IsolationProcess
 	}
-	for _, option := range config.ExecOptions {
-		key, val, err := parsers.ParseKeyValueOpt(option)
-		if err != nil {
-			return err
+	val, ok, err := config.GetExecOpt("isolation")
+	if err != nil {
+		return err
+	}
+	if ok {
+		isolation := containertypes.Isolation(strings.ToLower(val))
+		if !isolation.IsValid() {
+			return fmt.Errorf("invalid exec-opt value for 'isolation':'%s'", val)
 		}
-		key = strings.ToLower(key)
-		switch key {
-
-		case "isolation":
-			if !containertypes.Isolation(val).IsValid() {
-				return fmt.Errorf("Invalid exec-opt value for 'isolation':'%s'", val)
-			}
-			if containertypes.Isolation(val).IsHyperV() {
-				daemon.defaultIsolation = containertypes.IsolationHyperV
-			}
-			if containertypes.Isolation(val).IsProcess() {
-				daemon.defaultIsolation = containertypes.IsolationProcess
-			}
-		default:
-			return fmt.Errorf("Unrecognised exec-opt '%s'\n", key)
+		if isolation.IsHyperV() {
+			daemon.defaultIsolation = containertypes.IsolationHyperV
+		}
+		if isolation.IsProcess() {
+			daemon.defaultIsolation = containertypes.IsolationProcess
 		}
 	}