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.

Sign up for GitHub

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

fix: error reporting for archive endpoint #12431

Merged
merged 1 commit into from
Nov 29, 2021
Merged

fix: error reporting for archive endpoint #12431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/api/handlers/compat/containers_archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,10 @@ func handlePut(w http.ResponseWriter, r *http.Request, decoder *schema.Decoder,
return
}

w.WriteHeader(http.StatusOK)
if err := copyFunc(); err != nil {
logrus.Error(err.Error())
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per spec here, we should return 403 if the container is RO.

https://docs.podman.io/en/latest/_static/api.html#operation/PutContainerArchiveLibpod

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've go 500 from original docker implementation so I used that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, their docs also says it should be a 403. I'm ok with either paths (say same behavior with docker vs say same behavior with the documentation).

https://docs.docker.com/engine/api/v1.41/#operation/ContainerArchive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I am not sure if I can determine precise error here, beside doing string matching on error message. It's not necessarily perm. denied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't see any better options than matching the error messages. Let's keep it 500 then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref: docker's implementation.

https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/daemon/archive.go#L362

Copy link
Contributor Author

@matejvasek matejvasek Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting I believe that I got 500 on moby on Fedora with your usecase, ReadonlyRootfs is that used only for root not mounts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with docker source code, but that seems to be the case.

On my archlinux box, it also reports 500 (docker 20.10.11)

< HTTP/1.1 100 Continue                                                                                                                                        
* We are completely uploaded and fine                                                                                                                          
* Mark bundle as not supporting multiuse                  
< HTTP/1.1 500 Internal Server Error
< Api-Version: 1.41
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/20.10.11 (linux)
< Date: Tue, 30 Nov 2021 03:32:37 GMT
< Content-Length: 99
< 
{"message":"Error processing tar file(exit status 1): lchown /run/act/act: read-only file system"}
* Connection #0 to host localhost left intact

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    --unix-socket /var/run/docker.sock \
    -X PUT \
    -T act.tar \
    "http://localhost/v1.40/containers/act-CI-build/archive?noOverwriteDirNonDir=true&path=%2Fvar%2Frun%2Fact%2F"
*   Trying /var/run/docker.sock:0...
* Connected to localhost (/run/docker.sock) port 80 (#0)
> PUT /v1.40/containers/act-CI-build/archive?noOverwriteDirNonDir=true&path=%2Fvar%2Frun%2Fact%2F HTTP/1.1
> Host: localhost
> User-Agent: curl/7.76.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 20480
> Expect: 100-continue
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 100 Continue
* We are completely uploaded and fine
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Api-Version: 1.41
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/20.10.9 (linux)
< Date: Tue, 30 Nov 2021 03:38:05 GMT
< Content-Length: 107
< 
{"message":"Error processing tar file(exit status 1): unlinkat /run/act/README.md: read-only file system"}
* Connection #0 to host localhost left intact

return
}
w.WriteHeader(http.StatusOK)
}
14 changes: 13 additions & 1 deletion test/python/docker/compat/test_containers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from docker import DockerClient, errors
from docker.models.containers import Container
from docker.models.images import Image
from docker.models.volumes import Volume

from test.python.docker import Podman
from test.python.docker.compat import common, constant
Expand Down Expand Up @@ -207,9 +208,14 @@ def test_filters(self):

def test_copy_to_container(self):
ctr: Optional[Container] = None
vol: Optional[Volume] = None
try:
test_file_content = b"Hello World!"
ctr = self.client.containers.create(image="alpine", detach=True, command="top")
vol = self.client.volumes.create("test-volume")
ctr = self.client.containers.create(image="alpine",
detach=True,
command="top",
volumes=["test-volume:/test-volume-read-only:ro"])
ctr.start()

buff: IO[bytes] = io.BytesIO()
Expand All @@ -234,10 +240,16 @@ def test_copy_to_container(self):
ret, out = ctr.exec_run(["cat", "/tmp/a.txt"])
self.assertEqual(ret, 0)
self.assertEqual(out.rstrip(), test_file_content, "Content of copied file")

buff.seek(0)
with self.assertRaises(errors.APIError):
ctr.put_archive("/test-volume-read-only/", buff)
finally:
if ctr is not None:
ctr.stop()
ctr.remove()
if vol is not None:
vol.remove(force=True)

def test_mount_preexisting_dir(self):
dockerfile = (B'FROM quay.io/libpod/alpine:latest\n'
Expand Down