Fix HTTP status-code when loading invalid tar by thaJeztah · Pull Request #34425 · moby/moby

thaJeztah · 2017-08-07T15:18:20Z

Opening for discussion, because this changes the WriteFlusher interface, which is used in various places; also the error-code may need to be more specific based on the actual error.

Before this change:

echo "hello world, I am not a tar" | curl -v --unix-socket  /var/run/docker.sock -H "Content-Type: application/x-tar"  --data-binary @- -X POST http://localhost/v1.29/images/load
*   Trying /var/run/docker.sock...
* Connected to localhost (/Users/sebastiaan/Library/Containers/com.dock) port 80 (#0)
> POST /v1.29/images/load HTTP/1.1
> Host: localhost
> User-Agent: curl/7.51.0
> Accept: */*
> Content-Type: application/x-tar
> Content-Length: 28
>
* upload completely sent off: 28 out of 28 bytes
< HTTP/1.1 200 OK
< Api-Version: 1.31
< Content-Type: application/json
< Date: Mon, 07 Aug 2017 15:11:35 GMT
< Docker-Experimental: true
< Ostype: linux
< Server: Docker/17.07.0-ce-rc1 (linux)
< Transfer-Encoding: chunked
<
{"errorDetail":{"message":"Error processing tar file(exit status 1): unexpected EOF"},"error":"Error processing tar file(exit status 1): unexpected EOF"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

After this change:

/ # echo "hello world, I am not a tar" | curl -v --unix-socket  /var/run/docker.sock -H "Content-Type: application/x-tar"  --data-binary @- -X POST http://localhost/v1.29/images/load
   *   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> POST /v1.29/images/load HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/x-tar
> Content-Length: 28
> 
* upload completely sent off: 28 out of 28 bytes
< HTTP/1.1 400 Bad Request
< Api-Version: 1.36
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Fri, 16 Feb 2018 16:21:46 GMT
< Transfer-Encoding: chunked
< 
Api-Version: 1.36
Content-Type: application/json
Docker-Experimental: false
Ostype: linux
Server: Docker/dev (linux)
{"errorDetail":{"message":"Error processing tar file(exit status 1): unexpected EOF"},"error":"Error processing tar file(exit status 1): unexpected EOF"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

thaJeztah · 2017-08-07T15:19:12Z

ping @cpuguy83 could you have a look if this direction is ok or not?

cpuguy83

Importing HTTP in the backend is not what we want.
In the attach code we pass a function to get the write streams rather than passing the actual writers, which allows the backend to return early due to an error without having sent a 200 response header already.

api/server/router/image/image_routes.go

cpuguy83 · 2017-08-07T15:22:12Z

api/types/backend/backend.go

This is not right.

I ended up changing it due to

moby/daemon/stats.go

Lines 41 to 47 in 22472c8

outStream := config.OutStream

if config.Stream {

wf := ioutils.NewWriteFlusher(outStream)

defer wf.Close()

wf.Flush()

outStream = wf

}

(also a reason I opened this for discussion)

thaJeztah · 2017-09-18T17:14:25Z

ping @cpuguy83 @tonistiigi PTAL

cpuguy83 · 2018-01-17T02:19:55Z

api/server/router/image/image_routes.go

Does this work? Would LoadImage have already written to the stream and thus already have sent a 200 OK?
We could check output.Flushed(), although I would pretty much expect LoadImage to handle all things on the stream by this point.

OK, can we gate this with a check for !output.Flushed()?

Yes, I can add that

thaJeztah · 2018-01-19T16:46:10Z

Does this work? Would LoadImage have already written to the stream and thus already have sent a 200 OK?

Testing other situations: valid tar, but wrong format (export <--> load)

docker run -dit --name foo busybox
docker export foo > exported.tar

With this PR:

$ cat exported.tar | curl -v --unix-socket  /var/run/docker.sock -H "Content-Type: application/x-tar"  --data-binary @- -X POST http://localhost/v1.29/images/load

*   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> POST /v1.29/images/load HTTP/1.1
> Host: localhost
> User-Agent: curl/7.52.1
> Accept: */*
> Content-Type: application/x-tar
> Content-Length: 1359872
> Expect: 100-continue
> 
< HTTP/1.1 100 Continue
* We are completely uploaded and fine
< HTTP/1.1 400 Bad Request
< Api-Version: 1.36
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/dev (linux)
< Date: Fri, 19 Jan 2018 16:25:33 GMT
< Transfer-Encoding: chunked
< 
Api-Version: 1.36
Content-Type: application/json
Docker-Experimental: false
Ostype: linux
Server: Docker/dev (linux)
{"errorDetail":{"message":"open /var/lib/docker/tmp/docker-import-641132035/bin/json: no such file or directory"},"error":"open /var/lib/docker/tmp/docker-import-641132035/bin/json: no such file or directory"}
* Curl_http_done: called premature == 0
* Connection #0 to host localhost left intact

Doing the same on Docker 18.01:

$ cat exported.tar | curl -v --unix-socket  /var/run/docker.sock -H "Content-Type: application/x-tar"  --data-binary @- -X POST http://localhost/v1.29/images/load

*   Trying /var/run/docker.sock...
* Connected to localhost (/var/run/docker.sock) port 80 (#0)
> POST /v1.29/images/load HTTP/1.1
> Host: localhost
> User-Agent: curl/7.57.0
> Accept: */*
> Content-Type: application/x-tar
> Content-Length: 1359872
> Expect: 100-continue
> 
< HTTP/1.1 100 Continue
* We are completely uploaded and fine
< HTTP/1.1 200 OK
< Api-Version: 1.35
< Content-Type: application/json
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/18.01.0-ce (linux)
< Date: Fri, 19 Jan 2018 16:43:37 GMT
< Transfer-Encoding: chunked
< 
{"errorDetail":{"message":"open /var/lib/docker/tmp/docker-import-640654783/bin/json: no such file or directory"},"error":"open /var/lib/docker/tmp/docker-import-640654783/bin/json: no such file or directory"}
* Connection #0 to host localhost left intact

cpuguy83 · 2018-02-16T15:16:31Z

image/tarexport/load.go

errdefs.InvalidParameter(err)

thaJeztah · 2018-02-16T16:23:24Z

ping @cpuguy83 updated 👍

thaJeztah · 2018-02-16T17:03:47Z

For some reason, the CLI doesn't print the error message with this change; only difference between before/after is the status code:

diff --git a/before b/after
index 0493ca6..149e900 100644
--- a/before
+++ b/after
@@ -1,13 +1,14 @@
 * upload completely sent off: 28 out of 28 bytes
-< HTTP/1.1 200 OK
+< HTTP/1.1 400 Bad Request
 < Api-Version: 1.36
 < Content-Type: application/json
-< Date: Fri, 16 Feb 2018 16:51:16 GMT
-< Docker-Experimental: true
+< Docker-Experimental: false
 < Ostype: linux
-< Server: Docker/18.02.0-ce (linux)
+< Server: Docker/dev (linux)
+< Date: Fri, 16 Feb 2018 16:50:38 GMT
 < Transfer-Encoding: chunked
 < 
 {"errorDetail":{"message":"Error processing tar file(exit status 1): unexpected EOF"},"error":"Error processing tar file(exit status 1): unexpected EOF"}
+* Curl_http_done: called premature == 0
 * Connection #0 to host localhost left intact

But with this change, the actual error is not printed:

echo "fooasdsddsdas" | docker load
Error response from daemon:

Wondering if it's an issue in the CLI and it stops early on a 400 status?

AkihiroSuda · 2018-09-18T02:16:58Z

Wondering if it's an issue in the CLI and it stops early on a 400 status?

Is it safer to keep the current behavior for ensuring compatibility?

olljanat · 2018-12-22T16:54:43Z

@thaJeztah CI looks failing to:

18:40:58 These files import internal code: (either directly or indirectly)
18:40:58  - pkg/chrootarchive/archive_unix.go imports github.com/docker/docker/errdefs
18:40:58 
18:40:59 Build step 'Execute shell' marked build as failure

Before this change: echo "hello world, I am not a tar" | curl -v --unix-socket /var/run/docker.sock -H "Content-Type: application/x-tar" --data-binary @- -X POST http://localhost/v1.29/images/load * Trying /var/run/docker.sock... * Connected to localhost (/Users/sebastiaan/Library/Containers/com.dock) port 80 (#0) > POST /v1.29/images/load HTTP/1.1 > Host: localhost > User-Agent: curl/7.51.0 > Accept: */* > Content-Type: application/x-tar > Content-Length: 28 > * upload completely sent off: 28 out of 28 bytes < HTTP/1.1 200 OK < Api-Version: 1.31 < Content-Type: application/json < Date: Mon, 07 Aug 2017 15:11:35 GMT < Docker-Experimental: true < Ostype: linux < Server: Docker/17.07.0-ce-rc1 (linux) < Transfer-Encoding: chunked < {"errorDetail":{"message":"Error processing tar file(exit status 1): unexpected EOF"},"error":"Error processing tar file(exit status 1): unexpected EOF"} * Curl_http_done: called premature == 0 * Connection #0 to host localhost left intact After this change: echo "hello world, I am not a tar" | curl -v --unix-socket /var/run/docker.sock -H "Content-Type: application/x-tar" --data-binary @- http://localhost/v1.29/images/load * Trying /var/run/docker.sock... * Connected to localhost (/var/run/docker.sock) port 80 (#0) > POST /v1.29/images/load HTTP/1.1 > Host: localhost > User-Agent: curl/7.55.0 > Accept: */* > Content-Type: application/x-tar > Content-Length: 28 > * upload completely sent off: 28 out of 28 bytes < HTTP/1.1 400 Bad Request < Api-Version: 1.32 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/17.06.0-dev (linux) < Date: Thu, 17 Aug 2017 11:14:55 GMT < Transfer-Encoding: chunked < Api-Version: 1.32 Content-Type: application/json Docker-Experimental: false Ostype: linux Server: Docker/17.06.0-dev (linux) {"errorDetail":{"message":"Error processing tar file(exit status 1): unexpected EOF"},"error":"Error processing tar file(exit status 1): unexpected EOF"} Attempting to import an "export" image using "docker load": docker export somecontainer > foo.tar cat foo.tar | curl -v --unix-socket /var/run/docker.sock -H "Content-Type: application/x-tar" --data-binary @- http://localhost/v1.29/images/load * Trying /var/run/docker.sock... * Connected to localhost (/var/run/docker.sock) port 80 (#0) > POST /v1.29/images/load HTTP/1.1 > Host: localhost > User-Agent: curl/7.55.0 > Accept: */* > Content-Type: application/x-tar > Content-Length: 5910528 > Expect: 100-continue > < HTTP/1.1 100 Continue * We are completely uploaded and fine < HTTP/1.1 400 Bad Request < Api-Version: 1.32 < Content-Type: application/json < Docker-Experimental: false < Ostype: linux < Server: Docker/17.06.0-dev (linux) < Date: Thu, 17 Aug 2017 12:34:02 GMT < Transfer-Encoding: chunked < Api-Version: 1.32 Content-Type: application/json Docker-Experimental: false Ostype: linux Server: Docker/17.06.0-dev (linux) {"errorDetail":{"message":"open /var/lib/docker/tmp/docker-import-708284929/bin/json: no such file or directory"},"error":"open /var/lib/docker/tmp/docker-import-708284929/bin/json: no such file or directory"} Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2021-11-12T10:20:14Z

Hmm.. good call; looks like we have some fixing to do with the ppc64le and s390x builds (they're disabled by default now, but were started on this)

ppc64le

[2021-11-12T09:51:47.615Z]  > [criu 2/2] RUN --mount=type=cache,sharing=locked,id=moby-criu-aptlib,target=/var/lib/apt     --mount=type=cache,sharing=locked,id=moby-criu-aptcache,target=/var/cache/apt         echo 'deb https://download.opensuse.org/repositories/devel:/tools:/criu/Debian_11/ /' > /etc/apt/sources.list.d/criu.list         && apt-get update         && apt-get install -y --no-install-recommends criu         && install -D /usr/sbin/criu /build/criu:
[2021-11-12T09:51:47.615Z] #46 7.620 Reading package lists...
[2021-11-12T09:51:47.615Z] #46 12.62 Reading package lists...
[2021-11-12T09:51:47.615Z] #46 17.28 Building dependency tree...
[2021-11-12T09:51:47.615Z] #46 18.13 Reading state information...
[2021-11-12T09:51:47.615Z] #46 18.30 Package criu is not available, but is referred to by another package.
[2021-11-12T09:51:47.615Z] #46 18.30 This may mean that the package is missing, has been obsoleted, or
[2021-11-12T09:51:47.615Z] #46 18.30 is only available from another source
[2021-11-12T09:51:47.615Z] #46 18.30
[2021-11-12T09:51:47.615Z] #46 18.85 E: Package 'criu' has no installation candidate

s390x

[2021-11-12T09:54:16.489Z] + sudo modprobe ip6table_filter
[2021-11-12T09:54:16.489Z] sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper
script returned exit code 1

GordonTheTurtle added the status/0-triage label Aug 7, 2017

thaJeztah mentioned this pull request Aug 7, 2017

Status code images load API returns is misleading #34416

Open

thaJeztah added status/1-design-review and removed status/0-triage labels Aug 7, 2017

cpuguy83 requested changes Aug 7, 2017

View reviewed changes

thaJeztah force-pushed the fix-invalid-import-status-code branch 2 times, most recently from 6a24b31 to 0004b18 Compare August 15, 2017 14:33

thaJeztah added the rebuild/* label Aug 17, 2017

GordonTheTurtle removed the rebuild/* label Aug 17, 2017

thaJeztah force-pushed the fix-invalid-import-status-code branch from 0004b18 to 1f59db5 Compare August 17, 2017 21:49

GordonTheTurtle assigned tonistiigi Aug 22, 2017

thaJeztah force-pushed the fix-invalid-import-status-code branch from 1f59db5 to 99e7612 Compare October 11, 2017 22:27

cpuguy83 reviewed Jan 17, 2018

View reviewed changes

thaJeztah mentioned this pull request Jan 19, 2018

Docker for Mac "hangs up" when docker load invalid archive docker/for-mac#2471

Closed

vdemeester requested review from AkihiroSuda, cpuguy83 and tonistiigi February 16, 2018 10:35

cpuguy83 reviewed Feb 16, 2018

View reviewed changes

image/tarexport/load.go Outdated

Copy link

Member

cpuguy83 Feb 16, 2018

Choose a reason for hiding this comment

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

errdefs.InvalidParameter(err)

thaJeztah force-pushed the fix-invalid-import-status-code branch from 99e7612 to fa8fbbe Compare February 16, 2018 16:22

thaJeztah added area/api API status/2-code-review and removed status/1-design-review labels Feb 16, 2018

thaJeztah force-pushed the fix-invalid-import-status-code branch from fa8fbbe to a09c1c5 Compare February 16, 2018 17:00

thaJeztah force-pushed the fix-invalid-import-status-code branch from a09c1c5 to 924450d Compare October 25, 2018 16:09

derek bot added the rebuild/* label Dec 21, 2018

GordonTheTurtle removed the rebuild/* label Dec 21, 2018

derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

thaJeztah added the kind/bugfix PR's that fix bugs label Oct 10, 2019

thaJeztah mentioned this pull request Nov 25, 2019

image_load returns 200 even when error occurs for "no space left on device" #40220

Closed

thaJeztah force-pushed the fix-invalid-import-status-code branch 3 times, most recently from 2cacd95 to a06ba87 Compare April 23, 2021 10:36

thaJeztah force-pushed the fix-invalid-import-status-code branch from a06ba87 to 7518ec9 Compare November 12, 2021 09:48

crazy-max mentioned this pull request Sep 2, 2022

Dockerfile: pin criu version and build from source #44086

Open

	outStream := config.OutStream
	if config.Stream {
	wf := ioutils.NewWriteFlusher(outStream)
	defer wf.Close()
	wf.Flush()
	outStream = wf
	}

Conversation

thaJeztah commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 7, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpuguy83 Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 18, 2017

Uh oh!

cpuguy83 Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 19, 2018

Uh oh!

cpuguy83 Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 16, 2018

Uh oh!

thaJeztah commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Sep 18, 2018

Uh oh!

olljanat commented Dec 22, 2018

Uh oh!

thaJeztah commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaJeztah commented Aug 7, 2017 •

edited

Loading

thaJeztah commented Feb 16, 2018 •

edited

Loading