Issue containers#155 Making sure existing OCI index gets not overwritten

Signed-off-by: Hardy Ferentschik <[email protected]>

.gitignore

@@ -1,2 +1,9 @@
 vendor
 tools.timestamp
+
+# Idea IDE
+*.iml
+.idea
+
+# Visual Studio Code
+.vscode/*

oci/layout/oci_dest.go

@@ -27,12 +27,23 @@ func newImageDestination(ref ociReference) (types.ImageDestination, error) {
 	if ref.image == "" {
 		return nil, errors.Errorf("cannot save image with empty image.ref.name")
 	}
-	index := imgspecv1.Index{
-		Versioned: imgspec.Versioned{
-			SchemaVersion: 2,
-		},
+
+	var index *imgspecv1.Index
+	if indexExists(ref) {
+		var err error
+		index, err = ref.getIndex()
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		index = &imgspecv1.Index{
+			Versioned: imgspec.Versioned{
+				SchemaVersion: 2,
+			},
+		}
 	}
-	return &ociImageDestination{ref: ref, index: index}, nil
+
+	return &ociImageDestination{ref: ref, index: *index}, nil
 }
 
 // Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
@@ -191,23 +202,20 @@ func (d *ociImageDestination) PutManifest(m []byte) error {
 		Architecture: runtime.GOARCH,
 		OS:           runtime.GOOS,
 	}
-	d.index.Manifests = append(d.index.Manifests, desc)
+	d.addManifest(&desc)
 
 	return nil
 }
 
-func ensureDirectoryExists(path string) error {
-	if _, err := os.Stat(path); err != nil && os.IsNotExist(err) {
-		if err := os.MkdirAll(path, 0755); err != nil {
-			return err
+func (d *ociImageDestination) addManifest(desc *imgspecv1.Descriptor) {
+	for i, manifest := range d.index.Manifests {
+		if manifest.Annotations["org.opencontainers.image.ref.name"] == desc.Annotations["org.opencontainers.image.ref.name"] {
+			// TODO Should there first be a cleanup based on the descriptor we are going to replace?
+			d.index.Manifests[i] = *desc
+			return
 		}
 	}
-	return nil
-}
-
-// ensureParentDirectoryExists ensures the parent of the supplied path exists.
-func ensureParentDirectoryExists(path string) error {
-	return ensureDirectoryExists(filepath.Dir(path))
+	d.index.Manifests = append(d.index.Manifests, *desc)
 }
 
 func (d *ociImageDestination) PutSignatures(signatures [][]byte) error {
@@ -231,3 +239,30 @@ func (d *ociImageDestination) Commit() error {
 	}
 	return ioutil.WriteFile(d.ref.indexPath(), indexJSON, 0644)
 }
+
+func ensureDirectoryExists(path string) error {
+	if _, err := os.Stat(path); err != nil && os.IsNotExist(err) {
+		if err := os.MkdirAll(path, 0755); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// ensureParentDirectoryExists ensures the parent of the supplied path exists.
+func ensureParentDirectoryExists(path string) error {
+	return ensureDirectoryExists(filepath.Dir(path))
+}
+
+// indexExists checks whether the index location specified in the OCI reference exists.
+// The implementation is opinionated, since in case of unexpected errors false is returned
+func indexExists(ref ociReference) bool {
+	_, err := os.Stat(ref.indexPath())
+	if err == nil {
+		return true
+	}
+	if os.IsNotExist(err) {
+		return false
+	}
+	return true
+}

oci/layout/oci_dest_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"path/filepath"
 )
 
 // readerFromFunc allows implementing Reader by any function, e.g. a closure.
@@ -59,3 +60,64 @@ func TestPutBlobDigestFailure(t *testing.T) {
 	require.Error(t, err)
 	require.True(t, os.IsNotExist(err))
 }
+
+// TestPutManifestAppendsToExistingManifest tests that new manifests are getting added to existing index.
+func TestPutManifestAppendsToExistingManifest(t *testing.T) {
+	ref, tmpDir := refToTempOCI(t)
+	defer os.RemoveAll(tmpDir)
+
+	ociRef, ok := ref.(ociReference)
+	require.True(t, ok)
+
+	// iniitally we have one manifest
+	index, err := ociRef.getIndex()
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(index.Manifests), "Unexpected number of manifests")
+
+	// create a new test reference
+	ociRef2, err := NewReference(tmpDir, "new-image")
+	assert.NoError(t, err)
+
+	putTestManifest(t, ociRef2.(ociReference), tmpDir)
+
+	index, err = ociRef.getIndex()
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(index.Manifests), "Unexpected number of manifests")
+}
+
+// TestPutManifestTwice tests that existing manifest gets updated and not appended.
+func TestPutManifestTwice(t *testing.T) {
+	ref, tmpDir := refToTempOCI(t)
+	defer os.RemoveAll(tmpDir)
+
+	ociRef, ok := ref.(ociReference)
+	require.True(t, ok)
+
+	putTestManifest(t, ociRef, tmpDir)
+	putTestManifest(t, ociRef, tmpDir)
+
+	index, err := ociRef.getIndex()
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(index.Manifests), "Unexpected number of manifests")
+}
+
+func putTestManifest(t *testing.T, ociRef ociReference, tmpDir string) {
+	imageDest, err := newImageDestination(ociRef)
+	assert.NoError(t, err)
+
+	data := []byte("abc")
+	err = imageDest.PutManifest(data)
+	assert.NoError(t, err)
+
+	err = imageDest.Commit()
+	assert.NoError(t, err)
+
+	paths := []string{}
+	filepath.Walk(tmpDir, func(path string, info os.FileInfo, err error) error {
+		paths = append(paths, path)
+		return nil
+	})
+
+	digest := "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"
+	assert.Contains(t, paths, filepath.Join(tmpDir, "blobs", "sha256", digest), "The OCI directory does not contain the new manifest data")
+}

oci/layout/oci_transport.go

@@ -189,14 +189,25 @@ func (ref ociReference) NewImage(ctx *types.SystemContext) (types.Image, error)
 	return image.FromSource(src)
 }
 
-func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) {
+// getIndex returns a pointer to the index references by this ociReference. If an error occurs opening an index nil is returned together
+// with an error.
+func (ref ociReference) getIndex() (*imgspecv1.Index, error) {
 	indexJSON, err := os.Open(ref.indexPath())
 	if err != nil {
-		return imgspecv1.Descriptor{}, err
+		return nil, err
 	}
 	defer indexJSON.Close()
-	index := imgspecv1.Index{}
-	if err := json.NewDecoder(indexJSON).Decode(&index); err != nil {
+
+	index := &imgspecv1.Index{}
+	if err := json.NewDecoder(indexJSON).Decode(index); err != nil {
+		return nil, err
+	}
+	return index, nil
+}
+
+func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) {
+	index, err := ref.getIndex()
+	if err != nil {
 		return imgspecv1.Descriptor{}, err
 	}