[APIv2]: container copy rewrite broke PUT /containers/{id}/archive endpoint #8649

riyad · 2020-12-08T15:50:04Z

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

Trying to find regressions by exercising the APIv2 trough docker-py's test suite (see #5386) I came across a regressions in the ArchiveTest.test_copy_directory_to_container test.

It specifically broke with commit 0f496e4 .

Steps to reproduce the issue:

Assuming you have a tar archive for upload:

touch test.txt
tar cf test.tar test.txt

create a container

curl -sS --unix-socket /var/run/user/1000/podman/podman.sock -XPOST 'http://localhost/v1.40/containers/create' -H 'Content-Type: application/json' -d '{"Tty": false, "OpenStdin": false, "StdinOnce": false, "AttachStdin": false, "AttachStdout": true, "AttachStderr": true, "Cmd": ["ls", "-p", "/vol1"], "Image": "alpine:3.10", "Volumes": {"/vol1": {}}, "NetworkDisabled": false}'

upload the created archive to it

curl -sS --unix-socket /var/run/user/1000/podman/podman.sock -XPUT 'http://localhost/v1.40/containers/a8922857d1daf9bbcff5b3703c42e31101ef9d0ed7b956e9f2359acea2449d99/archive?path=/vol1' --data-binary '@test.tar' -H 'Content-Type:'

Describe the results you received:

The archive upload fails with:

200 {"cause":"destination must be a directory or stream when copying from a stream","message":"destination must be a directory or stream when copying from a stream","response":500}

Note: the 200 response code. 😞

Describe the results you expected:

The contents of the archive should be extracted into the directory given in the path parameter (here: "/vol1").

Additional information you deem important (e.g. issue happens only occasionally):

The 200 response code in case of an error is because of

podman/pkg/api/handlers/compat/containers_archive.go

Line 173 in 0cccba8

w.WriteHeader(http.StatusOK)

writing the http.StatusOK header out before the last fallible operation.

The copy error happens in:

podman/pkg/copy/copy.go

Line 120 in 0cccba8

    
           return errors.New("destination must be a directory or stream when copying from a stream")

called by

podman/pkg/copy/copy.go

Line 32 in 0cccba8

