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

http: Handle missing but unambiguous ETags in response #1159

Merged
merged 2 commits into from
Sep 5, 2019

Conversation

rwe
Copy link
Contributor

@rwe rwe commented Sep 4, 2019

Fixes #905 and #784.

If a single ETag is requested in If-None-Match, some servers do not include that (unambiguous) ETag header in the response.

This has not been tested. I'm developing on a Mac and not able to devote much more time to this bug, but figured it was worth at least a beginner shot at fixing it.

Detailed description from #905 (comment) partially reproduced below:

Reproduction Steps

Dockerfile

FROM scratch
ADD https://storage.googleapis.com/gpt-2/models/774M/checkpoint

The first run (or a run after the cache is cleared) succeeds:

DOCKER_BUILDKIT=1 docker build .
[+] Building 0.3s (5/5) FINISHED
 => [internal] load .dockerignore                                                              0.0s
 => => transferring context: 244B                                                              0.0s
 => [internal] load build definition from Dockerfile                                           0.0s
 => => transferring dockerfile: 121B                                                           0.0s
 => https://storage.googleapis.com/gpt-2/models/774M/checkpoint                                0.0s
 => [1/1] ADD https://storage.googleapis.com/gpt-2/models/774M/checkpoint .                    0.0s
 => exporting to image                                                                         0.0s
 => => exporting layers                                                                        0.0s
 => => writing image sha256:9a630453cc77705a4f57c121393a38a4f45f65eba6155ba6c3f27ecab18e2b05   0.0s

Running the same command again:

[+] Building 0.2s (3/4)
 => [internal] load build definition from Dockerfile                                           0.0s
 => => transferring dockerfile: 36B                                                            0.0s
 => [internal] load .dockerignore                                                              0.0s
 => => transferring context: 35B                                                               0.0s
 => ERROR https://storage.googleapis.com/gpt-2/models/774M/checkpoint                          0.1s
------
 > https://storage.googleapis.com/gpt-2/models/774M/checkpoint:
------
invalid not-modified ETag:

Clearing the cache with docker builder prune resets the state.

Cause

If a single ETag is requested in If-None-Match, the server may not include that (unambiguous) ETag header in the response.

Detailed demonstration

Requesting the file directly:

curl --http1.1 -s -L -D /dev/stderr -o /dev/null https://storage.googleapis.com/gpt-2/models/774M/checkpoint

Response headers. Notice the ETag.

HTTP/1.1 200 OK
X-GUploader-UploadID: AEnB2Up6PhdYRb_UZ18VAl30f6XLzFkOoBPnSYSjSKqzk90Go8Zqk-zZoenkbL3inKQz1ozoLjcObKKuIbvOV7XFlSSFO6aW0Q
Expires: Wed, 04 Sep 2019 19:23:16 GMT
Date: Wed, 04 Sep 2019 19:23:16 GMT
Cache-Control: private, max-age=0
Last-Modified: Tue, 20 Aug 2019 15:50:08 GMT
ETag: "ca0368fcd3c4c1a99aca42511d0c1f12"
x-goog-generation: 1566316208157027
x-goog-metageneration: 1
x-goog-stored-content-encoding: identity
x-goog-stored-content-length: 77
Content-Type: application/octet-stream
x-goog-hash: crc32c=BI0EFw==
x-goog-hash: md5=ygNo/NPEwamaykJRHQwfEg==
x-goog-storage-class: MULTI_REGIONAL
Accept-Ranges: bytes
Content-Length: 77
Server: UploadServer
Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"

Requesting the file If-None-Match with that ETag:

curl --http1.1 -s -L -D /dev/stderr -o /dev/null -H 'If-None-Match: "ca0368fcd3c4c1a99aca42511d0c1f12"' https://storage.googleapis.com/gpt-2/models/774M/checkpoint

Notice that the ETag is not included in the response headers.

HTTP/1.1 304 Not Modified
X-GUploader-UploadID: AEnB2UpEqNrI5fDJjglD2f4--3CCzskMyUg-Fo1RZxoqqHq17HG8W_gURMO6uUVy9B6Mg4450GyA4yRTjPqEJY8v6dtxhuHcLQ
Expires: Wed, 04 Sep 2019 19:23:36 GMT
Date: Wed, 04 Sep 2019 19:23:36 GMT
Cache-Control: private, max-age=0
Last-Modified: Tue, 20 Aug 2019 15:50:08 GMT
Content-Length: 0
Server: UploadServer
Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"

