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

credentials/google: support new-style xDS cluster names #5399

Merged
merged 3 commits into from Jun 6, 2022

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Jun 3, 2022

RELEASE NOTES:

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

@dfawley dfawley added this to the 1.48 Release milestone Jun 3, 2022
@dfawley dfawley requested a review from easwars June 3, 2022 17:44
chi := credentials.ClientHandshakeInfoFromContext(ctx)
if chi.Attributes == nil {
return c.tls.ClientHandshake(ctx, authority, rawConn)
return false
}
cn, ok := internal.GetXDSHandshakeClusterName(chi.Attributes)
if !ok || strings.HasPrefix(cn, cfeClusterNamePrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the second half of this conditional statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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_).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -50,18 +53,36 @@ func newClusterTransportCreds(tls, alts credentials.TransportCredentials) *clust
}
}

func (c *clusterTransportCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
func isXDSNonCFECluster(ctx context.Context) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@easwars easwars assigned dfawley and unassigned easwars Jun 3, 2022
@dfawley dfawley assigned easwars and unassigned dfawley Jun 3, 2022
easwars
easwars approved these changes Jun 6, 2022
@easwars easwars assigned dfawley and unassigned easwars Jun 6, 2022
@dfawley dfawley merged commit ca5cc0b into grpc:master Jun 6, 2022
11 checks passed
@dfawley dfawley deleted the gdcreds branch June 6, 2022 20:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants