ringhash: implement gRFC A76 #8159
Conversation
atollena
added
P1
Type: Feature
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8159 +/- ##
==========================================
+ Coverage 82.42% 82.44% +0.01%
==========================================
Files 392 393 +1
Lines 39106 39212 +106
==========================================
+ Hits 32232 32327 +95
- Misses 5564 5571 +7
- Partials 1310 1314 +4
🚀 New features to boost your workflow:
|
- Add the ability to set the request hash header. This allows using ring hash without xDS. - Add the ability to specify the location of endpoints on the ring, and a default implementation based on EDS metadata that matches Envoy behavior. See https://github.com/grpc/proposal/blob/master/A76-ring-hash-improvements.md for details. Release Notes: - ringhash: implement gRFC A76.
| // XDSEndpointHashKeyBackwardCompat disables parsing of the endpoint hash | ||
| // key from EDS LbEndpoint metadata. We can disable this behavior by setting | ||
| // the environment variable "GRPC_XDS_ENDPOINT_HASH_KEY_BACKWARD_COMPAT" to | ||
| // "true". |
There was a problem hiding this comment.
nit: Since the default value of this GRPC_XDS_ENDPOINT_HASH_KEY_BACKWARD_COMPAT is true and the plan is to flip the default to false once it is stable (which is the opposite of most other env var protection). I think this comment should mention the migration strategy, e.g. the current default if unset is true and once this feature proves stable, we would flip the default to false
| @@ -708,6 +713,10 @@ func EndpointResourceWithOptions(opts EndpointOptions) *v3endpointpb.ClusterLoad | |||
| }, | |||
| } | |||
| } | |||
| metadata, err := structpb.NewStruct(b.Metadata) | |||
| if err != nil { | |||
| panic(err) | |||
There was a problem hiding this comment.
For my education, would it be the right behaviour to panic here? Or should we return an empty metadata?
| cfg.RequestHashHeader = "" | ||
| } | ||
| if cfg.RequestHashHeader != "" { | ||
| // See rules in https://github.com/grpc/proposal/blob/b64e6d3953816ed3b16b88bde0b7c16d3b62654f/A76-ring-hash-improvements.md#explicitly-setting-the-request-hash-key |
There was a problem hiding this comment.
nit: perhaps replace with just https://github.com/grpc/proposal/blob/master/A76-ring-hash-improvements.md#explicitly-setting-the-request-hash-key
| @@ -94,6 +98,27 @@ func (s) TestParseConfig(t *testing.T) { | |||
| want: nil, | |||
| wantErr: true, | |||
| }, | |||
| { | |||
| name: "request metadata key set", | |||
There was a problem hiding this comment.
there should be a test case for when there are multiple header values from the gRFC A76
If the header contains multiple values, then values are joined with a comma , character before hashing.
| if err := metadata.ValidateKey(cfg.RequestHashHeader); err != nil { | ||
| return nil, fmt.Errorf("invalid request_metadata_key %q: %s", cfg.RequestHashHeader, err) | ||
| } |
There was a problem hiding this comment.
Would metadata.ValidateKey cover the case with multiple header values from the gRFC A76?
If the header contains multiple values, then values are joined with a comma , character before hashing.
Fixes #8147.
Add the ability to set the request hash key, to extract the hash from a header. This allows using ring hash without xDS.
Add the ability to specify the location of endpoints on the ring, and a default implementation based on EDS metadata that matches Envoy behavior.
See https://github.com/grpc/proposal/blob/master/A76-ring-hash-improvements.md for details.
RELEASE NOTES: