OCPBUGS-65845: fix stale image digest race condition#504
OCPBUGS-65845: fix stale image digest race condition#504joepvd wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@joepvd: This pull request references Jira Issue OCPBUGS-65845, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThese changes modify the default image pull policy from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joepvd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/build/builder/daemonless_test.go (1)
18-75: Consider calling the actual implementation rather than duplicating the logic in the test.The test implements the reconstruction logic inline (lines 62-69) instead of invoking
DaemonlessClient.PullImage. This validates the expected behavior but doesn't verify the actual implementation matches.♻️ Suggested approach to test the actual implementation
func TestPullImageNameReconstruction(t *testing.T) { + // Track the image name passed to pullDaemonlessImage + var capturedImageName string + // Note: This would require either: + // 1. Mocking pullDaemonlessImage (which is difficult without interfaces) + // 2. Or keeping the current approach as a specification test + tests := []struct { name string repo string tag string want string }{ // ... test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := docker.PullImageOptions{ Repository: tt.repo, Tag: tt.tag, } - imageName := opts.Repository - if opts.Tag != "" { - if strings.Contains(opts.Tag, ":") { - imageName = imageName + "@" + opts.Tag - } else { - imageName = imageName + ":" + opts.Tag - } - } + // Ideally call DaemonlessClient.PullImage and capture the imageName + // For now, the inline logic serves as a specification testGiven the testing constraints (mocking
pullDaemonlessImagewould require significant refactoring), the current approach is acceptable as a specification test that documents expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/build/builder/daemonless_test.go` around lines 18 - 75, The test TestPullImageNameReconstruction duplicates the image-name reconstruction logic instead of exercising the real implementation; update the test to call the actual code path (e.g., invoke DaemonlessClient.PullImage or the internal helper used by pullDaemonlessImage) so the behavior is validated against the implementation — either refactor the reconstruction into an exported helper (e.g., ReconstructImageName or similar) and test that helper, or construct a DaemonlessClient with minimal/mocked dependencies and call DaemonlessClient.PullImage for each case to assert the produced image reference matches the expected value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/build/builder/daemonless_test.go`:
- Around line 18-75: The test TestPullImageNameReconstruction duplicates the
image-name reconstruction logic instead of exercising the real implementation;
update the test to call the actual code path (e.g., invoke
DaemonlessClient.PullImage or the internal helper used by pullDaemonlessImage)
so the behavior is validated against the implementation — either refactor the
reconstruction into an exported helper (e.g., ReconstructImageName or similar)
and test that helper, or construct a DaemonlessClient with minimal/mocked
dependencies and call DaemonlessClient.PullImage for each case to assert the
produced image reference matches the expected value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd6c074d-971a-4bd9-8b0a-7a9fd0291356
📒 Files selected for processing (4)
pkg/build/builder/daemonless.gopkg/build/builder/daemonless_test.gopkg/build/builder/docker_test.gopkg/build/builder/source.go
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@joepvd: This pull request references Jira Issue OCPBUGS-65845, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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-robot
removed
the
jira/invalid-bug
Change the default pull policy from PullIfMissing to PullIfNewer in buildDaemonlessImage and extractSourceFromImage. With PullIfMissing, if an image was already pulled (e.g. during a pre-pull step), a newer version pushed to the registry would not be detected. PullIfNewer adds a lightweight registry manifest check that catches cases where a tag was updated between Build object creation and the actual pull. For digest-pinned references this is a no-op. Also fix DaemonlessClient.PullImage to use '@' instead of ':' when the tag field contains a digest (e.g. "sha256:..."), preventing incorrect image name reconstruction.
joepvd
force-pushed
the
fix/OCPBUGS-65845-stale-image-digest
branch
from
b1e7909 to
254d97c
Compare
|
@joepvd: This pull request references Jira Issue OCPBUGS-65845, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@joepvd: The following test failed, say
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. |
2 participants
Summary
Fixes OCPBUGS-65845 — a race condition where the builder uses an older image digest instead of the latest one.
PullIfMissingtoPullIfNewerin bothbuildDaemonlessImageandextractSourceFromImage. WithPullIfMissing, if an image was already pulled (e.g. during a pre-pull step), a newer version pushed to the registry would not be detected.PullIfNeweradds a lightweight registry manifest check that catches cases where a tag was updated between Build object creation and the actual pull. For digest-pinned references this is a no-op (a specific digest cannot have a "newer" version).DaemonlessClient.PullImagedigest reference reconstruction. When the tag field contains a digest (e.g.sha256:...), use@instead of:as the separator, preventing malformed image names likeimage:sha256:abcinstead of the correctimage@sha256:abc.DockerBuilder.pullImagecorrectly passes digest references to the docker client.Test plan
TestPullImageNameReconstruction— validates image name reconstruction logic handles tags, digests-as-tags, digests-in-repository, and no-tag casesTestPullImageDigestReference— validatesDockerBuilder.pullImagecorrectly passes digest and tag references through to the docker clientTestCheckRemoteGitfailure due to network access)Summary by CodeRabbit
Improvements
Tests