From 0edccb687b864884645a34d4c807ed817a60503d Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 22 Jul 2026 09:17:40 -0400 Subject: [PATCH] OADP-7384: Copy imagestream images from the stream's own repository CopyLocalImageStreamImages built the copy source path from the status item's DockerImageReference. When an image was promoted across namespaces (e.g. `oc tag namespace-dev/imagefoo:v1 namespace-int/imagefoo:v1`), that reference can point at the source namespace's repository, which stops serving the digest once the source tag is deleted or pruned ("manifest unknown", or a 500 when the source imagestream/namespace is gone entirely), failing the whole imagestream backup item. The digest is by definition a member of the imagestream being copied, so build the source path from the stream's own namespace/name instead. Same-namespace references produce byte-identical paths, and backup (write to migration registry) and restore (read back) become symmetric. Tag events with no image digest (verbatim reference tags from `oc tag --reference`) are skipped with a log line: there is nothing content-addressed to copy, and the imagestreamtag restore path restores reference tags as pointers without needing image data. Building the own-repository path for them would produce a malformed "@" reference. Surfaced by e2e work in openshift/oadp-operator#2313. Fixes #443 Co-Authored-By: Claude Fable 5 Signed-off-by: Tiger Kaovilai --- velero-plugins/imagecopy/imagestream.go | 21 ++- velero-plugins/imagecopy/imagestream_test.go | 179 +++++++++++++++++++ 2 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 velero-plugins/imagecopy/imagestream_test.go diff --git a/velero-plugins/imagecopy/imagestream.go b/velero-plugins/imagecopy/imagestream.go index 3cbec1c2..98ca9106 100644 --- a/velero-plugins/imagecopy/imagestream.go +++ b/velero-plugins/imagecopy/imagestream.go @@ -88,6 +88,14 @@ func CopyLocalImageStreamImages( for i := len(tag.Items) - 1; i >= 0; i-- { dockerImageReference := tag.Items[i].DockerImageReference if len(o.InternalRegistryPath) > 0 && strings.HasPrefix(dockerImageReference, o.InternalRegistryPath) { + // Verbatim reference tags (`oc tag --reference`) have no image digest in + // their tag event, so there is nothing content-addressed to copy; the + // imagestreamtag restore path restores such tags as pointers without + // needing the image data. + if len(tag.Items[i].Image) == 0 { + o.Log.Info(fmt.Sprintf("[imagecopy] skipping copy of %s (tag %s): tag event has no image digest (reference tag)", dockerImageReference, tag.Tag)) + continue + } if len(o.SrcRegistry) == 0 { return errors.New("copy source registry not found but ImageStream has internal images") } @@ -128,7 +136,12 @@ func CopyLocalImageStreamImages( } else { destPath += dockerTransport } - srcPath += fmt.Sprintf("%s%s", srcPathRegistry, strings.TrimPrefix(dockerImageReference, o.InternalRegistryPath)) + // Copy from the imagestream's own repository rather than the repository + // named in DockerImageReference: the digest is by definition a member of + // this stream, while a cross-namespace reference (e.g. from `oc tag`) + // stops being servable once the source tag is deleted or pruned. + // https://github.com/openshift/openshift-velero-plugin/issues/443 + srcPath += fmt.Sprintf("%s/%s/%s@%s", srcPathRegistry, imageStream.Namespace, imageStream.Name, tag.Items[i].Image) destPath += fmt.Sprintf("%s/%s/%s%s", destPathRegistry, o.DestNamespace, imageStream.Name, destTag) // if src or dest registry is empty (ie. when using udistribution), remove extra '/' @@ -138,7 +151,7 @@ func CopyLocalImageStreamImages( o.Log.Info(fmt.Sprintf("[imagecopy] copying from: %s", srcPath)) o.Log.Info(fmt.Sprintf("[imagecopy] copying to: %s", destPath)) - imgManifest, err := copyImage(o.Log, srcPath, destPath, o.CopyOptions) + imgManifest, err := copyImageFn(o.Log, srcPath, destPath, o.CopyOptions) if err != nil { o.Log.Info(fmt.Sprintf("[imagecopy] Error copying image: %v", err)) return err @@ -168,6 +181,10 @@ func CopyLocalImageStreamImages( return nil } +// copyImageFn is a package-level indirection so unit tests can stub out the +// actual registry-to-registry copy. +var copyImageFn = copyImage + func copyImage(log logr.Logger, src, dest string, copyOptions *copy.Options) ([]byte, error) { policyContext, err := getPolicyContext() if err != nil { diff --git a/velero-plugins/imagecopy/imagestream_test.go b/velero-plugins/imagecopy/imagestream_test.go new file mode 100644 index 00000000..7620b862 --- /dev/null +++ b/velero-plugins/imagecopy/imagestream_test.go @@ -0,0 +1,179 @@ +package imagecopy + +import ( + "testing" + + "github.com/containers/image/v5/copy" + "github.com/go-logr/logr" + imagev1API "github.com/openshift/api/image/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const ( + testInternalRegistry = "image-registry.openshift-image-registry.svc:5000" + testMigrationRegistry = "migration-registry.example.com:5000" + testDigest = "sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +) + +type copyCall struct { + src string + dest string +} + +// stubCopyImage replaces copyImageFn for the duration of the test, recording +// the src/dest paths instead of talking to a registry. +func stubCopyImage(t *testing.T) *[]copyCall { + t.Helper() + calls := &[]copyCall{} + orig := copyImageFn + copyImageFn = func(_ logr.Logger, src, dest string, _ *copy.Options) ([]byte, error) { + *calls = append(*calls, copyCall{src: src, dest: dest}) + return []byte(`{"schemaVersion":2}`), nil + } + t.Cleanup(func() { copyImageFn = orig }) + return calls +} + +func newImageStream(namespace, name, dockerImageReference string) imagev1API.ImageStream { + return imagev1API.ImageStream{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Status: imagev1API.ImageStreamStatus{ + Tags: []imagev1API.NamedTagEventList{ + { + Tag: "v1", + Items: []imagev1API.TagEvent{ + { + DockerImageReference: dockerImageReference, + Image: testDigest, + }, + }, + }, + }, + }, + } +} + +func defaultOptions() CopyLocalImageStreamImagesOptions { + return CopyLocalImageStreamImagesOptions{ + InternalRegistryPath: testInternalRegistry, + SrcRegistry: testInternalRegistry, + DestRegistry: testMigrationRegistry, + DestNamespace: "namespace-int", + Log: logr.Discard(), + } +} + +// Images promoted across namespaces (e.g. `oc tag namespace-dev/imagefoo:v1 +// namespace-int/imagefoo:v1`) can leave a status item whose +// DockerImageReference points at the *source* namespace's repository. The +// internal registry only serves a digest from a repository whose imagestream +// still references it, so once the source tag is deleted/pruned that path +// returns "manifest unknown". The digest is always a member of the stream +// being copied, so the copy source must be the stream's own repository. +// https://github.com/openshift/openshift-velero-plugin/issues/443 +func TestCopyLocalImageStreamImagesCrossNamespaceReference(t *testing.T) { + calls := stubCopyImage(t) + + is := newImageStream("namespace-int", "imagefoo", + testInternalRegistry+"/namespace-dev/imagefoo@"+testDigest) + + if err := CopyLocalImageStreamImages(is, defaultOptions()); err != nil { + t.Fatalf("CopyLocalImageStreamImages returned error: %v", err) + } + if len(*calls) != 1 { + t.Fatalf("expected 1 copy call, got %d", len(*calls)) + } + wantSrc := "docker://" + testInternalRegistry + "/namespace-int/imagefoo@" + testDigest + if got := (*calls)[0].src; got != wantSrc { + t.Errorf("src path must use the imagestream's own repository\n got: %s\nwant: %s", got, wantSrc) + } + wantDest := "docker://" + testMigrationRegistry + "/namespace-int/imagefoo:v1" + if got := (*calls)[0].dest; got != wantDest { + t.Errorf("unexpected dest path\n got: %s\nwant: %s", got, wantDest) + } +} + +// Same-namespace references must keep producing the exact same paths as +// before (the common case). +func TestCopyLocalImageStreamImagesSameNamespaceReference(t *testing.T) { + calls := stubCopyImage(t) + + is := newImageStream("namespace-int", "imagefoo", + testInternalRegistry+"/namespace-int/imagefoo@"+testDigest) + + if err := CopyLocalImageStreamImages(is, defaultOptions()); err != nil { + t.Fatalf("CopyLocalImageStreamImages returned error: %v", err) + } + if len(*calls) != 1 { + t.Fatalf("expected 1 copy call, got %d", len(*calls)) + } + wantSrc := "docker://" + testInternalRegistry + "/namespace-int/imagefoo@" + testDigest + if got := (*calls)[0].src; got != wantSrc { + t.Errorf("unexpected src path\n got: %s\nwant: %s", got, wantSrc) + } +} + +// Images not hosted in the internal registry are not local and must not be copied. +func TestCopyLocalImageStreamImagesExternalImageSkipped(t *testing.T) { + calls := stubCopyImage(t) + + is := newImageStream("namespace-int", "imagefoo", + "quay.io/libpod/busybox@"+testDigest) + + if err := CopyLocalImageStreamImages(is, defaultOptions()); err != nil { + t.Fatalf("CopyLocalImageStreamImages returned error: %v", err) + } + if len(*calls) != 0 { + t.Fatalf("expected no copy calls for external image, got %d: %+v", len(*calls), *calls) + } +} + +// Verbatim reference tags (`oc tag --reference`) produce tag events with an +// internal-registry DockerImageReference but no image digest. There is nothing +// content-addressed to copy, so the item must be skipped rather than producing +// a malformed "@" source reference. +func TestCopyLocalImageStreamImagesReferenceTagWithoutDigestSkipped(t *testing.T) { + calls := stubCopyImage(t) + + is := newImageStream("namespace-int", "imagefoo", + testInternalRegistry+"/namespace-dev/imagefoo@"+testDigest) + is.Status.Tags[0].Items[0].Image = "" + + if err := CopyLocalImageStreamImages(is, defaultOptions()); err != nil { + t.Fatalf("CopyLocalImageStreamImages returned error: %v", err) + } + if len(*calls) != 0 { + t.Fatalf("expected no copy calls for digest-less reference tag, got %d: %+v", len(*calls), *calls) + } +} + +// Every history item of a tag is copied, each from the stream's own +// repository under its own digest. +func TestCopyLocalImageStreamImagesTagHistory(t *testing.T) { + calls := stubCopyImage(t) + + const olderDigest = "sha256:fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" + is := newImageStream("namespace-int", "imagefoo", + testInternalRegistry+"/namespace-dev/imagefoo@"+testDigest) + is.Status.Tags[0].Items = append(is.Status.Tags[0].Items, imagev1API.TagEvent{ + DockerImageReference: testInternalRegistry + "/namespace-dev/imagefoo@" + olderDigest, + Image: olderDigest, + }) + + if err := CopyLocalImageStreamImages(is, defaultOptions()); err != nil { + t.Fatalf("CopyLocalImageStreamImages returned error: %v", err) + } + if len(*calls) != 2 { + t.Fatalf("expected 2 copy calls, got %d", len(*calls)) + } + // items are iterated in reverse so the most recent is copied last + wantSrcs := []string{ + "docker://" + testInternalRegistry + "/namespace-int/imagefoo@" + olderDigest, + "docker://" + testInternalRegistry + "/namespace-int/imagefoo@" + testDigest, + } + for i, want := range wantSrcs { + if got := (*calls)[i].src; got != want { + t.Errorf("call %d: src path must use the imagestream's own repository\n got: %s\nwant: %s", i, got, want) + } + } +}