-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes from 12 commits
e308543
9a35ee6
c66cf72
960d384
7d919f7
bb71708
b9a7650
c882c16
01375d4
5eb5211
4eca127
ea83d59
485ea4c
6265227
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: please use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I suggest:
Suggested change
... in order to honor rewritten URLs? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure: we've been using a custom resolver which translates
The above 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 | ||||||
|
@@ -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 | ||||||
|
@@ -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 | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client
orClientConn
instead ofconnection
.And:
for which the resolver is built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.