Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[router] Add SRDS configUpdate impl #7451

Merged
merged 71 commits into from
Aug 22, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
6a3f13b
add scope key-builder impl and test
stevenzzzz Jun 11, 2019
cc806d7
Merge branch 'master' into add-key-builder
stevenzzzz Jun 11, 2019
bb55a44
fix typo
stevenzzzz Jun 11, 2019
8d7b4da
move impl code from header to cc file.
stevenzzzz Jun 12, 2019
8bcddce
add review fixes and unit test case for keybuidler
stevenzzzz Jun 12, 2019
b1d0d5c
clang-tidy fix
stevenzzzz Jun 12, 2019
1007ee0
use expect_debug_death for assertion test
stevenzzzz Jun 12, 2019
184ff2f
fix failed test in opt mode
stevenzzzz Jun 12, 2019
fe6b643
add assert around addFragment and test, and a few review fix
stevenzzzz Jun 13, 2019
fc98e56
Merge branch 'master' into add-key-builder
stevenzzzz Jun 19, 2019
951bd28
Merge branch 'master' into add-key-builder
stevenzzzz Jun 20, 2019
75718d9
fix based on feedbacks
stevenzzzz Jun 21, 2019
d84839d
fix nits
stevenzzzz Jun 25, 2019
27ff0a3
switch from constructing proto memeber by value(copy-elision) to more…
stevenzzzz Jun 25, 2019
36a6e72
refactor scoped config impl a little bit, andd SRDS config update impl
stevenzzzz Jul 2, 2019
46e23f2
Merge branch 'master' into scoped-config-impl-refactor
stevenzzzz Jul 2, 2019
74038f4
fix typo
stevenzzzz Jul 2, 2019
ff7ccdb
fix srds integration test and add some more test on scopekey
stevenzzzz Jul 8, 2019
06bd690
Merge branch 'CLEAN' into scoped-config-impl-refactor
stevenzzzz Jul 8, 2019
cf38720
save review fixes
stevenzzzz Jul 9, 2019
5cad3ee
add conflict detection for scope keys
stevenzzzz Jul 9, 2019
50e8fb5
Review feedbacks fix, and some more changes to the config provider fr…
stevenzzzz Jul 12, 2019
ac65ba2
Merge branch 'master' into scoped-config-impl-refactor
stevenzzzz Jul 12, 2019
cb5324f
make SRDS the SotW onConfigUpdate API a quasi-incremental one per dic…
stevenzzzz Jul 12, 2019
5c8a546
fix clang-tidy bug
stevenzzzz Jul 12, 2019
4a1fb01
revert the quasi-incremental delta onConfigUpdate change after offlin…
stevenzzzz Jul 15, 2019
fdd0a74
Merge branch 'CLEAN' into scoped-config-impl-refactor
stevenzzzz Jul 15, 2019
467a6a1
refactor config provider framework
stevenzzzz Jul 24, 2019
7dea1a0
some renames and stale comment fix
stevenzzzz Jul 24, 2019
c354a1e
merge PR #7704, add init_manager injection when creating RdsRouteConf…
stevenzzzz Jul 25, 2019
2b5f572
Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
stevenzzzz Jul 25, 2019
9dd4214
fix a bug in update-propagation: new version should overwrite old ver…
stevenzzzz Jul 29, 2019
5d2f8a2
make a ScopeRouteInfo owns a RouteConfigProvider, as per the config p…
stevenzzzz Jul 29, 2019
86c455e
fixes for review feedbacks
stevenzzzz Jul 30, 2019
60dd433
fix comment
stevenzzzz Jul 30, 2019
6c1a672
snowp comments
stevenzzzz Jul 30, 2019
c31de28
Merge branch 'master' of https://github.com/envoyproxy/envoy into CLEAN
stevenzzzz Jul 30, 2019
abfe558
Merge branch 'master' into config-provider-update
stevenzzzz Jul 30, 2019
69df754
merge main & config-provider-update into srds onConfigUpdate PR
stevenzzzz Jul 30, 2019
5f357bd
some cleanup around noop watcher
stevenzzzz Jul 30, 2019
dd3b97d
fix-format
stevenzzzz Jul 30, 2019
ee09a5b
add more comment
stevenzzzz Aug 1, 2019
46ad9af
fix typo, add back missing constructor in config_provider_test
stevenzzzz Aug 1, 2019
47a417d
Merge branch 'master' of https://github.com/envoyproxy/envoy
stevenzzzz Aug 1, 2019
9c1f616
Merge branch 'master' of https://github.com/envoyproxy/envoy
stevenzzzz Aug 1, 2019
ae2ac31
merge with upstream master
stevenzzzz Aug 1, 2019
b1acd70
expect death when a null fragment is added into scopekey
stevenzzzz Aug 1, 2019
d08bdb2
fix per andres comments
stevenzzzz Aug 2, 2019
eb65b89
fix pedant spelling check errors.
stevenzzzz Aug 2, 2019
07bdfee
fix failing add null fragment assertion test. expect_death ==> expect…
stevenzzzz Aug 2, 2019
40b214e
only death test in debug mode for add null fragment to scopeKey
stevenzzzz Aug 2, 2019
4e57308
fix clang tidy errors around router/mock.h by removing empty destruct…
stevenzzzz Aug 6, 2019
710c7e8
move empty ctors,destructors back to cc file
stevenzzzz Aug 8, 2019
53081b2
Merge branch 'master' of https://github.com/envoyproxy/envoy
stevenzzzz Aug 8, 2019
2593bfa
merge with upstream master
stevenzzzz Aug 8, 2019
2dd6714
fix a race condition, in SRDS onConfigUpdate, when calling applyConfi…
stevenzzzz Aug 12, 2019
cef747d
add more review fixes
stevenzzzz Aug 12, 2019
bb0cdb8
disable cleanup timer on SRDS subscription destruction
stevenzzzz Aug 13, 2019
fa6ecb1
update comment on initManager overriding
stevenzzzz Aug 13, 2019
efaa891
A bunch of update after several issues found:
stevenzzzz Aug 20, 2019
bf4dc23
fix-format, and fix update for htuch's comments around ScopeKey hash …
stevenzzzz Aug 20, 2019
e329f49
Merge branch 'master' of https://github.com/envoyproxy/envoy into sco…
stevenzzzz Aug 20, 2019
c4719d8
split SRDS onConfigUpdate into smaller functions and some minor comme…
stevenzzzz Aug 21, 2019
56e2876
cleanup comment and remove unused header.
stevenzzzz Aug 21, 2019
d26d9b7
fix a bug introduced in the most recent function splitting, adjust th…
stevenzzzz Aug 21, 2019
4ab7005
fixes feedbacks from Andres
stevenzzzz Aug 22, 2019
bf703fc
fix typo
stevenzzzz Aug 22, 2019
2b375d4
fix nits from htuch feedbacks
stevenzzzz Aug 22, 2019
c7a241c
change back scope_name assignment to value, as protobuf returns a ref…
stevenzzzz Aug 22, 2019
c2ff015
fix nits
stevenzzzz Aug 22, 2019
eb5af9c
merge w/ upstream master
stevenzzzz Aug 22, 2019
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
15 changes: 5 additions & 10 deletions source/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -146,22 +146,16 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "scoped_config_manager_lib",
srcs = ["scoped_config_manager.cc"],
hdrs = ["scoped_config_manager.h"],
deps = [
"@envoy_api//envoy/api/v2:srds_cc",
],
)

envoy_cc_library(
name = "scoped_config_lib",
srcs = ["scoped_config_impl.cc"],
hdrs = ["scoped_config_impl.h"],
external_deps = [
"abseil_str_format",
],
deps = [
":config_lib",
":scoped_config_manager_lib",
"//include/envoy/router:rds_interface",
"//include/envoy/router:scopes_interface",
"//include/envoy/thread_local:thread_local_interface",
"@envoy_api//envoy/api/v2:srds_cc",
Expand All @@ -176,6 +170,7 @@ envoy_cc_library(
deps = [
":scoped_config_lib",
"//include/envoy/config:subscription_interface",
"//include/envoy/router:route_config_provider_manager_interface",
"//include/envoy/stats:stats_interface",
"//source/common/common:assert_lib",
"//source/common/common:minimal_logger_lib",
Expand Down
34 changes: 23 additions & 11 deletions source/common/router/scoped_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,7 @@ bool ScopeKey::operator==(const ScopeKey& other) const {
// An empty key equals to nothing, "NULL" != "NULL".
return false;
}
return std::equal(fragments_.begin(), fragments_.end(), other.fragments_.begin(),
other.fragments_.end(),
[](const std::unique_ptr<ScopeKeyFragmentBase>& left,
const std::unique_ptr<ScopeKeyFragmentBase>& right) -> bool {
// Both should be non-NULL now.
return *left == *right;
});
return this->hash() == other.hash();
}

HeaderValueExtractorImpl::HeaderValueExtractorImpl(
Expand Down Expand Up @@ -104,13 +98,31 @@ ScopeKeyBuilderImpl::computeScopeKey(const Http::HeaderMap& headers) const {
return std::make_unique<ScopeKey>(std::move(key));
}

void ThreadLocalScopedConfigImpl::addOrUpdateRoutingScope(const ScopedRouteInfoConstSharedPtr&) {}
void ThreadLocalScopedConfigImpl::addOrUpdateRoutingScope(
const ScopedRouteInfoConstSharedPtr& scoped_route_info) {
scoped_route_info_by_name_.try_emplace(scoped_route_info->scopeName(), scoped_route_info);
scoped_route_info_by_key_.try_emplace(scoped_route_info->scopeKey().hash(), scoped_route_info);
}

void ThreadLocalScopedConfigImpl::removeRoutingScope(const std::string&) {}
void ThreadLocalScopedConfigImpl::removeRoutingScope(const std::string& scope_name) {
const auto iter = scoped_route_info_by_name_.find(scope_name);
if (iter != scoped_route_info_by_name_.end()) {
scoped_route_info_by_key_.erase(iter->second->scopeKey().hash());
stevenzzzz marked this conversation as resolved.
Show resolved Hide resolved
scoped_route_info_by_name_.erase(iter);
}
}

Router::ConfigConstSharedPtr
ThreadLocalScopedConfigImpl::getRouteConfig(const Http::HeaderMap&) const {
return std::make_shared<const NullConfigImpl>();
ThreadLocalScopedConfigImpl::getRouteConfig(const Http::HeaderMap& headers) const {
std::unique_ptr<ScopeKey> scope_key = scope_key_builder_.computeScopeKey(headers);
if (scope_key == nullptr) {
return nullptr;
}
auto iter = scoped_route_info_by_key_.find(scope_key->hash());
if (iter != scoped_route_info_by_key_.end()) {
return iter->second->routeConfig();
}
return nullptr;
}

} // namespace Router
Expand Down
76 changes: 62 additions & 14 deletions source/common/router/scoped_config_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@

#include "envoy/api/v2/srds.pb.h"
#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h"
#include "envoy/router/rds.h"
#include "envoy/router/router.h"
#include "envoy/router/scopes.h"
#include "envoy/thread_local/thread_local.h"

#include "common/common/hash.h"
#include "common/protobuf/utility.h"
#include "common/router/config_impl.h"
#include "common/router/scoped_config_manager.h"

#include "absl/numeric/int128.h"
#include "absl/strings/str_format.h"

namespace Envoy {
namespace Router {
Expand All @@ -26,15 +30,14 @@ class ScopeKeyFragmentBase {

bool operator==(const ScopeKeyFragmentBase& other) const {
if (typeid(*this) == typeid(other)) {
return equals(other);
return hash() == other.hash();
}
return false;
}
virtual ~ScopeKeyFragmentBase() = default;

private:
// Returns true if the two fragments equal else false.
virtual bool equals(const ScopeKeyFragmentBase&) const PURE;
// Hash of the fragment.
virtual uint64_t hash() const PURE;
};

/**
Expand All @@ -52,14 +55,22 @@ class ScopeKey {
// Caller should guarantee the fragment is not nullptr.
void addFragment(std::unique_ptr<ScopeKeyFragmentBase>&& fragment) {
ASSERT(fragment != nullptr, "null fragment not allowed in ScopeKey.");
updateHash(*fragment);
fragments_.emplace_back(std::move(fragment));
}

uint64_t hash() const { return hash_; }
bool operator!=(const ScopeKey& other) const;

bool operator==(const ScopeKey& other) const;

private:
// Update the key's hash with the new fragment hash.
void updateHash(const ScopeKeyFragmentBase& fragment) {
absl::uint128 buffer = absl::MakeUint128(hash_, fragment.hash());
hash_ = HashUtil::xxHash64(absl::string_view(reinterpret_cast<char*>(&buffer), 16));
Copy link
Member

Choose a reason for hiding this comment

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

One observation that I think we need to start thinking more about independent of this is that we should move away from using non-cryptographic hashes for comparing configuration. I think that parts of the control plane should be viewed as untrusted, e.g. in a managed control plane, maybe route names are under the control of an adversary. In this scenario, it's plausible that hash collisions are not hard to create and we need to have something with strong crypto properties (at the expense of performance).
CC @mattklein123 @yanavlasov

htuch marked this conversation as resolved.
Show resolved Hide resolved
}

uint64_t hash_{0};
std::vector<std::unique_ptr<ScopeKeyFragmentBase>> fragments_;
};

Expand All @@ -68,11 +79,9 @@ class StringKeyFragment : public ScopeKeyFragmentBase {
public:
explicit StringKeyFragment(absl::string_view value) : value_(value) {}

private:
bool equals(const ScopeKeyFragmentBase& other) const override {
return value_ == static_cast<const StringKeyFragment&>(other).value_;
}
uint64_t hash() const override { return HashUtil::xxHash64(value_); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest memoizing the computed hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

it's the ScopeKey's hash() that matters, that value will be compared on a per-requests basis. This value once computed, will be used once to update the scopeKey's hash.


private:
const std::string value_;
};

Expand Down Expand Up @@ -132,9 +141,45 @@ class ScopeKeyBuilderImpl : public ScopeKeyBuilderBase {
std::vector<std::unique_ptr<FragmentBuilderBase>> fragment_builders_;
};

// ScopedRouteConfiguration and corresponding RouteConfigProvider.
class ScopedRouteInfo {
public:
ScopedRouteInfo(envoy::api::v2::ScopedRouteConfiguration&& config_proto,
Copy link
Contributor

Choose a reason for hiding this comment

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

const envoy::api::v2::ScopedRouteConfiguration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, the constructor is expecting a rvalue to be moved into the proto member (per htuch@ pointing it out Envoy prefers more visible && than the copy-elision envoy::api::v2::ScopedRouteConfiguration parameter).

std::shared_ptr<RouteConfigProvider>&& route_provider)
: config_proto_(std::move(config_proto)), route_provider_(std::move(route_provider)) {
ASSERT(route_provider_ != nullptr, "ScopedRouteInfo expects a valid RouteConfigProvider.");
ASSERT(!route_provider_->configInfo().has_value() ||
route_provider_->config()->name() == config_proto_.route_configuration_name(),
absl::StrFormat(
stevenzzzz marked this conversation as resolved.
Show resolved Hide resolved
"RouteConfigProvider's name '%s' doesn't match route_configuration_name '%s'.",
route_provider_->config()->name(), config_proto_.route_configuration_name()));
// TODO(stevenzzzz): Maybe worth a KeyBuilder abstraction when there are more than one type of
// Fragment.
for (const auto& fragment : config_proto_.key().fragments()) {
switch (fragment.type_case()) {
case envoy::api::v2::ScopedRouteConfiguration::Key::Fragment::kStringKey:
scope_key_.addFragment(std::make_unique<StringKeyFragment>(fragment.string_key()));
break;
default:
NOT_REACHED_GCOVR_EXCL_LINE;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Move to implementation file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Router::ConfigConstSharedPtr routeConfig() const { return route_provider_->config(); }
const ScopeKey& scopeKey() const { return scope_key_; }
const envoy::api::v2::ScopedRouteConfiguration& configProto() const { return config_proto_; }
const std::string& scopeName() const { return config_proto_.name(); }

private:
const envoy::api::v2::ScopedRouteConfiguration config_proto_;
ScopeKey scope_key_;
std::shared_ptr<RouteConfigProvider> route_provider_;
};
using ScopedRouteInfoConstSharedPtr = std::shared_ptr<const ScopedRouteInfo>;
// Ordered map for consistent config dumping.
using ScopedRouteMap = std::map<std::string, ScopedRouteInfoConstSharedPtr>;

/**
* TODO(AndresGuedez): implement scoped routing logic.
*
* Each Envoy worker is assigned an instance of this type. When config updates are received,
* addOrUpdateRoutingScope() and removeRoutingScope() are called to update the set of scoped routes.
*
Expand All @@ -153,8 +198,11 @@ class ThreadLocalScopedConfigImpl : public ScopedConfig, public ThreadLocal::Thr
Router::ConfigConstSharedPtr getRouteConfig(const Http::HeaderMap& headers) const override;

private:
const envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes::ScopeKeyBuilder
scope_key_builder_;
ScopeKeyBuilderImpl scope_key_builder_;
// From scope name to cached ScopedRouteInfo.
absl::flat_hash_map<std::string, const ScopedRouteInfoConstSharedPtr> scoped_route_info_by_name_;
// Hash by ScopeKey hash to lookup in constant time.
absl::flat_hash_map<uint64_t, const ScopedRouteInfoConstSharedPtr> scoped_route_info_by_key_;
};

/**
Expand Down
22 changes: 0 additions & 22 deletions source/common/router/scoped_config_manager.cc

This file was deleted.

49 changes: 0 additions & 49 deletions source/common/router/scoped_config_manager.h

This file was deleted.

Loading