From 19c49d79efae416bb24f27532a91cf583b6cf79b Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Wed, 1 May 2019 18:22:02 -0700 Subject: [PATCH 1/9] add a route name field in route.Route list Signed-off-by: Divyani Rao --- api/envoy/api/v2/route/route.proto | 3 ++ docs/root/intro/version_history.rst | 1 + include/envoy/router/router.h | 7 ++++ include/envoy/stream_info/stream_info.h | 3 ++ source/common/http/async_client_impl.h | 3 +- source/common/router/config_impl.cc | 2 +- source/common/router/config_impl.h | 7 ++++ source/common/router/router.cc | 3 ++ source/common/stream_info/stream_info_impl.h | 5 +++ test/common/router/config_impl_test.cc | 34 +++++++++++++++++++- test/common/router/router_test.cc | 8 +++++ test/common/stream_info/test_util.h | 3 ++ test/mocks/router/mocks.cc | 1 + test/mocks/router/mocks.h | 3 ++ test/mocks/stream_info/mocks.h | 2 ++ 15 files changed, 82 insertions(+), 3 deletions(-) diff --git a/api/envoy/api/v2/route/route.proto b/api/envoy/api/v2/route/route.proto index 10ba8f6b4b7b4..80b7dced5a37f 100644 --- a/api/envoy/api/v2/route/route.proto +++ b/api/envoy/api/v2/route/route.proto @@ -161,6 +161,9 @@ message VirtualHost { // `. // [#comment:next free field: 14] message Route { + // name for the route + string name = 14; + // Route matching parameters. RouteMatch match = 1 [(validate.rules).message.required = true, (gogoproto.nullable) = false]; diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 98529d8590e27..79409fa4d6f97 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -30,6 +30,7 @@ Version history :ref:`buffer_flush_timeout ` to control how quickly the buffer is flushed if it is not full. * router: add support for configuring a :ref:`grpc timeout offset ` on incoming requests. * router: added ability to control retry back-off intervals via :ref:`retry policy `. +* router: added a route name field to each http route in route.Route list * router: per try timeouts will no longer start before the downstream request has been received in full by the router. This ensures that the per try timeout does not account for slow downstreams and that will not start before the global timeout. diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 103e345061670..50d3cc4d700d5 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -86,6 +86,11 @@ class DirectResponseEntry : public ResponseEntry { */ virtual void rewritePathHeader(Http::HeaderMap& headers, bool insert_envoy_original_path) const PURE; + + /** + * return name of the route + */ + virtual const std::string& routeName() const PURE; }; /** @@ -711,6 +716,8 @@ class RouteEntry : public ResponseEntry { * @returns the internal redirect action which should be taken on this route. */ virtual InternalRedirectAction internalRedirectAction() const PURE; + + virtual const std::string& routeName() const PURE; }; /** diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 6be5724e3ecbe..55827f04e90ee 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -202,6 +202,9 @@ class StreamInfo { */ virtual void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) PURE; + virtual void setRouteName(std::string name) PURE; + + virtual const std::string& getRouteName() const PURE; /** * @param bytes_received denotes number of bytes to add to total received bytes. */ diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 0cabd8d2b18fe..40d64f5aa3504 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -262,7 +262,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::InternalRedirectAction internalRedirectAction() const override { return Router::InternalRedirectAction::PassThrough; } - + const std::string& routeName() const override { return route_name_; } static const NullHedgePolicy hedge_policy_; static const NullRateLimitPolicy rate_limit_policy_; static const NullRetryPolicy retry_policy_; @@ -277,6 +277,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::RouteEntry::UpgradeMap upgrade_map_; const std::string& cluster_name_; absl::optional timeout_; + std::string route_name_{}; }; struct RouteImpl : public Router::Route { diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 182c6c804f557..c08cc03234026 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -365,7 +365,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, direct_response_body_(ConfigUtility::parseDirectResponseBody(route, factory_context.api())), per_filter_configs_(route.typed_per_filter_config(), route.per_filter_config(), factory_context), - time_source_(factory_context.dispatcher().timeSource()), + route_name_(route.name()), time_source_(factory_context.dispatcher().timeSource()), internal_redirect_action_(convertInternalRedirectAction(route.route())) { if (route.route().has_metadata_match()) { const auto filter_it = route.route().metadata_match().filter_metadata().find( diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 9e7266b9a9765..39b4b40871287 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -75,6 +75,10 @@ class SslRedirector : public DirectResponseEntry { void rewritePathHeader(Http::HeaderMap&, bool) const override {} Http::Code responseCode() const override { return Http::Code::MovedPermanently; } const std::string& responseBody() const override { return EMPTY_STRING; } + const std::string& routeName() const override { return route_name_; } + +private: + std::string route_name_{}; }; class SslRedirectRoute : public Route { @@ -376,6 +380,7 @@ class RouteEntryImplBase : public RouteEntry, Http::Code clusterNotFoundResponseCode() const override { return cluster_not_found_response_code_; } + const std::string& routeName() const { return route_name_; } const CorsPolicy* corsPolicy() const override { return cors_policy_.get(); } void finalizeRequestHeaders(Http::HeaderMap& headers, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path) const override; @@ -462,6 +467,7 @@ class RouteEntryImplBase : public RouteEntry, DynamicRouteEntry(const RouteEntryImplBase* parent, const std::string& name) : parent_(parent), cluster_name_(name) {} + const std::string& routeName() const override { return parent_->routeName(); } // Router::RouteEntry const std::string& clusterName() const override { return cluster_name_; } Http::Code clusterNotFoundResponseCode() const override { @@ -653,6 +659,7 @@ class RouteEntryImplBase : public RouteEntry, const absl::optional direct_response_code_; std::string direct_response_body_; PerFilterConfigs per_filter_configs_; + std::string route_name_; TimeSource& time_source_; InternalRedirectAction internal_redirect_action_; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index f8ac10974a69d..8113a5c5a4a17 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -308,11 +308,14 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e direct_response->finalizeResponseHeaders(response_headers, callbacks_->streamInfo()); }, absl::nullopt, StreamInfo::ResponseCodeDetails::get().DirectResponse); + callbacks_->streamInfo().setRouteName(direct_response->routeName()); return Http::FilterHeadersStatus::StopIteration; } // A route entry matches for the request. route_entry_ = route_->routeEntry(); + callbacks_->streamInfo().setRouteName(route_entry_->routeName()); + Upstream::ThreadLocalCluster* cluster = config_.cm_.get(route_entry_->clusterName()); if (!cluster) { config_.stats_.no_cluster_.inc(); diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index fcdb953a3cc7c..62e21f6926307 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -128,6 +128,10 @@ struct StreamInfoImpl : public StreamInfo { Upstream::HostDescriptionConstSharedPtr upstreamHost() const override { return upstream_host_; } + void setRouteName(std::string route_name) { route_name_ = route_name; } + + const std::string& getRouteName() const { return route_name_; } + void setUpstreamLocalAddress( const Network::Address::InstanceConstSharedPtr& upstream_local_address) override { upstream_local_address_ = upstream_local_address; @@ -220,6 +224,7 @@ struct StreamInfoImpl : public StreamInfo { const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; FilterStateImpl filter_state_{}; + std::string route_name_; private: uint64_t bytes_received_{}; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index cfc5b55adb9fa..1417567921729 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -2908,6 +2908,38 @@ static Http::TestHeaderMapImpl genRedirectHeaders(const std::string& host, const return headers; } +TEST_F(RouteMatcherTest, RouteName) { + std::string yaml = R"EOF( +virtual_hosts: + - name: "www2" + domains: ["www.lyft.com"] + routes: + - name: "route-test" + match: { prefix: "/"} + route: + cluster: "ufesservice" + - name: redirect + domains: [redirect.lyft.com] + routes: + - name: "route-test-2" + match: { path: /host } + redirect: { host_redirect: new.lyft.com } + )EOF"; + NiceMock factory_context; + TestConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), factory_context, false); + { + Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/", "GET"); + EXPECT_EQ("route-test", config.route(headers, 0)->routeEntry()->routeName()); + } + + { + Http::TestHeaderMapImpl headers = + genRedirectHeaders("redirect.lyft.com", "/host", false, false); + const DirectResponseEntry* redirect = config.route(headers, 0)->directResponseEntry(); + EXPECT_EQ("route-test-2", redirect->routeName()); + } +} + TEST_F(RouteMatcherTest, DirectResponse) { const auto pathname = TestEnvironment::writeStringToFileForTest("direct_response_body", "Example text 3"); @@ -5618,4 +5650,4 @@ name: foo } // namespace } // namespace Router -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 496a1e7f48a58..99e09aa19376b 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2253,7 +2253,9 @@ TEST_F(RouterTest, AltStatName) { TEST_F(RouterTest, Redirect) { MockDirectResponseEntry direct_response; + std::string route_name("route-test-name"); EXPECT_CALL(direct_response, newPath(_)).WillOnce(Return("hello")); + EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name)); EXPECT_CALL(direct_response, rewritePathHeader(_, _)); EXPECT_CALL(direct_response, responseCode()).WillOnce(Return(Http::Code::MovedPermanently)); EXPECT_CALL(direct_response, responseBody()).WillOnce(ReturnRef(EMPTY_STRING)); @@ -2270,7 +2272,9 @@ TEST_F(RouterTest, Redirect) { TEST_F(RouterTest, RedirectFound) { MockDirectResponseEntry direct_response; + std::string route_name("route-test-name"); EXPECT_CALL(direct_response, newPath(_)).WillOnce(Return("hello")); + EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name)); EXPECT_CALL(direct_response, rewritePathHeader(_, _)); EXPECT_CALL(direct_response, responseCode()).WillOnce(Return(Http::Code::Found)); EXPECT_CALL(direct_response, responseBody()).WillOnce(ReturnRef(EMPTY_STRING)); @@ -2287,6 +2291,8 @@ TEST_F(RouterTest, RedirectFound) { TEST_F(RouterTest, DirectResponse) { NiceMock direct_response; + std::string route_name("route-test-name"); + EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name)); EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(EMPTY_STRING)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); @@ -2303,6 +2309,8 @@ TEST_F(RouterTest, DirectResponse) { TEST_F(RouterTest, DirectResponseWithBody) { NiceMock direct_response; + std::string route_name("route-test-name"); + EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name)); EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); const std::string response_body("static response"); EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(response_body)); diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 011cfd7b27892..d4b0ea692d996 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -92,6 +92,8 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const Ssl::ConnectionInfo* downstreamSslConnection() const override { return downstream_connection_info_; } + void setRouteName(std::string name) override { route_name_ = name; } + const std::string& getRouteName() const override { return route_name_; } const Router::RouteEntry* routeEntry() const override { return route_entry_; } @@ -198,6 +200,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { uint64_t response_flags_{}; Upstream::HostDescriptionConstSharedPtr upstream_host_{}; bool health_check_request_{}; + std::string route_name_; Network::Address::InstanceConstSharedPtr upstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_direct_remote_address_; diff --git a/test/mocks/router/mocks.cc b/test/mocks/router/mocks.cc index 31c8037701ff5..9a2d2e2f96fda 100644 --- a/test/mocks/router/mocks.cc +++ b/test/mocks/router/mocks.cc @@ -82,6 +82,7 @@ MockRouteEntry::MockRouteEntry() { ON_CALL(*this, metadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, upgradeMap()).WillByDefault(ReturnRef(upgrade_map_)); ON_CALL(*this, hedgePolicy()).WillByDefault(ReturnRef(hedge_policy_)); + ON_CALL(*this, routeName()).WillByDefault(ReturnRef(route_name_)); } MockRouteEntry::~MockRouteEntry() {} diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index ceee9580da2d0..45979da33e5d1 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -44,6 +44,7 @@ class MockDirectResponseEntry : public DirectResponseEntry { void(Http::HeaderMap& headers, bool insert_envoy_original_path)); MOCK_CONST_METHOD0(responseCode, Http::Code()); MOCK_CONST_METHOD0(responseBody, const std::string&()); + MOCK_CONST_METHOD0(routeName, const std::string&()); }; class TestCorsPolicy : public CorsPolicy { @@ -300,8 +301,10 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(includeAttemptCount, bool()); MOCK_CONST_METHOD0(upgradeMap, const UpgradeMap&()); MOCK_CONST_METHOD0(internalRedirectAction, InternalRedirectAction()); + MOCK_CONST_METHOD0(routeName, const std::string&()); std::string cluster_name_{"fake_cluster"}; + std::string route_name_{"fake_route_name"}; std::multimap opaque_config_; TestVirtualCluster virtual_cluster_; TestRetryPolicy retry_policy_; diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 88da794daf9fd..c1800ad0c805f 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -42,6 +42,8 @@ class MockStreamInfo : public StreamInfo { MOCK_CONST_METHOD0(requestComplete, absl::optional()); MOCK_METHOD1(addBytesReceived, void(uint64_t)); MOCK_CONST_METHOD0(bytesReceived, uint64_t()); + MOCK_METHOD1(setRouteName, void(std::string route_name)); + MOCK_CONST_METHOD0(getRouteName, const std::string&()); MOCK_CONST_METHOD0(protocol, absl::optional()); MOCK_METHOD1(protocol, void(Http::Protocol protocol)); MOCK_CONST_METHOD0(responseCode, absl::optional()); From fdca10aab984903def2b542889684aa3a854dfdd Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Thu, 2 May 2019 12:24:32 -0700 Subject: [PATCH 2/9] fixing compilation errors and addressing comments Signed-off-by: Divyani Rao --- api/envoy/api/v2/route/route.proto | 4 ++-- source/common/router/config_impl.h | 2 +- source/common/stream_info/stream_info_impl.h | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/envoy/api/v2/route/route.proto b/api/envoy/api/v2/route/route.proto index 80b7dced5a37f..23dc44fc2624d 100644 --- a/api/envoy/api/v2/route/route.proto +++ b/api/envoy/api/v2/route/route.proto @@ -159,9 +159,9 @@ message VirtualHost { // // Envoy supports routing on HTTP method via :ref:`header matching // `. -// [#comment:next free field: 14] +// [#comment:next free field: 15] message Route { - // name for the route + // Name for the route. string name = 14; // Route matching parameters. diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 39b4b40871287..d2c5eba3a574a 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -380,7 +380,7 @@ class RouteEntryImplBase : public RouteEntry, Http::Code clusterNotFoundResponseCode() const override { return cluster_not_found_response_code_; } - const std::string& routeName() const { return route_name_; } + const std::string& routeName() const override { return route_name_; } const CorsPolicy* corsPolicy() const override { return cors_policy_.get(); } void finalizeRequestHeaders(Http::HeaderMap& headers, const StreamInfo::StreamInfo& stream_info, bool insert_envoy_original_path) const override; diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 62e21f6926307..d468ce5752d6c 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -128,9 +128,9 @@ struct StreamInfoImpl : public StreamInfo { Upstream::HostDescriptionConstSharedPtr upstreamHost() const override { return upstream_host_; } - void setRouteName(std::string route_name) { route_name_ = route_name; } + void setRouteName(std::string route_name) override { route_name_ = route_name; } - const std::string& getRouteName() const { return route_name_; } + const std::string& getRouteName() const override { return route_name_; } void setUpstreamLocalAddress( const Network::Address::InstanceConstSharedPtr& upstream_local_address) override { From 11c45f0f2645fcd5b7408911c86b423df91ba65f Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Fri, 3 May 2019 11:36:54 -0700 Subject: [PATCH 3/9] adding comments Signed-off-by: Divyani Rao --- include/envoy/stream_info/stream_info.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 55827f04e90ee..4bdc4722ac76b 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -202,8 +202,14 @@ class StreamInfo { */ virtual void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) PURE; + /** + * @param name denotes the name of the route. + */ virtual void setRouteName(std::string name) PURE; + /** + * @return the name of the route. + */ virtual const std::string& getRouteName() const PURE; /** * @param bytes_received denotes number of bytes to add to total received bytes. From 7bdb1278b019c7af8602ac2e4f33fae3bb58c84b Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Tue, 7 May 2019 10:58:14 -0700 Subject: [PATCH 4/9] fixing comment format Signed-off-by: Divyani Rao --- include/envoy/router/router.h | 5 ++++- include/envoy/stream_info/stream_info.h | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index 50d3cc4d700d5..ad2eeab6bff18 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -88,7 +88,7 @@ class DirectResponseEntry : public ResponseEntry { bool insert_envoy_original_path) const PURE; /** - * return name of the route + * @return std::string& the name of the route. */ virtual const std::string& routeName() const PURE; }; @@ -717,6 +717,9 @@ class RouteEntry : public ResponseEntry { */ virtual InternalRedirectAction internalRedirectAction() const PURE; + /** + * @return std::string& the name of the route. + */ virtual const std::string& routeName() const PURE; }; diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 4bdc4722ac76b..ba7c7211daf09 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -203,12 +203,12 @@ class StreamInfo { virtual void onUpstreamHostSelected(Upstream::HostDescriptionConstSharedPtr host) PURE; /** - * @param name denotes the name of the route. + * @param std::string name denotes the name of the route. */ virtual void setRouteName(std::string name) PURE; /** - * @return the name of the route. + * @return std::string& the name of the route. */ virtual const std::string& getRouteName() const PURE; /** From d97ed27ff65bcb621745aa03f4f5ce7409ce95b3 Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Mon, 13 May 2019 14:52:06 -0700 Subject: [PATCH 5/9] addressing comments & adding checks for StreamInfo Signed-off-by: Divyani Rao --- include/envoy/stream_info/stream_info.h | 2 +- source/common/http/async_client_impl.h | 2 +- source/common/router/config_impl.h | 4 ++-- source/common/stream_info/stream_info_impl.h | 2 +- test/common/router/router_test.cc | 8 ++++++++ test/common/stream_info/test_util.h | 2 +- test/mocks/stream_info/mocks.h | 2 +- 7 files changed, 15 insertions(+), 7 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index ba7c7211daf09..34f4e26c6d3a2 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -205,7 +205,7 @@ class StreamInfo { /** * @param std::string name denotes the name of the route. */ - virtual void setRouteName(std::string name) PURE; + virtual void setRouteName(absl::string_view name) PURE; /** * @return std::string& the name of the route. diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 40d64f5aa3504..4faf042ac9364 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -277,7 +277,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, Router::RouteEntry::UpgradeMap upgrade_map_; const std::string& cluster_name_; absl::optional timeout_; - std::string route_name_{}; + const std::string route_name_; }; struct RouteImpl : public Router::Route { diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index d2c5eba3a574a..9829dbcca3a57 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -78,7 +78,7 @@ class SslRedirector : public DirectResponseEntry { const std::string& routeName() const override { return route_name_; } private: - std::string route_name_{}; + const std::string route_name_; }; class SslRedirectRoute : public Route { @@ -659,7 +659,7 @@ class RouteEntryImplBase : public RouteEntry, const absl::optional direct_response_code_; std::string direct_response_body_; PerFilterConfigs per_filter_configs_; - std::string route_name_; + const std::string route_name_; TimeSource& time_source_; InternalRedirectAction internal_redirect_action_; }; diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index d468ce5752d6c..ab2e8dd473280 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -128,7 +128,7 @@ struct StreamInfoImpl : public StreamInfo { Upstream::HostDescriptionConstSharedPtr upstreamHost() const override { return upstream_host_; } - void setRouteName(std::string route_name) override { route_name_ = route_name; } + void setRouteName(absl::string_view route_name) override { route_name_ = std::string(route_name); } const std::string& getRouteName() const override { return route_name_; } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 99e09aa19376b..6909728926f5a 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2261,6 +2261,8 @@ TEST_F(RouterTest, Redirect) { EXPECT_CALL(direct_response, responseBody()).WillOnce(ReturnRef(EMPTY_STRING)); EXPECT_CALL(direct_response, finalizeResponseHeaders(_, _)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + absl::string_view route_name_view(route_name); + EXPECT_CALL(callbacks_.stream_info_, setRouteName(route_name_view)); Http::TestHeaderMapImpl response_headers{{":status", "301"}, {"location", "hello"}}; EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); @@ -2280,6 +2282,8 @@ TEST_F(RouterTest, RedirectFound) { EXPECT_CALL(direct_response, responseBody()).WillOnce(ReturnRef(EMPTY_STRING)); EXPECT_CALL(direct_response, finalizeResponseHeaders(_, _)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + absl::string_view route_name_view(route_name); + EXPECT_CALL(callbacks_.stream_info_, setRouteName(route_name_view)); Http::TestHeaderMapImpl response_headers{{":status", "302"}, {"location", "hello"}}; EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); @@ -2296,6 +2300,8 @@ TEST_F(RouterTest, DirectResponse) { EXPECT_CALL(direct_response, responseCode()).WillRepeatedly(Return(Http::Code::OK)); EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(EMPTY_STRING)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + absl::string_view route_name_view(route_name); + EXPECT_CALL(callbacks_.stream_info_, setRouteName(route_name_view)); Http::TestHeaderMapImpl response_headers{{":status", "200"}}; EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); @@ -2315,6 +2321,8 @@ TEST_F(RouterTest, DirectResponseWithBody) { const std::string response_body("static response"); EXPECT_CALL(direct_response, responseBody()).WillRepeatedly(ReturnRef(response_body)); EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + absl::string_view route_name_view(route_name); + EXPECT_CALL(callbacks_.stream_info_, setRouteName(route_name_view)); Http::TestHeaderMapImpl response_headers{ {":status", "200"}, {"content-length", "15"}, {"content-type", "text/plain"}}; diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index d4b0ea692d996..47dde1a1cb72f 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -92,7 +92,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const Ssl::ConnectionInfo* downstreamSslConnection() const override { return downstream_connection_info_; } - void setRouteName(std::string name) override { route_name_ = name; } + void setRouteName(absl::string_view route_name) override { route_name_ = std::string(route_name); } const std::string& getRouteName() const override { return route_name_; } const Router::RouteEntry* routeEntry() const override { return route_entry_; } diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index c1800ad0c805f..ca67240a131ac 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -42,7 +42,7 @@ class MockStreamInfo : public StreamInfo { MOCK_CONST_METHOD0(requestComplete, absl::optional()); MOCK_METHOD1(addBytesReceived, void(uint64_t)); MOCK_CONST_METHOD0(bytesReceived, uint64_t()); - MOCK_METHOD1(setRouteName, void(std::string route_name)); + MOCK_METHOD1(setRouteName, void(absl::string_view route_name)); MOCK_CONST_METHOD0(getRouteName, const std::string&()); MOCK_CONST_METHOD0(protocol, absl::optional()); MOCK_METHOD1(protocol, void(Http::Protocol protocol)); From 57bb8341d6e38da35b8de434f6063f0843cc04ee Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Mon, 13 May 2019 16:18:48 -0700 Subject: [PATCH 6/9] adding access log changes Signed-off-by: Divyani Rao --- .../common/access_log/access_log_formatter.cc | 5 ++++ .../common/access_log/access_log_impl_test.cc | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 452e94ae73f30..26bb58e6fd123 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -377,6 +377,11 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) { return UnspecifiedValueString; } }; + } else if (field_name == "ROUTE_NAME") { + field_extractor_ = [](const StreamInfo::StreamInfo& stream_info) { + std::string route_name = stream_info.getRouteName(); + return route_name.empty() ? UnspecifiedValueString : route_name; + }; } else if (field_name == "DOWNSTREAM_PEER_URI_SAN") { field_extractor_ = sslConnectionInfoStringExtractor([](const Ssl::ConnectionInfo& connection_info) { diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index d322532997153..48f383420890b 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -111,6 +111,30 @@ TEST_F(AccessLogImplTest, DownstreamDisconnect) { "\"10.0.0.5:1234\"\n", output_); } +TEST_F(AccessLogImplTest, RouteName) { + const std::string json = R"EOF( + { + "path": "/dev/null", + "format": "[%START_TIME%] \"%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH):256% %PROTOCOL%\" %RESPONSE_CODE% %RESPONSE_FLAGS% %ROUTE_NAME% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% \"%REQ(X-FORWARDED-FOR)%\" \"%REQ(USER-AGENT)%\" \"%REQ(X-REQUEST-ID)%\" \"%REQ(:AUTHORITY)%\"\n" + } + )EOF"; + + InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromJson(json), context_); + + EXPECT_CALL(*file_, write(_)); + stream_info_.route_name_ = "route-test-name"; + stream_info_.response_flags_ = StreamInfo::ResponseFlag::UpstreamConnectionFailure; + request_headers_.addCopy(Http::Headers::get().UserAgent, "user-agent-set"); + request_headers_.addCopy(Http::Headers::get().RequestId, "id"); + request_headers_.addCopy(Http::Headers::get().Host, "host"); + request_headers_.addCopy(Http::Headers::get().ForwardedFor, "x.x.x.x"); + + + log->log(&request_headers_, &response_headers_, &response_trailers_, stream_info_); + EXPECT_EQ("[1999-01-01T00:00:00.000Z] \"GET / HTTP/1.1\" 0 UF route-test-name 1 2 3 - \"x.x.x.x\" " + "\"user-agent-set\" \"id\" \"host\"\n", + output_); +} TEST_F(AccessLogImplTest, EnvoyUpstreamServiceTime) { const std::string json = R"EOF( From 06b503a28515960e0fcf46f4e4cd798b296649be Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Mon, 13 May 2019 16:55:29 -0700 Subject: [PATCH 7/9] format fix changes Signed-off-by: Divyani Rao --- source/common/stream_info/stream_info_impl.h | 4 +++- test/common/access_log/access_log_impl_test.cc | 8 ++++---- test/common/stream_info/test_util.h | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index ab2e8dd473280..d435a060e278a 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -128,7 +128,9 @@ struct StreamInfoImpl : public StreamInfo { Upstream::HostDescriptionConstSharedPtr upstreamHost() const override { return upstream_host_; } - void setRouteName(absl::string_view route_name) override { route_name_ = std::string(route_name); } + void setRouteName(absl::string_view route_name) override { + route_name_ = std::string(route_name); + } const std::string& getRouteName() const override { return route_name_; } diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 48f383420890b..2f4f418502a6a 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -129,11 +129,11 @@ TEST_F(AccessLogImplTest, RouteName) { request_headers_.addCopy(Http::Headers::get().Host, "host"); request_headers_.addCopy(Http::Headers::get().ForwardedFor, "x.x.x.x"); - log->log(&request_headers_, &response_headers_, &response_trailers_, stream_info_); - EXPECT_EQ("[1999-01-01T00:00:00.000Z] \"GET / HTTP/1.1\" 0 UF route-test-name 1 2 3 - \"x.x.x.x\" " - "\"user-agent-set\" \"id\" \"host\"\n", - output_); + EXPECT_EQ( + "[1999-01-01T00:00:00.000Z] \"GET / HTTP/1.1\" 0 UF route-test-name 1 2 3 - \"x.x.x.x\" " + "\"user-agent-set\" \"id\" \"host\"\n", + output_); } TEST_F(AccessLogImplTest, EnvoyUpstreamServiceTime) { diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 47dde1a1cb72f..931c22feea205 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -92,7 +92,9 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const Ssl::ConnectionInfo* downstreamSslConnection() const override { return downstream_connection_info_; } - void setRouteName(absl::string_view route_name) override { route_name_ = std::string(route_name); } + void setRouteName(absl::string_view route_name) override { + route_name_ = std::string(route_name); + } const std::string& getRouteName() const override { return route_name_; } const Router::RouteEntry* routeEntry() const override { return route_entry_; } From 1c5dae1008f64320122f94bfa9a73141b0ee6084 Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Fri, 17 May 2019 16:57:47 -0700 Subject: [PATCH 8/9] route name in gRPC access logger Signed-off-by: Divyani Rao Signed-off-by: Divyani Rao --- api/envoy/data/accesslog/v2/accesslog.proto | 3 +++ docs/root/configuration/access_log.rst | 3 +++ docs/root/intro/version_history.rst | 1 + .../access_loggers/http_grpc/grpc_access_log_impl.cc | 5 +++++ test/common/access_log/access_log_impl_test.cc | 1 + .../access_loggers/http_grpc/grpc_access_log_impl_test.cc | 3 +++ test/mocks/stream_info/mocks.cc | 4 ++++ test/mocks/stream_info/mocks.h | 1 + 8 files changed, 21 insertions(+) diff --git a/api/envoy/data/accesslog/v2/accesslog.proto b/api/envoy/data/accesslog/v2/accesslog.proto index 7f58ecacbff08..e1cf421849821 100644 --- a/api/envoy/data/accesslog/v2/accesslog.proto +++ b/api/envoy/data/accesslog/v2/accesslog.proto @@ -142,6 +142,9 @@ message AccessLogCommon { // upstream transport socket. Common TLS failures are in // :ref:`TLS trouble shooting `. string upstream_transport_failure_reason = 18; + + // The name of the route + string route_name = 19; } // Flags indicating occurrences during request/response processing. diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index d966dd250f237..d35bda09483d9 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -228,6 +228,9 @@ The following command operators are supported: TCP Not implemented ("-"). +%ROUTE_NAME% + Name of the route. + %UPSTREAM_HOST% Upstream host URL (e.g., tcp://ip:port for TCP connections). diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 79409fa4d6f97..026419fc904bc 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,7 @@ Version history 1.11.0 (Pending) ================ * access log: added a new field for downstream TLS session ID to file and gRPC access logger. +* access log: added a new field for route name to file and gRPC access logger. * access log: added a new field for response code details in :ref:`file access logger` and :ref:`gRPC access logger`. * admin: the administration interface now includes a :ref:`/ready endpoint ` for easier readiness checks. * admin: extend :ref:`/runtime_modify endpoint ` to support parameters within the request body. diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc index 963180ab409ae..e5eaa2efafd4c 100644 --- a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc @@ -277,6 +277,11 @@ void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, *common_properties->mutable_upstream_remote_address()); common_properties->set_upstream_cluster(stream_info.upstreamHost()->cluster().name()); } + + if (stream_info.getRouteName() != "") { + common_properties->set_route_name(stream_info.getRouteName()); + } + if (stream_info.upstreamLocalAddress() != nullptr) { Network::Utility::addressToProtobufAddress( *stream_info.upstreamLocalAddress(), *common_properties->mutable_upstream_local_address()); diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 2f4f418502a6a..23179c5dfddad 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -111,6 +111,7 @@ TEST_F(AccessLogImplTest, DownstreamDisconnect) { "\"10.0.0.5:1234\"\n", output_); } + TEST_F(AccessLogImplTest, RouteName) { const std::string json = R"EOF( { diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc index 0502513f980e1..a0e12ee5325fb 100644 --- a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc @@ -228,6 +228,8 @@ TEST_F(HttpGrpcAccessLogTest, Marshalling) { stream_info.addBytesSent(20); stream_info.response_code_ = 200; stream_info.response_code_details_ = "via_upstream"; + absl::string_view route_name_view("route-name-test"); + stream_info.setRouteName(route_name_view); ON_CALL(stream_info, hasResponseFlag(StreamInfo::ResponseFlag::FaultInjected)) .WillByDefault(Return(true)); @@ -283,6 +285,7 @@ TEST_F(HttpGrpcAccessLogTest, Marshalling) { upstream_cluster: "fake_cluster" response_flags: fault_injected: true + route_name: "route-name-test" protocol_version: HTTP10 request: scheme: "scheme_value" diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index ec2820957dd8d..0d102036dd90b 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -88,6 +88,10 @@ MockStreamInfo::MockStreamInfo() requested_server_name_ = std::string(requested_server_name); })); ON_CALL(*this, requestedServerName()).WillByDefault(ReturnRef(requested_server_name_)); + ON_CALL(*this, setRouteName(_)).WillByDefault(Invoke([this](const absl::string_view route_name) { + route_name_ = std::string(route_name); + })); + ON_CALL(*this, getRouteName()).WillByDefault(ReturnRef(route_name_)); ON_CALL(*this, upstreamTransportFailureReason()) .WillByDefault(ReturnRef(upstream_transport_failure_reason_)); } diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index ca67240a131ac..9332fec087ecf 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -105,6 +105,7 @@ class MockStreamInfo : public StreamInfo { Network::Address::InstanceConstSharedPtr downstream_remote_address_; const Ssl::ConnectionInfo* downstream_connection_info_{}; std::string requested_server_name_; + std::string route_name_; std::string upstream_transport_failure_reason_; }; From cb11c7582c52f0159273e15d2b44d6a42c58d3fb Mon Sep 17 00:00:00 2001 From: Divyani Rao Date: Mon, 20 May 2019 23:18:49 -0700 Subject: [PATCH 9/9] fixing clang-tidy error Signed-off-by: Divyani Rao --- .../extensions/access_loggers/http_grpc/grpc_access_log_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc index e5eaa2efafd4c..29e8bd9cc5da7 100644 --- a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc @@ -278,7 +278,7 @@ void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, common_properties->set_upstream_cluster(stream_info.upstreamHost()->cluster().name()); } - if (stream_info.getRouteName() != "") { + if (!stream_info.getRouteName().empty()) { common_properties->set_route_name(stream_info.getRouteName()); }