Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace API Debug Middleware #631

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace API Debug Middleware #631

wants to merge 1 commit into from

Conversation

DanielCosme
Copy link
Contributor

@DanielCosme DanielCosme commented Nov 13, 2024

This MR tries to address a problem when using Enduro with the API in Debug mode.

The Debug middle-ware prints to stdout details about the request, including the bodies of both the request and response. This is usually what we want, except when we are downloading binary files (like compressed AIPs), when the Download package endpoint is hit with download request, the Download fails (if debug = true) while the payload is printed (a binary blob) to stdout. In this usecase the download fails (bytes get dropped consistently).

To solve this issue I decided to avoid printing the response body when the Content-Type is of: application/x-7z-compressed. Since the Debug middleware belongs to the Goa library I could not modify it, I decided to copy the exact middleware and modify only what I needed.

So, this MR is almost a perfect copy of this file here: https://github.com/goadesign/goa/blob/v3/http/middleware/debug.go

With the difference that we just avoid printing the Body if the condition is meet.

@@ -90,7 +90,7 @@ func HTTPServer(
handler = versionHeaderMiddleware(config.AppVersion)(handler)
if config.Debug {
handler = goahttpmwr.Log(loggerAdapter(logger))(handler)
handler = goahttpmwr.Debug(mux, os.Stdout)(handler)
handler = Debug(mux, os.Stdout)(handler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to wrap goahttpmwr.Debug with an additional layer that checks the content type and conditionally intercepts the request? If the request content type indicates a large body, you could temporarily replace the body with an empty reader, allowing it to pass through the middleware without printing the full body. After the middleware processes the request, then reassign the original body for further processing. Not sure if that's possible but it seems doable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func conditionalDebug(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if r.Header.Get("Content-Type") == "..." {
            originalBody := r.Body
            r.Body = io.NopCloser(io.LimitReader(originalBody, 0))
            next.ServeHTTP(w, r)
            r.Body = originalBody
        } else {
            next.ServeHTTP(w, r)
        }
    })
}

handler = conditionalDebug(goahttpmwr.Debug(mux, os.Stdout)(handler))

Copy link
Contributor Author

@DanielCosme DanielCosme Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really not that simple because this middleware:

  • Intercepts the (outgoing request), writes info to the buffer
  • calls next.ServeHTTP (in the middle of the function)
  • When the ServerHTTP returns then it works on the response.
  • Writes to the buffer the rest of the data (including the body)

And the Content-Type I check is the response one, not the request.

Copy link
Member

@sevein sevein Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, sorry I missed that! Let me know if this other option works for you. This time, I used a different criteria: if the size of the debugging payload exceeds 16 KiB, then dump it into a file instead - but you could tweak the criterion as needed.

// middleware.go

// debugMiddleware is a wrapper around goahttpmwr.Debug that logs the output to
// a temporary file if it exceeds maxSize.
func debugMiddleware(mux goahttp.Muxer, w io.Writer) func(http.Handler) http.Handler {
	const maxSize = 16 * 1024
	return func(h http.Handler) http.Handler {
		return http.HandlerFunc(func(wr http.ResponseWriter, r *http.Request) {
			buf := &bytes.Buffer{}
			goahttpmwr.Debug(mux, buf)(h).ServeHTTP(wr, r)
			if buf.Len() < maxSize {
				io.Copy(w, buf)
			} else {
				tmpfile := "/tmp/debug.12345.log" // TODO: write to a temporary file.
				io.WriteString(w, fmt.Sprintf("\n [!] Response too large to be logged. Writing to %q.\n", tmpfile))
			}
		})
	}
}

Tests: https://go.dev/play/p/N8QDLTkZbBA.

I'd prefer a solution that doesn't require us to vendor all that code if possible.

Copy link
Contributor Author

@DanielCosme DanielCosme Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution is better but it still does not fit the optimal solution for the client. Fundamentally the client wants be able to Debug the API when invoking the Download package endpoint, and by Debug we mean to be able to show request/response headers, params, status, etc.. without printing the response body (because it's a binary blob, we don't care about that).

Something like this would be the expected output in stdout:

> [VVbQVl5C] GET /collection/3/download
> [VVbQVl5C] Accept: */*
> [VVbQVl5C] User-Agent: curl/8.10.1
< [VVbQVl5C] OK
< [VVbQVl5C] Content-Disposition: attachment; filename="transfer_2_1.zip-830237d2-d82b-4943-b561-7fccac2ada1f.7z"
< [VVbQVl5C] Content-Length: 200895757
< [VVbQVl5C] Content-Type: application/x-7z-compressed
< [VVbQVl5C] X-Enduro-Version: (dev-version)

Note that we print all information, skipping only the body being printed (but successfully downloaded).

Your current proposal will either print all the Debug information (including the body) or nothing at all. Basically the io.Writer that the middleware takes will write everything. That is why I decided to fork/vendor this specific middleware, I could not see how to achieve the objective without that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants