Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: Add RLS in xDS e2e test #5281

Merged
merged 5 commits into from Apr 4, 2022
Merged

xds: Add RLS in xDS e2e test #5281

merged 5 commits into from Apr 4, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Mar 29, 2022

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

@zasweq zasweq added this to the 1.46 Release milestone Mar 29, 2022
@zasweq zasweq requested a review from easwars Mar 29, 2022
@easwars easwars changed the title xds: Added RLS in xDS e2e test. xds: Add RLS in xDS e2e test. Mar 31, 2022
Copy link
Contributor

@easwars easwars left a comment

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 Show resolved Hide resolved
xds/internal/httpfilter/rbac/rbac.go Show resolved Hide resolved
@@ -28,12 +28,19 @@ import (
"testing"

"google.golang.org/grpc"
_ "google.golang.org/grpc/balancer/rls"
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Done.

@@ -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)
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

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

@@ -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 {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Moved to same package and changed to unexported.

@@ -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 {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Moved to same package and changed to unexported.

defer func() {
envconfig.XDSRLS = oldRLS
}()
rlscsp.RegisterForTesting()
defer rlscsp.UnregisterForTesting()
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

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

_, err = lis.NewConnCh.Receive(ctx)
if err != nil {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Please combine these two lines.

Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Good point. Done.

@easwars easwars assigned zasweq and unassigned easwars Mar 31, 2022
Copy link
Contributor Author

@zasweq zasweq left a comment

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 Show resolved Hide resolved
xds/internal/httpfilter/rbac/rbac.go Show resolved Hide resolved
@@ -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)
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

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

defer func() {
envconfig.XDSRLS = oldRLS
}()
rlscsp.RegisterForTesting()
defer rlscsp.UnregisterForTesting()
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

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

_, err = lis.NewConnCh.Receive(ctx)
if err != nil {
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Good point. Done.

@@ -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 {
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Moved to same package and changed to unexported.

@@ -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 {
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Moved to same package and changed to unexported.

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

"google.golang.org/grpc"
_ "google.golang.org/grpc/balancer/rls"
Copy link
Contributor Author

@zasweq zasweq Mar 31, 2022

Choose a reason for hiding this comment

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

Done.

@zasweq zasweq assigned easwars and unassigned zasweq Mar 31, 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
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Please remove this comment.

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Whoops, sorry, deleted.

@@ -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 {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

@@ -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 {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Same here. Undo renaming.

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

@@ -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 {
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

And here. And in all tests below.

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

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

package e2e
// Package rls contains utilities for RouteLookupService tests.
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Package rls contains utilities for RouteLookupService e2e tests?

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Added.

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
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

Are these changes required?

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Whoops, sorry, deleted.

// 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"
Copy link
Contributor

@easwars easwars Mar 31, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

No worries, thanks. Switched.

@easwars easwars assigned zasweq and unassigned easwars Mar 31, 2022
@easwars
Copy link
Contributor

@easwars easwars commented Apr 1, 2022

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.

Copy link
Contributor Author

@zasweq zasweq left a comment

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

// 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
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Whoops, sorry, deleted.

@@ -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 {
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

@@ -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 {
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

@@ -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 {
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Switched.

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

package e2e
// Package rls contains utilities for RouteLookupService tests.
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Added.

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
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

Whoops, sorry, deleted.

// 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"
Copy link
Contributor Author

@zasweq zasweq Apr 1, 2022

Choose a reason for hiding this comment

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

No worries, thanks. Switched.

easwars
easwars approved these changes Apr 1, 2022
@dfawley dfawley changed the title xds: Add RLS in xDS e2e test. xds: Add RLS in xDS e2e test Apr 4, 2022
@dfawley dfawley merged commit e583b19 into grpc:master Apr 4, 2022
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants