Add terminal attribute to request hash. by stevenzzzz · Pull Request #4292 · envoyproxy/envoy

stevenzzzz · 2018-08-29T14:58:54Z

Add a terminal attribute to request hash policy.

Think about a case where customers want to hash on a cookie if it's present but if it's not present, do best-effort sticky based on something like IP so the customer has a stable hash.

This "terminal" allows request hashing to have the ability of "if A not working, fallback to B.", which also saves time to generate the hash.

Changes:
* Add a terminal attribute to HashMethod, which shortcircuit the hash generating process if a policy is marked terminal and there is a hash computed already.

Signed-off-by: Xin Zhuang stevenzzz@google.com

Description: Add terminal attribute to request hash.
Risk Level: Low
Testing: unit tests.
[Optional Fixes #Issue]
[Optional Deprecated:]

…ow CookieHashMethod to use whole cookies header by not specifying a cookie name. Changes: * Add a terminal attribute to HashMethod, which shortcircuit the hash generating process if a policy is marked terminal and there is a hash computed already. * Add QueryParams hash method to hash on URL query parameter value. * Add HostPath hash method to hash on HOST and PATH in URL. * Allow CookieHashMethod to hash on the cookies header if no cookie key specified. Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch · 2018-08-29T17:41:52Z

@akonradi has best context on this code, so will defer to him for primary review.

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

akonradi

Sending initial thoughts for discussion while I keep reading the tests.

source/common/router/config_impl.cc

api/envoy/api/v2/route/route.proto

akonradi · 2018-08-29T18:09:36Z

api/envoy/api/v2/route/route.proto

-  // identical lists of hash policies will produce the same hash. Since a hash
+  // identical lists of hash policies will produce the same hash.Since a hash
  // policy examines specific parts of a request, it can fail to produce a hash
  // (i.e. if the hashed header is not present). If (and only if) all configured


Can you update this section with info about the terminal field? It seems important but the documentation for it is kind of buried up above.

source/common/router/config_impl.cc

akonradi · 2018-08-29T18:14:59Z

source/common/router/config_impl.cc

      const uint64_t old_value = hash ? ((hash.value() << 1) | (hash.value() >> 63)) : 0;
      hash = old_value ^ new_hash.value();
    }
+    // If the policy is a terminal policy and there are hash generated, ignore


nits:
"there are hash" -> "a hash was"
"rest" -> "rest of the"

akonradi

Review of tests.

test/common/router/config_impl_test.cc

…about adding the "terminal" attribute. Test adjusted accordingly. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz · 2018-08-31T18:45:37Z

@akonradi Hey Alex please take a look at your convenience. :) Thx!

akonradi

LGTM modulo minor nits.

akonradi · 2018-08-31T18:48:04Z

test/common/router/config_impl_test.cc

 }

+TEST_F(RouterMatcherHashPolicyTest, HashTerminal) {
+  // Hash pilicy list: cookie, header [terminal=true], user_ip.


nit: policy

akonradi · 2018-08-31T18:53:30Z

test/common/router/config_impl_test.cc

+
+  uint64_t hash_1, hash_2;
+  // Test terminal works when there is hash computed, the rest of the policy
+  // list is ignored.


Can you add a note about how these are using different addresses? It was hard to make out the one-character difference at first glance.

Thanks, and done.

test 123 ( I don't know how github UI works)

test/common/router/config_impl_test.cc

akonradi · 2018-08-31T19:05:49Z

api/envoy/api/v2/route/route.proto

      ConnectionProperties connection_properties = 3;
    }
+    // Reserve for new policy_specifier.
+    /* RESERVED */ reserved 4 to 20;


Out of curiosity, why are we reserving these field numbers?

I think we will add more hash method into the oneof list above and I naively think it'd be nice if we have a ascending order for all the policy_specifier to be added.
Revert it as it's not the general recognition.

Thanks Alex! Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

htuch

Yeah, PR changes look great (thanks @akonradi for the review). I would like to make sure we communicate clearly the value being delivered here.

htuch · 2018-09-03T13:39:12Z

api/envoy/api/v2/route/route.proto

+    //   [(Header A,    true),
+    //    (Cookie B,    false),
+    //    (Cookie C,    false)]
+    // The generateHash process ends after computing "header A", as it's a terminal policy.


Can you add to the commit message and comments here the motivation for the terminal attribute; is there some real-world use case where this is important? Knowing what you are working on, I'm sure there is, but it would be helpful to share.

Merge updates from upstream. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz · 2018-09-04T18:25:36Z

@htuch @akonradi PTAL at your convenience.
Thanks!

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

htuch · 2018-09-04T23:36:12Z

api/envoy/api/v2/route/route.proto

+
+    // The flag that shortcircuits the hash computing. This field provides a
+    // 'fallback' style of configuration: "if a terminal policy doesn't work,
+    // fallback to rest of the policy list", it saves time when the terminal


Is this only about performance, or is there a functional aspect? I.e. avoiding taking unnecessary detail into account when computing the hash? We can talk IRL tomorrow if it would be easier to describe the use case then.

It's about functionality: it provides a fallback for a primary hash method when it didn't work. The performance part is a little gain comes with this attribute.
Updated the PR description to sate it a little bit more clear. Thanks for pointing it out!

htuch

Thanks!

htuch requested a review from akonradi August 29, 2018 17:41

htuch assigned akonradi Aug 29, 2018

fix error caught by CI asan.

5adeff2

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

akonradi suggested changes Aug 29, 2018

View reviewed changes

akonradi reviewed Aug 29, 2018

View reviewed changes

Revert the HostPath and QueryParam hash methods to make this PR only …

dff9c22

…about adding the "terminal" attribute. Test adjusted accordingly. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz force-pushed the request-hashing branch from 83390f8 to dff9c22 Compare August 31, 2018 18:43

akonradi previously approved these changes Aug 31, 2018

View reviewed changes

Review fixes.

987179c

Thanks Alex! Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

stevenzzzz dismissed akonradi’s stale review via 987179c August 31, 2018 19:28

stevenzzzz changed the title ~~Add terminal attribute to request hash and two more hash methods.~~ Add terminal attribute to request hash. Aug 31, 2018

htuch reviewed Sep 3, 2018

View reviewed changes

Xin Zhuang added 2 commits September 4, 2018 14:09

Merge branch 'master' into request-hashing

450448b

Merge updates from upstream. Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

Add more description for request hash policy terminal attr.

c93bd57

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

fix the doc format problem in route.proto for terminal attr.

261ae3a

Signed-off-by: Xin Zhuang <stevenzzz@stevenzzz6.cam.corp.google.com>

htuch reviewed Sep 4, 2018

View reviewed changes

htuch approved these changes Sep 5, 2018

View reviewed changes

htuch merged commit ee710d0 into envoyproxy:master Sep 5, 2018

stevenzzzz deleted the request-hashing branch September 6, 2018 14:10

Conversation

stevenzzzz commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Aug 29, 2018

Uh oh!

akonradi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akonradi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzzzz commented Aug 31, 2018

Uh oh!

akonradi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzzzz commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevenzzzz commented Aug 29, 2018 •

edited

Loading