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
4 changes: 4 additions & 0 deletions clientconn.go
Expand Up @@ -1771,6 +1771,10 @@ func parseTarget(target string) (resolver.Target, error) {
return resolver.Target{URL: *u}, nil
}

// encodeAuthority escapes the authority string based on valid chars defined in
// [Section 3.2 of RFC3986].
//
// [Section 3.2 of RFC3986]: 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 connection for which the
// resolver is built for.
Copy link
Member

Choose a reason for hiding this comment

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

client or ClientConn instead of connection.

And: for which the resolver is built.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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
80 changes: 80 additions & 0 deletions test/xds/xds_client_federation_test.go
Expand Up @@ -140,6 +140,86 @@ 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 := "my-service-client-side-xds/2nd component"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please use const anywhere that it's possible to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

specialEscapedServiceName := "my-service-client-side-xds/2nd%20component"
fullyEscapedServiceName := "my-service-client-side-xds%2F2nd%20component"

// LDS is new style name. Since the LDS resource name is prefixed with
// xdstp, the string will be %-encoded excluding '/'s. See
// bootstrap.PopulateResourceTemplate()
ldsName := fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%s", authority, specialEscapedServiceName)

// 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

// 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.
resourceUpdate := e2e.UpdateOptions{
NodeID: nodeID,
Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)},
Routes: []*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
11 changes: 9 additions & 2 deletions xds/internal/resolver/xds_resolver.go
Expand Up @@ -119,6 +119,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon
endpoint = target.URL.Opaque
}
endpoint = strings.TrimPrefix(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, endpoint)
r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r)
return r, nil
Expand Down Expand Up @@ -190,6 +191,12 @@ type xdsResolver struct {
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 +420,9 @@ func (r *xdsResolver) onResolutionComplete() {
}

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))
return
}
r.currentRouteConfig = update
Expand Down