fix(inspect): always close CRL response body in checkCRLValidCert#466
fix(inspect): always close CRL response body in checkCRLValidCert#466alliasgher wants to merge 1 commit into
Conversation
checkCRLValidCert calls http.DefaultClient.Do and then reads the body with io.ReadAll. The explicit resp.Body.Close() only runs after the ReadAll succeeds; if io.ReadAll returns an error the function returns early and the body is never closed, leaking the underlying TCP connection. Replace the explicit close with a defer immediately after the successful Do(), so both the success and the read-error path close the body. Fixes cert-manager#442 Signed-off-by: alliasgher <alliasgher123@gmail.com>
cert-manager-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
cert-manager-prow
Bot
added
the
needs-ok-to-test
|
Hi @alliasgher. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
cert-manager-prow
Bot
added
the
size/XS
|
/ok-to-test |
cert-manager-prow
Bot
added
ok-to-test
and removed
needs-ok-to-test
There was a problem hiding this comment.
Pull request overview
Fixes a resource leak in the CRL validation path by ensuring HTTP response bodies are always closed, even when reading the body fails (addresses #442).
Changes:
- Add
defer resp.Body.Close()immediately after a successfulhttp.DefaultClient.Do(req)incheckCRLValidCert. - Remove the explicit
resp.Body.Close()that only executed on the happy path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error reading HTTP body: %w", err) |
There was a problem hiding this comment.
Consider adding a unit test to lock in the intended behavior that the HTTP response body is closed on the error path (e.g., when the response body reader returns an error during read). Since this function uses http.DefaultClient, you can temporarily swap the client's Transport in the test to return a Response with a custom ReadCloser that tracks whether Close() was called, then assert it was closed even when the read fails.
|
Thanks @alliasgher. I'll merge #463 instead, which is an older PR. /close |
|
@erikgb: Closed this PR. 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. |
Summary
checkCRLValidCertinpkg/inspect/secret/util.gocallshttp.DefaultClient.Doand then reads the body withio.ReadAll. The explicitresp.Body.Close()only runs after theReadAllsucceeds; ifio.ReadAllreturns an error the function returns early and the body is never closed, leaking the underlying TCP connection.This PR replaces the explicit close with a
defer resp.Body.Close()immediately after the successfulDo()call, so both the success and the read-error path close the body. Behaviour on the happy path is unchanged.Fixes #442