webtransport: add custom resolver to add SNI #1761
Conversation
| } | ||
| return true | ||
| }) | ||
| return sni, foundSniComponent |
There was a problem hiding this comment.
Nit:
| return sni, foundSniComponent | |
| return |
There was a problem hiding this comment.
I know we can do this, but I'm generally not a fan. Do you feel strongly?
There was a problem hiding this comment.
No strong feelings. I usually prefer the naked return when using the variables from the function the function signature, but I'm not sure if that's an actual Go convention.
There was a problem hiding this comment.
https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_naked_returns
And yes, I know the section right before discourages named return values. I think it's better to keep them so I can reference them in the comment so it's clear from the signature. Maybe returning a struct would be better?
This probably isn't worth the bike-shedding.
| // foundSniComponent will be true. If there's no SNI component, but there is a | ||
| // DNS-like compoent, then that will be returned for the sni and | ||
| // foundSniComponent will be false (since we didn't find an actual sni component). | ||
| func extractSNI(maddr ma.Multiaddr) (sni string, foundSniComponent bool) { |
There was a problem hiding this comment.
Do we need the bool here? Would an empty string be sufficient? As far as I know, there's no way to distinguish between empty string and unset in the TLS ClientHello: https://github.com/golang/go/blob/d74bf73be052851e83fb59a40f47d49f4b890ca3/src/crypto/tls/handshake_messages.go#L124-L135.
I think the more idiomatic variable name for the bool would be ok.
There was a problem hiding this comment.
Did you read the comment? I know this is a bit confusing and I was hoping the comment would explain it.
There was a problem hiding this comment.
Not carefully enough, apparently ;)
marten-seemann
changed the title
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
MarcoPolo
force-pushed
the
marco/webtransport-resolver
branch
from
9bd7fb9
to
afc0d84
Compare
The continuation of #1719 for webtransport. Fixes #1760