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

caddyhttp.HandlerErr non-500 status logging #4428

Closed
jjiang-stripe opened this issue Nov 19, 2021 · 5 comments · Fixed by #4429
Closed

caddyhttp.HandlerErr non-500 status logging #4428

jjiang-stripe opened this issue Nov 19, 2021 · 5 comments · Fixed by #4429
Labels
feature ⚙️ New feature or request

Comments

@jjiang-stripe
Copy link
Contributor

Hello! Is there a reason why Caddy only logs handler errors that have 500-level status codes (see

if errStatus >= 500 {
logger.Error(errMsg, errFields...)
}
)? I've run into this failure case before and it wasn't obvious because that error doesn't actually end up being logged because it's a 403. Are there non-500 handler errors that we don't want logged? Otherwise, that seems like it'd be useful information for debugging.

@francislavoie
Copy link
Member

francislavoie commented Nov 20, 2021

Are there non-500 handler errors that we don't want logged?

I would say yes, things like basicauth errors, because those can get very noisy, especially if some bots are making lots of attempts.

We could probably log them at the Debug level though, which would be quiet by default.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Nov 20, 2021
francislavoie added a commit that referenced this issue Nov 20, 2021
Fixes #4428

It's best to still log handler errors at debug level so that they're hidden by default, but still accessible if additional details are necessary.
@jjiang-stripe
Copy link
Contributor Author

hmm ok so follow-up question (sorry if this is basic! just trying to wrap my head around the semantics here): why are unauthorized responses returned as a HandlerErr? looking at

if !authed {
return caddyhttp.Error(http.StatusUnauthorized, fmt.Errorf("not authenticated"))
}
I'm wondering what the difference is between doing something like

http.Error(w, "not authenticated", http.StatusUnauthorized)
return nil

versus returning it as a HandlerErr as it currently does.

@francislavoie
Copy link
Member

francislavoie commented Nov 20, 2021

Handler errors can be recovered from via the handle_errors directive, so you can handle them however you want, e.g. to display an error page of your choice with file_server, or re-proxy to a rewritten URL if you have an error page on your upstream you can render.

This is particularly useful for file_server itself which will emit a 404 handler error, so you can show a "page not found" 404 page. You can also do the same with basicauth and show an "Unauthorized" page instead of re-prompting for basicauth if you prefer.

@jjiang-stripe
Copy link
Contributor Author

ahh gotcha, that makes sense, thank you for explaining!

@mholt
Copy link
Member

mholt commented Nov 22, 2021

(Catching up)

Yeah, so 4xx errors are client errors, not errors in the server, so we don't log those specially. But I guess it makes sense in a debug mode that logging all errors, even client ones, could be useful. Thanks for bringing up the question!

@mholt mholt closed this as completed Nov 22, 2021
mholt pushed a commit that referenced this issue Nov 22, 2021
Fixes #4428

It's best to still log handler errors at debug level so that they're hidden by default, but still accessible if additional details are necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants