fix: Close response body when watch content-type negotiation fails#140467
fix: Close response body when watch content-type negotiation fails#140467karlkfi wants to merge 2 commits into
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: karlkfi 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 |
Extend TestWatchUnknownContentType to track response body Close calls using the existing readTracker helper, and hold the watch stream open in the handler so the client must return the negotiation error without waiting for the stream to end. When content-type negotiation fails, Request.Watch returns an error without closing the response body, leaking the connection. This test fails without the fix in the following commit. Originally identified as a TODO in kubernetes#131505.
When Request.Watch receives a 200 response but the negotiator has no stream decoder for the response content type, the error was returned without closing the response body, leaking the connection. Close the body without draining it: a watch response can be a stream held open by the server, so draining could block indefinitely, and the connection cannot be reused anyway. Originally identified as a TODO in kubernetes#131505.
fc0310a to
d89d9bb
Compare
What type of PR is this?
/kind bug
/sig api-machinery
What this PR does / why we need it:
When
Request.Watchgets a 200 response but the negotiator has no stream decoder for the response content type,newStreamWatcherreturns the error without closingresp.Body, leaking the connection.The fix closes the body on that error path without draining it: a watch response can be a stream held open by the server, so draining could block indefinitely, and the connection cannot be reused anyway. The test holds the stream open to pin that Watch returns promptly.
Structured as two commits: the first extends
TestWatchUnknownContentTypewith the in-packagereadTrackerclose counter and fails without the fix, the second is the fix. Test output without the fix:With the fix,
-raceand stress are clean:Which issue(s) this PR fixes:
Fixes #140464
Special notes for your reviewer:
Follow-up to #131706, which cleaned up response body handling in the watch tests. Originally identified as a TODO in #131505.
/cc @liggitt @jpbetz
Does this PR introduce a user-facing change?