CONSOLE-4728: Create a cached server endpoint for operator icons with OLMv1 #15889
Conversation
Adds infrastructure to cache operator icons from catalogd: - CachedIcon struct to store icon data with ETag/LastModified headers - FetchPackageIcon client method to query catalogd metas endpoint - GetPackageIcon service method with in-memory caching - parsePackageIcon to extract icon data from FBC package metadata 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds /api/olm/catalog-icons/{catalogName}/{packageName} endpoint that:
- Serves cached operator icons by package name
- Supports conditional requests with ETag and If-Modified-Since headers
- Returns 304 Not Modified when cache is still valid
- Sets appropriate Cache-Control headers for browser caching
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updates frontend to use the new icon caching endpoint:
- Add icon URL pointing to /api/olm/catalog-icons/{catalog}/{package}
- Use lazy loading (loading="lazy") for efficient icon loading
- Return custom icon node from getIconProps for URL-based icons
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@Leo6Leo: This pull request references CONSOLE-4728 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 "4.22.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. |
WalkthroughAdds end-to-end package icon support: frontend renders icons via a new catalog-icons URL or a lazy-loaded img node; backend introduces CachedIcon, client FetchPackageIcon, service GetPackageIcon with caching and parsing, and a new HTTP handler serving /api/olm/catalog-icons/{catalog}/{package} with ETag/If-Modified-Since semantics and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Leo6Leo 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 |
openshift-ci
bot
added
component/olm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx:
- Around line 268-271: The img element assigned to iconNode lacks an alt
attribute; update the creation of iconNode (the constant named iconNode) to
include a descriptive alt (e.g., alt={item?.name || "Operator icon"}) or alt=""
if the icon is purely decorative so that screen readers handle it correctly
while keeping loading="lazy" and the existing className.
In @frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsx:
- Line 102: The URL is built by interpolating catalog and name directly into the
path, which can produce malformed or unsafe URLs; update the construction to
URL-encode both variables before insertion by applying encodeURIComponent to
catalog and name (so the icon.url uses the encoded catalog and encoded name) to
prevent special chars and path traversal issues and ensure the frontend matches
the router's decoded path handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsxfrontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsxpkg/olm/catalog.gopkg/olm/catalog_client.gopkg/olm/catalog_service.gopkg/olm/catalog_service_test.gopkg/olm/handler.gopkg/olm/handler_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/olm/catalog_client.gofrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsxpkg/olm/handler.gopkg/olm/catalog_service_test.gopkg/olm/catalog_service.gopkg/olm/handler_test.gofrontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsxpkg/olm/catalog.go
🧬 Code graph analysis (5)
pkg/olm/handler.go (2)
pkg/middleware/middleware.go (1)
AllowMethod(86-88)pkg/serverutils/utils.go (3)
SendResponse(13-28)ApiError(87-89)ModifiedSince(63-85)
pkg/olm/catalog_service_test.go (2)
pkg/olm/catalog.go (1)
CachedIcon(16-21)pkg/olm/catalog_service.go (1)
CatalogService(35-41)
pkg/olm/catalog_service.go (1)
pkg/olm/catalog.go (1)
CachedIcon(16-21)
pkg/olm/handler_test.go (3)
pkg/olm/catalog.go (1)
CachedIcon(16-21)pkg/olm/catalog_service.go (1)
NewCatalogService(59-66)pkg/olm/handler.go (1)
NewOLMHandler(26-40)
frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsx (1)
frontend/packages/integration-tests-cypress/views/catalogs.ts (1)
catalog(1-10)
🔇 Additional comments (13)
pkg/olm/catalog.go (1)
15-21: LGTM!The
CachedIconstruct is well-designed with appropriate fields for HTTP caching semantics (ETag, LastModified) and icon data (Data, MediaType).pkg/olm/catalog_service_test.go (3)
20-27: LGTM!The mock client extension with
packageIconMap,fetchPackageErr, andfetchPackageCodeprovides good flexibility for testing various icon fetch scenarios.
34-70: LGTM!The mock
FetchPackageIconimplementation properly handles all scenarios: network errors, 404 responses, and successful icon retrieval with correctly formatted JSON output.
273-399: LGTM!Comprehensive test coverage for
GetPackageIconincluding cache hit, cache miss with fetch, package not found, missing icon, and error propagation scenarios.pkg/olm/handler.go (2)
35-35: LGTM!Route registration follows the existing pattern and correctly uses
middleware.AllowMethodfor GET-only access.
99-153: LGTM!The handler implementation is well-structured:
- Proper input validation with clear error messages.
- Correct ETag comparison using quoted format per HTTP spec.
- Graceful handling of invalid
If-Modified-Sinceheaders.- Appropriate cache headers (24h max-age matching the service cache expiration).
pkg/olm/catalog_client.go (2)
16-16: LGTM!Interface extension is consistent with existing methods.
94-118: LGTM!The implementation follows the established pattern from other fetch methods, correctly builds the URL with query parameters, and logs the request appropriately.
pkg/olm/handler_test.go (2)
4-6: LGTM!New imports are required for the test functionality (base64 encoding and formatting).
120-272: LGTM!Excellent test coverage for the catalog icon handler covering:
- Cache hit with proper headers
- 404 scenarios (not found, URL pattern mismatch)
- 304 responses for both ETag and If-Modified-Since
- End-to-end fetch from catalogd on cache miss
pkg/olm/catalog_service.go (3)
55-57: LGTM!Cache key helper follows the established naming pattern.
79-141: LGTM!
GetPackageIconimplements a robust cache-first pattern with:
- Defensive handling of malformed cache entries
- Proper response body cleanup
- Clear status code handling (404 vs other errors)
- Appropriate caching of fetched icons
143-177: LGTM!The parsing implementation correctly uses
WalkMetasReaderfor consistency with other catalog processing. The MD5-based ETag generation is appropriate for cache validation purposes.
frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsx
Outdated
Show resolved
Hide resolved
|
/retest-required |
|
@Leo6Leo the PR is very well written and follows all conventions! lgtm from my end, i just have one suggestion. If we can accept coderabbits' comment on incorporating the |
…catalog-utils.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…log-item.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@Leo6Leo: The following tests 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. |
3 participants
Description
How to test it