downloader: check etag in a HEAD request

downloader/download.go

@@ -49,7 +49,7 @@
 	return logrus.NewEntry(log)
 }
 
-// SHA256 returns the hash of the file if possible, empty string otherwise.
+// SHA256 returns the (hex-encoded) hash of the file if possible, empty string otherwise.
 func SHA256(path string) (string, error) {
 	file, err := os.Open(path)
 
@@ -204,9 +204,10 @@
 	return dstInfo.ModTime(), dstInfo.Mode().Perm()
 }
 
-// checkLastModified returns true if the file seems up to date according to its modification time.
-func (d *Downloader) checkLastModified(ctx context.Context, url string, modTime time.Time) (bool, error) {
-	if !d.lastModified {
+// isLocalFresh returns whether we can skip the download, according to mtime and etag values, when set.
+// If neither is set, the file is considered stale after the shelf life period.
+func (d *Downloader) isLocalFresh(ctx context.Context, url string, modTime time.Time, etag string) (bool, error) {
+	if !d.lastModified && d.etagFn == nil {
 		return false, nil
 	}
 
@@ -221,6 +222,10 @@
 		return false, fmt.Errorf("failed to create HEAD request for %s: %w", url, err)
 	}
 
+	if etag != "" {
+		req.Header.Add("If-None-Match", etag)
+	}
+
 	client := d.httpClient
 	if client == nil {
 		client = http.DefaultClient
@@ -232,7 +237,13 @@
 	}
 	defer resp.Body.Close()
 
-	if resp.StatusCode != http.StatusOK {
+	switch resp.StatusCode {
+	case http.StatusNotModified:
+		d.logger.Debug("Not modified (head)")
+		return true, nil
+	case http.StatusOK:
+		break
+	default:
 		return false, BadHTTPCodeError{url, resp.StatusCode}
 	}
 
@@ -442,7 +453,7 @@
 
 // Download downloads the file from the URL to the destination path.
 // Returns true if the file was downloaded, false if it was already up to date.
 func (d *Downloader) Download(ctx context.Context, url string) (bool, error) {
 	// only one of etagfn, ifmod, lastmod
 	if err := d.ValidateOptions(); err != nil {
 		return false, fmt.Errorf("downloader options: %w", err)
@@ -452,7 +463,18 @@
 
 	destModTime, destFileMode := d.getDestInfo()
 
-	uptodate, err := d.checkLastModified(ctx, url, destModTime)
+	etag := ""
+	// only add If-None-Match if destPath exists, it could have been deleted leaving an .etag
+	if d.etagFn != nil && (destModTime != time.Time{}) {
+		var err error
+
+		etag, err = (*d.etagFn)(d.destPath)
+		if err != nil {
+			d.logger.Warnf("Failed to get etag: %s", err)
+		}
+	}
+
+	uptodate, err := d.isLocalFresh(ctx, url, destModTime, etag)
 	if err != nil {
 		d.logger.Warnf("Failed to check last modified: %s", err)
 	}
@@ -470,19 +492,12 @@
 
 	if d.ifModifiedSince && (destModTime != time.Time{}) {
 		req.Header.Add("If-Modified-Since", destModTime.Format(http.TimeFormat))
+		d.logger.Trace("If-Modified-Since: ", destModTime)
 	}
 
-	// only add If-None-Match if destPath exists,
-	// it could have been deleted leaving an .etag
-	if d.etagFn != nil && (destModTime != time.Time{}) {
-		etag, err := (*d.etagFn)(d.destPath)
-		if err != nil {
-			d.logger.Warnf("Failed to get etag: %s", err)
-		}
-
-		if etag != "" {
-			req.Header.Add("If-None-Match", etag)
-		}
+	if etag != "" {
+		req.Header.Add("If-None-Match", etag)
+		d.logger.Trace("If-None-Match: ", etag)
 	}
 
 	resp, err := d.httpClient.Do(req)
@@ -498,7 +513,7 @@
 	case http.StatusOK:
 		break
 	case http.StatusNotModified:
-		d.logger.Debug("Not modified")
+		d.logger.Debug("Not modified (get)")
 		return false, nil
 	default:
 		return false, BadHTTPCodeError{url, resp.StatusCode}