daemon/server/router/container: postCommit: only decode Config by thaJeztah · Pull Request #50458 · moby/moby

thaJeztah · 2025-07-20T19:59:55Z

The postCommit used the container-decoder from runconfig to unmarshal the body. However, this function was written to decode a container CreateRequest. Commit only accepts a container Config, so we can just unmarshal to that type.

The postCommit used the container-decoder from runconfig to unmarshal the body. However, this function was written to decode a container CreateRequest. Commit only accepts a container Config, so we can just unmarshal to that type.

A local commitRequest type was added because the client posts a bare *container.Config but it may be empty / nil (see Client.ContainerCommit and container.CommitOptions), in which case it must be ignored, and no overrides to be applied.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2025-07-20T20:03:54Z

-	config, _, _, err := c.decoder.DecodeConfig(r.Body)
-	if err != nil && !errors.Is(err, io.EOF) { // Do not fail if body is empty.
+	var config *container.Config
+	if err := httputils.ReadJSON(r, config); err != nil && !errors.Is(err, io.EOF) { // Do not fail if body is empty.


Checking for io.EOF looks redundant as well; ReadJSON returns nil for an empty Body;

moby/daemon/server/httputils/httputils.go

Lines 62 to 71 in 79f802d

func ReadJSON(r *http.Request, out interface{}) error {

err := CheckForJSON(r)

if err != nil {

return err

}

if r.Body == nil || r.ContentLength == 0 {

// an empty body is not invalid, so don't return an error; see

// https://lists.w3.org/Archives/Public/ietf-http-wg/2010JulSep/0272.html

return nil

}

It will return an error when getting an io.EOF on unmarshaling, but that would be invalid JSON;

moby/daemon/server/httputils/httputils.go

Lines 74 to 79 in 79f802d

err = dec.Decode(out)

defer r.Body.Close()

if err != nil {

if errors.Is(err, io.EOF) {

return errdefs.InvalidParameter(errors.New("invalid JSON: got EOF while reading request body"))

}

☝️ removed the check

thaJeztah · 2025-07-21T06:48:38Z

Failure is related; it expects config to be nil when not set;

=== RUN   TestCommitInheritsEnv
    commit_test.go:49: assertion failed: 
        --- expectedEnv2
        +++ image2.Config.Env
          []string{
        - 	"PATH=/usr/bin:/bin",
        + 	"PATH=/usr/bin:",
          }
        
--- FAIL: TestCommitInheritsEnv (0.06s)

The postCommit used the container-decoder from runconfig to unmarshal the body. However, this function was written to decode a container CreateRequest. Commit only accepts a container Config, so we can just unmarshal to that type. A local `commitRequest` type was added because the client posts a bare `*container.Config` but it may be empty / nil (see [Client.ContainerCommit] and [container.CommitOptions]), in which case it must be ignored, and no overrides to be applied. [Client.ContainerCommit]: https://github.com/moby/moby/blob/c4afa7715715a1020e50b19ad60728c4479fb0a5/client/container_commit.go#L52 [container.CommitOptions]: https://github.com/moby/moby/blob/c4afa7715715a1020e50b19ad60728c4479fb0a5/api/types/container/options.go#L30 Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2025-07-27T13:11:32Z

This test looks flaky;

=== RUN   TestSaveAndLoadPlatform/With_single_pulled_platform
    save_test.go:341: assertion failed: error is not nil: Error response from daemon: No such image: alpine:latest
--- FAIL: TestSaveAndLoadPlatform (2.83s)
    --- FAIL: TestSaveAndLoadPlatform/With_single_pulled_platform (2.83s)

☝️ opened a tracking ticket flaky test: TestSaveAndLoadPlatform #50531

vvoland · 2025-07-28T08:38:58Z

-	// are discarded here, but decoder.DecodeConfig also performs validation,
-	// so a request containing those additional fields would result in a
-	// validation error.
-	config, _, _, err := c.decoder.DecodeConfig(r.Body)


This one had some additional logic for validation:

moby/runconfig/config.go

Lines 20 to 78 in b486373

func (r ContainerDecoder) DecodeConfig(src io.Reader) (*container.Config, *container.HostConfig, *network.NetworkingConfig, error) {

var si *sysinfo.SysInfo

if r.GetSysInfo != nil {

si = r.GetSysInfo()

} else {

si = sysinfo.New()

}

return decodeContainerConfig(src, si)

}

// decodeContainerConfig decodes a json encoded [container.CreateRequest] struct

// and returns the Config, HostConfig, and NetworkingConfig struct, and performs some

// validation. Certain parameters need daemon-side validation that cannot be done

// on the client, as only the daemon knows what is valid for the platform.

// Be aware this function is not checking whether the resulted structs are nil,

// it's your business to do so

func decodeContainerConfig(src io.Reader, si *sysinfo.SysInfo) (*container.Config, *container.HostConfig, *network.NetworkingConfig, error) {

var w container.CreateRequest

if err := loadJSON(src, &w); err != nil {

return nil, nil, nil, err

}

hc := w.HostConfig

if hc == nil {

// We may not be passed a host config, such as in the case of docker commit

return w.Config, hc, w.NetworkingConfig, nil

}

// Make sure NetworkMode has an acceptable value. We do this to ensure

// backwards compatible API behavior.

//

// TODO(thaJeztah): platform check may be redundant, as other code-paths execute this unconditionally. Also check if this code is still needed here, or already handled elsewhere.

if runtime.GOOS != "windows" && hc.NetworkMode == "" {

hc.NetworkMode = network.NetworkDefault

}

if err := validateNetMode(w.Config, hc); err != nil {

return nil, nil, nil, err

}

if err := validateIsolation(hc); err != nil {

return nil, nil, nil, err

}

if err := validateQoS(hc); err != nil {

return nil, nil, nil, err

}

if err := validateResources(hc, si); err != nil {

return nil, nil, nil, err

}

if err := validatePrivileged(hc); err != nil {

return nil, nil, nil, err

}

if err := validateReadonlyRootfs(hc); err != nil {

return nil, nil, nil, err

}

if w.Config != nil && w.Config.Volumes == nil {

w.Config.Volumes = make(map[string]struct{})

}

return w.Config, hc, w.NetworkingConfig, nil

}

Looks like most of them were related to HostConfig (which isn't present in the commit request though), but there are some related to Config like:

moby/runconfig/config.go

Lines 74 to 76 in b486373

if w.Config != nil && w.Config.Volumes == nil {

w.Config.Volumes = make(map[string]struct{})

}

(making sure Volumes map isn't nil, but perhaps this one isn't needed anymore)

moby/runconfig/hostconfig_unix.go

Lines 20 to 22 in afd6487

if hc.UTSMode.IsHost() && c.Hostname != "" {

return ErrConflictUTSHostname

}

moby/runconfig/hostconfig.go

Lines 39 to 41 in afd6487

if len(c.ExposedPorts) > 0 {

return ErrConflictNetworkExposePorts

}

(for container-mode networking only)

But I assume these would fail at later point of the commit anyway, so perhaps it's not worth to bother with them?

Ah, yes, I stared at that a few times as well, but the old one would return early if there's no HostConfig, so none of those paths would be hit!

moby/runconfig/config.go

Lines 37 to 47 in a7d4b91

func decodeContainerConfig(src io.Reader, si *sysinfo.SysInfo) (*container.Config, *container.HostConfig, *network.NetworkingConfig, error) {

var w container.CreateRequest

if err := loadJSON(src, &w); err != nil {

return nil, nil, nil, err

}

hc := w.HostConfig

if hc == nil {

// We may not be passed a host config, such as in the case of docker commit

return w.Config, hc, w.NetworkingConfig, nil

}

It was actually because I looked at that code at the time that I considered "why are we even using this decoder if nothing it does is used for docker commit ? Then I looked at the API side, and .... basically commit only accepts a Config, nothing else.

Also "fun fact" that docker commit never sends a body; we only send the changes to the daemon; https://github.com/docker/cli/blob/a1035b0796241b3bbd0f41940525373001fc146c/cli/command/container/commit.go#L59-L66

func runCommit(ctx context.Context, dockerCli command.Cli, options *commitOptions) error { response, err := dockerCli.Client().ContainerCommit(ctx, options.container, container.CommitOptions{ Reference: options.reference, Comment: options.comment, Author: options.author, Changes: options.changes.GetSlice(), Pause: options.pause, })

Which brought me back to a ticket I created a while ago, but at the time didn't even realise we already have an "option" for this (send overrides as body), but it's ... not used (for some reason). Possibly it was used by the classic builder?

docker import, docker commit: separate --changes from Dockerfile syntax #50170

thaJeztah · 2025-07-28T09:30:46Z

Let me bring this one in, then I can also remove some special handling in #50532

thaJeztah added this to the 29.0.0 milestone Jul 20, 2025

thaJeztah added status/2-code-review area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code labels Jul 20, 2025

thaJeztah commented Jul 20, 2025

View reviewed changes

thaJeztah force-pushed the commit_simplify branch 2 times, most recently from 7fba745 to c1aaf66 Compare July 20, 2025 20:16

thaJeztah marked this pull request as draft July 20, 2025 20:36

thaJeztah force-pushed the commit_simplify branch from c1aaf66 to cdd1a61 Compare July 20, 2025 21:05

thaJeztah force-pushed the commit_simplify branch from cdd1a61 to 8a372d4 Compare July 27, 2025 12:30

thaJeztah force-pushed the commit_simplify branch from 8a372d4 to b448dc5 Compare July 27, 2025 12:38

thaJeztah marked this pull request as ready for review July 27, 2025 16:56

This was referenced Jul 27, 2025

flaky test: TestSaveAndLoadPlatform #50531

Open

runconfig: remove exported errors #50532

Merged

runconfig: rewrite ContainerDecoder to DecodeCreateRequest #50533

Merged

vvoland approved these changes Jul 28, 2025

View reviewed changes

thaJeztah mentioned this pull request Jul 28, 2025

docker import, docker commit: separate --changes from Dockerfile syntax #50170

Open

thaJeztah merged commit bf8b738 into moby:master Jul 28, 2025
286 of 289 checks passed

thaJeztah deleted the commit_simplify branch July 28, 2025 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon/server/router/container: postCommit: only decode Config#50458

daemon/server/router/container: postCommit: only decode Config#50458
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:commit_simplify

thaJeztah commented Jul 20, 2025 •

edited

Loading

Uh oh!

thaJeztah Jul 20, 2025

Uh oh!

thaJeztah Jul 20, 2025

Uh oh!

thaJeztah commented Jul 21, 2025

Uh oh!

thaJeztah commented Jul 27, 2025 •

edited

Loading

Uh oh!

vvoland Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah Jul 28, 2025

Uh oh!

thaJeztah commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	func ReadJSON(r *http.Request, out interface{}) error {
	err := CheckForJSON(r)
	if err != nil {
	return err
	}
	if r.Body == nil \|\| r.ContentLength == 0 {
	// an empty body is not invalid, so don't return an error; see
	// https://lists.w3.org/Archives/Public/ietf-http-wg/2010JulSep/0272.html
	return nil
	}

	err = dec.Decode(out)
	defer r.Body.Close()
	if err != nil {
	if errors.Is(err, io.EOF) {
	return errdefs.InvalidParameter(errors.New("invalid JSON: got EOF while reading request body"))
	}

	func (r ContainerDecoder) DecodeConfig(src io.Reader) (container.Config, container.HostConfig, *network.NetworkingConfig, error) {
	var si *sysinfo.SysInfo
	if r.GetSysInfo != nil {
	si = r.GetSysInfo()
	} else {
	si = sysinfo.New()
	}

	return decodeContainerConfig(src, si)
	}

	// decodeContainerConfig decodes a json encoded [container.CreateRequest] struct
	// and returns the Config, HostConfig, and NetworkingConfig struct, and performs some
	// validation. Certain parameters need daemon-side validation that cannot be done
	// on the client, as only the daemon knows what is valid for the platform.
	// Be aware this function is not checking whether the resulted structs are nil,
	// it's your business to do so
	func decodeContainerConfig(src io.Reader, si sysinfo.SysInfo) (container.Config, container.HostConfig, network.NetworkingConfig, error) {
	var w container.CreateRequest
	if err := loadJSON(src, &w); err != nil {
	return nil, nil, nil, err
	}

	hc := w.HostConfig
	if hc == nil {
	// We may not be passed a host config, such as in the case of docker commit
	return w.Config, hc, w.NetworkingConfig, nil
	}

	// Make sure NetworkMode has an acceptable value. We do this to ensure
	// backwards compatible API behavior.
	//
	// TODO(thaJeztah): platform check may be redundant, as other code-paths execute this unconditionally. Also check if this code is still needed here, or already handled elsewhere.
	if runtime.GOOS != "windows" && hc.NetworkMode == "" {
	hc.NetworkMode = network.NetworkDefault
	}
	if err := validateNetMode(w.Config, hc); err != nil {
	return nil, nil, nil, err
	}
	if err := validateIsolation(hc); err != nil {
	return nil, nil, nil, err
	}
	if err := validateQoS(hc); err != nil {
	return nil, nil, nil, err
	}
	if err := validateResources(hc, si); err != nil {
	return nil, nil, nil, err
	}
	if err := validatePrivileged(hc); err != nil {
	return nil, nil, nil, err
	}
	if err := validateReadonlyRootfs(hc); err != nil {
	return nil, nil, nil, err
	}
	if w.Config != nil && w.Config.Volumes == nil {
	w.Config.Volumes = make(map[string]struct{})
	}
	return w.Config, hc, w.NetworkingConfig, nil
	}

	if hc.UTSMode.IsHost() && c.Hostname != "" {
	return ErrConflictUTSHostname
	}

	if len(c.ExposedPorts) > 0 {
	return ErrConflictNetworkExposePorts
	}

Conversation

thaJeztah commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 21, 2025

Uh oh!

thaJeztah commented Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thaJeztah commented Jul 20, 2025 •

edited

Loading

thaJeztah commented Jul 27, 2025 •

edited

Loading