OCPBUGS-81512: Use ETag conditional requests for OpenAPI v2 fetching#16457
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references Jira Issue OCPBUGS-81512, 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@rhamilto: This pull request references Jira Issue OCPBUGS-81512, which is valid. The bug has been moved to the POST state. 3 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
openshift-ci
Bot
added
component/core
|
@rhamilto: This pull request references Jira Issue OCPBUGS-81512, which is valid. 3 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. |
📝 WalkthroughWalkthroughThis PR implements ETag-based HTTP caching for Kubernetes OpenAPI specification fetching. The implementation updates 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@frontend/public/module/k8s/__tests__/swagger.spec.ts`:
- Around line 31-37: The test's 304 mock (create304Response) incorrectly sets
ok: true and the assertion checks a new Response instead of the actual mock,
causing a false-positive; update create304Response to set ok: false, keep json:
jest.fn() on the returned object, and change the test assertion in the swagger
spec to verify that the json mock on the returned create304Response instance was
called (i.e., assert the json jest.fn() of the same Response object passed to
fetchSwagger was invoked) so the test truly verifies fetchSwagger called
response.json() on the real 304 response.
In `@frontend/public/module/k8s/swagger.ts`:
- Line 25: The code has a race where cachedETag and swaggerDefinitions are
assigned separately (symbols: cachedETag, swaggerDefinitions), allowing a
request to observe the new ETag but old definitions; fix by coupling them into a
single atomic cache object (e.g., cached = { etag, definitions }) and update all
reads/writes to use that single object so assignments and returns are atomic,
then replace all usages that read cachedETag or swaggerDefinitions (including
the If-None-Match check and the return path) to read from the unified cached
object.
🪄 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: 72b85227-8bda-471f-a35e-d86d5b1e7c69
📒 Files selected for processing (2)
frontend/public/module/k8s/__tests__/swagger.spec.tsfrontend/public/module/k8s/swagger.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/**/*.{ts,tsx,js,jsx}: Never import from package index files (e.g.,@console/shared) in new code, as they can create circular dependencies and slow builds. Import from specific file paths instead.
Do not use backticks int()calls for i18n strings, as the i18n parser cannot extract keys from template literals. Use single or double quotes instead.
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import from deprecated packages or use code with the
@deprecatedTSdoc tag in new code.
**/*.{ts,tsx}: Use React functional components with hooks instead of class components
State Management: Use React hooks and Context API (migrating away from legacy Redux/Immutable.js)
Hooks: Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
API calls: Use k8s resource hooks for data fetching,consoleFetchJSONfor HTTP requests
Extensions: Use console extension points for plugin integration
Types: Check existing types inconsole-sharedbefore creating new ones
Dynamic Plugins: Use console extension points for plugin integration
Styling: Use SCSS modules co-located with components, PatternFly design system components, avoid any SCSS/CSS if possible
Accessibility: Follow WCAG 2.1 AA standards, use semantic HTML, ARIA labels where needed, ensure keyboard navigation, test with screen readers
i18n: UseuseTranslation('namespace')hook withkeyformat for translation keys
Error Handling: Use ErrorBoundary components and graceful degradation patterns
Optimize re-renders: UseuseCallbackfor memoized callbacks to avoid function recreation every render
Optimize re-renders: UseuseMemofor expensive computations to avoid recalculating on every render
Lazy loading: UseReact.lazy()to lazy load heavy components
TypeScript type safety: Avoid usinganytype; suggest proper type definitions and verify null/undefined are handled properly
Type component props properly: Reuse existing component prop types instead of duplicating type definitions
Use proper hooks: Use specialized hooks likeusePluginInfofor plugin data instead of generic data fetching patterns
Avoid deprecated components: Check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefix before using components
Importing from barrel files and circular dependencies: Import directly from specific files instead...
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
frontend/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Never use absolute URLs or paths in the console code. The console runs behind a proxy under an arbitrary path.
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When writing code for static plugins, ensure that all
$codeRefreference the corresponding extension type from the@console/dynamic-plugin-sdkpackage.
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (TESTING.md)
**/*.{tsx,ts}: Always usepage.getByTestId('x')for Playwright selectors which queries[data-test="x"]. If a React element only has a legacy test attribute, adddata-testto the element. Never remove legacy attributes
Preferdata-testattributes in Cypress selectors (e.g.,cy.get('[data-test="create-deployment"]')) over brittle CSS/ARIA selectorsFile Naming: PascalCase for components, kebab-case for utilities,
*.spec.ts(x)for tests
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files to avoid git issues with case-insensitive file systems
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Prefer functional programming patterns and immutable data structures
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code - the app should be able to run behind a proxy under an arbitrary path
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.ts
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Plugin SDK Changes: Any updates to
console-dynamic-plugin-sdkshould aim to maintain backward compatibility as it's a public API - use theplugin-api-reviewskill to vet changes for public API impact and ensure proper documentation updates
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
frontend/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (README.md)
frontend/**/*.{js,ts,tsx}: Support only the latest versions of Edge, Chrome, Safari, and Firefox browsers; IE 11 and earlier are not supported
CSP violations should be automatically reported to telemetry by parsing dynamic plugin names from securitypolicyviolation events, with throttling to prevent duplicate reports within a day
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
For dynamic translation keys that cannot be parsed by i18next-parser (t(key), t('key' + id), t(
key${id})), specify possible static values in comments for the parser to extract
Files:
frontend/public/module/k8s/swagger.tsfrontend/public/module/k8s/__tests__/swagger.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Tests should follow a similar 'test tables' convention as used in Go where applicable
Files:
frontend/public/module/k8s/__tests__/swagger.spec.ts
🔀 Multi-repo context openshift/console-operator
Linked repositories findings
openshift/console-operator
- Kubernetes API server and kube-openapi handlers set and compute ETag and honor If-None-Match / 304 behaviour:
- vendor/k8s.io/kube-openapi/pkg/handler3/handler.go — computeETag and ETag-related comments/logic. [::openshift/console-operator::vendor/k8s.io/kube-openapi/pkg/handler3/handler.go]
- vendor/k8s.io/kube-openapi/pkg/handler/handler.go — computeETag usage and ETag comments. [::openshift/console-operator::vendor/k8s.io/kube-openapi/pkg/handler/handler.go]
- vendor/k8s.io/apiserver/pkg/endpoints/discovery/aggregated/etag.go — ServeHTTPWithETag, sets ETag header and returns 304 when If-None-Match matches. [::openshift/console-operator::vendor/k8s.io/apiserver/pkg/endpoints/discovery/aggregated/etag.go]
- vendor/k8s.io/apiserver/pkg/endpoints/discovery/aggregated/peer_aggregated_handler.go — cachedResponseETag usage. [::openshift/console-operator::vendor/k8s.io/apiserver/pkg/endpoints/discovery/aggregated/peer_aggregated_handler.go]
(Repository inspected: file list count and README/go.mod shown.)
Conclusion: server-side components in this repo already compute and expose ETag and support If-None-Match/304 semantics for discovery/OpenAPI endpoints — relevant positive signal for the PR's switch to conditional requests.
🔇 Additional comments (2)
frontend/public/module/k8s/swagger.ts (2)
3-3: LGTM!
29-29: Strong caching strategy for large cluster payloads.The ETag-based conditional request flow correctly optimizes repeated OpenAPI fetches:
- Explicit
Accept: application/jsonheader clarifies expected response format.- Relative path and
coFetchusage follow console proxy conventions.- Graceful fallback when server omits ETag (sets
null, next request sends noIf-None-Match).- Error handling and event dispatch preserve existing semantics.
This addresses the 50MB+ repeated download problem on clusters with 1000+ CRDs. Once the ETag/definitions synchronization issue is resolved, this will deliver significant bandwidth and parse-time savings.
Also applies to: 33-33, 37-42, 45-46
The console fetches the full OpenAPI v2 schema (~50MB+ on large clusters) at startup, after API discovery, and every 5 minutes without HTTP conditional request headers. This causes repeated full downloads of an unchanged payload. Switch from coFetchJSON to coFetch to access response headers, cache the ETag from successful responses, and send If-None-Match on subsequent requests. When the server returns 304 Not Modified, return the cached definitions without re-downloading or re-parsing the payload. Also removes the legacy localStorage cleanup code that was marked as safe to remove in a future release. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QA Verification Evidence
Verification Steps
Network Analysis: ETag Caching BehaviorThe key change in this PR is adding Baseline (
Candidate (
CRD Cache Invalidation TestCreated a test CRD (
Unit Tests8 of 9 tests pass. 1 test fails: Failing test: Note Minor bug: The Visual Regression CheckNo visual regressions detected. Screenshots are identical between baseline and candidate (expected — this is a logic-only change). Animated overview (click to expand)
Step 1: Dashboard overview (pass)
Step 2: API Explorer resource list (pass)
Step 3: Pod API resource detail (pass)
Step 4: Pod list page (pass)
Step 5: YAML editor with schema validation (pass)
Step 6: Network ETag evidence (pass)Candidate-only — showing the dashboard page where network requests were monitored: 
Console ErrorsNo new console errors introduced by this change. Both baseline and candidate show the same 3 pre-existing cluster-specific errors:
Warning This verification was performed by an AI agent. Results may contain false positives or miss Automated QA verification by Claude Code |
Co-authored-by: logonoff <git@logonoff.co>
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
/verified by claude |
openshift-ci-robot
added
the
verified
|
@logonoff: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto 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 |
|
/cherry-pick release-4.22 |
|
@rhamilto: 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. |
|
@rhamilto: all tests passed! 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. |
|
@rhamilto: Jira Issue Verification Checks: Jira Issue OCPBUGS-81512 Jira Issue OCPBUGS-81512 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
@rhamilto: #16457 failed to apply on top of branch "release-4.22": 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. |
4 participants
Analysis / Root cause:
The console fetches the full OpenAPI v2 schema (
/api/kubernetes/openapi/v2) at startup, after every API discovery cycle, and on a 5-minute polling interval — all without HTTP conditional request headers (ETag/If-None-Match). On clusters with 1000+ CRDs, the OpenAPI spec grows to 50MB+, resulting in repeated full downloads of an unchanged payload.The
fetchSwagger()function usedcoFetchJSON, which discards theResponseobject (and its headers) after parsing JSON. This made it impossible to capture theETagheader for conditional requests.Solution description:
coFetchJSONtocoFetchto access response headersETagheader from each successful responseIf-None-Matchwith the cached ETag on subsequent requests304 Not Modified, return the already-cached definitions without re-downloading or re-parsing the payloadlocalStoragecleanup code that was marked as safe to remove in a future releaseScreenshots / screen recording:
Test setup:
A running OpenShift cluster. No special setup required for unit tests (
yarn test public/module/k8s/__tests__/swagger.spec.ts).Test cases:
ETagheader present, definitions returnedIf-None-Matchheader sent, 304 response, no payload transferred, cached definitions returnedIf-None-Matchsent, behaves same as beforeBrowser conformance:
Additional info:
Jira: https://redhat.atlassian.net/browse/OCPBUGS-81512
Summary by CodeRabbit