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

[hcm] Add scoped RDS routing into HCM #7762

Merged
merged 107 commits into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
107 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
dad2d59
snowp comment, merge with upstream master
stevenzzzz Jul 30, 2019
6b6a0ad
Merge branch 'master' into config-provider-update
stevenzzzz Jul 30, 2019
964de34
add scoped RDS routing
stevenzzzz Jul 30, 2019
5f357bd
some cleanup around noop watcher
stevenzzzz Jul 30, 2019
dd3b97d
fix-format
stevenzzzz Jul 30, 2019
e796b09
fix pedantic spelling check
stevenzzzz Jul 30, 2019
6699279
save
stevenzzzz Jul 30, 2019
75c8f49
fix tidy format errors
stevenzzzz Jul 31, 2019
fb28731
fix tidy format error
stevenzzzz Jul 31, 2019
1bd43be
fixes per htuch review feedbacks
stevenzzzz Jul 31, 2019
e43d8eb
Add more test cases and improve error message when srds scope not found
stevenzzzz Jul 31, 2019
f074e9d
improve the comment per htuch's suggestion.
stevenzzzz Jul 31, 2019
7cbb6ae
fix comment nit
stevenzzzz Jul 31, 2019
fb7867b
move updateConfigCb and add a description for it.
stevenzzzz Jul 31, 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
e328d46
Merge branch 'master' into config-provider-update
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
dcea85c
merge with master and PR #7451
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
bbae81c
merge w/ upstream master
stevenzzzz Aug 20, 2019
d847a24
merge srds config update pr 7451
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
d3a4dde
merge w/ srds config-update pr 7451
stevenzzzz Aug 21, 2019
d26d9b7
fix a bug introduced in the most recent function splitting, adjust th…
stevenzzzz Aug 21, 2019
9f37e93
save
stevenzzzz Aug 21, 2019
14dd4c7
Merge branch 'scoped-config-impl-refactor' of github.com:stevenzzzz/e…
stevenzzzz Aug 21, 2019
8addd25
merge w/ upstream master and fix for feedbacks from AndresGuedez
stevenzzzz Aug 23, 2019
ef07569
merge w/ upstream master and fix feedbacks from AndresGuedez
stevenzzzz Aug 23, 2019
facf46d
Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
stevenzzzz Aug 23, 2019
aa4949a
revert some change around scoped_rds_test introduced by git sync
stevenzzzz Aug 23, 2019
3f2de6d
add delta API support into scoped_rds_integration_test
stevenzzzz Aug 26, 2019
d20590c
feedback fixes for htuch@
stevenzzzz Aug 27, 2019
c971cf3
allow scope repick when refreshCachedRoute, add a unit test for the c…
stevenzzzz Aug 27, 2019
bbb7918
fix typo
stevenzzzz Aug 27, 2019
121a4a7
feedbacks Andres doc and test
stevenzzzz Aug 28, 2019
ae88649
fix doc reference and typo
stevenzzzz Aug 28, 2019
2386e93
fix multiple-person todo assignments
stevenzzzz Aug 28, 2019
6f67b78
fix the doc reference problem
stevenzzzz Aug 28, 2019
3a39c68
Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
stevenzzzz Aug 28, 2019
0c3b2ed
try again on the protobuf ref in docs
stevenzzzz Aug 29, 2019
fde988b
turn the snapped_scoped_routes_config_ null check into an assertion a…
stevenzzzz Aug 29, 2019
e111c0c
if only I know how to do local spellcheck
stevenzzzz Aug 29, 2019
ea0ec01
fix the broken unit test
stevenzzzz Aug 29, 2019
97c1496
if only I know how to gen doc locally
stevenzzzz Aug 29, 2019
3af2d3b
fix nits
stevenzzzz Aug 30, 2019
430c450
add version log
stevenzzzz Aug 30, 2019
4a0ef9a
reorder release note entries
stevenzzzz Aug 30, 2019
e2dbe75
Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
stevenzzzz Aug 30, 2019
ab0eabb
reorder some more release entries
stevenzzzz Aug 30, 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
55 changes: 24 additions & 31 deletions api/envoy/api/v2/srds.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,27 @@ syntax = "proto3";

package envoy.api.v2;

option java_outer_classname = "SrdsProto";
option java_package = "io.envoyproxy.envoy.api.v2";
option java_multiple_files = true;
option java_generic_services = true;

import "envoy/api/v2/discovery.proto";

import "gogoproto/gogo.proto";
import "google/api/annotations.proto";

import "validate/validate.proto";
import "gogoproto/gogo.proto";

option java_outer_classname = "SrdsProto";
option java_package = "io.envoyproxy.envoy.api.v2";
option java_multiple_files = true;
option java_generic_services = true;
option (gogoproto.equal_all) = true;

// [#protodoc-title: HTTP scoped routing configuration]
// * Routing :ref:`architecture overview <arch_overview_http_routing>`
//
// .. attention::
//
// The Scoped RDS API is not yet fully implemented and *should not* be enabled in
// :ref:`envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager`.
//
// TODO(AndresGuedez): Update :ref:`arch_overview_http_routing` with scoped routing overview and
// configuration details.

// The Scoped Routes Discovery Service (SRDS) API distributes
// :ref:`ScopedRouteConfiguration<envoy_api_msg.ScopedRouteConfiguration>` resources. Each
// ScopedRouteConfiguration resource represents a "routing scope" containing a mapping that allows
// the HTTP connection manager to dynamically assign a routing table (specified via
// a :ref:`RouteConfiguration<envoy_api_msg_RouteConfiguration>` message) to each HTTP request.
// :ref:`ScopedRouteConfiguration<envoy_api_msg.ScopedRouteConfiguration>`
// resources. Each ScopedRouteConfiguration resource represents a "routing
// scope" containing a mapping that allows the HTTP connection manager to
// dynamically assign a routing table (specified via a
// :ref:`RouteConfiguration<envoy_api_msg_RouteConfiguration>` message) to each
// HTTP request.
// [#proto-status: experimental]
service ScopedRoutesDiscoveryService {
rpc StreamScopedRoutes(stream DiscoveryRequest) returns (stream DiscoveryResponse) {
Expand All @@ -52,9 +43,9 @@ service ScopedRoutesDiscoveryService {
// :ref:`Key<envoy_api_msg_ScopedRouteConfiguration.Key>` to a
// :ref:`envoy_api_msg_RouteConfiguration` (identified by its resource name).
//
// The HTTP connection manager builds up a table consisting of these Key to RouteConfiguration
// mappings, and looks up the RouteConfiguration to use per request according to the algorithm
// specified in the
// The HTTP connection manager builds up a table consisting of these Key to
// RouteConfiguration mappings, and looks up the RouteConfiguration to use per
// request according to the algorithm specified in the
// :ref:`scope_key_builder<envoy_api_field_config.filter.network.http_connection_manager.v2.ScopedRoutes.scope_key_builder>`
// assigned to the HttpConnectionManager.
//
Expand Down Expand Up @@ -104,8 +95,8 @@ service ScopedRoutesDiscoveryService {
// Host: foo.com
// X-Route-Selector: vip=172.10.10.20
//
// would result in the routing table defined by the `route-config1` RouteConfiguration being
// assigned to the HTTP request/stream.
// would result in the routing table defined by the `route-config1`
// RouteConfiguration being assigned to the HTTP request/stream.
//
// [#comment:next free field: 4]
// [#proto-status: experimental]
Expand All @@ -115,8 +106,9 @@ message ScopedRouteConfiguration {

// Specifies a key which is matched against the output of the
// :ref:`scope_key_builder<envoy_api_field_config.filter.network.http_connection_manager.v2.ScopedRoutes.scope_key_builder>`
// specified in the HttpConnectionManager. The matching is done per HTTP request and is dependent
// on the order of the fragments contained in the Key.
// specified in the HttpConnectionManager. The matching is done per HTTP
// request and is dependent on the order of the fragments contained in the
// Key.
message Key {
message Fragment {
oneof type {
Expand All @@ -127,14 +119,15 @@ message ScopedRouteConfiguration {
}
}

// The ordered set of fragments to match against. The order must match the fragments in the
// corresponding
// The ordered set of fragments to match against. The order must match the
// fragments in the corresponding
// :ref:`scope_key_builder<envoy_api_field_config.filter.network.http_connection_manager.v2.ScopedRoutes.scope_key_builder>`.
repeated Fragment fragments = 1 [(validate.rules).repeated .min_items = 1];
}

// The resource name to use for a :ref:`envoy_api_msg_DiscoveryRequest` to an RDS server to
// fetch the :ref:`envoy_api_msg_RouteConfiguration` associated with this scope.
// The resource name to use for a :ref:`envoy_api_msg_DiscoveryRequest` to an
// RDS server to fetch the :ref:`envoy_api_msg_RouteConfiguration` associated
// with this scope.
string route_configuration_name = 2 [(validate.rules).string.min_bytes = 1];

// The key to match against.
Expand Down
29 changes: 29 additions & 0 deletions docs/root/intro/arch_overview/http/http_routing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,35 @@ 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.

.. _arch_overview_http_routing_route_scope:

Route Scope
--------------

Scoped routing enables Envoy to put constraints on search space of domains and route rules.
A :ref:`Route Scope<envoy_api_msg_ScopedRouteConfiguration>` associates a key with a :ref:`route table <arch_overview_http_routing_route_table>`.
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.
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
fragments:
- header_value_extractor:
name: Addr
element_separator: ;
element:
key: x-foo-key
separator: =

.. _arch_overview_http_routing_route_table:

Route table
-----------

Expand Down
5 changes: 3 additions & 2 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,17 @@ Version history
<deprecated>` for more information.
* rbac: added conditions to the policy, see :ref:`condition <envoy_api_field_config.rbac.v2.Policy.condition>`.
* router: added :ref:`rq_retry_skipped_request_not_complete <config_http_filters_router_stats>` counter stat to router stats.
* router: :ref:`Scoped routing <arch_overview_http_routing_route_scope>` is supported.
* router check tool: add coverage reporting & enforcement.
* router check tool: add comprehensive coverage reporting.
* tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP.
* router check tool: add deprecated field check.
* tls: added verification of IP address SAN fields in certificates against configured SANs in the
* tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP.
certificate validation context.
* tracing: added tags for gRPC response status and meesage.
* upstream: added :ref:`an option <envoy_api_field_Cluster.CommonLbConfig.close_connections_on_host_set_change>` that allows draining HTTP, TCP connection pools on cluster membership change.
* upstream: added network filter chains to upstream connections, see :ref:`filters<envoy_api_field_Cluster.filters>`.
* upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1.
* upstream: added :ref:`an option <envoy_api_field_Cluster.CommonLbConfig.close_connections_on_host_set_change>` that allows draining HTTP, TCP connection pools on cluster membership change.
* zookeeper: parse responses and emit latency stats.

1.11.1 (August 13, 2019)
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/stream_info/stream_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ struct ResponseCodeDetailValues {
// The request was rejected because it attempted an unsupported upgrade.
const std::string UpgradeFailed = "upgrade_failed";

// The request was rejected by the HCM because there was no route configuration found.
const std::string RouteConfigurationNotFound = "route_configuration_not_found";
// The request was rejected by the router filter because there was no route found.
const std::string RouteNotFound = "route_not_found";
// A direct response was generated by the router filter.
Expand Down
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ envoy_cc_library(
"//include/envoy/router:rds_interface",
"//include/envoy/router:scopes_interface",
"//include/envoy/runtime:runtime_interface",
"//include/envoy/server:admin_interface",
"//include/envoy/server:overload_manager_interface",
"//include/envoy/ssl:connection_interface",
"//include/envoy/stats:stats_interface",
Expand Down
57 changes: 55 additions & 2 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we rely on the refreshCAchedRoute() at the end of this method to perform the snap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

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.

return;
}
}

if (Http::Headers::get().MethodValues.Head ==
request_headers_->Method()->value().getStringView()) {
is_head_request_ = true;
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
The Server::Admin impl is just a special HCM impl which doesn't need any routing.

// 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);
Expand Down
7 changes: 6 additions & 1 deletion source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,11 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

void traceRequest();

// Updates the snapped_route_config_ if scope found, or ends the stream by
// sending local reply.
// Returns true if scoped route config snapped, false otherwise.
bool snapScopedRouteConfig();

void refreshCachedRoute();

// Pass on watermark callbacks to watermark subscribers. This boils down to passing watermark
Expand Down Expand Up @@ -585,7 +590,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

ConnectionManagerImpl& connection_manager_;
Router::ConfigConstSharedPtr snapped_route_config_;
Router::ScopedConfigConstSharedPtr snapped_scoped_route_config_;
Router::ScopedConfigConstSharedPtr snapped_scoped_routes_config_;
Tracing::SpanPtr active_span_;
const uint64_t stream_id_;
StreamEncoder* response_encoder_{};
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/scoped_rds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
*to_remove_repeated.Add() = scoped_route.first;
}
onConfigUpdate(to_add_repeated, to_remove_repeated, version_info);
} // namespace Router
}

ScopedRdsConfigProvider::ScopedRdsConfigProvider(
ScopedRdsConfigSubscriptionSharedPtr&& subscription)
Expand Down
26 changes: 24 additions & 2 deletions test/common/grpc/grpc_client_integration.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ class GrpcClientIntegrationParamTest
: public BaseGrpcClientIntegrationParamTest,
public testing::TestWithParam<std::tuple<Network::Address::IpVersion, ClientType>> {
public:
~GrpcClientIntegrationParamTest() override = default;
static std::string protocolTestParamsToString(
const ::testing::TestParamInfo<std::tuple<Network::Address::IpVersion, ClientType>>& p) {
return fmt::format("{}_{}",
Expand All @@ -54,10 +53,26 @@ class GrpcClientIntegrationParamTest
ClientType clientType() const override { return std::get<1>(GetParam()); }
};

class DeltaSotwGrpcClientIntegrationParamTest
: public BaseGrpcClientIntegrationParamTest,
public testing::TestWithParam<std::tuple<Network::Address::IpVersion, ClientType, bool>> {
public:
static std::string protocolTestParamsToString(
const ::testing::TestParamInfo<std::tuple<Network::Address::IpVersion, ClientType, bool>>&
p) {
return fmt::format("{}_{}",
std::get<0>(p.param) == Network::Address::IpVersion::v4 ? "IPv4" : "IPv6",
std::get<1>(p.param) == ClientType::GoogleGrpc ? "GoogleGrpc" : "EnvoyGrpc",
std::get<2>(p.param) ? "Delta" : "StateOfTheWorld");
}
Network::Address::IpVersion ipVersion() const override { return std::get<0>(GetParam()); }
ClientType clientType() const override { return std::get<1>(GetParam()); }
bool isDelta() { return std::get<2>(GetParam()); }
};

class DeltaSotwIntegrationParamTest
: public testing::TestWithParam<std::tuple<Network::Address::IpVersion, SotwOrDelta>> {
public:
~DeltaSotwIntegrationParamTest() override = default;
static std::string protocolTestParamsToString(
const ::testing::TestParamInfo<std::tuple<Network::Address::IpVersion, SotwOrDelta>>& p) {
return fmt::format("{}_{}_{}",
Expand All @@ -84,10 +99,17 @@ class DeltaSotwIntegrationParamTest
#define GRPC_CLIENT_INTEGRATION_PARAMS \
testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \
testing::Values(Grpc::ClientType::EnvoyGrpc, Grpc::ClientType::GoogleGrpc))
#define DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS \
testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \
testing::Values(Grpc::ClientType::EnvoyGrpc, Grpc::ClientType::GoogleGrpc), \
testing::Bool())
#else
#define GRPC_CLIENT_INTEGRATION_PARAMS \
testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \
testing::Values(Grpc::ClientType::EnvoyGrpc))
#define DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS \
testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \
testing::Values(Grpc::ClientType::EnvoyGrpc), testing::Bool())
#endif // ENVOY_GOOGLE_GRPC

#define DELTA_INTEGRATION_PARAMS \
Expand Down
Loading