Conversation
Record a positive catalog publication version from the OCI image config label alongside cached catalog content and in ClusterCatalog status. Reject a newly resolved digest when the currently accepted catalog is versioned and the new catalog has no version or a non-increasing version. Preserve the last accepted version when content is unavailable so subsequent reconciles continue to reject the rollback. Add controller, cache, and puller coverage for version parsing, metadata persistence and garbage collection, and rollback rejection. Validation: make test-unit Signed-off-by: Todd Short <tshort@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
tmshort
changed the title
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change reads catalog publication versions from OCI image metadata, stores them with cached images, and exposes them in ChangesCatalog publication version
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ClusterCatalogController
participant CatalogPuller
participant CatalogVersionCache
participant ClusterCatalogStatus
ClusterCatalogController->>CatalogPuller: Pull catalog content and version
CatalogPuller->>CatalogVersionCache: Look up version for canonical image reference
CatalogVersionCache-->>CatalogPuller: Return cached catalog version
CatalogPuller-->>ClusterCatalogController: Return content, reference, and version
ClusterCatalogController->>ClusterCatalogController: Compare versions when image digests differ
alt Version accepted
ClusterCatalogController->>ClusterCatalogStatus: Record resolved reference and catalog version
else Rollback rejected
ClusterCatalogController->>ClusterCatalogStatus: Set progressing status and retain accepted source
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The publication-version guard behaves as documented, including when a catalog source changes. No identified issue needs resolution before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Rollback protection can lose track of a newly served catalog when saving its accepted version fails. A later retry may then accept an older publication. Existing image-trust controls still limit which content can be accepted. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 10 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
I think this approach cannot proceed because it negatively impacts custom ClusterCatalogs from users which express a catalog-version. In this mode, a rollback may be desired/required and the feature provides no capability to override the new behavior. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
| // greater version before its content can replace the currently served catalog. | ||
| // +kubebuilder:validation:Minimum:=1 | ||
| // +optional | ||
| CatalogVersion int64 `json:"catalogVersion,omitempty"` |
There was a problem hiding this comment.
We don't have to record this in the API. It could just be stored and read from the cache. Something we should discuss more.
Also, I think monotonic increase checks should only apply if the spec's reference is unchanged.
|
|
||
| fsys, canonicalRef, unpackTime, err := r.ImagePuller.Pull(ctx, catalog.Name, catalog.Spec.Source.Image.Ref, r.ImageCache) | ||
| catalogPuller, ok := r.ImagePuller.(imageutil.CatalogPuller) | ||
| if !ok { |
There was a problem hiding this comment.
We shouldn't type assert here. That breaks the abstraction. We may need to think about extending the source interface or adding a new, optional SourcePolicy interface/implementation that abstractly validates an update. That would also keep this function much cleaner.
|
Cribbing from ongoing slack convo for notes:
|
| // +kubebuilder:validation:XValidation:rule="self.find('(@.*:)') != \"\" ? self.find(':.*$').matches(':[0-9A-Fa-f]*$') : true",message="digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)" | ||
| Ref string `json:"ref"` | ||
| // catalogVersion is the monotonic version assigned to the catalog publication by the image publisher. | ||
| // It is read from the OCI image config label "olm.operatorframework.io/catalog-version". |
There was a problem hiding this comment.
I don't think we need something new here. I think we just need to look at the build timestamp that already exists in OCI image configs (at least until we end up using a custom OCI artifact, but that's another whole thing that's out-of-scope here)
This branch has not been deployed
3 participants
Summary
References OCPBUGS-123707.
olm.operatorframework.io/catalog-versionin cached catalog metadata andClusterCatalogstatus.Validation
make test-unitSummary by CodeRabbit