xds: Add RLS Cluster Specifier Plugin #5004

zasweq · 2021-11-24T02:02:00Z

This PR adds the RLS Cluster Specifier Plugin. This is branched off #4987. The RLS Cluster Specifier Plugin will not be functional until the RLS LB Policy itself gets completed, as right now the call into the RLS LB Policies ParseConfig() for validation purposes is commented out.

RELEASE NOTES: None

dfawley · 2021-11-24T20:13:50Z

xds/internal/clusterspecifier/rls/rls.go

+// LBConfigJSON is the RLS LB Policies configuration in JSON format.
+// RouteLookupConfig will be a raw JSON string from the passed in proto
+// configuration, and the other fields will be hardcoded.
+type LBConfigJSON struct {


This type shouldn't need to be exported.

Switched. This was exported only for testing, now that we moved testing to same package I can unexport.

dfawley · 2021-11-24T20:21:34Z

xds/internal/clusterspecifier/rls/rls.go

+// RouteLookupConfig will be a raw JSON string from the passed in proto
+// configuration, and the other fields will be hardcoded.
+type LBConfigJSON struct {
+	RouteLookupConfig                interface{}              `json:"routeLookupConfig"`


Switched to json.RawMessage (an alias for []byte).

dfawley · 2021-11-24T20:28:57Z

xds/internal/clusterspecifier/rls/rls.go

+	if err := ptypes.UnmarshalAny(any, rlcs); err != nil {
+		return nil, fmt.Errorf("rls_csp: error parsing config %v: %v", cfg, err)
+	}
+	// Do we use protojson or regular marshal function - see if Marshal produces correct results?


Why wouldn't we use protojson? Even if json.Marshal happens to work given what's currently there, protojson is what you're supposed to use to marshal proto into JSON and vice-versa.

Switched. I think I could only find two instances throughout the codebase of using protojson, which is why I was a touch hesitant. Even ParseConfig for the RLS LB policy itself (json raw bytestring -> RouteLookupConfig proto) in master doesn't use protojson: https://github.com/grpc/grpc-go/blob/master/balancer/rls/internal/config.go#L132.

dfawley · 2021-11-24T20:34:46Z

xds/internal/clusterspecifier/rls/rls.go

+	// return nil, fmt.Errorf("error: %v", err)
+	// }
+
+	return []map[string]interface{}{{"rls_experimental": lbCfgJSON}}, nil


return clusterspecifier.BalancerConfig{{"rls_experimental": lbCfgJSON}}, nil

dfawley · 2021-11-24T20:35:36Z

xds/internal/clusterspecifier/rls/rls.go

+	// Will be uncommented once RLS LB Policy completed
+
+	// rawJSON, err := json.Marshal(lbCfgJSON)
+	// if err != nil {
+	//	return nil, fmt.Errorf("rls_csp: error marshaling load balancing config %v: %v", lbCfgJSON, err)
+	// }
+
+	// _, err := rls.ParseConfig(rawJSON) (will this function need to change its logic at all?)
+	// if err != nil {
+	// return nil, fmt.Errorf("error: %v", err)
+	// }


rls.ParseConfig exists, actually. Can we make it accessible and use it?

Also, it would be used via:

rlsBB := balancer.Get("experimental_rls") if rlsBB == nil { return nil, fmt.Errorf("RLS LB policy not registered") } rlsBB.(balancer.ConfigParser).ParseConfig(.....)

dfawley · 2021-11-24T20:41:44Z

xds/internal/clusterspecifier/rls/rls.go

+		RouteLookupConfig: rlcJSON, // "JSON form of RouteLookupClusterSpecifier.config" - RLS in xDS Design Doc
+		ChildPolicy: []map[string]interface{}{
+			{
+				"cds_experimental": struct{}{}, // Is this right? {} in design doc


Does nil work?

I marshaled {} into a json.RawMessage to pass RLS Config Parsing.

dfawley · 2021-11-24T20:45:06Z

xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go

+var rlsClusterSpecifierConfig = testutils.MarshalAny(&grpc_lookup_v1.RouteLookupClusterSpecifier{
+	// Once Easwar merges the RLS LB implementation and the rls csp actually
+	// calls into it for validation, this config will have to be scaled up to
+	// pass the validations present in the RLS LB implementation.
+	RouteLookupConfig: &grpc_lookup_v1.RouteLookupConfig{
+		LookupService: "rls-specifier",
+	},
+})


I'm not sure this is the right way to test this.

The xdsclient tests shouldn't rely on any specific plugins, and the tests for any specific plugins should not be done in the xdsclient tests. Instead, the rls plugin's tests should be done in xds/internal/clusterspecifier/rls.

Hmmmmmm ok noted. Switched to package, and tested input/output of ParseClusterSpecifierConfig().

Actually, manually testing and comparing the output (containing long marshaled json.RawMessage) understandably led to some flakiness. I pushed the fix in a separate commit, that way you can see what I had previously, and to see if you'd like me to go back to that and figure out an unflaky way to test the raw JSON string output (the problem was marshaling the proto would sometimes pop out spaces in between fields, sometimes not).

zasweq

Got to all your comments, but commit is still a WIP.

zasweq · 2021-11-29T17:08:39Z

xds/internal/clusterspecifier/rls/rls.go

+	// return nil, fmt.Errorf("error: %v", err)
+	// }
+
+	return []map[string]interface{}{{"rls_experimental": lbCfgJSON}}, nil


zasweq · 2021-11-29T17:19:23Z

xds/internal/clusterspecifier/rls/rls.go

+	if err := ptypes.UnmarshalAny(any, rlcs); err != nil {
+		return nil, fmt.Errorf("rls_csp: error parsing config %v: %v", cfg, err)
+	}
+	// Do we use protojson or regular marshal function - see if Marshal produces correct results?


Switched. I think I could only find two instances throughout the codebase of using protojson, which is why I was a touch hesitant. Even ParseConfig for the RLS LB policy itself (json raw bytestring -> RouteLookupConfig proto) in master doesn't use protojson: https://github.com/grpc/grpc-go/blob/master/balancer/rls/internal/config.go#L132.

zasweq · 2021-11-29T17:22:45Z

xds/internal/clusterspecifier/rls/rls.go

+// RouteLookupConfig will be a raw JSON string from the passed in proto
+// configuration, and the other fields will be hardcoded.
+type LBConfigJSON struct {
+	RouteLookupConfig                interface{}              `json:"routeLookupConfig"`


Switched to json.RawMessage (an alias for []byte).

zasweq · 2021-11-30T00:32:25Z

xds/internal/clusterspecifier/rls/rls.go

+// LBConfigJSON is the RLS LB Policies configuration in JSON format.
+// RouteLookupConfig will be a raw JSON string from the passed in proto
+// configuration, and the other fields will be hardcoded.
+type LBConfigJSON struct {


Switched. This was exported only for testing, now that we moved testing to same package I can unexport.

zasweq · 2021-11-30T02:35:55Z

xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go

+var rlsClusterSpecifierConfig = testutils.MarshalAny(&grpc_lookup_v1.RouteLookupClusterSpecifier{
+	// Once Easwar merges the RLS LB implementation and the rls csp actually
+	// calls into it for validation, this config will have to be scaled up to
+	// pass the validations present in the RLS LB implementation.
+	RouteLookupConfig: &grpc_lookup_v1.RouteLookupConfig{
+		LookupService: "rls-specifier",
+	},
+})


Hmmmmmm ok noted. Switched to package, and tested input/output of ParseClusterSpecifierConfig().

zasweq · 2021-11-30T02:42:40Z

xds/internal/clusterspecifier/rls/rls.go

+	// Will be uncommented once RLS LB Policy completed
+
+	// rawJSON, err := json.Marshal(lbCfgJSON)
+	// if err != nil {
+	//	return nil, fmt.Errorf("rls_csp: error marshaling load balancing config %v: %v", lbCfgJSON, err)
+	// }
+
+	// _, err := rls.ParseConfig(rawJSON) (will this function need to change its logic at all?)
+	// if err != nil {
+	// return nil, fmt.Errorf("error: %v", err)
+	// }


dfawley · 2021-12-07T21:36:45Z

xds/internal/clusterspecifier/rls/rls.go

+		return nil, fmt.Errorf("rls_csp: validation error from rls lb policy parsing %v", err)
+	}
+
+	return clusterspecifier.BalancerConfig{{"rls_experimental": lbCfgJSON}}, nil


I was going to suggest using an exported constant in the RLS package, but I don't think we can do that for visibility reasons without moving stuff around invasively.

Will the tests break when there is no "rls_experimental" LB policy anymore (it will eventually be renamed to "rls")? If so, then I think this is fine.

This is named rls in master. I'm going to switch what gets emitted out of this function to rls: config, that way once it gets to balancer group, it will successfully do balancer.Get("rls").

As discussed, please rename to rls_experimental throughout.

dfawley · 2021-12-07T21:37:39Z

xds/internal/clusterspecifier/rls/rls.go

+ */
+
+// Package rls implements the RLS cluster specifier plugin.
+package rls


Should this be importing the RLS package? Seems like it should.

Added import of the RLS package.

dfawley · 2021-12-07T21:38:37Z

xds/internal/clusterspecifier/rls/rls.go

+)
+
+func init() {
+	clusterspecifier.Register(rls{})


Should this be conditional on GRPC_EXPERIMENTAL_XDS_RLS_LB?

dfawley · 2021-12-07T21:39:59Z

xds/internal/clusterspecifier/rls/rls.go

+	if err != nil {
+		return nil, fmt.Errorf("rls_csp: error marshaling route lookup config: %v: %v", rlcs.GetRouteLookupConfig(), err)
+	}
+	emptyJSON, err := json.Marshal(struct{}{})


Isn't this just json.RawMessage("{}")? If so, let's use that (inline below) instead of something that can error.

dfawley · 2021-12-07T21:41:52Z

xds/internal/clusterspecifier/rls/rls.go

+		return nil, fmt.Errorf("rls_csp: error marshaling load balancing config %v: %v", lbCfgJSON, err)
+	}
+
+	rlsBB := balancer.Get("rls")


Do this as a global?

Also...shouldn't this match "rls_experimental" below? Can you use a single const for both of these?

This is the name it's registered by in balancer/rls/internal. Below corresponds to what is defined in design doc. Switched the below config to "rls", as that is what is currently in master, so used one const for both.

dfawley · 2021-12-07T21:43:47Z

xds/internal/clusterspecifier/rls/rls_test.go

+		if cs == nil {
+			t.Fatal("Error getting cluster specifier")
+		}
+		_, err := cs.ParseClusterSpecifierConfig(test.rlcs)


Validate the result? (I'm assuming it would be marshaling the result to JSON and compare to a string.)

Done, with a solution discussed offline.

I was expecting to see something like this here now:

lbCfgJSON, err := json.Marshal(lbCfg) // handle err var got interface{} err := json.Unmarshal(lbCfgJSON, &got) // handle err wantJSON := ` { // JSON that we wanted } ` var want interface{} err := json.Unmarshal(wantJSON, &want) // handle err if diff := cmp.Diff(got, want); diff != "" { // error }

Done. Although I kept the want as a struct, and then Marshaled and then Unmarshaled. I think it's cleaner.

zasweq

Whoops, these were sitting in pending.

zasweq · 2021-12-08T00:15:38Z

xds/internal/clusterspecifier/rls/rls.go

+ */
+
+// Package rls implements the RLS cluster specifier plugin.
+package rls


Added import of the RLS package.

zasweq · 2021-12-08T00:15:57Z

xds/internal/clusterspecifier/rls/rls.go

+)
+
+func init() {
+	clusterspecifier.Register(rls{})


zasweq · 2021-12-08T00:18:21Z

xds/internal/clusterspecifier/rls/rls.go

+	if err != nil {
+		return nil, fmt.Errorf("rls_csp: error marshaling route lookup config: %v: %v", rlcs.GetRouteLookupConfig(), err)
+	}
+	emptyJSON, err := json.Marshal(struct{}{})


zasweq · 2021-12-08T00:18:57Z

xds/internal/clusterspecifier/rls/rls.go

+		return nil, fmt.Errorf("rls_csp: error marshaling load balancing config %v: %v", lbCfgJSON, err)
+	}
+
+	rlsBB := balancer.Get("rls")


This is the name it's registered by in balancer/rls/internal. Below corresponds to what is defined in design doc. Switched the below config to "rls", as that is what is currently in master, so used one const for both.

zasweq · 2021-12-08T00:20:35Z

xds/internal/clusterspecifier/rls/rls.go

+		return nil, fmt.Errorf("rls_csp: validation error from rls lb policy parsing %v", err)
+	}
+
+	return clusterspecifier.BalancerConfig{{"rls_experimental": lbCfgJSON}}, nil


This is named rls in master. I'm going to switch what gets emitted out of this function to rls: config, that way once it gets to balancer group, it will successfully do balancer.Get("rls").

zasweq · 2021-12-08T00:53:06Z

xds/internal/clusterspecifier/rls/rls_test.go

+		if cs == nil {
+			t.Fatal("Error getting cluster specifier")
+		}
+		_, err := cs.ParseClusterSpecifierConfig(test.rlcs)


Done, with a solution discussed offline.

dfawley · 2021-12-15T21:16:24Z

balancer/rls/internal/builder.go

@@ -24,7 +24,7 @@ import (
 	"google.golang.org/grpc/internal/grpcsync"
 )

-const rlsBalancerName = "rls"
+const rlsBalancerName = "rls_experimental"


Looks like this was changed on master; hopefully rebasing will go smoothly.

dfawley · 2021-12-15T21:19:31Z

xds/internal/clusterspecifier/rls/rls_test.go

+		if !cmp.Equal(want, got, cmpopts.EquateEmpty()) {
+			t.Fatalf("ParseClusterSpecifierConfig(%+v) returned expected, diff (-want +got):\\n%s", test.rlcs, cmp.Diff(want, got, cmpopts.EquateEmpty()))
 		}


Use if diff := cmp.Diff(want, got, cmpopts.EquateEmpty()); diff != "" { and then there's no need to call Diff in the error message.

zasweq requested a review from dfawley November 24, 2021 02:02

zasweq assigned dfawley Nov 24, 2021

zasweq added the Type: Feature label Nov 24, 2021

zasweq added this to the 1.43 release milestone Nov 24, 2021

dfawley mentioned this pull request Nov 24, 2021

draft: Added RLS Cluster Specifier Plugin #4996

Closed

dfawley reviewed Nov 24, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Nov 24, 2021

zasweq commented Nov 30, 2021

View reviewed changes

zasweq assigned dfawley and unassigned zasweq Dec 1, 2021

zasweq force-pushed the add-rls-csp-rebased branch from e48fc23 to 560f642 Compare December 6, 2021 20:08

dfawley modified the milestones: 1.43 release, 1.44 Release Dec 6, 2021

dfawley reviewed Dec 7, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Dec 7, 2021

zasweq assigned dfawley and unassigned zasweq Dec 9, 2021

zasweq commented Dec 10, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Dec 14, 2021

zasweq assigned dfawley and unassigned zasweq Dec 15, 2021

dfawley approved these changes Dec 15, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Dec 15, 2021

zasweq added 3 commits December 15, 2021 17:24

Added support for cluster specifier plugins in xds resolver

a650d1a

Responded to Doug's comments

9f5b9b9

Responded to Doug's comments

57d2dcf

zasweq added 2 commits December 15, 2021 17:26

Responded to Doug's comments

0abbe42

Responded to Doug's comments

4a1b633

zasweq force-pushed the add-rls-csp-rebased branch from 7dbaec0 to 4a1b633 Compare December 15, 2021 22:34

zasweq merged commit 029b822 into grpc:master Dec 15, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2022

xds: Add RLS Cluster Specifier Plugin #5004

xds: Add RLS Cluster Specifier Plugin #5004

Conversation

zasweq commented Nov 24, 2021

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 Nov 30, 2021 • edited

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 Dec 1, 2021 • edited

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

zasweq Nov 30, 2021 • edited

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 Dec 8, 2021 • edited

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 Dec 8, 2021 • edited

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

zasweq Dec 8, 2021 • edited

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 Dec 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq Nov 30, 2021 •

edited

zasweq Dec 1, 2021 •

edited

zasweq Nov 30, 2021 •

edited

zasweq Dec 8, 2021 •

edited

zasweq Dec 8, 2021 •

edited

zasweq Dec 8, 2021 •

edited

zasweq Dec 8, 2021 •

edited