From 89f3956d8904d0501f5c5a4b56e98e11a0f69623 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Tue, 12 Oct 2021 11:01:41 -0700 Subject: [PATCH 1/5] [federation_target_parsing] xds resolver [federation_target_parsing] template populate [federation_target_parsing] default bootstrap for test client [federation_target_parsing] fixed resolver tests, new tests not added yet [federation_target_parsing] new tests [federation_target_parsing] fix test [federation_target_parsing] c1 --- xds/internal/resolver/xds_resolver.go | 45 ++++++- xds/internal/resolver/xds_resolver_test.go | 126 +++++++++++++++++- xds/internal/testutils/fakeclient/client.go | 1 + xds/internal/xdsclient/bootstrap/template.go | 47 +++++++ .../xdsclient/bootstrap/template_test.go | 97 ++++++++++++++ xds/server.go | 7 +- 6 files changed, 310 insertions(+), 13 deletions(-) create mode 100644 xds/internal/xdsclient/bootstrap/template.go create mode 100644 xds/internal/xdsclient/bootstrap/template_test.go diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go index 19ee01773e8..e04ce00b2d2 100644 --- a/xds/internal/resolver/xds_resolver.go +++ b/xds/internal/resolver/xds_resolver.go @@ -22,6 +22,7 @@ package resolver import ( "errors" "fmt" + "strings" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpclog" @@ -30,6 +31,7 @@ import ( iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal/xdsclient" + "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" ) const xdsScheme = "xds" @@ -60,7 +62,7 @@ type xdsResolverBuilder struct { // // The xds bootstrap process is performed (and a new xds client is built) every // time an xds resolver is built. -func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) { +func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (_ resolver.Resolver, retErr error) { r := &xdsResolver{ target: t, cc: cc, @@ -68,7 +70,14 @@ func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, op updateCh: make(chan suWithError, 1), activeClusters: make(map[string]*clusterInfo), } - r.logger = prefixLogger((r)) + defer func() { + if retErr != nil { + if r.client != nil { + r.client.Close() + } + } + }() + r.logger = prefixLogger(r) r.logger.Infof("Creating resolver for target: %+v", t) newXDSClient := newXDSClient @@ -81,6 +90,10 @@ func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, op return nil, fmt.Errorf("xds: failed to create xds-client: %v", err) } r.client = client + bootstrapConfig := client.BootstrapConfig() + if bootstrapConfig == nil { + return nil, errors.New("bootstrap configuration is empty") + } // If xds credentials were specified by the user, but bootstrap configs do // not contain any certificate provider configuration, it is better to fail @@ -94,14 +107,36 @@ func (b *xdsResolverBuilder) Build(t resolver.Target, cc resolver.ClientConn, op creds = opts.CredsBundle.TransportCredentials() } if xc, ok := creds.(interface{ UsesXDS() bool }); ok && xc.UsesXDS() { - bc := client.BootstrapConfig() - if len(bc.CertProviderConfigs) == 0 { + if len(bootstrapConfig.CertProviderConfigs) == 0 { return nil, errors.New("xds: xdsCreds specified but certificate_providers config missing in bootstrap file") } } + // Find the client listener template to use from the bootstrap config: + // - If authority is not set in the target, use the top level template + // - If authority is set, use the template from the authority map. + template := bootstrapConfig.ClientDefaultListenerResourceNameTemplate + if authority := r.target.URL.Host; authority != "" { + a := bootstrapConfig.Authorities[authority] + if a == nil { + return nil, fmt.Errorf("xds: authority %q is not found in the bootstrap file", authority) + } + if a.ClientListenerResourceNameTemplate != "" { + // This check will never be false, because + // ClientListenerResourceNameTemplate is required to start with + // xdstp://, and has a default value (not an empty string) if unset. + template = a.ClientListenerResourceNameTemplate + } + } + endpoint := r.target.URL.Path + if endpoint == "" { + endpoint = r.target.URL.Opaque + } + endpoint = strings.TrimPrefix(endpoint, "/") + resourceName := bootstrap.PopulateResourceTemplate(template, endpoint) + // Register a watch on the xdsClient for the user's dial target. - cancelWatch := watchService(r.client, r.target.Endpoint, r.handleServiceUpdate, r.logger) + cancelWatch := watchService(r.client, resourceName, r.handleServiceUpdate, r.logger) r.logger.Infof("Watch started on resource name %v with xds-client %p", r.target.Endpoint, r.client) r.cancelWatch = func() { cancelWatch() diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index c05a7422904..c6f00dac27f 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -21,6 +21,7 @@ package resolver import ( "context" "errors" + "net/url" "reflect" "strings" "testing" @@ -62,7 +63,7 @@ const ( defaultTestShortTimeout = 100 * time.Microsecond ) -var target = resolver.Target{Endpoint: targetStr} +var target = resolver.Target{Endpoint: targetStr, URL: url.URL{Scheme: "xds", Path: "/" + targetStr}} var routerFilter = xdsresource.HTTPFilter{Name: "rtr", Filter: httpfilter.Get(router.TypeURL)} var routerFilterList = []xdsresource.HTTPFilter{routerFilter} @@ -117,6 +118,7 @@ func (s) TestResolverBuilder(t *testing.T) { tests := []struct { name string xdsClientFunc func() (xdsclient.XDSClient, error) + target resolver.Target wantErr bool }{ { @@ -124,6 +126,7 @@ func (s) TestResolverBuilder(t *testing.T) { xdsClientFunc: func() (xdsclient.XDSClient, error) { return fakeclient.NewClient(), nil }, + target: target, wantErr: false, }, { @@ -131,6 +134,29 @@ func (s) TestResolverBuilder(t *testing.T) { xdsClientFunc: func() (xdsclient.XDSClient, error) { return nil, errors.New("newXDSClient-throws-error") }, + target: target, + wantErr: true, + }, + { + name: "authority not defined in bootstrap", + xdsClientFunc: func() (xdsclient.XDSClient, error) { + c := fakeclient.NewClient() + c.SetBootstrapConfig(&bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "%s", + Authorities: map[string]*bootstrap.Authority{ + "test-authority": { + ClientListenerResourceNameTemplate: "xdstp://test-authority/%s", + }, + }, + }) + return c, nil + }, + target: resolver.Target{ + URL: url.URL{ + Host: "non-existing-authority", + Path: "/" + targetStr, + }, + }, wantErr: true, }, } @@ -148,7 +174,7 @@ func (s) TestResolverBuilder(t *testing.T) { t.Fatalf("resolver.Get(%v) returned nil", xdsScheme) } - r, err := builder.Build(target, newTestClientConn(), resolver.BuildOptions{}) + r, err := builder.Build(test.target, newTestClientConn(), resolver.BuildOptions{}) if (err != nil) != test.wantErr { t.Fatalf("builder.Build(%v) returned err: %v, wantErr: %v", target, err, test.wantErr) } @@ -197,6 +223,7 @@ func (s) TestResolverBuilder_xdsCredsBootstrapMismatch(t *testing.T) { type setupOpts struct { xdsClientFunc func() (xdsclient.XDSClient, error) + target *resolver.Target } func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *testClientConn, func()) { @@ -214,7 +241,11 @@ func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *testClientConn, fun } tcc := newTestClientConn() - r, err := builder.Build(target, tcc, resolver.BuildOptions{}) + tgt := target + if opts.target != nil { + tgt = *opts.target + } + r, err := builder.Build(tgt, tcc, resolver.BuildOptions{}) if err != nil { t.Fatalf("builder.Build(%v) returned err: %v", target, err) } @@ -251,6 +282,95 @@ func waitForWatchRouteConfig(ctx context.Context, t *testing.T, xdsC *fakeclient } } +// TestXDSResolverResourceNameToWatch tests that the correct resource name is +// used to watch for the service. This covers cases with different bootstrap +// config, and different authority. +func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { + tests := []struct { + name string + bc *bootstrap.Config + target *resolver.Target + want string + }{ + { + name: "default %s old style", + bc: &bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "%s", + }, + target: &resolver.Target{ + URL: url.URL{Path: "/" + targetStr}, + }, + want: targetStr, + }, + { + name: "old style no percent encoding", + bc: &bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "/path/to/%s", + }, + target: &resolver.Target{ + URL: url.URL{Path: "/" + targetStr}, + }, + want: "/path/to/" + targetStr, + }, + { + name: "new style with %s", + bc: &bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "xdstp://authority.com/%s", + Authorities: nil, + }, + target: &resolver.Target{ + URL: url.URL{Path: "/0.0.0.0:8080"}, + }, + want: "xdstp://authority.com/0.0.0.0:8080", + }, + { + name: "new style percent encoding", + bc: &bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "xdstp://authority.com/%s", + Authorities: nil, + }, + target: &resolver.Target{ + URL: url.URL{Path: "/[::1]:8080"}, + }, + want: "xdstp://authority.com/%5B::1%5D:8080", + }, + { + name: "new style different authority", + bc: &bootstrap.Config{ + ClientDefaultListenerResourceNameTemplate: "xdstp://authority.com/%s", + Authorities: map[string]*bootstrap.Authority{ + "test-authority": { + ClientListenerResourceNameTemplate: "xdstp://test-authority/%s", + }, + }, + }, + target: &resolver.Target{ + URL: url.URL{ + Host: "test-authority", + Path: "/" + targetStr, + }, + }, + want: "xdstp://test-authority/" + targetStr, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + xdsC := fakeclient.NewClient() + xdsC.SetBootstrapConfig(tt.bc) + xdsR, _, cancel := testSetup(t, setupOpts{ + xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, + target: tt.target, + }) + defer cancel() + defer xdsR.Close() + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + waitForWatchListener(ctx, t, xdsC, tt.want) + }) + } +} + // TestXDSResolverWatchCallbackAfterClose tests the case where a service update // from the underlying xdsClient is received after the resolver is closed. func (s) TestXDSResolverWatchCallbackAfterClose(t *testing.T) { diff --git a/xds/internal/testutils/fakeclient/client.go b/xds/internal/testutils/fakeclient/client.go index 132fa413a7e..17505d87849 100644 --- a/xds/internal/testutils/fakeclient/client.go +++ b/xds/internal/testutils/fakeclient/client.go @@ -317,6 +317,7 @@ func NewClientWithName(name string) *Client { loadReportCh: testutils.NewChannel(), lrsCancelCh: testutils.NewChannel(), loadStore: load.NewStore(), + bootstrapCfg: &bootstrap.Config{ClientDefaultListenerResourceNameTemplate: "%s"}, rdsCbs: make(map[string]func(xdsresource.RouteConfigUpdate, error)), cdsCbs: make(map[string]func(xdsresource.ClusterUpdate, error)), edsCbs: make(map[string]func(xdsresource.EndpointsUpdate, error)), diff --git a/xds/internal/xdsclient/bootstrap/template.go b/xds/internal/xdsclient/bootstrap/template.go new file mode 100644 index 00000000000..9b51fcc8397 --- /dev/null +++ b/xds/internal/xdsclient/bootstrap/template.go @@ -0,0 +1,47 @@ +/* + * + * 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 bootstrap + +import ( + "net/url" + "strings" +) + +// PopulateResourceTemplate populates the given template using the target +// string. "%s", if exists in the template, will be replaced with target. +// +// If the template starts with "xdstp:", the replaced string will be %-encoded. +// But note that "/" is not percent encoded. +func PopulateResourceTemplate(template, target string) string { + if !strings.Contains(template, "%s") { + return template + } + if strings.HasPrefix(template, "xdstp:") { + target = percentEncode(target) + } + return strings.Replace(template, "%s", target, -1) +} + +// percentEncode percent encode t, except for "/". See the tests for examples. +func percentEncode(t string) string { + segs := strings.Split(t, "/") + for i := range segs { + segs[i] = url.PathEscape(segs[i]) + } + return strings.Join(segs, "/") +} diff --git a/xds/internal/xdsclient/bootstrap/template_test.go b/xds/internal/xdsclient/bootstrap/template_test.go new file mode 100644 index 00000000000..1511cf0774a --- /dev/null +++ b/xds/internal/xdsclient/bootstrap/template_test.go @@ -0,0 +1,97 @@ +/* + * + * 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 bootstrap + +import "testing" + +func Test_percentEncode(t *testing.T) { + tests := []struct { + name string + t string + want string + }{ + { + name: "normal name", + t: "server.example.com", + want: "server.example.com", + }, + { + name: "ipv4", + t: "0.0.0.0:8080", + want: "0.0.0.0:8080", + }, + { + name: "ipv6", + t: "[::1]:8080", + want: "%5B::1%5D:8080", // [ and ] are percent encoded. + }, + { + name: "/ should not be percent encoded", + t: "my/service/region", + want: "my/service/region", // "/"s are kept. + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := percentEncode(tt.t); got != tt.want { + t.Errorf("percentEncode() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestPopulateResourceTemplate(t *testing.T) { + tests := []struct { + name string + template string + t string + want string + }{ + { + name: "no %s", + template: "/name/template", + t: "[::1]:8080", + want: "/name/template", + }, + { + name: "with %s, no xdstp: prefix, ipv6", + template: "/name/template/%s", + t: "[::1]:8080", + want: "/name/template/[::1]:8080", + }, + { + name: "with %s, with xdstp: prefix", + template: "xdstp://authority.com/%s", + t: "0.0.0.0:8080", + want: "xdstp://authority.com/0.0.0.0:8080", + }, + { + name: "with %s, with xdstp: prefix, and ipv6", + template: "xdstp://authority.com/%s", + t: "[::1]:8080", + want: "xdstp://authority.com/%5B::1%5D:8080", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := PopulateResourceTemplate(tt.template, tt.t); got != tt.want { + t.Errorf("PopulateResourceTemplate() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/xds/server.go b/xds/server.go index 28abaf84f5f..0b47fd27ef4 100644 --- a/xds/server.go +++ b/xds/server.go @@ -23,7 +23,6 @@ import ( "errors" "fmt" "net" - "strings" "sync" "google.golang.org/grpc" @@ -42,6 +41,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/grpc/xds/internal/server" "google.golang.org/grpc/xds/internal/xdsclient" + "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" ) @@ -217,10 +217,7 @@ func (s *GRPCServer) Serve(lis net.Listener) error { if cfg.ServerListenerResourceNameTemplate == "" { return errors.New("missing server_listener_resource_name_template in the bootstrap configuration") } - name := cfg.ServerListenerResourceNameTemplate - if strings.Contains(cfg.ServerListenerResourceNameTemplate, "%s") { - name = strings.Replace(cfg.ServerListenerResourceNameTemplate, "%s", lis.Addr().String(), -1) - } + name := bootstrap.PopulateResourceTemplate(cfg.ServerListenerResourceNameTemplate, lis.Addr().String()) modeUpdateCh := buffer.NewUnbounded() go func() { From 0db53aff3b8dbbfbc7f6fde3559088980fa9fe57 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Wed, 3 Nov 2021 13:19:01 -0700 Subject: [PATCH 2/5] [federation_target_parsing] t->target --- .../xdsclient/bootstrap/template_test.go | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/xds/internal/xdsclient/bootstrap/template_test.go b/xds/internal/xdsclient/bootstrap/template_test.go index 1511cf0774a..bc12eb42991 100644 --- a/xds/internal/xdsclient/bootstrap/template_test.go +++ b/xds/internal/xdsclient/bootstrap/template_test.go @@ -21,34 +21,34 @@ import "testing" func Test_percentEncode(t *testing.T) { tests := []struct { - name string - t string - want string + name string + target string + want string }{ { - name: "normal name", - t: "server.example.com", - want: "server.example.com", + name: "normal name", + target: "server.example.com", + want: "server.example.com", }, { - name: "ipv4", - t: "0.0.0.0:8080", - want: "0.0.0.0:8080", + name: "ipv4", + target: "0.0.0.0:8080", + want: "0.0.0.0:8080", }, { - name: "ipv6", - t: "[::1]:8080", - want: "%5B::1%5D:8080", // [ and ] are percent encoded. + name: "ipv6", + target: "[::1]:8080", + want: "%5B::1%5D:8080", // [ and ] are percent encoded. }, { - name: "/ should not be percent encoded", - t: "my/service/region", - want: "my/service/region", // "/"s are kept. + name: "/ should not be percent encoded", + target: "my/service/region", + want: "my/service/region", // "/"s are kept. }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := percentEncode(tt.t); got != tt.want { + if got := percentEncode(tt.target); got != tt.want { t.Errorf("percentEncode() = %v, want %v", got, tt.want) } }) @@ -59,37 +59,37 @@ func TestPopulateResourceTemplate(t *testing.T) { tests := []struct { name string template string - t string + target string want string }{ { name: "no %s", template: "/name/template", - t: "[::1]:8080", + target: "[::1]:8080", want: "/name/template", }, { name: "with %s, no xdstp: prefix, ipv6", template: "/name/template/%s", - t: "[::1]:8080", + target: "[::1]:8080", want: "/name/template/[::1]:8080", }, { name: "with %s, with xdstp: prefix", template: "xdstp://authority.com/%s", - t: "0.0.0.0:8080", + target: "0.0.0.0:8080", want: "xdstp://authority.com/0.0.0.0:8080", }, { name: "with %s, with xdstp: prefix, and ipv6", template: "xdstp://authority.com/%s", - t: "[::1]:8080", + target: "[::1]:8080", want: "xdstp://authority.com/%5B::1%5D:8080", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := PopulateResourceTemplate(tt.template, tt.t); got != tt.want { + if got := PopulateResourceTemplate(tt.template, tt.target); got != tt.want { t.Errorf("PopulateResourceTemplate() = %v, want %v", got, tt.want) } }) From a141bafa123031d4de9d60514e33476364067900 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Wed, 3 Nov 2021 13:29:17 -0700 Subject: [PATCH 3/5] [federation_target_parsing] add test for client close --- xds/internal/resolver/xds_resolver_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index c6f00dac27f..ebe3ade7951 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -194,13 +194,20 @@ func (s) TestResolverBuilder(t *testing.T) { func (s) TestResolverBuilder_xdsCredsBootstrapMismatch(t *testing.T) { // Fake out the xdsClient creation process by providing a fake, which does // not have any certificate provider configuration. + fc := fakeclient.NewClient() + fc.SetBootstrapConfig(&bootstrap.Config{}) oldClientMaker := newXDSClient newXDSClient = func() (xdsclient.XDSClient, error) { - fc := fakeclient.NewClient() - fc.SetBootstrapConfig(&bootstrap.Config{}) return fc, nil } defer func() { newXDSClient = oldClientMaker }() + defer func() { + select { + case <-time.After(defaultTestTimeout): + t.Fatalf("timeout waiting for close") + case <-fc.Closed.Done(): + } + }() builder := resolver.Get(xdsScheme) if builder == nil { From c06515a19f823589b0ab96851ddcf890fe74bc5f Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Wed, 3 Nov 2021 13:39:34 -0700 Subject: [PATCH 4/5] [federation_target_parsing] refactor resolver tests to check for close --- xds/internal/resolver/xds_resolver_test.go | 104 +++++++------------- xds/internal/testutils/fakeclient/client.go | 4 +- 2 files changed, 40 insertions(+), 68 deletions(-) diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index ebe3ade7951..150f32b4714 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -229,16 +229,29 @@ func (s) TestResolverBuilder_xdsCredsBootstrapMismatch(t *testing.T) { } type setupOpts struct { - xdsClientFunc func() (xdsclient.XDSClient, error) - target *resolver.Target + bootstrapC *bootstrap.Config + target *resolver.Target } -func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *testClientConn, func()) { +func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *fakeclient.Client, *testClientConn, func()) { t.Helper() + fc := fakeclient.NewClient() + if opts.bootstrapC != nil { + fc.SetBootstrapConfig(opts.bootstrapC) + } oldClientMaker := newXDSClient - newXDSClient = opts.xdsClientFunc + newXDSClient = func() (xdsclient.XDSClient, error) { + return fc, nil + } cancel := func() { + // Make sure the xDS client is closed, in all (successful or failed) + // cases. + select { + case <-time.After(defaultTestTimeout): + t.Fatalf("timeout waiting for close") + case <-fc.Closed.Done(): + } newXDSClient = oldClientMaker } @@ -256,7 +269,10 @@ func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *testClientConn, fun if err != nil { t.Fatalf("builder.Build(%v) returned err: %v", target, err) } - return r.(*xdsResolver), tcc, cancel + return r.(*xdsResolver), fc, tcc, func() { + r.Close() + cancel() + } } // waitForWatchListener waits for the WatchListener method to be called on the @@ -362,11 +378,9 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsC.SetBootstrapConfig(tt.bc) - xdsR, _, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - target: tt.target, + xdsR, xdsC, _, cancel := testSetup(t, setupOpts{ + bootstrapC: tt.bc, + target: tt.target, }) defer cancel() defer xdsR.Close() @@ -381,10 +395,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { // TestXDSResolverWatchCallbackAfterClose tests the case where a service update // from the underlying xdsClient is received after the resolver is closed. func (s) TestXDSResolverWatchCallbackAfterClose(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer cancel() ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -413,10 +424,7 @@ func (s) TestXDSResolverWatchCallbackAfterClose(t *testing.T) { // TestXDSResolverCloseClosesXDSClient tests that the XDS resolver's Close // method closes the XDS client. func (s) TestXDSResolverCloseClosesXDSClient(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, _, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, _, cancel := testSetup(t, setupOpts{}) defer cancel() xdsR.Close() if !xdsC.Closed.HasFired() { @@ -427,10 +435,7 @@ func (s) TestXDSResolverCloseClosesXDSClient(t *testing.T) { // TestXDSResolverBadServiceUpdate tests the case the xdsClient returns a bad // service update. func (s) TestXDSResolverBadServiceUpdate(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -453,10 +458,7 @@ func (s) TestXDSResolverBadServiceUpdate(t *testing.T) { // TestXDSResolverGoodServiceUpdate tests the happy case where the resolver // gets a good service update from the xdsClient. func (s) TestXDSResolverGoodServiceUpdate(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -592,10 +594,7 @@ func (s) TestXDSResolverRequestHash(t *testing.T) { env.RingHashSupport = true defer func() { env.RingHashSupport = oldRH }() - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -652,10 +651,7 @@ func (s) TestXDSResolverRequestHash(t *testing.T) { // TestXDSResolverRemovedWithRPCs tests the case where a config selector sends // an empty update to the resolver after the resource is removed. func (s) TestXDSResolverRemovedWithRPCs(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer cancel() defer xdsR.Close() @@ -712,10 +708,7 @@ func (s) TestXDSResolverRemovedWithRPCs(t *testing.T) { // TestXDSResolverRemovedResource tests for proper behavior after a resource is // removed. func (s) TestXDSResolverRemovedResource(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer cancel() defer xdsR.Close() @@ -820,10 +813,7 @@ func (s) TestXDSResolverRemovedResource(t *testing.T) { } func (s) TestXDSResolverWRR(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -880,10 +870,7 @@ func (s) TestXDSResolverWRR(t *testing.T) { } func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -973,10 +960,7 @@ func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { // TestXDSResolverDelayedOnCommitted tests that clusters remain in service // config if RPCs are in flight. func (s) TestXDSResolverDelayedOnCommitted(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -1122,10 +1106,7 @@ func (s) TestXDSResolverDelayedOnCommitted(t *testing.T) { // TestXDSResolverUpdates tests the cases where the resolver gets a good update // after an error, and an error after the good update. func (s) TestXDSResolverGoodUpdateAfterError(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -1176,10 +1157,7 @@ func (s) TestXDSResolverGoodUpdateAfterError(t *testing.T) { // a ResourceNotFoundError. It should generate a service config picking // weighted_target, but no child balancers. func (s) TestXDSResolverResourceNotFoundError(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -1222,10 +1200,7 @@ func (s) TestXDSResolverResourceNotFoundError(t *testing.T) { // // This test case also makes sure the resolver doesn't panic. func (s) TestXDSResolverMultipleLDSUpdates(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() @@ -1397,10 +1372,7 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) { for i, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - xdsC := fakeclient.NewClient() - xdsR, tcc, cancel := testSetup(t, setupOpts{ - xdsClientFunc: func() (xdsclient.XDSClient, error) { return xdsC, nil }, - }) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) defer xdsR.Close() defer cancel() diff --git a/xds/internal/testutils/fakeclient/client.go b/xds/internal/testutils/fakeclient/client.go index 17505d87849..871aa7288c6 100644 --- a/xds/internal/testutils/fakeclient/client.go +++ b/xds/internal/testutils/fakeclient/client.go @@ -306,11 +306,11 @@ func NewClient() *Client { func NewClientWithName(name string) *Client { return &Client{ name: name, - ldsWatchCh: testutils.NewChannel(), + ldsWatchCh: testutils.NewChannelWithSize(10), rdsWatchCh: testutils.NewChannelWithSize(10), cdsWatchCh: testutils.NewChannelWithSize(10), edsWatchCh: testutils.NewChannelWithSize(10), - ldsCancelCh: testutils.NewChannel(), + ldsCancelCh: testutils.NewChannelWithSize(10), rdsCancelCh: testutils.NewChannelWithSize(10), cdsCancelCh: testutils.NewChannelWithSize(10), edsCancelCh: testutils.NewChannelWithSize(10), From ec1a81fcd664526c54df234e34c9c43b8159458e Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Mon, 8 Nov 2021 09:59:11 -0800 Subject: [PATCH 5/5] [federation_target_parsing] always set target --- xds/internal/resolver/xds_resolver_test.go | 49 ++++++++++------------ 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index 150f32b4714..e366b9fb626 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -230,7 +230,7 @@ func (s) TestResolverBuilder_xdsCredsBootstrapMismatch(t *testing.T) { type setupOpts struct { bootstrapC *bootstrap.Config - target *resolver.Target + target resolver.Target } func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *fakeclient.Client, *testClientConn, func()) { @@ -254,18 +254,13 @@ func testSetup(t *testing.T, opts setupOpts) (*xdsResolver, *fakeclient.Client, } newXDSClient = oldClientMaker } - builder := resolver.Get(xdsScheme) if builder == nil { t.Fatalf("resolver.Get(%v) returned nil", xdsScheme) } tcc := newTestClientConn() - tgt := target - if opts.target != nil { - tgt = *opts.target - } - r, err := builder.Build(tgt, tcc, resolver.BuildOptions{}) + r, err := builder.Build(opts.target, tcc, resolver.BuildOptions{}) if err != nil { t.Fatalf("builder.Build(%v) returned err: %v", target, err) } @@ -312,7 +307,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { tests := []struct { name string bc *bootstrap.Config - target *resolver.Target + target resolver.Target want string }{ { @@ -320,7 +315,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { bc: &bootstrap.Config{ ClientDefaultListenerResourceNameTemplate: "%s", }, - target: &resolver.Target{ + target: resolver.Target{ URL: url.URL{Path: "/" + targetStr}, }, want: targetStr, @@ -330,7 +325,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { bc: &bootstrap.Config{ ClientDefaultListenerResourceNameTemplate: "/path/to/%s", }, - target: &resolver.Target{ + target: resolver.Target{ URL: url.URL{Path: "/" + targetStr}, }, want: "/path/to/" + targetStr, @@ -341,7 +336,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { ClientDefaultListenerResourceNameTemplate: "xdstp://authority.com/%s", Authorities: nil, }, - target: &resolver.Target{ + target: resolver.Target{ URL: url.URL{Path: "/0.0.0.0:8080"}, }, want: "xdstp://authority.com/0.0.0.0:8080", @@ -352,7 +347,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { ClientDefaultListenerResourceNameTemplate: "xdstp://authority.com/%s", Authorities: nil, }, - target: &resolver.Target{ + target: resolver.Target{ URL: url.URL{Path: "/[::1]:8080"}, }, want: "xdstp://authority.com/%5B::1%5D:8080", @@ -367,7 +362,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { }, }, }, - target: &resolver.Target{ + target: resolver.Target{ URL: url.URL{ Host: "test-authority", Path: "/" + targetStr, @@ -395,7 +390,7 @@ func (s) TestXDSResolverResourceNameToWatch(t *testing.T) { // TestXDSResolverWatchCallbackAfterClose tests the case where a service update // from the underlying xdsClient is received after the resolver is closed. func (s) TestXDSResolverWatchCallbackAfterClose(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer cancel() ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -424,7 +419,7 @@ func (s) TestXDSResolverWatchCallbackAfterClose(t *testing.T) { // TestXDSResolverCloseClosesXDSClient tests that the XDS resolver's Close // method closes the XDS client. func (s) TestXDSResolverCloseClosesXDSClient(t *testing.T) { - xdsR, xdsC, _, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, _, cancel := testSetup(t, setupOpts{target: target}) defer cancel() xdsR.Close() if !xdsC.Closed.HasFired() { @@ -435,7 +430,7 @@ func (s) TestXDSResolverCloseClosesXDSClient(t *testing.T) { // TestXDSResolverBadServiceUpdate tests the case the xdsClient returns a bad // service update. func (s) TestXDSResolverBadServiceUpdate(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -458,7 +453,7 @@ func (s) TestXDSResolverBadServiceUpdate(t *testing.T) { // TestXDSResolverGoodServiceUpdate tests the happy case where the resolver // gets a good service update from the xdsClient. func (s) TestXDSResolverGoodServiceUpdate(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -594,7 +589,7 @@ func (s) TestXDSResolverRequestHash(t *testing.T) { env.RingHashSupport = true defer func() { env.RingHashSupport = oldRH }() - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -651,7 +646,7 @@ func (s) TestXDSResolverRequestHash(t *testing.T) { // TestXDSResolverRemovedWithRPCs tests the case where a config selector sends // an empty update to the resolver after the resource is removed. func (s) TestXDSResolverRemovedWithRPCs(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer cancel() defer xdsR.Close() @@ -708,7 +703,7 @@ func (s) TestXDSResolverRemovedWithRPCs(t *testing.T) { // TestXDSResolverRemovedResource tests for proper behavior after a resource is // removed. func (s) TestXDSResolverRemovedResource(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer cancel() defer xdsR.Close() @@ -813,7 +808,7 @@ func (s) TestXDSResolverRemovedResource(t *testing.T) { } func (s) TestXDSResolverWRR(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -870,7 +865,7 @@ func (s) TestXDSResolverWRR(t *testing.T) { } func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -960,7 +955,7 @@ func (s) TestXDSResolverMaxStreamDuration(t *testing.T) { // TestXDSResolverDelayedOnCommitted tests that clusters remain in service // config if RPCs are in flight. func (s) TestXDSResolverDelayedOnCommitted(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -1106,7 +1101,7 @@ func (s) TestXDSResolverDelayedOnCommitted(t *testing.T) { // TestXDSResolverUpdates tests the cases where the resolver gets a good update // after an error, and an error after the good update. func (s) TestXDSResolverGoodUpdateAfterError(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -1157,7 +1152,7 @@ func (s) TestXDSResolverGoodUpdateAfterError(t *testing.T) { // a ResourceNotFoundError. It should generate a service config picking // weighted_target, but no child balancers. func (s) TestXDSResolverResourceNotFoundError(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -1200,7 +1195,7 @@ func (s) TestXDSResolverResourceNotFoundError(t *testing.T) { // // This test case also makes sure the resolver doesn't panic. func (s) TestXDSResolverMultipleLDSUpdates(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel() @@ -1372,7 +1367,7 @@ func (s) TestXDSResolverHTTPFilters(t *testing.T) { for i, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{}) + xdsR, xdsC, tcc, cancel := testSetup(t, setupOpts{target: target}) defer xdsR.Close() defer cancel()