Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dockerd neglects to populate OCI Descriptor size field #46641

Open
waynr opened this issue Oct 13, 2023 · 11 comments
Open

dockerd neglects to populate OCI Descriptor size field #46641

waynr opened this issue Oct 13, 2023 · 11 comments
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/0-triage

Comments

@waynr
Copy link

waynr commented Oct 13, 2023

As background, I originally reported this issue in the buildkit repo: moby/buildkit#4328

Since that issue was closed yesterday I felt a bit crazy because on the one hand I can clearly see in my WIP registry implementation that manifests pushed to it by dockerd definitely do not contain the size field in the descriptors as required by the OCI image spec, yet on the other hand the issue doesn't seem reproducible pushing images to a local copy of distribution.

@neersighted mentioned I should open a PR here if I can prove that the size field is missing from manifests on the wire during a PUT /v2/<name>/manifests/<reference> call so that's what I'm doing.

Here are the details of my setup:

  • my WIP registry (named portfolio) running on localhost:13030
  • a local build of distribution running on localhost:5000 with the following config:
version: 0.1
log:
  fields:
    service: registry
storage:
  cache:
    blobdescriptor: inmemory
  filesystem:
    rootdirectory: ./var/lib/registry
http:
  addr: :5001
  headers:
    X-Content-Type-Options: [nosniff]
health:
  storagedriver:
    enabled: true
    interval: 10s
    threshold: 3
  • mitmdump in reverse proxy mode listening at localhost:8080 pointed at localhost:13030 (portfolio):
mitmdump --mode reverse:http://127.0.0.1:13030@8080 --flow-detail 4 > portfolio.mitmproxy.flow
  • mitmdump in reverse proxy mode listening at localhost:8081 pointed at localhost:5000 (distribution)
mitmdump --mode reverse:http://127.0.0.1:5000@8081 --flow-detail 4 > distribution.mitmproxy.flow

With this setup I run the following docker build commands:

# aiming at the distribution mitmdump instance
docker buildx build --push ./ -t 127.0.0.1:8081/woof:meowmeow --no-cache
# aiming at the portfolio mitmdump instance
docker buildx build --push ./ -t 127.0.0.1:8080/woof:meowmeow --no-cache

(I will upload a tarball with the resulting logs after submitting this issue since I am not using a web browser to write this description)

The relevant snippet of mitmdump output for portfolio (note the missing size filed in the config descriptor):

[08:27:34.419][127.0.0.1:39808] client connect
[08:27:34.421][127.0.0.1:39808] server connect 127.0.0.1:13030
127.0.0.1:39808: PUT http://127.0.0.1:13030/v2/woof/manifests/meowmeow
    Host: 127.0.0.1:13030
    User-Agent: docker/24.0.6 go/go1.21.1 git-commit/1a7969545d kernel/6.1.54-1-lts os/linux arch/amd64 UpstreamClient(Go-http-client/1.1)
    Content-Length: 714
    Content-Type: application/vnd.docker.distribution.manifest.v2+json
    Accept-Encoding: gzip
    Connection: close

    {
        "config": {
            "digest": "sha256:7bddfd52170f7ebde74e065ec731299f182d5fa332bbfbe8cd07331031b75ab9",
            "mediaType": "application/vnd.docker.container.image.v1+json"
        },
        "layers": [
            {
                "digest": "sha256:96526aa774ef0126ad0fe9e9a95764c5fc37f409ab9e97021e7b4775d82bf6fa",
                "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
                "size": 3401967
            },
            {
                "digest": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1",
                "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
                "size": 32
            }
        ],
        "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
        "schemaVersion": 2
    }

 << 201 Created 0b
    content-type: text/plain; charset=utf-8
    location: /v2/woof/manifests/meowmeow
    docker-content-digest: sha256:5d3183be0559955ce465d519e0d6a6803249e20d1d7291febcb9b9ac3e5f1ccf
    docker-distribution-api-version: registry/2.0
    content-length: 0
    date: Fri, 13 Oct 2023 14:27:34 GMT

[08:27:34.538][127.0.0.1:39808] server disconnect 127.0.0.1:13030
[08:27:34.539][127.0.0.1:39808] client disconnect

The relevant snippet of mitmdump output for distribution (note the presence of the config.size field here, in contrast with the above snippet):

