CLID-690: Automate invalid related image catalog test (OCP-73784)

internal/pkg/archive/archive.go

@@ -66,7 +66,12 @@ func NewMirrorArchive(opts *mirror.CopyOptions, destination, iscPath, workingDir
 // * docker/v2/blobs/sha256 : blobs that haven't been mirrored (diff)
 // * working-dir
 // * image set config
-func (o *MirrorArchive) BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema) error {
+//
+// onBlobsGathered, if non-nil, runs once the image blobs diff is gathered (the last step
+// needing the local registry) and before working-dir - which includes the registry's
+// still-open log file - is archived. This lets callers stop the registry at the right
+// time, avoiding a race between the tar header size and the log file still growing.
+func (o *MirrorArchive) BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema, onBlobsGathered func()) error {
 	if err := o.createTarball(); err != nil {
 		return fmt.Errorf("unable to create the mirror archive: %w", err)
 	}
@@ -79,18 +84,7 @@ func (o *MirrorArchive) BuildArchive(ctx context.Context, schema v2alpha1.Collec
 	if err != nil {
 		return fmt.Errorf("unable to add cache repositories to the archive : %w", err)
 	}
-	// 2- Add working-dir contents to archive
-	err = o.adder.addAllFolder(o.workingDir, filepath.Dir(o.workingDir))
-	if err != nil {
-		return fmt.Errorf("unable to add working-dir to the archive : %w", err)
-	}
-	// 3 - Add imageSetConfig
-	iscName := imageSetConfigPrefix + time.Now().UTC().Format(time.RFC3339)
-	err = o.adder.addFile(o.iscPath, iscName)
-	if err != nil {
-		return fmt.Errorf("unable to add image set configuration to the archive : %w", err)
-	}
-	// 4 - Add blobs
+	// 2 - Add blobs
 	blobsInHistory, err := o.history.Read()
 	if err != nil && !errors.Is(err, &history.EmptyHistoryError{}) {
 		return fmt.Errorf("unable to read history metadata from working-dir : %w", err)
@@ -101,7 +95,24 @@ func (o *MirrorArchive) BuildArchive(ctx context.Context, schema v2alpha1.Collec
 	if err != nil {
 		return fmt.Errorf("unable to add image blobs to the archive : %w", err)
 	}
-	// 5 - update history file with addedBlobs
+
+	// 3 - local registry no longer needed: let the caller stop it before working-dir is added
+	if onBlobsGathered != nil {
+		onBlobsGathered()
+	}
+
+	// 4- Add working-dir contents to archive
+	err = o.adder.addAllFolder(o.workingDir, filepath.Dir(o.workingDir))
+	if err != nil {
+		return fmt.Errorf("unable to add working-dir to the archive : %w", err)
+	}
+	// 5 - Add imageSetConfig
+	iscName := imageSetConfigPrefix + time.Now().UTC().Format(time.RFC3339)
+	err = o.adder.addFile(o.iscPath, iscName)
+	if err != nil {
+		return fmt.Errorf("unable to add image set configuration to the archive : %w", err)
+	}
+	// 6 - update history file with addedBlobs
 	_, err = o.history.Append(addedBlobs)
 	if err != nil {
 		return fmt.Errorf("unable to update history metadata: %w", err)

internal/pkg/archive/archive_test.go

@@ -81,7 +81,7 @@ func TestArchive_BuildArchive(t *testing.T) {
 				Origin:      consts.DockerProtocol + "registry.redhat.io/ubi8/ubi:latest",
 			},
 		}
-		err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images})
+		err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images}, nil)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -104,7 +104,7 @@ func TestArchive_BuildArchive(t *testing.T) {
 				Origin:      consts.DockerProtocol + "registry.redhat.io/ubi8/ubi:latest",
 			},
 		}
-		err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images})
+		err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images}, nil)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -133,7 +133,7 @@ func TestArchive_CacheDirError(t *testing.T) {
 	ma.cacheDir = "none"
 	ma.workingDir = consts.TestFolder + "working-dir-fake"
 
-	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images})
+	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images}, nil)
 	if err == nil {
 		t.Fatal("should fail")
 	}
@@ -158,7 +158,7 @@ func TestArchive_WorkingDirError(t *testing.T) {
 	ma.cacheDir = consts.TestFolder + "cache-fake"
 	ma.workingDir = "none"
 
-	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images})
+	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images}, nil)
 	if err == nil {
 		t.Fatal("should fail")
 	}
