xds: Add RLS in xDS e2e test #5281

zasweq · 2022-03-29T20:38:42Z

This PR adds an RLS in xDS e2e test. This tests that the RLS in xDS functionality works, and correctly configures and uses a RLS Load Balancer. This also pulls out some testing functionality from balancer/rls/internal to balancer/rls. This addresses issue #5216.

RELEASE NOTES: None

easwars

Wrt to moving code around from balancer/rls/helpers_test.go and balancer/rls/internal/test/e2e/rls_fakeserver.go to balancer/rls/test/rls.go, it seems to be moving stuff out of an internal directory into a non-internal directory.

May I suggest the following:

Types like ListenerWrapper and ConnWrapper are nothing specific to rls. You might consider moving them to internal/testutils package
Other functionality like spinning up the fake RLS server etc, you might consider moving to some common, but internal directory. Maybe something like internal/testutils/rls or internal/rls/testutils. The only downside of these two approaches is that it would require import renaming when they are used. But I would prefer that instead of having these in non-internal directories.

xds/internal/clusterspecifier/rls/rls.go

xds/internal/httpfilter/rbac/rbac.go

easwars · 2022-03-31T04:30:22Z

xds/internal/test/xds_client_integration_test.go

@@ -28,12 +28,19 @@ import (
 	"testing"

 	"google.golang.org/grpc"
+	_ "google.golang.org/grpc/balancer/rls"


Please group blank imports in a separate group and add a one line comment for each as to why they are imported. I find that much more readable. Thanks.

easwars · 2022-03-31T04:30:50Z

xds/internal/test/xds_client_integration_test.go

@@ -55,7 +62,7 @@ func clientSetup(t *testing.T, tss testpb.TestServiceServer) (uint32, func()) {
 	// Create a local listener and pass it to Serve().
 	lis, err := testutils.LocalTCPListener()
 	if err != nil {
-		t.Fatalf("testutils.LocalTCPListener() failed: %v", err)
+		t.Fatalf("test.LocalTCPListener() failed: %v", err)


Why do we need this change?

Sorry, I originally had all the moved utils to internal/testutils, then I moved to test. This was automatically done from IDE. Switched back.

easwars · 2022-03-31T04:32:52Z

xds/internal/testutils/e2e/clientresources.go

@@ -97,6 +98,21 @@ func DefaultClientResources(params ResourceParams) UpdateOptions {
 	}
 }

+// DefaultClientResourcesWithRLSCSP returns a set of resources (LDS, RDS, CDS, EDS) for a
+// client to connect to a server with a RLS Load Balancer as a child of Cluster Manager.
+func DefaultClientResourcesWithRLSCSP(params ResourceParams, rlsProto *rlspb.RouteLookupConfig) UpdateOptions {


Does this function need to live here and be exported? Or can it live as an unexported function in the same package in which the test is defined. I don't foresee other tests needing this.

Moved to same package and changed to unexported.

easwars · 2022-03-31T04:33:07Z

xds/internal/testutils/e2e/clientresources.go

@@ -303,6 +319,33 @@ func DefaultRouteConfig(routeName, ldsTarget, clusterName string) *v3routepb.Rou
 	}
 }

+// DefaultRouteConfigWithRLSCSP returns a basic xds RouteConfig resource with an
+// RLS Cluster Specifier Plugin configured as the route.
+func DefaultRouteConfigWithRLSCSP(routeName, ldsTarget string, rlsProto *rlspb.RouteLookupConfig) *v3routepb.RouteConfiguration {


Moved to same package and changed to unexported.

easwars · 2022-03-31T04:34:35Z

xds/internal/test/xds_client_integration_test.go

+	defer func() {
+		envconfig.XDSRLS = oldRLS
+	}()
+	rlscsp.RegisterForTesting()
+	defer rlscsp.UnregisterForTesting()


Nit: combine the two statements being deferred into a single defer call?

Done. In other places in the codebase it is two defer calls, including my RBAC Filter test.

easwars · 2022-03-31T04:37:25Z

xds/internal/test/xds_client_integration_test.go

+	_, err = lis.NewConnCh.Receive(ctx)
+	if err != nil {


Please combine these two lines.

Good point. Done.

zasweq

Thanks for the comments :D! I originally moved testing helpers to internal/testutils, but there was a cyclic dependency with grpc (the fake RLS Server uses grpc). Your suggestion of moving the Conn/Listener wrappers to internal/testutils and moving the fake RLS Server to internal/testutils/rls fixed this :D!

xds/internal/clusterspecifier/rls/rls.go

xds/internal/httpfilter/rbac/rbac.go

zasweq · 2022-03-31T15:22:52Z

xds/internal/test/xds_client_integration_test.go

@@ -55,7 +62,7 @@ func clientSetup(t *testing.T, tss testpb.TestServiceServer) (uint32, func()) {
 	// Create a local listener and pass it to Serve().
 	lis, err := testutils.LocalTCPListener()
 	if err != nil {
-		t.Fatalf("testutils.LocalTCPListener() failed: %v", err)
+		t.Fatalf("test.LocalTCPListener() failed: %v", err)


Sorry, I originally had all the moved utils to internal/testutils, then I moved to test. This was automatically done from IDE. Switched back.

zasweq · 2022-03-31T15:25:17Z

xds/internal/test/xds_client_integration_test.go

+	defer func() {
+		envconfig.XDSRLS = oldRLS
+	}()
+	rlscsp.RegisterForTesting()
+	defer rlscsp.UnregisterForTesting()


Done. In other places in the codebase it is two defer calls, including my RBAC Filter test.

zasweq · 2022-03-31T15:25:56Z

xds/internal/test/xds_client_integration_test.go

+	_, err = lis.NewConnCh.Receive(ctx)
+	if err != nil {


Good point. Done.

zasweq · 2022-03-31T15:39:28Z

xds/internal/testutils/e2e/clientresources.go

@@ -97,6 +98,21 @@ func DefaultClientResources(params ResourceParams) UpdateOptions {
 	}
 }

+// DefaultClientResourcesWithRLSCSP returns a set of resources (LDS, RDS, CDS, EDS) for a
+// client to connect to a server with a RLS Load Balancer as a child of Cluster Manager.
+func DefaultClientResourcesWithRLSCSP(params ResourceParams, rlsProto *rlspb.RouteLookupConfig) UpdateOptions {


Moved to same package and changed to unexported.

zasweq · 2022-03-31T15:40:35Z

xds/internal/testutils/e2e/clientresources.go

@@ -303,6 +319,33 @@ func DefaultRouteConfig(routeName, ldsTarget, clusterName string) *v3routepb.Rou
 	}
 }

+// DefaultRouteConfigWithRLSCSP returns a basic xds RouteConfig resource with an
+// RLS Cluster Specifier Plugin configured as the route.
+func DefaultRouteConfigWithRLSCSP(routeName, ldsTarget string, rlsProto *rlspb.RouteLookupConfig) *v3routepb.RouteConfiguration {


Moved to same package and changed to unexported.

zasweq · 2022-03-31T15:56:31Z

xds/internal/test/xds_client_integration_test.go

@@ -28,12 +28,19 @@ import (
 	"testing"

 	"google.golang.org/grpc"
+	_ "google.golang.org/grpc/balancer/rls"


easwars · 2022-03-31T22:06:19Z

balancer/rls/helpers_test.go

+// Move Listener Wrapper, newListenerWrapper, and setupFakeRLSServer to rls_fakeserver.go
+// 1. Delete what was present here that I moved to prevent code duplication - then fix balancer test


Please remove this comment.

Whoops, sorry, deleted.

easwars · 2022-03-31T22:07:24Z

balancer/rls/picker_test.go

@@ -143,8 +143,8 @@ func (s) TestPick_DataCacheMiss_PendingEntryExists(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


No need to rename test to tst now that the testing package is called rlstest.

easwars · 2022-03-31T22:07:36Z

balancer/rls/picker_test.go

@@ -261,12 +261,12 @@ func (s) TestPick_DataCacheHit_NoPendingEntry_StaleEntry(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


Same here. Undo renaming.

easwars · 2022-03-31T22:07:44Z

balancer/rls/picker_test.go

@@ -361,12 +361,12 @@ func (s) TestPick_DataCacheHit_NoPendingEntry_ExpiredEntry(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


And here. And in all tests below.

easwars · 2022-03-31T22:08:34Z

internal/testutils/rls/fake_rls_server.go

@@ -16,7 +16,8 @@
 *
 */

-package e2e
+// Package rls contains utilities for RouteLookupService tests.


Package rls contains utilities for RouteLookupService e2e tests?

easwars · 2022-03-31T22:09:33Z

internal/testutils/rls/fake_rls_server.go

+	resp := s.respCb(ctx, req) // func(context.Context, *rlspb.RouteLookupRequest) *RouteLookupResponse - write your own function that sends correct CDS response backward
+	return resp.Resp, resp.Err // Set this target to correspond to correct cluster for RLS to proceed as normal


Are these changes required?

Whoops, sorry, deleted.

easwars · 2022-03-31T22:12:02Z

xds/internal/test/xds_client_integration_test.go

+	// To register the RLS Load Balancing policy.
+	_ "google.golang.org/grpc/balancer/rls"
+	// To register the RLS Cluster Specifier Plugin.
+	_ "google.golang.org/grpc/xds/internal/clusterspecifier/rls"


I meant something like this:

_ "google.golang.org/grpc/balancer/rls" // Register the RLS Load Balancing policy. _ "google.golang.org/grpc/xds/internal/clusterspecifier/rls" // Register the RLS Cluster Specifier Plugin.

Sorry for the nit.

No worries, thanks. Switched.

easwars · 2022-04-01T00:23:38Z

In #5285, I figured out a way to use the stubBalancer to use a real balancer underneath it. Once that is merged, we could get rid of the code in rls_child_policy.go and use the former instead.

zasweq

Thanks for the comments :D! Sorry about all the personal checked in comments. Noted @ your stub balancer comment.

zasweq · 2022-04-01T14:50:24Z

balancer/rls/helpers_test.go

+// Move Listener Wrapper, newListenerWrapper, and setupFakeRLSServer to rls_fakeserver.go
+// 1. Delete what was present here that I moved to prevent code duplication - then fix balancer test


Whoops, sorry, deleted.

zasweq · 2022-04-01T14:52:03Z

balancer/rls/picker_test.go

@@ -143,8 +143,8 @@ func (s) TestPick_DataCacheMiss_PendingEntryExists(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


zasweq · 2022-04-01T14:52:20Z

balancer/rls/picker_test.go

@@ -261,12 +261,12 @@ func (s) TestPick_DataCacheHit_NoPendingEntry_StaleEntry(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


zasweq · 2022-04-01T14:52:25Z

balancer/rls/picker_test.go

@@ -361,12 +361,12 @@ func (s) TestPick_DataCacheHit_NoPendingEntry_ExpiredEntry(t *testing.T) {
 		},
 	}

-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
+	for _, tst := range tests {


zasweq · 2022-04-01T14:53:04Z

internal/testutils/rls/fake_rls_server.go

@@ -16,7 +16,8 @@
 *
 */

-package e2e
+// Package rls contains utilities for RouteLookupService tests.


zasweq · 2022-04-01T14:54:00Z

internal/testutils/rls/fake_rls_server.go

+	resp := s.respCb(ctx, req) // func(context.Context, *rlspb.RouteLookupRequest) *RouteLookupResponse - write your own function that sends correct CDS response backward
+	return resp.Resp, resp.Err // Set this target to correspond to correct cluster for RLS to proceed as normal


Whoops, sorry, deleted.

zasweq · 2022-04-01T14:58:34Z

xds/internal/test/xds_client_integration_test.go

+	// To register the RLS Load Balancing policy.
+	_ "google.golang.org/grpc/balancer/rls"
+	// To register the RLS Cluster Specifier Plugin.
+	_ "google.golang.org/grpc/xds/internal/clusterspecifier/rls"


No worries, thanks. Switched.

Added RLS in xDS e2e test.

692e00d

zasweq added the Type: Feature label Mar 29, 2022

zasweq added this to the 1.46 Release milestone Mar 29, 2022

zasweq requested a review from easwars March 29, 2022 20:39

zasweq assigned easwars Mar 29, 2022

easwars changed the title ~~xds: Added RLS in xDS e2e test.~~ xds: Add RLS in xDS e2e test. Mar 31, 2022

easwars requested changes Mar 31, 2022

View reviewed changes

easwars assigned zasweq and unassigned easwars Mar 31, 2022

zasweq commented Mar 31, 2022

View reviewed changes

Responded to Easwar's comments

22badc9

zasweq assigned easwars and unassigned zasweq Mar 31, 2022

easwars requested changes Mar 31, 2022

View reviewed changes

easwars assigned zasweq and unassigned easwars Mar 31, 2022

zasweq commented Apr 1, 2022

View reviewed changes

Responded to Easwar's comments

7be9e7e

easwars approved these changes Apr 1, 2022

View reviewed changes

zasweq added 2 commits April 1, 2022 14:16

Vet

9f6c512

Added timeout to a blocking part of test

dee960b

dfawley changed the title ~~xds: Add RLS in xDS e2e test.~~ xds: Add RLS in xDS e2e test Apr 4, 2022

dfawley merged commit e583b19 into grpc:master Apr 4, 2022

github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2022

		// Move Listener Wrapper, newListenerWrapper, and setupFakeRLSServer to rls_fakeserver.go
		// 1. Delete what was present here that I moved to prevent code duplication - then fix balancer test

		resp := s.respCb(ctx, req) // func(context.Context, rlspb.RouteLookupRequest) RouteLookupResponse - write your own function that sends correct CDS response backward
		return resp.Resp, resp.Err // Set this target to correspond to correct cluster for RLS to proceed as normal

xds: Add RLS in xDS e2e test #5281

xds: Add RLS in xDS e2e test #5281

Conversation

zasweq commented Mar 29, 2022 • edited

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Apr 1, 2022

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Mar 29, 2022 •

edited