crc-org / crc

CRC is a tool to help you run containers. It manages a local OpenShift 4.x cluster, Microshift or a Podman VM optimized for testing and development purposes
https://crc.dev
Apache License 2.0
1.26k stars 242 forks source link

fix (daemon) : Add logging to provide additional information for non-200 status codes (#3766) #4467

Open rohanKanojia opened 2 days ago

rohanKanojia commented 2 days ago

Fixes: Issue #3766

Relates to: Issue #3766

Solution/Idea

Add a custom response writer to capture the response body and status code so that we can conditionally log the response body in case of failure.

Proposed changes

  1. Add CustomResponseWriter object as a wrapper around http.ResponseWriter and override methods to capture response status code and body.
  2. Add interceptResponseBodyMiddleware that would inject the abovementioned response writer into HTTP Handlers.
  3. Use interceptResponseBodyMiddleware in daemon endpoint declarations and add handler for conditionally logging response body when response is not successful

Testing

What is the bottom-line functionality that needs testing? Describe in pseudo-code or in English. Use verifiable statements that tie your changes to existing functionality.

  1. Run crc daemon to start CRC daemon
  2. Make some requests to the daemon API endpoints, so that they fail. You would notice that there is some additional logging whenever response code is not 200.

For example if we make requests like these

# For failed requests, there would be additional logging of response body
curl -i --unix-socket ~/.crc/crc-http.sock http://crc/api/stop
curl -i --unix-socket ~/.crc/crc-http.sock http://crc/api/start
curl -i --unix-socket ~/.crc/crc-http.sock http://crc/api/delete

# For successful requests, it won't log
curl -i --unix-socket ~/.crc/crc-http.sock http://crc/api/version

We see these logs (notice there is no logging in case of successful request done in the end) :

ERRO [19/Nov/2024:22:11:06 +0530] "GET /api/stop" Response Body: Instance is already stopped

@ - - [19/Nov/2024:22:11:06 +0530] "GET /api/stop HTTP/1.1" 500 28

ERRO [19/Nov/2024:22:11:00 +0530] "GET /api/start" Response Body: lstat /home/rokumar/.crc/bin/crc: no such file or directory

@ - - [19/Nov/2024:22:11:00 +0530] "GET /api/start HTTP/1.1" 500 60

ERRO [19/Nov/2024:22:08:44 +0530] "GET /api/delete" Response Body: Cannot load machine: no such libmachine vm: crc

@ - - [19/Nov/2024:22:08:44 +0530] "GET /api/delete HTTP/1.1" 500 48

@ - - [19/Nov/2024:22:12:33 +0530] "GET /api/version HTTP/1.1" 200 101
openshift-ci[bot] commented 2 days ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign gbraad for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/crc-org/crc/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 2 days ago

Hi @rohanKanojia. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
praveenkumar commented 1 day ago

/assign @anjannath