ACM-38826 fix(backend): use GET /api with body drain instead of HEAD [release-2.15] - #6598
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: stolostron/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Randy424 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 |
d84a76b to
dbabbc9
Compare
|
/retest |
Randy424
changed the title
|
…te response body drain (stolostron#6537) getAuthenticatedToken() called isAuthenticated() on every authenticated request, which issued GET /apis to the kube API. The response body (up to several MB on CRD-heavy clusters) was never consumed on the success path, preventing the socket from returning to the keepAlive pool. Under sustained load, native (external) memory accumulated proportionally to request volume. Replace GET /apis with HEAD /api: - HEAD responses have no message body by HTTP spec — nothing to drain - /api (core group) is ~200 bytes of headers; it does not grow with CRDs - Returns HTTP status so callers preserve 401/403/5xx distinctions - No client-side caching required: OpenShift oauth-apiserver caches valid tokens ~30 seconds server-side; failures are not cached isAuthenticated() now returns Promise<number> (HTTP status) instead of Promise<Response> so callers preserve upstream status codes. authenticated.ts updated accordingly. All route tests updated to mock HEAD /api instead of GET /apis. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-authored-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
HEAD /api returns 405 on clusters where the API server or proxy chain rejects HEAD requests, causing every auth check to fail. Switch to GET /api with explicit body drain — the response is ~200 bytes (core API group only), drained immediately so the socket returns to the keepAlive pool. This preserves the memory optimization while restoring compatibility. Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ba4599b to
1406bd1
Compare
|
Rebased onto the latest Conflict: Resolution: Followed the same approach used for the release-2.14 backport (#6671), which itself mirrored how Additional fix: Several Verified: all 227 backend tests pass, lint clean. |
|
@Randy424: 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. |
1 participant
Summary
HEAD /apiis rejected by some cluster API server / proxy configurationsisAuthenticated()toGET /apiwith explicit body drain (.resume())/apiresponse is ~200 bytes (vs several MB for/apis), drained immediately so sockets return to the keepAlive poolChanges
backend/src/lib/token.ts: GET /api with body drain, returnsPromise<number>.get('/apis')to.get('/api')Test plan
Backport of #6600 to release-2.15
🤖 Generated with Claude Code