From d179e90a03adce2aba3c10f889aa4ac2675a7d39 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 2 Oct 2017 16:58:55 -0400 Subject: [PATCH 01/44] Allow non ws requests on WS route Signed-off-by: Shriram Rajagopalan --- source/common/http/conn_manager_impl.cc | 42 ++++++++++------------ source/common/http/conn_manager_impl.h | 1 - test/common/http/conn_manager_impl_test.cc | 26 -------------- 3 files changed, 18 insertions(+), 51 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 5953325b1c91..69c17ec4a293 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -504,34 +504,28 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // should return 404. The current returns no response if there is no router filter. if ((protocol == Protocol::Http11) && cached_route_.value()) { const Router::RouteEntry* route_entry = cached_route_.value()->routeEntry(); - const bool websocket_required = (route_entry != nullptr) && route_entry->useWebSocket(); + const bool websocket_allowed = (route_entry != nullptr) && route_entry->useWebSocket(); const bool websocket_requested = Utility::isWebSocketUpgradeRequest(*request_headers_); - if (websocket_requested || websocket_required) { - if (websocket_requested && websocket_required) { - ENVOY_STREAM_LOG(debug, "found websocket connection. (end_stream={}):", *this, end_stream); - - connection_manager_.ws_connection_.reset(new WebSocket::WsHandlerImpl( - *request_headers_, request_info_, *route_entry, *this, - connection_manager_.cluster_manager_, connection_manager_.read_callbacks_)); - connection_manager_.ws_connection_->onNewConnection(); - connection_manager_.stats_.named_.downstream_cx_websocket_active_.inc(); - connection_manager_.stats_.named_.downstream_cx_http1_active_.dec(); - connection_manager_.stats_.named_.downstream_cx_websocket_total_.inc(); - } else if (websocket_requested) { - // Do not allow WebSocket upgrades if the route does not support it. - connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); - HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::Forbidden))}}; - encodeHeaders(nullptr, headers, true); - } else { - // Do not allow normal connections on WebSocket routes. - connection_manager_.stats_.named_.downstream_rq_non_ws_on_ws_route_.inc(); - HeaderMapImpl headers{ - {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}}; - encodeHeaders(nullptr, headers, true); - } + if (websocket_requested && websocket_allowed) { + ENVOY_STREAM_LOG(debug, "found websocket connection. (end_stream={}):", *this, end_stream); + + connection_manager_.ws_connection_.reset(new WebSocket::WsHandlerImpl( + *request_headers_, request_info_, *route_entry, *this, + connection_manager_.cluster_manager_, connection_manager_.read_callbacks_)); + connection_manager_.ws_connection_->onNewConnection(); + connection_manager_.stats_.named_.downstream_cx_websocket_active_.inc(); + connection_manager_.stats_.named_.downstream_cx_http1_active_.dec(); + connection_manager_.stats_.named_.downstream_cx_websocket_total_.inc(); + return; + } else if (websocket_requested) { + // Do not allow WebSocket upgrades if the route does not support it. + connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); + HeaderMapImpl headers{{Headers::get().Status, std::to_string(enumToInt(Code::Forbidden))}}; + encodeHeaders(nullptr, headers, true); return; } + // Allow non websocket requests to go through websocket enabled routes. } // Check if tracing is enabled at all. diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 294686155ab4..8fe03069a5a1 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -74,7 +74,6 @@ namespace Http { COUNTER(downstream_rq_tx_reset) \ COUNTER(downstream_rq_non_relative_path) \ COUNTER(downstream_rq_ws_on_non_ws_route) \ - COUNTER(downstream_rq_non_ws_on_ws_route) \ COUNTER(downstream_rq_too_large) \ COUNTER(downstream_rq_2xx) \ COUNTER(downstream_rq_3xx) \ diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8a3d1898a09b..9fb2b2221d1b 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -616,32 +616,6 @@ TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { EXPECT_EQ(1U, stats_.named_.downstream_rq_ws_on_non_ws_route_.value()); } -TEST_F(HttpConnectionManagerImplTest, RejectNonWebSocketOnWebSocketRoute) { - setup(false, ""); - - StreamDecoder* decoder = nullptr; - NiceMock encoder; - ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) - .WillByDefault(Return(true)); - EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { - decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{ - new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/"}}}; - decoder->decodeHeaders(std::move(headers), true); - data.drain(4); - })); - - EXPECT_CALL(encoder, encodeHeaders(_, true)) - .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { - EXPECT_STREQ("426", headers.Status()->value().c_str()); - })); - - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input); - - EXPECT_EQ(1U, stats_.named_.downstream_rq_non_ws_on_ws_route_.value()); -} - TEST_F(HttpConnectionManagerImplTest, WebSocketNoThreadLocalCluster) { setup(false, ""); From 6e84e554b8b74cb8ff9cd04f4e7b560900a5392b Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 10:17:54 -0400 Subject: [PATCH 02/44] More tests for WebSockets Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 46 ++++++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 9fb2b2221d1b..86181644b962 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -616,6 +616,45 @@ TEST_F(HttpConnectionManagerImplTest, RejectWebSocketOnNonWebSocketRoute) { EXPECT_EQ(1U, stats_.named_.downstream_rq_ws_on_non_ws_route_.value()); } +TEST_F(HttpConnectionManagerImplTest, AllowNonWebSocketOnWebSocketRoute) { + setup(false, ""); + + StreamDecoder* decoder = nullptr; + NiceMock encoder; + NiceMock* upstream_connection_ = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection_; + conn_info.host_description_.reset( + new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::Metadata::default_instance(), 1, + envoy::api::v2::Locality().default_instance())); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); + + // Websocket enabled route + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) + .WillByDefault(Return(true)); + + // Non websocket request + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, + {":method", "GET"}, + {":path", "/scooby"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(4); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + + EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_active_.value()); + EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_total_.value()); + EXPECT_EQ(1U, stats_.named_.downstream_cx_http1_active_.value()); +} + TEST_F(HttpConnectionManagerImplTest, WebSocketNoThreadLocalCluster) { setup(false, ""); @@ -646,7 +685,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketNoConnInPool) { EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_active_.value()); } -TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixRewrite) { +TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { setup(false, ""); StreamDecoder* decoder = nullptr; @@ -669,12 +708,11 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixRewrite) { EXPECT_CALL(route_config_provider_.route_config_->route_->route_entry_, finalizeRequestHeaders(_, _)); EXPECT_CALL(route_config_provider_.route_config_->route_->route_entry_, autoHostRewrite()) - .WillOnce(Return(false)); - // TODO (rshriram) figure out how to test the auto host rewrite. Need handle over headers. + .WillOnce(Return(true)); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "newhost"}, {":method", "GET"}, {":path", "/scooby"}, {"connection", "Upgrade"}, From 2cedf446a227164fe9fdaf47b67e53a02990c523 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 11:04:07 -0400 Subject: [PATCH 03/44] Fix test for Non WS on WS route Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 86181644b962..ec454713a41a 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -621,17 +621,6 @@ TEST_F(HttpConnectionManagerImplTest, AllowNonWebSocketOnWebSocketRoute) { StreamDecoder* decoder = nullptr; NiceMock encoder; - NiceMock* upstream_connection_ = - new NiceMock(); - Upstream::MockHost::MockCreateConnectionData conn_info; - - conn_info.connection_ = upstream_connection_; - conn_info.host_description_.reset( - new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", - Network::Utility::resolveUrl("tcp://127.0.0.1:80"), - envoy::api::v2::Metadata::default_instance(), 1, - envoy::api::v2::Locality().default_instance())); - EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); // Websocket enabled route ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) @@ -640,9 +629,8 @@ TEST_F(HttpConnectionManagerImplTest, AllowNonWebSocketOnWebSocketRoute) { // Non websocket request EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, - {":method", "GET"}, - {":path", "/scooby"}}}; + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/"}}}; decoder->decodeHeaders(std::move(headers), true); data.drain(4); })); From 37af80d8064d84123e856ee825e939fa8cc8c27b Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 13:15:02 -0400 Subject: [PATCH 04/44] fix Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ec454713a41a..600a0f4b6e93 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -700,7 +700,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "newhost"}, + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, {":path", "/scooby"}, {"connection", "Upgrade"}, From 1645eaf7f82350cac6526841ed5650c0e07ba626 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 14:14:46 -0400 Subject: [PATCH 05/44] connect timeout test Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 48 ++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 600a0f4b6e93..596f88059f66 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -673,6 +673,54 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketNoConnInPool) { EXPECT_EQ(0U, stats_.named_.downstream_cx_websocket_active_.value()); } +TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { + setup(false, ""); + + StreamDecoder* decoder = nullptr; + NiceMock encoder; + NiceMock* upstream_connection_ = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) + .WillByDefault(Return(true)); + + conn_info.connection_ = upstream_connection_; + conn_info.host_description_.reset( + new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::Metadata::default_instance(), 1, + envoy::api::v2::Locality().default_instance())); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, + {":method", "GET"}, + {":path", "/scooby"}, + {"connection", "Upgrade"}, + {"upgrade", "websocket"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(4); + })); + + // gateway timeout error + EXPECT_CALL(encoder, encodeHeaders(_, true)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("504", headers.Status()->value().c_str()); + })); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + + Event::MockTimer* connect_timeout_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + EXPECT_CALL(*connect_timeout_timer, enableTimer(_)); + connect_timeout_timer->callback_(); + + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + conn_manager_.reset(); +} + TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { setup(false, ""); From 928272d72a9c136de55bf7f5b5a04f7720bd506b Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 14:59:15 -0400 Subject: [PATCH 06/44] format Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 596f88059f66..ee4b45471b79 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -713,7 +713,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); - Event::MockTimer* connect_timeout_timer = new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); + Event::MockTimer* connect_timeout_timer = + new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); EXPECT_CALL(*connect_timeout_timer, enableTimer(_)); connect_timeout_timer->callback_(); From 6ee665738a84611c547dcf340259199c6ecaf4dd Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 15:48:16 -0400 Subject: [PATCH 07/44] fix timeout test Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ee4b45471b79..587ad2bf32ad 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -678,12 +678,16 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { StreamDecoder* decoder = nullptr; NiceMock encoder; + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection_ = new NiceMock(); Upstream::MockHost::MockCreateConnectionData conn_info; ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) .WillByDefault(Return(true)); + EXPECT_CALL(*connect_timer, enableTimer(_)); conn_info.connection_ = upstream_connection_; conn_info.host_description_.reset( @@ -704,19 +708,16 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { data.drain(4); })); + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + // gateway timeout error EXPECT_CALL(encoder, encodeHeaders(_, true)) .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { EXPECT_STREQ("504", headers.Status()->value().c_str()); })); - Buffer::OwnedImpl fake_input("1234"); - conn_manager_->onData(fake_input); - - Event::MockTimer* connect_timeout_timer = - new Event::MockTimer(&filter_callbacks_.connection_.dispatcher_); - EXPECT_CALL(*connect_timeout_timer, enableTimer(_)); - connect_timeout_timer->callback_(); + connect_timer->callback_(); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); From 8599956fc9c636c1829fc9daa220f5fc2002c62b Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 3 Oct 2017 16:42:18 -0400 Subject: [PATCH 08/44] formatting Signed-off-by: Shriram Rajagopalan --- source/common/http/conn_manager_impl.h | 2 +- test/common/http/conn_manager_impl_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 0d84ee276754..fba7b30da227 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -74,7 +74,7 @@ namespace Http { COUNTER(downstream_rq_tx_reset) \ COUNTER(downstream_rq_non_relative_path) \ COUNTER(downstream_rq_ws_on_non_ws_route) \ - COUNTER(downstream_rq_too_large) \ + COUNTER(downstream_rq_too_large) \ COUNTER(downstream_rq_2xx) \ COUNTER(downstream_rq_3xx) \ COUNTER(downstream_rq_4xx) \ diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a92940c19791..2312377290aa 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -706,7 +706,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { decoder = &conn_manager_->newStream(encoder); HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, - {":path", "/scooby"}, + {":path", "/"}, {"connection", "Upgrade"}, {"upgrade", "websocket"}}}; decoder->decodeHeaders(std::move(headers), true); @@ -757,7 +757,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { decoder = &conn_manager_->newStream(encoder); HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, {":method", "GET"}, - {":path", "/scooby"}, + {":path", "/"}, {"connection", "Upgrade"}, {"upgrade", "websocket"}}}; decoder->decodeHeaders(std::move(headers), true); From 75ef0743aff1d6eefb9119abca27405825d613ad Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 4 Oct 2017 13:31:02 -0400 Subject: [PATCH 09/44] fix auto host rewrite check Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2312377290aa..ec1ccbe74076 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -736,6 +736,12 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { NiceMock* upstream_connection_ = new NiceMock(); Upstream::MockHost::MockCreateConnectionData conn_info; + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, + {":method", "GET"}, + {":path", "/"}, + {"connection", "Upgrade"}, + {"upgrade", "websocket"}}}; + auto raw_header_ptr = headers.get() conn_info.connection_ = upstream_connection_; conn_info.host_description_.reset( @@ -755,11 +761,6 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(encoder); - HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, - {":method", "GET"}, - {":path", "/"}, - {"connection", "Upgrade"}, - {"upgrade", "websocket"}}}; decoder->decodeHeaders(std::move(headers), true); data.drain(4); })); @@ -767,6 +768,8 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); + // rewritten authority header when auto_host_rewrite is true + EXPECT_STREQ("newhost", raw_header_ptr->Host()->value().c_str()); EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_active_.value()); EXPECT_EQ(1U, stats_.named_.downstream_cx_websocket_total_.value()); EXPECT_EQ(0U, stats_.named_.downstream_cx_http1_active_.value()); From baf1ee174fb86b4ccef4028bed6a3dc467475779 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 4 Oct 2017 14:08:36 -0400 Subject: [PATCH 10/44] compile fix Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ec1ccbe74076..fc46bed4f417 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -737,11 +737,11 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { new NiceMock(); Upstream::MockHost::MockCreateConnectionData conn_info; HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, - {":method", "GET"}, - {":path", "/"}, - {"connection", "Upgrade"}, - {"upgrade", "websocket"}}}; - auto raw_header_ptr = headers.get() + {":method", "GET"}, + {":path", "/"}, + {"connection", "Upgrade"}, + {"upgrade", "websocket"}}}; + auto raw_header_ptr = headers.get(); conn_info.connection_ = upstream_connection_; conn_info.host_description_.reset( From dd4b8aa85999718fba29b80ec5f257cf8ed0fc63 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 4 Oct 2017 16:23:00 -0400 Subject: [PATCH 11/44] connectionfailure test Signed-off-by: Shriram Rajagopalan --- test/common/http/conn_manager_impl_test.cc | 73 ++++++++++++++++++---- 1 file changed, 60 insertions(+), 13 deletions(-) diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index fc46bed4f417..a1f65ce430a7 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -681,27 +681,27 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketNoConnInPool) { TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { setup(false, ""); - StreamDecoder* decoder = nullptr; - NiceMock encoder; Event::MockTimer* connect_timer = new NiceMock(&filter_callbacks_.connection_.dispatcher_); - - NiceMock* upstream_connection_ = + NiceMock* upstream_connection = new NiceMock(); Upstream::MockHost::MockCreateConnectionData conn_info; - ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) - .WillByDefault(Return(true)); - EXPECT_CALL(*connect_timer, enableTimer(_)); - - conn_info.connection_ = upstream_connection_; + conn_info.connection_ = upstream_connection; conn_info.host_description_.reset( new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", Network::Utility::resolveUrl("tcp://127.0.0.1:80"), envoy::api::v2::Metadata::default_instance(), 1, envoy::api::v2::Locality().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); + StreamDecoder* decoder = nullptr; + NiceMock encoder; + + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) + .WillByDefault(Return(true)); + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { decoder = &conn_manager_->newStream(encoder); HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, @@ -713,17 +713,64 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketConnectTimeoutError) { data.drain(4); })); + EXPECT_CALL(encoder, encodeHeaders(_, true)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("504", headers.Status()->value().c_str()); + })); + Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input); - // gateway timeout error + connect_timer->callback_(); + filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); + conn_manager_.reset(); +} + +TEST_F(HttpConnectionManagerImplTest, WebSocketConnectionFailure) { + setup(false, ""); + + Event::MockTimer* connect_timer = + new NiceMock(&filter_callbacks_.connection_.dispatcher_); + NiceMock* upstream_connection = + new NiceMock(); + Upstream::MockHost::MockCreateConnectionData conn_info; + + conn_info.connection_ = upstream_connection; + conn_info.host_description_.reset( + new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", + Network::Utility::resolveUrl("tcp://127.0.0.1:80"), + envoy::api::v2::Metadata::default_instance(), 1, + envoy::api::v2::Locality().default_instance())); + EXPECT_CALL(*connect_timer, enableTimer(_)); + EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)).WillOnce(Return(conn_info)); + + StreamDecoder* decoder = nullptr; + NiceMock encoder; + + ON_CALL(route_config_provider_.route_config_->route_->route_entry_, useWebSocket()) + .WillByDefault(Return(true)); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, + {":method", "GET"}, + {":path", "/"}, + {"connection", "Upgrade"}, + {"upgrade", "websocket"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(4); + })); + EXPECT_CALL(encoder, encodeHeaders(_, true)) .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { EXPECT_STREQ("504", headers.Status()->value().c_str()); })); - connect_timer->callback_(); + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input); + // expectOnUpstreamInitFailure("504"); + upstream_connection->raiseEvent(Network::ConnectionEvent::RemoteClose); filter_callbacks_.connection_.dispatcher_.clearDeferredDeleteList(); conn_manager_.reset(); } @@ -733,7 +780,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { StreamDecoder* decoder = nullptr; NiceMock encoder; - NiceMock* upstream_connection_ = + NiceMock* upstream_connection = new NiceMock(); Upstream::MockHost::MockCreateConnectionData conn_info; HeaderMapPtr headers{new TestHeaderMapImpl{{":authority", "host"}, @@ -743,7 +790,7 @@ TEST_F(HttpConnectionManagerImplTest, WebSocketPrefixAndAutoHostRewrite) { {"upgrade", "websocket"}}}; auto raw_header_ptr = headers.get(); - conn_info.connection_ = upstream_connection_; + conn_info.connection_ = upstream_connection; conn_info.host_description_.reset( new Upstream::HostImpl(cluster_manager_.thread_local_cluster_.cluster_.info_, "newhost", Network::Utility::resolveUrl("tcp://127.0.0.1:80"), From c9a9fa4b4f2284b01b2723f36cf494e1fa9b8dd8 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 4 Oct 2017 17:11:04 -0400 Subject: [PATCH 12/44] update docs Signed-off-by: Shriram Rajagopalan --- .../http_conn_man/route_config/route.rst | 17 ++++++++--------- docs/configuration/http_conn_man/stats.rst | 1 - docs/intro/arch_overview/websocket.rst | 6 ++++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/configuration/http_conn_man/route_config/route.rst b/docs/configuration/http_conn_man/route_config/route.rst index ac6ad2a66b5d..0628554e4208 100644 --- a/docs/configuration/http_conn_man/route_config/route.rst +++ b/docs/configuration/http_conn_man/route_config/route.rst @@ -151,19 +151,18 @@ case_sensitive use_websocket *(optional, boolean)* Indicates that a HTTP/1.1 client connection to this particular route - should be allowed (and expected) to upgrade to a WebSocket connection. The default is false. + should be allowed to upgrade to a WebSocket connection. The default is false. .. attention:: If set to true, Envoy will expect the first request matching this route to contain WebSocket - upgrade headers. If the headers are not present, the connection will be rejected. If set to - true, Envoy will setup plain TCP proxying between the client and the upstream server. Hence, - an upstream server that rejects the WebSocket upgrade request is also responsible for closing - the associated connection. Until then, Envoy will continue to proxy data from the client to - the upstream server. - - Redirects, timeouts and retries are not supported on routes where websocket upgrades are - allowed. + upgrade headers. If the headers are not present, the connection will be processed as a normal + HTTP/1.1 connection. If the upgrade headers are present, Envoy will setup plain TCP proxying + between the client and the upstream server. Hence, an upstream server that rejects the WebSocket + upgrade request is also responsible for closing the associated connection. Until then, Envoy will + continue to proxy data from the client to the upstream server. + + Redirects, timeouts and retries are not supported on requests with WebSocket upgrade headers. .. _config_http_conn_man_route_table_route_timeout: diff --git a/docs/configuration/http_conn_man/stats.rst b/docs/configuration/http_conn_man/stats.rst index 49013f1019e9..495fe926bc8d 100644 --- a/docs/configuration/http_conn_man/stats.rst +++ b/docs/configuration/http_conn_man/stats.rst @@ -50,7 +50,6 @@ statistics: downstream_rq_4xx, Counter, Total 4xx responses downstream_rq_5xx, Counter, Total 5xx responses downstream_rq_ws_on_non_ws_route, Counter, Total WebSocket upgrade requests rejected by non WebSocket routes - downstream_rq_non_ws_on_ws_route, Counter, Total HTTP requests rejected by WebSocket enabled routes due to missing upgrade header downstream_rq_time, Timer, Request time milliseconds rs_too_large, Counter, Total response errors due to buffering an overly large body. diff --git a/docs/intro/arch_overview/websocket.rst b/docs/intro/arch_overview/websocket.rst index a9029dc83576..9d65b3b680e7 100644 --- a/docs/intro/arch_overview/websocket.rst +++ b/docs/intro/arch_overview/websocket.rst @@ -4,10 +4,12 @@ WebSocket support ================= Envoy supports upgrading a HTTP/1.1 connection to a WebSocket connection. -Connection upgrade will be considered if and only if the downstream client +Connection upgrade will be allowed only if the downstream client sends the correct upgrade headers and the matching HTTP route is explicitly -configured to use websockets +configured to use WebSockets (:ref:`use_websocket `). +If a request arrives at a WebSocket enabled route without the requisite +upgrade headers, it will be treated as any regular HTTP/1.1 request. Since Envoy treats WebSocket connections as plain TCP connections, it supports all drafts of the WebSocket protocol, independent of their wire From 659e79373bb089a869c0504827fb90adcff6e6bb Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Oct 2017 17:32:28 -0400 Subject: [PATCH 13/44] configurable cluster name length Signed-off-by: Shriram Rajagopalan --- include/envoy/server/options.h | 5 +++ source/common/config/BUILD | 1 + source/common/config/cds_json.cc | 9 +++++- source/common/json/config_schemas.cc | 3 +- source/common/stats/stats_impl.cc | 31 ++++++++++++++----- source/common/stats/stats_impl.h | 23 ++++++++++++++ source/server/BUILD | 1 + source/server/options_impl.cc | 24 ++++++++++++++ source/server/options_impl.h | 2 ++ .../upstream/cluster_manager_impl_test.cc | 5 +-- test/integration/server.h | 1 + test/mocks/server/mocks.cc | 1 + test/mocks/server/mocks.h | 1 + test/server/options_impl_test.cc | 5 +++ 14 files changed, 97 insertions(+), 15 deletions(-) diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h index e961bbec12be..891cd5a911da 100644 --- a/include/envoy/server/options.h +++ b/include/envoy/server/options.h @@ -130,6 +130,11 @@ class Options { * @return uint64_t the maximum name length of a stat. */ virtual uint64_t maxStatNameLength() PURE; + + /** + * @return uint64_t the maximum name length of a cluster/route/listener. + */ + virtual uint64_t maxUserSuppliedNameLength() PURE; }; } // namespace Server diff --git a/source/common/config/BUILD b/source/common/config/BUILD index acf65f95a46d..5728b25965a8 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -66,6 +66,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", + "//source/common/stats:stats_lib", ], ) diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index 40935b0cdebd..9818f1548f41 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -7,6 +7,7 @@ #include "common/config/tls_context_json.h" #include "common/config/utility.h" #include "common/json/config_schemas.h" +#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { @@ -84,7 +85,13 @@ void CdsJson::translateCluster(const Json::Object& json_cluster, const Optional& eds_config, envoy::api::v2::Cluster& cluster) { json_cluster.validateSchema(Json::Schema::CLUSTER_SCHEMA); - cluster.set_name(json_cluster.getString("name")); + + const std::string name = json_cluster.getString("name"); + if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { + throw EnvoyException("Cluster name is longer than max configured length"); + } + cluster.set_name(name); + const std::string string_type = json_cluster.getString("type"); auto set_dns_hosts = [&json_cluster, &cluster] { const auto hosts = json_cluster.getObjectArray("hosts"); diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index 421c26edda60..8e541cdedd5d 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1391,8 +1391,7 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "properties" : { "name" : { "type" : "string", - "minLength" : 1, - "maxLength" : 60 + "minLength" : 1 }, "type" : { "type" : "string", diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 2fdb75fd929b..9313add8118a 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -1,11 +1,10 @@ -#include "common/stats/stats_impl.h" - #include #include #include #include "common/common/utility.h" +#include "common/stats/stats_impl.h" namespace Envoy { namespace Stats { @@ -36,19 +35,35 @@ size_t& RawStatData::initializeAndGetMutableMaxNameLength(size_t configured_size return size; } +size_t& RawStatData::initializeAndGetMutableMaxUserSuppliedNameLength(size_t configured_size) { + // Like CONSTRUCT_ON_FIRST_USE, but non-const so that the value can be changed by tests + static size_t size = configured_size; + return size; +} + void RawStatData::configure(Server::Options& options) { - const size_t configured = options.maxStatNameLength(); - RELEASE_ASSERT(configured > 0); - size_t max_name_length = initializeAndGetMutableMaxNameLength(configured); + const size_t opt_max_stat_name_len = options.maxStatNameLength(); + const size_t opt_max_user_supplied_name_len = options.maxUserSuppliedNameLength(); + + RELEASE_ASSERT(opt_max_stat_name_len > 0); + RELEASE_ASSERT(opt_max_user_supplied_name_len > 0); + RELEASE_ASSERT(opt_max_stat_name_len >= (DEFAULT_MAX_STAT_SUFFIX_LENGTH + opt_max_user_supplied_name_len)); + size_t max_name_len = initializeAndGetMutableMaxNameLength(opt_max_stat_name_len); + size_t max_user_supplied_name_len = + initializeAndGetMutableMaxUserSuppliedNameLength(opt_max_user_supplied_name_len); // If this fails, it means that this function was called too late during // startup because things were already using this size before it was set. - RELEASE_ASSERT(max_name_length == configured); + RELEASE_ASSERT(max_name_len == opt_max_stat_name_len); + RELEASE_ASSERT(max_user_supplied_name_len == opt_max_user_supplied_name_len); } void RawStatData::configureForTestsOnly(Server::Options& options) { - const size_t configured = options.maxStatNameLength(); - initializeAndGetMutableMaxNameLength(configured) = configured; + const size_t opt_max_stat_name_len = options.maxStatNameLength(); + const size_t opt_max_user_supplied_name_len = options.maxUserSuppliedNameLength(); + initializeAndGetMutableMaxNameLength(opt_max_stat_name_len) = opt_max_stat_name_len; + initializeAndGetMutableMaxUserSuppliedNameLength(opt_max_user_supplied_name_len) = + opt_max_user_supplied_name_len; } std::string Utility::sanitizeStatsName(const std::string& name) { diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index d6d114916f5c..cad2d94b818b 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -16,6 +16,10 @@ #include "common/common/assert.h" namespace Envoy { + +//total length must be equal to DEFAULT_MAX_NAME_SIZE +#define DEFAULT_MAX_STAT_SUFFIX_LENGTH 67 + namespace Stats { /** @@ -67,6 +71,23 @@ struct RawStatData { return initializeAndGetMutableMaxNameLength(DEFAULT_MAX_NAME_SIZE); } + /** + * Returns the maximum length of a user supplied field in a stat. This length + * does not include a trailing NULL-terminator. + */ + static size_t maxUserSuppliedNameLength() { + return initializeAndGetMutableMaxUserSuppliedNameLength(DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE); + } + + /** + * Returns the maximum length of a envoy generated stats suffix that + * contributes to a stat name. This length does not include a + * trailing NULL-terminator. + */ + static size_t maxStatSuffixLength() { + return DEFAULT_MAX_STAT_SUFFIX_LENGTH; + } + /** * size in bytes of name_ */ @@ -103,8 +124,10 @@ struct RawStatData { private: static const size_t DEFAULT_MAX_NAME_SIZE = 127; + static const size_t DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE = 60; static size_t& initializeAndGetMutableMaxNameLength(size_t configured_size); + static size_t& initializeAndGetMutableMaxUserSuppliedNameLength(size_t configured_size); }; /** diff --git a/source/server/BUILD b/source/server/BUILD index e33387e6a8f3..53d2370a1af9 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -148,6 +148,7 @@ envoy_cc_library( "//include/envoy/server:options_interface", "//source/common/common:macros", "//source/common/common:version_lib", + "//source/common/stats:stats_lib", ], ) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 7109dd9d0683..3d3b07609582 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -7,6 +7,7 @@ #include "common/common/macros.h" #include "common/common/version.h" +#include "common/stats/stats_impl.h" #include "fmt/format.h" #include "spdlog/spdlog.h" @@ -22,10 +23,22 @@ #define ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH 127 #endif +// Can be overridden at compile time +#ifndef ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH +#define ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH 60 +#endif + #if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < 127 #error "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= 127" #endif +// Based on current defaults: cluster name is 60 chars, and the rest is 67 chars. +// So, with a configurable cluster name length, we still need to have head room for 67 chars. +#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH + DEFAULT_MAX_STAT_SUFFIX_LENGTH) +#error \ + "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH+DEFAULT_MAX_STAT_SUFFIX_LENGTH)" +#endif + namespace Envoy { OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_restart_version_cb, spdlog::level::level_enum default_log_level) { @@ -85,6 +98,9 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r TCLAP::ValueArg max_stat_name_len("", "max-stat-name-len", "Maximum name length for a stat", false, ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH, "uint64_t", cmd); + TCLAP::ValueArg max_user_supplied_name_len( + "", "max-user-supplied-name-len", "Maximum name length for a cluster, route or listener", + false, ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH, "uint64_t", cmd); try { cmd.parse(argc, argv); @@ -99,6 +115,13 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r exit(1); } + if (max_stat_name_len.getValue() < (max_user_supplied_name_len.getValue() + DEFAULT_MAX_STAT_SUFFIX_LENGTH)) { + std::cerr << "error: the 'max-stat-name-len' value specified (" << max_stat_name_len.getValue() + << ") is less than DEFAULT_MAX_STAT_SUFFIX_LENGTH (" << DEFAULT_MAX_STAT_SUFFIX_LENGTH << ") + max-user-supplied-name-len value (" + << max_user_supplied_name_len.getValue() << ")" << std::endl; + exit(1); + } + if (hot_restart_version_option.getValue()) { std::cerr << hot_restart_version_cb(max_stats.getValue(), max_stat_name_len.getValue()); exit(0); @@ -145,5 +168,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); max_stats_ = max_stats.getValue(); max_stat_name_length_ = max_stat_name_len.getValue(); + max_user_supplied_name_length_ = max_user_supplied_name_len.getValue(); } } // namespace Envoy diff --git a/source/server/options_impl.h b/source/server/options_impl.h index e8f75a036fda..75dff1659b1c 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -37,6 +37,7 @@ class OptionsImpl : public Server::Options { const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return max_stats_; } uint64_t maxStatNameLength() override { return max_stat_name_length_; } + uint64_t maxUserSuppliedNameLength() override { return max_user_supplied_name_length_; } private: uint64_t base_id_; @@ -56,5 +57,6 @@ class OptionsImpl : public Server::Options { Server::Mode mode_; uint64_t max_stats_; uint64_t max_stat_name_length_; + uint64_t max_user_supplied_name_length_; }; } // namespace Envoy diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d1f317324075..9f9ef9faca62 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -242,10 +242,7 @@ TEST_F(ClusterManagerImplTest, MaxClusterName) { } )EOF"; - EXPECT_THROW_WITH_MESSAGE(create(parseBootstrapFromJson(json)), Json::Exception, - "JSON at lines 4-6 does not conform to schema.\n Invalid schema: " - "#/properties/name\n Schema violation: maxLength\n Offending " - "document key: #/name"); + EXPECT_THROW(create(parseBootstrapFromJson(json)), EnvoyException); } TEST_F(ClusterManagerImplTest, ValidClusterName) { diff --git a/test/integration/server.h b/test/integration/server.h index b38f21657993..ed5b374bd87b 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -52,6 +52,7 @@ class TestOptionsImpl : public Options { const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return 16384; } uint64_t maxStatNameLength() override { return 127; } + uint64_t maxUserSuppliedNameLength() override { return 60; } private: const std::string config_path_; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index bf55d371cf73..e71926a89c4c 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -27,6 +27,7 @@ MockOptions::MockOptions(const std::string& config_path) ON_CALL(*this, logPath()).WillByDefault(ReturnRef(log_path_)); ON_CALL(*this, maxStats()).WillByDefault(Return(1000)); ON_CALL(*this, maxStatNameLength()).WillByDefault(Return(150)); + ON_CALL(*this, maxUserSuppliedNameLength()).WillByDefault(Return(70)); } MockOptions::~MockOptions() {} diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index a8cf4ae9e8f1..f0c670eba3db 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -60,6 +60,7 @@ class MockOptions : public Options { MOCK_METHOD0(serviceZone, const std::string&()); MOCK_METHOD0(maxStats, uint64_t()); MOCK_METHOD0(maxStatNameLength, uint64_t()); + MOCK_METHOD0(maxUserSuppliedNameLength, uint64_t()); std::string config_path_; std::string admin_address_path_; diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index ad237027527c..a6ea1fcb7216 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -76,4 +76,9 @@ TEST(OptionsImplTest, BadStatNameLenOption) { EXPECT_DEATH(createOptionsImpl("envoy --max-stat-name-len 1"), "error: the 'max-stat-name-len' value specified"); } + +TEST(OptionsImplTest, BadUserSuppliedNameLenOption) { + EXPECT_DEATH(createOptionsImpl("envoy --max-user-supplied-name-len 100"), + "error: the 'max-stat-name-len' value specified"); +} } // namespace Envoy From b51a66600bfdf04aea6db9f354e0093350dd0799 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Mon, 16 Oct 2017 17:39:12 -0400 Subject: [PATCH 14/44] formatting Signed-off-by: Shriram Rajagopalan --- source/common/stats/stats_impl.cc | 6 ++++-- source/common/stats/stats_impl.h | 6 ++---- source/server/options_impl.cc | 11 +++++++---- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 9313add8118a..8ae3a26803c8 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -1,10 +1,11 @@ +#include "common/stats/stats_impl.h" + #include #include #include #include "common/common/utility.h" -#include "common/stats/stats_impl.h" namespace Envoy { namespace Stats { @@ -47,7 +48,8 @@ void RawStatData::configure(Server::Options& options) { RELEASE_ASSERT(opt_max_stat_name_len > 0); RELEASE_ASSERT(opt_max_user_supplied_name_len > 0); - RELEASE_ASSERT(opt_max_stat_name_len >= (DEFAULT_MAX_STAT_SUFFIX_LENGTH + opt_max_user_supplied_name_len)); + RELEASE_ASSERT(opt_max_stat_name_len >= + (DEFAULT_MAX_STAT_SUFFIX_LENGTH + opt_max_user_supplied_name_len)); size_t max_name_len = initializeAndGetMutableMaxNameLength(opt_max_stat_name_len); size_t max_user_supplied_name_len = initializeAndGetMutableMaxUserSuppliedNameLength(opt_max_user_supplied_name_len); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index cad2d94b818b..824bfd9d34f1 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -17,7 +17,7 @@ namespace Envoy { -//total length must be equal to DEFAULT_MAX_NAME_SIZE +// total length must be equal to DEFAULT_MAX_NAME_SIZE #define DEFAULT_MAX_STAT_SUFFIX_LENGTH 67 namespace Stats { @@ -84,9 +84,7 @@ struct RawStatData { * contributes to a stat name. This length does not include a * trailing NULL-terminator. */ - static size_t maxStatSuffixLength() { - return DEFAULT_MAX_STAT_SUFFIX_LENGTH; - } + static size_t maxStatSuffixLength() { return DEFAULT_MAX_STAT_SUFFIX_LENGTH; } /** * size in bytes of name_ diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 3d3b07609582..e800d7c069fa 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -34,7 +34,8 @@ // Based on current defaults: cluster name is 60 chars, and the rest is 67 chars. // So, with a configurable cluster name length, we still need to have head room for 67 chars. -#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH + DEFAULT_MAX_STAT_SUFFIX_LENGTH) +#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < \ + (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH + DEFAULT_MAX_STAT_SUFFIX_LENGTH) #error \ "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH+DEFAULT_MAX_STAT_SUFFIX_LENGTH)" #endif @@ -115,10 +116,12 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r exit(1); } - if (max_stat_name_len.getValue() < (max_user_supplied_name_len.getValue() + DEFAULT_MAX_STAT_SUFFIX_LENGTH)) { + if (max_stat_name_len.getValue() < + (max_user_supplied_name_len.getValue() + DEFAULT_MAX_STAT_SUFFIX_LENGTH)) { std::cerr << "error: the 'max-stat-name-len' value specified (" << max_stat_name_len.getValue() - << ") is less than DEFAULT_MAX_STAT_SUFFIX_LENGTH (" << DEFAULT_MAX_STAT_SUFFIX_LENGTH << ") + max-user-supplied-name-len value (" - << max_user_supplied_name_len.getValue() << ")" << std::endl; + << ") is less than DEFAULT_MAX_STAT_SUFFIX_LENGTH (" << DEFAULT_MAX_STAT_SUFFIX_LENGTH + << ") + max-user-supplied-name-len value (" << max_user_supplied_name_len.getValue() + << ")" << std::endl; exit(1); } From c5d6e8fdce677a3371e950462610b1b61a70db4e Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 00:19:12 -0400 Subject: [PATCH 15/44] remove CLI option Signed-off-by: Shriram Rajagopalan --- include/envoy/server/options.h | 5 ----- source/common/config/BUILD | 2 ++ source/common/config/lds_json.cc | 6 ++++++ source/common/config/rds_json.cc | 1 + source/common/config/utility.cc | 6 ++++++ source/common/stats/stats_impl.cc | 29 ++++++----------------------- source/common/stats/stats_impl.h | 17 +++-------------- source/server/BUILD | 1 - source/server/options_impl.cc | 27 --------------------------- source/server/options_impl.h | 2 -- test/common/router/rds_impl_test.cc | 21 +++++++++++++++++++++ test/integration/server.h | 1 - test/mocks/server/mocks.cc | 1 - test/mocks/server/mocks.h | 1 - test/server/options_impl_test.cc | 5 ----- 15 files changed, 45 insertions(+), 80 deletions(-) diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h index 891cd5a911da..e961bbec12be 100644 --- a/include/envoy/server/options.h +++ b/include/envoy/server/options.h @@ -130,11 +130,6 @@ class Options { * @return uint64_t the maximum name length of a stat. */ virtual uint64_t maxStatNameLength() PURE; - - /** - * @return uint64_t the maximum name length of a cluster/route/listener. - */ - virtual uint64_t maxUserSuppliedNameLength() PURE; }; } // namespace Server diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 5728b25965a8..1a39b79fee69 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -189,6 +189,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", + "//source/common/stats:stats_lib", ], ) @@ -233,6 +234,7 @@ envoy_cc_library( "//include/envoy/json:json_object_interface", "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", + "//source/common/stats:stats_lib", ], ) diff --git a/source/common/config/lds_json.cc b/source/common/config/lds_json.cc index 79d1147176b6..3ee5cbee6217 100644 --- a/source/common/config/lds_json.cc +++ b/source/common/config/lds_json.cc @@ -7,6 +7,7 @@ #include "common/config/well_known_names.h" #include "common/json/config_schemas.h" #include "common/network/utility.h" +#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { @@ -15,6 +16,11 @@ void LdsJson::translateListener(const Json::Object& json_listener, envoy::api::v2::Listener& listener) { json_listener.validateSchema(Json::Schema::LISTENER_SCHEMA); + const std::string name = json_listener.getString("name"); + if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { + throw EnvoyException("Listener name is longer than max configured length"); + } + AddressJson::translateAddress(json_listener.getString("address"), true, true, *listener.mutable_address()); diff --git a/source/common/config/rds_json.cc b/source/common/config/rds_json.cc index feddd798ffe9..e0832eddb40c 100644 --- a/source/common/config/rds_json.cc +++ b/source/common/config/rds_json.cc @@ -7,6 +7,7 @@ #include "common/config/metadata.h" #include "common/config/well_known_names.h" #include "common/json/config_schemas.h" +#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index fcd1497a3a29..bed84f524245 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -86,6 +86,12 @@ void Utility::translateCdsConfig(const Json::Object& json_config, void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::filter::Rds& rds) { json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA); + + const std::string name = json_rds.getString("route_config_name"); + if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { + throw EnvoyException("Route config name is longer than max configured length"); + } + translateApiConfigSource(json_rds.getString("cluster"), json_rds.getInteger("refresh_delay_ms", 30000), json_rds.getString("api_type", ApiType::get().RestLegacy), diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 8ae3a26803c8..2fdb75fd929b 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -36,36 +36,19 @@ size_t& RawStatData::initializeAndGetMutableMaxNameLength(size_t configured_size return size; } -size_t& RawStatData::initializeAndGetMutableMaxUserSuppliedNameLength(size_t configured_size) { - // Like CONSTRUCT_ON_FIRST_USE, but non-const so that the value can be changed by tests - static size_t size = configured_size; - return size; -} - void RawStatData::configure(Server::Options& options) { - const size_t opt_max_stat_name_len = options.maxStatNameLength(); - const size_t opt_max_user_supplied_name_len = options.maxUserSuppliedNameLength(); - - RELEASE_ASSERT(opt_max_stat_name_len > 0); - RELEASE_ASSERT(opt_max_user_supplied_name_len > 0); - RELEASE_ASSERT(opt_max_stat_name_len >= - (DEFAULT_MAX_STAT_SUFFIX_LENGTH + opt_max_user_supplied_name_len)); - size_t max_name_len = initializeAndGetMutableMaxNameLength(opt_max_stat_name_len); - size_t max_user_supplied_name_len = - initializeAndGetMutableMaxUserSuppliedNameLength(opt_max_user_supplied_name_len); + const size_t configured = options.maxStatNameLength(); + RELEASE_ASSERT(configured > 0); + size_t max_name_length = initializeAndGetMutableMaxNameLength(configured); // If this fails, it means that this function was called too late during // startup because things were already using this size before it was set. - RELEASE_ASSERT(max_name_len == opt_max_stat_name_len); - RELEASE_ASSERT(max_user_supplied_name_len == opt_max_user_supplied_name_len); + RELEASE_ASSERT(max_name_length == configured); } void RawStatData::configureForTestsOnly(Server::Options& options) { - const size_t opt_max_stat_name_len = options.maxStatNameLength(); - const size_t opt_max_user_supplied_name_len = options.maxUserSuppliedNameLength(); - initializeAndGetMutableMaxNameLength(opt_max_stat_name_len) = opt_max_stat_name_len; - initializeAndGetMutableMaxUserSuppliedNameLength(opt_max_user_supplied_name_len) = - opt_max_user_supplied_name_len; + const size_t configured = options.maxStatNameLength(); + initializeAndGetMutableMaxNameLength(configured) = configured; } std::string Utility::sanitizeStatsName(const std::string& name) { diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 824bfd9d34f1..f37f902eb623 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -16,10 +16,6 @@ #include "common/common/assert.h" namespace Envoy { - -// total length must be equal to DEFAULT_MAX_NAME_SIZE -#define DEFAULT_MAX_STAT_SUFFIX_LENGTH 67 - namespace Stats { /** @@ -76,16 +72,9 @@ struct RawStatData { * does not include a trailing NULL-terminator. */ static size_t maxUserSuppliedNameLength() { - return initializeAndGetMutableMaxUserSuppliedNameLength(DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE); + return maxNameLength() - DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE; } - /** - * Returns the maximum length of a envoy generated stats suffix that - * contributes to a stat name. This length does not include a - * trailing NULL-terminator. - */ - static size_t maxStatSuffixLength() { return DEFAULT_MAX_STAT_SUFFIX_LENGTH; } - /** * size in bytes of name_ */ @@ -122,10 +111,10 @@ struct RawStatData { private: static const size_t DEFAULT_MAX_NAME_SIZE = 127; - static const size_t DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE = 60; + // total length must be equal to DEFAULT_MAX_NAME_SIZE + static const size_t DEFAULT_MAX_STAT_SUFFIX_LENGTH = 67; static size_t& initializeAndGetMutableMaxNameLength(size_t configured_size); - static size_t& initializeAndGetMutableMaxUserSuppliedNameLength(size_t configured_size); }; /** diff --git a/source/server/BUILD b/source/server/BUILD index 53d2370a1af9..e33387e6a8f3 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -148,7 +148,6 @@ envoy_cc_library( "//include/envoy/server:options_interface", "//source/common/common:macros", "//source/common/common:version_lib", - "//source/common/stats:stats_lib", ], ) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index e800d7c069fa..7109dd9d0683 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -7,7 +7,6 @@ #include "common/common/macros.h" #include "common/common/version.h" -#include "common/stats/stats_impl.h" #include "fmt/format.h" #include "spdlog/spdlog.h" @@ -23,23 +22,10 @@ #define ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH 127 #endif -// Can be overridden at compile time -#ifndef ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH -#define ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH 60 -#endif - #if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < 127 #error "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= 127" #endif -// Based on current defaults: cluster name is 60 chars, and the rest is 67 chars. -// So, with a configurable cluster name length, we still need to have head room for 67 chars. -#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < \ - (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH + DEFAULT_MAX_STAT_SUFFIX_LENGTH) -#error \ - "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= (ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH+DEFAULT_MAX_STAT_SUFFIX_LENGTH)" -#endif - namespace Envoy { OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_restart_version_cb, spdlog::level::level_enum default_log_level) { @@ -99,9 +85,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r TCLAP::ValueArg max_stat_name_len("", "max-stat-name-len", "Maximum name length for a stat", false, ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH, "uint64_t", cmd); - TCLAP::ValueArg max_user_supplied_name_len( - "", "max-user-supplied-name-len", "Maximum name length for a cluster, route or listener", - false, ENVOY_DEFAULT_MAX_USER_SUPPLIED_NAME_LENGTH, "uint64_t", cmd); try { cmd.parse(argc, argv); @@ -116,15 +99,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r exit(1); } - if (max_stat_name_len.getValue() < - (max_user_supplied_name_len.getValue() + DEFAULT_MAX_STAT_SUFFIX_LENGTH)) { - std::cerr << "error: the 'max-stat-name-len' value specified (" << max_stat_name_len.getValue() - << ") is less than DEFAULT_MAX_STAT_SUFFIX_LENGTH (" << DEFAULT_MAX_STAT_SUFFIX_LENGTH - << ") + max-user-supplied-name-len value (" << max_user_supplied_name_len.getValue() - << ")" << std::endl; - exit(1); - } - if (hot_restart_version_option.getValue()) { std::cerr << hot_restart_version_cb(max_stats.getValue(), max_stat_name_len.getValue()); exit(0); @@ -171,6 +145,5 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); max_stats_ = max_stats.getValue(); max_stat_name_length_ = max_stat_name_len.getValue(); - max_user_supplied_name_length_ = max_user_supplied_name_len.getValue(); } } // namespace Envoy diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 75dff1659b1c..e8f75a036fda 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -37,7 +37,6 @@ class OptionsImpl : public Server::Options { const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return max_stats_; } uint64_t maxStatNameLength() override { return max_stat_name_length_; } - uint64_t maxUserSuppliedNameLength() override { return max_user_supplied_name_length_; } private: uint64_t base_id_; @@ -57,6 +56,5 @@ class OptionsImpl : public Server::Options { Server::Mode mode_; uint64_t max_stats_; uint64_t max_stat_name_length_; - uint64_t max_user_supplied_name_length_; }; } // namespace Envoy diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 7ca441809cce..899b0cfd5d30 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -182,6 +182,27 @@ TEST_F(RdsImplTest, UnknownCluster) { EnvoyException); } +TEST_F(RdsImplTest, MaxRouteConfigName) { + const std::string config_json = R"EOF( + { + "rds": { + "cluster": "foo_cluster", + "route_config_name": "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema" + }, + "codec_type": "auto", + "stat_prefix": "foo", + "filters": [ + { "name": "http_dynamo_filter", "config": {} } + ] + } + )EOF"; + + EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), + runtime_, cm_, store_, "foo.", init_manager_, + *route_config_provider_manager_), + EnvoyException); +} + TEST_F(RdsImplTest, DestroyDuringInitialize) { InSequence s; diff --git a/test/integration/server.h b/test/integration/server.h index ed5b374bd87b..b38f21657993 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -52,7 +52,6 @@ class TestOptionsImpl : public Options { const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return 16384; } uint64_t maxStatNameLength() override { return 127; } - uint64_t maxUserSuppliedNameLength() override { return 60; } private: const std::string config_path_; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index e71926a89c4c..bf55d371cf73 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -27,7 +27,6 @@ MockOptions::MockOptions(const std::string& config_path) ON_CALL(*this, logPath()).WillByDefault(ReturnRef(log_path_)); ON_CALL(*this, maxStats()).WillByDefault(Return(1000)); ON_CALL(*this, maxStatNameLength()).WillByDefault(Return(150)); - ON_CALL(*this, maxUserSuppliedNameLength()).WillByDefault(Return(70)); } MockOptions::~MockOptions() {} diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index f0c670eba3db..a8cf4ae9e8f1 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -60,7 +60,6 @@ class MockOptions : public Options { MOCK_METHOD0(serviceZone, const std::string&()); MOCK_METHOD0(maxStats, uint64_t()); MOCK_METHOD0(maxStatNameLength, uint64_t()); - MOCK_METHOD0(maxUserSuppliedNameLength, uint64_t()); std::string config_path_; std::string admin_address_path_; diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index a6ea1fcb7216..ad237027527c 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -76,9 +76,4 @@ TEST(OptionsImplTest, BadStatNameLenOption) { EXPECT_DEATH(createOptionsImpl("envoy --max-stat-name-len 1"), "error: the 'max-stat-name-len' value specified"); } - -TEST(OptionsImplTest, BadUserSuppliedNameLenOption) { - EXPECT_DEATH(createOptionsImpl("envoy --max-user-supplied-name-len 100"), - "error: the 'max-stat-name-len' value specified"); -} } // namespace Envoy From d6caed2c9f65f430763ab3b528a230c57e535854 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 00:25:35 -0400 Subject: [PATCH 16/44] fix Signed-off-by: Shriram Rajagopalan --- source/common/stats/stats_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index f37f902eb623..466165202e01 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -72,7 +72,7 @@ struct RawStatData { * does not include a trailing NULL-terminator. */ static size_t maxUserSuppliedNameLength() { - return maxNameLength() - DEFAULT_MAX_USER_SUPPLIED_NAME_SIZE; + return maxNameLength() - DEFAULT_MAX_STAT_SUFFIX_LENGTH; } /** From 3092c06889e907bbd95a78b037678fde5a8659a2 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 13:04:56 -0400 Subject: [PATCH 17/44] rds/lds/cds tests Signed-off-by: Shriram Rajagopalan --- source/common/config/BUILD | 5 ++--- source/common/config/cds_json.cc | 5 +---- source/common/config/json_utility.h | 6 ++++++ source/common/config/lds_json.cc | 11 ++++------- source/common/config/rds_json.cc | 1 - source/common/config/utility.cc | 18 ++++++++++++------ source/common/config/utility.h | 8 ++++++++ test/common/config/BUILD | 1 + test/common/config/utility_test.cc | 21 +++++++++++++++++++++ 9 files changed, 55 insertions(+), 21 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 1a39b79fee69..4f6ef5513282 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -66,7 +66,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", - "//source/common/stats:stats_lib", ], ) @@ -184,12 +183,12 @@ envoy_cc_library( ":address_json_lib", ":json_utility_lib", ":tls_context_json_lib", + ":utility_lib", ":well_known_names", "//include/envoy/json:json_object_interface", "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", "//source/common/network:utility_lib", - "//source/common/stats:stats_lib", ], ) @@ -234,7 +233,6 @@ envoy_cc_library( "//include/envoy/json:json_object_interface", "//source/common/common:assert_lib", "//source/common/json:config_schemas_lib", - "//source/common/stats:stats_lib", ], ) @@ -293,6 +291,7 @@ envoy_cc_library( "//source/common/json:config_schemas_lib", "//source/common/protobuf", "//source/common/protobuf:utility_lib", + "//source/common/stats:stats_lib", ], ) diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index 9818f1548f41..c90155d3453f 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -7,7 +7,6 @@ #include "common/config/tls_context_json.h" #include "common/config/utility.h" #include "common/json/config_schemas.h" -#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { @@ -87,9 +86,7 @@ void CdsJson::translateCluster(const Json::Object& json_cluster, json_cluster.validateSchema(Json::Schema::CLUSTER_SCHEMA); const std::string name = json_cluster.getString("name"); - if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { - throw EnvoyException("Cluster name is longer than max configured length"); - } + Utility::checkUserSuppliedNameLength("Invalid cluster name", name); cluster.set_name(name); const std::string string_type = json_cluster.getString("type"); diff --git a/source/common/config/json_utility.h b/source/common/config/json_utility.h index a01c8ff5eecc..5dfb5d9ca6c2 100644 --- a/source/common/config/json_utility.h +++ b/source/common/config/json_utility.h @@ -6,6 +6,12 @@ // NOLINT(namespace-envoy) +// Set a string field in a protobuf message with the corresponding string value +#define JSON_UTIL_SET_STRING_VALUE(json, message, field_name, value) \ + do { \ + (message).set_##field_name(value); \ + } while (0) + // Set a string field in a protobuf message with the corresponding field's string // value from a JSON object. #define JSON_UTIL_SET_STRING(json, message, field_name) \ diff --git a/source/common/config/lds_json.cc b/source/common/config/lds_json.cc index 3ee5cbee6217..17c7f38b9c73 100644 --- a/source/common/config/lds_json.cc +++ b/source/common/config/lds_json.cc @@ -4,10 +4,10 @@ #include "common/config/address_json.h" #include "common/config/json_utility.h" #include "common/config/tls_context_json.h" +#include "common/config/utility.h" #include "common/config/well_known_names.h" #include "common/json/config_schemas.h" #include "common/network/utility.h" -#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { @@ -16,11 +16,8 @@ void LdsJson::translateListener(const Json::Object& json_listener, envoy::api::v2::Listener& listener) { json_listener.validateSchema(Json::Schema::LISTENER_SCHEMA); - const std::string name = json_listener.getString("name"); - if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { - throw EnvoyException("Listener name is longer than max configured length"); - } - + const std::string name = json_listener.getString("name", ""); + Utility::checkUserSuppliedNameLength("Invalid listener name", name); AddressJson::translateAddress(json_listener.getString("address"), true, true, *listener.mutable_address()); @@ -51,7 +48,7 @@ void LdsJson::translateListener(const Json::Object& json_listener, JSON_UTIL_SET_BOOL(json_listener, listener, use_original_dst); JSON_UTIL_SET_INTEGER(json_listener, listener, per_connection_buffer_limit_bytes); - JSON_UTIL_SET_STRING(json_listener, listener, name); + JSON_UTIL_SET_STRING_VALUE(json_listener, listener, name, name); JSON_UTIL_SET_BOOL(json_listener, *listener.mutable_deprecated_v1(), bind_to_port); } diff --git a/source/common/config/rds_json.cc b/source/common/config/rds_json.cc index e0832eddb40c..feddd798ffe9 100644 --- a/source/common/config/rds_json.cc +++ b/source/common/config/rds_json.cc @@ -7,7 +7,6 @@ #include "common/config/metadata.h" #include "common/config/well_known_names.h" #include "common/json/config_schemas.h" -#include "common/stats/stats_impl.h" namespace Envoy { namespace Config { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index bed84f524245..7efdd23016db 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -8,6 +8,7 @@ #include "common/json/config_schemas.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" +#include "common/stats/stats_impl.h" #include "fmt/format.h" @@ -87,16 +88,13 @@ void Utility::translateCdsConfig(const Json::Object& json_config, void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::filter::Rds& rds) { json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA); - const std::string name = json_rds.getString("route_config_name"); - if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { - throw EnvoyException("Route config name is longer than max configured length"); - } - + const std::string name = json_rds.getString("route_config_name", ""); + checkUserSuppliedNameLength("Invalid route_config_name", name); translateApiConfigSource(json_rds.getString("cluster"), json_rds.getInteger("refresh_delay_ms", 30000), json_rds.getString("api_type", ApiType::get().RestLegacy), *rds.mutable_config_source()->mutable_api_config_source()); - JSON_UTIL_SET_STRING(json_rds, rds, route_config_name); + JSON_UTIL_SET_STRING_VALUE(json_rds, rds, route_config_name, name); } void Utility::translateLdsConfig(const Json::Object& json_lds, @@ -125,5 +123,13 @@ std::string Utility::resourceName(const ProtobufWkt::Any& resource) { fmt::format("Unknown type URL {} in DiscoveryResponse", resource.type_url())); } +void Utility::checkUserSuppliedNameLength(const std::string& error_prefix, + const std::string& name) { + if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { + throw EnvoyException(fmt::format("{}: name exceeds allowed maximum {}", error_prefix, + Stats::RawStatData::maxUserSuppliedNameLength())); + } +} + } // namespace Config } // namespace Envoy diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 6e54bac4b42b..783617e8caec 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -212,6 +212,14 @@ class Utility { * @return std::string resource name. */ static std::string resourceName(const ProtobufWkt::Any& resource); + + /** + * Check user supplied name in RDS/CDS/LDS for sanity. + * It should be within the configured length limit. Throws on error. + * @param error_prefix supplies the prefix to use in error messages. + * @param name supplies the name to check for length limits. + */ + static void checkUserSuppliedNameLength(const std::string& error_prefix, const std::string& name); }; } // namespace Config diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 264bb918a71f..870b54987d0a 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -152,6 +152,7 @@ envoy_cc_test( srcs = ["utility_test.cc"], external_deps = ["envoy_eds"], deps = [ + "//source/common/config:lds_json_lib", "//source/common/config:utility_lib", "//test/mocks/local_info:local_info_mocks", ], diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index b5b6696ebad9..748ea55bf835 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -1,3 +1,4 @@ +#include "common/config/lds_json.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" @@ -69,5 +70,25 @@ TEST(UtilityTest, TranslateApiConfigSource) { EXPECT_EQ("test_grpc_cluster", api_config_source_grpc.cluster_name(0)); } +TEST(UtilityTest, UserSuppliedNameLength) { + EXPECT_THROW(Utility::checkUserSuppliedNameLength( + "test", "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema"), + EnvoyException); +} + +TEST(LdsJsonTest, TranslateListenerWithLongName) { + std::string lds_json = R"EOF( + { + "name": "listenerwithareallyreallylongnamemorethanmaxcharsallowedbyschema", + "address": "tcp://0.0.0.0:1", + "filters": [] + } + )EOF"; + + envoy::api::v2::Listener listener; + auto json_object_ptr = Json::Factory::loadFromString(lds_json); + EXPECT_THROW(Config::LdsJson::translateListener(*json_object_ptr, listener), EnvoyException); +} + } // namespace Config } // namespace Envoy From 03c9b312e65826bf1cd99224a9e835014b720b36 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 13:07:32 -0400 Subject: [PATCH 18/44] rename var Signed-off-by: Shriram Rajagopalan --- source/common/stats/stats_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 466165202e01..4f5f17838ac7 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -72,7 +72,7 @@ struct RawStatData { * does not include a trailing NULL-terminator. */ static size_t maxUserSuppliedNameLength() { - return maxNameLength() - DEFAULT_MAX_STAT_SUFFIX_LENGTH; + return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } /** @@ -112,7 +112,7 @@ struct RawStatData { private: static const size_t DEFAULT_MAX_NAME_SIZE = 127; // total length must be equal to DEFAULT_MAX_NAME_SIZE - static const size_t DEFAULT_MAX_STAT_SUFFIX_LENGTH = 67; + static const size_t MAX_STAT_SUFFIX_LENGTH = 67; static size_t& initializeAndGetMutableMaxNameLength(size_t configured_size); }; From 7a9e602f7199520dca2f3504e17f11188724a6ef Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 13:13:17 -0400 Subject: [PATCH 19/44] format Signed-off-by: Shriram Rajagopalan --- source/common/stats/stats_impl.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 4f5f17838ac7..802ce86de86b 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -71,9 +71,7 @@ struct RawStatData { * Returns the maximum length of a user supplied field in a stat. This length * does not include a trailing NULL-terminator. */ - static size_t maxUserSuppliedNameLength() { - return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; - } + static size_t maxUserSuppliedNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } /** * size in bytes of name_ From 9ae316013326f421f6f79ef5175fda562cffba93 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 15:18:46 -0400 Subject: [PATCH 20/44] change CLI option name Signed-off-by: Shriram Rajagopalan --- bazel/README.md | 7 ++++--- source/server/options_impl.cc | 30 +++++++++++++++++------------ test/integration/hotrestart_test.sh | 4 ++-- test/server/options_impl_test.cc | 6 +++--- 4 files changed, 27 insertions(+), 20 deletions(-) diff --git a/bazel/README.md b/bazel/README.md index ea300ded80b8..bd89b87fcf51 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -245,11 +245,12 @@ on the Bazel command line. ## Stats Tunables -The default maximum number of stats in shared memory, and the default maximum length of a stat name, -can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH`, +The default maximum number of stats in shared memory, and the default maximum length of a cluster/ +route config/listener name, +can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH`, respectively, to the desired value. For example: ``` -bazel build --copts=-DENVOY_DEFAULT_MAX_STATS=32768 //source/exe:envoy-static +bazel build --copts=-DENVOY_DEFAULT_MAX_STATS=32768 --copts=-DENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH=150 //source/exe:envoy-static ``` diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 7109dd9d0683..17973bdc346a 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -18,12 +18,14 @@ #endif // Can be overridden at compile time -#ifndef ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH -#define ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH 127 +#ifndef ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH +#define ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH 60 #endif -#if ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH < 127 -#error "ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH must be >= 127" +#define ENVOY_MAX_STAT_SUFFIX_LENGTH 67 + +#if ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH < 60 +#error "ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH must be >= 60" #endif namespace Envoy { @@ -82,9 +84,12 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r "Maximum number of stats guages and counters " "that can be allocated in shared memory.", false, ENVOY_DEFAULT_MAX_STATS, "uint64_t", cmd); - TCLAP::ValueArg max_stat_name_len("", "max-stat-name-len", - "Maximum name length for a stat", false, - ENVOY_DEFAULT_MAX_STAT_NAME_LENGTH, "uint64_t", cmd); + TCLAP::ValueArg max_obj_name_len("", "max-obj-name-len", + "Maximum name length for a field in the config " + "(applies to listener name, route config name and" + " the cluster name)", + false, ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH, "uint64_t", + cmd); try { cmd.parse(argc, argv); @@ -93,14 +98,15 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r exit(1); } - if (max_stat_name_len.getValue() < 127) { - std::cerr << "error: the 'max-stat-name-len' value specified (" << max_stat_name_len.getValue() - << ") is less than the minimum value of 127" << std::endl; + if (max_obj_name_len.getValue() < 60) { + std::cerr << "error: the 'max-obj-name-len' value specified (" << max_obj_name_len.getValue() + << ") is less than the minimum value of 60" << std::endl; exit(1); } if (hot_restart_version_option.getValue()) { - std::cerr << hot_restart_version_cb(max_stats.getValue(), max_stat_name_len.getValue()); + std::cerr << hot_restart_version_cb(max_stats.getValue(), + max_obj_name_len.getValue() + ENVOY_MAX_STAT_SUFFIX_LENGTH); exit(0); } @@ -144,6 +150,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r drain_time_ = std::chrono::seconds(drain_time_s.getValue()); parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); max_stats_ = max_stats.getValue(); - max_stat_name_length_ = max_stat_name_len.getValue(); + max_stat_name_length_ = max_obj_name_len.getValue() + ENVOY_MAX_STAT_SUFFIX_LENGTH; } } // namespace Envoy diff --git a/test/integration/hotrestart_test.sh b/test/integration/hotrestart_test.sh index 2a794b2478bb..ed72cb359e10 100755 --- a/test/integration/hotrestart_test.sh +++ b/test/integration/hotrestart_test.sh @@ -80,8 +80,8 @@ do exit 2 fi - echo "Checking max-stat-name-len" - CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --max-stat-name-len 1234 2>&1) + echo "Checking max-obj-name-len" + CLI_HOT_RESTART_VERSION=$("${ENVOY_BIN}" --hot-restart-version --max-obj-name-len 1234 2>&1) if [[ "${ADMIN_HOT_RESTART_VERSION}" = "${CLI_HOT_RESTART_VERSION}" ]]; then echo "Hot restart version match when it should mismatch: ${ADMIN_HOT_RESTART_VERSION} == " \ "${CLI_HOT_RESTART_VERSION}" diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index ad237027527c..e87826da98f9 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -72,8 +72,8 @@ TEST(OptionsImplTest, BadCliOption) { "error: unknown IP address version 'foo'"); } -TEST(OptionsImplTest, BadStatNameLenOption) { - EXPECT_DEATH(createOptionsImpl("envoy --max-stat-name-len 1"), - "error: the 'max-stat-name-len' value specified"); +TEST(OptionsImplTest, BadObjNameLenOption) { + EXPECT_DEATH(createOptionsImpl("envoy --max-obj-name-len 1"), + "error: the 'max-obj-name-len' value specified"); } } // namespace Envoy From 503eecfbdf0c4ea5cd4102c0b1f5b767a40e8d8d Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 16:02:48 -0400 Subject: [PATCH 21/44] feedback Signed-off-by: Shriram Rajagopalan --- source/common/config/utility.cc | 3 ++- source/common/stats/stats_impl.h | 6 ++++++ source/server/BUILD | 1 + source/server/options_impl.cc | 8 ++++---- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 7efdd23016db..1cd08ddfbb8e 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -126,7 +126,8 @@ std::string Utility::resourceName(const ProtobufWkt::Any& resource) { void Utility::checkUserSuppliedNameLength(const std::string& error_prefix, const std::string& name) { if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { - throw EnvoyException(fmt::format("{}: name exceeds allowed maximum {}", error_prefix, + throw EnvoyException(fmt::format("{}: Length of {} ({}) exceeds allowed maximum length ({})", + error_prefix, name, name.length(), Stats::RawStatData::maxUserSuppliedNameLength())); } } diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 802ce86de86b..aca8235e0407 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -73,6 +73,12 @@ struct RawStatData { */ static size_t maxUserSuppliedNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } + /** + * Returns the maximum length of a stat suffix that Envoy generates (over the user supplied name). + * This length does not include a trailing NULL-terminator. + */ + static size_t maxStatSuffixLength() { return MAX_STAT_SUFFIX_LENGTH; } + /** * size in bytes of name_ */ diff --git a/source/server/BUILD b/source/server/BUILD index e33387e6a8f3..53d2370a1af9 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -148,6 +148,7 @@ envoy_cc_library( "//include/envoy/server:options_interface", "//source/common/common:macros", "//source/common/common:version_lib", + "//source/common/stats:stats_lib", ], ) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 17973bdc346a..2c4ba73a0c64 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -7,6 +7,7 @@ #include "common/common/macros.h" #include "common/common/version.h" +#include "common/stats/stats_impl.h" #include "fmt/format.h" #include "spdlog/spdlog.h" @@ -22,8 +23,6 @@ #define ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH 60 #endif -#define ENVOY_MAX_STAT_SUFFIX_LENGTH 67 - #if ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH < 60 #error "ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH must be >= 60" #endif @@ -106,7 +105,8 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r if (hot_restart_version_option.getValue()) { std::cerr << hot_restart_version_cb(max_stats.getValue(), - max_obj_name_len.getValue() + ENVOY_MAX_STAT_SUFFIX_LENGTH); + max_obj_name_len.getValue() + + Stats::RawStatData::maxStatSuffixLength()); exit(0); } @@ -150,6 +150,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r drain_time_ = std::chrono::seconds(drain_time_s.getValue()); parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); max_stats_ = max_stats.getValue(); - max_stat_name_length_ = max_obj_name_len.getValue() + ENVOY_MAX_STAT_SUFFIX_LENGTH; + max_stat_name_length_ = max_obj_name_len.getValue() + Stats::RawStatData::maxStatSuffixLength(); } } // namespace Envoy From aad79deafb5eff7ca8d90345844ce9892ff8dd24 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 16:35:43 -0400 Subject: [PATCH 22/44] sweeping renames Signed-off-by: Shriram Rajagopalan --- include/envoy/server/options.h | 5 +++-- source/common/config/utility.cc | 7 +++---- source/common/stats/stats_impl.cc | 4 ++-- source/common/stats/stats_impl.h | 6 +++--- source/server/options_impl.cc | 2 +- source/server/options_impl.h | 4 ++-- test/integration/server.h | 2 +- test/mocks/server/mocks.cc | 2 +- test/mocks/server/mocks.h | 2 +- test/server/BUILD | 1 + test/server/hot_restart_impl_test.cc | 10 +++++++--- 11 files changed, 25 insertions(+), 20 deletions(-) diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h index e961bbec12be..4e571de4c613 100644 --- a/include/envoy/server/options.h +++ b/include/envoy/server/options.h @@ -127,9 +127,10 @@ class Options { virtual uint64_t maxStats() PURE; /** - * @return uint64_t the maximum name length of a stat. + * @return uint64_t the maximum name length of the name field in + * router/cluster/listener. */ - virtual uint64_t maxStatNameLength() PURE; + virtual uint64_t maxObjNameLength() PURE; }; } // namespace Server diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 1cd08ddfbb8e..630d59c4f681 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -123,12 +123,11 @@ std::string Utility::resourceName(const ProtobufWkt::Any& resource) { fmt::format("Unknown type URL {} in DiscoveryResponse", resource.type_url())); } -void Utility::checkUserSuppliedNameLength(const std::string& error_prefix, - const std::string& name) { - if (name.length() > Stats::RawStatData::maxUserSuppliedNameLength()) { +void Utility::checkObjNameLength(const std::string& error_prefix, const std::string& name) { + if (name.length() > Stats::RawStatData::maxObjNameLength()) { throw EnvoyException(fmt::format("{}: Length of {} ({}) exceeds allowed maximum length ({})", error_prefix, name, name.length(), - Stats::RawStatData::maxUserSuppliedNameLength())); + Stats::RawStatData::maxObjNameLength())); } } diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 2fdb75fd929b..bfc6d1e382b3 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -37,7 +37,7 @@ size_t& RawStatData::initializeAndGetMutableMaxNameLength(size_t configured_size } void RawStatData::configure(Server::Options& options) { - const size_t configured = options.maxStatNameLength(); + const size_t configured = options.maxObjNameLength() + maxStatSuffixLength(); RELEASE_ASSERT(configured > 0); size_t max_name_length = initializeAndGetMutableMaxNameLength(configured); @@ -47,7 +47,7 @@ void RawStatData::configure(Server::Options& options) { } void RawStatData::configureForTestsOnly(Server::Options& options) { - const size_t configured = options.maxStatNameLength(); + const size_t configured = options.maxObjNameLength() + maxStatSuffixLength(); initializeAndGetMutableMaxNameLength(configured) = configured; } diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index aca8235e0407..7736fecaabb2 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -68,10 +68,10 @@ struct RawStatData { } /** - * Returns the maximum length of a user supplied field in a stat. This length - * does not include a trailing NULL-terminator. + * Returns the maximum length of a user supplied object (route/cluster/listener) + * name field in a stat. This length does not include a trailing NULL-terminator. */ - static size_t maxUserSuppliedNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } + static size_t maxObjNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } /** * Returns the maximum length of a stat suffix that Envoy generates (over the user supplied name). diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 2c4ba73a0c64..df4818da2be6 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -150,6 +150,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r drain_time_ = std::chrono::seconds(drain_time_s.getValue()); parent_shutdown_time_ = std::chrono::seconds(parent_shutdown_time_s.getValue()); max_stats_ = max_stats.getValue(); - max_stat_name_length_ = max_obj_name_len.getValue() + Stats::RawStatData::maxStatSuffixLength(); + max_obj_name_length_ = max_obj_name_len.getValue(); } } // namespace Envoy diff --git a/source/server/options_impl.h b/source/server/options_impl.h index e8f75a036fda..73d3c2941a2c 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -36,7 +36,7 @@ class OptionsImpl : public Server::Options { const std::string& serviceNodeName() override { return service_node_; } const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return max_stats_; } - uint64_t maxStatNameLength() override { return max_stat_name_length_; } + uint64_t maxObjNameLength() override { return max_obj_name_length_; } private: uint64_t base_id_; @@ -55,6 +55,6 @@ class OptionsImpl : public Server::Options { std::chrono::seconds parent_shutdown_time_; Server::Mode mode_; uint64_t max_stats_; - uint64_t max_stat_name_length_; + uint64_t max_obj_name_length_; }; } // namespace Envoy diff --git a/test/integration/server.h b/test/integration/server.h index b38f21657993..2b7a8e73c0af 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -51,7 +51,7 @@ class TestOptionsImpl : public Options { const std::string& serviceNodeName() override { return service_node_name_; } const std::string& serviceZone() override { return service_zone_; } uint64_t maxStats() override { return 16384; } - uint64_t maxStatNameLength() override { return 127; } + uint64_t maxObjNameLength() override { return 60; } private: const std::string config_path_; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index bf55d371cf73..fb787f2aecac 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -26,7 +26,7 @@ MockOptions::MockOptions(const std::string& config_path) ON_CALL(*this, serviceZone()).WillByDefault(ReturnRef(service_zone_name_)); ON_CALL(*this, logPath()).WillByDefault(ReturnRef(log_path_)); ON_CALL(*this, maxStats()).WillByDefault(Return(1000)); - ON_CALL(*this, maxStatNameLength()).WillByDefault(Return(150)); + ON_CALL(*this, maxObjNameLength()).WillByDefault(Return(150)); } MockOptions::~MockOptions() {} diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index a8cf4ae9e8f1..0e1bcdcde58e 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -59,7 +59,7 @@ class MockOptions : public Options { MOCK_METHOD0(serviceNodeName, const std::string&()); MOCK_METHOD0(serviceZone, const std::string&()); MOCK_METHOD0(maxStats, uint64_t()); - MOCK_METHOD0(maxStatNameLength, uint64_t()); + MOCK_METHOD0(maxObjNameLength, uint64_t()); std::string config_path_; std::string admin_address_path_; diff --git a/test/server/BUILD b/test/server/BUILD index d338371f65dd..5c86a7640778 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -59,6 +59,7 @@ envoy_cc_test( name = "hot_restart_impl_test", srcs = envoy_select_hot_restart(["hot_restart_impl_test.cc"]), deps = [ + "//source/common/stats:stats_lib", "//source/server:hot_restart_lib", "//test/mocks/server:server_mocks", ], diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 2ef43ac122f4..4784ccbd5c23 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -1,3 +1,5 @@ +#include "common/stats/stats_impl.h" + #include "server/hot_restart_impl.h" #include "test/mocks/api/mocks.h" @@ -50,8 +52,10 @@ class HotRestartImplTest : public testing::Test { TEST_F(HotRestartImplTest, versionString) { setup(); - EXPECT_EQ(hot_restart_->version(), Envoy::Server::SharedMemory::version( - options_.maxStats(), options_.maxStatNameLength())); + EXPECT_EQ(hot_restart_->version(), + Envoy::Server::SharedMemory::version(options_.maxStats(), + options_.maxObjNameLength() + + Stats::RawStatData::maxStatSuffixLength())); } TEST_F(HotRestartImplTest, crossAlloc) { @@ -101,7 +105,7 @@ class HotRestartImplAlignmentTest : public HotRestartImplTest, public: HotRestartImplAlignmentTest() : name_len_(8 + GetParam()) { EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(num_stats_)); - EXPECT_CALL(options_, maxStatNameLength()).WillRepeatedly(Return(name_len_)); + EXPECT_CALL(options_, maxObjNameLength()).WillRepeatedly(Return(name_len_)); setup(); EXPECT_EQ(name_len_, Stats::RawStatData::maxNameLength()); } From 292516358d90cb9c4eadd9babfdb9d653bbd1b61 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 16:37:56 -0400 Subject: [PATCH 23/44] fix Signed-off-by: Shriram Rajagopalan --- source/common/config/cds_json.cc | 2 +- source/common/config/lds_json.cc | 2 +- source/common/config/utility.cc | 2 +- source/common/config/utility.h | 2 +- test/common/config/utility_test.cc | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/config/cds_json.cc b/source/common/config/cds_json.cc index c90155d3453f..2f780cbf6c6a 100644 --- a/source/common/config/cds_json.cc +++ b/source/common/config/cds_json.cc @@ -86,7 +86,7 @@ void CdsJson::translateCluster(const Json::Object& json_cluster, json_cluster.validateSchema(Json::Schema::CLUSTER_SCHEMA); const std::string name = json_cluster.getString("name"); - Utility::checkUserSuppliedNameLength("Invalid cluster name", name); + Utility::checkObjNameLength("Invalid cluster name", name); cluster.set_name(name); const std::string string_type = json_cluster.getString("type"); diff --git a/source/common/config/lds_json.cc b/source/common/config/lds_json.cc index 17c7f38b9c73..48f577505b84 100644 --- a/source/common/config/lds_json.cc +++ b/source/common/config/lds_json.cc @@ -17,7 +17,7 @@ void LdsJson::translateListener(const Json::Object& json_listener, json_listener.validateSchema(Json::Schema::LISTENER_SCHEMA); const std::string name = json_listener.getString("name", ""); - Utility::checkUserSuppliedNameLength("Invalid listener name", name); + Utility::checkObjNameLength("Invalid listener name", name); AddressJson::translateAddress(json_listener.getString("address"), true, true, *listener.mutable_address()); diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 630d59c4f681..715b1f704718 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -89,7 +89,7 @@ void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::f json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA); const std::string name = json_rds.getString("route_config_name", ""); - checkUserSuppliedNameLength("Invalid route_config_name", name); + checkObjNameLength("Invalid route_config_name", name); translateApiConfigSource(json_rds.getString("cluster"), json_rds.getInteger("refresh_delay_ms", 30000), json_rds.getString("api_type", ApiType::get().RestLegacy), diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 783617e8caec..618c91c81546 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -219,7 +219,7 @@ class Utility { * @param error_prefix supplies the prefix to use in error messages. * @param name supplies the name to check for length limits. */ - static void checkUserSuppliedNameLength(const std::string& error_prefix, const std::string& name); + static void checkObjNameLength(const std::string& error_prefix, const std::string& name); }; } // namespace Config diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 748ea55bf835..5da5b1f3088c 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -70,8 +70,8 @@ TEST(UtilityTest, TranslateApiConfigSource) { EXPECT_EQ("test_grpc_cluster", api_config_source_grpc.cluster_name(0)); } -TEST(UtilityTest, UserSuppliedNameLength) { - EXPECT_THROW(Utility::checkUserSuppliedNameLength( +TEST(UtilityTest, ObjNameLength) { + EXPECT_THROW(Utility::checkObjNameLength( "test", "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema"), EnvoyException); } From 4507c1008eb40d0e4f085182742d67dc04943f3c Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 18:00:35 -0400 Subject: [PATCH 24/44] test fix Signed-off-by: Shriram Rajagopalan --- test/server/hot_restart_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 4784ccbd5c23..e1056d85800d 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -107,7 +107,7 @@ class HotRestartImplAlignmentTest : public HotRestartImplTest, EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(num_stats_)); EXPECT_CALL(options_, maxObjNameLength()).WillRepeatedly(Return(name_len_)); setup(); - EXPECT_EQ(name_len_, Stats::RawStatData::maxNameLength()); + EXPECT_EQ(name_len_, Stats::RawStatData::maxObjNameLength()); } static const uint64_t num_stats_ = 8; @@ -136,7 +136,7 @@ TEST_P(HotRestartImplAlignmentTest, objectOverlap) { std::vector stats; for (uint64_t i = 0; i < num_stats_; i++) { std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzz", i) - .substr(0, Stats::RawStatData::maxNameLength()); + .substr(0, Stats::RawStatData::maxObjNameLength()); TestStat ts; ts.stat_ = hot_restart_->alloc(name); ts.name_ = ts.stat_->name_; @@ -144,7 +144,7 @@ TEST_P(HotRestartImplAlignmentTest, objectOverlap) { // If this isn't true then the hard coded part of the name isn't long enough to make the test // valid. - EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxNameLength()); + EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxObjNameLength()); stats.push_back(ts); } From cae08f0a2d13ff459b638f584f4dcb8720bd3357 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 18:44:54 -0400 Subject: [PATCH 25/44] long string to fix hot_restart_test Signed-off-by: Shriram Rajagopalan --- test/server/hot_restart_impl_test.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index e1056d85800d..d393596171f3 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -135,8 +135,12 @@ TEST_P(HotRestartImplAlignmentTest, objectOverlap) { }; std::vector stats; for (uint64_t i = 0; i < num_stats_; i++) { - std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzz", i) - .substr(0, Stats::RawStatData::maxObjNameLength()); + // 67 Z characters + 1 digit (num_stats == 8) for total of 68 characters + std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" + "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", + i) + .substr(0, Stats::RawStatData::maxNameLength()); TestStat ts; ts.stat_ = hot_restart_->alloc(name); ts.name_ = ts.stat_->name_; @@ -144,7 +148,7 @@ TEST_P(HotRestartImplAlignmentTest, objectOverlap) { // If this isn't true then the hard coded part of the name isn't long enough to make the test // valid. - EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxObjNameLength()); + EXPECT_EQ(ts.name_.size(), Stats::RawStatData::maxNameLength()); stats.push_back(ts); } From 5afd3ba2acb580deac0d08e2d4e718bbfda50987 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Tue, 17 Oct 2017 18:53:11 -0400 Subject: [PATCH 26/44] remove stray comment Signed-off-by: Shriram Rajagopalan --- test/server/hot_restart_impl_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index d393596171f3..a76ce8618193 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -135,7 +135,6 @@ TEST_P(HotRestartImplAlignmentTest, objectOverlap) { }; std::vector stats; for (uint64_t i = 0; i < num_stats_; i++) { - // 67 Z characters + 1 digit (num_stats == 8) for total of 68 characters std::string name = fmt::format("{}zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", From d4de240f71a378f14597117e52471b443bff8d7e Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Oct 2017 09:57:02 -0400 Subject: [PATCH 27/44] docs and nits Signed-off-by: Shriram Rajagopalan --- bazel/README.md | 10 ++++++---- docs/operations/cli.rst | 13 ++++++++++--- source/common/config/json_utility.h | 6 ------ source/common/config/lds_json.cc | 7 +++---- source/common/config/utility.cc | 2 +- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/bazel/README.md b/bazel/README.md index bd89b87fcf51..ecb40342f88d 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -245,10 +245,12 @@ on the Bazel command line. ## Stats Tunables -The default maximum number of stats in shared memory, and the default maximum length of a cluster/ -route config/listener name, -can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH`, -respectively, to the desired value. For example: +The default maximum number of stats in shared memory, and the default +maximum length of a cluster/ route config/listener name, can be +overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and +`ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH`, respectively, to the desired +value. For example: + ``` bazel build --copts=-DENVOY_DEFAULT_MAX_STATS=32768 --copts=-DENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH=150 //source/exe:envoy-static ``` diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst index fe7d1e2d4937..297feff8318a 100644 --- a/docs/operations/cli.rst +++ b/docs/operations/cli.rst @@ -117,10 +117,17 @@ following are the command line options that Envoy supports. during a hot restart. See the :ref:`hot restart overview ` for more information. Defaults to 900 seconds (15 minutes). -.. option:: --max-stat-name-len +.. option:: --max-obj-name-len - *(optional)* The maximum name length (in bytes) for a stat. This setting affects the output - of :option:`--hot-restart-version`; the same value must be used to hot restart. Defaults to 127. + *(optional)* The maximum name length (in bytes) of the name field in a cluster/route_config/listener. + This setting is typically used in scenarios where the cluster names are auto generated, and often exceed + the built-in limit of 60 characters. Defaults to 60. + + .. attention:: + + This setting affects the output of :option:`--hot-restart-version`. If you started envoy with this + option set to a non default value, you should use the same option (and same value) for subsequent hot + restarts. .. option:: --max-stats diff --git a/source/common/config/json_utility.h b/source/common/config/json_utility.h index 5dfb5d9ca6c2..a01c8ff5eecc 100644 --- a/source/common/config/json_utility.h +++ b/source/common/config/json_utility.h @@ -6,12 +6,6 @@ // NOLINT(namespace-envoy) -// Set a string field in a protobuf message with the corresponding string value -#define JSON_UTIL_SET_STRING_VALUE(json, message, field_name, value) \ - do { \ - (message).set_##field_name(value); \ - } while (0) - // Set a string field in a protobuf message with the corresponding field's string // value from a JSON object. #define JSON_UTIL_SET_STRING(json, message, field_name) \ diff --git a/source/common/config/lds_json.cc b/source/common/config/lds_json.cc index 48f577505b84..aa8ab279e7c2 100644 --- a/source/common/config/lds_json.cc +++ b/source/common/config/lds_json.cc @@ -18,6 +18,8 @@ void LdsJson::translateListener(const Json::Object& json_listener, const std::string name = json_listener.getString("name", ""); Utility::checkObjNameLength("Invalid listener name", name); + listener.set_name(name); + AddressJson::translateAddress(json_listener.getString("address"), true, true, *listener.mutable_address()); @@ -45,12 +47,9 @@ void LdsJson::translateListener(const Json::Object& json_listener, } JSON_UTIL_SET_BOOL(json_listener, *filter_chain, use_proxy_proto); - JSON_UTIL_SET_BOOL(json_listener, listener, use_original_dst); - JSON_UTIL_SET_INTEGER(json_listener, listener, per_connection_buffer_limit_bytes); - JSON_UTIL_SET_STRING_VALUE(json_listener, listener, name, name); - JSON_UTIL_SET_BOOL(json_listener, *listener.mutable_deprecated_v1(), bind_to_port); + JSON_UTIL_SET_INTEGER(json_listener, listener, per_connection_buffer_limit_bytes); } } // namespace Config diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 715b1f704718..b72043fe3edb 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -94,7 +94,7 @@ void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::f json_rds.getInteger("refresh_delay_ms", 30000), json_rds.getString("api_type", ApiType::get().RestLegacy), *rds.mutable_config_source()->mutable_api_config_source()); - JSON_UTIL_SET_STRING_VALUE(json_rds, rds, route_config_name, name); + rds.set_route_config_name(name); } void Utility::translateLdsConfig(const Json::Object& json_lds, From 65debb2ee4c23faf52498398d3cff5a892712e64 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Oct 2017 11:25:03 -0400 Subject: [PATCH 28/44] max len limit for vhosts Signed-off-by: Shriram Rajagopalan --- .../configuration/cluster_manager/cluster.rst | 4 +- docs/configuration/http_conn_man/rds.rst | 4 +- .../http_conn_man/route_config/vhost.rst | 5 +- docs/configuration/listeners/listeners.rst | 3 + docs/operations/cli.rst | 2 + source/common/config/rds_json.cc | 5 +- source/common/config/utility.cc | 5 +- test/common/config/BUILD | 3 + test/common/config/utility_test.cc | 67 ++++++++++++++----- 9 files changed, 76 insertions(+), 22 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index 9448aa82caa8..eb83e23739fb 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -31,7 +31,9 @@ Cluster name *(required, string)* Supplies the name of the cluster which must be unique across all clusters. The cluster name is used when emitting :ref:`statistics `. - The cluster name can be at most 60 characters long. + By default, the maximum length of a cluster name is limited to 60 characters. This limit can be + increased by setting the :ref:`--max-obj-name-len ` command line + argument to the desired value. .. _config_cluster_manager_type: diff --git a/docs/configuration/http_conn_man/rds.rst b/docs/configuration/http_conn_man/rds.rst index a8e3205250b6..06bd9ad60956 100644 --- a/docs/configuration/http_conn_man/rds.rst +++ b/docs/configuration/http_conn_man/rds.rst @@ -28,7 +28,9 @@ route_config_name *(required, string)* The name of the route configuration. This name will be passed to the :ref:`RDS HTTP API `. This allows an Envoy configuration with multiple HTTP listeners (and associated HTTP connection manager filters) to use different route - configurations. + configurations. By default, the maximum length of the name is limited to 60 characters. This + limit can be increased by setting the :ref:`--max-obj-name-len ` + command line argument to the desired value. refresh_delay_ms *(optional, integer)* The delay, in milliseconds, between fetches to the RDS API. Envoy will add diff --git a/docs/configuration/http_conn_man/route_config/vhost.rst b/docs/configuration/http_conn_man/route_config/vhost.rst index f019d9ce5fd4..b5a2591016f1 100644 --- a/docs/configuration/http_conn_man/route_config/vhost.rst +++ b/docs/configuration/http_conn_man/route_config/vhost.rst @@ -23,7 +23,10 @@ upstream cluster to route to or whether to perform a redirect. name *(required, string)* The logical name of the virtual host. This is used when emitting certain - statistics but is not relevant for forwarding. + statistics but is not relevant for forwarding. By default, the maximum length of the name is + limited to 60 characters. This limit can be increased by setting the + :ref:`--max-obj-name-len ` command line argument to the desired + value. domains *(required, array)* A list of domains (host/authority header) that will be matched to this diff --git a/docs/configuration/listeners/listeners.rst b/docs/configuration/listeners/listeners.rst index 9170a0ea0007..16bc1b3d2406 100644 --- a/docs/configuration/listeners/listeners.rst +++ b/docs/configuration/listeners/listeners.rst @@ -34,6 +34,9 @@ name *(optional, string)* The unique name by which this listener is known. If no name is provided, Envoy will allocate an internal UUID for the listener. If the listener is to be dynamically updated or removed via :ref:`LDS ` a unique name must be provided. + By default, the maximum length of a listener's name is limited to 60 characters. This limit can be + increased by setting the :ref:`--max-obj-name-len ` command line + argument to the desired value. address *(required, string)* The address that the listener should listen on. Currently only TCP diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst index 297feff8318a..3ae741047f19 100644 --- a/docs/operations/cli.rst +++ b/docs/operations/cli.rst @@ -117,6 +117,8 @@ following are the command line options that Envoy supports. during a hot restart. See the :ref:`hot restart overview ` for more information. Defaults to 900 seconds (15 minutes). +.. _operations_cli_max_obj_name_len: + .. option:: --max-obj-name-len *(optional)* The maximum name length (in bytes) of the name field in a cluster/route_config/listener. diff --git a/source/common/config/rds_json.cc b/source/common/config/rds_json.cc index feddd798ffe9..14923be19457 100644 --- a/source/common/config/rds_json.cc +++ b/source/common/config/rds_json.cc @@ -130,7 +130,10 @@ void RdsJson::translateRouteConfiguration(const Json::Object& json_route_config, void RdsJson::translateVirtualHost(const Json::Object& json_virtual_host, envoy::api::v2::VirtualHost& virtual_host) { json_virtual_host.validateSchema(Json::Schema::VIRTUAL_HOST_CONFIGURATION_SCHEMA); - JSON_UTIL_SET_STRING(json_virtual_host, virtual_host, name); + + const std::string name = json_virtual_host.getString("name", ""); + Utility::checkObjNameLength("Invalid virtual host name", name); + virtual_host.set_name(name); for (const std::string& domain : json_virtual_host.getStringArray("domains", true)) { virtual_host.add_domains(domain); diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index b72043fe3edb..c8736f9ab88e 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -89,12 +89,13 @@ void Utility::translateRdsConfig(const Json::Object& json_rds, envoy::api::v2::f json_rds.validateSchema(Json::Schema::RDS_CONFIGURATION_SCHEMA); const std::string name = json_rds.getString("route_config_name", ""); - checkObjNameLength("Invalid route_config_name", name); + checkObjNameLength("Invalid route_config name", name); + rds.set_route_config_name(name); + translateApiConfigSource(json_rds.getString("cluster"), json_rds.getInteger("refresh_delay_ms", 30000), json_rds.getString("api_type", ApiType::get().RestLegacy), *rds.mutable_config_source()->mutable_api_config_source()); - rds.set_route_config_name(name); } void Utility::translateLdsConfig(const Json::Object& json_lds, diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 870b54987d0a..92edfd0cf468 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -152,8 +152,11 @@ envoy_cc_test( srcs = ["utility_test.cc"], external_deps = ["envoy_eds"], deps = [ + "//source/common/config:cds_json_lib", "//source/common/config:lds_json_lib", + "//source/common/config:rds_json_lib", "//source/common/config:utility_lib", + "//source/common/stats:stats_lib", "//test/mocks/local_info:local_info_mocks", ], ) diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 5da5b1f3088c..7c50186ab1a6 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -1,10 +1,14 @@ +#include "common/config/cds_json.h" #include "common/config/lds_json.h" +#include "common/config/rds_json.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" +#include "common/stats/stats_impl.h" #include "test/mocks/local_info/mocks.h" #include "api/eds.pb.h" +#include "fmt/format.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -71,23 +75,54 @@ TEST(UtilityTest, TranslateApiConfigSource) { } TEST(UtilityTest, ObjNameLength) { - EXPECT_THROW(Utility::checkObjNameLength( - "test", "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema"), - EnvoyException); -} -TEST(LdsJsonTest, TranslateListenerWithLongName) { - std::string lds_json = R"EOF( - { - "name": "listenerwithareallyreallylongnamemorethanmaxcharsallowedbyschema", - "address": "tcp://0.0.0.0:1", - "filters": [] - } - )EOF"; - - envoy::api::v2::Listener listener; - auto json_object_ptr = Json::Factory::loadFromString(lds_json); - EXPECT_THROW(Config::LdsJson::translateListener(*json_object_ptr, listener), EnvoyException); + std::string name = "listenerwithareallyreallylongnamemorethanmaxcharsallowedbyschema"; + std::string err_prefix; + std::string err_suffix = fmt::format(": Length of {} ({}) exceeds allowed maximum length ({})", + name, name.length(), Stats::RawStatData::maxObjNameLength()); + { + err_prefix = "test"; + EXPECT_THROW_WITH_MESSAGE(Utility::checkObjNameLength(err_prefix, name), EnvoyException, + err_prefix + err_suffix); + } + + { + err_prefix = "Invalid listener name"; + std::string json = "{ 'name': " + name + "'address': 'foo', 'filters': [] }"; + auto json_object_ptr = Json::Factory::loadFromString(json); + + envoy::api::v2::Listener listener; + EXPECT_THROW_WITH_MESSAGE(Config::LdsJson::translateListener(*json_object_ptr, listener), + EnvoyException, err_prefix + err_suffix); + } + + { + err_prefix = "Invalid virtual host name"; + std::string json = "{ 'name': " + name + "'domains': [], 'routes': [] }"; + auto json_object_ptr = Json::Factory::loadFromString(json); + envoy::api::v2::VirtualHost vhost; + EXPECT_THROW_WITH_MESSAGE(Config::RdsJson::translateVirtualHost(*json_object_ptr, vhost), + EnvoyException, err_prefix + err_suffix); + } + + { + err_prefix = "Invalid cluster name"; + std::string json = + "{ 'name': " + name + "'type': 'static', 'lb_type': 'random', 'connect_timeout_ms' : 1}"; + auto json_object_ptr = Json::Factory::loadFromString(json); + envoy::api::v2::Cluster cluster; + EXPECT_THROW_WITH_MESSAGE(Config::CdsJson::translateCluster(*json_object_ptr, cluster), + EnvoyException, err_prefix + err_suffix); + } + + { + err_prefix = "Invalid route_config name"; + std::string json = "{ 'route_config_name': " + name + "'cluster': 'foo'}"; + auto json_object_ptr = Json::Factory::loadFromString(json); + envoy::api::v2::filter::Rds rds; + EXPECT_THROW_WITH_MESSAGE(Config::Utility::translateRdsConfig(*json_object_ptr, rds), + EnvoyException, err_prefix + err_suffix); + } } } // namespace Config From fa2d91b4cd0a8ea74b61b94c666ae5e9feef9087 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Oct 2017 12:13:47 -0400 Subject: [PATCH 29/44] review comments Signed-off-by: Shriram Rajagopalan --- source/common/config/BUILD | 1 + source/common/config/rds_json.cc | 1 + source/common/stats/stats_impl.cc | 12 ++++----- source/common/stats/stats_impl.h | 25 +++++++++++++------ source/server/options_impl.cc | 2 ++ test/common/config/BUILD | 1 + test/common/config/utility_test.cc | 17 ++++++++----- .../upstream/cluster_manager_impl_test.cc | 13 ---------- 8 files changed, 40 insertions(+), 32 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 4f6ef5513282..1317dc1171d1 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -232,6 +232,7 @@ envoy_cc_library( ":well_known_names", "//include/envoy/json:json_object_interface", "//source/common/common:assert_lib", + "//source/common/config:utility_lib", "//source/common/json:config_schemas_lib", ], ) diff --git a/source/common/config/rds_json.cc b/source/common/config/rds_json.cc index 14923be19457..fa7960449653 100644 --- a/source/common/config/rds_json.cc +++ b/source/common/config/rds_json.cc @@ -5,6 +5,7 @@ #include "common/config/base_json.h" #include "common/config/json_utility.h" #include "common/config/metadata.h" +#include "common/config/utility.h" #include "common/config/well_known_names.h" #include "common/json/config_schemas.h" diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index bfc6d1e382b3..1c8eaf73fa29 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -30,25 +30,25 @@ size_t RawStatData::size() { return roundUpMultipleNaturalAlignment(sizeof(RawStatData) + nameSize()); } -size_t& RawStatData::initializeAndGetMutableMaxNameLength(size_t configured_size) { +size_t& RawStatData::initializeAndGetMutableMaxObjNameLength(size_t configured_size) { // Like CONSTRUCT_ON_FIRST_USE, but non-const so that the value can be changed by tests static size_t size = configured_size; return size; } void RawStatData::configure(Server::Options& options) { - const size_t configured = options.maxObjNameLength() + maxStatSuffixLength(); + const size_t configured = options.maxObjNameLength(); RELEASE_ASSERT(configured > 0); - size_t max_name_length = initializeAndGetMutableMaxNameLength(configured); + size_t max_obj_name_length = initializeAndGetMutableMaxObjNameLength(configured); // If this fails, it means that this function was called too late during // startup because things were already using this size before it was set. - RELEASE_ASSERT(max_name_length == configured); + RELEASE_ASSERT(max_obj_name_length == configured); } void RawStatData::configureForTestsOnly(Server::Options& options) { - const size_t configured = options.maxObjNameLength() + maxStatSuffixLength(); - initializeAndGetMutableMaxNameLength(configured) = configured; + const size_t configured = options.maxObjNameLength(); + initializeAndGetMutableMaxObjNameLength(configured) = configured; } std::string Utility::sanitizeStatsName(const std::string& name) { diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 7736fecaabb2..456d60d26cee 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -63,15 +63,15 @@ struct RawStatData { * Returns the maximum length of the name of a stat. This length * does not include a trailing NULL-terminator. */ - static size_t maxNameLength() { - return initializeAndGetMutableMaxNameLength(DEFAULT_MAX_NAME_SIZE); - } + static size_t maxNameLength() { return maxObjNameLength() + MAX_STAT_SUFFIX_LENGTH; } /** * Returns the maximum length of a user supplied object (route/cluster/listener) * name field in a stat. This length does not include a trailing NULL-terminator. */ - static size_t maxObjNameLength() { return maxNameLength() - MAX_STAT_SUFFIX_LENGTH; } + static size_t maxObjNameLength() { + return initializeAndGetMutableMaxObjNameLength(DEFAULT_MAX_OBJ_NAME_LENGTH); + } /** * Returns the maximum length of a stat suffix that Envoy generates (over the user supplied name). @@ -114,11 +114,22 @@ struct RawStatData { char name_[]; private: - static const size_t DEFAULT_MAX_NAME_SIZE = 127; - // total length must be equal to DEFAULT_MAX_NAME_SIZE + // The max name length is based on current set of stats. + // As of now, the longest stat is + // cluster..outlier_detection.ejections_consecutive_5xx + // which is 52 characters long without the cluster name. + // The max stat name length is 127 (default). So, in order to give room + // for growth to both the envoy generated stat characters + // (e.g., outlier_detection...) and user supplied names (e.g., cluster name), + // we set the max user supplied name length to 60, and the max internally + // generated stat suffixes to 67 (15 more characters to grow). + // If you want to increase the max user supplied name length, use the compiler + // option ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH or the CLI option + // max-obj-name-len + static const size_t DEFAULT_MAX_OBJ_NAME_LENGTH = 60; static const size_t MAX_STAT_SUFFIX_LENGTH = 67; - static size_t& initializeAndGetMutableMaxNameLength(size_t configured_size); + static size_t& initializeAndGetMutableMaxObjNameLength(size_t configured_size); }; /** diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index df4818da2be6..01ce1f60f224 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -19,6 +19,8 @@ #endif // Can be overridden at compile time +// See comment in common/stat/stat_impl.h for rationale behind +// this constant. #ifndef ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH #define ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH 60 #endif diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 92edfd0cf468..ef9d50962fa9 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -158,5 +158,6 @@ envoy_cc_test( "//source/common/config:utility_lib", "//source/common/stats:stats_lib", "//test/mocks/local_info:local_info_mocks", + "//test/test_common:utility_lib", ], ) diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 7c50186ab1a6..cefedba66dcc 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -6,6 +6,7 @@ #include "common/stats/stats_impl.h" #include "test/mocks/local_info/mocks.h" +#include "test/test_common/utility.h" #include "api/eds.pb.h" #include "fmt/format.h" @@ -88,7 +89,8 @@ TEST(UtilityTest, ObjNameLength) { { err_prefix = "Invalid listener name"; - std::string json = "{ 'name': " + name + "'address': 'foo', 'filters': [] }"; + std::string json = + R"EOF({ "name": ")EOF" + name + R"EOF(", "address": "foo", "filters":[]})EOF"; auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::Listener listener; @@ -98,7 +100,7 @@ TEST(UtilityTest, ObjNameLength) { { err_prefix = "Invalid virtual host name"; - std::string json = "{ 'name': " + name + "'domains': [], 'routes': [] }"; + std::string json = R"EOF({ "name": ")EOF" + name + R"EOF(", "domains": [], "routes": []})EOF"; auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::VirtualHost vhost; EXPECT_THROW_WITH_MESSAGE(Config::RdsJson::translateVirtualHost(*json_object_ptr, vhost), @@ -108,16 +110,19 @@ TEST(UtilityTest, ObjNameLength) { { err_prefix = "Invalid cluster name"; std::string json = - "{ 'name': " + name + "'type': 'static', 'lb_type': 'random', 'connect_timeout_ms' : 1}"; + R"EOF({ "name": ")EOF" + name + + R"EOF(", "type": "static", "lb_type": "random", "connect_timeout_ms" : 1})EOF"; auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::Cluster cluster; - EXPECT_THROW_WITH_MESSAGE(Config::CdsJson::translateCluster(*json_object_ptr, cluster), - EnvoyException, err_prefix + err_suffix); + envoy::api::v2::ConfigSource eds_config; + EXPECT_THROW_WITH_MESSAGE( + Config::CdsJson::translateCluster(*json_object_ptr, eds_config, cluster), EnvoyException, + err_prefix + err_suffix); } { err_prefix = "Invalid route_config name"; - std::string json = "{ 'route_config_name': " + name + "'cluster': 'foo'}"; + std::string json = R"EOF({ "route_config_name": ")EOF" + name + R"EOF(", "cluster": "foo"})EOF"; auto json_object_ptr = Json::Factory::loadFromString(json); envoy::api::v2::filter::Rds rds; EXPECT_THROW_WITH_MESSAGE(Config::Utility::translateRdsConfig(*json_object_ptr, rds), diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 9f9ef9faca62..185947c24be6 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -232,19 +232,6 @@ TEST_F(ClusterManagerImplTest, UnknownHcType) { EXPECT_THROW(create(parseBootstrapFromJson(json)), EnvoyException); } -TEST_F(ClusterManagerImplTest, MaxClusterName) { - const std::string json = R"EOF( - { - "clusters": [ - { - "name": "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema" - }] - } - )EOF"; - - EXPECT_THROW(create(parseBootstrapFromJson(json)), EnvoyException); -} - TEST_F(ClusterManagerImplTest, ValidClusterName) { const std::string json = R"EOF( { From 9ec016cd8568a769bce4dc8e95f4456f37539c77 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Wed, 18 Oct 2017 13:46:43 -0400 Subject: [PATCH 30/44] nits Signed-off-by: Shriram Rajagopalan --- bazel/README.md | 2 +- .../configuration/cluster_manager/cluster.rst | 3 +-- docs/configuration/http_conn_man/rds.rst | 4 ++-- .../http_conn_man/route_config/vhost.rst | 3 +-- docs/configuration/listeners/listeners.rst | 3 +-- docs/operations/cli.rst | 2 -- test/common/router/rds_impl_test.cc | 21 ------------------- 7 files changed, 6 insertions(+), 32 deletions(-) diff --git a/bazel/README.md b/bazel/README.md index ecb40342f88d..c99d8db4a452 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -246,7 +246,7 @@ on the Bazel command line. ## Stats Tunables The default maximum number of stats in shared memory, and the default -maximum length of a cluster/ route config/listener name, can be +maximum length of a cluster/route config/listener name, can be overriden at compile-time by defining `ENVOY_DEFAULT_MAX_STATS` and `ENVOY_DEFAULT_MAX_OBJ_NAME_LENGTH`, respectively, to the desired value. For example: diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index eb83e23739fb..1da992ffb3b1 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -32,8 +32,7 @@ name *(required, string)* Supplies the name of the cluster which must be unique across all clusters. The cluster name is used when emitting :ref:`statistics `. By default, the maximum length of a cluster name is limited to 60 characters. This limit can be - increased by setting the :ref:`--max-obj-name-len ` command line - argument to the desired value. + increased by setting the :option:`--max-obj-name-len` command line argument to the desired value. .. _config_cluster_manager_type: diff --git a/docs/configuration/http_conn_man/rds.rst b/docs/configuration/http_conn_man/rds.rst index 06bd9ad60956..fea548926cda 100644 --- a/docs/configuration/http_conn_man/rds.rst +++ b/docs/configuration/http_conn_man/rds.rst @@ -29,8 +29,8 @@ route_config_name :ref:`RDS HTTP API `. This allows an Envoy configuration with multiple HTTP listeners (and associated HTTP connection manager filters) to use different route configurations. By default, the maximum length of the name is limited to 60 characters. This - limit can be increased by setting the :ref:`--max-obj-name-len ` - command line argument to the desired value. + limit can be increased by setting the :option:`--max-obj-name-len` command line argument to the + desired value. refresh_delay_ms *(optional, integer)* The delay, in milliseconds, between fetches to the RDS API. Envoy will add diff --git a/docs/configuration/http_conn_man/route_config/vhost.rst b/docs/configuration/http_conn_man/route_config/vhost.rst index b5a2591016f1..32064487e778 100644 --- a/docs/configuration/http_conn_man/route_config/vhost.rst +++ b/docs/configuration/http_conn_man/route_config/vhost.rst @@ -25,8 +25,7 @@ name *(required, string)* The logical name of the virtual host. This is used when emitting certain statistics but is not relevant for forwarding. By default, the maximum length of the name is limited to 60 characters. This limit can be increased by setting the - :ref:`--max-obj-name-len ` command line argument to the desired - value. + :option:`--max-obj-name-len` command line argument to the desired value. domains *(required, array)* A list of domains (host/authority header) that will be matched to this diff --git a/docs/configuration/listeners/listeners.rst b/docs/configuration/listeners/listeners.rst index 16bc1b3d2406..595112313335 100644 --- a/docs/configuration/listeners/listeners.rst +++ b/docs/configuration/listeners/listeners.rst @@ -35,8 +35,7 @@ name Envoy will allocate an internal UUID for the listener. If the listener is to be dynamically updated or removed via :ref:`LDS ` a unique name must be provided. By default, the maximum length of a listener's name is limited to 60 characters. This limit can be - increased by setting the :ref:`--max-obj-name-len ` command line - argument to the desired value. + increased by setting the :option:`--max-obj-name-len` command line argument to the desired value. address *(required, string)* The address that the listener should listen on. Currently only TCP diff --git a/docs/operations/cli.rst b/docs/operations/cli.rst index 3ae741047f19..297feff8318a 100644 --- a/docs/operations/cli.rst +++ b/docs/operations/cli.rst @@ -117,8 +117,6 @@ following are the command line options that Envoy supports. during a hot restart. See the :ref:`hot restart overview ` for more information. Defaults to 900 seconds (15 minutes). -.. _operations_cli_max_obj_name_len: - .. option:: --max-obj-name-len *(optional)* The maximum name length (in bytes) of the name field in a cluster/route_config/listener. diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 899b0cfd5d30..7ca441809cce 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -182,27 +182,6 @@ TEST_F(RdsImplTest, UnknownCluster) { EnvoyException); } -TEST_F(RdsImplTest, MaxRouteConfigName) { - const std::string config_json = R"EOF( - { - "rds": { - "cluster": "foo_cluster", - "route_config_name": "clusterwithareallyreallylongnamemorethanmaxcharsallowedbyschema" - }, - "codec_type": "auto", - "stat_prefix": "foo", - "filters": [ - { "name": "http_dynamo_filter", "config": {} } - ] - } - )EOF"; - - EXPECT_THROW(RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json), - runtime_, cm_, store_, "foo.", init_manager_, - *route_config_provider_manager_), - EnvoyException); -} - TEST_F(RdsImplTest, DestroyDuringInitialize) { InSequence s; From 70633647cd9e7d77913863978d11a6f2a86a32df Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 01:02:08 -0500 Subject: [PATCH 31/44] buffer filter v2 Signed-off-by: Shriram Rajagopalan --- source/common/config/BUILD | 1 + source/common/config/filter_json.cc | 8 +++++++ source/common/config/filter_json.h | 10 ++++++++ source/server/config/http/BUILD | 3 ++- source/server/config/http/buffer.cc | 31 ++++++++++++++++++------ source/server/config/http/buffer.h | 14 +++++++++++ test/server/config/http/config_test.cc | 33 ++++++++++++++++++++++++-- 7 files changed, 90 insertions(+), 10 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 578a4044a0a5..47d515d5dfb8 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -89,6 +89,7 @@ envoy_cc_library( hdrs = ["filter_json.h"], external_deps = [ "envoy_filter_http_http_connection_manager", + "envoy_filter_http_buffer", "envoy_filter_http_fault", "envoy_filter_http_router", "envoy_filter_network_mongo_proxy", diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index 4bafa0d7c411..a4a1550270e6 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -274,5 +274,13 @@ void FilterJson::translateRouter(const Json::Object& json_router, router.set_start_child_span(json_router.getBoolean("start_child_span", false)); } +void FilterJson::translateBufferFilter(const Json::Object& json_buffer, + envoy::api::v2::filter::http::Buffer& buffer) { + json_buffer.validateSchema(Json::Schema::BUFFER_HTTP_FILTER_SCHEMA); + + buffer.mutable_max_request_bytes()->set_value(json_buffer.getInteger("max_request_bytes")); + JSON_UTIL_SET_DURATION_SECONDS(json_buffer, buffer, max_request_time); +} + } // namespace Config } // namespace Envoy diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index b96fd8c916de..1b454e315c9d 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -65,6 +65,16 @@ class FilterJson { */ static void translateRouter(const Json::Object& json_router, envoy::api::v2::filter::http::Router& router); + + /** + * Translate a v1 JSON Buffer filter object to v2 envoy::api::v2::filter::http::Buffer. + * @param config source v1 JSON HTTP Buffer Filter object. + * @param buffer destination v2 + * envoy::api::v2::filter::http::Buffer. + */ + static void translateBufferFilter(const Json::Object& config, + envoy::api::v2::filter::http::HTTPFault& fault); + }; } // namespace Config diff --git a/source/server/config/http/BUILD b/source/server/config/http/BUILD index 5cd59f86509c..cabd33a79bb5 100644 --- a/source/server/config/http/BUILD +++ b/source/server/config/http/BUILD @@ -15,9 +15,10 @@ envoy_cc_library( deps = [ "//include/envoy/registry", "//include/envoy/server:filter_config_interface", + "//source/common/config:filter_json_lib", "//source/common/config:well_known_names", "//source/common/http/filter:buffer_filter_lib", - "//source/common/json:config_schemas_lib", + "//source/common/protobuf:utility_lib", ], ) diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index 78facea48e6f..02d55df9ed54 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -6,28 +6,45 @@ #include "envoy/registry/registry.h" +#include "common/config/filter_json.h" #include "common/http/filter/buffer_filter.h" -#include "common/json/config_schemas.h" +#include "common/protobuf/utility.h" namespace Envoy { namespace Server { namespace Configuration { -HttpFilterFactoryCb BufferFilterConfig::createFilterFactory(const Json::Object& json_config, - const std::string& stats_prefix, - FactoryContext& context) { - json_config.validateSchema(Json::Schema::BUFFER_HTTP_FILTER_SCHEMA); +HttpFilterFactoryCb +BufferFilterConfig::createBufferFilter(const envoy::api::v2::filter::http::Buffer& buffer, + const std::string& stats_prefix, FactoryContext& context) { + ASSERT(buffer.has_max_request_bytes()); + ASSERT(buffer.has_max_request_time()); Http::BufferFilterConfigConstSharedPtr config(new Http::BufferFilterConfig{ Http::BufferFilter::generateStats(stats_prefix, context.scope()), - static_cast(json_config.getInteger("max_request_bytes")), - std::chrono::seconds(json_config.getInteger("max_request_time_s"))}); + static_cast(buffer.max_request_bytes()), + std::chrono::seconds(PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time)/1000)}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter( Http::StreamDecoderFilterSharedPtr{new Http::BufferFilter(config)}); }; } +HttpFilterFactoryCb BufferFilterConfig::createFilterFactory(const Json::Object& json_config, + const std::string& stats_prefix, + FactoryContext& context) { + envoy::api::v2::filter::http::Buffer buffer; + Config::FilterJson::translateBufferFilter(json_config, buffer); + return createBufferFilter(buffer, stats_prefix, context); +} + +HttpFilterFactoryCb BufferFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& config, + const std::string& stats_prefix, + FactoryContext& context) { + return createBufferFilter(dynamic_cast(config), + stats_prefix, context); +} + /** * Static registration for the buffer filter. @see RegisterFactory. */ diff --git a/source/server/config/http/buffer.h b/source/server/config/http/buffer.h index 1250d7bf2011..51f8b1372e9b 100644 --- a/source/server/config/http/buffer.h +++ b/source/server/config/http/buffer.h @@ -6,6 +6,8 @@ #include "common/config/well_known_names.h" +#include "api/filter/http/buffer.pb.h" + namespace Envoy { namespace Server { namespace Configuration { @@ -18,7 +20,19 @@ class BufferFilterConfig : public NamedHttpFilterConfigFactory { HttpFilterFactoryCb createFilterFactory(const Json::Object& json_config, const std::string& stats_prefix, FactoryContext& context) override; + HttpFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config, + const std::string& stats_prefix, + FactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::api::v2::filter::http::Buffer()}; + } + std::string name() override { return Config::HttpFilterNames::get().BUFFER; } + +private: + HttpFilterFactoryCb createBufferFilter(const envoy::api::v2::filter::http::Buffer& buffer, + const std::string& stats_prefix, FactoryContext& context); }; } // namespace Configuration diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index 3cfdc782ce96..a2ab618bbbd4 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -37,7 +37,7 @@ namespace Envoy { namespace Server { namespace Configuration { -TEST(HttpFilterConfigTest, BufferFilter) { +TEST(HttpFilterConfigTest, CorrectBufferFilterInJson) { std::string json_string = R"EOF( { "max_request_bytes" : 1028, @@ -54,7 +54,7 @@ TEST(HttpFilterConfigTest, BufferFilter) { cb(filter_callback); } -TEST(HttpFilterConfigTest, BadBufferFilterConfig) { +TEST(HttpFilterConfigTest, BadBufferFilterConfigInJson) { std::string json_string = R"EOF( { "max_request_bytes" : 1028, @@ -68,6 +68,35 @@ TEST(HttpFilterConfigTest, BadBufferFilterConfig) { EXPECT_THROW(factory.createFilterFactory(*json_config, "stats", context), Json::Exception); } +TEST(HttpFilterConfigTest, CorrectBufferFilterInProto) { + envoy::api::v2::filter::http::Buffer config{}; + config.mutable_max_request_bytes()->set_value(1028); + config.mutable_max_request_time()->set_seconds(2); + + NiceMock context; + BufferFilterConfig factory; + HttpFilterFactoryCb cb = factory.createFilterFactoryFromProto(config, "stats", context); + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + cb(filter_callback); +} + +TEST(HttpFilterConfigTest, BufferFilterWithEmptyProto) { + BufferFilterConfig factory; + envoy::api::v2::filter::http::Buffer config = + *dynamic_cast( + factory.createEmptyConfigProto().get()); + + config.mutable_max_request_bytes()->set_value(1028); + config.mutable_max_request_time()->set_seconds(2); + + NiceMock context; + HttpFilterFactoryCb cb = factory.createFilterFactoryFromProto(config, "stats", context); + Http::MockFilterChainFactoryCallbacks filter_callback; + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + cb(filter_callback); +} + TEST(HttpFilterConfigTest, RateLimitFilter) { std::string json_string = R"EOF( { From 247518fd83b78c24d7b26faa32fd02210f5612d6 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 01:07:50 -0500 Subject: [PATCH 32/44] format Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.h | 1 - source/server/config/http/buffer.cc | 13 ++++++------- test/server/config/http/config_test.cc | 3 +-- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index 1b454e315c9d..e728ec14e915 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -74,7 +74,6 @@ class FilterJson { */ static void translateBufferFilter(const Json::Object& config, envoy::api::v2::filter::http::HTTPFault& fault); - }; } // namespace Config diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index 02d55df9ed54..aa738570fb9f 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -16,14 +16,14 @@ namespace Configuration { HttpFilterFactoryCb BufferFilterConfig::createBufferFilter(const envoy::api::v2::filter::http::Buffer& buffer, - const std::string& stats_prefix, FactoryContext& context) { + const std::string& stats_prefix, FactoryContext& context) { ASSERT(buffer.has_max_request_bytes()); ASSERT(buffer.has_max_request_time()); Http::BufferFilterConfigConstSharedPtr config(new Http::BufferFilterConfig{ Http::BufferFilter::generateStats(stats_prefix, context.scope()), - static_cast(buffer.max_request_bytes()), - std::chrono::seconds(PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time)/1000)}); + static_cast(buffer.max_request_bytes()), + std::chrono::seconds(PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time) / 1000)}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter( Http::StreamDecoderFilterSharedPtr{new Http::BufferFilter(config)}); @@ -38,11 +38,10 @@ HttpFilterFactoryCb BufferFilterConfig::createFilterFactory(const Json::Object& return createBufferFilter(buffer, stats_prefix, context); } -HttpFilterFactoryCb BufferFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& config, - const std::string& stats_prefix, - FactoryContext& context) { +HttpFilterFactoryCb BufferFilterConfig::createFilterFactoryFromProto( + const Protobuf::Message& config, const std::string& stats_prefix, FactoryContext& context) { return createBufferFilter(dynamic_cast(config), - stats_prefix, context); + stats_prefix, context); } /** diff --git a/test/server/config/http/config_test.cc b/test/server/config/http/config_test.cc index a2ab618bbbd4..673fc4c1b2d5 100644 --- a/test/server/config/http/config_test.cc +++ b/test/server/config/http/config_test.cc @@ -84,8 +84,7 @@ TEST(HttpFilterConfigTest, CorrectBufferFilterInProto) { TEST(HttpFilterConfigTest, BufferFilterWithEmptyProto) { BufferFilterConfig factory; envoy::api::v2::filter::http::Buffer config = - *dynamic_cast( - factory.createEmptyConfigProto().get()); + *dynamic_cast(factory.createEmptyConfigProto().get()); config.mutable_max_request_bytes()->set_value(1028); config.mutable_max_request_time()->set_seconds(2); From df3969333be84caf2d26ed8e1e9b305424027ac5 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 01:10:12 -0500 Subject: [PATCH 33/44] includes Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index e728ec14e915..3c60bcc4e299 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -2,6 +2,7 @@ #include "envoy/json/json_object.h" +#include "api/filter/http/buffer.pb.h" #include "api/filter/http/fault.pb.h" #include "api/filter/http/http_connection_manager.pb.h" #include "api/filter/http/router.pb.h" From ef80354d9453df24b4993515358908e19a7acc5a Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 01:31:18 -0500 Subject: [PATCH 34/44] compile fix Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.h | 4 ++-- source/server/config/http/buffer.cc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index 3c60bcc4e299..88499c31738e 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -73,8 +73,8 @@ class FilterJson { * @param buffer destination v2 * envoy::api::v2::filter::http::Buffer. */ - static void translateBufferFilter(const Json::Object& config, - envoy::api::v2::filter::http::HTTPFault& fault); + static void translateBufferFilter(const Json::Object& json_buffer, + envoy::api::v2::filter::http::Buffer& buffer); }; } // namespace Config diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index aa738570fb9f..b81cb30d75e9 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -22,7 +22,7 @@ BufferFilterConfig::createBufferFilter(const envoy::api::v2::filter::http::Buffe Http::BufferFilterConfigConstSharedPtr config(new Http::BufferFilterConfig{ Http::BufferFilter::generateStats(stats_prefix, context.scope()), - static_cast(buffer.max_request_bytes()), + static_cast(buffer.max_request_bytes().value()), std::chrono::seconds(PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time) / 1000)}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter( From 170140a9fec6286a938a85bf440269efcbdd49b8 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 11:27:34 -0500 Subject: [PATCH 35/44] health check filter - v2 Signed-off-by: Shriram Rajagopalan --- source/common/config/BUILD | 1 + source/common/config/filter_json.cc | 11 +++++ source/common/config/filter_json.h | 10 +++++ source/server/http/BUILD | 6 ++- source/server/http/health_check.cc | 60 +++++++++++++++++++++------ source/server/http/health_check.h | 26 ++++++++++-- test/server/http/health_check_test.cc | 40 +++++++++++++++++- 7 files changed, 133 insertions(+), 21 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 578a4044a0a5..4e41d4595a4e 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -90,6 +90,7 @@ envoy_cc_library( external_deps = [ "envoy_filter_http_http_connection_manager", "envoy_filter_http_fault", + "envoy_filter_http_health_check", "envoy_filter_http_router", "envoy_filter_network_mongo_proxy", ], diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index 4bafa0d7c411..78b42355f06d 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -266,6 +266,17 @@ void FilterJson::translateFaultFilter(const Json::Object& config, } } +void FilterJson::translateHealthCheckFilter( + const Json::Object& json_health_check, + envoy::api::v2::filter::http::HealthCheck& health_check) { + json_health_check.validateSchema(Json::Schema::HEALTH_CHECK_HTTP_FILTER_SCHEMA); + + health_check.mutable_pass_through_mode()->set_value( + json_health_check.getBoolean("pass_through_mode")); + JSON_UTIL_SET_DURATION_FROM_FIELD(json_health_check, health_check, cache_time); + JSON_UTIL_SET_STRING(json_health_check, health_check, endpoint); +} + void FilterJson::translateRouter(const Json::Object& json_router, envoy::api::v2::filter::http::Router& router) { json_router.validateSchema(Json::Schema::ROUTER_HTTP_FILTER_SCHEMA); diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index b96fd8c916de..c41386f4021e 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -3,6 +3,7 @@ #include "envoy/json/json_object.h" #include "api/filter/http/fault.pb.h" +#include "api/filter/http/health_check.pb.h" #include "api/filter/http/http_connection_manager.pb.h" #include "api/filter/http/router.pb.h" #include "api/filter/network/mongo_proxy.pb.h" @@ -58,6 +59,15 @@ class FilterJson { static void translateFaultFilter(const Json::Object& config, envoy::api::v2::filter::http::HTTPFault& fault); + /** + * Translate a v1 JSON Health Check filter object to v2 envoy::api::v2::filter::http::HealthCheck. + * @param config source v1 JSON Health Check Filter object. + * @param health_check destination v2 + * envoy::api::v2::filter::http::HealthCheck. + */ + static void translateHealthCheckFilter(const Json::Object& config, + envoy::api::v2::filter::http::HealthCheck& health_check); + /* * Translate a v1 JSON Router object to v2 envoy::api::v2::filter::http::Router. * @param json_router source v1 JSON HTTP router object. diff --git a/source/server/http/BUILD b/source/server/http/BUILD index b01bfae09eff..b902a5c168fa 100644 --- a/source/server/http/BUILD +++ b/source/server/http/BUILD @@ -52,6 +52,7 @@ envoy_cc_library( name = "health_check_lib", srcs = ["health_check.cc"], hdrs = ["health_check.h"], + external_deps = ["envoy_filter_http_health_check"], deps = [ "//include/envoy/event:dispatcher_interface", "//include/envoy/event:timer_interface", @@ -60,12 +61,13 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//source/common/common:assert_lib", "//source/common/common:enum_to_int", + "//source/common/config:filter_json_lib", + "//source/common/config:well_known_names", "//source/common/http:codes_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/http:utility_lib", - "//source/common/json:config_schemas_lib", - "//source/common/json:json_loader_lib", + "//source/common/protobuf:utility_lib", "//source/server/config/network:http_connection_manager_lib", ], ) diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 8c2df6b18484..cfc23b8fcd17 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -14,8 +14,7 @@ #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/utility.h" -#include "common/json/config_schemas.h" -#include "common/json/json_loader.h" +#include "common/protobuf/utility.h" #include "server/config/network/http_connection_manager.h" @@ -23,17 +22,15 @@ namespace Envoy { namespace Server { namespace Configuration { -/** - * Config registration for the health check filter. @see NamedHttpFilterConfigFactory. - */ -HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext& context) { - config.validateSchema(Json::Schema::HEALTH_CHECK_HTTP_FILTER_SCHEMA); +HttpFilterFactoryCb HealthCheckFilterConfig::createHealthCheckFilter( + const envoy::api::v2::filter::http::HealthCheck& health_check, const std::string& stats_prefix, + FactoryContext& context) { + ASSERT(health_check.has_pass_through_mode()); + ASSERT(!health_check.endpoint().empty()); - bool pass_through_mode = config.getBoolean("pass_through_mode"); - int64_t cache_time_ms = config.getInteger("cache_time_ms", 0); - std::string hc_endpoint = config.getString("endpoint"); + bool pass_through_mode = health_check.pass_through_mode().value(); + int64_t cache_time_ms = PROTOBUF_GET_MS_OR_DEFAULT(health_check, cache_time, 0); + std::string hc_endpoint = health_check.endpoint(); if (!pass_through_mode && cache_time_ms) { throw EnvoyException("cache_time_ms must not be set when path_through_mode is disabled"); @@ -52,13 +49,50 @@ HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Obj }; } +HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Object& json_config, + const std::string& stats_prefix, + FactoryContext& context) { + envoy::api::v2::filter::http::HealthCheck health_check; + Config::FilterJson::translateHealthCheckFilter(json_config, health_check); + return createHealthCheckFilter(health_check, stats_prefix, context); +} + +HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactoryFromProto( + const Protobuf::Message& config, const std::string& stats_prefix, FactoryContext& context) { + return createHealthCheckFilter( + dynamic_cast(config), stats_prefix, + context); +} + +/** + * Config registration for the health check filter. @see NamedHttpFilterConfigFactory. + */ +HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Object& config, + const std::string&, + FactoryContext& context) { + throw EnvoyException("cache_time_ms must not be set when path_through_mode is disabled"); +} + +HealthCheckCacheManagerSharedPtr cache_manager; +if (cache_time_ms > 0) { + cache_manager.reset( + new HealthCheckCacheManager(context.dispatcher(), std::chrono::milliseconds(cache_time_ms))); +} + +return [&context, pass_through_mode, cache_manager, + hc_endpoint](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(Http::StreamFilterSharedPtr{ + new HealthCheckFilter(context, pass_through_mode, cache_manager, hc_endpoint)}); +}; +} // namespace Configuration + /** * Static registration for the health check filter. @see RegisterFactory. */ static Registry::RegisterFactory register_; -} // namespace Configuration } // namespace Server +} // namespace Envoy HealthCheckCacheManager::HealthCheckCacheManager(Event::Dispatcher& dispatcher, std::chrono::milliseconds timeout) diff --git a/source/server/http/health_check.h b/source/server/http/health_check.h index d36e5ea097dd..de30f8f2f630 100644 --- a/source/server/http/health_check.h +++ b/source/server/http/health_check.h @@ -9,6 +9,10 @@ #include "envoy/http/filter.h" #include "envoy/server/filter_config.h" +#include "common/config/well_known_names.h" + +#include "api/filter/http/health_check.pb.h" + namespace Envoy { namespace Server { namespace Configuration { @@ -17,11 +21,25 @@ class HealthCheckFilterConfig : public NamedHttpFilterConfigFactory { public: HttpFilterFactoryCb createFilterFactory(const Json::Object& config, const std::string&, FactoryContext& context) override; - std::string name() override { return "envoy.health_check"; } -}; + HttpFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config, + const std::string& stats_prefix, + FactoryContext& context) override; + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new envoy::api::v2::filter::http::HealthCheck()}; + } + + std::string name() override { + return Config::HttpFilterNames::get().HEALTH_CHECK; + + private: + HttpFilterFactoryCb createHealthCheckFilter( + const envoy::api::v2::filter::http::HealthCheck& health_check, + const std::string& stats_prefix, FactoryContext& context); + }; } // namespace Configuration -} // namespace Server +} // namespace Configuration /** * Shared cache manager used by all instances of a health check filter configuration as well as @@ -94,4 +112,4 @@ class HealthCheckFilter : public Http::StreamFilter { HealthCheckCacheManagerSharedPtr cache_manager_{}; const std::string endpoint_; }; -} // namespace Envoy +} // namespace Server diff --git a/test/server/http/health_check_test.cc b/test/server/http/health_check_test.cc index 07c1a2acb6e4..aeeda7183470 100644 --- a/test/server/http/health_check_test.cc +++ b/test/server/http/health_check_test.cc @@ -208,7 +208,7 @@ TEST_F(HealthCheckFilterCachingTest, NotHcRequest) { filter_->decodeHeaders(request_headers_no_hc_, true)); } -TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSet) { +TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSetJson) { Server::Configuration::HealthCheckFilterConfig healthCheckFilterConfig; Json::ObjectSharedPtr config = Json::Factory::loadFromString( "{\"pass_through_mode\":false, \"cache_time_ms\":234, \"endpoint\":\"foo\"}"); @@ -218,7 +218,7 @@ TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSet) { EnvoyException); } -TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSet) { +TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSetJson) { Server::Configuration::HealthCheckFilterConfig healthCheckFilterConfig; Json::ObjectSharedPtr config = Json::Factory::loadFromString("{\"pass_through_mode\":false, \"endpoint\":\"foo\"}"); @@ -226,4 +226,40 @@ TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSet) { healthCheckFilterConfig.createFilterFactory(*config, "dummy_stats_prefix", context); } + +TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSetProto) { + Server::Configuration::HealthCheckFilterConfig healthCheckFilterConfig; + envoy::api::v2::filter::http::HealthCheck config{}; + NiceMock context; + + config.mutable_pass_through_mode()->set_value(false); + config.mutable_endpoint()->set_value("foo"); + config.mutable_cache_time()->set_seconds(10); + + EXPECT_THROW( + healthCheckFilterConfig.createFilterFactoryFromProto(config, "dummy_stats_prefix", context), + EnvoyException); +} + +TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSetProto) { + Server::Configuration::HealthCheckFilterConfig healthCheckFilterConfig; + envoy::api::v2::filter::http::HealthCheck config{}; + NiceMock context; + + config.mutable_pass_through_mode()->set_value(false); + config.mutable_endpoint()->set_value("foo"); + healthCheckFilterConfig.createFilterFactoryFromProto(config, "dummy_stats_prefix", context); +} + +TEST(HealthCheckFilterConfig, HealthCheckFilterWithEmptyProto) { + Server::Configuration::HealthCheckFilterConfig healthCheckFilterConfig; + NiceMock context; + envoy::api::v2::filter::http::HealthCheck config = + *dynamic_cast( + factory.createEmptyConfigProto().get()); + + config.mutable_pass_through_mode()->set_value(false); + config.mutable_endpoint()->set_value("foo"); + healthCheckFilterConfig.createFilterFactoryFromProto(config, "dummy_stats_prefix", context); +} } // namespace Envoy From 38fb9a6a4961222447ab49353c018e8b2437038d Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 11:38:00 -0500 Subject: [PATCH 36/44] nits Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.cc | 16 ++++++++-------- source/common/config/filter_json.h | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index a4a1550270e6..e299b1f1a0cf 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -231,12 +231,12 @@ void FilterJson::translateMongoProxy(const Json::Object& json_mongo_proxy, } } -void FilterJson::translateFaultFilter(const Json::Object& config, +void FilterJson::translateFaultFilter(const Json::Object& json_fault, envoy::api::v2::filter::http::HTTPFault& fault) { - config.validateSchema(Json::Schema::FAULT_HTTP_FILTER_SCHEMA); + json_fault.validateSchema(Json::Schema::FAULT_HTTP_FILTER_SCHEMA); - const Json::ObjectSharedPtr config_abort = config.getObject("abort", true); - const Json::ObjectSharedPtr config_delay = config.getObject("delay", true); + const Json::ObjectSharedPtr config_abort = json_fault.getObject("abort", true); + const Json::ObjectSharedPtr config_delay = json_fault.getObject("delay", true); if (!config_abort->empty()) { auto* abort_fault = fault.mutable_abort(); @@ -253,14 +253,14 @@ void FilterJson::translateFaultFilter(const Json::Object& config, JSON_UTIL_SET_DURATION_FROM_FIELD(*config_delay, *delay, fixed_delay, fixed_duration); } - for (const auto json_header_matcher : config.getObjectArray("headers", true)) { + for (const auto json_header_matcher : json_fault.getObjectArray("headers", true)) { auto* header_matcher = fault.mutable_headers()->Add(); RdsJson::translateHeaderMatcher(*json_header_matcher, *header_matcher); } - JSON_UTIL_SET_STRING(config, fault, upstream_cluster); + JSON_UTIL_SET_STRING(json_fault, fault, upstream_cluster); - for (auto json_downstream_node : config.getStringArray("downstream_nodes", true)) { + for (auto json_downstream_node : json_fault.getStringArray("downstream_nodes", true)) { auto* downstream_node = fault.mutable_downstream_nodes()->Add(); *downstream_node = json_downstream_node; } @@ -278,7 +278,7 @@ void FilterJson::translateBufferFilter(const Json::Object& json_buffer, envoy::api::v2::filter::http::Buffer& buffer) { json_buffer.validateSchema(Json::Schema::BUFFER_HTTP_FILTER_SCHEMA); - buffer.mutable_max_request_bytes()->set_value(json_buffer.getInteger("max_request_bytes")); + JSON_UTIL_SET_INTEGER(json_buffer, buffer, max_request_bytes); JSON_UTIL_SET_DURATION_SECONDS(json_buffer, buffer, max_request_time); } diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index 88499c31738e..7fb8acc56292 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -52,11 +52,11 @@ class FilterJson { /** * Translate a v1 JSON Fault filter object to v2 envoy::api::v2::filter::http::HTTPFault. - * @param config source v1 JSON HTTP Fault Filter object. + * @param json_fault source v1 JSON HTTP Fault Filter object. * @param fault destination v2 * envoy::api::v2::filter::http::HTTPFault. */ - static void translateFaultFilter(const Json::Object& config, + static void translateFaultFilter(const Json::Object& json_fault, envoy::api::v2::filter::http::HTTPFault& fault); /* @@ -69,7 +69,7 @@ class FilterJson { /** * Translate a v1 JSON Buffer filter object to v2 envoy::api::v2::filter::http::Buffer. - * @param config source v1 JSON HTTP Buffer Filter object. + * @param json_buffer source v1 JSON HTTP Buffer Filter object. * @param buffer destination v2 * envoy::api::v2::filter::http::Buffer. */ From 4dcc0b9089f50e7504532c8e6081b21349ca6f54 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 11:52:52 -0500 Subject: [PATCH 37/44] compile fixes Signed-off-by: Shriram Rajagopalan --- source/server/http/health_check.cc | 27 ++++----------------------- source/server/http/health_check.h | 17 ++++++++--------- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index cfc23b8fcd17..8e71b024801c 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -49,6 +49,9 @@ HttpFilterFactoryCb HealthCheckFilterConfig::createHealthCheckFilter( }; } +/** + * Config registration for the health check filter. @see NamedHttpFilterConfigFactory. + */ HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Object& json_config, const std::string& stats_prefix, FactoryContext& context) { @@ -64,35 +67,13 @@ HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactoryFromProto( context); } -/** - * Config registration for the health check filter. @see NamedHttpFilterConfigFactory. - */ -HttpFilterFactoryCb HealthCheckFilterConfig::createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext& context) { - throw EnvoyException("cache_time_ms must not be set when path_through_mode is disabled"); -} - -HealthCheckCacheManagerSharedPtr cache_manager; -if (cache_time_ms > 0) { - cache_manager.reset( - new HealthCheckCacheManager(context.dispatcher(), std::chrono::milliseconds(cache_time_ms))); -} - -return [&context, pass_through_mode, cache_manager, - hc_endpoint](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamFilter(Http::StreamFilterSharedPtr{ - new HealthCheckFilter(context, pass_through_mode, cache_manager, hc_endpoint)}); -}; -} // namespace Configuration - /** * Static registration for the health check filter. @see RegisterFactory. */ static Registry::RegisterFactory register_; +} // namespace Configuration } // namespace Server -} // namespace Envoy HealthCheckCacheManager::HealthCheckCacheManager(Event::Dispatcher& dispatcher, std::chrono::milliseconds timeout) diff --git a/source/server/http/health_check.h b/source/server/http/health_check.h index de30f8f2f630..4214d9aceb83 100644 --- a/source/server/http/health_check.h +++ b/source/server/http/health_check.h @@ -29,17 +29,16 @@ class HealthCheckFilterConfig : public NamedHttpFilterConfigFactory { return ProtobufTypes::MessagePtr{new envoy::api::v2::filter::http::HealthCheck()}; } - std::string name() override { - return Config::HttpFilterNames::get().HEALTH_CHECK; + std::string name() override { return Config::HttpFilterNames::get().HEALTH_CHECK; } - private: - HttpFilterFactoryCb createHealthCheckFilter( - const envoy::api::v2::filter::http::HealthCheck& health_check, - const std::string& stats_prefix, FactoryContext& context); - }; +private: + HttpFilterFactoryCb + createHealthCheckFilter(const envoy::api::v2::filter::http::HealthCheck& health_check, + const std::string& stats_prefix, FactoryContext& context); +}; } // namespace Configuration -} // namespace Configuration +} // namespace Server /** * Shared cache manager used by all instances of a health check filter configuration as well as @@ -112,4 +111,4 @@ class HealthCheckFilter : public Http::StreamFilter { HealthCheckCacheManagerSharedPtr cache_manager_{}; const std::string endpoint_; }; -} // namespace Server +} // namespace Envoy From a1b9ad0f447c380b2cc88270e7c3124f1cbe7417 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 11:56:48 -0500 Subject: [PATCH 38/44] duration cast Signed-off-by: Shriram Rajagopalan --- source/server/config/http/buffer.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index b81cb30d75e9..52bffb152211 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -20,10 +20,11 @@ BufferFilterConfig::createBufferFilter(const envoy::api::v2::filter::http::Buffe ASSERT(buffer.has_max_request_bytes()); ASSERT(buffer.has_max_request_time()); - Http::BufferFilterConfigConstSharedPtr config(new Http::BufferFilterConfig{ - Http::BufferFilter::generateStats(stats_prefix, context.scope()), - static_cast(buffer.max_request_bytes().value()), - std::chrono::seconds(PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time) / 1000)}); + Http::BufferFilterConfigConstSharedPtr config( + new Http::BufferFilterConfig{Http::BufferFilter::generateStats(stats_prefix, context.scope()), + static_cast(buffer.max_request_bytes().value()), + std::chrono::duration_cast( + PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time))}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter( Http::StreamDecoderFilterSharedPtr{new Http::BufferFilter(config)}); From 9dffdeeefc88d58c2f9df760cdeb8d2c6d8b0680 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 12:06:10 -0500 Subject: [PATCH 39/44] new utility macro Signed-off-by: Shriram Rajagopalan --- source/common/protobuf/utility.h | 7 +++++++ source/server/config/http/buffer.cc | 9 ++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index b55ae1697de8..020453034064 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -35,6 +35,13 @@ ? Protobuf::util::TimeUtil::DurationToMilliseconds((message).field_name()) \ : throw MissingFieldException(#field_name, (message))) +// Obtain the seconds value of a google.protobuf.Duration field if set. Otherwise, throw a +// MissingFieldException. +#define PROTOBUF_GET_SECONDS_REQUIRED(message, field_name) \ + ((message).has_##field_name() \ + ? Protobuf::util::TimeUtil::DurationToSeconds((message).field_name()) \ + : throw MissingFieldException(#field_name, (message))) + namespace Envoy { class MissingFieldException : public EnvoyException { diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc index 52bffb152211..fe34c5e704c4 100644 --- a/source/server/config/http/buffer.cc +++ b/source/server/config/http/buffer.cc @@ -20,11 +20,10 @@ BufferFilterConfig::createBufferFilter(const envoy::api::v2::filter::http::Buffe ASSERT(buffer.has_max_request_bytes()); ASSERT(buffer.has_max_request_time()); - Http::BufferFilterConfigConstSharedPtr config( - new Http::BufferFilterConfig{Http::BufferFilter::generateStats(stats_prefix, context.scope()), - static_cast(buffer.max_request_bytes().value()), - std::chrono::duration_cast( - PROTOBUF_GET_MS_REQUIRED(buffer, max_request_time))}); + Http::BufferFilterConfigConstSharedPtr config(new Http::BufferFilterConfig{ + Http::BufferFilter::generateStats(stats_prefix, context.scope()), + static_cast(buffer.max_request_bytes().value()), + std::chrono::seconds(PROTOBUF_GET_SECONDS_REQUIRED(buffer, max_request_time))}); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamDecoderFilter( Http::StreamDecoderFilterSharedPtr{new Http::BufferFilter(config)}); From 0dbda03edaa36264c2d60b8db4197b9b6967ccbf Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 12:11:53 -0500 Subject: [PATCH 40/44] lint Signed-off-by: Shriram Rajagopalan --- source/server/http/health_check.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 8e71b024801c..4b23881d7598 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -8,6 +8,7 @@ #include "envoy/http/header_map.h" #include "envoy/registry/registry.h" +#include "common/config/filter_json.h" #include "common/common/assert.h" #include "common/common/enum_to_int.h" #include "common/http/codes.h" @@ -23,7 +24,7 @@ namespace Server { namespace Configuration { HttpFilterFactoryCb HealthCheckFilterConfig::createHealthCheckFilter( - const envoy::api::v2::filter::http::HealthCheck& health_check, const std::string& stats_prefix, + const envoy::api::v2::filter::http::HealthCheck& health_check, const std::string&, FactoryContext& context) { ASSERT(health_check.has_pass_through_mode()); ASSERT(!health_check.endpoint().empty()); From 5dddde7672f0c3b066b2caf0b400f405c1a009f2 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 12:19:56 -0500 Subject: [PATCH 41/44] compile fixes Signed-off-by: Shriram Rajagopalan --- test/server/http/health_check_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/server/http/health_check_test.cc b/test/server/http/health_check_test.cc index aeeda7183470..ba5c86d5f42e 100644 --- a/test/server/http/health_check_test.cc +++ b/test/server/http/health_check_test.cc @@ -233,7 +233,7 @@ TEST(HealthCheckFilterConfig, failsWhenNotPassThroughButTimeoutSetProto) { NiceMock context; config.mutable_pass_through_mode()->set_value(false); - config.mutable_endpoint()->set_value("foo"); + config.set_endpoint("foo"); config.mutable_cache_time()->set_seconds(10); EXPECT_THROW( @@ -247,7 +247,7 @@ TEST(HealthCheckFilterConfig, notFailingWhenNotPassThroughAndTimeoutNotSetProto) NiceMock context; config.mutable_pass_through_mode()->set_value(false); - config.mutable_endpoint()->set_value("foo"); + config.set_endpoint("foo"); healthCheckFilterConfig.createFilterFactoryFromProto(config, "dummy_stats_prefix", context); } @@ -256,10 +256,10 @@ TEST(HealthCheckFilterConfig, HealthCheckFilterWithEmptyProto) { NiceMock context; envoy::api::v2::filter::http::HealthCheck config = *dynamic_cast( - factory.createEmptyConfigProto().get()); + healthCheckFilterConfig.createEmptyConfigProto().get()); config.mutable_pass_through_mode()->set_value(false); - config.mutable_endpoint()->set_value("foo"); + config.set_endpoint("foo"); healthCheckFilterConfig.createFilterFactoryFromProto(config, "dummy_stats_prefix", context); } } // namespace Envoy From e8de97cfd71fed83eb78aad06a15a938dc8434df Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 12:28:07 -0500 Subject: [PATCH 42/44] really fix compiler errors Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index 3ed43f280428..9b71145b360e 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -273,7 +273,7 @@ void FilterJson::translateHealthCheckFilter( health_check.mutable_pass_through_mode()->set_value( json_health_check.getBoolean("pass_through_mode")); - JSON_UTIL_SET_DURATION_FROM_FIELD(json_health_check, health_check, cache_time); + JSON_UTIL_SET_DURATION(json_health_check, health_check, cache_time); JSON_UTIL_SET_STRING(json_health_check, health_check, endpoint); } From 0f1e30716b86c9599b21e481889aa6bb5af42d69 Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 14:53:56 -0500 Subject: [PATCH 43/44] format fix Signed-off-by: Shriram Rajagopalan --- source/server/http/health_check.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/http/health_check.cc b/source/server/http/health_check.cc index 4b23881d7598..805029e60861 100644 --- a/source/server/http/health_check.cc +++ b/source/server/http/health_check.cc @@ -8,9 +8,9 @@ #include "envoy/http/header_map.h" #include "envoy/registry/registry.h" -#include "common/config/filter_json.h" #include "common/common/assert.h" #include "common/common/enum_to_int.h" +#include "common/config/filter_json.h" #include "common/http/codes.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" From 1ee768fd1caeda79a54bd5eb9c597dac2a29f01e Mon Sep 17 00:00:00 2001 From: Shriram Rajagopalan Date: Fri, 10 Nov 2017 15:04:58 -0500 Subject: [PATCH 44/44] json_util_set_bool Signed-off-by: Shriram Rajagopalan --- source/common/config/filter_json.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index 9b71145b360e..07f5bcbf7ecf 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -271,8 +271,7 @@ void FilterJson::translateHealthCheckFilter( envoy::api::v2::filter::http::HealthCheck& health_check) { json_health_check.validateSchema(Json::Schema::HEALTH_CHECK_HTTP_FILTER_SCHEMA); - health_check.mutable_pass_through_mode()->set_value( - json_health_check.getBoolean("pass_through_mode")); + JSON_UTIL_SET_BOOL(json_health_check, health_check, pass_through_mode); JSON_UTIL_SET_DURATION(json_health_check, health_check, cache_time); JSON_UTIL_SET_STRING(json_health_check, health_check, endpoint); }