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. |
WalkthroughImplements end-to-end icon caching and serving for operator packages. Frontend components render icons via a dynamic URL, backend routes requests through a new HTTP handler with ETag/If-Modified-Since support, fetches icons from catalogd on cache miss, and stores results locally. 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.
| // Use a custom icon node with loading="lazy" for efficient loading | ||
| // eslint-disable-next-line jsx-a11y/alt-text | ||
| const iconNode = <img src={icon.url} loading="lazy" className="catalog-tile-pf-icon" />; | ||
| return { iconImg: null, iconClass: null, icon: iconNode }; |
There was a problem hiding this comment.
Add alt text for accessibility compliance.
The image element lacks an alt attribute, which is essential for screen readers and accessibility. Even icon images should provide descriptive alternative text or an empty alt="" if purely decorative.
🔎 Proposed fix
- // Use a custom icon node with loading="lazy" for efficient loading
- // eslint-disable-next-line jsx-a11y/alt-text
- const iconNode = <img src={icon.url} loading="lazy" className="catalog-tile-pf-icon" />;
+ // Use a custom icon node with loading="lazy" for efficient loading
+ const iconNode = <img src={icon.url} loading="lazy" className="catalog-tile-pf-icon" alt="" />;Note: Use alt="" for decorative icons or provide a meaningful description if the icon conveys important information (e.g., alt={item.name} or alt="Operator icon").
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use a custom icon node with loading="lazy" for efficient loading | |
| // eslint-disable-next-line jsx-a11y/alt-text | |
| const iconNode = <img src={icon.url} loading="lazy" className="catalog-tile-pf-icon" />; | |
| return { iconImg: null, iconClass: null, icon: iconNode }; | |
| // Use a custom icon node with loading="lazy" for efficient loading | |
| const iconNode = <img src={icon.url} loading="lazy" className="catalog-tile-pf-icon" alt="" />; | |
| return { iconImg: null, iconClass: null, icon: iconNode }; |
🤖 Prompt for AI Agents
In
@frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
around lines 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.
| // Remove icon until we have an endpoint to lazy load cached icons. | ||
| // TODO Add icon back once https://issues.redhat.com/browse/CONSOLE-4728 is completed. | ||
| // icon: { url: '/api/olm/catalog-icons/<catalog-name>/<package-name> }, | ||
| icon: { url: `/api/olm/catalog-icons/${catalog}/${name}` }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file to understand the context around line 102
cat -n frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsx | head -120 | tail -40Repository: openshift/console
Length of output: 1674
🏁 Script executed:
# Check if there's any URL encoding or validation in the file
rg -n "encodeURI|decodeURI|sanitize|validate" frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsxRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for the backend endpoint handler that serves catalog-icons
rg -n -A 10 "catalog-icons" --type=go --type=ts --type=jsRepository: openshift/console
Length of output: 5915
🏁 Script executed:
# Look for any TypeScript type definitions for OLMCatalogItem to understand catalog/name types
rg -n "interface OLMCatalogItem|type OLMCatalogItem" --type=tsRepository: openshift/console
Length of output: 368
🏁 Script executed:
# Find the catalogIconHandler implementation
rg -n -A 30 "func.*catalogIconHandler" pkg/olm/ --type=goRepository: openshift/console
Length of output: 4178
🏁 Script executed:
# Search for any path validation or sanitization in the handler
rg -n "filepath|CleanPath|path\.Clean|Join" pkg/olm/ --type=goRepository: openshift/console
Length of output: 129
🏁 Script executed:
# Check the full OLMCatalogItem type definition
cat -n frontend/packages/operator-lifecycle-manager-v1/src/types.ts | head -50Repository: openshift/console
Length of output: 756
URL-encode path parameters to prevent malformed URLs and potential path traversal issues.
The catalog and name variables are interpolated directly into the URL path without encoding. If these values contain special characters (spaces, slashes, percent signs, etc.), the resulting URL will be malformed. Go's HTTP router automatically URL-decodes path segments, requiring the frontend to properly encode them before insertion.
Proposed fix
- icon: { url: `/api/olm/catalog-icons/${catalog}/${name}` },
+ icon: { url: `/api/olm/catalog-icons/${encodeURIComponent(catalog)}/${encodeURIComponent(name)}` },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| icon: { url: `/api/olm/catalog-icons/${catalog}/${name}` }, | |
| icon: { url: `/api/olm/catalog-icons/${encodeURIComponent(catalog)}/${encodeURIComponent(name)}` }, |
🤖 Prompt for AI Agents
In @frontend/packages/operator-lifecycle-manager-v1/src/utils/catalog-item.tsx
around 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.
|
@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. |
2 participants
Description
How to test it