FIX mutate.Time not respecting history (#1520)

Author: miguelvalerio

pkg/v1/mutate/mutate.go

@@ -332,6 +332,13 @@ func inWhiteoutDir(fileMap map[string]bool, file string) bool {
 	return false
 }
 
+func max(a, b int) int {
+	if a > b {
+		return a
+	}
+	return b
+}
+
 // Time sets all timestamps in an image to the given timestamp.
 func Time(img v1.Image, t time.Time) (v1.Image, error) {
 	newImage := empty.Image
@@ -341,24 +348,43 @@ func Time(img v1.Image, t time.Time) (v1.Image, error) {
 		return nil, fmt.Errorf("getting image layers: %w", err)
 	}
 
-	// Strip away all timestamps from layers
-	newLayers := make([]v1.Layer, len(layers))
-	for idx, layer := range layers {
-		newLayer, err := layerTime(layer, t)
+	ocf, err := img.ConfigFile()
+	if err != nil {
+		return nil, fmt.Errorf("getting original config file: %w", err)
+	}
+
+	addendums := make([]Addendum, max(len(ocf.History), len(layers)))
+	var historyIdx, addendumIdx int
+	for layerIdx := 0; layerIdx < len(layers); addendumIdx, layerIdx = addendumIdx+1, layerIdx+1 {
+		newLayer, err := layerTime(layers[layerIdx], t)
 		if err != nil {
 			return nil, fmt.Errorf("setting layer times: %w", err)
 		}
-		newLayers[idx] = newLayer
+
+		// try to search for the history entry that corresponds to this layer
+		for ; historyIdx < len(ocf.History); historyIdx++ {
+			addendums[addendumIdx].History = ocf.History[historyIdx]
+			// if it's an EmptyLayer, do not set the Layer and have the Addendum with just the History
+			// and move on to the next History entry
+			if ocf.History[historyIdx].EmptyLayer {
+				addendumIdx++
+				continue
+			}
+			// otherwise, we can exit from the cycle
+			historyIdx++
+			break
+		}
+		addendums[addendumIdx].Layer = newLayer
 	}
 
-	newImage, err = AppendLayers(newImage, newLayers...)
-	if err != nil {
-		return nil, fmt.Errorf("appending layers: %w", err)
+	// add all leftover History entries
+	for ; historyIdx < len(ocf.History); historyIdx, addendumIdx = historyIdx+1, addendumIdx+1 {
+		addendums[addendumIdx].History = ocf.History[historyIdx]
 	}
 
-	ocf, err := img.ConfigFile()
+	newImage, err = Append(newImage, addendums...)
 	if err != nil {
-		return nil, fmt.Errorf("getting original config file: %w", err)
+		return nil, fmt.Errorf("appending layers: %w", err)
 	}
 
 	cf, err := newImage.ConfigFile()
@@ -383,6 +409,7 @@ func Time(img v1.Image, t time.Time) (v1.Image, error) {
 		h.Comment = ocf.History[i].Comment
 		h.EmptyLayer = ocf.History[i].EmptyLayer
 		// Explicitly ignore Author field; which hinders reproducibility
+		h.Author = ""
 		cfg.History[i] = h
 	}
 

pkg/v1/mutate/mutate_test.go

@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/match"
@@ -324,20 +325,47 @@ func TestMutateCreatedAt(t *testing.T) {
 }
 
 func TestMutateTime(t *testing.T) {
-	source := sourceImage(t)
-	want := time.Time{}
-	result, err := mutate.Time(source, want)
-	if err != nil {
-		t.Fatalf("failed to mutate a config: %v", err)
-	}
+	for _, tc := range []struct {
+		name   string
+		source v1.Image
+	}{
+		{
+			name:   "image with matching history and layers",
+			source: sourceImage(t),
+		},
+		{
+			name:   "image with empty_layer history entries",
+			source: sourceImagePath(t, "testdata/source_image_with_empty_layer_history.tar"),
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			want := time.Time{}
+			result, err := mutate.Time(tc.source, want)
+			if err != nil {
+				t.Fatalf("failed to mutate a config: %v", err)
+			}
 
-	if configDigestsAreEqual(t, source, result) {
-		t.Fatal("mutating the created time MUST mutate the config digest")
-	}
+			if configDigestsAreEqual(t, tc.source, result) {
+				t.Fatal("mutating the created time MUST mutate the config digest")
+			}
 
-	got := getConfigFile(t, result).Created.Time
-	if got != want {
-		t.Fatalf("mutating the created time MUST mutate the time from %v to %v", got, want)
+			mutatedOriginalConfig := getConfigFile(t, tc.source).DeepCopy()
+			gotConfig := getConfigFile(t, result)
+
+			// manually change the fields we expect to be changed by mutate.Time
+			mutatedOriginalConfig.Author = ""
+			mutatedOriginalConfig.Created = v1.Time{Time: want}
+			for i := range mutatedOriginalConfig.History {
+				mutatedOriginalConfig.History[i].Created = v1.Time{Time: want}
+				mutatedOriginalConfig.History[i].Author = ""
+			}
+
+			if diff := cmp.Diff(mutatedOriginalConfig, gotConfig,
+				cmpopts.IgnoreFields(v1.RootFS{}, "DiffIDs"),
+			); diff != "" {
+				t.Errorf("configFile() mismatch (-want +got):\n%s", diff)
+			}
+		})
 	}
 }
 
@@ -636,9 +664,13 @@ func assertMTime(t *testing.T, layer v1.Layer, expectedTime time.Time) {
 }
 
 func sourceImage(t *testing.T) v1.Image {
+	return sourceImagePath(t, "testdata/source_image.tar")
+}
+
+func sourceImagePath(t *testing.T, tarPath string) v1.Image {
 	t.Helper()
 
-	image, err := tarball.ImageFromPath("testdata/source_image.tar", nil)
+	image, err := tarball.ImageFromPath(tarPath, nil)
 	if err != nil {
 		t.Fatalf("Error loading image: %v", err)
 	}