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

logging: Fix skip_hosts with wildcards #5102

Merged
merged 1 commit into from Oct 5, 2022
Merged

Conversation

francislavoie
Copy link
Member

Fix #4859

See #4859 (comment) for an explanation of the bug.

The fix was simple, really, we just need to reorder the loops in server.go to return true early if a logger is explicitly configured for a hostname.

While testing it though, I noticed that there's a bug in the Caddyfile adapter when using the log directive with no options (i.e. log to the default logger); it wasn't adding those hosts to the logger_names config, so the wildcard skip_hosts would still cause that to get skipped when it was explicitly configured to log.

Basically, I removed some code because it was trying to be "too smart" with omitting some config in the output JSON for "cleanliness" but that just causes edgecase bugs.

Also a side benefit is that logger checks should be slightly faster now (like... nanoseconds at best) because we check a map in O(1) before looping over skip_hosts which is O(n).

Also set up a const in the caddy package for the default logger name, because we use that in lots of places.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Oct 1, 2022
@francislavoie francislavoie added this to the v2.6.2 milestone Oct 1, 2022
@francislavoie francislavoie force-pushed the fix-logging-skip-hosts branch 2 times, most recently from 1ba3362 to a928c80 Compare October 1, 2022 01:18
@mholt
Copy link
Member

mholt commented Oct 5, 2022

Huh, thanks! I like fixes that delete code. Thanks for correcting my mistake. 😁 👏

@mholt mholt merged commit 99ffe93 into master Oct 5, 2022
@mholt mholt deleted the fix-logging-skip-hosts branch October 5, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG?] Access logs for sub-domains are not written after Apr 26 update
2 participants