Merge pull request #312 from estebanreyl/esrey/bugfix-tags-on-manifest-deduplication

Fix userspace convertor output tagging on manifest dedup

Author: BigVan

cmd/convertor/builder/builder.go

@@ -363,7 +363,13 @@ func (b *overlaybdBuilder) Build(ctx context.Context) (v1.Descriptor, error) {
 	// when errors are encountered fallback to regular conversion
 	if convertedDesc, err := b.engine.CheckForConvertedManifest(ctx); err == nil && convertedDesc.Digest != "" {
 		logrus.Infof("Image found already converted in registry with digest %s", convertedDesc.Digest)
-		return convertedDesc, nil
+		// Even if the image has been found we still need to make sure the requested tag is set
+		// fetch the manifest then push again with the requested tag
+		if err := b.engine.TagPreviouslyConvertedManifest(ctx, convertedDesc); err != nil {
+			logrus.Warnf("failed to tag previously converted manifest: %s. Falling back to regular conversion", err)
+		} else {
+			return convertedDesc, nil
+		}
 	}
 
 	// Errgroups will close the context after wait returns so the operations need their own

cmd/convertor/builder/builder_engine.go

@@ -89,6 +89,9 @@ type Deduplicateable interface {
 	// store manifest digest -> converted manifest to avoid re-conversion
 	CheckForConvertedManifest(ctx context.Context) (specs.Descriptor, error)
 
+	// tag a converted manifest -> converted manifest to avoid re-conversion
+	TagPreviouslyConvertedManifest(ctx context.Context, desc specs.Descriptor) error
+
 	// store manifest digest -> converted manifest to avoid re-conversion
 	StoreConvertedManifestDetails(ctx context.Context) error
 }

cmd/convertor/builder/builder_test.go

@@ -139,5 +139,9 @@ func (e *mockFuzzBuilderEngine) DownloadConvertedLayer(ctx context.Context, idx
 	return nil
 }
 
+func (e *mockFuzzBuilderEngine) TagPreviouslyConvertedManifest(ctx context.Context, desc specs.Descriptor) error {
+	return nil
+}
+
 func (e *mockFuzzBuilderEngine) Cleanup() {
 }

cmd/convertor/builder/builder_utils.go

@@ -234,6 +234,21 @@ func uploadBytes(ctx context.Context, pusher remotes.Pusher, desc specs.Descript
 	return content.Copy(ctx, cw, bytes.NewReader(data), desc.Size, desc.Digest)
 }
 
+func tagPreviouslyConvertedManifest(ctx context.Context, pusher remotes.Pusher, fetcher remotes.Fetcher, desc specs.Descriptor) error {
+	manifest := specs.Manifest{}
+	if err := fetch(ctx, fetcher, desc, &manifest); err != nil {
+		return fmt.Errorf("failed to fetch converted manifest: %w", err)
+	}
+	cbuf, err := json.Marshal(manifest)
+	if err != nil {
+		return err
+	}
+	if err := uploadBytes(ctx, pusher, desc, cbuf); err != nil {
+		return fmt.Errorf("failed to tag converted manifest: %w", err)
+	}
+	return nil
+}
+
 func buildArchiveFromFiles(ctx context.Context, target string, compress compression.Compression, files ...string) error {
 	archive, err := os.Create(target)
 	if err != nil {

cmd/convertor/builder/builder_utils_test.go

@@ -467,3 +467,22 @@ func Test_writeConfig(t *testing.T) {
 		}
 	})
 }
+
+func Test_tagPreviouslyConvertedManifest(t *testing.T) {
+	ctx := context.Background()
+	resolver := testingresources.GetTestResolver(t, ctx)
+	pusher := testingresources.GetTestPusherFromResolver(t, ctx, resolver, "sample.localstore.io/hello-world:anothertag")
+	fetcher := testingresources.GetTestFetcherFromResolver(t, ctx, resolver, testingresources.DockerV2_Manifest_Simple_Converted_Ref)
+
+	_, convertedDesc, err := resolver.Resolve(ctx, testingresources.DockerV2_Manifest_Simple_Converted_Ref) // Simulate a previously converted manifest
+	testingresources.Assert(t, err == nil, "Could not resolve manifest")
+	convertedDesc.Annotations = map[string]string{} // Simulate a manifest that has been converted and is found by digest
+
+	err = tagPreviouslyConvertedManifest(ctx, pusher, fetcher, convertedDesc)
+	testingresources.Assert(t, err == nil, "Could not tag previously converted manifest")
+
+	// Check if the manifest was tagged correctly
+	_, desc, err := resolver.Resolve(ctx, "sample.localstore.io/hello-world:anothertag")
+	testingresources.Assert(t, err == nil, "Could not resolve tagged manifest")
+	testingresources.Assert(t, desc.Digest == convertedDesc.Digest, "Tagged manifest digest does not match original")
+}

