Merge pull request #2608 from norio-nomura/re-download-digest-less-ca…

…ched-images-if-last-modified-differs

downloader: re-download digest-less cached images if `last-modified` differs

pkg/downloader/downloader.go

@@ -229,6 +229,7 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 	}
 	if _, err := os.Stat(shadData); err == nil {
 		logrus.Debugf("file %q is cached as %q", localPath, shadData)
+		useCache := true
 		if _, err := os.Stat(shadDigest); err == nil {
 			logrus.Debugf("Comparing digest %q with the cached digest file %q, not computing the actual digest of %q",
 				o.expectedDigest, shadDigest, shadData)
@@ -239,18 +240,27 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 				return nil, err
 			}
 		} else {
-			if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, o.description, o.expectedDigest); err != nil {
-				return nil, err
+			if match, lmCached, lmRemote, err := matchLastModified(ctx, shadTime, remote); err != nil {
+				logrus.WithError(err).Info("Failed to retrieve last-modified for cached digest-less image; using cached image.")
+			} else if match {
+				if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, o.description, o.expectedDigest); err != nil {
+					return nil, err
+				}
+			} else {
+				logrus.Infof("Re-downloading digest-less image: last-modified mismatch (cached: %q, remote: %q)", lmCached, lmRemote)
+				useCache = false
 			}
 		}
-		res := &Result{
-			Status:          StatusUsedCache,
-			CachePath:       shadData,
-			LastModified:    readTime(shadTime),
-			ContentType:     readFile(shadType),
-			ValidatedDigest: o.expectedDigest != "",
+		if useCache {
+			res := &Result{
+				Status:          StatusUsedCache,
+				CachePath:       shadData,
+				LastModified:    readTime(shadTime),
+				ContentType:     readFile(shadType),
+				ValidatedDigest: o.expectedDigest != "",
+			}
+			return res, nil
 		}
-		return res, nil
 	}
 	if err := os.RemoveAll(shad); err != nil {
 		return nil, err
@@ -548,6 +558,43 @@ func validateLocalFileDigest(localPath string, expectedDigest digest.Digest) err
 	return nil
 }
 
+// mathLastModified takes params:
+//   - ctx: context for calling httpclientutil.Head
+//   - lastModifiedPath: path of the cached last-modified time file
+//   - url: URL to fetch the last-modified time
+//
+// returns:
+//   - matched: whether the last-modified time matches
+//   - lmCached: last-modified time string from the lastModifiedPath
+//   - lmRemote: last-modified time string from the URL
+//   - err: error if fetching the last-modified time from the URL fails
+func matchLastModified(ctx context.Context, lastModifiedPath, url string) (matched bool, lmCached, lmRemote string, err error) {
+	lmCached = readFile(lastModifiedPath)
+	if lmCached == "" {
+		return false, "<not cached>", "<not checked>", nil
+	}
+	resp, err := httpclientutil.Head(ctx, http.DefaultClient, url)
+	if err != nil {
+		return false, lmCached, "<failed to fetch remote>", err
+	}
+	defer resp.Body.Close()
+	lmRemote = resp.Header.Get("Last-Modified")
+	if lmRemote == "" {
+		return false, lmCached, "<missing Last-Modified header>", nil
+	}
+	lmCachedTime, errParsingCachedTime := time.Parse(http.TimeFormat, lmCached)
+	lmRemoteTime, errParsingRemoteTime := time.Parse(http.TimeFormat, lmRemote)
+	if errParsingCachedTime != nil && errParsingRemoteTime != nil {
+		// both time strings are failed to parse, so compare them as strings
+		return lmCached == lmRemote, lmCached, lmRemote, nil
+	} else if errParsingCachedTime == nil && errParsingRemoteTime == nil {
+		// both time strings are successfully parsed, so compare them as times
+		return lmRemoteTime.Equal(lmCachedTime), lmCached, lmRemote, nil
+	}
+	// ignore parsing errors for either time string and assume they are different
+	return false, lmCached, lmRemote, nil
+}
+
 func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url, description string, expectedDigest digest.Digest) error {
 	if localPath == "" {
 		return fmt.Errorf("downloadHTTP: got empty localPath")

pkg/downloader/downloader_test.go

@@ -121,6 +121,69 @@ func TestDownloadRemote(t *testing.T) {
 	})
 }
 
