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

autohttps: Move log WARN to INFO, reduce confusion #6185

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

Conversation

francislavoie
Copy link
Member

We've seen some users be confused about warnings surrounding Auto HTTPS in the logs. I don't think these need to be warnings, it doesn't really add much value. It's just tracing through whether things are enabled or not, it's not really an actionable problem.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Mar 21, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Mar 21, 2024
@mholt
Copy link
Member

mholt commented Mar 21, 2024

I mean, it is kind of important though, isn't it? Caddy has automatic security features that have been overridden, so they won't be enabled. I feel like that deserves to be a little louder than normal.

@francislavoie
Copy link
Member Author

If things are disabled, it was clearly the intent of the user, so there's no need to warn. If things didn't get enabled, that's also not a problem, it's just "working as intended".

@mholt
Copy link
Member

mholt commented Mar 29, 2024

If things are disabled, it was clearly the intent of the user

My only concern with that is, what if it wasn't their intent?

Like, imagine a light switch on a wall, like a 2-gang switch. It's easy to accidentally switch the wrong one (or both, when only one was intended). Similarly in a web server config a user might unintentionally disable security features (for example, maybe they forgot that they moved the HTTP port to a different number).

Other than that, I don't have a strong preference, but I do feel like a warning is better...

@francislavoie
Copy link
Member Author

How could it not be their intent to use auto_https off? How do you accidentally configure that?

Either way, it's not like we're not logging it anymore. It's still INFO and visible, it's just less confusing because it's not actually a problem it's just a mention of what Caddy decided to do based on the circumstances.

@mholt
Copy link
Member

mholt commented Mar 29, 2024

How do you accidentally configure that?

Imagine you set http_port 1234 for testing locally or something. And your site is foo:1234, it will be served over HTTP since it's on the HTTP port. You might forget you have http_port set, or it might not be obvious. (That's one downside of Caddy's implicit config logic.)

If there's confusion about these logs, maybe we should enhance the verbiage.

@francislavoie
Copy link
Member Author

francislavoie commented Mar 30, 2024

That config reads like pretty clear intent to me. I don't see how you could accidentally do that.

@mholt
Copy link
Member

mholt commented Apr 1, 2024

It's like I was saying, they intentionally set it at first, but later forgot it was set, and expected their site to then be served over HTTPS, but it wasn't.

Anyway, the example isn't the point.

I think things that disable security should still be slightly elevated for now. I'd be open to clarifying the wording, however. Have any suggestions?

@mholt mholt removed this from the v2.8.0 milestone Apr 11, 2024
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.

None yet

2 participants