From c63c6f9683bd3c1f449d100605c8a27fc20bf9dd Mon Sep 17 00:00:00 2001 From: Robert Estelle Date: Wed, 4 Sep 2019 17:59:17 -0400 Subject: [PATCH 1/2] http: Handle missing but unambiguous ETags in response 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: https://github.com/moby/buildkit/issues/905#issuecomment-528058142 Signed-off-by: Robert Estelle --- source/http/httpsource.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/source/http/httpsource.go b/source/http/httpsource.go index 7394a03e0a02..ba25608a7c52 100644 --- a/source/http/httpsource.go +++ b/source/http/httpsource.go @@ -144,6 +144,11 @@ func (hs *httpSourceHandler) CacheKey(ctx context.Context, index int) (string, b req = req.WithContext(ctx) m := map[string]*metadata.StorageItem{} + // If we request a single ETag in 'If-None-Match', some servers omit the + // unambiguous ETag in their response. + // See: https://github.com/moby/buildkit/issues/905 + var onlyETag string + if len(sis) > 0 { for _, si := range sis { // if metaDigest := getMetaDigest(si); metaDigest == hs.formatCacheKey("") { @@ -160,6 +165,10 @@ func (hs *httpSourceHandler) CacheKey(ctx context.Context, index int) (string, b etags = append(etags, t) } req.Header.Add("If-None-Match", strings.Join(etags, ", ")) + + if len(etags) == 1 { + onlyETag = etags[0] + } } } @@ -172,6 +181,9 @@ func (hs *httpSourceHandler) CacheKey(ctx context.Context, index int) (string, b if err == nil { if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNotModified { respETag := resp.Header.Get("ETag") + if respETag == "" && onlyETag != "" { + respETag = onlyETag + } si, ok := m[respETag] if ok { hs.refID = si.ID() @@ -197,6 +209,13 @@ func (hs *httpSourceHandler) CacheKey(ctx context.Context, index int) (string, b } if resp.StatusCode == http.StatusNotModified { respETag := resp.Header.Get("ETag") + if respETag == "" && onlyETag != "" { + respETag = onlyETag + + // Set the missing ETag header on the response so that it's available + // to .save() + resp.Header.Set("ETag", onlyETag) + } si, ok := m[respETag] if !ok { return "", false, errors.Errorf("invalid not-modified ETag: %v", respETag) From 6ff0868457aea28b3884d00704443d55d4bdb673 Mon Sep 17 00:00:00 2001 From: Robert Estelle Date: Wed, 4 Sep 2019 20:22:48 -0400 Subject: [PATCH 2/2] http: Response refers to requested ETag on 304, not 200 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 --- source/http/httpsource.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/http/httpsource.go b/source/http/httpsource.go index ba25608a7c52..9bde6195cb4c 100644 --- a/source/http/httpsource.go +++ b/source/http/httpsource.go @@ -181,7 +181,10 @@ func (hs *httpSourceHandler) CacheKey(ctx context.Context, index int) (string, b if err == nil { if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNotModified { respETag := resp.Header.Get("ETag") - if respETag == "" && onlyETag != "" { + + // If a 304 is returned without an ETag and we had only sent one ETag, + // the response refers to the ETag we asked about. + if respETag == "" && onlyETag != "" && resp.StatusCode == http.StatusNotModified { respETag = onlyETag } si, ok := m[respETag]