From a604ddf906e858a65603d9072194fe96df8ca11f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 22 Jul 2026 13:47:31 -0400 Subject: [PATCH] Add e2e test for ImageStream backup with stale cross-namespace tag reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression coverage for openshift-velero-plugin#443: backing up an ImageStream whose tag references another namespace's repository in the internal registry fails once that source repository's tag is deleted or pruned, because the plugin built the image-copy source path from the stale DockerImageReference instead of the stream's own repository. Fixed upstream by openshift-velero-plugin#444 (merged 2026-07-22). Test flow (tests/e2e/imagestream_stale_reference_suite_test.go): 1. An in-cluster `oc new-build` pushes a real image into a source namespace's ImageStream, landing it in the internal registry under that namespace's own repository (an external `oc tag` import would record the external pull spec instead, and the plugin would skip it entirely). 2. Create the target ImageStream directly via the API with a status tag entry copied from the source stream's resolved digest and DockerImageReference, reproducing the exact "tag references another namespace's repository" condition the bug depends on. This mirrors how openshift-velero-plugin's own restore path recreates ImageStream status (the only way a restored stream's tags are ever populated), and sidesteps `oc tag` entirely — see the iteration history below for why. No spec.tags entry is set: a cross-namespace Kind=ImageStreamImage spec.tags[].from is validated by the apiserver on every create (including the one Velero's restore performs), and fails once the source is gone — a separate, restore-time admission concern this test isn't targeting. The plugin's copy gate only reads status.tags, so spec.tags isn't needed to reproduce or fix the backup-time bug. 3. Delete the source tag and the whole source namespace so the source repository is definitively unavailable. 4. Back up the target namespace: assert it reaches Completed (not PartiallyFailed), and assert the backup logs show the image copy ran from the target stream's own repository. 5. Restore into a fresh namespace and confirm the ImageStream comes back with its tag history intact. Also registers github.com/openshift/api/image/v1 in the shared e2e controller-runtime scheme (tests/e2e/lib/scheme.go). Validated in CI across six revisions of the arrange phase, each diagnosed against openshift-velero-plugin's actual CopyLocalImageStreamImages logic (velero-plugins/imagecopy/imagestream.go): - Against the unfixed plugin, reproduced PartiallyFailed with the plugin reading from the stale source-namespace repository. - `oc tag --source=docker --reference`: the `--reference` flag left the tag event's Image (digest) field empty. The plugin's only hard skip condition is exactly `len(tag.Items[i].Image) == 0` ("no image digest, reference tag") — Kind doesn't gate the copy at all, only whether the copied image also gets a destination :tag label. Copy never attempted; false green. - Plain `oc tag src/x:tag dst/x:tag --reference-policy=local` (no --source=docker): resolved the digest correctly, but on the test cluster the apiserver normalized DockerImageReference to the destination's own repository, so the precondition never held. - `oc tag --source=docker` without `--reference`: resolves the digest and retains the stale path in principle, but depends on the image import controller completing an async import of an internal-registry reference — this never resolved within 5 minutes, identically across all three OCP lanes tested in one run, ruling out a one-off flake. - Constructing the target ImageStream directly via the API sidesteps oc-tag/import-controller behavior and reliably reproduces the backup- time bug; against the fixed plugin, the backup completes with a confirmed copy from the target stream's own repository. An interim revision also set a matching spec.tags[].from (Kind=ImageStreamImage, cross-namespace) for realism, but that entry fails apiserver validation on restore once the source is gone (a separate, restore- time admission concern) — dropped, since it isn't needed for the plugin's copy gate. Signed-off-by: Tiger Kaovilai --- .../imagestream_stale_reference_suite_test.go | 278 ++++++++++++++++++ tests/e2e/lib/scheme.go | 2 + 2 files changed, 280 insertions(+) create mode 100644 tests/e2e/imagestream_stale_reference_suite_test.go diff --git a/tests/e2e/imagestream_stale_reference_suite_test.go b/tests/e2e/imagestream_stale_reference_suite_test.go new file mode 100644 index 00000000000..1bee29bc102 --- /dev/null +++ b/tests/e2e/imagestream_stale_reference_suite_test.go @@ -0,0 +1,278 @@ +package e2e_test + +import ( + "context" + "fmt" + "log" + "os/exec" + "strings" + "time" + + "github.com/google/uuid" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" + imagev1 "github.com/openshift/api/image/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/oadp-operator/tests/e2e/lib" +) + +// ocCommandTimeout bounds runOC calls so a stalled registry import or API +// call fails the test instead of hanging indefinitely. +const ocCommandTimeout = 2 * time.Minute + +// runOC shells out to the oc binary, mirroring the pattern already used by +// lib.RunMustGather. This test needs `oc new-build`/`oc tag`/`oc delete istag`, +// which have no controller-runtime client equivalent that reproduces the same +// server-side ImageStreamTag resolution behavior as the CLI. +func runOC(args ...string) (string, error) { + log.Printf("Running: oc %s", strings.Join(args, " ")) + ctx, cancel := context.WithTimeout(context.Background(), ocCommandTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "oc", args...) + out, err := cmd.CombinedOutput() + if ctx.Err() == context.DeadlineExceeded { + return string(out), fmt.Errorf("command timed out after %v: oc %s", ocCommandTimeout, strings.Join(args, " ")) + } + return string(out), err +} + +// Regression test for https://github.com/openshift/openshift-velero-plugin/issues/443 +// +// ImageStream backup fails with "manifest unknown" when a status.tags[].items[] +// entry references another namespace's repository in the internal registry +// (e.g. because the image was promoted via `oc tag`), and the source +// repository no longer serves that digest (e.g. the source tag was deleted or +// pruned). The openshift-velero-plugin's imagecopy logic builds the copy +// source path directly from that stale DockerImageReference, so the copy +// 404s even though the digest is still fully servable from the ImageStream +// actually being backed up. +var _ = ginkgo.Describe("ImageStream backup with stale cross-namespace tag reference", ginkgo.Ordered, func() { + const ( + sourceNamespace = "imagestream-stale-ref-src" + targetNamespace = "imagestream-stale-ref-dst" + imageStreamName = "imagefoo" + tagName = "v1" + // Base image for the in-cluster build. The build's push (not this + // pull) is what lands a managed image in the source namespace's + // internal-registry repository — an external `oc tag` import would + // record the external pull spec in status and the plugin would skip + // it entirely. Same Red Hat registry precedent as + // sample-applications/parks-app. + buildBaseImage = "registry.access.redhat.com/ubi9/ubi-micro:latest" + ) + + var backupName, restoreName string + + ginkgo.BeforeAll(func() { + log.Print("Creating DPA for imagestream stale-reference test") + dpaSpec := dpaCR.Build(lib.CSI) + err := dpaCR.CreateOrUpdate(dpaSpec) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + log.Print("Checking if DPA is reconciled") + gomega.Eventually(dpaCR.IsReconciledTrue(), time.Minute*3, time.Second*5).Should(gomega.BeTrue()) + + log.Print("Waiting for Velero Pod to be running") + gomega.Eventually(lib.VeleroPodIsRunning(kubernetesClientForSuiteRun, namespace), time.Minute*3, time.Second*5).Should(gomega.BeTrue()) + + log.Print("Checking if BSL is available") + gomega.Eventually(dpaCR.BSLsAreAvailable(), time.Minute*3, time.Second*5).Should(gomega.BeTrue()) + }) + + ginkgo.AfterAll(func() { + log.Print("Cleaning up imagestream stale-reference test") + // The test body deletes both namespaces on the happy path, so + // tolerate NotFound but surface any other deletion failure. + for _, ns := range []string{sourceNamespace, targetNamespace} { + if err := lib.DeleteNamespace(kubernetesClientForSuiteRun, ns); err != nil && !apierrors.IsNotFound(err) { + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to delete namespace %s", ns) + } + } + + log.Print("Deleting DPA") + err := dpaCR.Delete() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + log.Print("Waiting for velero to be deleted") + gomega.Eventually(lib.VeleroIsDeleted(kubernetesClientForSuiteRun, namespace), time.Minute*3, time.Second*5).Should(gomega.BeTrue()) + }) + + ginkgo.It("backs up and restores successfully after the source repository's tag is deleted", func() { + log.Printf("Creating source namespace %s", sourceNamespace) + err := lib.CreateNamespace(kubernetesClientForSuiteRun, sourceNamespace) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Expect(lib.DoesNamespaceExist(kubernetesClientForSuiteRun, sourceNamespace)).Should(gomega.BeTrue()) + + log.Printf("Creating target namespace %s", targetNamespace) + err = lib.CreateNamespace(kubernetesClientForSuiteRun, targetNamespace) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Expect(lib.DoesNamespaceExist(kubernetesClientForSuiteRun, targetNamespace)).Should(gomega.BeTrue()) + + sourceRef := fmt.Sprintf("%s/%s:%s", sourceNamespace, imageStreamName, tagName) + + // An in-cluster build pushes its output to the internal registry, + // creating an ImageStreamMapping whose status item records the + // internal source-namespace repository path — the state this bug + // depends on. An external `oc tag` import would instead record the + // external pull spec verbatim and the plugin would never copy it. + log.Printf("Building %s in-cluster so a managed image is pushed to the internal registry", sourceRef) + out, err := runOC("new-build", "-n", sourceNamespace, + "--name", imageStreamName, + "--to", fmt.Sprintf("%s:%s", imageStreamName, tagName), + "-D", "FROM "+buildBaseImage) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), out) + + log.Print("Waiting for the build to push the image to the source ImageStream") + var sourceItems []imagev1.TagEvent + gomega.Eventually(func() ([]imagev1.TagEvent, error) { + var err error + sourceItems, err = getTagItems(sourceNamespace, imageStreamName, tagName) + return sourceItems, err + }, time.Minute*10, time.Second*10).ShouldNot(gomega.BeEmpty()) + + gomega.Expect(sourceItems[0].DockerImageReference).To( + gomega.ContainSubstring(sourceNamespace+"/"+imageStreamName), + "expected the build push to record an internal-registry reference in the source stream", + ) + + // Construct the target ImageStream's status directly via the API + // rather than promoting through `oc tag`: every `oc tag` construction + // tried here either left the digest unresolved (--reference), got its + // DockerImageReference normalized to the destination's own repository + // by the apiserver (plain ImageStreamTag-kind cross-namespace tag), or + // (--source=docker without --reference) depends on the image import + // controller actually completing an import of an internal-registry + // reference — observed to never resolve within 5 minutes, identically + // across all three OCP lanes tested, so not a flake. Writing + // status.tags directly reproduces exactly the condition the plugin + // depends on (a tag item whose DockerImageReference still points at + // another namespace's repository) without depending on any of that + // CLI/controller behavior — this mirrors how the openshift-velero- + // plugin's own restore path recreates ImageStream status, since that + // is the only way a restored stream's tags are ever populated. + // + // Deliberately no spec.tags entry: a cross-namespace + // Kind=ImageStreamImage spec.tags[].from is validated by the + // apiserver on every create (including the one Velero's restore + // performs), and fails once the source is gone — that's a separate, + // restore-time admission concern this test isn't targeting. The + // plugin's copy gate only reads status.tags, so spec.tags isn't + // needed to reproduce or fix the backup-time bug. + log.Printf("Creating target ImageStream %s/%s with a stale cross-namespace tag reference", targetNamespace, imageStreamName) + targetStream := &imagev1.ImageStream{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: targetNamespace, + Name: imageStreamName, + }, + } + err = dpaCR.Client.Create(context.Background(), targetStream) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + targetStream.Status = imagev1.ImageStreamStatus{ + DockerImageRepository: fmt.Sprintf("image-registry.openshift-image-registry.svc:5000/%s/%s", targetNamespace, imageStreamName), + Tags: []imagev1.NamedTagEventList{ + { + Tag: tagName, + Items: []imagev1.TagEvent{ + { + Created: metav1.Now(), + DockerImageReference: sourceItems[0].DockerImageReference, + Image: sourceItems[0].Image, + }, + }, + }, + }, + } + err = dpaCR.Client.Status().Update(context.Background(), targetStream) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + log.Println("Verifying target tag's dockerImageReference is scoped to the source namespace's repository (stale-reference precondition)") + targetItems, err := getTagItems(targetNamespace, imageStreamName, tagName) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Expect(targetItems).NotTo(gomega.BeEmpty()) + gomega.Expect(targetItems[0].DockerImageReference).To( + gomega.ContainSubstring(sourceNamespace+"/"+imageStreamName), + "test setup invalid: target ImageStream tag should still reference the source namespace's repository at this point", + ) + + log.Printf("Deleting source tag %s (simulates the source being pruned/removed)", sourceRef) + out, err = runOC("delete", "istag", fmt.Sprintf("%s:%s", imageStreamName, tagName), "-n", sourceNamespace) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), out) + + log.Printf("Deleting source namespace %s so its repository is definitively unavailable", sourceNamespace) + err = lib.DeleteNamespace(kubernetesClientForSuiteRun, sourceNamespace) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Eventually( + lib.IsNamespaceDeleted(kubernetesClientForSuiteRun, sourceNamespace), + time.Minute*4, + time.Second*5, + ).Should(gomega.BeTrue()) + + log.Printf("Backing up namespace %s", targetNamespace) + backupUid, _ := uuid.NewUUID() + backupName = fmt.Sprintf("backup-imagestream-stale-ref-%s", backupUid.String()) + err = lib.CreateBackupForNamespaces(dpaCR.Client, namespace, backupName, []string{targetNamespace}, false, false) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Eventually(lib.IsBackupDone(dpaCR.Client, namespace, backupName), time.Minute*10, time.Second*10).Should(gomega.BeTrue()) + + log.Print("Verifying backup completed successfully (not PartiallyFailed due to stale cross-namespace image copy)") + result, err := lib.IsBackupCompletedSuccessfully(kubernetesClientForSuiteRun, dpaCR.Client, namespace, backupName) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(result).To(gomega.BeTrue()) + + // A Completed backup alone is a false green if the imagestream + // image copy never ran (e.g. image backup disabled): assert the + // plugin actually copied, and from the target stream's OWN + // repository — the fixed behavior — rather than the stale + // cross-namespace reference. + log.Print("Verifying the imagestream image copy executed from the stream's own repository") + backupLogs := lib.BackupLogs(kubernetesClientForSuiteRun, dpaCR.Client, namespace, backupName) + gomega.Expect(backupLogs).To(gomega.ContainSubstring("copying from")) + gomega.Expect(backupLogs).To(gomega.ContainSubstring(fmt.Sprintf("%s/%s@", targetNamespace, imageStreamName))) + + log.Printf("Deleting namespace %s to force restore-from-backup", targetNamespace) + err = lib.DeleteNamespace(kubernetesClientForSuiteRun, targetNamespace) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Eventually(lib.IsNamespaceDeleted(kubernetesClientForSuiteRun, targetNamespace), time.Minute*4, time.Second*5).Should(gomega.BeTrue()) + + log.Print("Restoring from backup") + restoreUid, _ := uuid.NewUUID() + restoreName = fmt.Sprintf("restore-imagestream-stale-ref-%s", restoreUid.String()) + err = lib.CreateRestoreFromBackup(dpaCR.Client, namespace, backupName, restoreName) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Eventually(lib.IsRestoreDone(dpaCR.Client, namespace, restoreName), time.Minute*10, time.Second*10).Should(gomega.BeTrue()) + + log.Print("Verifying restore completed successfully") + restoreResult, err := lib.IsRestoreCompletedSuccessfully(kubernetesClientForSuiteRun, dpaCR.Client, namespace, restoreName) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(restoreResult).To(gomega.BeTrue()) + + log.Print("Verifying ImageStream was restored with its tag intact") + gomega.Eventually(func() ([]imagev1.TagEvent, error) { + return getTagItems(targetNamespace, imageStreamName, tagName) + }, time.Minute*2, time.Second*5).ShouldNot(gomega.BeEmpty()) + }) +}) + +// getTagItems returns the TagEvent history for a given ImageStream tag. A +// missing ImageStream or tag returns (nil, nil) so pollers can keep waiting; +// any other client error is returned so callers surface auth/server failures +// instead of masking them as "not found". +func getTagItems(ns, imageStreamName, tag string) ([]imagev1.TagEvent, error) { + imageStream := &imagev1.ImageStream{} + err := dpaCR.Client.Get(context.Background(), client.ObjectKey{Namespace: ns, Name: imageStreamName}, imageStream) + if err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, err + } + for _, namedTagEventList := range imageStream.Status.Tags { + if namedTagEventList.Tag == tag { + return namedTagEventList.Items, nil + } + } + return nil, nil +} diff --git a/tests/e2e/lib/scheme.go b/tests/e2e/lib/scheme.go index ede8e8de6fd..3c48736a48d 100644 --- a/tests/e2e/lib/scheme.go +++ b/tests/e2e/lib/scheme.go @@ -5,6 +5,7 @@ import ( openshiftappsv1 "github.com/openshift/api/apps/v1" openshiftbuildv1 "github.com/openshift/api/build/v1" openshiftconfigv1 "github.com/openshift/api/config/v1" + openshiftimagev1 "github.com/openshift/api/image/v1" openshiftroutev1 "github.com/openshift/api/route/v1" openshiftsecurityv1 "github.com/openshift/api/security/v1" openshifttemplatev1 "github.com/openshift/api/template/v1" @@ -28,6 +29,7 @@ func init() { _ = velerov1.AddToScheme(Scheme) _ = openshiftappsv1.AddToScheme(Scheme) _ = openshiftbuildv1.AddToScheme(Scheme) + _ = openshiftimagev1.AddToScheme(Scheme) _ = openshiftsecurityv1.AddToScheme(Scheme) _ = openshifttemplatev1.AddToScheme(Scheme) _ = openshiftroutev1.AddToScheme(Scheme)