image-inspect: remove Config fields that are not part of the image #48457

thaJeztah · 2024-09-10T08:20:42Z

relates to:

commit af0cdc3 marked these fields as deprecated and to be removed in API v1.47 (which was targeted for v28.0). We shipped v1.47 with the v27.2 release, but did not yet remove the erroneous fields, so the version to deprecate was updated to v1.48 through 3df03d8

This patch removes fields that are not part of the image by replacing the type with the Config struct from the docker image-spec.

curl -s --unix-socket /var/run/docker.sock http://localhost/v1.47/images/alpine/json | jq .Config
{
  "Env": [
    "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
  ],
  "Cmd": [
    "/bin/sh"
  ]
}

curl -s --unix-socket /var/run/docker.sock http://localhost/v1.46/images/alpine/json | jq .Config
{
  "Hostname": "",
  "Domainname": "",
  "User": "",
  "AttachStdin": false,
  "AttachStdout": false,
  "AttachStderr": false,
  "Tty": false,
  "OpenStdin": false,
  "StdinOnce": false,
  "Env": [
    "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
  ],
  "Cmd": [
    "/bin/sh"
  ],
  "Image": "",
  "Volumes": null,
  "WorkingDir": "",
  "Entrypoint": null,
  "OnBuild": null,
  "Labels": null
}

- What I did

- How I did it

- How to verify it

- Description for the changelog

API: Deprecated: `GET /images/{name}/json` no longer returns the following `Config`
fields; `Hostname`, `Domainname`, `AttachStdin`, `AttachStdout`, `AttachStderr`
`Tty`, `OpenStdin`, `StdinOnce`, `Image`, `NetworkDisabled` (already omitted unless set),
`MacAddress` (already omitted unless set), `StopTimeout` (already omitted unless set).
These additional fields were included in the response due to an implementation
detail but not part of the image's Configuration. These fields were marked
deprecated in API v1.46, and are now omitted. Older versions of the API still
return these fields, but they are always empty.

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

thaJeztah · 2024-09-10T08:22:04Z

Keeping this in draft for now; it looks like the image-spec types use omitempty for most fields, which means that fields that were not set will be omitted.

thaJeztah · 2024-09-10T08:22:47Z

api/server/router/image/image_routes.go

