From 9d992a4a37640c89d9e79c21054affa0487fdd8c Mon Sep 17 00:00:00 2001 From: Esteban Date: Thu, 24 Oct 2024 01:48:55 +0000 Subject: [PATCH] Fix convertor output tagging on manifest dedup Deduplicated manifests now account for possible adjustments to output tag. Updated tests and test resources to account for this scenario. Signed-off-by: Esteban --- cmd/convertor/builder/builder.go | 8 +++++++- cmd/convertor/builder/builder_engine.go | 3 +++ cmd/convertor/builder/builder_test.go | 4 ++++ cmd/convertor/builder/builder_utils.go | 15 ++++++++++++++ cmd/convertor/builder/builder_utils_test.go | 19 ++++++++++++++++++ cmd/convertor/builder/overlaybd_builder.go | 5 +++++ cmd/convertor/builder/turboOCI_builder.go | 5 +++++ ...mysql-db-manifest-cache-sample-workload.sh | 15 +++++++++++--- .../run-userspace-convertor-ubuntu.Dockerfile | 18 +++++++++++++---- cmd/convertor/testingresources/local_repo.go | 20 ++++++++++++------- 10 files changed, 97 insertions(+), 15 deletions(-) diff --git a/cmd/convertor/builder/builder.go b/cmd/convertor/builder/builder.go index 6d952205..5ed922f7 100644 --- a/cmd/convertor/builder/builder.go +++ b/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 diff --git a/cmd/convertor/builder/builder_engine.go b/cmd/convertor/builder/builder_engine.go index 2ff1548c..d690561c 100644 --- a/cmd/convertor/builder/builder_engine.go +++ b/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 } diff --git a/cmd/convertor/builder/builder_test.go b/cmd/convertor/builder/builder_test.go index 8a1b9317..c1e92c78 100644 --- a/cmd/convertor/builder/builder_test.go +++ b/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() { } diff --git a/cmd/convertor/builder/builder_utils.go b/cmd/convertor/builder/builder_utils.go index a8da659b..6fad2c40 100644 --- a/cmd/convertor/builder/builder_utils.go +++ b/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 { diff --git a/cmd/convertor/builder/builder_utils_test.go b/cmd/convertor/builder/builder_utils_test.go index 020fd2a5..5f5a74f8 100644 --- a/cmd/convertor/builder/builder_utils_test.go +++ b/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") +} diff --git a/cmd/convertor/builder/overlaybd_builder.go b/cmd/convertor/builder/overlaybd_builder.go index e66c8160..b1fb135a 100644 --- a/cmd/convertor/builder/overlaybd_builder.go +++ b/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 { diff --git a/cmd/convertor/builder/turboOCI_builder.go b/cmd/convertor/builder/turboOCI_builder.go index 42d2ad4c..a7a67e92 100644 --- a/cmd/convertor/builder/turboOCI_builder.go +++ b/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. diff --git a/cmd/convertor/resources/samples/mysql-db-manifest-cache-sample-workload.sh b/cmd/convertor/resources/samples/mysql-db-manifest-cache-sample-workload.sh index 50318507..68e6cf9e 100755 --- a/cmd/convertor/resources/samples/mysql-db-manifest-cache-sample-workload.sh +++ b/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: +# +# -- +# -- -obd-cache +# -- -obd-cache-2 +# -2 +# -- +# -- -obd-cache-3 diff --git a/cmd/convertor/resources/samples/run-userspace-convertor-ubuntu.Dockerfile b/cmd/convertor/resources/samples/run-userspace-convertor-ubuntu.Dockerfile index 4fb6f778..a779cc5d 100644 --- a/cmd/convertor/resources/samples/run-userspace-convertor-ubuntu.Dockerfile +++ b/cmd/convertor/resources/samples/run-userspace-convertor-ubuntu.Dockerfile @@ -11,7 +11,7 @@ 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++ && \ @@ -19,9 +19,9 @@ RUN apt update && \ 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"] \ No newline at end of file diff --git a/cmd/convertor/testingresources/local_repo.go b/cmd/convertor/testingresources/local_repo.go index dc10bb3f..58731a6e 100644 --- a/cmd/convertor/testingresources/local_repo.go +++ b/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 }