cmd/convertor/builder/overlaybd_builder.go

@@ -337,6 +337,11 @@ func (e *overlaybdBuilderEngine) CheckForConvertedManifest(ctx context.Context)
 	return specs.Descriptor{}, errdefs.ErrNotFound
 }
 
+// If a converted manifest has been found we still need to tag it to match the expected output tag.
+func (e *overlaybdBuilderEngine) TagPreviouslyConvertedManifest(ctx context.Context, desc specs.Descriptor) error {
+	return tagPreviouslyConvertedManifest(ctx, e.pusher, e.fetcher, desc)
+}
+
 // mountImage is responsible for mounting a specific manifest from a source repository, this includes
 // mounting all layers + config and then pushing the manifest.
 func (e *overlaybdBuilderEngine) mountImage(ctx context.Context, manifest specs.Manifest, desc specs.Descriptor, mountRepository string) error {

cmd/convertor/builder/turboOCI_builder.go

@@ -213,6 +213,11 @@ func (e *turboOCIBuilderEngine) UploadImage(ctx context.Context) (specs.Descript
 	return e.uploadManifestAndConfig(ctx)
 }
 
+// If a converted manifest has been found we still need to tag it to match the expected output tag.
+func (e *turboOCIBuilderEngine) TagPreviouslyConvertedManifest(ctx context.Context, desc specs.Descriptor) error {
+	return tagPreviouslyConvertedManifest(ctx, e.pusher, e.fetcher, desc)
+}
+
 // Layer deduplication in FastOCI is not currently supported due to conversion not
 // being reproducible at the moment which can lead to occasional bugs.
 

cmd/convertor/resources/samples/mysql-db-manifest-cache-sample-workload.sh

@@ -11,11 +11,20 @@ mysqldbpassword=$8 # mysql password
 oras login $registry -u $username -p $password
 oras cp $sourceImage $registry/$repository:$tag
 # Try one conversion
-./bin/convertor --repository $registry/$repository -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache --db-str "$mysqldbuser:mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
+./bin/convertor --repository $registry/$repository -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache --db-str "$mysqldbuser:$mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
 
 # Retry, result manifest should be cached
-./bin/convertor --repository $registry/$repository -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache-2 --db-str "$mysqldbuser:mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
+./bin/convertor --repository $registry/$repository -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache-2 --db-str "$mysqldbuser:$mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
 
 # Retry, cross repo mount
 oras cp $sourceImage $registry/$repository-2:$tag
-./bin/convertor --repository $registry/$repository -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache-2 --db-str "$mysqldbuser:mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
+./bin/convertor --repository $registry/$repository-2 -u $username:$password --input-tag $tag --oci --overlaybd $tag-obd-cache-3 --db-str "$mysqldbuser:$mysqldbpassword@tcp(127.0.0.1:3306)/conversioncache" --db-type mysql
+
+# Expected output in the registry:
+# <Repository>
+#    -- <Tag>
+#    -- <Tag>-obd-cache
+#    -- <Tag>-obd-cache-2
+# <Repository>-2
+#    -- <Tag>
+#    -- <Tag>-obd-cache-3

cmd/convertor/resources/samples/run-userspace-convertor-ubuntu.Dockerfile

@@ -11,17 +11,17 @@ RUN apt update && \
     apt install -y libcurl4-openssl-dev libext2fs-dev libaio-dev mysql-server
 
 # --- OVERLAYBD TOOLS ---
-FROM base As overlaybd-build
+FROM base AS overlaybd-build
 RUN apt update && \
     apt install -y libgflags-dev libssl-dev libnl-3-dev libnl-genl-3-dev libzstd-dev && \
     apt install -y zlib1g-dev binutils make git wget sudo tar gcc cmake build-essential g++ && \
     apt install -y uuid-dev libjson-c-dev libkmod-dev libsystemd-dev autoconf automake libtool libpci-dev nasm && \
     apt install -y pkg-config
 
 # Download and install Golang version 1.21
-RUN wget https://go.dev/dl/go1.20.12.linux-amd64.tar.gz && \
-    tar -C /usr/local -xzf go1.20.12.linux-amd64.tar.gz && \
-    rm go1.20.12.linux-amd64.tar.gz
+RUN wget https://go.dev/dl/go1.23.2.linux-amd64.tar.gz && \
+    tar -C /usr/local -xzf go1.23.2.linux-amd64.tar.gz && \
+    rm go1.23.2.linux-amd64.tar.gz
 
 # Set environment variables
 ENV PATH="/usr/local/go/bin:${PATH}"
@@ -56,8 +56,18 @@ COPY --from=overlaybd-build /opt/overlaybd/baselayers /opt/overlaybd/baselayers
 COPY --from=overlaybd-build /etc/overlaybd/overlaybd.json /etc/overlaybd/overlaybd.json
 COPY --from=convert-build /home/limiteduser/accelerated-container-image/bin/convertor ./bin/convertor
 
+# EXTRAS
 # Useful resources
 COPY cmd/convertor/resources/samples/mysql.conf ./mysql.conf
 COPY cmd/convertor/resources/samples/mysql-db-setup.sh ./mysql-db-setup.sh
 COPY cmd/convertor/resources/samples/mysql-db-manifest-cache-sample-workload.sh ./mysql-db-manifest-cache-sample-workload.sh
+
+RUN apt update && apt install -y wget
+# Add Oras CLI
+RUN wget "https://github.com/oras-project/oras/releases/download/v1.2.0/oras_1.2.0_linux_amd64.tar.gz" && \
+    mkdir -p oras-install/ && \
+    tar -zxf oras_1.2.0_*.tar.gz -C oras-install/ && \
+    mv oras-install/oras /usr/local/bin/ && \
+    rm -rf oras_1.2.0_*.tar.gz oras-install/
+
 CMD ["./bin/convertor"]

cmd/convertor/testingresources/local_repo.go

@@ -163,18 +163,22 @@ func (r *RepoStore) Exists(ctx context.Context, descriptor v1.Descriptor) (bool,
 // Push pushes a blob to the in memory repository. If the blob already exists, it returns an error.
 // Tag is optional and can be empty.
 func (r *RepoStore) Push(ctx context.Context, desc v1.Descriptor, tag string, content []byte) error {
-	exists, err := r.Exists(ctx, desc)
+	manifestExists, err := r.Exists(ctx, desc)
 	if err != nil {
 		return err
 	}
-	if exists {
-		return nil // No error is returned on push if image is already present
+
+	// Verify content by computing the digest. Real registries are expected to do this.
+	contentDigest := digest.FromBytes(content)
+	if contentDigest != desc.Digest {
+		return fmt.Errorf("content digest %s does not match descriptor digest %s", contentDigest.String(), desc.Digest.String())
 	}
+
 	isManifest := false
 	switch desc.MediaType {
 	case v1.MediaTypeImageManifest, images.MediaTypeDockerSchema2Manifest:
 		isManifest = true
-		if r.opts.ManifestPushIgnoresLayers {
+		if r.opts.ManifestPushIgnoresLayers || manifestExists {
 			break // No layer verification necessary
 		}
 		manifest := v1.Manifest{}
@@ -185,12 +189,12 @@ func (r *RepoStore) Push(ctx context.Context, desc v1.Descriptor, tag string, co
 				return err
 			}
 			if !exists {
-				return fmt.Errorf("Layer %s not found", layer.Digest.String())
+				return fmt.Errorf("layer %s not found", layer.Digest.String())
 			}
 		}
 	case v1.MediaTypeImageIndex, images.MediaTypeDockerSchema2ManifestList:
 		isManifest = true
-		if r.opts.ManifestPushIgnoresLayers {
+		if r.opts.ManifestPushIgnoresLayers || manifestExists {
 			break // No manifest verification necessary
 		}
 		manifestList := v1.Index{}
@@ -201,12 +205,14 @@ func (r *RepoStore) Push(ctx context.Context, desc v1.Descriptor, tag string, co
 				return err
 			}
 			if !exists {
-				return fmt.Errorf("Sub manifest %s not found", subManifestDesc.Digest.String())
+				return fmt.Errorf("sub manifest %s not found", subManifestDesc.Digest.String())
 			}
 		}
 	}
 	r.inmemoryRepo.blobs[desc.Digest.String()] = content
 
+	// We need to always store the manifest digest to tag mapping as the latest pushed manifest
+	// may have a different digest than the previous one.
 	if isManifest && tag != "" {
 		r.inmemoryRepo.tags[tag] = desc.Digest
 	}