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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c95543
credentials/google: support new-style xDS cluster names
dfawley Jun 2, 2022
b26d653
remove duplicate check
dfawley Jun 3, 2022
dfb5b3b
rename refactor
dfawley Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions credentials/google/google_test.go
View file
Open in desktop
Expand Up @@ -122,6 +122,22 @@ func (s) TestClientHandshakeBasedOnClusterName(t *testing.T) {
// CFE should use tls.
wantTyp: "tls",
},
{
name: "with xdstp CFE cluster name",
ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://traffic-director-c2p.xds.googleapis.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes,
}),
// CFE should use tls.
wantTyp: "tls",
},
{
name: "with xdstp non-CFE cluster name",
ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, "xdstp://other.com/envoy.config.cluster.v3.Cluster/google_cfe_bigtable.googleapis.com").Attributes,
}),
// non-CFE should use atls.
wantTyp: "alts",
},
}
for _, tt := range tests {
t.Run(bundleTyp+" "+tt.name, func(t *testing.T) {
Expand Down
33 changes: 27 additions & 6 deletions credentials/google/xds.go
View file
Open in desktop
Expand Up @@ -21,13 +21,16 @@ package google
import (
"context"
"net"
"net/url"
"strings"

"google.golang.org/grpc/credentials"
"google.golang.org/grpc/internal"
)

const cfeClusterNamePrefix = "google_cfe_"
const cfeClusterResourceNamePrefix = "/envoy.config.cluster.v3.Cluster/google_cfe_"
const cfeClusterAuthorityName = "traffic-director-c2p.xds.googleapis.com"

// clusterTransportCreds is a combo of TLS + ALTS.
//
Expand All @@ -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
Member 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
Member 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.

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
Member 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
Member 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.

return c.tls.ClientHandshake(ctx, authority, rawConn)
return false
}
if strings.HasPrefix(cn, cfeClusterNamePrefix) {
return false
}
if !strings.HasPrefix(cn, "xdstp:") {
return true
}
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 isXDSNonCFECluster(ctx) {
// If attributes have cluster name, and cluster name is not cfe, it's a
// backend address, use ALTS.
return c.alts.ClientHandshake(ctx, authority, rawConn)
}
// If attributes have cluster name, and cluster name is not cfe, it's a
// backend address, use ALTS.
return c.alts.ClientHandshake(ctx, authority, rawConn)
return c.tls.ClientHandshake(ctx, authority, rawConn)
}

func (c *clusterTransportCreds) ServerHandshake(conn net.Conn) (net.Conn, credentials.AuthInfo, error) {
Expand Down
58 changes: 58 additions & 0 deletions credentials/google/xds_test.go
View file
Open in desktop
@@ -0,0 +1,58 @@
/*
*
* Copyright 2021 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package google

import (
"context"
"testing"

"google.golang.org/grpc/credentials"
"google.golang.org/grpc/internal"
icredentials "google.golang.org/grpc/internal/credentials"
"google.golang.org/grpc/resolver"
)

func (s) TestIsXDSNonCFECluster(t *testing.T) {
c := func(cluster string) context.Context {
return icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
Attributes: internal.SetXDSHandshakeClusterName(resolver.Address{}, cluster).Attributes,
})
}

testCases := []struct {
name string
ctx context.Context
want bool
}{
{"not an xDS cluster", context.Background(), false},
{"cfe", c("google_cfe_bigtable.googleapis.com"), false},
{"non-cfe", c("google_bigtable.googleapis.com"), true},
{"starts with xdstp but not cfe format", c("xdstp:google_cfe_bigtable.googleapis.com"), true},
{"no authority", c("xdstp:///envoy.config.cluster.v3.Cluster/google_cfe_"), true},
{"wrong authority", c("xdstp://foo.bar/envoy.config.cluster.v3.Cluster/google_cfe_"), true},
{"xdstp CFE", c("xdstp://traffic-director-c2p.xds.googleapis.com/envoy.config.cluster.v3.Cluster/google_cfe_"), false},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
if got := isXDSNonCFECluster(tc.ctx); got != tc.want {
t.Errorf("isXDSNonCFECluster(_) = %v; want %v", got, tc.want)
}
})
}
}