Make artifact pinned status respect pinned_images configuration#9836
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds artifact pinning: artifacts gain a pinned flag, the Store stores atomic pinned-image regexps, artifact construction is routed through Store to evaluate pin state, and pinned regexps are propagated at startup and on config reloads; tests and mocks updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Server as CRI-O Server
participant ImageSvc as ImageServer
participant ArtifactStore as OCI Artifact Store
participant LibArtifact as libartifact
Client->>Server: start / init
Server->>ImageSvc: PinnedImageRegexps()
ImageSvc-->>Server: []*regexp.Regexp
Server->>ArtifactStore: NewStore(..., pinnedImageRegexps)
ArtifactStore->>ArtifactStore: SetPinnedImageRegexps(regexps)
Client->>Server: Pull/List/Status artifact
Server->>ArtifactStore: Pull/List/Status
ArtifactStore->>LibArtifact: fetch metadata
LibArtifact-->>ArtifactStore: *libartifact.Artifact
ArtifactStore->>ArtifactStore: newArtifact(art, rootPath, pinned)
ArtifactStore->>ArtifactStore: isArtifactPinned(artifact)
ArtifactStore-->>Server: *Artifact (with Pinned)
Server->>Client: CRI image/status (pinned state)
Client->>Server: reload config (SIGHUP)
Server->>ImageSvc: UpdatePinnedImagesList(...)
ImageSvc-->>Server: PinnedImageRegexps()
Server->>ArtifactStore: SetPinnedImageRegexps(updated)
ArtifactStore-->>Server: regexps updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/storage/image.go`:
- Around line 1106-1108: The field regexForPinnedImages is read without
synchronization by PinnedImageRegexps and FilterPinnedImage while being written
by UpdatePinnedImagesList, causing a data race; change the storage to use an
atomic.Value (or add a sync.RWMutex) and update UpdatePinnedImagesList to store
the new []*regexp.Regexp into that atomic holder, then change PinnedImageRegexps
(and any callers like FilterPinnedImage and buildImageResult) to load the value
from the atomic.Value (or use the RWMutex RLock/RUnlock) and return a
safely-typed copy; ensure the accessor returns the concrete []*regexp.Regexp
after asserting the loaded atomic value and avoid returning the unsynchronized
field directly.
In `@server/server.go`:
- Around line 636-637: The config reload goroutine updates
svc.regexForPinnedImages without synchronization while
ListImages/buildImageResult/FilterPinnedImage read it concurrently; protect
regexForPinnedImages by introducing synchronization (either add a mutex
alongside imageCacheLock or replace regexForPinnedImages with an atomic.Value)
and update UpdatePinnedImagesList, PinnedImageRegexps, and any readers
(FilterPinnedImage/buildImageResult/ListImages) to use that synchronization
mechanism so all reads/writes are safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6f82b0e-899d-4933-ade3-c6b012890bc1
📒 Files selected for processing (8)
internal/ociartifact/artifact.gointernal/ociartifact/datastore/store.gointernal/ociartifact/store.gointernal/ociartifact/store_test.gointernal/storage/image.goserver/server.gotest/mocks/criostorage/criostorage.gotest/oci_artifacts.bats
| s.ContainerServer.StorageImageServer().UpdatePinnedImagesList(append(s.config.PinnedImages, s.config.PauseImage)) | ||
| s.artifactStore.SetPinnedImageRegexps(s.ContainerServer.StorageImageServer().PinnedImageRegexps()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the race condition exists by checking synchronization in imageService
echo "=== Checking imageService struct for sync primitives on regexForPinnedImages ==="
rg -n -A5 'type imageService struct' internal/storage/image.go
echo ""
echo "=== Checking UpdatePinnedImagesList for any locking ==="
rg -n -A10 'func.*UpdatePinnedImagesList' internal/storage/image.go
echo ""
echo "=== Checking PinnedImageRegexps for any locking ==="
rg -n -A5 'func.*PinnedImageRegexps' internal/storage/image.go
echo ""
echo "=== Checking reads of regexForPinnedImages ==="
rg -n 'regexForPinnedImages' internal/storage/image.goRepository: cri-o/cri-o
Length of output: 1552
🏁 Script executed:
# Check context around lines 636-637 in server/server.go
sed -n '630,645p' server/server.goRepository: cri-o/cri-o
Length of output: 779
🏁 Script executed:
# Check ListImages and ImageStatus implementations to see if they access regexForPinnedImages
rg -n 'func.*ListImages|func.*ImageStatus' internal/storage/image.go -A 5Repository: cri-o/cri-o
Length of output: 748
🏁 Script executed:
# Check how UpdatePinnedImagesList is called - look for config reload context
rg -n 'UpdatePinnedImagesList' --type goRepository: cri-o/cri-o
Length of output: 1331
🏁 Script executed:
# Check if config reload operations run in separate goroutines
rg -n 'go\s+func|goroutine' server/server.go | head -20Repository: cri-o/cri-o
Length of output: 196
🏁 Script executed:
# Get broader context around config reload to see if it's in a goroutine
sed -n '610,660p' server/server.goRepository: cri-o/cri-o
Length of output: 1559
🏁 Script executed:
# Find buildImageResult to confirm it reads regexForPinnedImages
rg -n 'func.*buildImageResult' internal/storage/image.go -A 15Repository: cri-o/cri-o
Length of output: 629
🏁 Script executed:
# Check if ListImages calls buildImageResult
rg -n -A 20 'func (svc \*imageService) ListImages' internal/storage/image.goRepository: cri-o/cri-o
Length of output: 37
🏁 Script executed:
# Get the full buildImageResult function
sed -n '320,380p' internal/storage/image.goRepository: cri-o/cri-o
Length of output: 1784
🏁 Script executed:
# Get the full ListImages function implementation
sed -n '403,450p' internal/storage/image.goRepository: cri-o/cri-o
Length of output: 1235
🏁 Script executed:
# Search for where FilterPinnedImage is called in the codebase
rg -n 'FilterPinnedImage' --type go -B 2 -A 2Repository: cri-o/cri-o
Length of output: 772
Data race between config reload and concurrent image operations.
The config reload handler (started as a goroutine in startReloadWatcher) writes to regexForPinnedImages via UpdatePinnedImagesList without synchronization, while concurrent ListImages calls read the same field via buildImageResult → FilterPinnedImage. The subsequent PinnedImageRegexps() call also reads the field unsynchronized.
From internal/storage/image.go:
UpdatePinnedImagesList(line 1102-1104) directly assigns tosvc.regexForPinnedImagesbuildImageResult(line 355) readssvc.regexForPinnedImagesviaFilterPinnedImageListImages(line 403+) can execute concurrently from different request handlers
The presence of imageCacheLock in imageService demonstrates concurrency awareness but fails to protect regexForPinnedImages. Add mutex protection or use atomic.Value for regexForPinnedImages to prevent the race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/server.go` around lines 636 - 637, The config reload goroutine updates
svc.regexForPinnedImages without synchronization while
ListImages/buildImageResult/FilterPinnedImage read it concurrently; protect
regexForPinnedImages by introducing synchronization (either add a mutex
alongside imageCacheLock or replace regexForPinnedImages with an atomic.Value)
and update UpdatePinnedImagesList, PinnedImageRegexps, and any readers
(FilterPinnedImage/buildImageResult/ListImages) to use that synchronization
mechanism so all reads/writes are safe.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9836 +/- ##
==========================================
+ Coverage 67.56% 67.64% +0.08%
==========================================
Files 212 212
Lines 29327 29366 +39
==========================================
+ Hits 19816 19866 +50
+ Misses 7817 7806 -11
Partials 1694 1694 🚀 New features to boost your workflow:
|
|
/retest |
1 similar comment
|
/retest |
| // ImageServer compiles the list with regex for both | ||
| // pinned and sandbox/pause images, we need to update them | ||
| s.ContainerServer.StorageImageServer().UpdatePinnedImagesList(append(s.config.PinnedImages, s.config.PauseImage)) | ||
| s.artifactStore.SetPinnedImageRegexps(s.ContainerServer.StorageImageServer().PinnedImageRegexps()) |
There was a problem hiding this comment.
Pre-existing issue: regexForPinnedImages in imageService has no synchronization, unlike the artifact store's atomic.Value. Worth a follow-up fix.
There was a problem hiding this comment.
yup I'll do it in another follow-up PR.
| // isArtifactPinned checks if the artifact's reference or canonical name | ||
| // matches any of the configured pinned image patterns. | ||
| func (s *Store) isArtifactPinned(artifact *Artifact) bool { | ||
| regexps, ok := s.pinnedImageRegexps.Load().([]*regexp.Regexp) | ||
| if !ok { | ||
| // This shouldn't happen. | ||
| return false | ||
| } | ||
|
|
||
| for _, re := range regexps { | ||
| if re.MatchString(artifact.Reference()) || re.MatchString(artifact.CanonicalName()) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Consider reusing storage.FilterPinnedImage to avoid duplicating the matching logic. Something like:
return storage.FilterPinnedImage(artifact.Reference(), regexps) ||
storage.FilterPinnedImage(artifact.CanonicalName(), regexps)There was a problem hiding this comment.
That completely makes sense and I wanted to do, but I couldn't due to cyclic dependency.
cri-o/internal/storage/image.go
Line 40 in f8be11c
I'm thinking changing how storage package calls artifact so that we can reuse the function and also artifacts can get storageID, but for now, I'd like to keep it as is, to narrower the scope of this PR to only artifacts change. Added a TODO comment.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, saschagrunert 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 |
Previously, all OCI artifacts were hardcoded as Pinned: true in CRIImage(). This change makes artifacts respect the same pinned_images configuration used by regular container images, by having the artifact store check artifact names against the compiled pinned image regexps. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/storage/image.go (1)
1108-1110:⚠️ Potential issue | 🟠 Major
PinnedImageRegexpsstill exposes the reload race.This returns the backing slice directly while
UpdatePinnedImagesListswaps it andbuildImageResultreads it during list/status operations. That leaves pin evaluation nondeterministic under config reloads. Keep the compiled regexps behind anatomic.Value/sync.RWMutex, and have readers load a synchronized copy instead of returning the raw slice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/image.go` around lines 1108 - 1110, PinnedImageRegexps currently returns the backing slice svc.regexForPinnedImages directly, exposing a race when UpdatePinnedImagesList swaps it and buildImageResult reads it; change the implementation to store the compiled regex slice behind a synchronization primitive (e.g., an atomic.Value or a sync.RWMutex) and have PinnedImageRegexps load the synchronized value and return a safe copy (not the raw slice). Update UpdatePinnedImagesList to store the new slice via the same atomic/lock and ensure buildImageResult reads the value through that synchronized accessor so readers never access the raw backing slice concurrently.
🧹 Nitpick comments (1)
internal/ociartifact/artifact.go (1)
37-58: Pass caller context intonewArtifact.The function currently logs parse warnings with
context.Background(), which strips request-scoped fields and tracing. All call sites inPull,List, andStatusalready havectxin scope—thread it through this helper and use it for logging instead.func (s *Store) newArtifact(ctx context.Context, art *libartifact.Artifact, rootPath string, pinned bool) *Artifact { // ... existing logic ... if art.Name != "" { namedRef, err := reference.ParseNormalizedNamed(art.Name) if err != nil { log.Warnf(ctx, "Failed to parse artifact name %s with the error %s", art.Name, err) // ... } } // ... }Aligns with the guideline: Propagate context.Context through function calls in Go code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ociartifact/artifact.go` around lines 37 - 58, The newArtifact helper uses context.Background() when logging parse warnings which loses request-scoped fields and tracing; change the signature of Store.newArtifact to accept ctx context.Context (func (s *Store) newArtifact(ctx context.Context, art *libartifact.Artifact, rootPath string, pinned bool) *Artifact), update all callers in Pull, List, and Status to pass their existing ctx, and replace log.Warnf(context.Background(), ...) with log.Warnf(ctx, ...) while preserving the existing fallback to unknownRef and the pinned logic (retain artifact.pinned = pinned || s.isArtifactPinned(artifact)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/storage/image.go`:
- Around line 1108-1110: PinnedImageRegexps currently returns the backing slice
svc.regexForPinnedImages directly, exposing a race when UpdatePinnedImagesList
swaps it and buildImageResult reads it; change the implementation to store the
compiled regex slice behind a synchronization primitive (e.g., an atomic.Value
or a sync.RWMutex) and have PinnedImageRegexps load the synchronized value and
return a safe copy (not the raw slice). Update UpdatePinnedImagesList to store
the new slice via the same atomic/lock and ensure buildImageResult reads the
value through that synchronized accessor so readers never access the raw backing
slice concurrently.
---
Nitpick comments:
In `@internal/ociartifact/artifact.go`:
- Around line 37-58: The newArtifact helper uses context.Background() when
logging parse warnings which loses request-scoped fields and tracing; change the
signature of Store.newArtifact to accept ctx context.Context (func (s *Store)
newArtifact(ctx context.Context, art *libartifact.Artifact, rootPath string,
pinned bool) *Artifact), update all callers in Pull, List, and Status to pass
their existing ctx, and replace log.Warnf(context.Background(), ...) with
log.Warnf(ctx, ...) while preserving the existing fallback to unknownRef and the
pinned logic (retain artifact.pinned = pinned || s.isArtifactPinned(artifact)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14ed759c-28a5-4243-93b2-243ac320cee8
📒 Files selected for processing (8)
internal/ociartifact/artifact.gointernal/ociartifact/datastore/store.gointernal/ociartifact/store.gointernal/ociartifact/store_test.gointernal/storage/image.goserver/server.gotest/mocks/criostorage/criostorage.gotest/oci_artifacts.bats
✅ Files skipped from review due to trivial changes (1)
- test/oci_artifacts.bats
🚧 Files skipped from review as they are similar to previous changes (3)
- server/server.go
- test/mocks/criostorage/criostorage.go
- internal/ociartifact/store_test.go
|
sorry. In the last time I changed to use atomic.Value to atomic.Pointer which doesn't require type assertion. |
|
/lgtm |
|
/cherry-pick release-1.35 |
|
@bitoku: 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/oci_artifacts.bats (1)
293-293: Use the same high-priority*.confnaming in the reload test.
01-pin-artifactis the only pinning snippet here without a.confsuffix and with lower precedence than the other overrides. Renaming it to99-...confmakes this test deterministic instead of depending on config-dir merge details.Suggested change
- printf '[crio.image]\npinned_images = ["%s"]\n' "$ARTIFACT_IMAGE" > "$CRIO_CONFIG_DIR"/01-pin-artifact + printf '[crio.image]\npinned_images = ["%s"]\n' "$ARTIFACT_IMAGE" > "$CRIO_CONFIG_DIR"/99-pin-artifact.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/oci_artifacts.bats` at line 293, The pinning config file created in test/oci_artifacts.bats uses the low-precedence name "01-pin-artifact" (created via the printf call writing to "$CRIO_CONFIG_DIR"/01-pin-artifact); change that to a high-priority name with a .conf suffix such as "99-pin-artifact.conf" so it sorts after other overrides and makes the reload test deterministic—update the filename in the printf destination to "$CRIO_CONFIG_DIR"/99-pin-artifact.conf and keep the same content produced by printf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ociartifact/store.go`:
- Around line 380-385: SetPinnedImageRegexps currently stores a pointer to a
local slice (®exps) into s.pinnedImageRegexps which leads to a dangling
pointer; replace the atomic.Pointer[[]*regexp.Regexp] with an atomic.Value and
store the slice value itself (not its address), cloning the incoming slice
(e.g., slices.Clone) and replacing nil with an empty slice inside
SetPinnedImageRegexps; update isArtifactPinned to Load() the value from
s.pinnedImageRegexps and type-assert to []*regexp.Regexp before iterating (check
nil/empty) so no pointer to a stack variable is kept and concurrent loads/stores
remain safe.
---
Nitpick comments:
In `@test/oci_artifacts.bats`:
- Line 293: The pinning config file created in test/oci_artifacts.bats uses the
low-precedence name "01-pin-artifact" (created via the printf call writing to
"$CRIO_CONFIG_DIR"/01-pin-artifact); change that to a high-priority name with a
.conf suffix such as "99-pin-artifact.conf" so it sorts after other overrides
and makes the reload test deterministic—update the filename in the printf
destination to "$CRIO_CONFIG_DIR"/99-pin-artifact.conf and keep the same content
produced by printf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47f74e73-ca2a-4588-94da-98dd9b8cb0fd
📒 Files selected for processing (8)
internal/ociartifact/artifact.gointernal/ociartifact/datastore/store.gointernal/ociartifact/store.gointernal/ociartifact/store_test.gointernal/storage/image.goserver/server.gotest/mocks/criostorage/criostorage.gotest/oci_artifacts.bats
🚧 Files skipped from review as they are similar to previous changes (4)
- server/server.go
- test/mocks/criostorage/criostorage.go
- internal/storage/image.go
- internal/ociartifact/store_test.go
| func (s *Store) SetPinnedImageRegexps(regexps []*regexp.Regexp) { | ||
| if regexps == nil { | ||
| regexps = []*regexp.Regexp{} | ||
| } | ||
|
|
||
| s.pinnedImageRegexps.Store(®exps) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 --type=go 'func \(s \*Store\) SetPinnedImageRegexps|pinnedImageRegexps\.Store|SetPinnedImageRegexps\('Repository: cri-o/cri-o
Length of output: 4145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check imports at the top of store.go
head -30 internal/ociartifact/store.go
# Check if slices package is used anywhere
rg -n 'import.*slices|slices\.Clone' internal/ociartifact/store.go
# Get the isArtifactPinned implementation
sed -n '388,420p' internal/ociartifact/store.goRepository: cri-o/cri-o
Length of output: 907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the isArtifactPinned function
rg -n -A 15 'func \(s \*Store\) isArtifactPinned' internal/ociartifact/store.goRepository: cri-o/cri-o
Length of output: 465
Critical use-after-free bug: do not store a pointer to a local variable with atomic.Pointer.
SetPinnedImageRegexps stores ®exps where regexps is a local variable. After the function returns, the stack frame is freed and the stored pointer becomes dangling. When isArtifactPinned dereferences *regexps (line 397), it accesses freed memory.
The suggested fix of cloning the slice does not solve this: even a cloned slice is still a local variable, so ®expsClone is still a dangling pointer.
Replace atomic.Pointer[[]*regexp.Regexp] with atomic.Value and store the slice directly:
func (s *Store) SetPinnedImageRegexps(regexps []*regexp.Regexp) {
if regexps == nil {
regexps = []*regexp.Regexp{}
} else {
regexps = slices.Clone(regexps)
}
s.pinnedImageRegexps.Store(regexps)
}
func (s *Store) isArtifactPinned(artifact *Artifact) bool {
regexps := s.pinnedImageRegexps.Load().([]*regexp.Regexp)
for _, re := range regexps {
if re.MatchString(artifact.Reference()) || re.MatchString(artifact.CanonicalName()) {
return true
}
}
return false
}Alternatively, use sync.RWMutex if you need to avoid type assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ociartifact/store.go` around lines 380 - 385, SetPinnedImageRegexps
currently stores a pointer to a local slice (®exps) into s.pinnedImageRegexps
which leads to a dangling pointer; replace the atomic.Pointer[[]*regexp.Regexp]
with an atomic.Value and store the slice value itself (not its address), cloning
the incoming slice (e.g., slices.Clone) and replacing nil with an empty slice
inside SetPinnedImageRegexps; update isArtifactPinned to Load() the value from
s.pinnedImageRegexps and type-assert to []*regexp.Regexp before iterating (check
nil/empty) so no pointer to a stack variable is kept and concurrent loads/stores
remain safe.
|
@bitoku: new pull request created: #9838 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. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Previously, all OCI artifacts were hardcoded as Pinned: true in CRIImage(). This change makes artifacts respect the same pinned_images configuration used by regular container images, by having the artifact store check artifact names against the compiled pinned image regexps.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
New Features
Tests