Skip to content

http: Handle missing but unambiguous ETags in response#1159

Merged
tiborvass merged 2 commits intomoby:masterfrom
rwe:fix-etag
Sep 5, 2019
Merged

http: Handle missing but unambiguous ETags in response#1159
tiborvass merged 2 commits intomoby:masterfrom
rwe:fix-etag

Conversation

@rwe
Copy link
Copy Markdown
Contributor

@rwe rwe commented Sep 4, 2019 •
edited
Loading

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
Copy Markdown
Collaborator

GordonTheTurtle commented Sep 4, 2019

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.

@rwe
http: Handle missing but unambiguous ETags in response
c63c6f9 
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>
@rwe rwe mentioned this pull request Sep 4, 2019
Closed
tonistiigi
tonistiigi reviewed Sep 4, 2019
View reviewed changes
@rwe
http: Response refers to requested ETag on 304, not 200
6ff0868 
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.

Sign in to view
@tiborvass tiborvass merged commit 7886d6b into moby:master Sep 5, 2019
@rwe rwe mentioned this pull request Sep 10, 2019
Closed
This was referenced Sep 20, 2019
Merged
Merged
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Oct 24, 2019
@thaJeztah
bump buildkit v0.6.2
869c8f8 
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>
@thaJeztah thaJeztah mentioned this pull request Oct 24, 2019
Closed
@howardjohn howardjohn mentioned this pull request Nov 26, 2019
Merged
@tonistiigi tonistiigi mentioned this pull request Mar 4, 2020
Closed
@BrendonW
Copy link
Copy Markdown

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?

@BrendonW BrendonW mentioned this pull request Apr 8, 2021
Closed
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Apr 8, 2021

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

@shatilov-diman shatilov-diman mentioned this pull request Oct 18, 2021
Closed
@zevisert zevisert mentioned this pull request Apr 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@tiborvass tiborvass tiborvass approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@rwe @GordonTheTurtle @BrendonW @thaJeztah @tonistiigi @tiborvass