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

xdsclient: use dataplane authority for virtualhost lookup #6997

Merged
merged 14 commits into from Feb 28, 2024
2 changes: 2 additions & 0 deletions clientconn.go
Expand Up @@ -1771,6 +1771,8 @@ func parseTarget(target string) (resolver.Target, error) {
return resolver.Target{URL: *u}, nil
}

// encodeAuthority escapes the authority string based on valid chars defined in
// https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.
func encodeAuthority(authority string) string {
const upperhex = "0123456789ABCDEF"

Expand Down
4 changes: 2 additions & 2 deletions internal/testutils/xds/e2e/clientresources.go
Expand Up @@ -365,11 +365,11 @@ func HTTPFilter(name string, config proto.Message) *v3httppb.HttpFilter {
}

// DefaultRouteConfig returns a basic xds RouteConfig resource.
func DefaultRouteConfig(routeName, ldsTarget, clusterName string) *v3routepb.RouteConfiguration {
func DefaultRouteConfig(routeName, vhDomain, clusterName string) *v3routepb.RouteConfiguration {
return &v3routepb.RouteConfiguration{
Name: routeName,
VirtualHosts: []*v3routepb.VirtualHost{{
Domains: []string{ldsTarget},
Domains: []string{vhDomain},
Routes: []*v3routepb.Route{{
Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"}},
Action: &v3routepb.Route_Route{Route: &v3routepb.RouteAction{
Expand Down
3 changes: 3 additions & 0 deletions resolver/resolver.go
Expand Up @@ -168,6 +168,9 @@ type BuildOptions struct {
// field. In most cases though, it is not appropriate, and this field may
// be ignored.
Dialer func(context.Context, string) (net.Conn, error)
// Authority is the effective authority of the clientconn for which the
// resolver is built.
Authority string
}

// An Endpoint is one network endpoint, or server, which may have multiple
Expand Down
1 change: 1 addition & 0 deletions resolver_wrapper.go
Expand Up @@ -75,6 +75,7 @@ func (ccr *ccResolverWrapper) start() error {
DialCreds: ccr.cc.dopts.copts.TransportCredentials,
CredsBundle: ccr.cc.dopts.copts.CredsBundle,
Dialer: ccr.cc.dopts.copts.Dialer,
Authority: ccr.cc.authority,
}
var err error
ccr.resolver, err = ccr.cc.resolverBuilder.Build(ccr.cc.parsedTarget, ccr, opts)
Expand Down
85 changes: 85 additions & 0 deletions test/xds/xds_client_federation_test.go
Expand Up @@ -140,6 +140,91 @@ func (s) TestClientSideFederation(t *testing.T) {
}
}

// TestClientSideFederationWithOnlyXDSTPStyleLDS tests that federation is
// supported with new xdstp style names for LDS only while using the old style
// for other resources. This test in addition also checks that when service name
// contains escapable characters, we "fully" encode it for looking up
// VirtualHosts in xDS RouteConfigurtion.
func (s) TestClientSideFederationWithOnlyXDSTPStyleLDS(t *testing.T) {
// Start a management server as a sophisticated authority.
const authority = "traffic-manager.xds.notgoogleapis.com"
mgmtServer, err := e2e.StartManagementServer(e2e.ManagementServerOptions{})
if err != nil {
t.Fatalf("Failed to spin up the xDS management server: %v", err)
}
t.Cleanup(mgmtServer.Stop)

// Create a bootstrap file in a temporary directory.
nodeID := uuid.New().String()
bootstrapContents, err := bootstrap.Contents(bootstrap.Options{
NodeID: nodeID,
ServerURI: mgmtServer.Address,
ClientDefaultListenerResourceNameTemplate: fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%%s", authority),
Authorities: map[string]string{authority: mgmtServer.Address},
})
if err != nil {
t.Fatalf("Failed to create bootstrap file: %v", err)
}

resolverBuilder := internal.NewXDSResolverWithConfigForTesting.(func([]byte) (resolver.Builder, error))
resolver, err := resolverBuilder(bootstrapContents)
if err != nil {
t.Fatalf("Failed to create xDS resolver for testing: %v", err)
}
server := stubserver.StartTestService(t, nil)
defer server.Stop()

// serviceName with escapable characters - ' ', and '/'.
const serviceName = "my-service-client-side-xds/2nd component"

// All other resources are with old style name.
rdsName := "route-" + serviceName
cdsName := "cluster-" + serviceName
edsName := "endpoints-" + serviceName
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved

// Resource update sent to go-control-plane mgmt server.
resourceUpdate := e2e.UpdateOptions{
NodeID: nodeID,
Listeners: func() []*v3listenerpb.Listener {
// LDS is new style xdstp name. Since the LDS resource name is prefixed
// with xdstp, the string will be %-encoded excluding '/'s. See
// bootstrap.PopulateResourceTemplate().
const specialEscapedServiceName = "my-service-client-side-xds/2nd%20component" // same as bootstrap.percentEncode(serviceName)
ldsName := fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%s", authority, specialEscapedServiceName)
return []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}
}(),
Routes: func() []*v3routepb.RouteConfiguration {
// RouteConfiguration will has one entry in []VirutalHosts that contains the
// "fully" escaped service name in []Domains. This is to assert that gRPC
// uses the escaped service name to lookup VirtualHosts. RDS is also with
// old style name.
const fullyEscapedServiceName = "my-service-client-side-xds%2F2nd%20component" // same as url.PathEscape(serviceName)
return []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, fullyEscapedServiceName, cdsName)}
}(),
Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(cdsName, edsName, e2e.SecurityLevelNone)},
Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint(edsName, "localhost", []uint32{testutils.ParsePort(t, server.Address)})},
SkipValidation: true,
}

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if err := mgmtServer.Update(ctx, resourceUpdate); err != nil {
t.Fatal(err)
}

// Create a ClientConn and make a successful RPC.
cc, err := grpc.Dial(fmt.Sprintf("xds:///%s", serviceName), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(resolver))
if err != nil {
t.Fatalf("failed to dial local test server: %v", err)
}
defer cc.Close()

client := testgrpc.NewTestServiceClient(cc)
if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); err != nil {
t.Fatalf("rpc EmptyCall() failed: %v", err)
}
}

// TestFederation_UnknownAuthorityInDialTarget tests the case where a ClientConn
// is created with a dial target containing an authority which is not specified
// in the bootstrap configuration. The test verifies that RPCs on the ClientConn
Expand Down
5 changes: 4 additions & 1 deletion xds/internal/resolver/helpers_test.go
Expand Up @@ -21,6 +21,7 @@ package resolver_test
import (
"context"
"fmt"
"net/url"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -104,7 +105,9 @@ func buildResolverForTarget(t *testing.T, target resolver.Target) (chan resolver
}
}
tcc := &testutils.ResolverClientConn{Logger: t, UpdateStateF: updateStateF, ReportErrorF: reportErrorF}
r, err := builder.Build(target, tcc, resolver.BuildOptions{})
r, err := builder.Build(target, tcc, resolver.BuildOptions{
Authority: url.PathEscape(target.Endpoint()),
})
if err != nil {
t.Fatalf("Failed to build xDS resolver for target %q: %v", target, err)
}
Expand Down
19 changes: 10 additions & 9 deletions xds/internal/resolver/xds_resolver.go
Expand Up @@ -22,7 +22,6 @@
import (
"context"
"fmt"
"strings"
"sync/atomic"

"google.golang.org/grpc/internal"
Expand Down Expand Up @@ -114,12 +113,8 @@
if err != nil {
return nil, err
}
endpoint := target.URL.Path
if endpoint == "" {
endpoint = target.URL.Opaque
}
endpoint = strings.TrimPrefix(endpoint, "/")
r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint)
r.dataplaneAuthority = opts.Authority

Choose a reason for hiding this comment

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

May I suggest:

Suggested change
r.dataplaneAuthority = opts.Authority
r.dataplaneAuthority = target.Endpoint()

... in order to honor rewritten URLs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you give me more details about what you mean by "rewritten URLs"?

Copy link

Choose a reason for hiding this comment

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

Sure: we've been using a custom resolver which translates custom-scheme://host URIs to xds:///host.some-domain ones ("rewrite-uri" of some sort) and then delegates to the original xDS resolver. This was working fine up to gRPC-Go 1.62.0 but, since gRPC-Go 1.62.1, we started facing:

WARNING: [core] [Channel #1] ccResolverWrapper: reporting error to cc: no matching virtual host found for ""

The above "" happens to come from the empty opts.Authority. Using target.Endpoint() instead makes sure the virtual host lookup is performed against the resolved target.

Both gRPC (C++) 1.62.2 and gRPC-Java 1.62.2 seem to honor "rewritten" target URIs.

r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, target.Endpoint())
r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r)
return r, nil
}
Expand Down Expand Up @@ -190,6 +185,12 @@
serializer *grpcsync.CallbackSerializer
serializerCancel context.CancelFunc

// dataplaneAuthority is the authority used for the data plane connections,
// which is also used to select the VirtualHost within the xDS
// RouteConfiguration. This is %-encoded to match with VirtualHost Domain
// in xDS RouteConfiguration.
dataplaneAuthority string

ldsResourceName string
listenerWatcher *listenerWatcher
listenerUpdateRecvd bool
Expand Down Expand Up @@ -413,9 +414,9 @@
}

func (r *xdsResolver) applyRouteConfigUpdate(update xdsresource.RouteConfigUpdate) {
matchVh := xdsresource.FindBestMatchingVirtualHost(r.ldsResourceName, update.VirtualHosts)
matchVh := xdsresource.FindBestMatchingVirtualHost(r.dataplaneAuthority, update.VirtualHosts)
if matchVh == nil {
r.onError(fmt.Errorf("no matching virtual host found for %q", r.ldsResourceName))
r.onError(fmt.Errorf("no matching virtual host found for %q", r.dataplaneAuthority))

Check warning on line 419 in xds/internal/resolver/xds_resolver.go

View check run for this annotation

Codecov / codecov/patch

xds/internal/resolver/xds_resolver.go#L419

Added line #L419 was not covered by tests
return
}
r.currentRouteConfig = update
Expand Down