api: image inspect: add back fields that did not omitempty #50135
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| // FIXME(thaJeztah): remove once we are a module; the go:build directive prevents go from downgrading language version to go1.16: | ||
| //go:build go1.23 | ||
|
|
||
| package image | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "maps" | ||
|
|
||
| "github.com/docker/docker/api/types/image" | ||
| ) | ||
|
|
||
| // legacyConfigFields defines legacy image-config fields to include in | ||
| // API responses on older API versions. | ||
| var legacyConfigFields = map[string]map[string]any{ | ||
| // Legacy fields for API v1.49 and lower. These fields are deprecated | ||
| // and omitted in newer API versions; see https://github.com/moby/moby/pull/48457 | ||
| "v1.49": { | ||
| "AttachStderr": false, | ||
| "AttachStdin": false, | ||
| "AttachStdout": false, | ||
| "Cmd": nil, | ||
| "Domainname": "", | ||
| "Entrypoint": nil, | ||
| "Env": nil, | ||
| "Hostname": "", | ||
| "Image": "", | ||
| "Labels": nil, | ||
| "OnBuild": nil, | ||
| "OpenStdin": false, | ||
| "StdinOnce": false, | ||
| "Tty": false, | ||
| "User": "", | ||
| "Volumes": nil, | ||
| "WorkingDir": "", | ||
| }, | ||
| // Legacy fields for current API versions (v1.50 and up). These fields | ||
| // did not have an "omitempty" and were always included in the response, | ||
| // even if not set; see https://github.com/moby/moby/issues/50134 | ||
| "current": { | ||
| "Cmd": nil, | ||
| "Entrypoint": nil, | ||
| "Env": nil, | ||
| "Labels": nil, | ||
| "OnBuild": nil, | ||
| "User": "", | ||
| "Volumes": nil, | ||
| "WorkingDir": "", | ||
| }, | ||
|
Comment on lines
+37
to
+49
Contributor
There was a problem hiding this comment. I think we can avoid this one and just marshal the base
Member
Author
There was a problem hiding this comment. These were the ones that had an omit empty, so were not included #50134 So I think using the base would then result in them disappearing again. But for api v1.51 we can consider doing so, but we'd have to deprecate that behavior first (can do that in a separate PR)
Contributor
There was a problem hiding this comment. Right... I think what confused me is that the caller is responsible for passing the correct value of Could we just pass the API version the response should be adapted to? So imageInspect := &inspectCompatResponse{
InspectResponse: resp,
}would mean the "current"/"latest" and imageInspect := &inspectCompatResponse{
InspectResponse: resp,
version: "1.49"
}would mean the 1.49 compatible response.. Although that's probably what you meant in https://github.com/moby/moby/pull/50135/files#r2126024062?
Contributor
There was a problem hiding this comment. Btw, I think it would be slightly easier to understand if we declared a copy-pasted, unexported versions of the "legacy" Config struct and just explicitly filled and marshalled them inside Just a thought though, not a blocker
Member
Author
There was a problem hiding this comment.
Yeah, that's something I was considering, and mostly had written, then threw it away thinking to KISS for now. I can still have a look at that though.
Ah, gotcha. Hm.. yes, possibly could work. My goal at least was to avoid having to depend on legacy types or fields if that meant we'd have to continue maintaining the fields (as "deprecated") purely for the old responses (but with empty / zero values). Having a (non-exported) copy of such structs purely for that purpose is indeed slightly better; what I slightly tried to avoid though was to (in some cases) that potentially requiring to also use legacy indirect types, such as the moby/api/server/router/image/image_routes.go Lines 604 to 609 in 9663b36 I recall we used to have these "versioned" legacy types, which I guess is a bit of that approach; some of those started to become quite involved though; |
||
| } | ||
|
|
||
| // inspectCompatResponse is a wrapper around [image.InspectResponse] with a | ||
| // custom marshal function for legacy [api/types/container.Config} fields | ||
| // that have been removed, or did not have omitempty. | ||
| type inspectCompatResponse struct { | ||
| *image.InspectResponse | ||
| legacyConfig map[string]any | ||
| } | ||
|
|
||
| // MarshalJSON implements a custom marshaler to include legacy fields | ||
| // in API responses. | ||
| func (ir *inspectCompatResponse) MarshalJSON() ([]byte, error) { | ||
| type tmp *image.InspectResponse | ||
| base, err := json.Marshal((tmp)(ir.InspectResponse)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(ir.legacyConfig) == 0 { | ||
| return base, nil | ||
| } | ||
|
|
||
| type resp struct { | ||
| *image.InspectResponse | ||
| Config map[string]any | ||
| } | ||
|
|
||
| var merged resp | ||
| err = json.Unmarshal(base, &merged) | ||
| if err != nil { | ||
| return base, nil | ||
| } | ||
|
|
||
| // prevent mutating legacyConfigFields. | ||
| cfg := maps.Clone(ir.legacyConfig) | ||
|
Comment on lines
+83
to
+84
Member
Author
There was a problem hiding this comment. This was the problem; I added the |
||
| maps.Copy(cfg, merged.Config) | ||
| merged.Config = cfg | ||
| return json.Marshal(merged) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package image | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/docker/docker/api/types/image" | ||
| dockerspec "github.com/moby/docker-image-spec/specs-go/v1" | ||
| ocispec "github.com/opencontainers/image-spec/specs-go/v1" | ||
| "gotest.tools/v3/assert" | ||
| is "gotest.tools/v3/assert/cmp" | ||
| ) | ||
|
|
||
| func TestInspectResponse(t *testing.T) { | ||
| tests := []struct { | ||
| doc string | ||
| cfg *ocispec.ImageConfig | ||
| legacyConfig map[string]any | ||
| expected string | ||
| }{ | ||
| { | ||
| doc: "empty", | ||
| expected: `null`, | ||
| }, | ||
| { | ||
| doc: "no legacy config", | ||
| cfg: &ocispec.ImageConfig{ | ||
| Cmd: []string{"/bin/sh"}, | ||
| StopSignal: "SIGQUIT", | ||
| }, | ||
| expected: `{"Cmd":["/bin/sh"],"StopSignal":"SIGQUIT"}`, | ||
| }, | ||
| { | ||
| doc: "api < v1.50", | ||
| cfg: &ocispec.ImageConfig{ | ||
| Cmd: []string{"/bin/sh"}, | ||
| StopSignal: "SIGQUIT", | ||
| }, | ||
| legacyConfig: legacyConfigFields["v1.49"], | ||
| 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":""}`, | ||
| }, | ||
| { | ||
| doc: "api >= v1.50", | ||
| cfg: &ocispec.ImageConfig{ | ||
| Cmd: []string{"/bin/sh"}, | ||
| StopSignal: "SIGQUIT", | ||
| }, | ||
| legacyConfig: legacyConfigFields["current"], | ||
| expected: `{"Cmd":["/bin/sh"],"Entrypoint":null,"Env":null,"Labels":null,"OnBuild":null,"StopSignal":"SIGQUIT","User":"","Volumes":null,"WorkingDir":""}`, | ||
| }, | ||
| } | ||
| for _, tc := range tests { | ||
| t.Run(tc.doc, func(t *testing.T) { | ||
| imgInspect := &image.InspectResponse{} | ||
| if tc.cfg != nil { | ||
| // Verify that fields that are set override the legacy values, | ||
| // or appended if not part of the legacy values. | ||
| imgInspect.Config = &dockerspec.DockerOCIImageConfig{ | ||
| ImageConfig: *tc.cfg, | ||
| } | ||
| } | ||
| out, err := json.Marshal(&inspectCompatResponse{ | ||
| InspectResponse: imgInspect, | ||
| legacyConfig: tc.legacyConfig, | ||
| }) | ||
| assert.NilError(t, err) | ||
|
|
||
| var outMap struct{ Config json.RawMessage } | ||
| err = json.Unmarshal(out, &outMap) | ||
| assert.NilError(t, err) | ||
| assert.Check(t, is.Equal(string(outMap.Config), tc.expected)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW; I started to look if adding a constructor for this would be cleaner, and possibly it could be, but rewriting took me down the rabbit hole, so I'm gonna leave that for a future refactor; possibly combining all the version-related changes below 🤔