OADP-7384: Copy imagestream images from the stream's own repository#444
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@kaovilai: This pull request references OADP-7384 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
Walkthrough
ChangesImageStream image copying
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/cherry-pick oadp-1.6 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
|
/cherry-pick oadp-1.5 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
This comment was marked as resolved.
This comment was marked as resolved.
6f6efd8 to
77d872a
Compare
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 "<repo>@"
reference. Surfaced by e2e work in openshift/oadp-operator#2313.
Fixes openshift#443
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
77d872a to
7870046
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@velero-plugins/imagecopy/imagestream.go`:
- Around line 91-98: Update the skip log in the tag-copy loop around
tag.Items[i].Image and dockerImageReference so it no longer includes the full
internal image reference. Use only the tag name or a generic sanitized
identifier while preserving the existing reference-tag skip reason.
🪄 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: 48fb23bf-4d7e-47b2-9ed6-2e7aecf66a61
📒 Files selected for processing (2)
velero-plugins/imagecopy/imagestream.govelero-plugins/imagecopy/imagestream_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- velero-plugins/imagecopy/imagestream_test.go
| // 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 | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the full internal image reference.
dockerImageReference may expose an internal registry hostname and repository path in logs. Log only a generic skip reason or a sanitized identifier.
As per coding guidelines, Go files must not log passwords, tokens, PII, internal hostnames, or customer data.
🤖 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 `@velero-plugins/imagecopy/imagestream.go` around lines 91 - 98, Update the
skip log in the tag-copy loop around tag.Items[i].Image and dockerImageReference
so it no longer includes the full internal image reference. Use only the tag
name or a generic sanitized identifier while preserving the existing
reference-tag skip reason.
Source: Coding guidelines
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kaovilai: new pull request created: #451 DetailsIn response to this:
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. |
|
@kaovilai: new pull request created: #452 DetailsIn response to this:
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. |
5 participants
Note
Responses generated with Claude
What this PR does
CopyLocalImageStreamImagesbuilt the copy source path verbatim from the status item'sDockerImageReference. When an image was promoted across namespaces (e.g.oc tag namespace-dev/imagefoo:v1 namespace-int/imagefoo:v1 --reference-policy local), that reference can point at the source namespace's repository. The internal registry scopes manifest reads to repositories whose imagestream still references the digest, so once the source tag is deleted or pruned, backups fail with:The digest is by definition a member of the imagestream being copied (it comes from that stream's
status.tags), so the source path is now built from the stream's own namespace/name:<migrationRegistry>/<is-namespace>/<is-name>) and restore-read become symmetric; previously cross-namespace-tagged images were also unrestorable because restore read the source path derived fromDockerImageReferencewhile backup had written under the stream's own repository.Tests
New unit tests for
CopyLocalImageStreamImages(via acopyImageFnstub seam):Fixes #443
🤖 Generated with Claude Code
Summary by CodeRabbit