[08:49:46.135][127.0.0.1:37552] client connect
[08:49:46.136][127.0.0.1:37552] server connect 127.0.0.1:5000
127.0.0.1:37552: PUT http://127.0.0.1:5000/v2/woof/manifests/meowmeow
    Host: 127.0.0.1:5000
    User-Agent: docker/24.0.6 go/go1.21.1 git-commit/1a7969545d kernel/6.1.54-1-lts os/linux arch/amd64 UpstreamClient(Go-http-client/1.1)
    Content-Length: 733
    Content-Type: application/vnd.docker.distribution.manifest.v2+json
    Accept-Encoding: gzip
    Connection: close

    {
        "config": {
            "digest": "sha256:e94c3024986a16ece2ea2d57034d97c91bf88662316c882f3ffdf8527010b6db",
            "mediaType": "application/vnd.docker.container.image.v1+json",
            "size": 816
        },
        "layers": [
            {
                "digest": "sha256:96526aa774ef0126ad0fe9e9a95764c5fc37f409ab9e97021e7b4775d82bf6fa",
                "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
                "size": 3401967
            },
            {
                "digest": "sha256:4f4fb700ef54461cfa02571ae0db9a0dc1e0cdb5577484a6d75e68dc38e8acc1",
                "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
                "size": 32
            }
        ],
        "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
        "schemaVersion": 2
    }

 << 201 Created 0b
    Docker-Content-Digest: sha256:c68777b0ce58b075d8113abb5c9285599d6a203cbd9b9a75babfd486a07e7083
    Docker-Distribution-Api-Version: registry/2.0
    Location: http://127.0.0.1:5000/v2/woof/manifests/sha256:c68777b0ce58b075d8113abb5c9285599d6a203cbd9b9a75babfd486a07e7083
    X-Content-Type-Options: nosniff
    Date: Fri, 13 Oct 2023 14:49:46 GMT
    Content-Length: 0
    Connection: close

[08:49:46.151][127.0.0.1:37552] server disconnect 127.0.0.1:5000
[08:49:46.151][127.0.0.1:37552] client disconnect

I believe that if I build the image without cached layers and push it first to my WIP registry I can get a manifest where the layer descriptors also don't include the size field, but the above logs should be enough to establish the fact that this is happening (and i'm not crazy 😄)

For what it's worth, I would like to share my WIP registry so others can more easily reproduce this but the dev env setup is somewhat complicated by the need for a postgresql wire compatible DB and a live S3 compatible API. I'll eventually implement support for sqlite metadata storage and local file system bulk data storage but that could be a while.

In the short term I intend to work on a cloud deployment of this that I will be happy to share with maintainers who want to see the issue for themselves. (probably won't be ready until next week)

I've also noticed some differences between distribution and my implementation in terms of the /v2/ response (eg distribution includes an empty json in the response body, mine doesn't) and various headers. I suspect that if I align the behavior of my registry more closely to distribution this problem might go away.

But I'm reporting this bug anyway because it does definitely seem like a bug somewhere in dockerd or one of the registry client libraries it might be using to exclude the descriptor size field under any circumstance.

@waynr
Copy link
Author

waynr commented Oct 13, 2023

Here are the mitmdump logs:
mitmdump-flows.tar.gz

@waynr
Copy link
Author

waynr commented Oct 13, 2023

oh shoot, since i didn't file this issue in the web ui i neglected to include some important info:

docker info

Client:
 Version:    24.0.6
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc.)
    Version:  0.11.2
    Path:     /usr/lib/docker/cli-plugins/docker-buildx
  compose: Docker Compose (Docker Inc.)
    Version:  v2.2.3
    Path:     /home/wayne/.docker/cli-plugins/docker-compose

Server:
 Containers: 9
  Running: 9
  Paused: 0
  Stopped: 0
 Images: 319
 Server Version: 24.0.6
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Using metacopy: true
  Native Overlay Diff: false
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: runc io.containerd.runc.v2
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 091922f03c2762540fd057fba91260237ff86acb.m
 runc version: 
 init version: de40ad0
 Security Options:
  seccomp
   Profile: builtin
  cgroupns
 Kernel Version: 6.1.54-1-lts
 Operating System: Arch Linux
 OSType: linux
 Architecture: x86_64
 CPUs: 16
 Total Memory: 46.29GiB
 Name: thing3
 ID: RXZW:VO3X:A36S:435C:5JOY:W4TO:S3CH:MTEE:QLQN:NJB3:CKEE:UG2W
 Docker Root Dir: /var/lib/docker
 Debug Mode: true
  File Descriptors: 96
  Goroutines: 118
  System Time: 2023-10-13T09:28:43.946004432-06:00
  EventsListeners: 0
 HTTP Proxy: http://127.0.0.1:8080
 Username: waynr
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false

docker version

Client:
 Version:           24.0.6
 API version:       1.43
 Go version:        go1.21.1
 Git commit:        ed223bc820
 Built:             Sat Sep 30 15:48:58 2023
 OS/Arch:           linux/amd64
 Context:           default

Server:
 Engine:
  Version:          24.0.6
  API version:      1.43 (minimum version 1.12)
  Go version:       go1.21.1
  Git commit:       1a7969545d
  Built:            Sat Sep 30 15:48:58 2023
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.7.6
  GitCommit:        091922f03c2762540fd057fba91260237ff86acb.m
 runc:
  Version:          1.1.9
  GitCommit:        
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

@neersighted
Copy link
Member

The mechanisms that generate the descriptors are different for config vs. layer blobs; see my dive here: moby/buildkit#4328 (comment)

In the case of the config descriptor, the descriptor is generated in distribution/distribution, in the client distribution.BlobStore implementation: https://github.com/distribution/distribution/blob/ebba01efeacc31a55640ee1efc858ea4e327c479/registry/client/repository.go#L714-L733

@waynr
Copy link
Author

waynr commented Oct 13, 2023

