credentials/google: support new-style xDS cluster names

[dfawley]

RELEASE NOTES:

• credentials/google (for xds): support xdstp C2P cluster names

[easwars]

Do we need the second half of this conditional statement?

[dfawley]

I don't understand. We will say it's not an xDS cluster if we can't find the cluster name in the attributes (!ok). And we will say it's xDS but also CFE if it matches the second half of the conditional (starts with google_cfe_).

[dfawley]

OH, sorry I see it is repeated below. I think that's because I combined C's code with what we already had. I will delete the duplicate if HasPrefix below.

[easwars]

This negation is making it harder for me to follow this code:

Do you think something like this would be simpler? I'm not completely convinced that this easier to read either.

But maybe adding a comment as to when a clusterName embedded in the context is considered a CFE cluster (and hence the use of TLS) and when it is considered a directPath cluster (and hence the use of ALTS) would be helpful.

// Returns clusterName stored as an attribute in the context. Returns empty string if
// the associated attribute is missing.
func clusterNameFromContext(ctx context.Context) string {
	chi := credentials.ClientHandshakeInfoFromContext(ctx)
	if chi.Attributes == nil {
		return ""
	}
	return internal.GetXDSHandshakeClusterName(chi.Attributes)
}

// isDirectPathCluster returns true if the clusterName embedded in the context
// corresponds to a directPath cluster, and hence signifies the use of ALTS creds.
func isDirectPathCluster(ctx context.Context) bool {
	cn := clusterNameFromContext(ctx)
	if cn == "" {
		return true
	}
	if strings.HasPrefix(cn, cfeClusterNamePrefix) {
		return false
	}
	if !strings.HasPrefix(cn, "xdstp:") {
		return false
	}
	u, err := url.Parse(cn)
	if err != nil {
		// Shouldn't happen, but assume ALTS.
		return true
	}
	return u.Host == cfeClusterAuthorityName && strings.HasPrefix(u.Path, cfeClusterResourceNamePrefix)	
}

func (c *clusterTransportCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
	if isDirectPathCluster(ctx) {
		return c.alts.ClientHandshake(ctx, authority, rawConn)
	}
	return c.tls.ClientHandshake(ctx, authority, rawConn)
}

[dfawley]

This negation is making it harder for me to follow this code:

I had the same feeling, but this follows the convention started in C: grpc/grpc#29764

[dfawley]

adding a comment as to when a clusterName embedded in the context is considered a CFE cluster (and hence the use of TLS) and when it is considered a directPath cluster (and hence the use of ALTS) would be helpful.

There is a comment where it is used, btw:

// If attributes have cluster name, and cluster name is not cfe, it's a
// backend address, use ALTS.