Skip to content

Commit fffb364

Browse files
authored
[CONS-8441] docker: accept port ranges in container ExposedPorts (#54108)
### What does this PR do? Prevents the Docker collector from dropping containers whose image metadata declares a port range in `Config.ExposedPorts`, such as `1061-1070`. When Moby rejects such a port key, `InspectNoCache` now refetches the raw inspect response, expands valid ranges into individual ports, and decodes it again. The fallback preserves the original error unless recovery is verified to be safe. Ranges larger than 1024 ports are dropped to prevent excessive expansion. Bazel targets and a release note are included. ### Motivation Older Docker daemons can return baked-in port ranges verbatim, while Moby’s strict port decoder rejects them. Because the port is a JSON map key, the entire container inspect fails, causing the Agent to skip all metrics, metadata, and autodiscovery for that container. This addresses customer escalation [CONS-8441](https://datadoghq.atlassian.net/browse/CONS-8441). ### Describe how you validated your changes - unit test - run on Docker v25.0.5: We use community-cluster:8.4 because it carries a JSON config. This one literally contains the string "33060-33061/tcp" as a key: `"ExposedPorts": {"1186/tcp":{}, "2202/tcp":{}, "3306/tcp":{}, "33060-33061/tcp":{}} ` ```bash docker network create ddnet25 docker run -d --privileged --name dind25 --network ddnet25 -e DOCKER_TLS_CERTDIR="" \ docker:25-dind --host=tcp://0.0.0.0:2375 docker exec dind25 docker pull container-registry.oracle.com/mysql/community-cluster:8.4 docker exec dind25 docker create --name mc container-registry.oracle.com/mysql/community-cluster:8.4 docker exec dind25 docker inspect mc --format '{{json .Config.ExposedPorts}}' # {"1186/tcp":{},"2202/tcp":{},"3306/tcp":{},"33060-33061/tcp":{}} <-- the range survives ``` before, `ContainerInspect` fails with `invalid port '33060-33061': invalid syntax`; after, the container is inspected normally with `33060/tcp` and `33061/tcp` expanded. [CONS-8441]: https://datadoghq.atlassian.net/browse/CONS-8441?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: minyi.zhu <minyi.zhu@datadoghq.com>
1 parent af98928 commit fffb364

5 files changed

Lines changed: 574 additions & 1 deletion

File tree

pkg/util/docker/BUILD.bazel

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ go_library(
1414
"global.go",
1515
"global_nodocker.go",
1616
"host_tags.go",
17+
"inspect_ports.go",
1718
"metadata.go",
1819
"metadata_no_docker.go",
1920
"rancher.go",
@@ -39,6 +40,7 @@ go_library(
3940
"@com_github_moby_moby_api//types/container",
4041
"@com_github_moby_moby_api//types/events",
4142
"@com_github_moby_moby_api//types/image",
43+
"@com_github_moby_moby_api//types/network",
4244
"@com_github_moby_moby_api//types/swarm",
4345
"@com_github_moby_moby_api//types/system",
4446
"@com_github_moby_moby_client//:client",
@@ -52,6 +54,7 @@ dd_agent_go_test(
5254
"event_pull_test.go",
5355
"event_stream_test.go",
5456
"host_tags_test.go",
57+
"inspect_ports_test.go",
5558
"rancher_test.go",
5659
"safe_info_test.go",
5760
"storage_test.go",
@@ -63,6 +66,7 @@ dd_agent_go_test(
6366
"//pkg/config/mock",
6467
"@com_github_moby_moby_api//types/container",
6568
"@com_github_moby_moby_api//types/events",
69+
"@com_github_moby_moby_api//types/network",
6670
"@com_github_moby_moby_api//types/swarm",
6771
"@com_github_moby_moby_api//types/system",
6872
"@com_github_moby_moby_client//:client",

pkg/util/docker/docker_util.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,19 @@ func (d *DockerUtil) InspectNoCache(ctx context.Context, id string, withSize boo
372372
return container, dderrors.NewNotFound("docker container " + id)
373373
}
374374
if err != nil {
375-
return container, err
375+
// A port key the strict decoder rejects fails the entire inspect, so try
376+
// to recover it rather than dropping the container. recoverInspect
377+
// confirms the payload was really at fault; anything else falls through
378+
// to the original error. See CONS-8441.
379+
if !isInvalidPortKeyError(err) {
380+
return container, err
381+
}
382+
if c, ok := d.recoverInspect(ctx, id, withSize); ok {
383+
log.Debugf("recovered inspect for container %s after decode error: %s", id, err)
384+
container = c
385+
} else {
386+
return container, err
387+
}
376388
}
377389

378390
// Check for empty inspect data

pkg/util/docker/inspect_ports.go

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed
2+
// under the Apache License Version 2.0.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
// Copyright 2016-present Datadog, Inc.
5+
6+
//go:build docker
7+
8+
package docker
9+
10+
import (
11+
"context"
12+
"encoding/json"
13+
"fmt"
14+
"io"
15+
"net"
16+
"net/http"
17+
"path"
18+
"sort"
19+
"strings"
20+
21+
dcontainer "github.com/moby/moby/api/types/container"
22+
"github.com/moby/moby/api/types/network"
23+
"github.com/moby/moby/client"
24+
25+
"github.com/DataDog/datadog-agent/pkg/util/log"
26+
)
27+
28+
// isInvalidPortKeyError reports whether err looks like moby rejecting a port map
29+
// key: network.Port.UnmarshalText returns "invalid port '<key>': ...", which
30+
// aborts the whole ContainerInspect decode. It matches any unparseable key, not
31+
// just ranges, since the sanitizer drops those too and still recovers the rest
32+
// of the container. The single quote avoids matching unrelated errors such as
33+
// net/url's `invalid port "x"`.
34+
//
35+
// This is only a cheap pre-filter to avoid refetching on unrelated failures;
36+
// correctness does not depend on it. recoverInspect independently verifies the
37+
// payload actually contained a fixable key, so a future rewording of moby's
38+
// message would merely stop the recovery attempt (the pre-fix behaviour) rather
39+
// than produce a wrong result. See CONS-8441.
40+
func isInvalidPortKeyError(err error) bool {
41+
return err != nil && strings.Contains(err.Error(), "invalid port '")
42+
}
43+
44+
// recoverInspect attempts to recover a container inspect that moby failed to
45+
// decode, by refetching the raw payload, expanding any port-range keys the
46+
// strict decoder rejects (CONS-8441), and re-decoding. It does not trust the
47+
// error text for correctness: it reports ok=false — so the caller surfaces the
48+
// original error — when the payload held no range to expand, the re-decode still
49+
// failed, or the response is not the container that was requested.
50+
func (d *DockerUtil) recoverInspect(ctx context.Context, id string, withSize bool) (dcontainer.InspectResponse, bool) {
51+
var c dcontainer.InspectResponse
52+
53+
// A done context cannot produce a successful refetch, so don't add daemon
54+
// load for it: the caller surfaces the original (timeout/cancel) error.
55+
if ctx.Err() != nil {
56+
return c, false
57+
}
58+
59+
id = strings.TrimSpace(id) // moby's client trims too; keep the paths identical
60+
raw, err := d.rawContainerInspect(ctx, id, withSize)
61+
if err != nil {
62+
return c, false
63+
}
64+
sanitized, changed := sanitizeInspectPortRanges(raw)
65+
if !changed {
66+
return c, false // no port range present; not something we can fix
67+
}
68+
if err := json.Unmarshal(sanitized, &c); err != nil {
69+
return c, false
70+
}
71+
// Callers may pass a name or a short ID, and the container could have been
72+
// replaced between the failed inspect and this refetch, so make sure we are
73+
// about to return the container that was actually asked for.
74+
if !matchesContainer(c, id) {
75+
return c, false
76+
}
77+
return c, true
78+
}
79+
80+
// matchesContainer reports whether the inspect response identifies the container
81+
// referred to by ref, which may be a full ID, an ID prefix, or a name.
82+
func matchesContainer(c dcontainer.InspectResponse, ref string) bool {
83+
if c.ID == "" || ref == "" {
84+
return false
85+
}
86+
if strings.HasPrefix(c.ID, ref) {
87+
return true
88+
}
89+
// Inspect reports names with a leading slash ("/my-container").
90+
return strings.EqualFold(strings.TrimPrefix(c.Name, "/"), strings.TrimPrefix(ref, "/"))
91+
}
92+
93+
// rawContainerInspect issues GET /containers/<id>/json to the daemon through the
94+
// moby client's dialer and returns the raw, undecoded response body. moby's
95+
// ContainerInspect discards the raw bytes when its strict decode fails, so the
96+
// fallback must refetch them here to sanitize and re-decode. This mirrors the
97+
// raw-fetch in safe_info.go's tolerantInfo (the analogous /info strict-decode
98+
// workaround), kept separate to avoid coupling the two.
99+
func (d *DockerUtil) rawContainerInspect(ctx context.Context, id string, withSize bool) ([]byte, error) {
100+
httpClient := &http.Client{
101+
Transport: &http.Transport{
102+
// One-shot client: no benefit from keep-alive, and DisableKeepAlives
103+
// ensures the dialed connection is closed when the response body
104+
// is, so the unreferenced transport does not retain FDs.
105+
DisableKeepAlives: true,
106+
DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) {
107+
return d.cli.Dialer()(ctx)
108+
},
109+
},
110+
}
111+
112+
// The dialer takes care of reaching the daemon (unix, npipe, tcp, tcp+tls).
113+
// For TCP daemons, preserve the configured host and base path so reverse
114+
// proxies relying on Host-header or path routing reach the same backend
115+
// as the SDK does. For unix/npipe, the SDK uses DummyHost — match it.
116+
// Use the "http" scheme even for tls-fronted daemons: the dialer returns
117+
// an already-TLS-encrypted connection, and the http transport writes plain
118+
// HTTP bytes over it.
119+
reqHost := client.DummyHost
120+
basePath := ""
121+
if hostURL, err := client.ParseHostURL(d.cli.DaemonHost()); err == nil {
122+
if hostURL.Scheme == "tcp" {
123+
reqHost = hostURL.Host
124+
}
125+
basePath = hostURL.Path
126+
}
127+
// Match the SDK's path construction so reverse proxies routing on
128+
// /vX.Y/containers/... don't reject the fallback (see moby client.getAPIPath).
129+
apiPath := "/containers/" + id + "/json"
130+
if v := d.cli.ClientVersion(); v != "" {
131+
apiPath = "/v" + strings.TrimPrefix(v, "v") + apiPath
132+
}
133+
url := "http://" + reqHost + path.Join("/", basePath, apiPath)
134+
if withSize {
135+
// Mirror the SDK's Size option, or the recovered container would come
136+
// back with its size fields unset.
137+
url += "?size=1"
138+
}
139+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
140+
if err != nil {
141+
return nil, err
142+
}
143+
144+
resp, err := httpClient.Do(req)
145+
if err != nil {
146+
return nil, err
147+
}
148+
defer resp.Body.Close()
149+
150+
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
151+
return nil, fmt.Errorf("unexpected status %d from %s", resp.StatusCode, apiPath)
152+
}
153+
154+
return io.ReadAll(resp.Body)
155+
}
156+
157+
// sanitizeInspectPortRanges rewrites port-range keys ("1061-1070" -> "1061/tcp"
158+
// … "1070/tcp") in Config.ExposedPorts of a raw inspect payload so the strict
159+
// moby decoder can parse it, returning the payload and whether it changed.
160+
// ExposedPorts is the only inspect field observed to carry a baked-in range.
161+
func sanitizeInspectPortRanges(raw []byte) ([]byte, bool) {
162+
var top map[string]json.RawMessage
163+
if json.Unmarshal(raw, &top) != nil {
164+
return raw, false
165+
}
166+
var config map[string]json.RawMessage
167+
if json.Unmarshal(top["Config"], &config) != nil {
168+
return raw, false // Config absent, null, or not an object
169+
}
170+
ports, changed := expandPortKeys(config["ExposedPorts"])
171+
if !changed {
172+
return raw, false
173+
}
174+
config["ExposedPorts"] = ports
175+
176+
cfg, err := json.Marshal(config)
177+
if err != nil {
178+
return raw, false
179+
}
180+
top["Config"] = cfg
181+
out, err := json.Marshal(top)
182+
if err != nil {
183+
return raw, false
184+
}
185+
return out, true
186+
}
187+
188+
// maxExpandedPorts bounds how many ports the range keys of one object may expand
189+
// into in total. Pathological input ("1-65535", or many wide ranges) would
190+
// otherwise inflate the payload and push a huge number of ports into
191+
// workloadmeta for a single container. The budget is shared across the whole
192+
// object, not per key, so a payload full of ranges cannot multiply past it.
193+
// Ranges that do not fit are dropped, which still recovers the container.
194+
const maxExpandedPorts = 1024
195+
196+
// expandPortKeys rewrites one port-keyed object. Valid single ports are kept
197+
// (normalized so a range never overwrites an equivalent explicit key); ranges
198+
// are expanded within the maxExpandedPorts budget; keys that are neither are
199+
// dropped.
200+
func expandPortKeys(raw json.RawMessage) (json.RawMessage, bool) {
201+
var ports map[string]json.RawMessage
202+
if json.Unmarshal(raw, &ports) != nil {
203+
return nil, false
204+
}
205+
206+
out := make(map[string]json.RawMessage, len(ports))
207+
ranges := make([]string, 0, len(ports))
208+
changed := false
209+
for k, v := range ports {
210+
if p, err := network.ParsePort(k); err == nil {
211+
out[p.String()] = v // valid single port, kept normalized
212+
continue
213+
}
214+
if _, err := network.ParsePortRange(k); err != nil {
215+
log.Debugf("dropping unparseable docker port key %q: %s", k, err)
216+
changed = true
217+
continue
218+
}
219+
ranges = append(ranges, k)
220+
}
221+
222+
// Expand in sorted order: map iteration is randomised, so without this the
223+
// set of ranges that fits the budget could differ between two inspects of
224+
// the same container, making its reported ports flap.
225+
sort.Strings(ranges)
226+
budget := maxExpandedPorts
227+
for _, k := range ranges {
228+
changed = true
229+
pr, err := network.ParsePortRange(k)
230+
if err != nil {
231+
continue // already validated above
232+
}
233+
width := int(pr.End()) - int(pr.Start()) + 1
234+
if width > budget {
235+
// Debug, not Warn: this runs per container event on an uncached path,
236+
// so a single offending container would otherwise spam warnings.
237+
log.Debugf("dropping docker port range %q: %d ports exceeds the remaining budget of %d",
238+
k, width, budget)
239+
continue
240+
}
241+
budget -= width
242+
v := ports[k]
243+
for p := range pr.All() {
244+
if _, exists := out[p.String()]; !exists {
245+
out[p.String()] = v
246+
}
247+
}
248+
}
249+
if !changed {
250+
return nil, false
251+
}
252+
if b, err := json.Marshal(out); err == nil {
253+
return b, true
254+
}
255+
return nil, false
256+
}

0 commit comments

Comments
 (0)