Here's a new set of captures, but this time I made sure to clear out the local storage in each registry (I think my previous mitmdumps excluded actual upload of blobs for distribution because it had cached data from previous pushes):
captures.tar.gz

This also includes tcpdump captures as per @neersighted's request in slack DM. The tcpdump captures were grabbed using the following command:

# for distribution
tcpdump -i lo -XX -s 0 'tcp port 5001 and (((ip[2:2] - ((ip[0]&0xf)<<2)) - ((tcp[12]&0xf0)>>2)) != 0)' | tee distribution.pcap

# for portfolio
tcpdump -i lo -XX -s 0 'tcp port 13030 and (((ip[2:2] - ((ip[0]&0xf)<<2)) - ((tcp[12]&0xf0)>>2)) != 0)' | tee portfolio.pcap

@neersighted
Copy link
Member

@waynr the PCAP files appear corrupt (at least according to wireshark), and the new mitmproxy flow still shows layers already present in the distribution/distribution registry.

@waynr
Copy link
Author

waynr commented Oct 13, 2023

@neersighted here's another attempt, but this time using tcpdump -i lo -s 0 -w <filename> 'tcp port <port>'. I still don't see the blob upload happening in the mitmdump output but I did clear out the filesystem storage driver directory and restarted the distribution server binary before collecting it.
mitmdump-flows_2.tar.gz

I did open the tcpdump captures in wireshark this time to verify that works. The tcpdump capture looks more reliable than the mitmproxy capture so I'll probably leave the latter out of future captures.

@waynr
Copy link
Author

waynr commented Oct 13, 2023

Okay one more try...one of my previous tcpdump captures was bad, this time I've checked in wireshark that both the distribution and the portfolio captures include all the expected http exchanges.
pcap-1.tar.gz

@waynr
Copy link
Author

waynr commented Oct 13, 2023

One other major difference I am noticing when dockerd pushes images to distribution vs portfolio is that for distribution the uploads look to be monolithic POST uploads whereas for portfolio they look to be chunked POST-PATCH-PUT uploads. This might be easier to use as a hint for where to start looking for what causes the difference in manifest. ie look for where the image push includes one or more PATCH requests.

Actually, forget this -- I was using mitmproxy again and compared with tcpdump it doesn't seem to show all the requests strangely. In the tcpdump capture I do see HTTP PATCH requests for distribution. 🤦

@waynr
Copy link
Author

waynr commented Oct 13, 2023

Okay, I figured out what I need to do in my registry to get the config.size field from dockerd!

The problem seems to be that I was using the empty string ("") to populate my response bodies which was causing the http framework I use to automatically add the content-type: text/plain; charset=utf-8 header. When I replace "" as the response body with Json("") this causes the framework to use content-type: application/json.

I'm guessing that returning the wrong content type in the HEAD /v2/<repository>/blobs/<digest> request that immediately preceeds the PUT /v2/<repository>/manifests request causes dockerd to follow a bad code path when generating the config descriptor.

While I acknowledge that the behavior of my code was wrong (I mostly raised this issue to understand what i was doing wrong), I still think it's wrong for dockerd to have a descriptor-generating code path that doesn't include a size field so it's probably a good idea to keep this issue open.

@waynr
Copy link
Author

waynr commented Oct 13, 2023

One more quick update!

So in my previous comment I mentioned replacing plain text body in all my registry's API endpoint responses with Json(""). This led to the body size being calculated as 2 bytes for all requests that didn't have an explicit larger response body set.

I initially assumed that just having content-type: application/json in the HEAD /v2/<repository>/blobs/<digest> responses is what led to the manifest pushed by PUT /v2/<repository>/manifests having a size field in its config descriptor. Then I realized something strange - the config descriptor's size field was being set to 2! I happened to know the config was actually 816 bytes since that's what the manifest being pushed to distribution showed; also, 2 bytes is a totally unrealistic size for a manifest config.

I fixed the route handler in my app for HEAD /v2/<repository>/blobs/<digest> so that it now actually returns the size in bytes of the blob represented by <digest> in the content-length response header and the manifest received from dockerd is also now correct! (in the example docker image i was working with, 816 is the correct size of the image config).

This suggests that in dockerd's image pushing code it sets the config descriptor's size field from the HEAD /v2/<repository>/blobs/<digest> response's content-length header. If there is no content-length header in that response, it seems to get set to 0. I believe that because of the omitempty json tag if the size is set to 0 it is considered "empty" and therefore excluded entirely from the resulting json representation.

Is any of this a bug in dockerd? I don't know. I would think that it would try to calculate the size of the image config directly rather than rely on the registry response header as described above. The OCI Distribution spec doesn't even say anything about HEAD /v2/<repository>/blobs/<digest> responses including a content-length header. (I plan to open a PR to update the spec)

@neersighted
Copy link
Member

It appears docker/for-linux#1296 was a duplicate/the first report of this, but the reporter chose not to open an issue once they realized that their registry was returning the 'wrong' size.

@sam-thibault sam-thibault added status/0-triage kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/0-triage
Projects
None yet
Development

No branches or pull requests

3 participants