Implement cookie hashing for v2 API #1766

akonradi · 2017-09-28T14:27:39Z

Add a new Http::CookieHashMethod class that hashes on a cookie in the headers sent by the downstream host. The behavior if a cookie is not found is configurable: if the configured TTL is nonzero, a new cookie is randomly generated and returned to the downstream host as an additional "Set-Cookie" header; otherwise, no hash is returned.

Fixes #1295

Support using a cookie as an input source for HTTP ketama hashing. Not yet implemented for the v2 API: setting a cookie if TTL is nonzero. Signed-off-by: Alex Konradi <akonradi@google.com>

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

This will be used to implement the second half of cookie-based HTTP ketama routing. If the route is appropriately configured, clients without the routing cookie set should receive a "Set-Cookie" header in the response that sets the appropriate cookie. Signed-off-by: Alex Konradi <akonradi@google.com>

If the configured TTL for the cookie hash policy is nonzero, then add a Set-Cookie header to the stream and use the randomly selected value for routing. Fixes envoyproxy#1295 Signed-off-by: Alex Konradi <akonradi@google.com>

htuch · 2017-09-28T14:35:03Z

@alyssawilk can you take this review?

dnoe

Looks good, a few comments on hasSetCookie(). Can we avoid repeated iteration over all the headers?

dnoe · 2017-09-28T14:45:13Z

source/common/http/utility.cc

@@ -121,6 +121,41 @@ std::string Utility::parseCookieValue(const HeaderMap& headers, const std::strin
  return state.ret_;
 }

+bool Utility::hasSetCookie(const HeaderMap& headers, const std::string& key) {


This seems like it shouldn't require iteration. Is there an issue with using HeaderMap::get() ?

OK, I think I see the issue which is that get() only returns the first occurrence of that header, and that's not sufficient. But maybe we can avoid calling hasSetCookie() multiple times and having the scan all the headers each time.

Yeah, get() is insufficient. I'll make it do something a little smarter.

dnoe · 2017-09-28T14:53:05Z

source/common/router/config_impl.cc

+
+private:
+  const std::string key_;
+  const long ttl_;


Can you use a comment (or change variable name) to make it clear that the units of ttl_ are seconds?

drive by: I would just use std::chrono::seconds

dnoe · 2017-09-28T14:53:59Z

source/common/router/config_impl.cc

  Optional<uint64_t> hash;
  for (const HashMethodPtr& hash_impl : hash_impls_) {
-    const Optional<uint64_t> new_hash = hash_impl->evaluate(downstream_addr, headers);
+    Optional<uint64_t> new_hash = hash_impl->evaluate(downstream_addr, headers, add_cookie);


It's not clear to me why this can't be const anymore.

dnoe · 2017-09-28T14:55:31Z

source/common/router/router.cc

+    const int max_age = std::get<2>(cookie);
+
+    // Don't overwrite a cookie if it's already set by the upstream host
+    if (Http::Utility::hasSetCookie(*headers, key)) {


The iteration in hasSetCookie() seems especially unpleasant given the call here: We call hasSetCookie() multiple times if there are multiple downstream_set_cookies_, and each time it iterates over the entire header map.

I think the best that can be done right now is to add a perf TODO here. This can be dealt with by potentially working on the TODO in header_map_impl which would allow set-cookie to become an O(1) header but still support append/multiples. I have thoughts on how to do this but probabably out of scope for this PR.

dnoe · 2017-09-28T15:08:00Z

source/common/router/router.h

@@ -264,6 +274,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
  UpstreamRequestPtr upstream_request_;
  Http::HeaderMap* downstream_headers_{};
  Http::HeaderMap* downstream_trailers_{};
+  std::list<std::tuple<std::string, std::string, int>> downstream_set_cookies_;


I think it might be a good idea to create an explicit type instead of using tuple here.
Why std::list instead of std::vector?

This preserves the const-ness of Filter::hashKey(). While addDownstreamSetCookie does modify internal state, the changes are only visible to callers of onUpstreamHeaders, which is not const. Signed-off-by: Alex Konradi <akonradi@google.com>

alyssawilk

Great start!
Glad you started off getting multiple cookie headers - I probably would have missed that possibility in review :-P

alyssawilk · 2017-09-28T15:14:53Z

source/common/router/router.cc

+    const int max_age = std::get<2>(cookie);
+
+    // Don't overwrite a cookie if it's already set by the upstream host
+    if (Http::Utility::hasSetCookie(*headers, key)) {


Do we think there's a need for this yet? It seems unlikely that upstream can set a cookie which will hash back to the "correct" server. I suppose we could check to make sure we're replacing - I'm not sure what browsers do if they get two set-cookies with the same key in the same response...

Fair. I guess the only thing this actually does is allow the upstream host to throw the downstream client to another random server since it can't really even pick which one. Chrome picks the second value if the same cookie is set twice, and I'd guess the last one generally.

alyssawilk · 2017-09-28T15:25:01Z

source/common/http/utility.cc

+          State* state = static_cast<State*>(context);
+          // If the key matches, parse the value from the rest of the cookie string.
+          if (k == state->key_) {
+            state->ret_ = true;


we can early return true at this point correct?

No, the iterate() function doesn't return early, though that'd make sense for both of the cookie-related functions.

Feel free to make iterate callback return a bool for early exit. Or just put in a TODO

@akonradi Can you add a TODO for this? I think it'd be a useful feature for the iterate to allow early-exit.

alyssawilk · 2017-09-28T15:29:03Z

source/common/http/utility.cc

+  state.key_ = key;
+  state.ret_ = false;
+
+  headers.iterate(


This is basically the same as parseCookieValue but skipping the string copy, right? I wonder if it's worth having a comon helper with a boolean to copy the string or not. Optional - if you prefer it this way feel free to leave it this way

The Cookie header has multiple cookie values inline whereas a Set-Cookie header has only a single cookie value. There's an extra inner loop in parseCookieValue that hasSetCookie doesn't need.

alyssawilk · 2017-09-28T15:31:11Z

source/common/router/config_impl.cc

+    } else if (!value.empty()) {
+      hash.value(HashUtil::xxHash64(value));
+    }
+    return hash;


So if there's no existing cookie and the TTL is 0, shouldn't we still hash, and just not set the cookie?

Not here, because there's nothing to hash on. The Router::Filter does that already when none of the policies return a hash.

alyssawilk · 2017-09-28T15:35:42Z

include/envoy/router/router.h

@@ -216,14 +216,23 @@ class HashPolicy {
  virtual ~HashPolicy() {}

  /**
+   * A callback used for requesting that a random cookie be set with the given


I don't think we really want a random cookie.

Imagine we're doing this because we want a client to consistently land on the same server. now imagine the client is using H2, and sends 3 requests "simultaneously" on one connection. Each will have no set cookie, so each will get a random hash, land on a random backend, and get a random cookie set. Whichever response lands last will "win". We probably want to hash on something stable for the connection to ensure we get consistent routing before the cookie is received.

Also an integration test for this case would be nice, making sure the cookie is consistent (don't care what it is) between requests. You should be able to crib heavily from (or simply extend config for) existing http2 tests which have multiple simultaneous requests and responses.

Should this comment ("random") be updated?

ggreenway · 2017-09-28T19:50:49Z

I think I may be way way late to this party, but I wanted to briefly discuss an alternate design with some different tradeoffs:

What if instead of hashing on a cookie, we make envoy generate a cookie with the chosen upstream instance encoded in it.

Advantages:

Requests will always get routed to the same server unless that specific server goes down (whereas with this approach there's 1/N error from the consistent hash).
The initial load balancing pick can use a different algorithm than random (such as least-conns).

Disadvantages:

Lose any ability to hash on a cookie that isn't set by envoy (probably)
Need to add a completely new LB type

Sorry if this has been discussed previously.

akonradi · 2017-09-28T20:51:03Z

@ggreenway The issue there is that the load will become unbalanced as new upstream hosts are added/discovered, since only downstream clients that have cookies assigned after those hosts join will have any chance of being assigned to them. The other clients will continue to be routed to their previous hosts.

alyssawilk · 2017-09-28T20:54:08Z

That could be managed by defaulting to picking least loaded in the case there's no cookie, but I think that'd be well outside the scope of this PR :-)

…

On Thu, Sep 28, 2017 at 4:51 PM, akonradi ***@***.***> wrote: @ggreenway <https://github.com/ggreenway> The issue there is that the load will become unbalanced as new upstream hosts are added/discovered, since only downstream clients that have cookies assigned after those hosts join will have any chance of being assigned to them. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1766 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvR3grxVVU5dLwp19IrNPtlz3vk5Rks5snAbAgaJpZM4PnWva> .

ggreenway · 2017-09-28T21:02:04Z

@akonradi Agreed. I think it depends on the upstream app which is better (more stickiness vs more even distribution). And it depends on how long the clients stick around (if they make a handful of requests in under a minute, the load will stay pretty evenly balanced; if they make requests over hours/days with the same cookie, it will get more unbalanced).

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

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

rshriram · 2017-10-02T20:32:20Z

@ggreenway can you clarify on what you mean by "What if instead of hashing on a cookie, we make envoy generate a cookie with the chosen upstream instance encoded in it." ?
When a request exits [client]envoy, it is heading to a particular upstream instance (unless its going to some load balancer).

ggreenway · 2017-10-02T20:41:55Z

@rshriram I mean that, instead of generating a cookie with a random value that then gets fed into a consistent-hash load-balancer algorithm, we generate a cookie that contains "ip:port" (probably encrypted or hashed or similar to not leak internals). When the client makes the next request and sends us this cookie, we check if the specified ip:port is still in the upstream pool. If it is, we use it. If it is not, we use a fallback load balancing mechanism (round-robin, random, least-conns, etc) and send back a new cookie with the new ip:port.

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

…p-routing-cookie-hash Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2017-10-03T21:05:54Z

The tests fail because one of the added integration tests won't work until #1798 is resolved (and merged into this branch).

htuch · 2017-10-09T16:44:16Z

Now that #1812 has merged, could you merge and rerun tests? Thanks.

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

akonradi · 2017-10-09T19:16:54Z

Passes all tests after merging head (which includes #1812)

htuch · 2017-10-09T19:21:00Z

@alyssawilk can you do the final approval/merge as appropriate?

alyssawilk · 2017-10-10T13:08:04Z

include/envoy/router/router.h

@@ -216,14 +216,23 @@ class HashPolicy {
  virtual ~HashPolicy() {}

  /**
+   * A callback used for requesting that a random cookie be set with the given


Should this comment ("random") be updated?

alyssawilk · 2017-10-10T13:19:12Z

source/common/router/router.h

+   * to be sent to the downstream host.
+   * @return std::string the value of the new cookie
+   *
+   * marked const so it can be called from hashKey(), and because all mutated


I'm not seeing much use of mutable in the envoy code base, and generally for things like stats.
I'm having trouble sorting this out without patching and compiling but would it be possible to instead pass in a list downstream_set_cookies in as an argument rather than accessing it as a member variable, so the member doesn't have to be mutable?

After talking in-person and thinking about this some more, I realized that there's no way to avoid having mutable somewhere. The call graph that hits addDownstreamSetCookie() always goes through Filter::hashKey(), which is itself const. Passing a mutable list of cookies in would also work but hashKey() would then be the mutating function masquerading as const.

Maybe we should change hashKey() to be non-const? While conceptually it starts out as a simple computational hash, if we're going to start modifying the actual request, let's just be upfront about it I reckon.

alyssawilk · 2017-10-10T13:23:39Z

source/common/router/router.h

+    // The cookie value should be the same per connection so that if multiple
+    // streams race on the same path, they all receive the same cookie.
+    std::string value;
+    const Network::Connection* conn = callbacks_->connection();


downstreamConnection()?

alyssawilk · 2017-10-10T13:30:34Z

test/common/router/config_impl_test.cc

+
+  EXPECT_FALSE(config.usesRuntime());
+
+  {


Not in the style guide (so optional) but Harvey and I encourage test comments so folks less familiar with the code can easily see what's going on. Like
// With no cookie set no hash will be generated
// With non-configured cookies set, the hash still will not be generated
// Finally with the configured "hash" cookie set, a hash will be generated.

alyssawilk · 2017-10-10T13:35:18Z

test/common/router/config_impl_test.cc

+      hash_2 =
+          route->routeEntry()->hashPolicy()->generateHash("", headers, add_cookie_nop_).value();
+    }
+    EXPECT_NE(hash_1, hash_2);


I think this could be a separate test since it's going from "is a hash generated or not" to "are generated hashes the same"

If you stick cm_ and runtime_ as member variables of the test fixture and have a utility function which creates and returns cookie policy, you'll be able to minimize code duplication

alyssawilk · 2017-10-10T13:52:23Z

source/common/router/router.h

+   */
+  std::string addDownstreamSetCookie(const std::string& key, std::chrono::seconds max_age) const {
+    // The cookie value should be the same per connection so that if multiple
+    // streams race on the same path, they all receive the same cookie.


As discussed, in person I'm not quite sure what the right thing to do here is on path diversity vs hashing consistency.

If most Envoy servers were set up with HTTP browsers hitting a single Envoy it'd be nice to hash on IP rather than Address to have consistent hashing for the 8 HTTP connections. Unfortunately that greatly reduces hash diversity so if you instead have a lot of connections from one source to one dest they'd end up all pointed at the same place.

I'd be inclined to err on the side of not overloading backends, but maybe worth a comment that for the HTTP + multiple connections case there will still be a cookie race.

Are we going to want to go to a harder form of sticky further down the track? If so, then some of this will be moot.

I think this is part of the convo above, which I think partly happened IRL, but, I'm a little confused as to why the local address is part of the hash. Also related to stickiness per @htuch. Can you maybe add some more detailed comments here on the rationale behind how this is being set / the overall algorithm being used?

alyssawilk · 2017-10-10T13:56:43Z

test/integration/http2_integration_test.cc

+        served_by.insert(
+            response.headers().get(Http::LowerCaseString("x-served-by"))->value().c_str());
+      });
+  EXPECT_EQ(served_by.size(), num_upstreams_);


I'm a bit twitchy about this as it's technically non-deterministic and eventually you'll get an unlucky hash. Think it's worth making num_upstreams_ non-const and bringing it down to 2 for this test or unlikely enough as-is?

alyssawilk · 2017-10-10T13:57:31Z

test/integration/http2_integration_test.cc

+
+void Http2RingHashIntegrationTest::createUpstreams() {
+  for (int i = 0; i < num_upstreams_; i++) {
+    fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));


I think this would be a generally useful extension to the base integration test but definitely outside the scope of this PR. Consider it as a follow up though :-)

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

htuch

Looks great, almost ready to ship, some final round of comments.

htuch · 2017-10-10T23:30:53Z

include/envoy/router/router.h

@@ -216,14 +216,23 @@ class HashPolicy {
  virtual ~HashPolicy() {}

  /**
+   * A callback used for requesting that a cookie be set with the given lifetime.


What does this return? What is key? etc.

htuch · 2017-10-10T23:38:04Z

source/common/http/utility.cc

@@ -121,6 +121,47 @@ std::string Utility::parseCookieValue(const HeaderMap& headers, const std::strin
  return state.ret_;
 }

+std::string Utility::makeSetCookieValue(const std::string& key, const std::string& value,
+                                        const std::chrono::seconds max_age) {
+  return fmt::format("{}=\"{}\"; Max-Age={}", key, value, max_age.count());


Is there any situation in which we might want to ensure that value is correctly escaped? Probably not today, but seems like something dangerous if value could come from the user.

Agreed; I've added a comment to the function declaration comment.

htuch · 2017-10-10T23:41:03Z

source/common/http/utility.cc

+          }
+          std::string k = value.substr(0, equals_index);
+          State* state = static_cast<State*>(context);
+          // If the key matches, parse the value from the rest of the cookie string.


Nit: here we're not doing any further parsing after key match.

htuch · 2017-10-10T23:41:15Z

source/common/http/utility.cc

+        // Find the set-cookie headers in the request
+        if (header.key() == Http::Headers::get().SetCookie.get().c_str()) {
+          const std::string value{header.value().c_str()};
+          size_t equals_index = value.find('=');


Nit: const size_t.

htuch · 2017-10-10T23:41:40Z

source/common/http/utility.cc

+            // The cookie is malformed if it does not have an `=`.
+            return;
+          }
+          std::string k = value.substr(0, equals_index);


Nit: const.

htuch · 2017-10-10T23:46:39Z

source/common/router/config_impl.cc

@@ -109,6 +134,10 @@ HashPolicyImpl::HashPolicyImpl(
    case envoy::api::v2::RouteAction::HashPolicy::kHeader:
      hash_impls_.emplace_back(new HeaderHashMethod(hash_policy.header().header_name()));
      break;
+    case envoy::api::v2::RouteAction::HashPolicy::kCookie:
+      hash_impls_.emplace_back(
+          new CookieHashMethod(hash_policy.cookie().name(), hash_policy.cookie().ttl().seconds()));


It would probably be more correct to convert from Duration to milliseconds and then round or something, but it's probably fine, I don't think anyone is going to care about fractional seconds here.

htuch · 2017-10-10T23:48:23Z

source/common/router/router.h

@@ -138,6 +145,33 @@ class Filter : Logger::Loggable<Logger::Id::router>,
    return callbacks_->connection();
  }

+  /**
+   * Set a pseudo-random cookie with the name @param key and TTL @param max_age


htuch · 2017-10-10T23:51:51Z

source/common/router/router.h

+   * to be sent to the downstream host.
+   * @return std::string the value of the new cookie
+   *
+   * marked const so it can be called from hashKey(), and because all mutated


Maybe we should change hashKey() to be non-const? While conceptually it starts out as a simple computational hash, if we're going to start modifying the actual request, let's just be upfront about it I reckon.

htuch · 2017-10-11T00:00:02Z

source/common/router/router.h

+   */
+  std::string addDownstreamSetCookie(const std::string& key, std::chrono::seconds max_age) const {
+    // The cookie value should be the same per connection so that if multiple
+    // streams race on the same path, they all receive the same cookie.


Are we going to want to go to a harder form of sticky further down the track? If so, then some of this will be moot.

htuch · 2017-10-11T00:01:00Z

source/common/router/router.h

+    if (conn) {
+      value = conn->remoteAddress().asString() + conn->localAddress().asString();
+    } else {
+      // TODO(akonradi) talk to mattklein123 and figure out when this can happen


Vague recollection that this could only happen if we had this hash policy on a connection we initiated from Envoy via AsyncClient, which we never would have? So maybe it should also be an ASSERT?

mattklein123 · 2017-10-11T00:01:51Z

source/common/router/router.h

+  // list of cookies to add to upstream headers
+  // 'mutable' is safe because this is only read in onUpstreamHeaders(), which
+  // is not const
+  mutable std::vector<std::string> downstream_set_cookies_;


Do you know if the default allocator/constructor for vector pre-allocates space? It would be unfortunate to allocate anything in the common case. Wondering if this should be a pointer to a vector and you can allocate it when needed?

The clang and GCC standard library vectors don't pre-allocate.

mattklein123 · 2017-10-11T00:31:57Z

Another random comment: This is a feature this is going to need full end user docs when we figure out the v2 doc situation. @htuch thoughts on just starting a v2_docs.md file on a temporary basis where we at least list things that need full docs? I don't want to lose things that need docs that are not getting implemented in v1.

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

Emphasize that this is a method that can modify state. Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 · 2017-10-11T18:24:29Z

source/common/router/router.h

-      ENVOY_LOG(warn, "Downstream connection was null while trying to make a routing cookie");
-    }
+    // need to check for null conn if this is ever used by Http::AsyncClient in the fiture
+    ASSERT(conn);


nit: You can remove this ASSERT() as it will crash in an obvious way on the next line.

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

mattklein123

LGTM pending more comments/docs about the algorithm and some resolution on where to stash temp v2 docs. Unless @htuch has a better idea I would say let's create a v2_temp_docs.md file and stick stuff in there until it can be put into sphinx. This can be a holding ground for doc notes for v2 features that do not exist in v1.

htuch

LGTM modulo nits. I think the cookie affinity should be documented separately to this PR, in https://github.com/envoyproxy/data-plane-api/blob/master/api/rds.proto#L222. This is not as feature rich as MD for writing docs, but it's where we will be drawing from when putting together the new docs.

htuch · 2017-10-12T14:56:27Z

source/common/router/router.h

@@ -130,7 +130,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,
  void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override;

  // Upstream::LoadBalancerContext
-  Optional<uint64_t> hashKey() const override {
+  Optional<uint64_t> hashKey() override {


Can you ASSERT at the top of here that the downstream cookies are empty? That would help catch multiple accidental calls.

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

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

htuch · 2017-10-16T02:28:10Z

I think this is ready to ship if we can get a merge. Would be nice to see a PR at https://github.com/envoyproxy/data-plane-api/ for the docs as mentioned above as well.

…ie-hash Manual resolution of merge conflicts Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2017-10-16T19:06:30Z

Docs pull request: envoyproxy/data-plane-api#194

htuch · 2017-10-16T19:46:32Z

I think the Mac failure is unrelated and that the RDS docs are a good place to continue that conversation, so let's merge.

* Update api sha (envoyproxy#1753) * Release-0.8: Update envoy sha to bb6762a (envoyproxy#1759) * Update envoy sha to bb6762a * update envoy sha to 12c470e * fix authn/integration tasn issue * Update_Dependencies (envoyproxy#1766) * Build addition artifacts with debug symbols (envoyproxy#1767) * Update_Dependencies (envoyproxy#1768)

…roxy#1780) * Update api sha (envoyproxy#1753) * Release-0.8: Update envoy sha to bb6762a (envoyproxy#1759) * Update envoy sha to bb6762a * update envoy sha to 12c470e * fix authn/integration tasn issue * Update_Dependencies (envoyproxy#1766) * Build addition artifacts with debug symbols (envoyproxy#1767) * Update_Dependencies (envoyproxy#1768) * Update api version to b549a3f770c833bad8f4f3871768c43960ab7309 (envoyproxy#1769) * Update istio.deps * Update repositories.bzl * Update istio.deps * Update istio.deps * Update Envoy to c2baf34. (envoyproxy#1773) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Update api sha to 8d67e57e3612dae1a3423795bce93a372cfe4fa4 (envoyproxy#1775) * revert api sha change

Description: Similar to #1764, this will help prevent port reuse from concurrently running tests. Risk Level: Low Testing: Local & CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

akonradi added 4 commits September 28, 2017 10:19

Hash on cookie for HTTP routing

5af4a18

Support using a cookie as an input source for HTTP ketama hashing. Not yet implemented for the v2 API: setting a cookie if TTL is nonzero. Signed-off-by: Alex Konradi <akonradi@google.com>

Add utility function to scan Set-Cookie headers

55af17c

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

Add missing routing cookie to return stream

b94bc32

If the configured TTL for the cookie hash policy is nonzero, then add a Set-Cookie header to the stream and use the randomly selected value for routing. Fixes envoyproxy#1295 Signed-off-by: Alex Konradi <akonradi@google.com>

htuch requested a review from alyssawilk September 28, 2017 14:38

dnoe reviewed Sep 28, 2017

View reviewed changes

alyssawilk reviewed Sep 28, 2017

View reviewed changes

akonradi added 5 commits September 29, 2017 10:03

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

b428be5

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

Use std::chrono for cookie hash TTL

82011b5

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

Add back accidentally removed const

b2d9bc2

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

Make cookie value more deterministic

3545e33

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

Assume upstream hosts don't set routing cookies

1e56684

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

akonradi added 3 commits October 3, 2017 15:32

Add integration test for ring cookie hashing

7d0fbad

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

Add integration test for cookie hash routing

7af1a14

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

Merge branch 'master' of https://github.com/envoyproxy/envoy into htt…

94e0a3b

…p-routing-cookie-hash Signed-off-by: Alex Konradi <akonradi@google.com>

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

cd9e9a0

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

alyssawilk reviewed Oct 10, 2017

View reviewed changes

Use correct connection for cookie generation

e13477a

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

htuch reviewed Oct 11, 2017

View reviewed changes

mattklein123 reviewed Oct 11, 2017

View reviewed changes

akonradi added 4 commits October 11, 2017 10:25

Clean up comments

04f4ce2

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

Replace null check with assert

9dd0ee9

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

Make LoadBalancerContext::hashKey() non-const

9385295

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

Rename hashKey() to computeHashKey()

1713502

Emphasize that this is a method that can modify state. Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 reviewed Oct 11, 2017

View reviewed changes

Remove unnecessary ASSERT

6b28dc7

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

mattklein123 reviewed Oct 11, 2017

View reviewed changes

rshriram mentioned this pull request Oct 12, 2017

Mixer-driven routing (was: Support for session affinity based routing) istio/old_pilot_repo#769

Closed

htuch reviewed Oct 12, 2017

View reviewed changes

akonradi added 2 commits October 13, 2017 09:45

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

4136480

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

Update hasSetCookie to return HeaderMap::Iterate

d1743a7

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

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

0917b85

…ie-hash Manual resolution of merge conflicts 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

htuch approved these changes Oct 16, 2017

View reviewed changes

htuch merged commit f3c9499 into envoyproxy:master Oct 16, 2017

akonradi deleted the http-routing-cookie-hash branch October 16, 2017 19:47

kyessenov mentioned this pull request Nov 3, 2017

Mixer-driven routing (was: Support for session affinity based routing) istio/istio#1433

Closed

Implement cookie hashing for v2 API #1766

Implement cookie hashing for v2 API #1766

Conversation

akonradi commented Sep 28, 2017 • edited

htuch commented Sep 28, 2017

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Sep 28, 2017

akonradi commented Sep 28, 2017 • edited

alyssawilk commented Sep 28, 2017 via email

ggreenway commented Sep 28, 2017

rshriram commented Oct 2, 2017

ggreenway commented Oct 2, 2017

akonradi commented Oct 3, 2017

htuch commented Oct 9, 2017

akonradi commented Oct 9, 2017 • edited

htuch commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akonradi Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Oct 11, 2017

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Oct 16, 2017

akonradi commented Oct 16, 2017

akonradi commented Sep 28, 2017 •

edited

akonradi commented Sep 28, 2017 •

edited

akonradi commented Oct 9, 2017 •

edited

akonradi Oct 10, 2017 •

edited

mattklein123 Oct 11, 2017 •

edited