Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion api/envoy/api/v2/route/route.proto
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,27 @@ message RouteAction {
// Connection properties hash policy.
ConnectionProperties connection_properties = 3;
}

// 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

// policy works.
//
// If true, and there is already a hash computed, ignore rest of the
// list of hash polices.
// For example, if the following hash methods are configured:
//
// ========= ========
// specifier terminal
// ========= ========
// Header A true
// Header B false
// Header C false
// ========= ========
//
// The generateHash process ends if policy "header A" generates a hash, as
// it's a terminal policy.
bool terminal = 4;
}

// Specifies a list of hash policies to use for ring hash load balancing. Each
Expand All @@ -596,7 +617,9 @@ message RouteAction {
// hash policies fail to generate a hash, no hash will be produced for
// the route. In this case, the behavior is the same as if no hash policies
// were specified (i.e. the ring hash load balancer will choose a random
// backend).
// backend). If a hash policy has the "terminal" attribute set to true, and
// there is already a hash generated, the hash is returned immediately,
// ignoring the rest of the hash policy list.
repeated HashPolicy hash_policy = 15;

// Indicates that a HTTP/1.1 client connection to this particular route is allowed to
Expand Down
40 changes: 29 additions & 11 deletions source/common/router/config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,20 @@ ShadowPolicyImpl::ShadowPolicyImpl(const envoy::api::v2::route::RouteAction& con
runtime_key_ = config.request_mirror_policy().runtime_key();
}

class HeaderHashMethod : public HashPolicyImpl::HashMethod {
class HashMethodImplBase : public HashPolicyImpl::HashMethod {
public:
HeaderHashMethod(const std::string& header_name) : header_name_(header_name) {}
HashMethodImplBase(bool terminal) : terminal_(terminal) {}

bool terminal() const override { return terminal_; }

private:
const bool terminal_;
};

class HeaderHashMethod : public HashMethodImplBase {
public:
HeaderHashMethod(const std::string& header_name, bool terminal)
: HashMethodImplBase(terminal), header_name_(header_name) {}

absl::optional<uint64_t> evaluate(const Network::Address::Instance*,
const Http::HeaderMap& headers,
Expand All @@ -96,18 +107,17 @@ class HeaderHashMethod : public HashPolicyImpl::HashMethod {
const Http::LowerCaseString header_name_;
};

class CookieHashMethod : public HashPolicyImpl::HashMethod {
class CookieHashMethod : public HashMethodImplBase {
public:
CookieHashMethod(const std::string& key, const std::string& path,
const absl::optional<std::chrono::seconds>& ttl)
: key_(key), path_(path), ttl_(ttl) {}
const absl::optional<std::chrono::seconds>& ttl, bool terminal)
: HashMethodImplBase(terminal), key_(key), path_(path), ttl_(ttl) {}

absl::optional<uint64_t> evaluate(const Network::Address::Instance*,
const Http::HeaderMap& headers,
const HashPolicy::AddCookieCallback add_cookie) const override {
absl::optional<uint64_t> hash;
std::string value = Http::Utility::parseCookieValue(headers, key_);

if (value.empty() && ttl_.has_value()) {
value = add_cookie(key_, path_, ttl_.value());
hash = HashUtil::xxHash64(value);
Expand All @@ -124,8 +134,10 @@ class CookieHashMethod : public HashPolicyImpl::HashMethod {
const absl::optional<std::chrono::seconds> ttl_;
};

class IpHashMethod : public HashPolicyImpl::HashMethod {
class IpHashMethod : public HashMethodImplBase {
public:
IpHashMethod(bool terminal) : HashMethodImplBase(terminal) {}

absl::optional<uint64_t> evaluate(const Network::Address::Instance* downstream_addr,
const Http::HeaderMap&,
const HashPolicy::AddCookieCallback) const override {
Expand Down Expand Up @@ -153,20 +165,21 @@ HashPolicyImpl::HashPolicyImpl(
for (auto& hash_policy : hash_policies) {
switch (hash_policy.policy_specifier_case()) {
case envoy::api::v2::route::RouteAction::HashPolicy::kHeader:
hash_impls_.emplace_back(new HeaderHashMethod(hash_policy.header().header_name()));
hash_impls_.emplace_back(
new HeaderHashMethod(hash_policy.header().header_name(), hash_policy.terminal()));
break;
case envoy::api::v2::route::RouteAction::HashPolicy::kCookie: {
absl::optional<std::chrono::seconds> ttl;
if (hash_policy.cookie().has_ttl()) {
ttl = std::chrono::seconds(hash_policy.cookie().ttl().seconds());
}
hash_impls_.emplace_back(
new CookieHashMethod(hash_policy.cookie().name(), hash_policy.cookie().path(), ttl));
hash_impls_.emplace_back(new CookieHashMethod(
hash_policy.cookie().name(), hash_policy.cookie().path(), ttl, hash_policy.terminal()));
break;
}
case envoy::api::v2::route::RouteAction::HashPolicy::kConnectionProperties:
if (hash_policy.connection_properties().source_ip()) {
hash_impls_.emplace_back(new IpHashMethod());
hash_impls_.emplace_back(new IpHashMethod(hash_policy.terminal()));
}
break;
default:
Expand All @@ -190,6 +203,11 @@ HashPolicyImpl::generateHash(const Network::Address::Instance* downstream_addr,
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 a hash has been generated, ignore
// the rest of the hash policies.
if (hash_impl->terminal() && hash) {
break;
}
}
return hash;
}
Expand Down
3 changes: 3 additions & 0 deletions source/common/router/config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ class HashPolicyImpl : public HashPolicy {
virtual absl::optional<uint64_t> evaluate(const Network::Address::Instance* downstream_addr,
const Http::HeaderMap& headers,
const AddCookieCallback add_cookie) const PURE;

// If the method is a terminal method, ignore rest of the hash policy chain.
virtual bool terminal() const PURE;
};

typedef std::unique_ptr<HashMethod> HashMethodPtr;
Expand Down
59 changes: 59 additions & 0 deletions test/common/router/config_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1699,6 +1699,65 @@ TEST_F(RouterMatcherHashPolicyTest, HashMultiple) {
EXPECT_NE(hash_h, hash_both);
}

TEST_F(RouterMatcherHashPolicyTest, HashTerminal) {
// Hash policy list: cookie, header [terminal=true], user_ip.
auto route = route_config_.mutable_virtual_hosts(0)->mutable_routes(0)->mutable_route();
route->add_hash_policy()->mutable_cookie()->set_name("cookie_hash");
auto* header_hash = route->add_hash_policy();
header_hash->mutable_header()->set_header_name("foo_header");
header_hash->set_terminal(true);
route->add_hash_policy()->mutable_connection_properties()->set_source_ip(true);
Network::Address::Ipv4Instance address1("4.3.2.1");
Network::Address::Ipv4Instance address2("1.2.3.4");

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, and done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
headers.addCopy("Cookie", "cookie_hash=foo;");
headers.addCopy("foo_header", "bar");
Router::RouteConstSharedPtr route = config().route(headers, 0);
hash_1 = route->routeEntry()
->hashPolicy()
->generateHash(&address1, headers, add_cookie_nop_)
.value();
}
{
Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
headers.addCopy("Cookie", "cookie_hash=foo;");
headers.addCopy("foo_header", "bar");
Router::RouteConstSharedPtr route = config().route(headers, 0);
hash_2 = route->routeEntry()
->hashPolicy()
->generateHash(&address2, headers, add_cookie_nop_)
.value();
}
EXPECT_EQ(hash_1, hash_2);

// If no hash computed after evaluating a hash policy, the rest of the policy
// list is evaluated.
{
// Input: {}, {}, address1. Hash on address1.
Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
Router::RouteConstSharedPtr route = config().route(headers, 0);
hash_1 = route->routeEntry()
->hashPolicy()
->generateHash(&address1, headers, add_cookie_nop_)
.value();
}
{
// Input: {}, {}, address2. Hash on address2.
Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
Router::RouteConstSharedPtr route = config().route(headers, 0);
hash_2 = route->routeEntry()
->hashPolicy()
->generateHash(&address2, headers, add_cookie_nop_)
.value();
}
EXPECT_NE(hash_1, hash_2);
}

TEST_F(RouterMatcherHashPolicyTest, InvalidHashPolicies) {
NiceMock<Server::Configuration::MockFactoryContext> factory_context;
{
Expand Down