CNTRLPLANE-3314: add unified image resolution package#8468
Draft
bryan-cox wants to merge 5 commits intoopenshift:mainfrom
Draft
CNTRLPLANE-3314: add unified image resolution package#8468bryan-cox wants to merge 5 commits intoopenshift:mainfrom
bryan-cox wants to merge 5 commits intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
Skipping CI for Draft Pull Request. |
|
@bryan-cox: This pull request references CNTRLPLANE-3314 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a new image resolution subsystem under Sequence Diagram(s)sequenceDiagram
participant Client
participant ReleaseProvider as releaseInfoProvider
participant Cache as releaseCache
participant Resolver as imageResolver
participant ConfigSource as configSource
participant ReleaseFetcher as releaseFetcher
Client->>ReleaseProvider: Lookup(pullSpec)
ReleaseProvider->>Cache: get(pullSpec)
alt cache miss
Cache-->>ReleaseProvider: nil
ReleaseProvider->>Resolver: resolveForDirectFetch(pullSpec)
Resolver->>ConfigSource: current()
ConfigSource-->>Resolver: ResolverConfig
Resolver-->>ReleaseProvider: resolvedSpec
ReleaseProvider->>ReleaseFetcher: fetch(resolvedSpec, pullSecret)
ReleaseFetcher-->>ReleaseProvider: ReleaseImage
ReleaseProvider->>Resolver: resolveForPodSpec(componentImage)
Resolver-->>ReleaseProvider: podSpecImage
ReleaseProvider->>Cache: put(pullSpec, ReleaseImage)
Cache-->>ReleaseProvider: stored
else cache hit
Cache-->>ReleaseProvider: ReleaseImage
end
ReleaseProvider-->>Client: ReleaseImage
sequenceDiagram
participant Client
participant Resolver as imageResolver
participant ConfigSource as configSource
participant MirrorCache as mirrorAvailabilityCache
participant MirrorChecker as mirrorAvailabilityChecker
Client->>Resolver: resolveForDirectFetch(ref)
Resolver->>ConfigSource: current()
ConfigSource-->>Resolver: ResolverConfig
Resolver->>Resolver: apply CLI registry overrides
Resolver->>MirrorCache: get(mirror)
alt cache miss
MirrorCache-->>Resolver: miss
Resolver->>MirrorChecker: isAvailable(mirror)
MirrorChecker-->>Resolver: available/unavailable
Resolver->>MirrorCache: set(mirror, available)
else cache hit
MirrorCache-->>Resolver: available/unavailable
end
alt mirror available
Resolver-->>Client: resolvedRef (mirror applied)
else no mirror available
Resolver-->>Client: resolvedRef (original or CLI-overridden)
end
ChangesImage Resolution Core
Adapters Exposing Prior Interfaces
Unit Tests
Test Utilities
Operator & Service Integrations
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@support/imageresolution/cache.go`:
- Around line 32-39: The get methods currently treat expired entries as misses
but leave stale keys in the map; update releaseCache.get (and similarly
mirrorAvailabilityCache.get and metadataCache.get) to remove expired entries
when observed: when a read finds entry missing or expired, upgrade to a write
lock (or unlock the read lock and take the write lock), check expiry again, and
delete c.entries[key] before returning nil so the map doesn't grow unbounded;
ensure you reference c.entries, c.ttl, entry.timestamp and maintain proper
locking (mu.RLock -> mu.RUnlock -> mu.Lock -> mu.Unlock) to avoid races.
In `@support/imageresolution/component.go`:
- Around line 30-35: componentProvider's accessors ComponentVersions() and
ComponentImages() currently return the backing maps directly; change both to
return defensive copies instead: allocate new maps, copy all key/value pairs
from p.release.ComponentVersions and p.release.ComponentImages into them (handle
nil by returning an empty map or nil consistently), and return those copies so
callers cannot mutate the provider's ReleaseImage state.
In `@support/imageresolution/config_source.go`:
- Around line 13-18: The staticConfigSource currently stores and returns
s.config by value but that is a shallow copy — update newStaticConfigSource to
deep-clone the incoming ResolverConfig (including reference-typed fields like
RegistryOverrides) before storing it on the staticConfigSource, and change
staticConfigSource.current to return a deep-cloned ResolverConfig copy (not the
stored instance) so callers cannot mutate internal maps/slices; implement a
robust clone helper used by both newStaticConfigSource and current, and add a
regression test that mutates a returned config and asserts subsequent current()
calls are unchanged.
In `@support/imageresolution/config.go`:
- Around line 53-64: ParseRegistryOverridesFlag currently silently skips
malformed tokens; change it to validate each token and fail fast by returning an
error instead of dropping invalid entries: update ParseRegistryOverridesFlag to
return (map[string]string, error), check for tokens missing '=' or with empty
key or value and return a descriptive error on first invalid token; apply the
same change to ParseImageRegistryMirrorsEnvVar so both functions validate
"key=value" pairs, return errors on malformed input, and update any callers to
handle the new (map, error) signature.
In `@support/imageresolution/provider_set.go`:
- Around line 47-53: WithRegistryOverrides and WithImageRegistryMirrors
currently store caller-provided maps/slices by reference (fields
registryOverrides and mirrors), so callers can mutate them later; defensively
clone the inputs before assigning to the builder fields and when returning built
objects. Implement small helpers (e.g., cloneStringMap and cloneStringSliceMap)
that copy map[string]string and map[string][]string (deep-copying slices), use
them in ProviderSetBuilder.WithRegistryOverrides,
ProviderSetBuilder.WithImageRegistryMirrors and the other similar setters
referenced in the review (the methods around lines 64-66 and 94-97) so the
builder holds independent copies to avoid shared-state/race hazards.
- Around line 58-62: The builder currently records dynamic mirror settings in
ProviderSetBuilder.WithDynamicImageRegistryMirrors (setting hasDynamicMirrors
and dynamicInterval) but Build() always wires the static source; change Build
(and its callers) to fail fast when dynamic mirrors are requested: detect
b.hasDynamicMirrors inside ProviderSetBuilder.Build (or the function that
constructs the ProviderSet) and return an explicit error like "dynamic image
registry mirrors not implemented" (or propagate an error) instead of silently
ignoring the flag; ensure the error includes the symbol names
(ProviderSetBuilder, WithDynamicImageRegistryMirrors, hasDynamicMirrors,
dynamicInterval) so callers know the feature is unimplemented and can be updated
later.
In `@support/imageresolution/registry_client.go`:
- Around line 18-26: In isAvailable on httpMirrorChecker: stop ignoring the
passed context, stop creating a per-call http.Client with TLS
InsecureSkipVerify, and perform a context-aware HEAD request; specifically,
remove TLS InsecureSkipVerify from the Transport, use
http.NewRequestWithContext(ctx, "HEAD", "https://"+registry+"/v2/", nil) (or
client.Head with context if available), and reuse a shared http.Client instance
on httpMirrorChecker instead of allocating one per call so the request respects
cancellation/timeouts and TLS verification remains enabled.
In `@support/imageresolution/release.go`:
- Around line 37-62: The cached ReleaseImage objects (from p.cache.get /
p.cache.put) are being returned and mutated in place (e.g.,
release.ComponentImages updated after resolveForPodSpec and maps.Copy with
p.imageOverrides), corrupting shared cache state; instead, make and use a deep
copy of the ReleaseImage (including a new map copy of ComponentImages) before
applying resolver.resolveForPodSpec results and image overrides, store that copy
with p.cache.put and return that copy, and when p.cache.get returns a value
return a defensive copy rather than the original cached pointer so callers
cannot mutate the cached object.
In `@support/imageresolution/resolver.go`:
- Around line 51-56: The applyCLIOverrides function currently iterates the
overrides map nondeterministically and may match mid-string substrings; fix it
by collecting the keys of the overrides map, sort them (e.g., by length or
lexicographically) to enforce deterministic iteration, then iterate the sorted
keys and perform strict prefix checks (use ref == src or strings.HasPrefix(ref,
src+"/")) instead of strings.Contains before doing the single replacement;
update applyCLIOverrides to use the sorted keys and prefix-safe match so
mirrors/overrides are applied deterministically and only when the src is a true
prefix.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0edb3e94-cf36-405a-9f5e-9006198cb503
📒 Files selected for processing (18)
support/imageresolution/cache.gosupport/imageresolution/cache_test.gosupport/imageresolution/component.gosupport/imageresolution/component_test.gosupport/imageresolution/config.gosupport/imageresolution/config_source.gosupport/imageresolution/config_source_test.gosupport/imageresolution/config_test.gosupport/imageresolution/metadata.gosupport/imageresolution/metadata_test.gosupport/imageresolution/provider_set.gosupport/imageresolution/provider_set_test.gosupport/imageresolution/registry_client.gosupport/imageresolution/release.gosupport/imageresolution/release_test.gosupport/imageresolution/resolver.gosupport/imageresolution/resolver_test.gosupport/imageresolution/testutil.go
openshift-ci
Bot
added
the
area/karpenter-operator
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/main.go (1)
450-478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep management-cluster registry overrides out of the data-plane ProviderSet.
imageRegistryOverridesis mutated here to includeregistryOverrides, and then that same map is passed todpProviderSet. So the data-plane resolver still inherits the management-cluster overrides through the mirror map, despite the comment saying it should not.Suggested direction
- if len(registryOverrides) > 0 { - if imageRegistryOverrides == nil { - imageRegistryOverrides = map[string][]string{} - } - for registry, override := range registryOverrides { - if _, exists := imageRegistryOverrides[registry]; !exists { - imageRegistryOverrides[registry] = []string{} - } - imageRegistryOverrides[registry] = append(imageRegistryOverrides[registry], override) - } - } + cpImageRegistryOverrides := cloneStringSliceMap(imageRegistryOverrides) + if len(registryOverrides) > 0 { + if cpImageRegistryOverrides == nil { + cpImageRegistryOverrides = map[string][]string{} + } + for registry, override := range registryOverrides { + cpImageRegistryOverrides[registry] = append(cpImageRegistryOverrides[registry], override) + } + } cpProviderSet, err := imageresolution.NewProviderSet(). WithRegistryOverrides(registryOverrides). - WithImageRegistryMirrors(imageRegistryOverrides). + WithImageRegistryMirrors(cpImageRegistryOverrides). WithImageOverrides(componentImages). Build() dpProviderSet, err := imageresolution.NewProviderSet(). WithImageRegistryMirrors(imageRegistryOverrides). Build()🤖 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 `@control-plane-operator/main.go` around lines 450 - 478, The code mutates imageRegistryOverrides by merging registryOverrides into it, then passes that mutated map to dpProviderSet which causes management-cluster registry overrides to leak into the data-plane; to fix, avoid mutating the original imageRegistryOverrides used for data-plane by creating a separate map for the control-plane merge (e.g. copy imageRegistryOverrides to a new local map, merge registryOverrides into that map) and pass that merged map into cpProviderSet.WithImageRegistryMirrors while passing the unmodified imageRegistryOverrides (or nil if originally nil) to dpProviderSet.WithImageRegistryMirrors; update the code around registryOverrides/imageRegistryOverrides and the calls that build cpProviderSet and dpProviderSet (imageresolution.NewProviderSet().WithImageRegistryMirrors(...)) accordingly.
♻️ Duplicate comments (5)
support/imageresolution/provider_set.go (2)
72-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClone caller-provided maps before storing or wiring them.
The builder keeps direct references to the input maps/slices and then reuses them in the live
ProviderSet. Mutating an input afterBuild()will change resolution behavior in-place and can race withReconcile()/lookup paths.Also applies to: 94-96, 124-165
🤖 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 `@support/imageresolution/provider_set.go` around lines 72 - 79, The builder currently stores and later exposes direct references to caller-provided maps/slices (e.g., in WithRegistryOverrides and WithImageRegistryMirrors), which allows external mutation to affect the built ProviderSet; clone all input maps and any nested slices before storing them in the builder and again when wiring into the live ProviderSet (apply the same cloning pattern referenced around lines 94-96 and 124-165). Specifically, in ProviderSetBuilder.WithRegistryOverrides(copy the overrides map into a new map) and ProviderSetBuilder.WithImageRegistryMirrors(copy the mirrors map and for each key copy the []string slice, set hasStaticMirrors accordingly), and ensure Build() or the code that transfers builder fields into ProviderSet also performs defensive copies so the runtime ProviderSet never holds references to caller-owned collections.
83-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently accept
WithDynamicImageRegistryMirrors()whileBuild()ignores it.This setter records
hasDynamicMirrorsanddynamicInterval, butBuild()never uses the interval and still succeeds even when nothing is wired to refresh mirrors. That leaves a public API that looks enabled but is effectively a no-op.Also applies to: 119-175, 177-197
🤖 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 `@support/imageresolution/provider_set.go` around lines 83 - 86, The WithDynamicImageRegistryMirrors setter records hasDynamicMirrors and dynamicInterval but Build() ignores them, producing a misleading no-op public API; update Build (method on ProviderSetBuilder) to check hasDynamicMirrors and either (a) wire up the dynamic refresh component using dynamicInterval (e.g., register/start the mirror refresher) or (b) return an error or panic if the necessary dynamic refresher implementation is not present, so callers enabling dynamic mirrors actually get the scheduled behavior; touch ProviderSetBuilder.Build, ensure dynamicInterval is used to configure the refresher, and keep hasDynamicMirrors consistent with the final built ProviderSet.support/imageresolution/registry_client.go (1)
19-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep TLS verification enabled and honor the caller's context.
InsecureSkipVerifymakes the availability probe MITM-able, andclient.Head()here can't be cancelled by the caller. This should be a context-awareHEADon a reused client with normal TLS verification.Suggested direction
-type httpMirrorChecker struct{} +type httpMirrorChecker struct { + client *http.Client +} -func (h *httpMirrorChecker) isAvailable(_ context.Context, registry string) bool { - client := &http.Client{ - Timeout: 15 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, - } - resp, err := client.Head("https://" + registry + "/v2/") +func (h *httpMirrorChecker) isAvailable(ctx context.Context, registry string) bool { + req, err := http.NewRequestWithContext(ctx, http.MethodHead, "https://"+registry+"/v2/", nil) + if err != nil { + return false + } + resp, err := h.client.Do(req) if err != nil { return false }🤖 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 `@support/imageresolution/registry_client.go` around lines 19 - 26, The isAvailable method currently creates a new http.Client with TLS InsecureSkipVerify and calls client.Head which ignores the provided context; change httpMirrorChecker.isAvailable to use a reused http.Client on the httpMirrorChecker struct (remove TLSClientConfig.InsecureSkipVerify so default TLS verification is used), create a context-aware HEAD request via http.NewRequestWithContext(ctx, "HEAD", "https://"+registry+"/v2/", nil), execute it with client.Do(req), ensure resp.Body is closed, and respect context cancellation/timeouts instead of calling client.Head.support/imageresolution/config_source.go (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeep-copy
ResolverConfigat every source boundary.These value copies are shallow.
current()hands out aliases to the internal maps/slices, andupdateMirrors()stores the caller's map directly, so external code can mutate live resolver state outside the lock.Suggested direction
func newStaticConfigSource(cfg ResolverConfig) *staticConfigSource { - return &staticConfigSource{config: cfg} + return &staticConfigSource{config: cfg.Clone()} } func (s *staticConfigSource) current(_ context.Context) (ResolverConfig, error) { - return s.config, nil + return s.config.Clone(), nil } func newMutableConfigSource(cfg ResolverConfig) *mutableConfigSource { - return &mutableConfigSource{config: cfg} + return &mutableConfigSource{config: cfg.Clone()} } func (m *mutableConfigSource) current(_ context.Context) (ResolverConfig, error) { m.mu.RLock() defer m.mu.RUnlock() - return m.config, nil + return m.config.Clone(), nil } func (m *mutableConfigSource) updateMirrors(mirrors map[string][]string) { m.mu.Lock() defer m.mu.Unlock() - m.config.ImageRegistryMirrors = mirrors + m.config.ImageRegistryMirrors = cloneStringSliceMap(mirrors) }Also applies to: 29-42
🤖 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 `@support/imageresolution/config_source.go` around lines 16 - 21, The ResolverConfig and its internals are being aliased instead of cloned; update newStaticConfigSource, staticConfigSource.current, and updateMirrors to perform a deep copy of the ResolverConfig (clone maps, slices and any nested structures) whenever storing or returning a ResolverConfig so callers never get direct references to internal maps/slices; specifically, in newStaticConfigSource clone the incoming cfg before assigning to staticConfigSource.config, in staticConfigSource.current return a deep-copied ResolverConfig (not s.config), and in updateMirrors copy the caller-provided map/slices into new map/slice instances before storing them.support/imageresolution/cache.go (1)
38-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEvict expired entries when they are observed.
These
getpaths treat expired data as a miss but keep the stale key in the map, so the caches still grow unbounded over time in a long-lived controller.Also applies to: 75-82, 112-119
🤖 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 `@support/imageresolution/cache.go` around lines 38 - 45, The get method in releaseCache currently treats expired entries as misses but leaves them in c.entries, so fix releaseCache.get by evicting observed expired entries: when you detect expiry (time.Since(entry.timestamp) > c.ttl) while holding c.mu.RLock(), release the RLock(), acquire c.mu.Lock(), re-check the entry still exists and is still expired (compare entry.timestamp and c.ttl), delete it from c.entries, release the Lock, and return nil; use c.mu, c.entries, entry.timestamp, c.ttl and releaseCache.get as the targets. Apply the same read->upgrade-to-write-delete pattern to the other cache getter functions that check time.Since(... ) so stale keys are removed when observed.
🤖 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 `@karpenter-operator/main.go`:
- Around line 162-166: The provider set is being given CLI registry overrides as
mirror entries (WithImageRegistryMirrors(imageRegistryOverrides)), which turns
mandatory CLI rewrites into fallbacks and leaves GetRegistryOverrides() empty;
instead pass the CLI overrides to the provider set via a dedicated method (e.g.,
add WithRegistryOverrides(registryOverrides) or
WithImageRegistryOverrides(registryOverrides) on
imageresolution.NewProviderSet()) and leave imageRegistryOverrides for mirror
configuration only; if the builder lacks that method, add it to the
imageresolution.ProviderSet API and ensure Build() stores the overrides so
GetRegistryOverrides() returns them.
In `@support/imageresolution/adapters.go`:
- Around line 36-44: The Lookup method on releaseProviderAdapter returns the
cached ri.ImageStream and ri.StreamMetadata directly; create and return
defensive copies instead to avoid shared-state mutation. In
releaseProviderAdapter.Lookup, after receiving ri from a.ps.release.Lookup,
deep-copy ri.ImageStream and ri.StreamMetadata (e.g., allocate new
releaseinfo.ReleaseImage and copy fields or use existing clone/copy helpers) and
populate the returned releaseinfo.ReleaseImage with those copies rather than
ri.ImageStream/ri.StreamMetadata so callers cannot mutate provider-held objects.
---
Outside diff comments:
In `@control-plane-operator/main.go`:
- Around line 450-478: The code mutates imageRegistryOverrides by merging
registryOverrides into it, then passes that mutated map to dpProviderSet which
causes management-cluster registry overrides to leak into the data-plane; to
fix, avoid mutating the original imageRegistryOverrides used for data-plane by
creating a separate map for the control-plane merge (e.g. copy
imageRegistryOverrides to a new local map, merge registryOverrides into that
map) and pass that merged map into cpProviderSet.WithImageRegistryMirrors while
passing the unmodified imageRegistryOverrides (or nil if originally nil) to
dpProviderSet.WithImageRegistryMirrors; update the code around
registryOverrides/imageRegistryOverrides and the calls that build cpProviderSet
and dpProviderSet
(imageresolution.NewProviderSet().WithImageRegistryMirrors(...)) accordingly.
---
Duplicate comments:
In `@support/imageresolution/cache.go`:
- Around line 38-45: The get method in releaseCache currently treats expired
entries as misses but leaves them in c.entries, so fix releaseCache.get by
evicting observed expired entries: when you detect expiry
(time.Since(entry.timestamp) > c.ttl) while holding c.mu.RLock(), release the
RLock(), acquire c.mu.Lock(), re-check the entry still exists and is still
expired (compare entry.timestamp and c.ttl), delete it from c.entries, release
the Lock, and return nil; use c.mu, c.entries, entry.timestamp, c.ttl and
releaseCache.get as the targets. Apply the same read->upgrade-to-write-delete
pattern to the other cache getter functions that check time.Since(... ) so stale
keys are removed when observed.
In `@support/imageresolution/config_source.go`:
- Around line 16-21: The ResolverConfig and its internals are being aliased
instead of cloned; update newStaticConfigSource, staticConfigSource.current, and
updateMirrors to perform a deep copy of the ResolverConfig (clone maps, slices
and any nested structures) whenever storing or returning a ResolverConfig so
callers never get direct references to internal maps/slices; specifically, in
newStaticConfigSource clone the incoming cfg before assigning to
staticConfigSource.config, in staticConfigSource.current return a deep-copied
ResolverConfig (not s.config), and in updateMirrors copy the caller-provided
map/slices into new map/slice instances before storing them.
In `@support/imageresolution/provider_set.go`:
- Around line 72-79: The builder currently stores and later exposes direct
references to caller-provided maps/slices (e.g., in WithRegistryOverrides and
WithImageRegistryMirrors), which allows external mutation to affect the built
ProviderSet; clone all input maps and any nested slices before storing them in
the builder and again when wiring into the live ProviderSet (apply the same
cloning pattern referenced around lines 94-96 and 124-165). Specifically, in
ProviderSetBuilder.WithRegistryOverrides(copy the overrides map into a new map)
and ProviderSetBuilder.WithImageRegistryMirrors(copy the mirrors map and for
each key copy the []string slice, set hasStaticMirrors accordingly), and ensure
Build() or the code that transfers builder fields into ProviderSet also performs
defensive copies so the runtime ProviderSet never holds references to
caller-owned collections.
- Around line 83-86: The WithDynamicImageRegistryMirrors setter records
hasDynamicMirrors and dynamicInterval but Build() ignores them, producing a
misleading no-op public API; update Build (method on ProviderSetBuilder) to
check hasDynamicMirrors and either (a) wire up the dynamic refresh component
using dynamicInterval (e.g., register/start the mirror refresher) or (b) return
an error or panic if the necessary dynamic refresher implementation is not
present, so callers enabling dynamic mirrors actually get the scheduled
behavior; touch ProviderSetBuilder.Build, ensure dynamicInterval is used to
configure the refresher, and keep hasDynamicMirrors consistent with the final
built ProviderSet.
In `@support/imageresolution/registry_client.go`:
- Around line 19-26: The isAvailable method currently creates a new http.Client
with TLS InsecureSkipVerify and calls client.Head which ignores the provided
context; change httpMirrorChecker.isAvailable to use a reused http.Client on the
httpMirrorChecker struct (remove TLSClientConfig.InsecureSkipVerify so default
TLS verification is used), create a context-aware HEAD request via
http.NewRequestWithContext(ctx, "HEAD", "https://"+registry+"/v2/", nil),
execute it with client.Do(req), ensure resp.Body is closed, and respect context
cancellation/timeouts instead of calling client.Head.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1eb395c5-bf2c-448b-b19a-0089253112a9
📒 Files selected for processing (17)
control-plane-operator/main.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.gohypershift-operator/controllers/hostedclustersizing/setup.gohypershift-operator/main.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.gokarpenter-operator/main.gosupport/imageresolution/adapters.gosupport/imageresolution/adapters_test.gosupport/imageresolution/cache.gosupport/imageresolution/config_source.gosupport/imageresolution/config_source_test.gosupport/imageresolution/provider_set.gosupport/imageresolution/registry_client.gosupport/imageresolution/registry_client_test.gosupport/imageresolution/release.go
✅ Files skipped from review due to trivial changes (1)
- support/imageresolution/config_source_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/imageresolution/release.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
support/imageresolution/provider_set.go (1)
45-52: ⚡ Quick winDefensively clone refreshed mirrors before storing shared state.
Reconcile()currently assigns the returnedmirrorsmap directly into internal fields. Cloning here avoids aliasing and surprise mutation/race hazards if the refresh implementation reuses that map later.Suggested fix
func (ps *ProviderSet) Reconcile(ctx context.Context, client crclient.Client) error { if ps.mirrorRefresh == nil { return nil } mirrors, err := ps.mirrorRefresh(ctx, client) if err != nil { return fmt.Errorf("refreshing image registry mirrors: %w", err) } - ps.configSource.updateMirrors(mirrors) + mirrorsCopy := cloneStringSliceMap(mirrors) + ps.configSource.updateMirrors(mirrorsCopy) if ps.rawMetadata != nil { - ps.rawMetadata.OpenShiftImageRegistryOverrides = mirrors + ps.rawMetadata.OpenShiftImageRegistryOverrides = cloneStringSliceMap(mirrorsCopy) } return nil }🤖 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 `@support/imageresolution/provider_set.go` around lines 45 - 52, The refreshed mirrors map returned by ps.mirrorRefresh should be defensively cloned before storing into shared state to avoid aliasing and mutation races; create a new map variable, copy all key/value pairs from mirrors into it (handling nil returned maps by creating an empty map), and then pass that cloned map to ps.configSource.updateMirrors and assign it to ps.rawMetadata.OpenShiftImageRegistryOverrides so the internal state holds an independent copy.
🤖 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 `@support/imageresolution/provider_set.go`:
- Around line 187-198: The ForDataPlane() invariant is bypassable via
WithMirrorRefresh(); update ProviderSetBuilder.validate() to reject
WithMirrorRefresh when b.forDataPlane is true by adding a check (e.g., if
b.forDataPlane && <mirrorRefreshFlag> { return fmt.Errorf("ForDataPlane() is
incompatible with WithMirrorRefresh()") }), where <mirrorRefreshFlag> is the
builder field set by WithMirrorRefresh(); also audit Reconcile() to ensure it
does not inject mirrors into resolver state when forDataPlane is true.
---
Nitpick comments:
In `@support/imageresolution/provider_set.go`:
- Around line 45-52: The refreshed mirrors map returned by ps.mirrorRefresh
should be defensively cloned before storing into shared state to avoid aliasing
and mutation races; create a new map variable, copy all key/value pairs from
mirrors into it (handling nil returned maps by creating an empty map), and then
pass that cloned map to ps.configSource.updateMirrors and assign it to
ps.rawMetadata.OpenShiftImageRegistryOverrides so the internal state holds an
independent copy.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 445d3ec6-b69f-4769-b8f5-36e4af898615
📒 Files selected for processing (29)
control-plane-operator/main.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.gohypershift-operator/controllers/hostedclustersizing/setup.gohypershift-operator/main.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.gokarpenter-operator/main.gosupport/imageresolution/adapters.gosupport/imageresolution/adapters_test.gosupport/imageresolution/cache.gosupport/imageresolution/cache_test.gosupport/imageresolution/component.gosupport/imageresolution/component_test.gosupport/imageresolution/config.gosupport/imageresolution/config_source.gosupport/imageresolution/config_source_test.gosupport/imageresolution/config_test.gosupport/imageresolution/metadata.gosupport/imageresolution/metadata_test.gosupport/imageresolution/provider_set.gosupport/imageresolution/provider_set_test.gosupport/imageresolution/registry_client.gosupport/imageresolution/registry_client_test.gosupport/imageresolution/release.gosupport/imageresolution/release_test.gosupport/imageresolution/resolver.gosupport/imageresolution/resolver_test.gosupport/imageresolution/testutil.go
✅ Files skipped from review due to trivial changes (6)
- support/imageresolution/provider_set_test.go
- support/imageresolution/cache_test.go
- support/imageresolution/component_test.go
- support/imageresolution/adapters_test.go
- support/imageresolution/testutil.go
- support/imageresolution/config_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- support/imageresolution/component.go
- hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go
- support/imageresolution/config_source_test.go
- support/imageresolution/cache.go
- support/imageresolution/release.go
- support/imageresolution/config_source.go
- support/imageresolution/registry_client_test.go
- support/imageresolution/resolver_test.go
- hypershift-operator/main.go
- support/imageresolution/release_test.go
- ignition-server/controllers/local_ignitionprovider.go
- support/imageresolution/metadata_test.go
- hypershift-operator/controllers/hostedclustersizing/setup.go
- support/imageresolution/resolver.go
- support/imageresolution/metadata.go
- karpenter-operator/main.go
- ignition-server/cmd/start.go
- support/imageresolution/adapters.go
- control-plane-operator/main.go
bryan-cox
force-pushed
the
CNTRLPLANE-3314
branch
2 times, most recently
from
5b2759c to
61076c0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
support/imageresolution/registry_client.go (1)
28-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-enable certificate validation for mirror probes.
Line [29] still sets
InsecureSkipVerify: true. Even for a HEAD probe, this allows MITM spoofing of mirror availability decisions. Keep TLS verification on and trust self-signed mirrors via CA roots instead.Suggested minimal change
Transport: &http.Transport{ // `#nosec` G402 -- mirrors may use self-signed certs; availability // probe only, no data is transferred. TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, MinVersion: tls.VersionTLS12, }, },#!/bin/bash # Verify all insecure TLS skips and inspect whether CA-root wiring exists for mirror checks. rg -n --type=go 'InsecureSkipVerify\s*:\s*true' rg -n --type=go 'httpMirrorChecker|newHTTPMirrorChecker|RootCAs|TLSClientConfig|additional.*trust|ca.*bundle'🤖 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 `@support/imageresolution/registry_client.go` around lines 28 - 31, The TLSClientConfig for mirror probes currently sets InsecureSkipVerify: true which disables certificate validation; change this in the TLSClientConfig used by the mirror probe (the struct instance created where TLSClientConfig is set) to enable verification (remove or set InsecureSkipVerify: false) and wire up RootCAs instead: load x509.SystemCertPool(), append any configured mirror CA certs if present, and assign that pool to TLSClientConfig.RootCAs while keeping MinVersion: tls.VersionTLS12; ensure the HTTP mirror checker (the code that constructs the TLSClientConfig/transport used by the HEAD probe) uses this verified config so self-signed mirrors are trusted via CA roots rather than skipping verification.
🤖 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.
Duplicate comments:
In `@support/imageresolution/registry_client.go`:
- Around line 28-31: The TLSClientConfig for mirror probes currently sets
InsecureSkipVerify: true which disables certificate validation; change this in
the TLSClientConfig used by the mirror probe (the struct instance created where
TLSClientConfig is set) to enable verification (remove or set
InsecureSkipVerify: false) and wire up RootCAs instead: load
x509.SystemCertPool(), append any configured mirror CA certs if present, and
assign that pool to TLSClientConfig.RootCAs while keeping MinVersion:
tls.VersionTLS12; ensure the HTTP mirror checker (the code that constructs the
TLSClientConfig/transport used by the HEAD probe) uses this verified config so
self-signed mirrors are trusted via CA roots rather than skipping verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1f77d81e-16e8-4cf9-921c-9a8d68d13a62
📒 Files selected for processing (29)
control-plane-operator/main.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.gohypershift-operator/controllers/hostedclustersizing/setup.gohypershift-operator/main.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.gokarpenter-operator/main.gosupport/imageresolution/adapters.gosupport/imageresolution/adapters_test.gosupport/imageresolution/cache.gosupport/imageresolution/cache_test.gosupport/imageresolution/component.gosupport/imageresolution/component_test.gosupport/imageresolution/config.gosupport/imageresolution/config_source.gosupport/imageresolution/config_source_test.gosupport/imageresolution/config_test.gosupport/imageresolution/metadata.gosupport/imageresolution/metadata_test.gosupport/imageresolution/provider_set.gosupport/imageresolution/provider_set_test.gosupport/imageresolution/registry_client.gosupport/imageresolution/registry_client_test.gosupport/imageresolution/release.gosupport/imageresolution/release_test.gosupport/imageresolution/resolver.gosupport/imageresolution/resolver_test.gosupport/imageresolution/testutil.go
✅ Files skipped from review due to trivial changes (5)
- support/imageresolution/component_test.go
- support/imageresolution/config_test.go
- support/imageresolution/testutil.go
- support/imageresolution/config_source_test.go
- support/imageresolution/adapters_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- support/imageresolution/provider_set_test.go
- control-plane-operator/main.go
- support/imageresolution/config_source.go
- ignition-server/cmd/start.go
- ignition-server/controllers/local_ignitionprovider.go
- karpenter-operator/main.go
- support/imageresolution/metadata_test.go
- hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.go
- support/imageresolution/registry_client_test.go
- support/imageresolution/release.go
- support/imageresolution/config.go
- support/imageresolution/resolver.go
- support/imageresolution/cache.go
- hypershift-operator/main.go
- support/imageresolution/adapters.go
- support/imageresolution/metadata.go
- support/imageresolution/release_test.go
- support/imageresolution/component.go
- support/imageresolution/resolver_test.go
- support/imageresolution/cache_test.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go
- support/imageresolution/provider_set.go
bryan-cox
force-pushed
the
CNTRLPLANE-3314
branch
2 times, most recently
from
0b41b8c to
1cab572
Compare
|
I now have all the evidence needed. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Codecov failures were transient race-condition failures caused by Codecov's Root CauseThe root cause is a race condition between Codecov status reporting and GitHub Actions coverage uploads, caused by the Detailed timeline on commit
The The failing check run IDs referenced by the user ( Recommendations
Evidence
|
bryan-cox
force-pushed
the
CNTRLPLANE-3314
branch
3 times, most recently
from
6f8e3f4 to
0c4cee3
Compare
…esolution target
- Vendor golang.org/x/tools/{go/analysis,go/ast,txtar,internal} for the
custom go/analysis linter
- Add IMAGERESOLUTION_LINT binary build target and lint-imageresolution
Makefile target
- Wire lint-imageresolution into lint with generate dependency
Signed-off-by: Bryan Cox <brcox@redhat.com>
Commit-Message-Assisted-by: Claude (via Claude Code)
Add support/imageresolution with a ProviderSet builder that consolidates the scattered decorator chain into a single construction API: - ProviderSet builder: NewProviderSet().WithRegistryOverrides(). WithImageRegistryMirrors().WithImageOverrides().ForDataPlane().Build() - ResolverConfig with RegistryOverridesFlag/ImageRegistryMirrorsEnvVar serialization and parsing helpers - Internal imageResolver with resolveForDirectFetch (CLI overrides + ICSP/IDMS mirrors) and resolveForPodSpec (CLI overrides only) - Thread-safe releaseCache, mirrorAvailabilityCache, and metadataCache - Mutex-protected mirroredReleaseImage field - Error propagation from configSource through resolver methods - ProviderSet.Lookup rewrites ImageStream tags from resolved component images so ComponentImages() returns overridden refs - Adapter layer: ImageMetadataProvider() with synchronized rawMetadata - Comprehensive behavior-driven tests with Gherkin naming Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Add a custom go/analysis linter that flags bypass of the imageresolution abstraction boundary: - Raw override map parameters and struct fields with override-related names (registryOverrides, imageRegistryMirrors, etc.) - Direct imports of go-containerregistry and containers/image - References to deprecated decorator types (RegistryMirrorProviderDecorator, ProviderWithOpenShiftImageRegistryOverridesDecorator, etc.) - Exempt: imageresolution package, support/util, ignition-server/cmd, hostedclusterconfigoperator root package, main packages, test files - analysistest-based test suite with testdata fixtures Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…types Wire imageresolution.ProviderSet into HO, karpenter-operator, and catalog image construction, replacing the old decorator chain: - HO main.go: ProviderSet with CLI overrides, static mirrors, dynamic mirror refresh via WithMirrorRefresh callback - Karpenter main.go: ProviderSet with mirrors only (no CLI overrides) - Catalogs images.go: accept ResolverConfig instead of raw map - HC controller: use ProviderSet directly as ReleaseProvider, remove dead RegistryOverrides field - HC webhook and sizing controller: widen to interface types - Etcd backup reconciler: remove dead mock methods from fake provider Delete old abstractions: - RegistryMirrorProviderDecorator (support/releaseinfo) - ProviderWithOpenShiftImageRegistryOverridesDecorator (support/releaseinfo) - ProviderWithRegistryOverrides, ProviderWithOpenShiftImageRegistryOverrides interfaces (support/releaseinfo) - CommonRegistryProvider, RegistryProvider (support/globalconfig) - ConvertRegistryOverridesToCommandLineFlag and related (support/util) Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
… HCCO, and ignition-server Wire imageresolution.ProviderSet into CPO, HCCO, and ignition-server: - CPO main.go: dual ProviderSet -- control-plane with CLI overrides + mirrors + image overrides, data-plane with ForDataPlane() + ICSP/IDMS mirrors for disconnected fetch - HCP controller: receive both ProviderSets, pass Config() to configoperator and ignition-server components - HCCO configoperator: accept ResolverConfig for --registry-overrides and OPENSHIFT_IMG_OVERRIDES serialization - HCCO cmd.go: ProviderSet with mirrors only - Ignition-server: ProviderSet with overrides and mirrors - OLM catalogs and HCCO OLM params: wrap mirrors in ResolverConfig Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
support/imageresolutionpackage consolidating scattered image resolution logic (5 decorator types, 3 utility functions) into a single ProviderSet with a builder APIresolveForDirectFetch(CLI overrides + ICSP/IDMS mirrors for registry I/O) andresolveForPodSpec(CLI overrides only for management cluster pod specs)ForDataPlane()builder preset enforces that CLI overrides and image overrides cannot leak to data-plane components atBuild()time, while still allowing ICSP/IDMS mirrors for disconnected release image fetch (per OCPBUGS-44470)go/analysislinter (lint-imageresolution) wired intomake lintto prevent the old patterns from returningRegistryMirrorProviderDecorator,ProviderWithOpenShiftImageRegistryOverridesDecorator,CommonRegistryProvider, and utility functionsEnhancement
openshift/enhancements#1981
Commits
golang.org/x/tools/go/analysis+ Makefile lint targetsupport/imageresolution/core library with builder, resolver, caches, adapters, and testsgo/analysisanalyzer with 3 rules (raw map params, banned imports, deprecated types) + test fixturesKey design decisions
resolveForPodSpecdoes NOT apply mirrors, so data-plane pod specs still see original pullspecs. CLI overrides, dynamic mirror refresh, and image overrides are rejected.releaseinfo.ReleaseImage.ComponentImages()returns the overridden refs.mirroredReleaseImageprotected bysync.RWMutex,rawMetadata.OpenShiftImageRegistryOverridessynchronized in adapter methods,configSourceusessync.RWMutexfor dynamic mirror updates.Test plan
-racedetectionmake verifyclean (excluding verify-git-clean)make test— zero failuresmake lint-imageresolution— zero violationsTestDualProviderSetInvariantvalidates management vs data-plane isolation including mirror fetch🤖 Generated with Claude Code