Skip to content

Commit f85394d

Browse files
thaJeztahthaJeztah
committed
api: image inspect: add back fields that did not omitempty
commit 4dc961d removed deprecated fields from the image inspect response for API v1.50 and up. As part of that change, it changed the type used for the Config field to use the docker image spect structs, which embeds the OCI image spec structs. While the OCI image spect struct contains the same fields as we used before, those fields also have "omitempty" set, which means they are now omitted when empty. We should probably consider deprecating that behavior in the API, and call out that these fields are omitted if not set, but in the meantime, we can add them back with their default (zero) value. Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent 9663b36 commit f85394d

4 files changed

Lines changed: 173 additions & 43 deletions

File tree

‎api/server/router/image/image_routes.go‎

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/docker/docker/api"
1616
"github.com/docker/docker/api/server/httputils"
1717
"github.com/docker/docker/api/types/backend"
18-
"github.com/docker/docker/api/types/container"
1918
"github.com/docker/docker/api/types/filters"
2019
imagetypes "github.com/docker/docker/api/types/image"
2120
"github.com/docker/docker/api/types/registry"
@@ -27,8 +26,6 @@ import (
2726
"github.com/docker/docker/pkg/ioutils"
2827
"github.com/docker/docker/pkg/progress"
2928
"github.com/docker/docker/pkg/streamformatter"
30-
"github.com/docker/go-connections/nat"
31-
dockerspec "github.com/moby/docker-image-spec/specs-go/v1"
3229
"github.com/opencontainers/go-digest"
3330
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
3431
"github.com/pkg/errors"
@@ -370,14 +367,22 @@ func (ir *imageRouter) getImagesByName(ctx context.Context, w http.ResponseWrite
370367
return errdefs.InvalidParameter(errors.New("conflicting options: manifests and platform options cannot both be set"))
371368
}
372369

373-
imageInspect, err := ir.backend.ImageInspect(ctx, vars["name"], backend.ImageInspectOpts{
370+
resp, err := ir.backend.ImageInspect(ctx, vars["name"], backend.ImageInspectOpts{
374371
Manifests: manifests,
375372
Platform: platform,
376373
})
377374
if err != nil {
378375
return err
379376
}
380377

378+
// inspectResponse preserves fields in the response that have an
379+
// "omitempty" in the OCI spec, but didn't omit such fields in
380+
// legacy responses before API v1.50.
381+
imageInspect := &inspectCompatResponse{
382+
InspectResponse: resp,
383+
legacyConfig: legacyConfigFields["current"],
384+
}
385+
381386
// Make sure we output empty arrays instead of nil. While Go nil slice is functionally equivalent to an empty slice,
382387
// it matters for the JSON representation.
383388
if imageInspect.RepoTags == nil {
@@ -405,14 +410,7 @@ func (ir *imageRouter) getImagesByName(ctx context.Context, w http.ResponseWrite
405410
imageInspect.Descriptor = nil
406411
}
407412
if versions.LessThan(version, "1.50") {
408-
type imageInspectLegacy struct {
409-
imagetypes.InspectResponse
410-
LegacyConfig *container.Config `json:"Config"`
411-
}
412-
return httputils.WriteJSON(w, http.StatusOK, imageInspectLegacy{
413-
InspectResponse: *imageInspect,
414-
LegacyConfig: dockerOCIImageConfigToContainerConfig(*imageInspect.Config),
415-
})
413+
imageInspect.legacyConfig = legacyConfigFields["v1.49"]
416414
}
417415

418416
return httputils.WriteJSON(w, http.StatusOK, imageInspect)
@@ -598,27 +596,3 @@ func validateRepoName(name reference.Named) error {
598596
}
599597
return nil
600598
}
601-
602-
// FIXME(thaJeztah): this is a copy of dockerOCIImageConfigToContainerConfig in daemon/containerd: https://github.com/moby/moby/blob/6b617699c500522aa6526cfcae4558333911b11f/daemon/containerd/imagespec.go#L107-L128
603-
func dockerOCIImageConfigToContainerConfig(cfg dockerspec.DockerOCIImageConfig) *container.Config {
604-
exposedPorts := make(nat.PortSet, len(cfg.ExposedPorts))
605-
for k, v := range cfg.ExposedPorts {
606-
exposedPorts[nat.Port(k)] = v
607-
}
608-
609-
return &container.Config{
610-
Entrypoint: cfg.Entrypoint,
611-
Env: cfg.Env,
612-
Cmd: cfg.Cmd,
613-
User: cfg.User,
614-
WorkingDir: cfg.WorkingDir,
615-
ExposedPorts: exposedPorts,
616-
Volumes: cfg.Volumes,
617-
Labels: cfg.Labels,
618-
ArgsEscaped: cfg.ArgsEscaped, //nolint:staticcheck // Ignore SA1019. Need to keep it in image.
619-
StopSignal: cfg.StopSignal,
620-
Healthcheck: cfg.Healthcheck,
621-
OnBuild: cfg.OnBuild,
622-
Shell: cfg.Shell,
623-
}
624-
}

‎api/server/router/image/inspect_response.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
// FIXME(thaJeztah): remove once we are a module; the go:build directive prevents go from downgrading language version to go1.16:
2+
//go:build go1.23
3+
4+
package image
5+
6+
import (
7+
"encoding/json"
8+
"maps"
9+
10+
"github.com/docker/docker/api/types/image"
11+
)
12+
13+
// legacyConfigFields defines legacy image-config fields to include in
14+
// API responses on older API versions.
15+
var legacyConfigFields = map[string]map[string]any{
16+
// Legacy fields for API v1.49 and lower. These fields are deprecated
17+
// and omitted in newer API versions; see https://github.com/moby/moby/pull/48457
18+
"v1.49": {
19+
"AttachStderr": false,
20+
"AttachStdin": false,
21+
"AttachStdout": false,
22+
"Cmd": nil,
23+
"Domainname": "",
24+
"Entrypoint": nil,
25+
"Env": nil,
26+
"Hostname": "",
27+
"Image": "",
28+
"Labels": nil,
29+
"OnBuild": nil,
30+
"OpenStdin": false,
31+
"StdinOnce": false,
32+
"Tty": false,
33+
"User": "",
34+
"Volumes": nil,
35+
"WorkingDir": "",
36+
},
37+
// Legacy fields for current API versions (v1.50 and up). These fields
38+
// did not have an "omitempty" and were always included in the response,
39+
// even if not set; see https://github.com/moby/moby/issues/50134
40+
"current": {
41+
"Cmd": nil,
42+
"Entrypoint": nil,
43+
"Env": nil,
44+
"Labels": nil,
45+
"OnBuild": nil,
46+
"User": "",
47+
"Volumes": nil,
48+
"WorkingDir": "",
49+
},
50+
}
51+
52+
// inspectCompatResponse is a wrapper around [image.InspectResponse] with a
53+
// custom marshal function for legacy [api/types/container.Config} fields
54+
// that have been removed, or did not have omitempty.
55+
type inspectCompatResponse struct {
56+
*image.InspectResponse
57+
legacyConfig map[string]any
58+
}
59+
60+
// MarshalJSON implements a custom marshaler to include legacy fields
61+
// in API responses.
62+
func (ir *inspectCompatResponse) MarshalJSON() ([]byte, error) {
63+
type tmp *image.InspectResponse
64+
base, err := json.Marshal((tmp)(ir.InspectResponse))
65+
if err != nil {
66+
return nil, err
67+
}
68+
if len(ir.legacyConfig) == 0 {
69+
return base, nil
70+
}
71+
72+
type resp struct {
73+
*image.InspectResponse
74+
Config map[string]any
75+
}
76+
77+
var merged resp
78+
err = json.Unmarshal(base, &merged)
79+
if err != nil {
80+
return base, nil
81+
}
82+
83+
// prevent mutating legacyConfigFields.
84+
cfg := maps.Clone(ir.legacyConfig)
85+
maps.Copy(cfg, merged.Config)
86+
merged.Config = cfg
87+
return json.Marshal(merged)
88+
}

‎api/server/router/image/inspect_response_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
package image
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
7+
"github.com/docker/docker/api/types/image"
8+
dockerspec "github.com/moby/docker-image-spec/specs-go/v1"
9+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
10+
"gotest.tools/v3/assert"
11+
is "gotest.tools/v3/assert/cmp"
12+
)
13+
14+
func TestInspectResponse(t *testing.T) {
15+
tests := []struct {
16+
doc string
17+
cfg *ocispec.ImageConfig
18+
legacyConfig map[string]any
19+
expected string
20+
}{
21+
{
22+
doc: "empty",
23+
expected: `null`,
24+
},
25+
{
26+
doc: "no legacy config",
27+
cfg: &ocispec.ImageConfig{
28+
Cmd: []string{"/bin/sh"},
29+
StopSignal: "SIGQUIT",
30+
},
31+
expected: `{"Cmd":["/bin/sh"],"StopSignal":"SIGQUIT"}`,
32+
},
33+
{
34+
doc: "api < v1.50",
35+
cfg: &ocispec.ImageConfig{
36+
Cmd: []string{"/bin/sh"},
37+
StopSignal: "SIGQUIT",
38+
},
39+
legacyConfig: legacyConfigFields["v1.49"],
40+
expected: `{"AttachStderr":false,"AttachStdin":false,"AttachStdout":false,"Cmd":["/bin/sh"],"Domainname":"","Entrypoint":null,"Env":null,"Hostname":"","Image":"","Labels":null,"OnBuild":null,"OpenStdin":false,"StdinOnce":false,"StopSignal":"SIGQUIT","Tty":false,"User":"","Volumes":null,"WorkingDir":""}`,
41+
},
42+
{
43+
doc: "api >= v1.50",
44+
cfg: &ocispec.ImageConfig{
45+
Cmd: []string{"/bin/sh"},
46+
StopSignal: "SIGQUIT",
47+
},
48+
legacyConfig: legacyConfigFields["current"],
49+
expected: `{"Cmd":["/bin/sh"],"Entrypoint":null,"Env":null,"Labels":null,"OnBuild":null,"StopSignal":"SIGQUIT","User":"","Volumes":null,"WorkingDir":""}`,
50+
},
51+
}
52+
for _, tc := range tests {
53+
t.Run(tc.doc, func(t *testing.T) {
54+
imgInspect := &image.InspectResponse{}
55+
if tc.cfg != nil {
56+
// Verify that fields that are set override the legacy values,
57+
// or appended if not part of the legacy values.
58+
imgInspect.Config = &dockerspec.DockerOCIImageConfig{
59+
ImageConfig: *tc.cfg,
60+
}
61+
}
62+
out, err := json.Marshal(&inspectCompatResponse{
63+
InspectResponse: imgInspect,
64+
legacyConfig: tc.legacyConfig,
65+
})
66+
assert.NilError(t, err)
67+
68+
var outMap struct{ Config json.RawMessage }
69+
err = json.Unmarshal(out, &outMap)
70+
assert.NilError(t, err)
71+
assert.Check(t, is.Equal(string(outMap.Config), tc.expected))
72+
})
73+
}
74+
}

‎integration-cli/docker_cli_build_test.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3138,13 +3138,7 @@ func (s *DockerCLIBuildSuite) TestBuildClearCmd(c *testing.T) {
31383138
CMD []`))
31393139

31403140
cmd := inspectFieldJSON(c, name, "Config.Cmd")
3141-
// OCI types specify `omitempty` JSON annotation which doesn't serialize
3142-
// empty arrays and the Cmd will not be present at all.
3143-
if testEnv.UsingSnapshotter() {
3144-
assert.Check(c, is.Equal(cmd, "null"))
3145-
} else {
3146-
assert.Check(c, is.Equal(cmd, "[]"))
3147-
}
3141+
assert.Check(c, is.Equal(cmd, "null"))
31483142
}
31493143

31503144
func (s *DockerCLIBuildSuite) TestBuildEmptyCmd(c *testing.T) {

0 commit comments

Comments
 (0)