A118: TLS Telemetry#550
Conversation
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
This looks good. I'll delay giving my Approval until others have, and then I'll Approve+merge when it seems there's concensus.
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks good overall! My comments are all relatively minor.
Please let me know if you have any questions.
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Did we want to find someone from otel to review the propsed metrics before we implement this? I don't think any of us are experts in this area.
| understanding authentication behavior. An enum describing the type of resumption | ||
| used (or none) will be created - it can be extended in the future to make |
There was a problem hiding this comment.
This says "or none" but there is no corresponding NONE enum value below?
There was a problem hiding this comment.
FULL_HANDSHAKE means no resumption was done - should we rename to NO_RESUMPTION?
There was a problem hiding this comment.
I don't feel too strongly about the names, but it might want a comment. But what exactly does "RESUMED_HANDSHAKE" actually mean, relative to the other future things you suggested could be added in the future like "ticket-based resumption vs. session-based resumption"? Is "RESUMED_HANDSHAKE" actually "SESSION_RESUMPTION"?
There was a problem hiding this comment.
RESUMED_HANDSHAKE means that it was resumed, with no specificity on session or ticket
| if transportCreds != nil { | ||
| + isTLS := transportCreds.Info().SecurityProtocol == "tls" | ||
| conn, authInfo, err = transportCreds.ClientHandshake(connectCtx, addr.ServerName, conn) | ||
| + if isTLS { |
There was a problem hiding this comment.
Other thought: do we want to read the authInfo returned by the handshake to determine whether TLS was involved (I guess cast it to TLSInfo?) or trust the creds string directly? I don't know that it matters at all, but it's something to ponder, perhaps.
There was a problem hiding this comment.
FYI: Both the tls creds and xds creds today set this field to "tls". Similarly, both of them set AuthInfo to TLSInfo. Maybe the idea was always to include xds creds as well, in which case we are good.
There was a problem hiding this comment.
Either way - I don't see a reason using XDS creds should preclude folks from monitoring - if it's doing TLS and we have TLS monitoring, the user can get monitoring?
As for how we want to see if it's TLS - can we discuss on the implementation PR since it's more of an impl detail? This is meant more as a demonstrative snippet, not a code source of truth.
|
@mbissa, can you look at this from the metrics definition perspective (did we define everything we needed to define)? |
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks good, modulo updating the C++ implementation details.
| @@ -185,7 +185,7 @@ In the few use-cases where TSI is not called via gRPC, we will ensure that | |||
| metric incrementation is not performed. | |||
|
|
|||
| // TODO: Update this comment to be more accurate based on the impl PR | |||
6 participants
The gRFC to support authentication telemetry in the TLS stack for handshake and TLS offload operation monitoring.