xds: Support UpstreamTlsContext.sni with XdsChannelCredentials

[ejona86]

gRFC A29 added XdsChannelCredentials, but the gRFC explicitly ignored the sni field. @dvilaverde in #11750 needs sni, so this issue was split out to track it. We'll need a small gRFC.

The current implementation is sending sni based on the original target instead of not sending any SNI. That's not good as it could cause a user to unknowingly depend on the incorrect behavior. So we do need to at least disable SNI in the short-term. But it seems the work to disable SNI is most of the work to support sni. (Even when we support sni we need to support disabling SNI; if the field is empty that implies SNI is disabled.)

[ejona86]

gRFC: grpc/proposal#507

[becomeStar]

While looking into the SNI handling for XdsChannelCredentials, I noticed that in ClientSecurityHandler there is still an unconditional fallback to using the channel authority as SNI when isXdsSniEnabled is false:

else {
        sni = grpcHandler.getAuthority();
        autoSniSanValidationDoesNotApply = false;
}

My understanding from gRFC A101 and the commit message (“rather than use the channel authority as the SNI”) is that when using XdsChannelCredentials, SNI is intended to be disabled by default unless explicitly enabled by xDS semantics (or via the temporary env var fallback).

Is this else path intentionally preserving the legacy TLS behavior, or is it an unintended leftover from the transition?

If the latter, would it be reasonable to remove this implicit authority-based fallback to better align the behavior with A101?

I just wanted to confirm the intended semantics here before proposing any changes.

@ejona86 @kannanjgithub — would appreciate your thoughts here, since this seems closely related to the A101 transition and its original implementation.

[kannanjgithub]

isXdsSniEnabled being false means turning off A101 behavior. It was a temporary env variable protection that is already turned to true. It may as well be removed.

[becomeStar]

@kannanjgithub

Thanks, that makes sense.
If this flag(isXdsSniEnabled) and legacy path are no longer needed, would you be open to a small cleanup PR that removes the isXdsSniEnabled guard and the legacy authority-based SNI fallback entirely?

[becomeStar]

Got it, thanks for the clarification.

In that case, I’ll go ahead and prepare a small cleanup PR that removes the isXdsSniEnabled guard and the legacy authority-based SNI fallback, and send it for review shortly.