+// FIXME(thaJeztah): this is a copy of dockerOCIImageConfigToContainerConfig in daemon/containerd: https://github.com/moby/moby/blob/6b617699c500522aa6526cfcae4558333911b11f/daemon/containerd/imagespec.go#L107-L128
+func dockerOCIImageConfigToContainerConfig(cfg dockerspec.DockerOCIImageConfig) *container.Config {


This was copied from

moby/daemon/containerd/imagespec.go

Lines 107 to 128 in 6b61769

func dockerOCIImageConfigToContainerConfig(cfg imagespec.DockerOCIImageConfig) *container.Config {

exposedPorts := make(nat.PortSet, len(cfg.ExposedPorts))

for k, v := range cfg.ExposedPorts {

exposedPorts[nat.Port(k)] = v

}

return &container.Config{

Entrypoint: cfg.Entrypoint,

Env: cfg.Env,

Cmd: cfg.Cmd,

User: cfg.User,

WorkingDir: cfg.WorkingDir,

ExposedPorts: exposedPorts,

Volumes: cfg.Volumes,

Labels: cfg.Labels,

ArgsEscaped: cfg.ArgsEscaped, //nolint:staticcheck // Ignore SA1019. Need to keep it in image.

StopSignal: cfg.StopSignal,

Healthcheck: cfg.Healthcheck,

OnBuild: cfg.OnBuild,

Shell: cfg.Shell,

}

}

We should probably look at moving that to an internal/ package so that we can re-use those functions

thaJeztah · 2024-09-10T08:24:10Z

daemon/images/image_inspect.go

@@ -53,6 +53,7 @@ func (i *ImageService) ImageInspect(ctx context.Context, refOrID string, _ backe
 		layers = append(layers, l.String())
 	}

+	imgConfig := containerConfigToDockerOCIImageConfig(img.Config)


Also wondering if we should consider (and if it'd be possible) to change the image-type we store on disk to use the new type so that we don't have to perform conversion (other than when committing a container to an image)

thaJeztah · 2024-09-10T08:32:19Z

Looks like docker-py may need an update, as I think it's checking for an empty OnBuild field to be present;

=================================== FAILURES ===================================
__________________ BuildTest.test_build_container_with_target __________________
tests/integration/api_build_test.py:278: in test_build_container_with_target
    assert not info['Config']['OnBuild']
E   KeyError: 'OnBuild'

edit: hm.. not sure now; it's assert not ? https://github.com/docker/docker-py/blob/a3652028b1ead708bd9191efb286f909ba6c2a49/tests/integration/api_build_test.py#L258-L278

edit2: ah; It's building the first target, which doesn't have OnBuild, so indeed it tries to verify that OnBuild is empty, but fails because the fields is not there (due to the omitempty.

☝️ opened integration: adjust tests for omitted "OnBuild" docker/docker-py#3336

thaJeztah · 2025-05-22T08:47:31Z

hack/make/test-docker-py

@@ -22,6 +22,9 @@ if [ -n "$TEST_INTEGRATION_USE_SNAPSHOTTER" ]; then
 	PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.py::BuildTest::test_build_squash"
 fi

+# TODO(thaJeztah) re-enable after https://github.com/docker/docker-py/pull/3336 is in the DOCKER_PY_COMMIT release.
+PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.pyy::BuildTest::test_build_container_with_target"


Arf;

Suggested change

PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.pyy::BuildTest::test_build_container_with_target"

PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.py::BuildTest::test_build_container_with_target"

Maybe instead of skipping, let's just pull docker-py by commit?

moby/hack/make/test-docker-py

Line 8 in 04d3817

: "${DOCKER_PY_COMMIT:=7.1.0}"

Yeah, there's a bunch of other tests that are also waiting for a new release; didn't want to include all of those in this PR (and ideally, look if we can have a new release of docker-py); let me keep that for a follow-up.

Mostly dusted-off this PR because we kept on forgetting about it 😂

Either way, fixed the typo for now 👍

vvoland · 2025-05-22T09:25:32Z

Looks like test is failing:

=== FAIL: amd64.integration-cli TestDockerRegistrySuite/TestBuildByDigest (0.81s)
    docker_cli_by_digest_test.go:191: assertion failed:  (res string) != sha256:56f26145b42bd638db4caef515cd8c72c9acfba61961545d7fe2ef0f4bc38f9e (imageID string)
    check_test.go:468: [db544858fd8a5] daemon is not started
    --- FAIL: TestDockerRegistrySuite/TestBuildByDigest (0.81s)

thaJeztah · 2025-05-22T09:58:40Z

Heh. Looks like that test piggy-backed on the Config.Image field being set (from the container's config);

moby/integration-cli/docker_cli_by_digest_test.go

Lines 188 to 189 in f4ffeb8

    
           // get the build's image id 
        
           res := inspectField(c, name, "Config.Image")

commit af0cdc3 marked these fields as deprecated and to be removed in API v1.47 (which was targeted for v28.0). We shipped v1.47 with the v27.2 release, but did not yet remove the erroneous fields, so the version to deprecate was updated to v1.48 through 3df03d8 This patch removes fields that are not part of the image by replacing the type with the Config struct from the docker image-spec. curl -s --unix-socket /var/run/docker.sock http://localhost/v1.50/images/alpine/json | jq .Config { "Env": [ "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" ], "Cmd": [ "/bin/sh" ] } curl -s --unix-socket /var/run/docker.sock http://localhost/v1.49/images/alpine/json | jq .Config { "Hostname": "", "Domainname": "", "User": "", "AttachStdin": false, "AttachStdout": false, "AttachStderr": false, "Tty": false, "OpenStdin": false, "StdinOnce": false, "Env": [ "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" ], "Cmd": [ "/bin/sh" ], "Image": "", "Volumes": null, "WorkingDir": "", "Entrypoint": null, "OnBuild": null, "Labels": null } Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2025-05-22T10:11:10Z

integration-cli/docker_cli_by_digest_test.go

+	// verify the build was ok
+	res := inspectField(c, name, "Config.Cmd")
+	assert.Equal(c, res, `[/bin/echo Hello World]`)


Changed it to a sanity check that the build indeed "built" (may even be redundant)

make BIND_DIR=. TEST_FILTER='TestBuildByDigest' test-integration ... === RUN TestDockerRegistrySuite/TestBuildByDigest check_test.go:468: [de4fbab56e382] daemon is not started --- PASS: TestDockerRegistrySuite (1.80s) --- PASS: TestDockerRegistrySuite/TestBuildByDigest (1.80s)

thaJeztah commented Sep 10, 2024

View reviewed changes

thaJeztah force-pushed the api_remove_deprecated_fields branch 5 times, most recently from 3df5ef6 to 6feb664 Compare May 21, 2025 12:50

thaJeztah added area/api status/2-code-review impact/api impact/changelog impact/deprecation area/go-sdk Changes affecting the Go SDK labels May 21, 2025

thaJeztah marked this pull request as ready for review May 21, 2025 12:52

thaJeztah force-pushed the api_remove_deprecated_fields branch from 6feb664 to c6e7214 Compare May 21, 2025 23:43

thaJeztah requested a review from tianon as a code owner May 21, 2025 23:43

thaJeztah mentioned this pull request May 21, 2025

integration: adjust tests for omitted "OnBuild" docker/docker-py#3336

Merged

thaJeztah force-pushed the api_remove_deprecated_fields branch from c6e7214 to 4b7dd37 Compare May 21, 2025 23:49

thaJeztah added this to the 28.2.0 milestone May 21, 2025

thaJeztah requested a review from vvoland May 21, 2025 23:55

thaJeztah commented May 22, 2025

View reviewed changes

thaJeztah force-pushed the api_remove_deprecated_fields branch from 4b7dd37 to 9da4321 Compare May 22, 2025 08:59

thaJeztah added 2 commits May 22, 2025 12:09

docs/api: update deprecation version for erroneous fields

505bd61

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the api_remove_deprecated_fields branch from 9da4321 to 505bd61 Compare May 22, 2025 10:09

thaJeztah commented May 22, 2025

View reviewed changes

thaJeztah mentioned this pull request May 22, 2025

vendor: github.com/docker/docker master (v28.2-dev) docker/cli#6099

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

image-inspect: remove Config fields that are not part of the image #48457

image-inspect: remove Config fields that are not part of the image #48457

Uh oh!

thaJeztah commented Sep 10, 2024 •

edited by vvoland

Loading

Uh oh!

thaJeztah commented Sep 10, 2024 •

edited

Loading

Uh oh!

thaJeztah Sep 10, 2024

Uh oh!

thaJeztah Sep 10, 2024

Uh oh!

thaJeztah commented Sep 10, 2024 •

edited

Loading

Uh oh!

thaJeztah May 22, 2025

Uh oh!

vvoland May 22, 2025 •

edited

Loading

Uh oh!

thaJeztah May 22, 2025

Uh oh!

thaJeztah May 22, 2025

Uh oh!

vvoland commented May 22, 2025

Uh oh!

thaJeztah commented May 22, 2025

Uh oh!

thaJeztah May 22, 2025

Uh oh!

Uh oh!

		// FIXME(thaJeztah): this is a copy of dockerOCIImageConfigToContainerConfig in daemon/containerd: https://github.com/moby/moby/blob/6b617699c500522aa6526cfcae4558333911b11f/daemon/containerd/imagespec.go#L107-L128
		func dockerOCIImageConfigToContainerConfig(cfg dockerspec.DockerOCIImageConfig) *container.Config {

	func dockerOCIImageConfigToContainerConfig(cfg imagespec.DockerOCIImageConfig) *container.Config {
	exposedPorts := make(nat.PortSet, len(cfg.ExposedPorts))
	for k, v := range cfg.ExposedPorts {
	exposedPorts[nat.Port(k)] = v
	}

	return &container.Config{
	Entrypoint: cfg.Entrypoint,
	Env: cfg.Env,
	Cmd: cfg.Cmd,
	User: cfg.User,
	WorkingDir: cfg.WorkingDir,
	ExposedPorts: exposedPorts,
	Volumes: cfg.Volumes,
	Labels: cfg.Labels,
	ArgsEscaped: cfg.ArgsEscaped, //nolint:staticcheck // Ignore SA1019. Need to keep it in image.
	StopSignal: cfg.StopSignal,
	Healthcheck: cfg.Healthcheck,
	OnBuild: cfg.OnBuild,
	Shell: cfg.Shell,
	}
	}

	PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.pyy::BuildTest::test_build_container_with_target"
	PY_TEST_OPTIONS="$PY_TEST_OPTIONS --deselect=tests/integration/api_build_test.py::BuildTest::test_build_container_with_target"

image-inspect: remove Config fields that are not part of the image #48457

Are you sure you want to change the base?

image-inspect: remove Config fields that are not part of the image #48457

Uh oh!

Conversation

thaJeztah commented Sep 10, 2024 • edited by vvoland Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah May 22, 2025

Choose a reason for hiding this comment

Uh oh!

vvoland May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 22, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 22, 2025

Choose a reason for hiding this comment

Uh oh!

vvoland commented May 22, 2025

Uh oh!

thaJeztah commented May 22, 2025

Uh oh!

thaJeztah May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Sep 10, 2024 •

edited by vvoland

Loading

thaJeztah commented Sep 10, 2024 •

edited

Loading

thaJeztah commented Sep 10, 2024 •

edited

Loading

vvoland May 22, 2025 •

edited

Loading