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

Build image pull field type mismatch with Docker #17778

Closed
cristianrgreco opened this issue Mar 14, 2023 · 6 comments · Fixed by #18583
Closed

Build image pull field type mismatch with Docker #17778

cristianrgreco opened this issue Mar 14, 2023 · 6 comments · Fixed by #18583
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. HTTP API Bug is in RESTful API kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@cristianrgreco
Copy link

cristianrgreco commented Mar 14, 2023

Issue Description

Docker accepts any string value for the pull field to indicate the image should be pulled:https://docs.docker.com/engine/api/v1.42/#tag/Image/operation/ImageBuild

Podman expects the value to equal either boolean true or string true:
https://docs.podman.io/en/latest/_static/api.html?version=v4.4#tag/images/operation/ImageBuildLibpod

Applications that align with Docker's spec are unable to switch to Podman as building an image with this field fails.

Steps to reproduce the issue

  1. POST to the create image endpoint with pull set to a string, such as always.

Describe the results you received

No response stream received

Describe the results you expected

Expected a response stream

podman info output

host:
  arch: amd64
  buildahVersion: 1.29.0
  cgroupControllers:
  - memory
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon_2:2.1.7-0debian9999+obs15.6_amd64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.7, commit: '
  cpuUtilization:
    idlePercent: 87.46
    systemPercent: 5.69
    userPercent: 6.84
  cpus: 2
  distribution:
    codename: jammy
    distribution: ubuntu
    version: "22.04"
  eventLogger: journald
  hostname: fv-az646-90
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 123
      size: 1
    - container_id: 1
      host_id: 165536
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1001
      size: 1
    - container_id: 1
      host_id: 165536
      size: 65536
  kernel: 5.15.0-1034-azure
  linkmode: dynamic
  logDriver: journald
  memFree: 4857856000
  memTotal: 7281278976
  networkBackend: netavark
  ociRuntime:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/runner/.local/share/containers/storage
  graphRootAllocated: 89297309696
  graphRootUsed: 58336636928
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 0
  runRoot: /run/user/1001/containers
  transientStore: false
  volumePath: /home/runner/.local/share/containers/storage/volumes
version:
  APIVersion: 4.4.2
  Built: 0
  BuiltTime: Thu Jan  1 00:00:00 1970
  GitCommit: ""
  GoVersion: go1.19.6
  Os: linux
  OsArch: linux/amd64
  Version: 4.4.2

Podman in a container

Yes

Privileged Or Rootless

Rootless

Upstream Latest Release

Yes

Additional environment details

DOCKER_HOST=unix:///run/user/$(id -u)/podman/podman.sock

Additional information

No response

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Apr 14, 2023

Do you have an example with curl to show us failing, where docker succeeds?

@rhatdan rhatdan added HTTP API Bug is in RESTful API Good First Issue This issue would be a good issue for a first time contributor to undertake. and removed stale-issue labels Apr 14, 2023
@eriksjolund
Copy link
Contributor

eriksjolund commented May 6, 2023

