[release-4.16] OCPBUGS-88027: Fix JSON annotation parsing to prevent OperatorHub crash#16588
[release-4.16] OCPBUGS-88027: Fix JSON annotation parsing to prevent OperatorHub crash#16588cardil wants to merge 1 commit into
Conversation
Harden annotation parsing for OperatorHub page to prevent TypeError when PackageManifest annotations contain malformed or non-array JSON. - Add parseJSONArrayAnnotation() with type validation - Guard .reduce()/.map() calls on parsed annotation data - Add optional chaining to installModesFor() for missing status/channels - Add unit tests for annotation parsing edge cases Backport of fix from PR openshift#14260 (OCPBUGS-35744) adapted for 4.16. Assisted-by: 🤖 Claude Opus/Sonnet 4.6
|
@cardil: This pull request references Jira Issue OCPBUGS-88027, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds parseJSONArrayAnnotation (validates JSON arrays of strings), tests it, migrates several operator-lifecycle-manager call sites to use it, and applies a safe optional-chaining fix when reading install modes. ChangesJSON Array Annotation Parsing
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
|
/jira refresh |
|
@cardil: This pull request references Jira Issue OCPBUGS-88027, which is invalid:
Comment 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. |
|
/jira refresh |
|
@cardil: This pull request references Jira Issue OCPBUGS-88027, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
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. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@cardil: This pull request references Jira Issue OCPBUGS-88027, which is valid. 7 validation(s) were run on this bug
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. |
|
Reproduction and fix verification evidence has been added to the linked JIRA OCPBUGS-88027. Summary: Tested on a 4.16 nightly cluster with a CatalogSource containing malformed CSV annotations. Stock console crashes with |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/test e2e-gcp-console |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/utils/annotations.ts (1)
31-33: ⚡ Quick winConsider improving the error message for array validation failures.
When
parsedis an array with non-string elements (e.g.,[1, 2, 3]),typeof parsedreports"object"rather than indicating it's an array with invalid element types. This could make debugging slightly harder.📝 Suggested improvement for clarity
const error = new Error( - `Expected annotation "${annotationKey}" to be an array of strings, got ${typeof parsed}`, + `Expected annotation "${annotationKey}" to be an array of strings, got ${ + Array.isArray(parsed) ? 'array with non-string elements' : typeof parsed + }`, );🤖 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 `@frontend/packages/console-shared/src/utils/annotations.ts` around lines 31 - 33, The error thrown in annotations.ts when validating annotation values uses typeof parsed which reports "object" for arrays and obscures cases like arrays with non-string elements; update the Error message created where annotationKey and parsed are used to detect and report "array with non-string elements" (or include the element types/preview of parsed) so failures show that parsed is an array but contains invalid element types (reference the variable names annotationKey and parsed and the Error construction site).
🤖 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.
Nitpick comments:
In `@frontend/packages/console-shared/src/utils/annotations.ts`:
- Around line 31-33: The error thrown in annotations.ts when validating
annotation values uses typeof parsed which reports "object" for arrays and
obscures cases like arrays with non-string elements; update the Error message
created where annotationKey and parsed are used to detect and report "array with
non-string elements" (or include the element types/preview of parsed) so
failures show that parsed is an array but contains invalid element types
(reference the variable names annotationKey and parsed and the Error
construction site).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 638407a6-2113-4be8-90a4-5d994e1f6e08
📒 Files selected for processing (5)
frontend/packages/console-shared/src/utils/__tests__/annotations.spec.tsfrontend/packages/console-shared/src/utils/annotations.tsfrontend/packages/operator-lifecycle-manager/src/components/index.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-page.tsxfrontend/packages/operator-lifecycle-manager/src/utils.ts
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cardil, fsgreco, logonoff 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 |
|
/retest |
|
The e2e-gcp-console failure is unrelated to this PR's changes. Our fix works correctly — the OperatorHub page loads successfully with 376 operators and no crash (see failure screenshot). Root cause of e2e failure: The The same test fails on other unrelated release-4.16 PRs: #16580, #16599 — confirming it's a pre-existing flake, not caused by this change. |
There is a PR to fix this flakiness: #16608 |
|
/cherrypick release-4.14 |
|
@cardil: 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. |
|
/test e2e-gcp-console |
1 similar comment
|
/test e2e-gcp-console |
|
/label backport-risk-assessed |
|
@cajieh: This PR has been marked as verified by 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. |
|
/test e2e-gcp-console |
|
/test e2e-gcp-console |
|
/retest-required |
|
@cardil: The following test 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. |
Manual backport of #14260 (OCPBUGS-35744) to release-4.16.
Problem
OperatorHub page crashes with
TypeError: V.reduce is not a functionwhen a PackageManifest has malformed JSON inoperators.openshift.io/infrastructure-featuresoroperators.openshift.io/valid-subscriptionannotations. The parsed value is a non-array truthy value, and the?? []fallback doesn't catch it.Fix (minimal, EUS-appropriate)
parseJSONArrayAnnotation()helper that validates the parsed result is actually astring[]before returninginstallModesFor()for missingstatus.channelsgetClusterServiceVersionPlugins()andgetInternalObjects()similarlyThis is a surgical minimal fix rather than a full port of the original refactor (which renamed types, reorganized code, etc.), appropriate for an EUS branch.
/assign cardil
Assisted-by: 🤖 Claude Opus/Sonnet 4.6
Summary by CodeRabbit
Bug Fixes
Improvements
Tests