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

pkg/downloader/downloader.go

@@ -240,14 +240,14 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 				return nil, err
 			}
 		} else {
-			if match, err := matchLastModified(ctx, shadTime, remote); err != nil {
+			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.Info("Re-downloading digest-less image: last-modified mismatch detected.")
+				logrus.Infof("Re-downloading digest-less image: last-modified mismatch (cached: %q, remote: %q)", lmCached, lmRemote)
 				useCache = false
 			}
 		}
@@ -526,25 +526,41 @@ func validateLocalFileDigest(localPath string, expectedDigest digest.Digest) err
 	return nil
 }
 
-func matchLastModified(ctx context.Context, lastModified, url string) (bool, error) {
-	if lastModified == "" {
-		return false, 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, err
+		return false, lmCached, "<failed to fetch remote>", err
 	}
 	defer resp.Body.Close()
-	lm := resp.Header.Get("Last-Modified")
-	if lm == "" {
-		return false, nil
-	}
-	lmTime, err := time.Parse(http.TimeFormat, lm)
-	if err != nil {
-		return false, nil
-	}
-	lastModifiedTime := readTime(lastModified)
-	return lmTime.Equal(lastModifiedTime), nil
+	lmRemote = resp.Header.Get("Last-Modified")
+	if lmRemote == "" {
+		return false, lmCached, "<missing 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 {

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"