Add e2e test for ImageStream stale cross-namespace tag reference (OVP#443)

[kaovilai]

Note

Responses generated with Claude

What this PR does

Adds an e2e regression test for openshift/openshift-velero-plugin#443: ImageStream backup failed with manifest unknown/500 when a tag was promoted cross-namespace and the source namespace's tag was later deleted/pruned. Fixed upstream by openshift/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 — the build's push (not an oc tag import) is what lands a managed image in the internal registry under that namespace's own repository.
2. Promote into a target namespace with oc tag --source=docker <internal-src-ref> --reference --reference-policy=local — stores the tag event verbatim, skipping the apiserver's cross-namespace localize rewrite, deterministically reproducing the stale cross-namespace reference. A precondition assertion verifies the target's dockerImageReference still points at the source namespace's repository before proceeding.
3. Delete the source tag and then the whole source namespace, so the source repository definitively no longer serves the digest.
4. Back up the target namespace; assert it reaches Completed (not PartiallyFailed), and assert the backup logs show the image copy actually ran from the target stream's own repository (guards against a false green if image backup is disabled, and directly asserts the fixed source-path behavior).
5. Restore into a fresh namespace and assert 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).

Validation

This test was iterated in CI against both the unfixed and fixed plugin before landing on the final form:

• Against the unfixed plugin: reproduced the exact bug — backup went PartiallyFailed, reading the copy source from the stale source-namespace repository (image-registry...svc:5000/imagestream-stale-ref-src/imagefoo@sha256:..., HTTP 500).
• Against the fixed plugin (post OVP#444 merge, after the hourly quay.io/konveyor/openshift-velero-plugin:latest mirror caught up): backup completes, image copy log shows the target stream's own repository, and restore succeeds with tag history intact.

Test plan

• go build ./... and go vet ./... pass for tests/e2e
• CI green against the merged fix (openshift-velero-plugin#444)

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds OpenShift ImageStream scheme registration, bounded oc execution, and an ordered end-to-end regression test for stale cross-namespace ImageStreamTag backup and restore behavior.

Changes

ImageStream stale-reference regression

Layer / File(s)	Summary
ImageStream scheme and test helpers tests/e2e/lib/scheme.go, tests/e2e/imagestream_stale_reference_suite_test.go	Registers ImageStream types, bounds oc commands to two minutes, and provides ImageStreamTag history lookup.
Stale-reference backup and restore flow tests/e2e/imagestream_stale_reference_suite_test.go	Creates and promotes tags, removes the source namespace, validates backup outcomes and logs, and conditionally restores the target namespace.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant E2ESuite
  participant runOC
  participant KubernetesAPI
  participant Velero
  E2ESuite->>runOC: Create and promote ImageStreamTag
  runOC->>KubernetesAPI: Apply ImageStream resources
  E2ESuite->>runOC: Delete source tag and namespace
  E2ESuite->>Velero: Start target namespace backup
  Velero->>KubernetesAPI: Read target ImageStream data
  Velero-->>E2ESuite: Report backup phase and logs
  E2ESuite->>Velero: Restore target namespace when fix is merged
  Velero->>KubernetesAPI: Recreate ImageStream resources
  E2ESuite->>KubernetesAPI: Read restored TagEvent history

Loading

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Test Structure And Quality	⚠️ Warning	The backup-deletion Eventually swallows non-NotFound errors, so RBAC/apiserver failures retry until a generic timeout.	Return the GetBackup error from the poll (or use a (bool,error) Eventually) so non-NotFound failures surface immediately.
Microshift Test Compatibility	⚠️ Warning	The new Ginkgo suite uses build/image APIs via oc new-build, ImageStream, and ImageStreamTag, and it has no [Skipped:MicroShift]/apigroup tag or runtime skip.	Add a MicroShift exclusion (preferred [apigroup:...] or [Skipped:MicroShift], or an exutil.IsMicroShiftCluster() g.Skip()) before running on MicroShift.
Ipv6 And Disconnected Network Test Compatibility	⚠️ Warning	The new e2e test pulls registry.access.redhat.com/ubi9/ubi-micro:latest, requiring public registry access in disconnected IPv6 jobs.	Use a mirrored/internal image (or skip with [Skipped:Disconnected]) and verify in an IPv6 disconnected CI job that no external fetches remain.
No-Sensitive-Data-In-Logs	❓ Inconclusive	Investigating changed test and scheme files for logged sensitive values.	Need code inspection of new logging paths to see whether any secrets/hostnames/PII are emitted.

✅ Passed checks (11 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The only Ginkgo title is a static Describe/It string; no titles embed generated names, dates, UUIDs, namespaces, or other run-specific values.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The new e2e test uses namespaces, builds, backup/restore, and ImageStream checks only; it has no multi-node scheduling or HA assumptions and no SNO skip is needed.
Topology-Aware Scheduling Compatibility	✅ Passed	Only e2e test code and scheme registration changed; no manifests, controllers, replicas, affinity, node selectors, or PDBs were introduced.
Ote Binary Stdout Contract	✅ Passed	No new stdout writes in process-level code; the added suite only uses stdlib log in Ginkgo hooks and scheme.go only registers APIs in init().
No-Weak-Crypto	✅ Passed	Changed files only add ImageStream test logic and scheme registration; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons found.
Container-Privileges	✅ Passed	Only Go e2e test and scheme registration changed; no container/K8s manifests or privileged settings were added.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: an e2e test for stale cross-namespace ImageStream tag references.
Description check	✅ Passed	The description covers why the PR exists and how it was tested, even though it uses different headings than the template.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (1)

tests/e2e/imagestream_stale_reference_suite_test.go (1)

60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add diagnostic messages to the Gomega assertions.

Include the resource and operation in each failure message so CI output identifies which setup, backup, restore, or status check failed.

As per coding guidelines, “Ginkgo test assertions should include meaningful failure messages.”

Also applies to: 87-94, 103-117, 132-158

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/imagestream_stale_reference_suite_test.go` around lines 60 - 70,
Add meaningful diagnostic messages to every Gomega assertion in the setup and
related checks, including the CreateOrUpdate error assertion and each Eventually
assertion in the referenced sections. Identify the specific resource and
operation—DPA reconciliation, Velero pod readiness, BSL availability, backup,
restore, and status checks—so failures clearly indicate what failed.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/imagestream_stale_reference_suite_test.go`:
- Around line 42-43: Update the TODO comment in the stale reference end-to-end
test to reference openshift/openshift-velero-plugin#444 instead of `#443`,
preserving the existing instruction to un-pend the suite once the fix is merged
and vendored.
- Line 44: Add the [apigroup:image.openshift.io] label to the Ginkgo suite
declaration in PDescribe for the stale cross-namespace ImageStream reference
test, preserving its existing Ordered configuration and behavior.
- Around line 50-52: Replace the public registry image assigned to sourceImage
in the stale-reference suite with an image guaranteed to be available through
the CI mirror/internal registry; if no such image exists, mark the suite as
Skipped:Disconnected so disconnected runs do not attempt the pull.
- Around line 156-158: Update the restored-tag verification in the test to use a
bounded gomega.Eventually around getTagItems(targetNamespace, imageStreamName,
tagName), asserting the returned history becomes non-empty before the timeout.
Preserve the existing namespace, ImageStream name, and tag values, and retain
the current failure expectation once the polling deadline is reached.
- Around line 24-28: Update runOC to execute oc through exec.CommandContext with
a bounded timeout and ensure the context is properly cleaned up. In getTagItems,
distinguish Kubernetes NotFound errors from other client failures: preserve the
existing not-found handling, but return non-NotFound errors to the caller so
polling reports authentication or server failures.
- Around line 151-154: Update the Eventually callback in the restore completion
check to return both the success result and the error from
lib.IsRestoreCompletedSuccessfully instead of discarding the error. Preserve the
existing polling intervals and BeTrue assertion so terminal restore failures
propagate immediately.
- Around line 73-82: Update the ginkgo.AfterAll cleanup to assert that
DeleteNamespace succeeds for both sourceNamespace and targetNamespace instead of
discarding their errors, and delete the generated Backup and Restore custom
resources before completing teardown. Preserve the existing DPA deletion and
Velero wait behavior.

In `@tests/e2e/lib/scheme.go`:
- Line 32: Update init in tests/e2e/lib/scheme.go to handle the error returned
by openshiftimagev1.AddToScheme instead of discarding it. Use utilruntime.Must
or an equivalent explicit setup failure while preserving the existing scheme
registration behavior.

---

Nitpick comments:
In `@tests/e2e/imagestream_stale_reference_suite_test.go`:
- Around line 60-70: Add meaningful diagnostic messages to every Gomega
assertion in the setup and related checks, including the CreateOrUpdate error
assertion and each Eventually assertion in the referenced sections. Identify the
specific resource and operation—DPA reconciliation, Velero pod readiness, BSL
availability, backup, restore, and status checks—so failures clearly indicate
what failed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ace42979-3c62-41bc-ab1c-3cdbd32b2930

📥 Commits

Reviewing files that changed from the base of the PR and between f1a19db and a00bdd5.

📒 Files selected for processing (2)

• tests/e2e/imagestream_stale_reference_suite_test.go
• tests/e2e/lib/scheme.go

[kaovilai]

/cherry-pick oadp-1.6

[kaovilai]

/cherry-pick oadp-1.5

[openshift-cherrypick-robot]

@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick oadp-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-cherrypick-robot]

@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.5 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick oadp-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/e2e/imagestream_stale_reference_suite_test.go (1)

71-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add failure messages to Expect(err).NotTo(HaveOccurred()) assertions.

Several assertions in this test omit a diagnostic message (e.g. Lines 75, 99, 107, 112, 145, 180, 191, 196, 211, 218, 223), so a failure only reports a generic error with no indication of which step failed. Lines 128, 158, 176 already follow the better pattern by attaching out.

As per coding guidelines, "Ginkgo test assertions should include meaningful failure messages to help diagnose what went wrong. Bad: Expect(err).NotTo(HaveOccurred()). Good: Expect(err).NotTo(HaveOccurred(), "failed to create test pod")."

🩹 Example fix (repeat pattern at the other unmessaged call sites)

-		err := dpaCR.CreateOrUpdate(dpaSpec)
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+		err := dpaCR.CreateOrUpdate(dpaSpec)
+		gomega.Expect(err).NotTo(gomega.HaveOccurred(), "failed to create/update DPA")

Also applies to: 98-99, 106-113, 144-146, 179-180, 190-191, 195-197, 210-211, 217-224

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/imagestream_stale_reference_suite_test.go` at line 71, Update every
unmessaged Expect(err).NotTo(HaveOccurred()) assertion in the test setup and
execution flows, including the call sites around lines 75, 99, 107, 112, 145,
180, 191, 196, 211, and 218–223, to pass a meaningful step-specific failure
message or the relevant out value. Preserve the existing assertions and follow
the already-message-bearing patterns near lines 128, 158, and 176.

Source: Coding guidelines

♻️ Duplicate comments (2)

tests/e2e/imagestream_stale_reference_suite_test.go (2)

54-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add [apigroup:image.openshift.io] to the suite title.

The suite exercises ImageStream/ImageStreamTag (an image.openshift.io API not available on MicroShift) but the PDescribe title still lacks the [apigroup:...] tag, so MicroShift-aware test selection can't exclude it once un-pended.

As per coding guidelines, "When MicroShift compatibility is needed, use [apigroup:...] tags (preferred), [Skipped:MicroShift] labels, or exutil.IsMicroShiftCluster() guards to skip tests on MicroShift."

🏷️ Proposed fix

-var _ = ginkgo.PDescribe("ImageStream backup with stale cross-namespace tag reference", ginkgo.Ordered, func() {
+var _ = ginkgo.PDescribe("ImageStream backup with stale cross-namespace tag reference [apigroup:image.openshift.io]", ginkgo.Ordered, func() {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/imagestream_stale_reference_suite_test.go` around lines 54 - 67,
Update the Ginkgo suite declaration containing PDescribe for “ImageStream backup
with stale cross-namespace tag reference” to include the
`[apigroup:image.openshift.io]` marker in its title, preserving the existing
Ordered configuration and test body.

Source: Coding guidelines

---

87-102: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Delete the generated Backup/Restore CRs in AfterAll.

Namespace-delete errors are now correctly surfaced (only NotFound is tolerated), but backupName/restoreName are still never cleaned up here, so every run leaves a Backup and Restore CR behind in namespace, accumulating across repeated executions.

log.Print("Deleting generated Backup/Restore CRs")
if backupName != "" {
    // delete the Backup CR named backupName in namespace, tolerating NotFound
}
if restoreName != "" {
    // delete the Restore CR named restoreName in namespace, tolerating NotFound
}

#!/bin/bash
# Check for existing helpers to delete Backup/Restore CRs used elsewhere in the e2e suite.
rg -n 'func Delete(Backup|Restore)\b' tests/e2e/lib
rg -n 'velerov1\.(Backup|Restore){}' tests/e2e -g '*.go' -A3 -B3

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/imagestream_stale_reference_suite_test.go` around lines 87 - 102,
Extend the ginkgo.AfterAll cleanup to delete the generated Backup and Restore
CRs identified by backupName and restoreName in namespace, skipping empty names
and tolerating only NotFound errors. Reuse existing e2e deletion helpers or the
established Velero client patterns, and surface any other deletion failure
through the suite assertions before continuing with DPA cleanup.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tests/e2e/imagestream_stale_reference_suite_test.go`:
- Line 71: Update every unmessaged Expect(err).NotTo(HaveOccurred()) assertion
in the test setup and execution flows, including the call sites around lines 75,
99, 107, 112, 145, 180, 191, 196, 211, and 218–223, to pass a meaningful
step-specific failure message or the relevant out value. Preserve the existing
assertions and follow the already-message-bearing patterns near lines 128, 158,
and 176.

---

Duplicate comments:
In `@tests/e2e/imagestream_stale_reference_suite_test.go`:
- Around line 54-67: Update the Ginkgo suite declaration containing PDescribe
for “ImageStream backup with stale cross-namespace tag reference” to include the
`[apigroup:image.openshift.io]` marker in its title, preserving the existing
Ordered configuration and test body.
- Around line 87-102: Extend the ginkgo.AfterAll cleanup to delete the generated
Backup and Restore CRs identified by backupName and restoreName in namespace,
skipping empty names and tolerating only NotFound errors. Reuse existing e2e
deletion helpers or the established Velero client patterns, and surface any
other deletion failure through the suite assertions before continuing with DPA
cleanup.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2bc3a3e8-21ba-46ba-af3a-52e8412236ab

📥 Commits

Reviewing files that changed from the base of the PR and between 7180d85 and 47403f5.

📒 Files selected for processing (1)

• tests/e2e/imagestream_stale_reference_suite_test.go

[kaovilai]

/test all

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/e2e/imagestream_stale_reference_suite_test.go`:
- Around line 220-229: The backup-deletion poll around lib.GetBackup must
propagate errors other than apierrors.IsNotFound(err) instead of converting them
to false. Update the Eventually callback to return the encountered error
alongside its completion status, while preserving successful completion when the
backup is NotFound.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f3703c13-602a-44c2-b7f8-4eccec979f60

📥 Commits

Reviewing files that changed from the base of the PR and between 6e1452b and 09c0ffe.

📒 Files selected for processing (1)

• tests/e2e/imagestream_stale_reference_suite_test.go

[kaovilai]

/test 4.23-ci-index

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test 4.23-ci-index
/test 5.0-ci-index

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test 4.23-e2e-test-aws
/test 4.22-e2e-test-cli-aws

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test 4.23-ci-index

Note

Responses generated with Claude

[kaovilai]

/test all

Note

Responses generated with Claude

[kaovilai]

/test 4.22-ci-index
/test 4.22-e2e-test-aws
/test 4.23-e2e-test-aws

Note

Responses generated with Claude

[kaovilai]

/test 5.0-e2e-test-cli-aws

Note

Responses generated with Claude

[openshift-ci]

@kaovilai: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, shubham-pampattiwar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• tests/e2e/OWNERS [kaovilai,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment