Handle rebasing images with empty layers properly (#733)

Manifest v2 specs do not include layer entries for empty layers,
but the history in the config will include an entry for empty
layers (e.g. produced by `ENV` directive). History entries for
empty layers include `empty_layer` field set to `true`.

As a result, when rebasing and copying from the new base and
original to the rebased image, it's not sufficient to simply
iterate layers and pull from the same position/index in the
history from config. Instead, history should be iterated and
used to generate `Addendum`, while simultaneously iterating
layers, but only including the layer (and advancing the iterator)
if it's for a history item for a non-empty layer.

Since the history in config is optional per the OCI spec [1],
iteration via layers will still happen in the event that not
all layers are consumed during the history pass. (This should
also guard against a malformed history).

[1]: https://github.com/opencontainers/image-spec/blob/master/config.md

Author: milas

pkg/v1/mutate/image.go

@@ -69,13 +69,15 @@ func (i *image) compute() error {
 	digestMap := make(map[v1.Hash]v1.Layer)
 
 	for _, add := range i.adds {
-		diffID, err := add.Layer.DiffID()
-		if err != nil {
-			return err
-		}
-		diffIDs = append(diffIDs, diffID)
 		history = append(history, add.History)
-		diffIDMap[diffID] = add.Layer
+		if add.Layer != nil {
+			diffID, err := add.Layer.DiffID()
+			if err != nil {
+				return err
+			}
+			diffIDs = append(diffIDs, diffID)
+			diffIDMap[diffID] = add.Layer
+		}
 	}
 
 	m, err := i.base.Manifest()
@@ -85,6 +87,11 @@ func (i *image) compute() error {
 	manifest := m.DeepCopy()
 	manifestLayers := manifest.Layers
 	for _, add := range i.adds {
+		if add.Layer == nil {
+			// Empty layers include only history in manifest.
+			continue
+		}
+
 		desc, err := partial.Descriptor(add.Layer)
 		if err != nil {
 			return err
@@ -251,7 +258,7 @@ func (i *image) LayerByDiffID(h v1.Hash) (v1.Layer, error) {
 
 func validate(adds []Addendum) error {
 	for _, add := range adds {
-		if add.Layer == nil {
+		if add.Layer == nil && !add.History.EmptyLayer {
 			return errors.New("unable to add a nil layer to the image")
 		}
 	}

pkg/v1/mutate/rebase.go

@@ -50,6 +50,11 @@ func Rebase(orig, oldBase, newBase v1.Image) (v1.Image, error) {
 		}
 	}
 
+	oldConfig, err := oldBase.ConfigFile()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get config for old base: %v", err)
+	}
+
 	origConfig, err := orig.ConfigFile()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get config for original: %v", err)
@@ -73,25 +78,48 @@ func Rebase(orig, oldBase, newBase v1.Image) (v1.Image, error) {
 		return nil, fmt.Errorf("could not get config for new base: %v", err)
 	}
 	// Add new base layers.
-	for i := range newBaseLayers {
-		rebasedImage, err = Append(rebasedImage, Addendum{
-			Layer:   newBaseLayers[i],
-			History: newConfig.History[i],
-		})
-		if err != nil {
-			return nil, fmt.Errorf("failed to append layer %d of new base layers", i)
-		}
+	rebasedImage, err = Append(rebasedImage, createAddendums(0, 0, newConfig.History, newBaseLayers)...)
+	if err != nil {
+		return nil, fmt.Errorf("failed to append new base image: %v", err)
 	}
+
 	// Add original layers above the old base.
-	start := len(oldBaseLayers)
-	for i := range origLayers[start:] {
-		rebasedImage, err = Append(rebasedImage, Addendum{
-			Layer:   origLayers[start+i],
-			History: origConfig.History[start+i],
-		})
-		if err != nil {
-			return nil, fmt.Errorf("failed to append layer %d of original layers", i)
-		}
+	rebasedImage, err = Append(rebasedImage, createAddendums(len(oldConfig.History), len(oldBaseLayers)+1, origConfig.History, origLayers)...)
+	if err != nil {
+		return nil, fmt.Errorf("failed to append original image: %v", err)
 	}
+
 	return rebasedImage, nil
 }
+
+// createAddendums makes a list of addendums from a history and layers starting from a specific history and layer
+// indexes.
+func createAddendums(startHistory, startLayer int, history []v1.History, layers []v1.Layer) []Addendum {
+	var adds []Addendum
+	// History should be a superset of layers; empty layers (e.g. ENV statements) only exist in history.
+	// They cannot be iterated identically but must be walked independently, only advancing the iterator for layers
+	// when a history entry for a non-empty layer is seen.
+	layerIndex := 0
+	for historyIndex := range history {
+		var layer v1.Layer
+		emptyLayer := history[historyIndex].EmptyLayer
+		if !emptyLayer {
+			layer = layers[layerIndex]
+			layerIndex++
+		}
+		if historyIndex >= startHistory || layerIndex >= startLayer {
+			adds = append(adds, Addendum{
+				Layer:   layer,
+				History: history[historyIndex],
+			})
+		}
+	}
+	// In the event history was malformed or non-existent, append the remaining layers.
+	for i := layerIndex; i < len(layers); i++ {
+		if i >= startLayer {
+			adds = append(adds, Addendum{Layer: layers[layerIndex]})
+		}
+	}
+
+	return adds
+}

pkg/v1/mutate/rebase_test.go

@@ -44,14 +44,15 @@ func layerDigests(t *testing.T, img v1.Image) []string {
 // random.Image layers.
 func TestRebase(t *testing.T) {
 	// Create a random old base image of 5 layers and get those layers' digests.
-	oldBase, err := random.Image(100, 5)
+	const oldBaseLayerCount = 5
+	oldBase, err := random.Image(100, oldBaseLayerCount)
 	if err != nil {
 		t.Fatalf("random.Image (oldBase): %v", err)
 	}
 	t.Log("Old base:")
 	_ = layerDigests(t, oldBase)
 
-	// Construct an image with 1 layer on top of oldBase.
+	// Construct an image with 2 layers on top of oldBase (an empty layer and a random layer).
 	top, err := random.Image(100, 1)
 	if err != nil {
 		t.Fatalf("random.Image (top): %v", err)
@@ -60,16 +61,27 @@ func TestRebase(t *testing.T) {
 	if err != nil {
 		t.Fatalf("top.Layers: %v", err)
 	}
-	topHistory := v1.History{
-		Author:    "me",
-		Created:   v1.Time{time.Now()},
-		CreatedBy: "test",
-		Comment:   "this is a test",
-	}
-	orig, err := mutate.Append(oldBase, mutate.Addendum{
-		Layer:   topLayers[0],
-		History: topHistory,
-	})
+	orig, err := mutate.Append(oldBase,
+		mutate.Addendum{
+			Layer: nil,
+			History: v1.History{
+				Author:     "me",
+				Created:    v1.Time{Time: time.Now()},
+				CreatedBy:  "test-empty",
+				Comment:    "this is an empty test",
+				EmptyLayer: true,
+			},
+		},
+		mutate.Addendum{
+			Layer: topLayers[0],
+			History: v1.History{
+				Author:    "me",
+				Created:   v1.Time{Time: time.Now()},
+				CreatedBy: "test",
+				Comment:   "this is a test",
+			},
+		},
+	)
 	if err != nil {
 		t.Fatalf("Append: %v", err)
 	}
@@ -116,4 +128,27 @@ func TestRebase(t *testing.T) {
 			t.Errorf("Layer %d mismatch, got %q, want %q", i, got, want)
 		}
 	}
+
+	// Compare rebased history.
+	origConfig, err := orig.ConfigFile()
+	if err != nil {
+		t.Fatalf("orig.ConfigFile: %v", err)
+	}
+	newBaseConfig, err := newBase.ConfigFile()
+	if err != nil {
+		t.Fatalf("newBase.ConfigFile: %v", err)
+	}
+	rebasedConfig, err := rebased.ConfigFile()
+	if err != nil {
+		t.Fatalf("rebased.ConfigFile: %v", err)
+	}
+	wantHistories := append(newBaseConfig.History, origConfig.History[oldBaseLayerCount:]...)
+	if len(wantHistories) != len(rebasedConfig.History) {
+		t.Fatalf("Rebased image contained %d history, want %d", len(rebasedConfig.History), len(wantHistories))
+	}
+	for i, rh := range rebasedConfig.History {
+		if got, want := rh.Comment, wantHistories[i].Comment; got != want {
+			t.Errorf("Layer %d mismatch, got %q, want %q", i, got, want)
+		}
+	}
 }

pkg/v1/random/image.go

@@ -69,7 +69,7 @@ func Image(byteSize, layers int64) (v1.Image, error) {
 			Layer: layer,
 			History: v1.History{
 				Author:    "random.Image",
-				Comment:   fmt.Sprintf("this is a random history %d", i),
+				Comment:   fmt.Sprintf("this is a random history %d of %d", i, layers),
 				CreatedBy: "random",
 				Created:   v1.Time{time.Now()},
 			},