Skip to content

ACM-39212 fix(backend): use GET /api with body drain instead of HEAD [release-2.16] - #6597

Open
Randy424 wants to merge 3 commits into
stolostron:release-2.16from
Randy424:ACM-38826-token-validation-fix-release-2.16
Open

ACM-39212 fix(backend): use GET /api with body drain instead of HEAD [release-2.16]#6597
Randy424 wants to merge 3 commits into
stolostron:release-2.16from
Randy424:ACM-38826-token-validation-fix-release-2.16

Conversation

@Randy424

@Randy424 Randy424 commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Fixes 405 regression from the original HEAD /api approach — HEAD /api is rejected by some cluster API server / proxy configurations
  • Switches isAuthenticated() to GET /api with explicit body drain (.resume())
  • Preserves the memory leak fix: /api response is ~200 bytes (vs several MB for /apis), drained immediately so sockets return to the keepAlive pool

Changes

  • backend/src/lib/token.ts: GET /api with body drain, returns Promise<number>
  • 12 test files: nock interceptors updated from .head('/api') to .get('/api')

Test plan

  • All backend unit tests pass (261 tests)

Backport of #6600 to release-2.16

🤖 Generated with Claude Code

@openshift-ci

openshift-ci Bot commented Jul 28, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: stolostron/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a3b82415-cf80-4775-a09d-f52dd9c5806a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@Randy424
Randy424 force-pushed the ACM-38826-token-validation-fix-release-2.16 branch from 17774ac to f70b4e5 Compare July 29, 2026 02:08
@Randy424

Randy424 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/test unit-tests-sonarcloud

1 similar comment
@Randy424

Randy424 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/test unit-tests-sonarcloud

@Randy424 Randy424 changed the title ACM-38826 fix(backend): use HEAD /api for token validation [release-2.16] ACM-38826 fix(backend): use GET /api with body drain instead of HEAD [release-2.16] Jul 29, 2026
@Randy424 Randy424 changed the title ACM-38826 fix(backend): use GET /api with body drain instead of HEAD [release-2.16] ACM-39212 fix(backend): use GET /api with body drain instead of HEAD [release-2.16] Jul 30, 2026
Randy424 and others added 3 commits August 16, 2026 21:53
@Randy424 @claude
ACM-38826 fix(backend): use HEAD /api for token validation to elimina…
1c3cbe5 
…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>
@Randy424 @claude
ACM-38826 fix(backend): use GET /api with body drain instead of HEAD
9f0f675 
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>
@Randy424 @claude
ACM-38826 fix(backend): remove blank lines left by sed comment strip
4d81762 
Signed-off-by: Randy Bruno Piverger <21374229+Randy424@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Randy424
Randy424 force-pushed the ACM-38826-token-validation-fix-release-2.16 branch from fe827df to 4d81762 Compare August 17, 2026 04:55
@Randy424

Randy424 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest release-2.16 to resolve the merge conflict.

Conflict: backend/test/routes/ansibletower.test.ts — release-2.16 had since picked up the glasswing RBAC-secret fix, which added nockCredentialSecret(TOWER_HOST) calls to several tests still mocking the auth check. This PR's first two commits (original HEAD /api attempt, then the GET /api correction) each touched the same test blocks, so both replayed with conflicts; the third (prettier cleanup) applied cleanly.

Resolution: Same approach used for the release-2.14 (#6671) and release-2.15 (#6598) backports, itself sourced from how main resolved this code: for each conflicting test, kept upstream's new nockCredentialSecret additions and applied only this PR's intended line-level change (mock method) on top.

Additional fix: A handful of .get('/apis') auth-check mocks elsewhere in the same file weren't touched by either commit (new tests release-2.16 added independently) but would have failed silently once isAuthenticated() calls GET /api — nock.disableNetConnect() is on, so an unmatched request throws instead of returning the mocked response. Converted those too for consistency.

Verified: all 266 backend tests pass, prettier/lint/tsc all clean.

@sonarqubecloud

sonarqubecloud Bot commented Aug 17, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

approved dco-signoff: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Randy424