Support multiple hash policies for HTTP routing by akonradi · Pull Request #1756 · envoyproxy/envoy

akonradi · 2017-09-27T15:27:32Z

Fixes envoyproxy#1422 Signed-off-by: Alex Konradi <akonradi@google.com>

htuch

General approach looks good.

htuch · 2017-09-27T15:38:44Z

source/common/router/config_impl.h

  Optional<uint64_t> generateHash(const std::string& downstream_addr,
                                  const Http::HeaderMap& headers) const override;

+  class HashImpl {


If this is a pure interface, it probably belongs in include/envoy/router. Also, for conventions, we tend not to suffix pure interfaces with Impl.

My thought was that it's part of HashPolicyImpl's implementation, not part of its publicly accessible API. I'll change "HashImpl" to "HashMethod"; the intent was to signify that this is an interface for various ways of producing a hash.

htuch · 2017-09-27T15:42:24Z

source/common/router/config_impl.cc

+  for (const HashImplPtr& hash_impl : hash_impls_) {
+    Optional<uint64_t> new_hash = hash_impl->evaluate(downstream_addr, headers);
+    if (new_hash.valid()) {
+      hash.value(hash.valid() ? (hash.value() ^ new_hash.value()) : new_hash.value());


You might want something like a shift + XOR or multiplication (I don't think performance is a big issue here, it's a few cycles more). Otherwise, you can get degenerate situations, for example the same hash policy repeated twice, which will just give you zero always.

htuch · 2017-09-27T15:43:59Z

source/common/router/config_impl.cc

+      }
+      break;
+    default:
+      ASSERT(false);


Might consider moving to throwing an EnvoyException here. I was a bit lazy initially as I expected to fill in all cases (or have constraints to cover them), but if we're writing new code, we should either add a TODO about using constraints to catch this/implement or throw.

htuch · 2017-09-27T15:44:44Z

source/common/router/config_impl.cc

+    if (header) {
+      hash.value(HashUtil::xxHash64(header->value().c_str()));
+    }
+    return hash;


Is it worth the complexity to have this optional? Why not just return a zero hash value if missing?

My intent was to remain compatible with the existing interface. Zero is a valid hash value, even if it doesn't show up often.

htuch · 2017-09-27T15:45:05Z

source/common/router/config_impl.cc

+public:
+  IpHashImpl() {}
+
+  ~IpHashImpl() override {}


Don't need to override these if default.

Signed-off-by: Alex Konradi <akonradi@google.com>

This prevents the degenerate case that htuch mentioned, where a duplicated hash policy would cancel out the first one. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

htuch

LGTM

htuch · 2017-09-27T17:51:15Z

source/common/router/config_impl.cc

-    if (header) {
-      hash.value(HashUtil::xxHash64(header->value().c_str()));
+  for (const HashMethodPtr& hash_impl : hash_impls_) {
+    Optional<uint64_t> new_hash = hash_impl->evaluate(downstream_addr, headers);


htuch · 2017-09-27T17:52:56Z

test/common/router/config_impl_test.cc

+  }
+  EXPECT_NE(hash_ip, hash_h);
+  EXPECT_NE(hash_ip, hash_both);
+  EXPECT_NE(hash_h, hash_both);


What about validating that if you hash twice the same input, you get a deterministic result, i.e. eq?

htuch · 2017-09-27T17:54:32Z

source/common/router/config_impl.cc

+  }
+
+private:
+  Http::LowerCaseString header_name_;


htuch · 2017-09-27T17:54:56Z

source/common/router/config_impl.cc

+      break;
+    default:
+      throw EnvoyException(
+          fmt::format("Unsupported hash policy {}", hash_policy.policy_specifier_case()));


Can you add a test for this?

mattklein123

Generally looks good apart from @htuch comments and my small one.

mattklein123 · 2017-09-27T18:27:52Z

source/common/router/config_impl.cc

+  for (const HashMethodPtr& hash_impl : hash_impls_) {
+    Optional<uint64_t> new_hash = hash_impl->evaluate(downstream_addr, headers);
+    if (new_hash.valid()) {
+      hash.value(hash.valid() ? (((hash.value() << 1) | (hash.value() >> 63)) ^ new_hash.value())


can you add a comment on how this bit shifting was chosen?

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 · 2017-09-27T19:48:34Z

Sorry please merge master again. Still trying to sort out CI issues.

…iple-hashes Signed-off-by: Alex Konradi <akonradi@google.com>

Fixes envoyproxy#1422 Signed-off-by: Alex Konradi <akonradi@google.com>

Description: Adding these has had no measurable benefit, and they're likely detrimental to timely passive healthchecking and interactions tuned for specific TCP keepalive settings. Risk Level: Low Testing: Local & CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This commit cleans up Makefile by removing unnecessary variables etc. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Support multiple hash policies for HTTP routing

a4b4dee

Fixes envoyproxy#1422 Signed-off-by: Alex Konradi <akonradi@google.com>

htuch reviewed Sep 27, 2017

View reviewed changes

akonradi added 5 commits September 27, 2017 12:07

Remove unnecessary empty (cons|des)tructors

dd6f6d2

Signed-off-by: Alex Konradi <akonradi@google.com>

Change HashPolicyImpl::HashImpl -> HashMethod

985b26a

Signed-off-by: Alex Konradi <akonradi@google.com>

Throw exception for unsupported hash policies

d966c46

Signed-off-by: Alex Konradi <akonradi@google.com>

Rotate old hash before XOR with new

2226b42

This prevents the degenerate case that htuch mentioned, where a duplicated hash policy would cancel out the first one. Signed-off-by: Alex Konradi <akonradi@google.com>

Fix formatting

2b92ce7

Signed-off-by: Alex Konradi <akonradi@google.com>

htuch reviewed Sep 27, 2017

View reviewed changes

mattklein123 reviewed Sep 27, 2017

View reviewed changes

akonradi added 3 commits September 27, 2017 14:32

Mark more constant things const

1e4a936

Signed-off-by: Alex Konradi <akonradi@google.com>

Add more hash policy tests

c54f0e6

Signed-off-by: Alex Konradi <akonradi@google.com>

Explain choice of hash rotation

c89e940

Signed-off-by: Alex Konradi <akonradi@google.com>

htuch approved these changes Sep 27, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into http-routing-mult…

fe0c525

…iple-hashes Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 approved these changes Sep 27, 2017

View reviewed changes

mattklein123 merged commit 8880857 into envoyproxy:master Sep 27, 2017

costinm pushed a commit to costinm/envoy that referenced this pull request Oct 2, 2017

Support multiple hash policies for HTTP routing (envoyproxy#1756)

21611cc

Fixes envoyproxy#1422 Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi mentioned this pull request Oct 16, 2017

Update RDS hash policy docs envoyproxy/data-plane-api#194

Merged

akonradi deleted the http-routing-multiple-hashes branch October 16, 2017 19:51

mathetake added a commit that referenced this pull request Mar 3, 2026

chore: clean up Makefile (#1756)

6561db0

**Description** This commit cleans up Makefile by removing unnecessary variables etc. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Conversation

akonradi commented Sep 27, 2017

Uh oh!

htuch 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!

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!

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants