Merge pull request #18047 from aaronlehmann/push-fix

Correct parent chain in v2 push when v1Compatibility files on the disk are inconsistent

graph/push_v2.go

@@ -3,6 +3,8 @@ package graph
 import (
 	"bufio"
 	"compress/gzip"
+	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -205,6 +207,11 @@ func (p *v2Pusher) pushV2Tag(tag string) error {
 		p.layersPushed[dgst] = true
 	}
 
+	// Fix parent chain if necessary
+	if err = fixHistory(m); err != nil {
+		return err
+	}
+
 	logrus.Infof("Signed manifest for %s:%s using daemon's key: %s", p.repo.Name(), tag, p.trustKey.KeyID())
 	signed, err := schema1.Sign(m, p.trustKey)
 	if err != nil {
@@ -226,6 +233,90 @@ func (p *v2Pusher) pushV2Tag(tag string) error {
 	return manSvc.Put(signed)
 }
 
+// fixHistory makes sure that the manifest has parent IDs that are consistent
+// with its image IDs. Because local image IDs are generated from the
+// configuration and filesystem contents, but IDs in the manifest are preserved
+// from the original pull, it's possible to have inconsistencies where parent
+// IDs don't match up with the other IDs in the manifest. This happens in the
+// case where an engine pulls images where are identical except the IDs from the
+// manifest - the local ID will be the same, and one of the v1Compatibility
+// files gets discarded.
+func fixHistory(m *schema1.Manifest) error {
+	var lastID string
+
+	for i := len(m.History) - 1; i >= 0; i-- {
+		var historyEntry map[string]*json.RawMessage
+		if err := json.Unmarshal([]byte(m.History[i].V1Compatibility), &historyEntry); err != nil {
+			return err
+		}
+
+		idJSON, present := historyEntry["id"]
+		if !present || idJSON == nil {
+			return errors.New("missing id key in v1compatibility file")
+		}
+		var id string
+		if err := json.Unmarshal(*idJSON, &id); err != nil {
+			return err
+		}
+
+		parentJSON, present := historyEntry["parent"]
+
+		if i == len(m.History)-1 {
+			// The base layer must not reference a parent layer,
+			// otherwise the manifest is incomplete. There is an
+			// exception for Windows to handle base layers.
+			if !allowBaseParentImage && present && parentJSON != nil {
+				var parent string
+				if err := json.Unmarshal(*parentJSON, &parent); err != nil {
+					return err
+				}
+				if parent != "" {
+					logrus.Debugf("parent id mismatch detected; fixing. parent reference: %s", parent)
+					delete(historyEntry, "parent")
+					fixedHistory, err := json.Marshal(historyEntry)
+					if err != nil {
+						return err
+					}
+					m.History[i].V1Compatibility = string(fixedHistory)
+				}
+			}
+		} else {
+			// For all other layers, the parent ID should equal the
+			// ID of the next item in the history list. If it
+			// doesn't, fix it up (but preserve all other fields,
+			// possibly including fields that aren't known to this
+			// engine version).
+			if !present || parentJSON == nil {
+				return errors.New("missing parent key in v1compatibility file")
+			}
+			var parent string
+			if err := json.Unmarshal(*parentJSON, &parent); err != nil {
+				return err
+			}
+			if parent != lastID {
+				logrus.Debugf("parent id mismatch detected; fixing. parent reference: %s actual id: %s", parent, id)
+				historyEntry["parent"] = rawJSON(lastID)
+				fixedHistory, err := json.Marshal(historyEntry)
+				if err != nil {
+					return err
+				}
+				m.History[i].V1Compatibility = string(fixedHistory)
+			}
+		}
+		lastID = id
+	}
+
+	return nil
+}
+
+func rawJSON(value interface{}) *json.RawMessage {
+	jsonval, err := json.Marshal(value)
+	if err != nil {
+		return nil
+	}
+	return (*json.RawMessage)(&jsonval)
+}
+
 func (p *v2Pusher) pushV2Image(bs distribution.BlobService, img *image.Image) (digest.Digest, error) {
 	out := p.config.OutStream
 

integration-cli/docker_cli_push_test.go

@@ -2,13 +2,16 @@ package main
 
 import (
 	"archive/tar"
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"time"
 
+	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/integration/checker"
 	"github.com/go-check/check"
 )
@@ -83,6 +86,46 @@ func (s *DockerRegistrySuite) TestPushMultipleTags(c *check.C) {
 	}
 }
 
+// TestPushBadParentChain tries to push an image with a corrupted parent chain
+// in the v1compatibility files, and makes sure the push process fixes it.
+func (s *DockerRegistrySuite) TestPushBadParentChain(c *check.C) {
+	repoName := fmt.Sprintf("%v/dockercli/badparent", privateRegistryURL)
+
+	id, err := buildImage(repoName, `
+	    FROM busybox
+	    CMD echo "adding another layer"
+	    `, true)
+	if err != nil {
+		c.Fatal(err)
+	}
+
+	// Push to create v1compatibility file
+	dockerCmd(c, "push", repoName)
+
+	// Corrupt the parent in the v1compatibility file from the top layer
+	filename := filepath.Join(dockerBasePath, "graph", id, "v1Compatibility")
+
+	jsonBytes, err := ioutil.ReadFile(filename)
+	c.Assert(err, check.IsNil, check.Commentf("Could not read v1Compatibility file: %s", err))
+
+	var img image.Image
+	err = json.Unmarshal(jsonBytes, &img)
+	c.Assert(err, check.IsNil, check.Commentf("Could not unmarshal json: %s", err))
+
+	img.Parent = "1234123412341234123412341234123412341234123412341234123412341234"
+
+	jsonBytes, err = json.Marshal(&img)
+	c.Assert(err, check.IsNil, check.Commentf("Could not marshal json: %s", err))
+
+	err = ioutil.WriteFile(filename, jsonBytes, 0600)
+	c.Assert(err, check.IsNil, check.Commentf("Could not write v1Compatibility file: %s", err))
+
+	dockerCmd(c, "push", repoName)
+
+	// pull should succeed
+	dockerCmd(c, "pull", repoName)
+}
+
 func (s *DockerRegistrySuite) TestPushEmptyLayer(c *check.C) {
 	repoName := fmt.Sprintf("%v/dockercli/emptylayer", privateRegistryURL)
 	emptyTarball, err := ioutil.TempFile("", "empty_tarball")