Reproduced on Fedora CoreOS (fedora-coreos-38.20230430.1.0-qemu.aarch64)

  1. boot up a new Fedora CoreOS VM
  2. sudo useradd test
  3. sudo usermod -aG docker test
  4. sudo systemctl start docker.service
  5. Open an interactive shell session for user test
    sudo machinectl shell test@`
    
  6. mkdir dir && cd dir
  7. Create a file
    cat << EOF > Dockerfile
    FROM docker.io/library/alpine
    EOF
    
  8. systemctl --user start podman.socket
  9. Run command
    tar cf - . | curl -XPOST \
      --unix-socket /var/run/docker.sock \
      --data-binary @- \
      -H 'Content-Type: application/x-tar' \
      http://localhost/v1.41/build?pull=always
    
  10. Run command
    tar cf - . | curl -XPOST \
      --unix-socket $XDG_RUNTIME_DIR/podman/podman.sock \
      --data-binary @- \
      -H 'Content-Type: application/x-tar' \
      http://localhost/v1.41/build?pull=always
    
  11. Run command
    tar cf - . | curl -XPOST \
      --unix-socket $XDG_RUNTIME_DIR/podman/podman.sock \
      --data-binary @- \
      -H 'Content-Type: application/x-tar' \
      http://localhost/v1.41/build?pull=true
    

Output from step 9

{"stream":"Step 1/1 : FROM docker.io/library/alpine"}
{"stream":"\n"}
{"status":"Pulling from library/alpine","id":"latest"}
{"status":"Pulling fs layer","progressDetail":{},"id":"c41833b44d91"}
{"status":"Downloading","progressDetail":{"current":32768,"total":3261854},"progress":"[\u003e                                                  ]  32.77kB/3.262MB","id":"c41833b44d91"}
{"status":"Verifying Checksum","progressDetail":{},"id":"c41833b44d91"}
{"status":"Download complete","progressDetail":{},"id":"c41833b44d91"}
{"status":"Extracting","progressDetail":{"current":32768,"total":3261854},"progress":"[\u003e                                                  ]  32.77kB/3.262MB","id":"c41833b44d91"}
{"status":"Extracting","progressDetail":{"current":294912,"total":3261854},"progress":"[====\u003e                                              ]  294.9kB/3.262MB","id":"c41833b44d91"}
{"status":"Extracting","progressDetail":{"current":884736,"total":3261854},"progress":"[=============\u003e                                     ]  884.7kB/3.262MB","id":"c41833b44d91"}
{"status":"Extracting","progressDetail":{"current":2949120,"total":3261854},"progress":"[=============================================\u003e     ]  2.949MB/3.262MB","id":"c41833b44d91"}
{"status":"Extracting","progressDetail":{"current":3261854,"total":3261854},"progress":"[==================================================\u003e]  3.262MB/3.262MB","id":"c41833b44d91"}
{"status":"Pull complete","progressDetail":{},"id":"c41833b44d91"}
{"status":"Digest: sha256:124c7d2707904eea7431fffe91522a01e5a861a624ee31d03372cc1d138a3126"}
{"status":"Status: Downloaded newer image for alpine:latest"}
{"stream":" ---\u003e 51e60588ff2c\n"}
{"aux":{"ID":"sha256:51e60588ff2cd9f45792b23de89bfface0a7fbd711d17c5f5ce900a4f6b16260"}}
{"stream":"Successfully built 51e60588ff2c\n"}

There was no output from step 10

Output from step 11

{"stream":"STEP 1/1: FROM docker.io/library/alpine\n"}
{"stream":"Trying to pull docker.io/library/alpine:latest...\n"}
{"stream":"Getting image source signatures\n"}
{"stream":"Copying blob sha256:c41833b44d910632b415cd89a9cdaa4d62c9725dc56c99a7ddadafd6719960f9\n"}
{"stream":"Copying config sha256:51e60588ff2cd9f45792b23de89bfface0a7fbd711d17c5f5ce900a4f6b16260\n"}
{"stream":"Writing manifest to image destination\n"}
{"stream":"Storing signatures\n"}
{"stream":"COMMIT\n"}
{"stream":"--\u003e 51e60588ff2c\n"}
{"stream":"51e60588ff2cd9f45792b23de89bfface0a7fbd711d17c5f5ce900a4f6b16260\n"}
{"aux":{"ID":"sha256:51e60588ff2cd9f45792b23de89bfface0a7fbd711d17c5f5ce900a4f6b16260"}}
{"stream":"Successfully built 51e60588ff2c\n"}

About the system:

[test@localhost ~]$ rpm -q podman
podman-4.5.0-1.fc38.aarch64
[test@localhost ~]$ rpm -q moby-engine
moby-engine-20.10.23-1.fc38.aarch64
[test@localhost ~]$ 

flouthoc added a commit to flouthoc/podman that referenced this issue May 16, 2023
`pull` parameter in `build` must accept string just like docker.

Ref: https://docs.docker.com/engine/api/v1.42/#tag/Image/operation/ImageBuild

Closes: containers#17778

Signed-off-by: Aditya R <[email protected]>
@matejvasek
Copy link
Contributor

matejvasek commented Aug 10, 2023

@eriksjolund @cristianrgreco @flouthoc

This is really strange. The docker client sets this value to "1" (for true), and server handles it like a boolean too.

client:
https://github.com/moby/moby/blob/e0da5cb9291982a4c7b3c6b0642eeced4ba9f8ac/client/image_build.go#L71-L73

server:
https://github.com/moby/moby/blob/f71fe8476a2f8493adf50b6d520a739568383a51/api/server/router/build/build_routes.go#L77-L79
https://github.com/moby/moby/blob/master/api/server/httputils/form.go#L14-L16

This means the always string is not interpreted as an pull-policy enum value but rather as boolean true (but that is rather accident).
Actually it appears server consider everything but "", "0", "no", "false", "none" to be true (+- case).

The PR #18583 actually breaks compatibility instead of fixing it.

# Broken compatibility with the docker CLI
docker build . -t alpine-1000 --pull
Sending build context to Docker daemon  2.048kB
Error response from daemon: failed to parse query parameter 'pull': "1": invalid pull policy: "1"

I would suggest to revert it back to boolean. The older code is not 100% compliant but much better that what we have now. Then it could be improved upon by register custom boolean deserializer for compat endpoints.

	dec.RegisterConverter(true, func(s string) reflect.Value {
		s = strings.ToLower(strings.TrimSpace(s))
		return reflect.ValueOf(!(s == "" || s == "0" || s == "no" || s == "false" || s == "none"))
	})

You may argue that doc says it's a string but I trust actual code much more.
Also it does not specify format of the string.

@matejvasek
Copy link
Contributor

matejvasek commented Aug 10, 2023

IMO the Docker API documentation should be fixed.

@matejvasek
Copy link
Contributor

matejvasek commented Aug 11, 2023

Looking at the source code of Docker it seems there is not such thing as pull policy. It simply interprets Always == true but note that also Never == true.

matejvasek added a commit to matejvasek/podman that referenced this issue Aug 14, 2023
This reverts commit 5b148a0.

Reverting to treating the `pull` query parameter as a boolean.
Because of deceiving Docker API documentation it was assumed that the
parameter is pull-policy, however that is not true. Docker does treat
`pull` as a boolean. What is interesting is that Docker indeed accepts
strings like `always` or `never` however Docekr both of these strings
treat as `true`, not as pull-policy. As matter of the fact it seems
there is no such a thing as pull-policy in Docker.

More context containers#17778 (comment)

Signed-off-by: Matej Vasek <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. HTTP API Bug is in RESTful API kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants