Skip to content

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 19, 2024

Fix #6248

The problem is that we were skipping hosts that have "" as their logger name (which happens when using the log directive with no options).

This is a regression from #6088, but the reporter in #6248 had a different problem on 2.7.6 which I think we've since fixed (causing skip_hosts to not work).

Also, I added a SplitHostPort in a couple spots cause I realized during testing that we were using the host with the port when we shouldn't to look up the logger names. This more thoroughly completes the idea from #5881

@francislavoie francislavoie requested a review from mholt April 19, 2024 02:42
@francislavoie francislavoie added the bug 🐞 Something isn't working label Apr 19, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Apr 19, 2024
@francislavoie
Copy link
Member Author

I made an additional change to simplify the logic for reading the logger names config, removes the "retry without port" because really there should never be any config with a port, it makes it ambiguous.

I've also added a check in the HTTP app's Validate() to make sure there's no ports in the config (only relevant for hand-crafted JSON config, Caddyfile produces the right thing already). Technically a small breaking change, but it should ensure users fix their config so it works as expected.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks Francis!

@mholt mholt merged commit 726a9a8 into master Apr 22, 2024
@mholt mholt deleted the fix-default-logger branch April 22, 2024 12:33
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.

logs.skip_hosts is ignored
2 participants