Requesting the file with multiple ETags in If-None-Match:

curl --http1.1 -s -L -D /dev/stderr -o /dev/null -H 'If-None-Match: "ca0368fcd3c4c1a99aca42511d0c1f12", "foobar"' https://storage.googleapis.com/gpt-2/models/774M/checkpoint

Now the ETag is included to disambiguate.

HTTP/1.1 200 OK
X-GUploader-UploadID: AEnB2UpD_nKA4ZMNpJvC97lMJfyXjcr9myMAxojFypxW8lUNiEGiwdaOtezf74-OBCHDXn7T4Ru57oelrDHb01wY9IMU1Qdl6A
Expires: Wed, 04 Sep 2019 19:26:02 GMT
Date: Wed, 04 Sep 2019 19:26:02 GMT
Cache-Control: private, max-age=0
Last-Modified: Tue, 20 Aug 2019 15:50:08 GMT
ETag: "ca0368fcd3c4c1a99aca42511d0c1f12"
x-goog-generation: 1566316208157027
x-goog-metageneration: 1
x-goog-stored-content-encoding: identity
x-goog-stored-content-length: 77
Content-Type: application/octet-stream
x-goog-hash: crc32c=BI0EFw==
x-goog-hash: md5=ygNo/NPEwamaykJRHQwfEg==
x-goog-storage-class: MULTI_REGIONAL
Accept-Ranges: bytes
Content-Length: 77
Server: UploadServer
Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"

@GordonTheTurtle
Copy link
Collaborator

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "fix-etag" git@github.com:erydo/buildkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

If a single ETag is requested in `If-None-Match`, some servers do not
include that (unambiguous) ETag header in the response.

For detailed description, see:
moby#905 (comment)

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
Otherwise a 200 response without an ETag could be incorrectly associated
to previous content in the following scenario:

* The remote server had in the past responded with an ETag for this
  resource, which was cached.
  - (Otherwise, onlyETag would be empty)
* That was the only ETag cached for this resource.
  - (Otherwise, onlyETag would be empty)
* The remote server then stopped supporting ETag/If-None-Match for this
  resource at all.
  - (Otherwise, it would respond with a 304 or a 200+ETag)

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
@rwe

This comment has been minimized.

@tiborvass tiborvass merged commit 7886d6b into moby:master Sep 5, 2019
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Oct 24, 2019
full diff: moby/buildkit@f704282...ae10b29

fixes:

- moby/buildkit#1144 Fix socket handling
    - fsutil: Handle copying unix sockets in local sources
    - update github.com/containerd/continuity to 75bee3e2ccb6
    - update github.com/tonistiigi/fsutil to 3d2716dd0a4d
- moby/buildkit#1150 ssh: Fix file descriptor leak when doing SSH forwarding
    - fixes moby/buildkit#1146 SSH Forwarding Doesn't Release File Descriptors
- moby/buildkit#1156 llbsolver: Fix using multiple remote cache importers
- moby/buildkit#1159 http: Handle missing but unambiguous ETags in response
    - fixes moby/buildkit#905 ADD from url fails with 'invalid not-modified ETag'
    - fixes moby/buildkit#784 invalid not-modified ETag with local network ADD in docker
- moby/buildkit#1166 solver: Fix possible inefficient parallelization in solver
    - fix cases where some events were dropped resulting inefficient parallelization.
- moby/buildkit#1168 vendor: update go-runc to e029b79d
- moby/buildkit#1140 contenthash: Fix bug with symlink in source path of a copy operation
    - fixes moby/buildkit#974 COPY --from fails when source path involves a symlink
    - fixes moby/buildkit#785 COPY rpc error: code = Unknown desc = not found: not found
    - fixes moby/buildkit#958 Issue COPY a file to a symlink directory
- moby/buildkit#1139 executor: oom_score_adj is no longer set from main process

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@BrendonW
Copy link

BrendonW commented Apr 8, 2021

Stupid question? Why is this bug still in "Docker for Mac" which is using a much later version of Buildkit?

Docker Desktop (Mac)

  • Version 3.2.2
  • Engine 20.10.5

Build fails when ADD tries to check Github repository commit with "failed to load cache key: invalid not-modified ETag"

What can I do to help resolve the issue?

@thaJeztah
Copy link
Member

@BrendonW could you open a new ticket with an example to reproduce the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADD from url fails with 'invalid not-modified ETag'
6 participants