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

atls: improve logging #1283

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

atls: improve logging #1283

wants to merge 2 commits into from

Conversation

3u13r
Copy link
Member

@3u13r 3u13r commented Mar 10, 2025

During debugging of the latest SNI issue I had to add more logging in my dev state. It would be best if customers could enable this without code changes to make debugging easier.

@3u13r 3u13r requested a review from burgerdev as a code owner March 10, 2025 22:11
@3u13r 3u13r requested review from katexochen and burgerdev and removed request for burgerdev March 10, 2025 22:11
@3u13r 3u13r added the no changelog PRs not listed in the release notes label Mar 10, 2025
Copy link
Contributor

@burgerdev burgerdev left a comment

Choose a reason for hiding this comment

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

Sunds helpful and lgtm overall

@@ -66,6 +75,7 @@ func (c *Credentials) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.A

conn := tls.Server(rawConn, serverCfg)
if err := conn.HandshakeContext(ctx); err != nil {
c.logger.DebugContext(ctx, "Handshake error", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this Debug and not Error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because you get a EOF error here every 5 seconds due to the readiness check. Example message:

time=2025-03-11T12:00:20.766Z level=DEBUG msg="Handshake error" atlscredentials.error=EOF

Since this is also confusing for the user when enabling debug logs, we might want to add an explainer in the message or omit EOFs. Though the second one has a obvious downside of also missing all other EOF errors. Any preference?

@3u13r 3u13r force-pushed the euler/feat/atlscredetials-logging branch from 972e962 to 3e4f6e2 Compare March 11, 2025 12:07
@katexochen katexochen added this to the v1.6.0 milestone Mar 11, 2025
@3u13r 3u13r requested a review from burgerdev March 11, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants