Implement downstream IP hashing for HTTP ketama routing by akonradi · Pull Request #1727 · envoyproxy/envoy

akonradi · 2017-09-22T21:04:35Z

The v2 API supports using the downstream (source) IP address to route HTTP requests to upstream hosts. This PR implements that functionality, as specified in #1296.

The downstream address information is necessary for source-IP-based hashing. Signed-off-by: Alex Konradi <akonradi@google.com>

Add support for hashing the downstream IP address for use in HTTP ketama routing. This complements the existing scheme of using one of the HTTP headers as a hash value. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2017-09-22T21:09:31Z

This code implements the desired functionality, and includes unit tests. An integration test might also be a good idea but I don't know how to implement it without depending on the choice of hashing scheme. For instance, it would be easy to check that requests from a downstream client C get routed to the same upstream host X, but if another upstream host Y joins, depending on where it ends up being placed in the hash ring, it might start receiving requests from C. Thoughts?

mattklein123 · 2017-09-23T03:36:56Z

@akonradi I think unit test only is fine for this change. (Haven't reviewed yet).

htuch

Looks good. I agree that we don't need an integration test for this (and adding one will be difficult, since we don't have a naive way of having multiple source addresses in IPv6-only setups).

htuch · 2017-09-24T16:26:10Z

include/envoy/router/router.h

+   *         exist.
   */
-  virtual Optional<uint64_t> generateHash(const Http::HeaderMap& headers) const PURE;
+  virtual Optional<uint64_t> generateHash(const Network::Address::Instance* downstream_address,


Can you add Doxygen comments for @param?

htuch · 2017-09-24T16:28:55Z

source/common/router/config_impl.cc

+    if (header) {
+      hash.value(HashUtil::xxHash64(header->value().c_str()));
+    }
+  } else if (hash_ip_ && downstream_addr) {


What about the idea of these being cumulative? I.e. I can make the hash a function of both header and downstream IP rather than either/or? I think this was the idea of making the policy a repeated.

Yeah this is the one part of this change that I find a little strange.

Cumulative also makes sense to me.

It seems odd to trigger header matching based on the empty string. When we translate the repeated is there a way to translate in such a way that we don't need to check on empty string and perhaps just translate to generic interface with an evaluate() function like we do in a few other places? (Not sure of details here but just a thought).

That's #1422, though it wouldn't be too bad to implement as part of this PR.

htuch · 2017-09-24T16:34:19Z

source/common/router/router.h

      auto hash_policy = route_entry_->hashPolicy();
      if (hash_policy) {
-        return hash_policy->generateHash(*downstream_headers_);
+        const Network::Connection* connection = this->downstreamConnection();


In what situations do we now have a downstreamConnection()? Should this a be an ASSERT and/or reference instead?

IIRC AsyncClient does not have a downstream connection. This actually confused me also and I had to think about it so I think worth a comment.

also nit: in general we don't use this-> in this type of context.

htuch · 2017-09-24T16:36:03Z

test/common/router/config_impl_test.cc

+TEST(RouterMatcherTest, HashPolicyIp) {
+  NiceMock<Runtime::MockLoader> runtime;
+  NiceMock<Upstream::MockClusterManager> cm;
+  auto routeConfig = baseHashPolicyRouteConfig();


I'd probably use a test fixture here.

also nit: route_config

htuch · 2017-09-24T16:38:28Z

test/common/router/config_impl_test.cc

+  }
+  {
+    Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
+    Network::Address::Ipv4Instance addr("1.2.3.4");


What about a test to show that different IPs hash to different addresses?

Bugfix: the previous version of this code used the address of the directly connected downstream host. This is not the same as the address of the client making the request unless the client is directly connected to the Envoy instance doing the routing. To fix this, use the downstream address of the routed stream instead. Signed-off-by: Alex Konradi <akonradi@google.com>

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

htuch

LGTM, just some one more thing..

htuch · 2017-09-25T21:43:19Z

include/envoy/router/router.h

-   *         A hash value might not be returned if for example the specified HTTP header does not
-   *         exist. In the future we might add additional support for hashing on origin address,
-   *         etc.
+   * @param const downstream_address the address of the connected client host


@param should have the name of the parameter, not the type.

htuch · 2017-09-25T21:45:12Z

test/common/router/config_impl_test.cc

+  {
+    Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
+    const auto hash_policy = config.route(headers, 0)->routeEntry()->hashPolicy();
+    uint64_t hash_1 = hash_policy->generateHash("1.2.3.4", headers).value();


Nit: const

htuch · 2017-09-25T21:46:48Z

source/common/router/config_impl.cc

 }

-Optional<uint64_t> HashPolicyImpl::generateHash(const Http::HeaderMap& headers) const {
+Optional<uint64_t> HashPolicyImpl::generateHash(const std::string& downstream_addr,


According to @mattklein123:

IIRC AsyncClient does not have a downstream connection. This actually confused me also and I had to think about it so I think worth a comment.

Should we at least add a check that a downstream IP based route config is not used alongside this?

I don't understand your question, but a comment was added to the docs for generateHash that addresses @mattklein123's comment. downstream_addr is an empty string for requests originated by AsyncClient.

OK, as long as it's empty it's fine.

OK cool. I wasn't sure. If it's empty string that's fine.

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

htuch · 2017-09-26T15:07:48Z

include/envoy/router/router.h

-   * @param const headers the HTTP headers for the stream
-   * @return Optional<uint64_t> an optional hash value to route on. A hash value might not be
-   * returned if for example the specified HTTP header does not exist.
+   * @return Optional<uint64_t> an optional hash value to route on.


Nit: reorder @return after @param.

htuch · 2017-09-26T15:08:05Z

include/envoy/router/router.h

-   * returned if for example the specified HTTP header does not exist.
+   * @return Optional<uint64_t> an optional hash value to route on.
+   * @param downstream_address contains the address of the connected client host, or an
+   * empty string if the request is initiated from within this host, and


Nit: no need for , and between @param

mattklein123 · 2017-09-26T15:15:05Z

source/common/router/config_impl.cc

-  const Http::HeaderEntry* header = headers.get(header_name_);
-  if (header) {
-    hash.value(HashUtil::xxHash64(header->value().c_str()));
+  if (!header_name_.get().empty()) {


What about the comment thread here re: pivoting on empty string and hashing on multiple things? Did we decide to defer that to a follow up?

Yup, that'll be my next PR.

OK sounds good.

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

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

akonradi · 2017-09-26T16:04:07Z

The coverage test failed because the JVM bugged out, lovely.

mattklein123 · 2017-09-26T18:27:59Z

@akonradi I kicked it for you

mattklein123

LGTM, thank you.

) The v2 API supports using the downstream (source) IP address to route HTTP requests to upstream hosts. This PR implements that functionality, as specified in envoyproxy#1296. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** The reason for the 401 response could also be token is missing in the request. Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

akonradi added 2 commits September 22, 2017 16:59

Add downstream src address argument to hash policy

a491f8a

The downstream address information is necessary for source-IP-based hashing. Signed-off-by: Alex Konradi <akonradi@google.com>

Implement source IP hashing for HTTP routing

f2a1e0d

Add support for hashing the downstream IP address for use in HTTP ketama routing. This complements the existing scheme of using one of the HTTP headers as a hash value. Signed-off-by: Alex Konradi <akonradi@google.com>

htuch reviewed Sep 24, 2017

View reviewed changes

akonradi added 4 commits September 25, 2017 10:51

Add docs for HashPolicy::generateHash arguments

01b43f8

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

Use fixture for HashPolicy test, fix formatting

b2c2180

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

Ensure not all IP addresses produce the same hash

9e61823

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

htuch reviewed Sep 25, 2017

View reviewed changes

akonradi added 3 commits September 26, 2017 10:31

Fix HashPolicy::generateHash doc

e4f8bd8

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

Make some test variables const

6f76348

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

Merge remote-tracking branch 'upstream/master'

7ea9570

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

htuch previously approved these changes Sep 26, 2017

View reviewed changes

mattklein123 reviewed Sep 26, 2017

View reviewed changes

akonradi added 2 commits September 26, 2017 11:17

Merge remote-tracking branch 'upstream/master' into stable-routing-ip…

7dbf54b

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

Fix nits in HashPolicy::generateHash docs

a0ef4e7

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

akonradi dismissed htuch’s stale review via a0ef4e7 September 26, 2017 15:21

mattklein123 approved these changes Sep 26, 2017

View reviewed changes

mattklein123 merged commit 0408b98 into envoyproxy:master Sep 27, 2017

akonradi deleted the stable-routing-ip-hashing branch September 27, 2017 13:49

akonradi mentioned this pull request Oct 4, 2017

Add explicit source IP hash to RDS. #1296

Closed

jpsim pushed a commit that referenced this pull request Nov 28, 2022

cc: clean up uses of this in engine.cc (#1727)

ac71e28

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

cc: clean up uses of this in engine.cc (#1727)

8ee2676

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

chore: update www authn headers (#1727)

b465ebf

**Description** The reason for the 401 response could also be token is missing in the request. Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Conversation

akonradi commented Sep 22, 2017

Uh oh!

akonradi commented Sep 22, 2017

Uh oh!

mattklein123 commented Sep 23, 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!

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!

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!

akonradi commented Sep 26, 2017

Uh oh!

mattklein123 commented Sep 26, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants