-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[hcm] Add scoped RDS routing into HCM #7762
Changes from 102 commits
6a3f13b
cc806d7
bb55a44
8d7b4da
8bcddce
b1d0d5c
1007ee0
184ff2f
fe6b643
fc98e56
951bd28
75718d9
d84839d
27ff0a3
36a6e72
46e23f2
74038f4
ff7ccdb
06bd690
cf38720
5cad3ee
50e8fb5
ac65ba2
cb5324f
5c8a546
4a1fb01
fdd0a74
467a6a1
7dea1a0
c354a1e
2b5f572
9dd4214
5d2f8a2
86c455e
60dd433
6c1a672
c31de28
abfe558
69df754
dad2d59
6b6a0ad
964de34
5f357bd
dd3b97d
e796b09
6699279
75c8f49
fb28731
1bd43be
e43d8eb
f074e9d
7cbb6ae
fb7867b
ee09a5b
46ad9af
47a417d
e328d46
9c1f616
ae2ac31
b1acd70
d08bdb2
eb65b89
07bdfee
40b214e
dcea85c
4e57308
710c7e8
53081b2
2593bfa
2dd6714
cef747d
bb0cdb8
fa6ecb1
efaa891
bf4dc23
bbae81c
d847a24
e329f49
c4719d8
56e2876
d3a4dde
d26d9b7
9f37e93
14dd4c7
8addd25
ef07569
facf46d
aa4949a
3f2de6d
d20590c
c971cf3
bbb7918
121a4a7
ae88649
2386e93
6f67b78
3a39c68
0c3b2ed
fde988b
e111c0c
ea0ec01
97c1496
3af2d3b
430c450
4a0ef9a
e2dbe75
ab0eabb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,34 @@ request. The router filter supports the following features: | |
* :ref:`Hash policy <envoy_api_field_route.RouteAction.hash_policy>` based routing. | ||
* :ref:`Absolute urls <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.http_protocol_options>` are supported for non-tls forward proxies. | ||
|
||
|
||
Routing Scope | ||
-------------- | ||
|
||
Scoped routing enables Envoy to put constraints on search space of domains and route rules. | ||
A :ref:`Routeing Scope<envoy_api_msg_ScopedRouteConfiguration>` associates a key with a :ref:`route table <arch_overview_http_routing_route_table>`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: s/Routeing/Routing/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops |
||
For each request, a scope key is computed dynamically by the HTTP connection manager to pick the :ref:`route table<envoy_api_msg_RouteConfiguration>`. | ||
|
||
The Scoped RDS (SRDS) API contains a set of :ref:`Scopes <envoy_api_msg_ScopedRouteConfiguration>` resources, each defining independent routing configuration, | ||
along with a :ref:`ScopeKeyBuilder <envoy_api_msg_config.filter.network.http_connection_manager.v2.ScopedRoutes.ScopeKeyBuilder>` | ||
defining the key construction algorithm used by Envoy to look up the scope corresponding to each request. | ||
|
||
htuch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
For example, for the following scoped route configuration, Envoy will look into the "addr" header value, split the header value by ";" first, and use the first value for key 'x-foo-key' as the scope key. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiny nit: there is a weird double space here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
If the "addr" header value is "foo=1;x-foo-key=127.0.0.1;x-bar-key=1.1.1.1", then "127.0.0.1" will be computed as the scope key to look up for corresponding route configuration. | ||
|
||
.. code-block:: yaml | ||
|
||
name: 'scope_by_addr' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: no quotes needed for YAML string value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
fragments: | ||
- header_value_extractor: | ||
name: Addr | ||
element_separator: ; | ||
element: | ||
key: x-foo-key | ||
separator: = | ||
|
||
.. _arch_overview_http_routing_route_table: | ||
|
||
Route table | ||
----------- | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
#include "envoy/event/dispatcher.h" | ||
#include "envoy/network/drain_decision.h" | ||
#include "envoy/router/router.h" | ||
#include "envoy/server/admin.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drive by follow up comment: I'm not crazy about including admin here and then special casing it with dynamic casts. Is it not possible for this logical to be external to the construction of the HCM and then specified correctly either by admin or by the normal filter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we've discussed it offline, I will make a cleanup/refactor PR to move the dynamic_cast outside of conn_manager_impl. |
||
#include "envoy/ssl/connection.h" | ||
#include "envoy/stats/scope.h" | ||
#include "envoy/tracing/http_tracer.h" | ||
|
@@ -431,12 +432,27 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re | |
|
||
ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager) | ||
: connection_manager_(connection_manager), | ||
snapped_route_config_(connection_manager.config_.routeConfigProvider()->config()), | ||
stream_id_(connection_manager.random_generator_.random()), | ||
request_response_timespan_(new Stats::Timespan( | ||
connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), | ||
stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()), | ||
upstream_options_(std::make_shared<Network::Socket::Options>()) { | ||
// For Server::Admin, no routeConfigProvider or SRDS route provider is used. | ||
ASSERT(dynamic_cast<Server::Admin*>(&connection_manager_.config_) != nullptr || | ||
((connection_manager.config_.routeConfigProvider() == nullptr && | ||
connection_manager.config_.scopedRouteConfigProvider() != nullptr) || | ||
(connection_manager.config_.routeConfigProvider() != nullptr && | ||
connection_manager.config_.scopedRouteConfigProvider() == nullptr)), | ||
"Either routeConfigProvider or scopedRouteConfigProvider should be set in " | ||
"ConnectionManagerImpl."); | ||
if (connection_manager.config_.routeConfigProvider() != nullptr) { | ||
snapped_route_config_ = connection_manager.config_.routeConfigProvider()->config(); | ||
} else if (connection_manager.config_.scopedRouteConfigProvider() != nullptr) { | ||
snapped_scoped_routes_config_ = | ||
connection_manager_.config_.scopedRouteConfigProvider()->config<Router::ScopedConfig>(); | ||
ASSERT(snapped_scoped_routes_config_ != nullptr, | ||
"Scoped rds provider returns null for scoped routes config."); | ||
} | ||
ScopeTrackerScopeState scope(this, | ||
connection_manager_.read_callbacks_->connection().dispatcher()); | ||
|
||
|
@@ -613,6 +629,17 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
ScopeTrackerScopeState scope(this, | ||
connection_manager_.read_callbacks_->connection().dispatcher()); | ||
request_headers_ = std::move(headers); | ||
// For Admin thread, we don't use routeConfigProvider or SRDS route provider. | ||
if (dynamic_cast<Server::Admin*>(&connection_manager_.config_) == nullptr && | ||
stevenzzzz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
connection_manager_.config_.scopedRouteConfigProvider() != nullptr) { | ||
ASSERT(snapped_route_config_ == nullptr, | ||
"Route config already latched to the active stream when scoped RDS is enabled."); | ||
// We need to snap snapped_route_config_ here as it's used in mutateRequestHeaders later. | ||
if (!snapScopedRouteConfig()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we rely on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not: there is header cleansing logic around line 771 that depends on snapped route config, so latching the route-config here is required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, but please document this dependency in a comment here. Can you move this closer to where we use it also? I generally dislike action at a distance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
return; | ||
} | ||
} | ||
|
||
if (Http::Headers::get().MethodValues.Head == | ||
request_headers_->Method()->value().getStringView()) { | ||
is_head_request_ = true; | ||
|
@@ -1220,10 +1247,36 @@ void ConnectionManagerImpl::startDrainSequence() { | |
drain_timer_->enableTimer(config_.drainTimeout()); | ||
} | ||
|
||
bool ConnectionManagerImpl::ActiveStream::snapScopedRouteConfig() { | ||
ASSERT(request_headers_ != nullptr, | ||
"Try to snap scoped route config when there is no request headers."); | ||
|
||
snapped_route_config_ = snapped_scoped_routes_config_->getRouteConfig(*request_headers_); | ||
// NOTE: if a RDS subscription hasn't got a RouteConfiguration back, a Router::NullConfigImpl is | ||
// returned, in that case we let it pass. | ||
if (snapped_route_config_ == nullptr) { | ||
ENVOY_STREAM_LOG(trace, "can't find SRDS scope.", *this); | ||
// Stop decoding now. | ||
maybeEndDecode(true); | ||
sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Http::Code::NotFound, | ||
"route scope not found", nullptr, is_head_request_, absl::nullopt, | ||
StreamInfo::ResponseCodeDetails::get().RouteConfigurationNotFound); | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { | ||
Router::RouteConstSharedPtr route; | ||
if (request_headers_ != nullptr) { | ||
route = snapped_route_config_->route(*request_headers_, stream_id_); | ||
if (dynamic_cast<Server::Admin*>(&connection_manager_.config_) == nullptr && | ||
connection_manager_.config_.scopedRouteConfigProvider() != nullptr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it also makes sense to config guard this behavior via bootstrap config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you mean in http_connnection_manager.proto? it's a one-of there already. so it's kinda guarded there. |
||
// NOTE: re-select scope as well in case the scope key header has been changed by a filter. | ||
snapScopedRouteConfig(); | ||
} | ||
if (snapped_route_config_ != nullptr) { | ||
route = snapped_route_config_->route(*request_headers_, stream_id_); | ||
} | ||
} | ||
stream_info_.route_entry_ = route ? route->routeEntry() : nullptr; | ||
cached_route_ = std::move(route); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have an entry in the release notes (version_history.rst) as we are now adding this feature for real in this PR?