@@ -182,7 +182,7 @@ func TestArchive_FileError(t *testing.T) {
 	// force error for addFile
 	ma.iscPath = "none"
 
-	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images})
+	err = ma.BuildArchive(context.Background(), v2alpha1.CollectorSchema{AllImages: images}, nil)
 	if err == nil {
 		t.Fatal("should fail")
 	}

internal/pkg/archive/interface.go

@@ -17,7 +17,9 @@ type BlobsGatherer interface {
 }
 
 type Archiver interface {
-	BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema) error
+	// BuildArchive creates the mirror archive. onBlobsGathered, if non-nil, is called
+	// once the local registry is no longer needed, before working-dir is archived.
+	BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema, onBlobsGathered func()) error
 }
 
 type UnArchiver interface {

internal/pkg/cli/executor.go

@@ -150,6 +150,7 @@ type ExecutorSchema struct {
 	MakeDir              MakeDirInterface
 	Delete               delete.DeleteInterface
 	MirrorStartTimeStamp string
+	registryStopped      bool
 }
 
 type MakeDirInterface interface {
@@ -759,8 +760,15 @@ func (o *ExecutorSchema) startLocalRegistry() {
 	}
 }
 
-// stopLocalRegistry - stops the local registry and closes the registry.log file
+// stopLocalRegistry - stops the local registry and closes the registry.log file.
+// Safe to call multiple times (e.g. from BuildArchive's callback and again in Run):
+// only the first call runs the shutdown logic.
 func (o *ExecutorSchema) stopLocalRegistry(ctx context.Context) {
+	if o.registryStopped {
+		return
+	}
+	o.registryStopped = true
+
 	// Try to gracefully shutdown the local registry
 	ctx, cancel := context.WithTimeout(ctx, time.Second)
 	defer cancel()
@@ -933,7 +941,11 @@ func (o *ExecutorSchema) RunMirrorToDisk(cmd *cobra.Command, args []string) erro
 	}
 
 	o.Log.Info(emoji.Package + " Preparing the tarball archive...")
-	return o.MirrorArchiver.BuildArchive(cmd.Context(), copiedSchema)
+	// The registry is stopped via callback once BuildArchive no longer needs it (see
+	// BuildArchive's doc comment), and before it archives working-dir/logs/registry-*.log.
+	return o.MirrorArchiver.BuildArchive(cmd.Context(), copiedSchema, func() {
+		o.stopLocalRegistry(cmd.Context())
+	})
 }
 
 // RunMirrorToMirror - execute the mirror to mirror functionality

internal/pkg/cli/executor_test.go

@@ -1290,8 +1290,11 @@ func (o *Collector) HelmImageCollector(ctx context.Context) (v2alpha1.CollectorS
 	return v2alpha1.CollectorSchema{}, nil
 }
 
