[release-4.13] OCPBUGS-16372: A variety of changes needed for correct…

… operation with multi… (#661)

* CFE-783: A variety of changes needed for correct operation with multi architecture catalogs (#611)

* A variety of changes needed for correct operation

Functional Changes

pkg/cli/mirror/fbc_operators.go

- remove findFBCConfig & replace with extractDeclarativeConfigFromImage
This is necessary because the layer processing did not handle whiteout
files. Switched to go-containerregistry which handles this correctly.
- modify getManifest so that it is fat manifest aware, and returns the
first manifest in the list, or for single images, returns that manifest
- Removed functions that are no longer necessary:
GetCatalogConfigPath, getConfigPathFromConfigLayer, UntarLayers

pkg/cli/mirror/create.go

- make createOlmArtifactsForOCI manifest list aware and discovers the
first image in the list, or for single images, uses that image directly
- calls replacement function extractDeclarativeConfigFromImage
- stores full path to the artifact in a map so this path does not need
to be recalculated later

pkg/cli/mirror/operator.go

- make use of operatorCatalogToFullArtifactPath map to obtain the full
path to the olm_artifacts directory and its contents and remove
getOperatorCatalogRef() since it is no longer necessary
- make writeLayout() use the image config file to discover the platform
when adding images, otherwise fall back to linux/amd64

pkg/cli/mirror/options.go

- add operatorCatalogToFullArtifactPath map[string]string

pkg/image/image.go

- replace getManifest with getFirstDigestFromPath which now gets the
first digest for manifest lists, or the image digest for single image
- change logging behavior in ParseReference() when path does not exist
and invalidate the ID in that case.

Non-functional Changes

pkg/cli/mirror/mirror.go

- removed redundant AsRespoitory() call since RepositoryName() already
does this
- use constant for olm_artifacts directory

pkg/cli/options.go

- use constant for workspace

all files
- add documentation and change formatting for long function signatures
- if any file used deprecated io/ioutil, replace with io or os package
as necessary
- update unit tests as necessary (the bulk of the files are related to
new test cases)

* reverted test case result

- The original PR had concerns about using latest tag, so did not bring
this change over, therefore this test case had to be reverted too.

* bring over remaining changes

This commit brings over the remaining changes needed for correct
manifest list handling.

internal/testutils/testutils.go

- update and create new functions for creating single/multiarch images
- created *WithURL function variants to allow pre-existing registry in
tests because the defaults server won't allow external calls while
debugging

pkg/cli/mirror/catalog_images.go

- in rebuildCatalogs, make sure that the catalog source docker
reference uses lower case values for "name" and "namespace" which
is required to make valid docker references. This covers the case where
a OCI filepath uses uppercase letters in its path.

pkg/cli/mirror/create.go

- add missing call to strip protocol from ctlg.OCIFBCPath when
constructing a layoutPath

pkg/cli/mirror/mirror.go

- add missing initialization of operatorCatalogToFullArtifactPath

pkg/image/image.go

- in ParseReference, make sure that the docker reference uses lower case
values for "name" and "namespace" which is required to make valid docker
references. This covers the case where a OCI filepath uses uppercase
letters in its path.

pkg/image/builder/image_builder.go

- make image builder handle single arch and multi arch images

pkg/image/builder/image_builder_test.go

- update test cases for image builder

update go modules and vendoring

* fix test case error message

* Change getFirstDigestFromPath based on feedback

- make function return error when number of manifests > 1
- do basic checks to ensure digest is valid before returning
- return image digest instead of config hash
- adjust documentation

* fix test case

* fix uninitialized error

* fix test case after rebase

* Fix OCPBUGS-13198

When copying the OCI layout, make sure we only copy files/folders that
are part of the OCI layout specification.

The bug resulted because the copy destination was inside the
source directory, which lead to an infinite circular copy operation when
using the github.com/otiai10/copy package. This fix avoids this
because it copies only the OCI layout files/folders and ignores others.

* refactor inner function to standalone per feedback

* refactor processImageIndex to be a method

- based on review, make inner function processImageIndex into a method
of ImageBuilder
- add/update documentation

* remove unused getManifest func and test case

* OCPBUGS-13762: make addRelatedImageToMapping multithreaded (#638)

* fix for OCPBUGS-13762

- make execution of addRelatedImageToMapping multi threaded
- make addRelatedImageToMapping take a sync.Map to support concurrency
- convert sync.Map back to image.TypedImageMapping after processing
- fix bug so that we actually use the mirror value if we found one

* add missing checkin for test case

* run make tidy

---------

Co-authored-by: John Hunkins <jhunkins@us.ibm.com>

go.mod

@@ -46,6 +46,7 @@ require golang.org/x/sys v0.4.0 // indirect
 require (
 	github.com/mitchellh/hashstructure/v2 v2.0.2
 	github.com/otiai10/copy v1.2.0
+	golang.org/x/sync v0.1.0
 )
 
 require (
@@ -206,7 +207,6 @@ require (
 	golang.org/x/exp v0.0.0-20221002003631-540bb7301a08 // indirect
 	golang.org/x/net v0.5.0 // indirect
 	golang.org/x/oauth2 v0.0.0-20220622183110-fd043fe589d2 // indirect
-	golang.org/x/sync v0.1.0 // indirect
 	golang.org/x/term v0.4.0 // indirect
 	golang.org/x/text v0.6.0 // indirect
 	golang.org/x/time v0.0.0-20220210224613-90d013bbcef8 // indirect

internal/testutils/testutils.go

@@ -3,28 +3,39 @@ package testutils
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/fs"
-	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"os"
 	"path"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/docker/distribution/manifest"
 	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/google/go-containerregistry/pkg/name"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/layout"
+	"github.com/google/go-containerregistry/pkg/v1/mutate"
+	"github.com/google/go-containerregistry/pkg/v1/random"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
+	"github.com/google/go-containerregistry/pkg/v1/types"
 )
 
 // WriteTestImage will use go-containerregistry to push a test image to
 // an httptest.Server and will write the image to an OCI layout if dir is not "".
 func WriteTestImage(testServer *httptest.Server, dir string) (string, error) {
-	u, err := url.Parse(testServer.URL)
+	return WriteTestImageWithURL(testServer.URL, dir)
+}
+
+// WriteTestImageWithURL is similar to WriteTestImage, but is useful when testing
+// with external registries rather than a httptest.Server
+func WriteTestImageWithURL(URL string, dir string) (string, error) {
+	u, err := url.Parse(URL)
 	if err != nil {
 		return "", err
 	}
@@ -37,24 +48,128 @@ func WriteTestImage(testServer *httptest.Server, dir string) (string, error) {
 		return "", err
 	}
 	i, _ := crane.Image(c)
-	if err := crane.Push(i, tag.String()); err != nil {
-		return "", err
-	}
+
 	if dir != "" {
+		// create a new layout.Path and append the image to it
 		lp, err := layout.Write(dir, empty.Index)
 		if err != nil {
 			return "", err
 		}
+		// write the image into the layout path
 		if err := lp.AppendImage(i); err != nil {
 			return "", err
 		}
+		// reload the path as an index and "push" it to remote
 		idx, err := lp.ImageIndex()
 		if err != nil {
 			return "", err
 		}
 		if err := remote.WriteIndex(tag, idx); err != nil {
 			return "", err
 		}
+	} else {
+		// push image to remote
+		if err := crane.Push(i, tag.String()); err != nil {
+			return "", err
+		}
+	}
+	return targetRef, nil
+}
+
+// WriteMultiarchTestImageWithURL will use go-containerregistry to push a multi arch test image to
+// an httptest.Server and will write the image to an OCI layout if dir is not "".
+func WriteMultiArchTestImage(testServer *httptest.Server, dir string) (string, error) {
+	return WriteMultiArchTestImageWithURL(testServer.URL, dir)
+}
+
+// WriteMultiArchTestImageWithURL is similar to WriteMultiArchTestImage, but is useful when testing
+// with external registries rather than a httptest.Server
+func WriteMultiArchTestImageWithURL(URL string, dir string) (string, error) {
+	u, err := url.Parse(URL)
+	if err != nil {
+		return "", err
+	}
+	targetRef := fmt.Sprintf("%s/bar:foo", u.Host)
+	tag, err := name.NewTag(targetRef)
+	if err != nil {
+		return "", err
+	}
+	makeLayer := func() v1.Layer {
+		layer, _ := random.Layer(100, types.DockerLayer)
+		if err != nil {
+			// this is a hack, but it should not happen
+			panic(err)
+		}
+		return layer
+	}
+	img1, err := mutate.ConfigFile(empty.Image, &v1.ConfigFile{OS: "linux", Architecture: "amd64"})
+	if err != nil {
+		return "", err
+	}
+	img1, err = mutate.Append(img1, mutate.Addendum{
+		Layer: makeLayer(),
+		History: v1.History{
+			Author:    "random.Image",
+			Comment:   fmt.Sprintf("this is a random history %d of %d", 1, 1),
+			CreatedBy: "random",
+			Created:   v1.Time{Time: time.Now()},
+		},
+		// MediaType: types.DockerManifestSchema2,
+	})
+	img2, err := mutate.ConfigFile(empty.Image, &v1.ConfigFile{OS: "linux", Architecture: "ppc64le"})
+	if err != nil {
+		return "", err
+	}
+	img2, err = mutate.Append(img2, mutate.Addendum{
+		Layer: makeLayer(),
+		History: v1.History{
+			Author:    "random.Image",
+			Comment:   fmt.Sprintf("this is a random history %d of %d", 1, 1),
+			CreatedBy: "random",
+			Created:   v1.Time{Time: time.Now()},
+		},
+		// MediaType: types.DockerManifestSchema2,
+	})
+	ii := mutate.AppendManifests(empty.Index,
+		mutate.IndexAddendum{
+			Add: img1,
+			Descriptor: v1.Descriptor{
+				Platform: &v1.Platform{
+					OS:           "linux",
+					Architecture: "amd64",
+				},
+			},
+		},
+		mutate.IndexAddendum{
+			Add: img2,
+			Descriptor: v1.Descriptor{
+				Platform: &v1.Platform{
+					OS:           "linux",
+					Architecture: "ppc64le",
+				},
+			},
+		},
+	)
+	// update the MediaType for this index
+	ii = mutate.IndexMediaType(ii, types.DockerManifestList)
+	if dir != "" {
+		// "wrap" the newly created index in a new index (i.e. create manifest list indirection)
+		// NOTE: manifest list indirection is only useful for OCI layouts... this is
+		// not supported when pushing to a remote registry
+		oci_ii := mutate.AppendManifests(empty.Index, mutate.IndexAddendum{
+			Add: ii,
+			Descriptor: v1.Descriptor{
+				MediaType: types.DockerManifestList,
+			},
+		})
+		_, err := layout.Write(dir, oci_ii)
+		if err != nil {
+			return "", err
+		}
+	}
+	// now "push" to remote
+	if err := remote.WriteIndex(tag, ii); err != nil {
+		return "", err
 	}
 	return targetRef, nil
 }
@@ -74,7 +189,7 @@ func RegistryFromFiles(source string) http.HandlerFunc {
 			case "manifests":
 				if f, err := dir.Open(req.URL.Path); err == nil {
 					defer f.Close()
-					if data, err := ioutil.ReadAll(f); err == nil {
+					if data, err := io.ReadAll(f); err == nil {
 						var versioned manifest.Versioned
 						if err = json.Unmarshal(data, &versioned); err == nil {
 							w.Header().Set("Content-Type", versioned.MediaType)
@@ -110,13 +225,13 @@ func LocalMirrorFromFiles(source string, destination string) error {
 		default:
 			newSource := filepath.Join(source, relPath)
 			cleanSource := filepath.Clean(newSource)
-			data, err := ioutil.ReadFile(cleanSource)
+			data, err := os.ReadFile(cleanSource)
 			if err != nil {
 				return err
 			}
 			newDest := filepath.Join(destination, relPath)
 			cleanDest := filepath.Clean(newDest)
-			return ioutil.WriteFile(cleanDest, data, 0600)
+			return os.WriteFile(cleanDest, data, 0600)
 		}
 		return nil
 	})

pkg/cli/mirror/catalog_images.go

@@ -14,6 +14,7 @@ import (
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/layout"
 	"github.com/openshift/library-go/pkg/image/reference"
+	"github.com/openshift/oc/pkg/cli/image/imagesource"
 	"github.com/operator-framework/operator-registry/pkg/containertools"
 	"github.com/operator-framework/operator-registry/pkg/image/containerdregistry"
 	"k8s.io/klog/v2"
@@ -22,7 +23,6 @@ import (
 	"github.com/openshift/oc-mirror/pkg/config"
 	"github.com/openshift/oc-mirror/pkg/image"
 	"github.com/openshift/oc-mirror/pkg/image/builder"
-	"github.com/openshift/oc/pkg/cli/image/imagesource"
 )
 
 // unpackCatalog will unpack file-based catalogs if they exists
@@ -40,6 +40,22 @@ func (o *MirrorOptions) unpackCatalog(dstDir string, filesInArchive map[string]s
 	return found, nil
 }
 
+/*
+rebuildCatalogs will modify an OCI catalog in <some path>/src/catalogs/<repoPath>/layout with
+the index.json files found in <some path>/src/catalogs/<repoPath>/index/index.json
+
+# Arguments
+
+• ctx: cancellation context
+
+• dstDir: the path to where the config.SourceDir resides
+
+# Returns
+
+• image.TypedImageMapping: the source/destination mapping for the catalog
+
+• error: non-nil if error occurs, nil otherwise
+*/
 func (o *MirrorOptions) rebuildCatalogs(ctx context.Context, dstDir string) (image.TypedImageMapping, error) {
 	refs := image.TypedImageMapping{}
 	var err error
@@ -68,12 +84,20 @@ func (o *MirrorOptions) rebuildCatalogs(ctx context.Context, dstDir string) (ima
 		// Using that path to determine the corresponding catalog image for processing.
 		slashPath := filepath.ToSlash(fpath)
 		if base := path.Base(slashPath); base == "index.json" {
+			// remove the index.json from the path
+			// results in <some path>/src/catalogs/<repoPath>/index
 			slashPath = path.Dir(slashPath)
+			// remove the index folder from the path
+			// results in <some path>/src/catalogs/<repoPath>
 			slashPath = strings.TrimSuffix(slashPath, config.IndexDir)
 
+			// remove the <some path>/src/catalogs from the path to arrive at <repoPath>
 			repoPath := strings.TrimPrefix(slashPath, fmt.Sprintf("%s/%s/", dstDir, config.CatalogsDir))
+			// get the repo namespace and id (where ID is a SHA or tag)
+			// example: foo.com/foo/bar/<id>
 			regRepoNs, id := path.Split(path.Dir(repoPath))
 			regRepoNs = path.Clean(regRepoNs)
+			// reconstitute the path into a valid docker ref
 			var img string
 			if strings.Contains(id, ":") {
 				// Digest.
@@ -85,6 +109,14 @@ func (o *MirrorOptions) rebuildCatalogs(ctx context.Context, dstDir string) (ima
 			ctlgRef := image.TypedImage{}
 			ctlgRef.Type = imagesource.DestinationRegistry
 			sourceRef, err := image.ParseReference(img)
+			// since we can't really tell if the "img" reference originated from an actual docker
+			// reference or from an OCI file path that approximates a docker reference, ParseReference
+			// might not lowercase the name and namespace values which is required by the
+			// docker reference spec (see https://github.com/distribution/distribution/blob/main/reference/reference.go).
+			// Therefore we lower case name and namespace here to make sure it's done.
+			sourceRef.Ref.Name = strings.ToLower(sourceRef.Ref.Name)
+			sourceRef.Ref.Namespace = strings.ToLower(sourceRef.Ref.Namespace)
+
 			if err != nil {
 				return fmt.Errorf("error parsing index dir path %q as image %q: %v", fpath, img, err)
 			}
@@ -107,10 +139,12 @@ func (o *MirrorOptions) rebuildCatalogs(ctx context.Context, dstDir string) (ima
 		return nil, err
 	}
 
+	// update the catalogs in the OCI layout directory and push them to their destination
 	if err := o.processCatalogRefs(ctx, catalogsByImage); err != nil {
 		return nil, err
 	}
 
+	// use the resolver to obtain the digests of the newly pushed images
 	resolver, err := containerdregistry.NewResolver("", o.DestSkipTLS, o.DestPlainHTTP, nil)
 	if err != nil {
 		return nil, fmt.Errorf("error creating image resolver: %v", err)
@@ -129,6 +163,19 @@ func (o *MirrorOptions) rebuildCatalogs(ctx context.Context, dstDir string) (ima
 	return refs, nil
 }
 
+/*
+processCatalogRefs uses the image builder to update a given image using the data provided in catalogRefs.
+
+# Arguments
+
+• ctx: cancellation context
+
+• catalogsByImage: key is catalog destination reference, value is <some path>/src/catalogs/<repoPath>
+
+# Returns
+
+• error: non-nil if error occurs, nil otherwise
+*/
 func (o *MirrorOptions) processCatalogRefs(ctx context.Context, catalogsByImage map[image.TypedImage]string) error {
 	for ctlgRef, artifactDir := range catalogsByImage {
 		// Always build the catalog image with the new declarative config catalog