From e3085434f9a6d4409374cffa74fe66a5dbe92380 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 21 Feb 2024 16:31:34 -0800 Subject: [PATCH 01/14] xdsclient: use dataplane authority for virtualhost lookup --- xds/internal/resolver/xds_resolver.go | 5 +- xds/internal/resolver/xds_resolver_test.go | 53 ++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index d1b3195dd1f..1ad9e2c3f6f 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -119,6 +119,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") + r.dataplaneAuthority = endpoint r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) return r, nil @@ -190,6 +191,8 @@ type xdsResolver struct { serializer *grpcsync.CallbackSerializer serializerCancel context.CancelFunc + dataplaneAuthority string + ldsResourceName string listenerWatcher *listenerWatcher listenerUpdateRecvd bool @@ -413,7 +416,7 @@ 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)) return diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index 1a0ca4ed5b4..b8413ef8dcd 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -116,6 +116,59 @@ func (s) TestResolverBuilder_AuthorityNotDefinedInBootstrap(t *testing.T) { } } +// // Tests the case where the specified dial target contains an authority that is +// // not specified in the bootstrap file. Verifies that the resolver.Build method +// // fails with the expected error string. +// func (s) TestResolverBuilder_ParsesDataplaneAuthorityCorrectly(t *testing.T) { +// bootstrapCleanup, err := xdsbootstrap.CreateFile(xdsbootstrap.Options{ +// NodeID: "node-id", +// ServerURI: "dummy-management-server", +// Authorities: map[string]string{ +// "something-authority": "dummy-management-server-address", +// }, +// }) +// if err != nil { +// t.Fatal(err) +// } +// defer bootstrapCleanup() + +// builder := resolver.Get(xdsresolver.Scheme) +// if builder == nil { +// t.Fatalf("Scheme %q is not registered", xdsresolver.Scheme) +// } + +// tests := []struct { +// targetURL string +// wantDataPlaneAuthority string +// }{ +// { +// targetURL: "xds:///target", +// wantDataPlaneAuthority: "target", +// }, +// { +// targetURL: "xds://something-authority/target", +// wantDataPlaneAuthority: "target", +// }, +// } + +// for i, tt := range tests { +// t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { +// target := resolver.Target{URL: *testutils.MustParseURL(tt.targetURL)} + +// r, err := builder.Build(target, &testutils.ResolverClientConn{Logger: t}, resolver.BuildOptions{}) +// if err != nil { +// t.Fatalf("xds Resolver Build(%v): %v", target, err) +// } +// r.Close() + +// if r.(*iresolver.xdsResolver).dataplaneAuthority != tt.wantDataPlaneAuthority { +// t.Fatalf("xds Resolver Build(%v) returned DataPlaneAuthority: %v, want: %v", target, r.(*iresolver.Resolver).DataPlaneAuthority(), tt.wantDataPlaneAuthority) +// } +// }) +// } + +// } + // Test builds an xDS resolver and verifies that the resource name specified in // the discovery request matches expectations. func (s) TestResolverResourceName(t *testing.T) { From 9a35ee695f0ec50110ca63b3da73b4e7d8c7f2b9 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 21 Feb 2024 18:19:22 -0800 Subject: [PATCH 02/14] add units --- test/xds/xds_client_federation_test.go | 72 +++++++++++++++++++++- xds/internal/resolver/xds_resolver.go | 2 +- xds/internal/resolver/xds_resolver_test.go | 53 ---------------- 3 files changed, 72 insertions(+), 55 deletions(-) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index 75dd77ba195..354e93c41a9 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -140,6 +140,76 @@ func (s) TestClientSideFederation(t *testing.T) { } } +// TestClientSideFederationWithTDOM tests that federation is supported with new +// xdstp style names with TD Open Mesh (TDOM) Authority. TD Open Mesh Authority +// currently only support new style for LDS. So, this test only tests the new +// style for LDS resources and the old style for other resources. +func (s) TestClientSideFederationWithTDOM(t *testing.T) { + // Start a management server as TDOM authority. + const tdomAuth = "traffic-director-global.xds.googleapis.com" + tdomAuthServer, err := e2e.StartManagementServer(e2e.ManagementServerOptions{}) + if err != nil { + t.Fatalf("Failed to spin up the xDS management server: %v", err) + } + t.Cleanup(tdomAuthServer.Stop) + + // Create a bootstrap file in a temporary directory. + nodeID := uuid.New().String() + bootstrapContents, err := bootstrap.Contents(bootstrap.Options{ + NodeID: nodeID, + ServerURI: tdomAuthServer.Address, + ClientDefaultListenerResourceNameTemplate: fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%%s", tdomAuth), + Authorities: map[string]string{tdomAuth: tdomAuthServer.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() + + const serviceName = "my-service-client-side-xds" + // LDS is new style name. + ldsName := fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%s", tdomAuth, serviceName) + // All other resources are with old style name. + rdsName := "route-" + serviceName + cdsName := "cluster-" + serviceName + edsName := "endpoints-" + serviceName + + resourceUpdate := e2e.UpdateOptions{ + NodeID: nodeID, + // This has only RDS and EDS. + Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}, + Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, serviceName, 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 := tdomAuthServer.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 @@ -232,7 +302,7 @@ func (s) TestFederation_UnknownAuthorityInReceivedResponse(t *testing.T) { resources := e2e.UpdateOptions{ NodeID: nodeID, Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}, - Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, ldsName, "cluster-"+serviceName)}, + Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, serviceName, "cluster-"+serviceName)}, SkipValidation: true, // This update has only LDS and RDS resources. } ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 1ad9e2c3f6f..70b66fb7d12 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -418,7 +418,7 @@ func (r *xdsResolver) onResolutionComplete() { func (r *xdsResolver) applyRouteConfigUpdate(update xdsresource.RouteConfigUpdate) { 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 diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index b8413ef8dcd..1a0ca4ed5b4 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -116,59 +116,6 @@ func (s) TestResolverBuilder_AuthorityNotDefinedInBootstrap(t *testing.T) { } } -// // Tests the case where the specified dial target contains an authority that is -// // not specified in the bootstrap file. Verifies that the resolver.Build method -// // fails with the expected error string. -// func (s) TestResolverBuilder_ParsesDataplaneAuthorityCorrectly(t *testing.T) { -// bootstrapCleanup, err := xdsbootstrap.CreateFile(xdsbootstrap.Options{ -// NodeID: "node-id", -// ServerURI: "dummy-management-server", -// Authorities: map[string]string{ -// "something-authority": "dummy-management-server-address", -// }, -// }) -// if err != nil { -// t.Fatal(err) -// } -// defer bootstrapCleanup() - -// builder := resolver.Get(xdsresolver.Scheme) -// if builder == nil { -// t.Fatalf("Scheme %q is not registered", xdsresolver.Scheme) -// } - -// tests := []struct { -// targetURL string -// wantDataPlaneAuthority string -// }{ -// { -// targetURL: "xds:///target", -// wantDataPlaneAuthority: "target", -// }, -// { -// targetURL: "xds://something-authority/target", -// wantDataPlaneAuthority: "target", -// }, -// } - -// for i, tt := range tests { -// t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) { -// target := resolver.Target{URL: *testutils.MustParseURL(tt.targetURL)} - -// r, err := builder.Build(target, &testutils.ResolverClientConn{Logger: t}, resolver.BuildOptions{}) -// if err != nil { -// t.Fatalf("xds Resolver Build(%v): %v", target, err) -// } -// r.Close() - -// if r.(*iresolver.xdsResolver).dataplaneAuthority != tt.wantDataPlaneAuthority { -// t.Fatalf("xds Resolver Build(%v) returned DataPlaneAuthority: %v, want: %v", target, r.(*iresolver.Resolver).DataPlaneAuthority(), tt.wantDataPlaneAuthority) -// } -// }) -// } - -// } - // Test builds an xDS resolver and verifies that the resource name specified in // the discovery request matches expectations. func (s) TestResolverResourceName(t *testing.T) { From c66cf724f13ea3f95f0e5a80125f3408fd8144e9 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 21 Feb 2024 18:25:31 -0800 Subject: [PATCH 03/14] add comment for xdsresolver.dataplaneAuthority --- test/xds/xds_client_federation_test.go | 2 +- xds/internal/resolver/xds_resolver.go | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index 354e93c41a9..6e0965b88bb 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -302,7 +302,7 @@ func (s) TestFederation_UnknownAuthorityInReceivedResponse(t *testing.T) { resources := e2e.UpdateOptions{ NodeID: nodeID, Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}, - Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, serviceName, "cluster-"+serviceName)}, + Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, ldsName, "cluster-"+serviceName)}, SkipValidation: true, // This update has only LDS and RDS resources. } ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 70b66fb7d12..e331fb43887 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -191,6 +191,13 @@ type xdsResolver struct { serializer *grpcsync.CallbackSerializer serializerCancel context.CancelFunc + // Per [A47] the authority used for the data plane connections (which is + // also used to select the VirtualHost within the xDS RouteConfiguration) + // will continue to be the last path component of the xds URI used to create + // the gRPC channel (i.e., the part following the last / character, or the + // entire path if the path contains no / character). + // + // [A47]: https://github.com/grpc/proposal/blob/master/A47-xds-federation.md dataplaneAuthority string ldsResourceName string From 960d384eb71eb1078e8b1a61eef4d141f2d9d1ed Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 21 Feb 2024 18:29:24 -0800 Subject: [PATCH 04/14] nix comment --- test/xds/xds_client_federation_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index 6e0965b88bb..af4f093356a 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -182,8 +182,7 @@ func (s) TestClientSideFederationWithTDOM(t *testing.T) { edsName := "endpoints-" + serviceName resourceUpdate := e2e.UpdateOptions{ - NodeID: nodeID, - // This has only RDS and EDS. + NodeID: nodeID, Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}, Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, serviceName, cdsName)}, Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster(cdsName, edsName, e2e.SecurityLevelNone)}, From 7d919f7854714cec606ad2823384457b154e6544 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Thu, 22 Feb 2024 16:38:16 -0800 Subject: [PATCH 05/14] update excerpt from a47 --- xds/internal/resolver/xds_resolver.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index e331fb43887..a103497c816 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -191,11 +191,12 @@ type xdsResolver struct { serializer *grpcsync.CallbackSerializer serializerCancel context.CancelFunc - // Per [A47] the authority used for the data plane connections (which is + // Per [A47], the authority used for the data plane connections (which is // also used to select the VirtualHost within the xDS RouteConfiguration) - // will continue to be the last path component of the xds URI used to create - // the gRPC channel (i.e., the part following the last / character, or the - // entire path if the path contains no / character). + // will continue to be the path component of the `xds` URI used to create + // the gRPC channel, stripping off the leading `/` if any. (Any remaining + // `/` characters will be percent-encoded, as is normal when determining the + // data plane authority from the gRPC target URI.). // // [A47]: https://github.com/grpc/proposal/blob/master/A47-xds-federation.md dataplaneAuthority string From bb71708d62a1c8adbae0118834e7f483786d6855 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Mon, 26 Feb 2024 11:18:47 -0800 Subject: [PATCH 06/14] update with TODO for URL encoding question --- xds/internal/resolver/xds_resolver.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index a103497c816..851857a3515 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -119,6 +119,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") + // TODO(https://github.com/grpc/grpc-go/issues/7002): Check if dataplaneAuthority is URL encoded (including /'s). If not, encode it. r.dataplaneAuthority = endpoint r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) @@ -191,14 +192,9 @@ type xdsResolver struct { serializer *grpcsync.CallbackSerializer serializerCancel context.CancelFunc - // Per [A47], the authority used for the data plane connections (which is - // also used to select the VirtualHost within the xDS RouteConfiguration) - // will continue to be the path component of the `xds` URI used to create - // the gRPC channel, stripping off the leading `/` if any. (Any remaining - // `/` characters will be percent-encoded, as is normal when determining the - // data plane authority from the gRPC target URI.). - // - // [A47]: https://github.com/grpc/proposal/blob/master/A47-xds-federation.md + // dataplaneAuthority is the authority used for the data plane connections, + // which is also used to select the VirtualHost within the xDS + // RouteConfiguration. dataplaneAuthority string ldsResourceName string From b9a76502a77cb9f78a540b60276b9ee344729283 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Mon, 26 Feb 2024 12:37:07 -0800 Subject: [PATCH 07/14] fmt --- xds/internal/resolver/xds_resolver.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 851857a3515..6b2f4ce210f 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -119,7 +119,8 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") - // TODO(https://github.com/grpc/grpc-go/issues/7002): Check if dataplaneAuthority is URL encoded (including /'s). If not, encode it. + // TODO(https://github.com/grpc/grpc-go/issues/7002): Check if + // dataplaneAuthority is URL encoded (including /'s). If not, encode it. r.dataplaneAuthority = endpoint r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) From c882c16165dec1b4162cda1a2a7aea0b6ea175ad Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Mon, 26 Feb 2024 17:31:37 -0800 Subject: [PATCH 08/14] fix lookup logic to lookup using %encoded authority --- xds/internal/resolver/xds_resolver.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 6b2f4ce210f..15a88d689c8 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,6 +22,7 @@ package resolver import ( "context" "fmt" + "net/url" "strings" "sync/atomic" @@ -119,11 +120,9 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") - // TODO(https://github.com/grpc/grpc-go/issues/7002): Check if - // dataplaneAuthority is URL encoded (including /'s). If not, encode it. - r.dataplaneAuthority = endpoint r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) + r.dataplaneAuthority = url.PathEscape(endpoint) return r, nil } @@ -195,7 +194,8 @@ type xdsResolver struct { // dataplaneAuthority is the authority used for the data plane connections, // which is also used to select the VirtualHost within the xDS - // RouteConfiguration. + // RouteConfiguration. This string is %-encoded to match with VirtualHost + // Domain in xDS RouteConfiguration. dataplaneAuthority string ldsResourceName string From 01375d460b7df992265bf0208721d64501d5a0f9 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Mon, 26 Feb 2024 17:37:35 -0800 Subject: [PATCH 09/14] update docstring --- xds/internal/resolver/xds_resolver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 15a88d689c8..fea7a4e0584 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -194,8 +194,8 @@ type xdsResolver struct { // dataplaneAuthority is the authority used for the data plane connections, // which is also used to select the VirtualHost within the xDS - // RouteConfiguration. This string is %-encoded to match with VirtualHost - // Domain in xDS RouteConfiguration. + // RouteConfiguration. This is %-encoded to match with VirtualHost Domain + // in xDS RouteConfiguration. dataplaneAuthority string ldsResourceName string From 5eb521199cccde5779000010b65bcffdad8d82ce Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Mon, 26 Feb 2024 17:53:06 -0800 Subject: [PATCH 10/14] fix race --- xds/internal/resolver/xds_resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index fea7a4e0584..cf71528f031 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -120,9 +120,9 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") + r.dataplaneAuthority = url.PathEscape(endpoint) r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) - r.dataplaneAuthority = url.PathEscape(endpoint) return r, nil } From 4eca127057c4cd24b46dd6f966ddd8cd5fa83486 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Tue, 27 Feb 2024 11:40:24 -0800 Subject: [PATCH 11/14] dataplane authority is plumbed from the clientconn to resolver via buildoptions --- clientconn.go | 4 ++ internal/testutils/xds/e2e/clientresources.go | 4 +- resolver/resolver.go | 3 ++ resolver_wrapper.go | 1 + test/xds/xds_client_federation_test.go | 45 ++++++++++++------- xds/internal/resolver/xds_resolver.go | 3 +- 6 files changed, 39 insertions(+), 21 deletions(-) diff --git a/clientconn.go b/clientconn.go index 00934c38a14..9954f8e0c27 100644 --- a/clientconn.go +++ b/clientconn.go @@ -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" diff --git a/internal/testutils/xds/e2e/clientresources.go b/internal/testutils/xds/e2e/clientresources.go index 347ea804231..175490e91cf 100644 --- a/internal/testutils/xds/e2e/clientresources.go +++ b/internal/testutils/xds/e2e/clientresources.go @@ -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{ diff --git a/resolver/resolver.go b/resolver/resolver.go index ca6dbd1e919..ccae785d2b2 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -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. + Authority string } // An Endpoint is one network endpoint, or server, which may have multiple diff --git a/resolver_wrapper.go b/resolver_wrapper.go index c79bab12149..f845ac95893 100644 --- a/resolver_wrapper.go +++ b/resolver_wrapper.go @@ -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) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index af4f093356a..33406ac4683 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -140,26 +140,27 @@ func (s) TestClientSideFederation(t *testing.T) { } } -// TestClientSideFederationWithTDOM tests that federation is supported with new -// xdstp style names with TD Open Mesh (TDOM) Authority. TD Open Mesh Authority -// currently only support new style for LDS. So, this test only tests the new -// style for LDS resources and the old style for other resources. -func (s) TestClientSideFederationWithTDOM(t *testing.T) { - // Start a management server as TDOM authority. - const tdomAuth = "traffic-director-global.xds.googleapis.com" - tdomAuthServer, err := e2e.StartManagementServer(e2e.ManagementServerOptions{}) +// 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 (in this case a `/`), we 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(tdomAuthServer.Stop) + 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: tdomAuthServer.Address, - ClientDefaultListenerResourceNameTemplate: fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%%s", tdomAuth), - Authorities: map[string]string{tdomAuth: tdomAuthServer.Address}, + 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) @@ -173,18 +174,28 @@ func (s) TestClientSideFederationWithTDOM(t *testing.T) { server := stubserver.StartTestService(t, nil) defer server.Stop() - const serviceName = "my-service-client-side-xds" - // LDS is new style name. - ldsName := fmt.Sprintf("xdstp://%s/envoy.config.listener.v3.Listener/%s", tdomAuth, serviceName) + serviceName := "my-service-client-side-xds/2nd component" + 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 + // 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, serviceName, cdsName)}, + 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, @@ -192,7 +203,7 @@ func (s) TestClientSideFederationWithTDOM(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - if err := tdomAuthServer.Update(ctx, resourceUpdate); err != nil { + if err := mgmtServer.Update(ctx, resourceUpdate); err != nil { t.Fatal(err) } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index cf71528f031..2ad6b98926a 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,7 +22,6 @@ package resolver import ( "context" "fmt" - "net/url" "strings" "sync/atomic" @@ -120,7 +119,7 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon endpoint = target.URL.Opaque } endpoint = strings.TrimPrefix(endpoint, "/") - r.dataplaneAuthority = url.PathEscape(endpoint) + r.dataplaneAuthority = opts.Authority r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) return r, nil From ea83d59d7a348d91e71dd4a9b64cd95d4c6b5824 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Tue, 27 Feb 2024 11:42:18 -0800 Subject: [PATCH 12/14] fix comment --- test/xds/xds_client_federation_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index 33406ac4683..09b735b929b 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -140,10 +140,10 @@ 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 (in this case a `/`), we encode it for looking up +// 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. From 485ea4cff0410b86d23e2c2e8eea97c384cb62c9 Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Tue, 27 Feb 2024 13:15:39 -0800 Subject: [PATCH 13/14] fix comments --- clientconn.go | 4 +-- resolver/resolver.go | 4 +-- test/xds/xds_client_federation_test.go | 35 +++++++++++++++----------- xds/internal/resolver/helpers_test.go | 5 +++- xds/internal/resolver/xds_resolver.go | 8 +----- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/clientconn.go b/clientconn.go index 9954f8e0c27..37913fdf527 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1772,9 +1772,7 @@ func parseTarget(target string) (resolver.Target, error) { } // 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 +// https://datatracker.ietf.org/doc/html/rfc3986#section-3.2. func encodeAuthority(authority string) string { const upperhex = "0123456789ABCDEF" diff --git a/resolver/resolver.go b/resolver/resolver.go index ccae785d2b2..95c46452464 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -168,8 +168,8 @@ 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. + // Authority is the effective authority of the clientconn for which the + // resolver is built. Authority string } diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index 09b735b929b..a0ff2eb5bbe 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -174,28 +174,33 @@ func (s) TestClientSideFederationWithOnlyXDSTPStyleLDS(t *testing.T) { server := stubserver.StartTestService(t, nil) defer server.Stop() - serviceName := "my-service-client-side-xds/2nd component" - 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) + // 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 - // 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. + // Resource update sent to go-control-plane mgmt server. resourceUpdate := e2e.UpdateOptions{ - NodeID: nodeID, - Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(ldsName, rdsName)}, - Routes: []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig(rdsName, fullyEscapedServiceName, cdsName)}, + 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, diff --git a/xds/internal/resolver/helpers_test.go b/xds/internal/resolver/helpers_test.go index d30ea10995a..e15982a46eb 100644 --- a/xds/internal/resolver/helpers_test.go +++ b/xds/internal/resolver/helpers_test.go @@ -21,6 +21,7 @@ package resolver_test import ( "context" "fmt" + "net/url" "strings" "testing" "time" @@ -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) } diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 2ad6b98926a..6d1966230c6 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,7 +22,6 @@ package resolver import ( "context" "fmt" - "strings" "sync/atomic" "google.golang.org/grpc/internal" @@ -114,13 +113,8 @@ func (b *xdsResolverBuilder) Build(target resolver.Target, cc resolver.ClientCon if err != nil { return nil, err } - endpoint := target.URL.Path - if endpoint == "" { - endpoint = target.URL.Opaque - } - endpoint = strings.TrimPrefix(endpoint, "/") r.dataplaneAuthority = opts.Authority - r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, endpoint) + r.ldsResourceName = bootstrap.PopulateResourceTemplate(template, target.Endpoint()) r.listenerWatcher = newListenerWatcher(r.ldsResourceName, r) return r, nil } From 62652279557cac82aab840bd80c71f555b4ac36b Mon Sep 17 00:00:00 2001 From: Arvind Bright Date: Wed, 28 Feb 2024 10:00:10 -0800 Subject: [PATCH 14/14] const-ing --- test/xds/xds_client_federation_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/xds/xds_client_federation_test.go b/test/xds/xds_client_federation_test.go index a0ff2eb5bbe..e826d443b97 100644 --- a/test/xds/xds_client_federation_test.go +++ b/test/xds/xds_client_federation_test.go @@ -178,9 +178,9 @@ func (s) TestClientSideFederationWithOnlyXDSTPStyleLDS(t *testing.T) { 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 + const rdsName = "route-" + serviceName + const cdsName = "cluster-" + serviceName + const edsName = "endpoints-" + serviceName // Resource update sent to go-control-plane mgmt server. resourceUpdate := e2e.UpdateOptions{