if err := enforceCopyRules(source, destination); err != nil {

called by

podman/pkg/api/handlers/compat/containers_archive.go

Line 174 in 0cccba8

if err := copy.Copy(&source, &destination, false); err != nil {

... incidentally exactly after setting the http.StatusOK header.

But the actual cause is that destination is invalid in

podman/pkg/api/handlers/compat/containers_archive.go

Line 159 in 0cccba8

destination, err := copy.CopyItemForContainer(ctr, query.Path, true, false)

Because the resolvedContainerPath (something like: "/home/riyad/podman/storage/overlay/02a2eb10376916256ef680acfbdf9296b48f3b4941a41d08e478618c228d68a9/merged/vol1") does not exist in

podman/pkg/copy/item.go

Line 237 in 0cccba8

statInfo, statError := secureStat(resolvedRoot, resolvedContainerPath)

we have statError != nil (specifically "file does not exist").
This leads us to fall through the conditions in L241 and L254 and enter the last block of code where we make sure the "parent" directory for resolvedContainerPath exists (but not the directory itself?). 🤷

All this worked before 0f496e4.

Output of podman version:

Version:      3.0.0-dev
API Version:  3.0.0
Go Version:   go1.15.6
Git Commit:   9b3a81a002e570b8a49e60c3dd3feb65d742f286-dirty
Built:        Tue Dec  8 16:08:02 2020
OS/Arch:      linux/amd64

The text was updated successfully, but these errors were encountered:

vrothberg · 2020-12-08T16:08:32Z

Thanks for opening the issues and providing all the details, @riyad.

@jwhonce and I were just chatting about which things may need further massaging. We'll spin up a couple of PRs to fix it soon.

vrothberg · 2020-12-08T16:21:56Z

Note that we're currently in process to get podman-remote cp to work. That will enable a whole load of tests. I expected the copy refactoring to have caused some bugs for /archive, so we made sure to not release it. First release will be Podman 3.0 early next year.

vrothberg · 2020-12-09T11:15:11Z

FWIW, the new error may not be a regression but actually pointing to another bug.

200 {"cause":"destination must be a directory or stream when copying from a stream","message":"destination must be a directory or stream when copying from a stream","response":500}

/vol1 seems to be currently ignored. @jwhonce @mheon is the Volumes parameter supposed to create or use a volume on the server? It's currently not processed so /vol1 simply does not exist on the container. Just creating the directory (as before) is a bug.

mheon · 2020-12-09T13:58:53Z

Alright, that's... Confusing. There are already two fields for making volumes in the request, Binds and Mounts, and now we have a third with no direct CLI companion (and thus no docs).

…

On Wed, Dec 9, 2020, 06:15 Valentin Rothberg ***@***.***> wrote: FWIW, the new error may not be a regression but actually pointing to another bug. 200 {"cause":"destination must be a directory or stream when copying from a stream","message":"destination must be a directory or stream when copying from a stream","response":500} /vol1 seems to be currently ignored. @jwhonce <https://github.com/jwhonce> @mheon <https://github.com/mheon> is the Volumes parameter supposed to create or use a volume on the server? It's currently not processed so /vol1 simply does not exist on the container. Just creating the directory (as before) is a bug. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCFHKRMJNVMBC4I2EY3ST5L47ANCNFSM4USE5Z5Q> .

riyad · 2020-12-10T11:29:13Z

Indeed ... just checked against Docker: PUT .../archive checks for but doesn't create the destination.

curl -sS --unix-socket /var/run/docker.sock -XPUT 'http://localhost/v1.40/containers/b4e1f854bc5df921134270e85ea774a895b2f1de490e613000a69fbdb333ea3e/archive?path=/does_not_exist' --data-binary '@test.tar' -H 'Content-Type:'
{"message":"Could not find the file /does_not_exist in container b4e1f854bc5df921134270e85ea774a895b2f1de490e613000a69fbdb333ea3e"}

riyad · 2020-12-10T11:56:37Z

Checking Docker's POST /containers/create behavior with "Volumes"set:

$ curl -sS --unix-socket /var/run/docker.sock -XPOST 'http://localhost/v1.40/containers/create' -H 'Content-Type: application/json' -d '{"Tty": false, "OpenStdin": false, "StdinOnce": false, "AttachStdin": false, "AttachStdout": true, "AttachStderr": true, "Cmd": ["ls", "-p", "/vol1"], "Image": "alpine:3.10", "Volumes": {"/vol1": {}}, "NetworkDisabled": false}'
{"Id":"d4b426a218052a59fd56463d199b9217b9dde3f5d27fae63bb49f688bb18b814","Warnings":[]}

$ docker inspect d4b426a218052a59fd56463d199b9217b9dde3f5d27fae63bb49f688bb18b814
[
    {
        [...]
        "Mounts": [
            {
                "Type": "volume",
                "Name": "1bef89e5b270d68aa62c192122f764d00ca1c99c4c5db8ce4b0d5ee833ed1496",
                "Source": "/var/lib/docker/volumes/1bef89e5b270d68aa62c192122f764d00ca1c99c4c5db8ce4b0d5ee833ed1496/_data",
                "Destination": "/vol1",
                "Driver": "local",
                "Mode": "",
                "RW": true,
                "Propagation": ""
            }
        ],
        "Config": {
            [...]
            "Volumes": {
                "/vol1": {}
            },
            [...]
        },
        [...]
    }
]

$ docker volume ls
DRIVER    VOLUME NAME
local     1bef89e5b270d68aa62c192122f764d00ca1c99c4c5db8ce4b0d5ee833ed1496

full docker-inspect output

Docker's API docs say about the "Volumes" object:

An object mapping mount point paths inside the container to empty objects.

It looks like the "Volumes" crates an anonymous (i.e. randomly-named), empty volume, but goes back and uses "Mounts" to have it mounted.

vrothberg · 2020-12-10T12:53:37Z

@mheon do you have cycles to tackle it?

mheon · 2020-12-10T15:57:08Z

Sure, I think I can get this done.

Tricky part: it looks like Docker adds everything to volumes - even things that are already in Binds and Mounts - so we need to deduplicate before we add the new anonymous volumes.

github-actions · 2021-01-10T00:35:52Z

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

rhatdan · 2021-01-10T13:04:26Z

@mheon did you make any progress on this?

mheon · 2021-01-10T14:37:52Z

Nope, not yet

…

On Sun, Jan 10, 2021 at 08:04 Daniel J Walsh ***@***.***> wrote: @mheon <https://github.com/mheon> did you make any progress on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCHWKWCRD7K2O3RIJ73SZGQWNANCNFSM4USE5Z5Q> .

mheon · 2021-01-19T18:46:11Z

#9025 to fix

Docker has, for unclear reasons, three separate fields in their Create Container struct in which volumes can be placed. Right now we support two of those - Binds and Mounts, which (roughly) correspond to `-v` and `--mount` respectively. Unfortunately, we did not support the third, `Volumes`, which is used for anonymous named volumes created by `-v` (e.g. `-v /test`). It seems that volumes listed here are *not* included in the remaining two from my investigation, so it should be safe to just append them into our handling of the `Binds` (`-v`) field. Fixes containers#8649 Signed-off-by: Matthew Heon <[email protected]>

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 8, 2020

github-actions bot added the stale-issue label Jan 10, 2021

rhatdan removed the stale-issue label Jan 18, 2021

mheon mentioned this issue Jan 19, 2021

Ensure the Volumes field in Compat Create is honored #9025

Merged

openshift-merge-robot closed this as completed in #9025 Jan 27, 2021

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 Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APIv2]: container copy rewrite broke PUT /containers/{id}/archive endpoint #8649

[APIv2]: container copy rewrite broke PUT /containers/{id}/archive endpoint #8649

riyad commented Dec 8, 2020 •

edited

Loading

vrothberg commented Dec 8, 2020

vrothberg commented Dec 8, 2020

vrothberg commented Dec 9, 2020

mheon commented Dec 9, 2020 via email

riyad commented Dec 10, 2020

riyad commented Dec 10, 2020

vrothberg commented Dec 10, 2020

mheon commented Dec 10, 2020

github-actions bot commented Jan 10, 2021

rhatdan commented Jan 10, 2021

mheon commented Jan 10, 2021 via email

mheon commented Jan 19, 2021

[APIv2]: container copy rewrite broke PUT /containers/{id}/archive endpoint #8649

[APIv2]: container copy rewrite broke PUT /containers/{id}/archive endpoint #8649

Comments

riyad commented Dec 8, 2020 • edited Loading

vrothberg commented Dec 8, 2020

vrothberg commented Dec 8, 2020

vrothberg commented Dec 9, 2020

mheon commented Dec 9, 2020 via email

riyad commented Dec 10, 2020

riyad commented Dec 10, 2020

vrothberg commented Dec 10, 2020

mheon commented Dec 10, 2020

github-actions bot commented Jan 10, 2021

rhatdan commented Jan 10, 2021

mheon commented Jan 10, 2021 via email

mheon commented Jan 19, 2021

riyad commented Dec 8, 2020 •

edited

Loading