rls: support extra_keys and constant_keys #4995
Conversation
|
FWIW: The massive change is at #4992. |
| constantKeys := make(map[string]string) | ||
| for k, v := range kb.GetConstantKeys() { | ||
| seenKeys[k] = true | ||
| constantKeys[k] = v |
There was a problem hiding this comment.
constantKeys becomes a copy of kb.GetConstantKeys(). Do we need to make a copy value by value instead of using it directly?
There was a problem hiding this comment.
If we use it directly, then the proto cannot be garbage collected, right. Not sure if this is a big deal either way. Wdyt?
There was a problem hiding this comment.
Why? The proto that references the map can still be GC'd can't it?
To be clear, the only thing I'm suggesting is:
constantKeys := kb.GetConstantKeys()There was a problem hiding this comment.
You are right. Done.
| for _, h := range kb.GetHeaders() { | ||
| if h.GetRequiredMatch() { | ||
| return nil, fmt.Errorf("rls: GrpcKeyBuilder in RouteLookupConfig has required_match field set {%+v}", kbs) | ||
| } | ||
| key := h.GetKey() | ||
| if seenKeys[key] { | ||
| return nil, fmt.Errorf("rls: GrpcKeyBuilder in RouteLookupConfig contains repeated Key field in headers {%+v}", kbs) | ||
| return nil, fmt.Errorf("rls: GrpcKeyBuilder in RouteLookupConfig contains repeated key across headers, constant_keys and extra_keys {%+v}", kbs) |
There was a problem hiding this comment.
Please include key which was repeated. And this can leave out extra_keys since that hasn't been checked yet.
| } | ||
| seenKeys[key] = true | ||
| matchers = append(matchers, matcher{key: h.GetKey(), names: h.GetNames()}) | ||
| } | ||
| b := builder{matchers: matchers} | ||
| if seenKeys[kb.GetExtraKeys().GetHost()] || seenKeys[kb.GetExtraKeys().GetService()] || seenKeys[kb.GetExtraKeys().GetMethod()] { | ||
| return nil, fmt.Errorf("rls: GrpcKeyBuilder in RouteLookupConfig contains repeated key across headers, constant_keys and extra_keys {%+v}", kbs) |
There was a problem hiding this comment.
Same: include keys and we know it's extra_keys that has the dupe. So "extra_keys contains duplicate key from constant_keys or headers" or something.
| for k, v := range b.constantKeys { | ||
| kvMap[k] = v | ||
| } | ||
| return KeyMap{Map: kvMap, Str: mapToString(kvMap)} |
There was a problem hiding this comment.
Do we need to store Str in KeyMap? Or can we compute it when we add/retrieve from the map? (Basically: if we're only consuming Str once, we shouldn't store it.)
There was a problem hiding this comment.
RLSKey method will be invoked for every Pick, and the returned keys (string form) will be used to lookup the data cache. There is no sharing of the key map between RPCs. So, we can either compute it here or Pick will have to compute it right after it calls this method. I don't have much of a preference and I'm leaning towards leaving it as is, to keep Pick working the way it currently does. Do you have a preference?
There was a problem hiding this comment.
For now it seems better to avoid storing it to me, but let's wait for the rest of the changes to decide if it's worth changing.
| matchers []matcher | ||
| headerKeys []matcher | ||
| constantKeys map[string]string | ||
| // The following keys mirror corresponding field in `extra_keys`. |
| kvMap := make(map[string]string) | ||
| for _, m := range b.matchers { | ||
| if md == nil { |
There was a problem hiding this comment.
if len(md) == 0 is probably even better.
Also it seems this is an allocation in the RPC path, so I'd return nil and move the make down below this if to avoid any unnecessary allocations.
And if it's possible to avoid this allocation altogether, that would be even better.
There was a problem hiding this comment.
Switched to checking for non-zero length.
We cannot avoid this allocation as the caller of this method, RLSKey(), will try to add extra_keys and constant_keys to the returned map. We could add the following block before every possible assignment in RLSKey
if kvMap == nil {
kvMap = make(map[string]string)
}but that would be a lot of boilerplate. Wdyt?
There was a problem hiding this comment.
Oops, missed this. No, what you have is fine in that case.
grpc/grpc-proto#92 added new fields
extra_keys(to includehost,serviceandmethodkeys) andconstant_keys(which are added to every request). This PR adds supports for those fields in our keyBuilder implementation.The changes in
pickerin this PR are the ones minimally required to get things to build and are only a subset of the upcoming changes.RELEASE NOTES: n/a