+func TestRedownloadRemote(t *testing.T) {
+	remoteDir, err := os.MkdirTemp("", "redownloadRemote")
+	assert.NilError(t, err)
+	t.Cleanup(func() { _ = os.RemoveAll(remoteDir) })
+	ts := httptest.NewServer(http.FileServer(http.Dir(remoteDir)))
+	t.Cleanup(ts.Close)
+
+	cacheDir, err := os.MkdirTemp("", "redownloadCache")
+	assert.NilError(t, err)
+	t.Cleanup(func() { _ = os.RemoveAll(cacheDir) })
+
+	downloadDir, err := os.MkdirTemp("", "redownloadLocal")
+	assert.NilError(t, err)
+	t.Cleanup(func() { _ = os.RemoveAll(downloadDir) })
+
+	cacheOpt := WithCacheDir(cacheDir)
+
+	t.Run("digest-less", func(t *testing.T) {
+		remoteFile := filepath.Join(remoteDir, "digest-less.txt")
+		assert.NilError(t, os.WriteFile(remoteFile, []byte("digest-less"), 0o644))
+		assert.NilError(t, os.Chtimes(remoteFile, time.Now(), time.Now().Add(-time.Hour)))
+		opt := []Opt{cacheOpt}
+
+		r, err := Download(context.Background(), filepath.Join(downloadDir, "digest-less1.txt"), ts.URL+"/digest-less.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusDownloaded, r.Status)
+		r, err = Download(context.Background(), filepath.Join(downloadDir, "digest-less2.txt"), ts.URL+"/digest-less.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusUsedCache, r.Status)
+
+		// modifying remote file will cause redownload
+		assert.NilError(t, os.Chtimes(remoteFile, time.Now(), time.Now()))
+		r, err = Download(context.Background(), filepath.Join(downloadDir, "digest-less3.txt"), ts.URL+"/digest-less.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusDownloaded, r.Status)
+	})
+
+	t.Run("has-digest", func(t *testing.T) {
+		remoteFile := filepath.Join(remoteDir, "has-digest.txt")
+		bytes := []byte("has-digest")
+		assert.NilError(t, os.WriteFile(remoteFile, bytes, 0o644))
+		assert.NilError(t, os.Chtimes(remoteFile, time.Now(), time.Now().Add(-time.Hour)))
+
+		digester := digest.SHA256.Digester()
+		_, err := digester.Hash().Write(bytes)
+		assert.NilError(t, err)
+		opt := []Opt{cacheOpt, WithExpectedDigest(digester.Digest())}
+
+		r, err := Download(context.Background(), filepath.Join(downloadDir, "has-digest1.txt"), ts.URL+"/has-digest.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusDownloaded, r.Status)
+		r, err = Download(context.Background(), filepath.Join(downloadDir, "has-digest2.txt"), ts.URL+"/has-digest.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusUsedCache, r.Status)
+
+		// modifying remote file won't cause redownload because expected digest is provided
+		assert.NilError(t, os.Chtimes(remoteFile, time.Now(), time.Now()))
+		r, err = Download(context.Background(), filepath.Join(downloadDir, "has-digest3.txt"), ts.URL+"/has-digest.txt", opt...)
+		assert.NilError(t, err)
+		assert.Equal(t, StatusUsedCache, r.Status)
+	})
+}
+
 func TestDownloadLocal(t *testing.T) {
 	const emptyFileDigest = "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
 	const testDownloadLocalDigest = "sha256:0c1e0fba69e8919b306d030bf491e3e0c46cf0a8140ff5d7516ba3a83cbea5b3"

pkg/httpclientutil/httpclientutil.go

@@ -32,6 +32,22 @@ func Get(ctx context.Context, c *http.Client, url string) (*http.Response, error
 	return resp, nil
 }
 
+func Head(ctx context.Context, c *http.Client, url string) (*http.Response, error) {
+	req, err := http.NewRequestWithContext(ctx, "HEAD", url, http.NoBody)
+	if err != nil {
+		return nil, err
+	}
+	resp, err := c.Do(req)
+	if err != nil {
+		return nil, err
+	}
+	if err := Successful(resp); err != nil {
+		resp.Body.Close()
+		return nil, err
+	}
+	return resp, nil
+}
+
 func Post(ctx context.Context, c *http.Client, url string, body io.Reader) (*http.Response, error) {
 	req, err := http.NewRequestWithContext(ctx, "POST", url, body)
 	if err != nil {