-func (o MockArchiver) BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema) error {
+func (o MockArchiver) BuildArchive(ctx context.Context, schema v2alpha1.CollectorSchema, onBlobsGathered func()) error {
 	// return filepath.Join(o.destination, "mirror_000001.tar"), nil
+	if onBlobsGathered != nil {
+		onBlobsGathered()
+	}
 	return nil
 }
 

tests/integration/image-builders/operator/catalogs/README.md

@@ -144,6 +144,39 @@ opm render ${REPO}:baz-bundle-v1.0.0 ${REPO}:baz-bundle-v1.0.1 ${REPO}:baz-bundl
 ```
 
 
+## test-catalog-invalid-images
+
+Used to test OCPBUGS-33081: a catalog may contain bundles with invalid
+related images (missing name, missing tag/digest, unsupported `oci://`
+scheme, ...). Currently, oc-mirror fails the whole catalog collection when
+any bundle has such an invalid related image, instead of skipping just the
+invalid bundle. `foo.v0.9.9-invalid-related-image`'s bad related image is
+never actually pulled - the string only needs to fail image reference
+parsing, so it doesn't need to point at a real image.
+
+### Contents
+ * Packages: foo
+ * Channels:
+    - foo: beta
+ * Bundles:
+    - foo.v0.1.0: valid, points at the real, already-published `foo-bundle-v0.1.0` image
+    - foo.v0.9.9-invalid-related-image: has a related image with no tag or digest
+      (`registry.example.com/foo/operand-missing-tag`)
+
+### Creating
+```bash
+CATALOG=test-catalog-invalid-images
+mkdir -p ${CATALOG}/foo
+
+opm init foo -c beta -o yaml > ${CATALOG}/foo/operator.yaml
+
+REPO="quay.io/oc-mirror/oc-mirror-dev"
+opm render ${REPO}:foo-bundle-v0.1.0 --output=yaml > ${CATALOG}/foo/bundles.yaml
+# then hand-edit ${CATALOG}/foo/channels.yaml and append the invalid bundle to
+# ${CATALOG}/foo/bundles.yaml - see the checked-in files for the exact content.
+```
+
+
 ## Catalog building
 ```bash
 make build # for all catalogs

tests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/bundles.yaml

@@ -0,0 +1,55 @@
+---
+image: quay.io/oc-mirror/oc-mirror-dev:foo-bundle-v0.1.0
+name: foo.v0.1.0
+package: foo
+properties:
+- type: olm.gvk
+  value:
+    group: test.foo
+    kind: Foo
+    version: v1
+- type: olm.gvk.required
+  value:
+    group: test.bar
+    kind: Bar
+    version: v1alpha1
+- type: olm.package
+  value:
+    packageName: foo
+    version: 0.1.0
+- type: olm.package.required
+  value:
+    packageName: bar
+    versionRange: <0.2.0
+- type: olm.bundle.object
+  value:
+    data: eyJhcGlWZXJzaW9uIjoiYXBpZXh0ZW5zaW9ucy5rOHMuaW8vdjEiLCJraW5kIjoiQ3VzdG9tUmVzb3VyY2VEZWZpbml0aW9uIiwibWV0YWRhdGEiOnsibmFtZSI6ImZvb3MudGVzdC5mb28ifSwic3BlYyI6eyJncm91cCI6InRlc3QuZm9vIiwibmFtZXMiOnsia2luZCI6IkZvbyIsInBsdXJhbCI6ImZvb3MifSwic2NvcGUiOiJOYW1lc3BhY2VkIiwidmVyc2lvbnMiOlt7Im5hbWUiOiJ2MSIsInNjaGVtYSI6eyJvcGVuQVBJVjNTY2hlbWEiOnsidHlwZSI6Im9iamVjdCIsIngta3ViZXJuZXRlcy1wcmVzZXJ2ZS11bmtub3duLWZpZWxkcyI6dHJ1ZX19LCJzZXJ2ZWQiOnRydWUsInN0b3JhZ2UiOnRydWV9XX19
+- type: olm.bundle.object
+  value:
+    data: eyJhcGlWZXJzaW9uIjoib3BlcmF0b3JzLmNvcmVvcy5jb20vdjFhbHBoYTEiLCJraW5kIjoiQ2x1c3RlclNlcnZpY2VWZXJzaW9uIiwibWV0YWRhdGEiOnsiYW5ub3RhdGlvbnMiOnsiY2FwYWJpbGl0aWVzIjoiQmFzaWNJbnN0YWxsIiwiY2VydGlmaWVkIjoiZmFsc2UiLCJjb250YWluZXJJbWFnZSI6InF1YXkuaW8vb2MtbWlycm9yL29jLW1pcnJvci1kZXZAc2hhMjU2OjFjZThjMDE4N2M4ZmU2YjRiZTMyN2RjODQ4YjhiYWYwNjJjZTFiYWE1MDk2YjRmNWQ5NTU4OTNkMTI2ZDViNTgiLCJkZXNjcmlwdGlvbiI6IlRoZSBGb28gT3BlcmF0b3IgZG9lcyBGb28gdGhpbmdzLiIsIm9sbS5za2lwUmFuZ2UiOiJcdTAwM2MwLjEuMCIsInJlcG9zaXRvcnkiOiJodHRwczovL2dpdGh1Yi5jb20vb3BlbnNoaWZ0L29jLW1pcnJvciJ9LCJuYW1lIjoiZm9vLnYwLjEuMCJ9LCJzcGVjIjp7ImN1c3RvbXJlc291cmNlZGVmaW5pdGlvbnMiOnsib3duZWQiOlt7Imdyb3VwIjoidGVzdC5mb28iLCJraW5kIjoiRm9vIiwibmFtZSI6ImZvb3MudGVzdC5mb28iLCJ2ZXJzaW9uIjoidjEifV19LCJkZXNjcmlwdGlvbiI6IlRoZSBGb28gT3BlcmF0b3IgZG9lcyBGb28gdGhpbmdzLiIsImRpc3BsYXlOYW1lIjoiRm9vIE9wZXJhdG9yIiwiaWNvbiI6W3siYmFzZTY0ZGF0YSI6IlBEOTRiV3dnZG1WeWMybHZiajBpTVM0d0lpQmxibU52WkdsdVp6MGlkWFJtTFRnaVB6NEtQSE4yWnlCMlpYSnphVzl1UFNJeExqRWlJR2xrUFNKcFkyOXVJaUI0Yld4dWN6MGlhSFIwY0RvdkwzZDNkeTUzTXk1dmNtY3ZNakF3TUM5emRtY2lJSGh0Ykc1ek9uaHNhVzVyUFNKb2RIUndPaTh2ZDNkM0xuY3pMbTl5Wnk4eE9UazVMM2hzYVc1cklpQjRQU0l3Y0hnaUlIazlJakJ3ZUNJS0lDQWdJSFpwWlhkQ2IzZzlJakFnTUNBeE1qZ3dJREV5T0RBaUlITjBlV3hsUFNKbGJtRmliR1V0WW1GamEyZHliM1Z1WkRwdVpYY2dNQ0F3SURFeU9EQWdNVEk0TURzaUlIaHRiRHB6Y0dGalpUMGljSEpsYzJWeWRtVWlQZ284YzNSNWJHVWdkSGx3WlQwaWRHVjRkQzlqYzNNaVBnb2dJQzV6Y1hWcGNtTnNaU0I3SUdacGJHdzZJQ00zUWpnM09UUTdJSFJ5WVc1elptOXliVG9nZEhKaGJuTnNZWFJsS0RWd2VDd2dNWEI0S1RzZ2ZRb2dJQzV1WVcxbElIc2dabTl1ZERvZ1ltOXNaQ0EwTURCd2VDQnpZVzV6SUhObGNtbG1PeUJtYVd4c09pQnlaV1E3SUhSeVlXNXpabTl5YlRvZ2RISmhibk5zWVhSbEtERXpNSEI0TENBMU1EQndlQ2tnY205MFlYUmxLREl3WkdWbktUc2dmUW84TDNOMGVXeGxQZ284Wno0S0lDQThjR0YwYUNCamJHRnpjejBpYzNGMWFYSmpiR1VpSUdROUlrMGdNQ3dnTlRBd0NpQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQkRJREFzSURVZ05Td2dNQ0ExTURBc0lEQUtJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJRk1nTVRBd01Dd2dOU0F4TURBd0xDQTFNREFLSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ01UQXdNQ3dnTVRBd01DQTFNREFzSURFd01EQUtJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnSUNBZ0lDQWdJQ0FnTUN3Z01UQXdNQ0F3TENBMU1EQWlMejRLSUNBOGRHVjRkQ0JqYkdGemN6MGlibUZ0WlNJK1ptOXZQQzkwWlhoMFBnbzhMMmMrQ2p3dmMzWm5QZ289IiwibWVkaWF0eXBlIjoiaW1hZ2Uvc3ZnK3htbCJ9XSwiaW5zdGFsbCI6eyJzcGVjIjp7ImRlcGxveW1lbnRzIjpbeyJuYW1lIjoiZm9vLW9wZXJhdG9yIiwic3BlYyI6eyJyZXBsaWNhcyI6MSwic2VsZWN0b3IiOnsibWF0Y2hMYWJlbHMiOnsiYXBwIjoiZm9vLW9wZXJhdG9yIn19LCJ0ZW1wbGF0ZSI6eyJtZXRhZGF0YSI6eyJsYWJlbHMiOnsiYXBwIjoiZm9vLW9wZXJhdG9yIiwidmVyc2lvbiI6InYwLjEuMCJ9LCJuYW1lIjoiZm9vLW9wZXJhdG9yIn0sInNwZWMiOnsiY29udGFpbmVycyI6W3siY29tbWFuZCI6WyIvcnVuLnNoIl0sImltYWdlIjoicXVheS5pby9vYy1taXJyb3Ivb2MtbWlycm9yLWRldkBzaGEyNTY6MWNlOGMwMTg3YzhmZTZiNGJlMzI3ZGM4NDhiOGJhZjA2MmNlMWJhYTUwOTZiNGY1ZDk1NTg5M2QxMjZkNWI1OCIsImltYWdlUHVsbFBvbGljeSI6IkFsd2F5cyIsIm5hbWUiOiJmb28ifV19fX19XX0sInN0cmF0ZWd5IjoiZGVwbG95bWVudCJ9LCJpbnN0YWxsTW9kZXMiOlt7InN1cHBvcnRlZCI6dHJ1ZSwidHlwZSI6Ik93bk5hbWVzcGFjZSJ9LHsic3VwcG9ydGVkIjp0cnVlLCJ0eXBlIjoiU2luZ2xlTmFtZXNwYWNlIn0seyJzdXBwb3J0ZWQiOmZhbHNlLCJ0eXBlIjoiTXVsdGlOYW1lc3BhY2UifSx7InN1cHBvcnRlZCI6dHJ1ZSwidHlwZSI6IkFsbE5hbWVzcGFjZXMifV0sImtleXdvcmRzIjpbImZvbyJdLCJsYWJlbHMiOnsibmFtZSI6ImZvby1vcGVyYXRvciJ9LCJtYWludGFpbmVycyI6W3siZW1haWwiOiJvYy1taXJyb3JAb3BlbnNoaWZ0Lm9yZyIsIm5hbWUiOiJvYy1taXJyb3IgZGV2ZWxvcGVycyJ9XSwibWF0dXJpdHkiOiJiZXRhIiwicHJvdmlkZXIiOnsibmFtZSI6IkZvbyJ9LCJyZWxhdGVkSW1hZ2VzIjpbeyJpbWFnZSI6InF1YXkuaW8vb2MtbWlycm9yL29jLW1pcnJvci1kZXZAc2hhMjU2OjFjZThjMDE4N2M4ZmU2YjRiZTMyN2RjODQ4YjhiYWYwNjJjZTFiYWE1MDk2YjRmNWQ5NTU4OTNkMTI2ZDViNTgiLCJuYW1lIjoib3BlcmF0b3IifV0sInZlcnNpb24iOiIwLjEuMCJ9fQ==
+relatedImages:
+- image: quay.io/oc-mirror/oc-mirror-dev:foo-bundle-v0.1.0
+  name: ""
+- image: quay.io/oc-mirror/oc-mirror-dev@sha256:1ce8c0187c8fe6b4be327dc848b8baf062ce1baa5096b4f5d955893d126d5b58
+  name: operator
+schema: olm.bundle
+---
+image: quay.io/oc-mirror/oc-mirror-dev:foo-bundle-v0.1.0
+name: foo.v0.9.9-invalid-related-image
+package: foo
+properties:
+- type: olm.gvk
+  value:
+    group: test.foo
+    kind: Foo
+    version: v1
+- type: olm.package
+  value:
+    packageName: foo
+    version: 0.9.9
+relatedImages:
+- image: quay.io/oc-mirror/oc-mirror-dev:foo-bundle-v0.1.0
+  name: ""
+- image: registry.example.com/foo/operand-missing-tag
+  name: operand
+schema: olm.bundle

tests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/channels.yaml

@@ -0,0 +1,7 @@
+schema: olm.channel
+package: foo
+name: beta
+entries:
+  - name: foo.v0.1.0
+  - name: foo.v0.9.9-invalid-related-image
+    replaces: foo.v0.1.0

tests/integration/image-builders/operator/catalogs/test-catalog-invalid-images/foo/operator.yaml

@@ -0,0 +1,3 @@
+defaultChannel: beta
+name: foo
+schema: olm.package

tests/integration/operators_test.go

@@ -117,6 +117,22 @@ var _ = Describe("operators", func() {
 			expectRebuiltTagMatchesDigest(ctx, *testRegistry, filepath.Join(iscDir, iscFile))
 		})
 	})
+
+	// OCPBUGS-33081: a catalog may contain bundles with invalid related images (missing
+	// name, missing tag/digest, unsupported oci:// scheme, ...).
+	Describe("catalog with a bundle containing an invalid related image", func() {
+		iscFile := filepath.Join("operators", "isc-operator-invalid-images.yaml")
+
+		It("handles invalid related image references in a catalog", func() {
+			By("running mirrorToMirror against a catalog with one valid and one invalid bundle")
+			result, err := runner.MirrorToMirror(ctx, filepath.Join(iscDir, iscFile), workDir, testRegistry.Endpoint(),
+				"--dest-tls-verify=false")
+			expectOcMirrorExitCode(result, err, 4, "collection error", "tag and digest are empty")
+
+			By("verifying no content was mirrored, even though one of the two bundles was valid")
+			expectNoRepositoriesInRegistry(*testRegistry)
+		})
+	})
 })
 
 // expectCatalogContainsOnlyExpectedPackages verifies that the rebuilt catalog in the registry

tests/integration/testdata/imagesetconfigs/operators/isc-operator-invalid-images.yaml

@@ -0,0 +1,8 @@
+# ImageSetConfig to test mirroring a catalog that has a bundle with an invalid
+# related image (OCPBUGS-33081)
+kind: ImageSetConfiguration
+apiVersion: mirror.openshift.io/v2alpha1
+mirror:
+  operators:
+    - catalog: quay.io/oc-mirror/oc-mirror-dev:test-catalog-invalid-images
+      full: true