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

rls: support extra_keys and constant_keys #4995

Merged
merged 3 commits into from Dec 3, 2021

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 19, 2021

grpc/grpc-proto#92 added new fields extra_keys (to include host, service and method keys) and constant_keys (which are added to every request). This PR adds supports for those fields in our keyBuilder implementation.

The changes in picker in 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

@easwars easwars requested a review from dfawley Nov 19, 2021
@easwars easwars added this to the 1.43 release milestone Nov 19, 2021
@easwars
Copy link
Contributor Author

@easwars easwars commented Nov 20, 2021

FWIW: The massive change is at #4992.

constantKeys := make(map[string]string)
for k, v := range kb.GetConstantKeys() {
seenKeys[k] = true
constantKeys[k] = v
Copy link
Contributor

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

constantKeys becomes a copy of kb.GetConstantKeys(). Do we need to make a copy value by value instead of using it directly?

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

If we use it directly, then the proto cannot be garbage collected, right. Not sure if this is a big deal either way. Wdyt?

Copy link
Contributor

@dfawley dfawley Dec 3, 2021

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

@easwars easwars Dec 3, 2021

Choose a reason for hiding this comment

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

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

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

Please include key which was repeated. And this can leave out extra_keys since that hasn't been checked yet.

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

Done.

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

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

Done.

for k, v := range b.constantKeys {
kvMap[k] = v
}
return KeyMap{Map: kvMap, Str: mapToString(kvMap)}
Copy link
Contributor

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dfawley dfawley Dec 3, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@easwars easwars Dec 3, 2021

Choose a reason for hiding this comment

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

Ack.

matchers []matcher
headerKeys []matcher
constantKeys map[string]string
// The following keys mirror corresponding field in `extra_keys`.
Copy link
Contributor

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

s/field/fields/

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

Done.

kvMap := make(map[string]string)
for _, m := range b.matchers {
if md == nil {
Copy link
Contributor

@dfawley dfawley Nov 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@easwars easwars Nov 29, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dfawley dfawley Dec 3, 2021

Choose a reason for hiding this comment

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

Oops, missed this. No, what you have is fine in that case.

@dfawley dfawley assigned easwars and unassigned dfawley Nov 24, 2021
@easwars easwars assigned dfawley and unassigned easwars Nov 29, 2021
@dfawley dfawley assigned easwars and unassigned dfawley Dec 3, 2021
@easwars easwars assigned dfawley and unassigned easwars Dec 3, 2021
dfawley
dfawley approved these changes Dec 3, 2021
@easwars easwars merged commit 512e894 into grpc:master Dec 3, 2021
10 checks passed
@easwars easwars deleted the rls_support_extra_and_constant_keys branch Dec 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants