daemon/config: add validation of exec-config options#48979
daemon/config: add validation of exec-config options#48979thaJeztah merged 1 commit intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
impact/changelog
impact/deprecation
area/daemon
daemon/config/config_linux.go
Outdated
| case "isolation": | ||
| return fmt.Errorf("invalid exec-opt (%s): '%s' option is only supported on windows", opt, k) | ||
| case "native.cgroupdriver": | ||
| // TODO(thaJeztah): add validation that's currently in daemon.verifyCgroupDriver |
There was a problem hiding this comment.
I'll work on this in a follow-up
daemon/config/config_windows.go
Outdated
| // TODO(thaJeztah): add validation that's currently in Daemon.setDefaultIsolation() | ||
| continue |
This comment was marked as resolved.
This comment was marked as resolved.
thaJeztah
force-pushed
the
execopts_parse
branch
from
7f42b34 to
8d090a5
Compare
daemon/config/config_linux.go
Outdated
| for _, opt := range execOptions { | ||
| k, v, ok := strings.Cut(opt, "=") | ||
| k = strings.ToLower(strings.TrimSpace(k)) | ||
| v = strings.TrimSpace(v) | ||
| if !ok || k == "" || v == "" { | ||
| return fmt.Errorf("invalid exec-opt (%s): must be formatted 'opt=value'", opt) | ||
| } |
There was a problem hiding this comment.
Looks like this part is duplicated in all platform specific verifyExecOptions implementations.
I think we can keep the loop separate and only make a platform-specific func validateExecOption(key, value string) error function that validates a single option.
There was a problem hiding this comment.
Yeah, this one was a bit tricky (and I so much would love to get rid of some of the "too permissive" handling).
I went a bit back-and-forth on this one; initially I had a non-platform-specific validate step that only validated the format, but then I still had to do similar steps in the per-platform one (trimming, lowercasing, all the things).
I then tried a single validate function but some conditionals (if runtime.GOOS != "windows" { return err ..}), which was "ok(ish)" for the set of options we currently had, but I wasn't sure if that was the cleanest approach. So I somewhat considered the duplication to be OK (keeping the _linux.go and _windows.go similar, to allow doing a side-by-side diff).
I was somewhat trying to avoid differentiating in too many places because we had the validatePlatformConfig as place to have things that are platform-specific.
I can have another look to see if there's other options (I may move the first couple of commits separate to get those in already)
thaJeztah
changed the title
thaJeztah
removed
impact/deprecation
area/go-sdk
impact/go-sdk
thaJeztah
force-pushed
the
execopts_parse
branch
2 times, most recently
from
589b297 to
c53342c
Compare
thaJeztah
force-pushed
the
execopts_parse
branch
2 times, most recently
from
a204a0f to
6e20161
Compare
Validate if options are passed in the right format and if the given option
is supported on the current platform.
Before this patch, no validation would happen until the daemon was started,
and unknown options as well as incorrectly formatted options would be silently
ignored on Linux;
dockerd --exec-opt =value-only --validate
configuration OK
dockerd --exec-opt unknown-opt=unknown-value --validate
configuration OK
dockerd --exec-opt unknown-opt=unknown-value --validate
...
INFO[2024-11-28T12:07:44.255942174Z] Daemon has completed initialization
INFO[2024-11-28T12:07:44.361412049Z] API listen on /var/run/docker.sock
With this patch, exec-opts are included in the validation before the daemon
is started/created, and errors are produced when trying to use an option
that's either unknown or not supported by the platform;
dockerd --exec-opt =value-only --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (=value-only): must be formatted 'opt=value'
dockerd --exec-opt isolation=default --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (isolation=default): 'isolation' option is only supported on windows
dockerd --exec-opt unknown-opt=unknown-value --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid exec-opt (unknown-opt=unknown-value): unknown option: 'unknown-opt'
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
execopts_parse
branch
from
6e20161 to
d7f59ce
Compare
|
Thanks! I'll bring this one in, and move #49177 out of draft ❤️ |
3 participants
daemon/config: add basic validation of exec-opt options
Validate if options are passed in the right format and if the given option
is supported on the current platform.
Before this patch, no validation would happen until the daemon was started,
and unknown options as well as incorrectly formatted options would be silently
ignored on Linux;
With this patch, exec-opts are included in the validation before the daemon
is started/created, and errors are produced when trying to use an option
that's either unknown or not supported by the platform;
- How to verify it
- Description for the changelog
- Improve validation of "exec-opts" in daemon configuration- A picture of a cute animal (not mandatory but encouraged)