From 21a2bc4c83079e81054065cfa7eeda4f20fb46a5 Mon Sep 17 00:00:00 2001 From: lhuang8 Date: Fri, 30 Aug 2019 14:58:53 -0700 Subject: [PATCH 1/3] xDS: gRPC connection failure shouldn't make Envoy continue startup Currently, if gRPC config stream disconnected while Envoy waiting for initial xDS response, xDS implementations' onConfigUpdateFailed() will allow Envoy startup to continue. This may cause Envoy begins taking traffics while route/cluster/endpoint config are still missing and return "404 NR" or "503 NR". This change makes Envoy waiting for initial xDS response until initial_fetch_timeout if specified. Signed-off-by: lhuang8 --- .../common/config/delta_subscription_state.cc | 5 +- .../config/grpc_mux_subscription_impl.cc | 7 +++ .../common/config/http_subscription_impl.cc | 48 ++++++++++++------- source/common/config/http_subscription_impl.h | 4 +- source/common/http/rest_api_fetcher.cc | 8 ++-- source/common/http/rest_api_fetcher.h | 5 +- source/common/upstream/eds.cc | 7 +-- .../client_ssl_auth/client_ssl_auth.cc | 4 +- .../network/client_ssl_auth/client_ssl_auth.h | 3 +- .../config/grpc_subscription_impl_test.cc | 8 +++- .../config/http_subscription_impl_test.cc | 8 ++-- .../config/subscription_factory_impl_test.cc | 3 +- test/common/router/rds_impl_test.cc | 4 +- test/common/runtime/runtime_impl_test.cc | 5 +- test/common/upstream/cds_api_impl_test.cc | 4 +- test/common/upstream/eds_test.cc | 8 ---- test/server/lds_api_test.cc | 3 +- 17 files changed, 78 insertions(+), 56 deletions(-) diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index bec633841aff..5c5a795b7af0 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -151,11 +151,10 @@ void DeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAc } void DeltaSubscriptionState::handleEstablishmentFailure() { - disableInitFetchTimeoutTimer(); + // New gRPC stream will be established and send requests again. + // If init_fetch_timeout is non-zero, server will continue startup after it timeout stats_.update_failure_.inc(); stats_.update_attempt_.inc(); - callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - nullptr); } envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { diff --git a/source/common/config/grpc_mux_subscription_impl.cc b/source/common/config/grpc_mux_subscription_impl.cc index a181b4efa174..bbbe29cca83e 100644 --- a/source/common/config/grpc_mux_subscription_impl.cc +++ b/source/common/config/grpc_mux_subscription_impl.cc @@ -81,7 +81,14 @@ void GrpcMuxSubscriptionImpl::onConfigUpdateFailed(ConfigUpdateFailureReason rea ENVOY_LOG(warn, "gRPC config for {} rejected: {}", type_url_, e->what()); break; } + stats_.update_attempt_.inc(); + if (Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure == reason) { + // New gRPC stream will be established and send requests again. + // If init_fetch_timeout is non-zero, server will continue startup after it timeout + return; + } + callbacks_.onConfigUpdateFailed(reason, e); } diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 908250c79495..4f532e9f2159 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -38,10 +38,7 @@ HttpSubscriptionImpl::HttpSubscriptionImpl( void HttpSubscriptionImpl::start(const std::set& resource_names) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { - ENVOY_LOG(warn, "REST config: initial fetch timed out for", path_); - stats_.init_fetch_timeout_.inc(); - callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, - nullptr); + handleFailure(Config::ConfigUpdateFailureReason::FetchTimedout, nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } @@ -77,8 +74,7 @@ void HttpSubscriptionImpl::parseResponse(const Http::Message& response) { try { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } catch (const EnvoyException& e) { - ENVOY_LOG(warn, "REST config JSON conversion error: {}", e.what()); - handleFailure(nullptr); + handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } try { @@ -87,23 +83,43 @@ void HttpSubscriptionImpl::parseResponse(const Http::Message& response) { stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.update_success_.inc(); } catch (const EnvoyException& e) { - ENVOY_LOG(warn, "REST config update rejected: {}", e.what()); - stats_.update_rejected_.inc(); - callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); + handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } } void HttpSubscriptionImpl::onFetchComplete() {} -void HttpSubscriptionImpl::onFetchFailure(const EnvoyException* e) { - disableInitFetchTimeoutTimer(); - ENVOY_LOG(warn, "REST config update failed: {}", e != nullptr ? e->what() : "fetch failure"); - handleFailure(e); +void HttpSubscriptionImpl::onFetchFailure(Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) { + handleFailure(reason, e); } -void HttpSubscriptionImpl::handleFailure(const EnvoyException* e) { - stats_.update_failure_.inc(); - callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, e); +void HttpSubscriptionImpl::handleFailure(Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) { + + switch (reason) { + case Config::ConfigUpdateFailureReason::ConnectionFailure: + ENVOY_LOG(warn, "REST update for {} failed", path_); + stats_.update_failure_.inc(); + break; + case Config::ConfigUpdateFailureReason::FetchTimedout: + ENVOY_LOG(warn, "REST config: initial fetch timeout for {}", path_); + stats_.init_fetch_timeout_.inc(); + break; + case Config::ConfigUpdateFailureReason::UpdateRejected: + ASSERT(e != nullptr); + ENVOY_LOG(warn, "REST config for {} rejected: {}", path_, e->what()); + stats_.update_rejected_.inc(); + disableInitFetchTimeoutTimer(); + break; + } + + // TODO(l8huang): test case "//test/integration:hotrestart_test" relies + // on Http::AsyncClient::FailureReason::Reset to get server startup without + // any initial CDS/LDS discovery response, so here calls onConfigUpdateFailed() + // even reason is ConnectionFailure. After the test case fixed, + // onConfigUpdateFailed() shouldn't be called for ConnectionFailure. + callbacks_.onConfigUpdateFailed(reason, e); } void HttpSubscriptionImpl::disableInitFetchTimeoutTimer() { diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index 452ba132582a..6ad8055e4e8b 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -37,10 +37,10 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, void createRequest(Http::Message& request) override; void parseResponse(const Http::Message& response) override; void onFetchComplete() override; - void onFetchFailure(const EnvoyException* e) override; + void onFetchFailure(Config::ConfigUpdateFailureReason reason, const EnvoyException* e) override; private: - void handleFailure(const EnvoyException* e); + void handleFailure(Config::ConfigUpdateFailureReason reason, const EnvoyException* e); void disableInitFetchTimeoutTimer(); std::string path_; diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index 9702f7081ad1..762623c0e980 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -41,14 +41,16 @@ void RestApiFetcher::onSuccess(Http::MessagePtr&& response) { try { parseResponse(*response); } catch (EnvoyException& e) { - onFetchFailure(&e); + onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } requestComplete(); } -void RestApiFetcher::onFailure(Http::AsyncClient::FailureReason) { - onFetchFailure(nullptr); +void RestApiFetcher::onFailure(Http::AsyncClient::FailureReason reason) { + // Currently Http::AsyncClient::FailureReason only has one value: "Reset". + ASSERT(reason == Http::AsyncClient::FailureReason::Reset); + onFetchFailure(Config::ConfigUpdateFailureReason::ConnectionFailure, nullptr); requestComplete(); } diff --git a/source/common/http/rest_api_fetcher.h b/source/common/http/rest_api_fetcher.h index 48b5636079b7..80ab2d3185fe 100644 --- a/source/common/http/rest_api_fetcher.h +++ b/source/common/http/rest_api_fetcher.h @@ -3,6 +3,7 @@ #include #include +#include "envoy/config/subscription.h" #include "envoy/event/dispatcher.h" #include "envoy/runtime/runtime.h" #include "envoy/upstream/cluster_manager.h" @@ -46,9 +47,11 @@ class RestApiFetcher : public Http::AsyncClient::Callbacks { /** * This will be called if the fetch fails (either due to non-200 response, network error, etc.). + * @param reason supplies the fetch failure reason. * @param e supplies any exception data on why the fetch failed. May be nullptr. */ - virtual void onFetchFailure(const EnvoyException* e) PURE; + virtual void onFetchFailure(Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) PURE; protected: const std::string remote_cluster_name_; diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 3d795c9a207b..b6cfb28a62c7 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -251,13 +251,8 @@ bool EdsClusterImpl::updateHostsPerLocality( return false; } -void EdsClusterImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, +void EdsClusterImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, const EnvoyException*) { - // We should not call onPreInitComplete if this method is called because of stream disconnection. - // This might potentially hang the initialization forever, if init_fetch_timeout is disabled. - if (reason == Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure) { - return; - } // We need to allow server startup to continue, even if we have a bad config. onPreInitComplete(); } diff --git a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc index 4495f38cf722..a41ce47192d1 100644 --- a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc +++ b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.cc @@ -77,7 +77,9 @@ void ClientSslAuthConfig::parseResponse(const Http::Message& message) { stats_.total_principals_.set(new_principals->size()); } -void ClientSslAuthConfig::onFetchFailure(const EnvoyException*) { stats_.update_failure_.inc(); } +void ClientSslAuthConfig::onFetchFailure(Config::ConfigUpdateFailureReason, const EnvoyException*) { + stats_.update_failure_.inc(); +} static const std::string Path = "/v1/certs/list/approved"; diff --git a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h index 54ad916fe8dd..ca521a9a5a40 100644 --- a/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h +++ b/source/extensions/filters/network/client_ssl_auth/client_ssl_auth.h @@ -6,6 +6,7 @@ #include #include "envoy/config/filter/network/client_ssl_auth/v2/client_ssl_auth.pb.h" +#include "envoy/config/subscription.h" #include "envoy/network/filter.h" #include "envoy/runtime/runtime.h" #include "envoy/stats/scope.h" @@ -94,7 +95,7 @@ class ClientSslAuthConfig : public Http::RestApiFetcher { void createRequest(Http::Message& request) override; void parseResponse(const Http::Message& response) override; void onFetchComplete() override {} - void onFetchFailure(const EnvoyException* e) override; + void onFetchFailure(Config::ConfigUpdateFailureReason reason, const EnvoyException* e) override; ThreadLocal::SlotPtr tls_; Network::Address::IpList ip_white_list_; diff --git a/test/common/config/grpc_subscription_impl_test.cc b/test/common/config/grpc_subscription_impl_test.cc index 7a1ca435985d..e79995b28f39 100644 --- a/test/common/config/grpc_subscription_impl_test.cc +++ b/test/common/config/grpc_subscription_impl_test.cc @@ -15,8 +15,10 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { InSequence s; EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(nullptr)); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure EXPECT_CALL(callbacks_, - onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)) + .Times(0); EXPECT_CALL(random_, random()); EXPECT_CALL(*timer_, enableTimer(_, _)); subscription_->start({"cluster0", "cluster1"}); @@ -38,8 +40,10 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { TEST_F(GrpcSubscriptionImplTest, RemoteStreamClose) { startSubscription({"cluster0", "cluster1"}); EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure EXPECT_CALL(callbacks_, - onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)) + .Times(0); EXPECT_CALL(*timer_, enableTimer(_, _)); EXPECT_CALL(random_, random()); subscription_->grpcMux().grpcStreamForTest().onRemoteClose(Grpc::Status::GrpcStatus::Canceled, diff --git a/test/common/config/http_subscription_impl_test.cc b/test/common/config/http_subscription_impl_test.cc index afd592c0220e..54a630cecaeb 100644 --- a/test/common/config/http_subscription_impl_test.cc +++ b/test/common/config/http_subscription_impl_test.cc @@ -34,14 +34,14 @@ TEST_F(HttpSubscriptionImplTest, BadJsonRecovery) { EXPECT_CALL(random_gen_, random()).WillOnce(Return(0)); EXPECT_CALL(*timer_, enableTimer(_, _)); EXPECT_CALL(callbacks_, - onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); http_callbacks_->onSuccess(std::move(message)); - EXPECT_TRUE(statsAre(1, 0, 0, 1, 0, 0)); + EXPECT_TRUE(statsAre(1, 0, 1, 0, 0, 0)); request_in_progress_ = false; timerTick(); - EXPECT_TRUE(statsAre(2, 0, 0, 1, 0, 0)); + EXPECT_TRUE(statsAre(2, 0, 1, 0, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); - EXPECT_TRUE(statsAre(3, 1, 0, 1, 0, 7148434200721666028)); + EXPECT_TRUE(statsAre(3, 1, 1, 0, 0, 7148434200721666028)); } TEST_F(HttpSubscriptionImplTest, ConfigNotModified) { diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index c5cd9c4c771a..403ad4d5f91f 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -302,7 +302,8 @@ TEST_F(SubscriptionFactoryTest, GrpcSubscription) { })); EXPECT_CALL(random_, random()); EXPECT_CALL(dispatcher_, createTimer_(_)).Times(2); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)).Times(0); subscriptionFromConfigSource(config)->start({"static_cluster"}); } diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index b581017f3f92..11424515eb32 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -253,8 +253,8 @@ TEST_F(RdsImplTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - rds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - {}); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure + rds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, {}); } class RouteConfigProviderManagerImplTest : public RdsTestBase { diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 11d029309411..7b06db0f9cdd 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -845,8 +845,9 @@ TEST_F(RtdsLoaderImplTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - rtds_callbacks_[0]->onConfigUpdateFailed( - Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, {}); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure + rtds_callbacks_[0]->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, + {}); EXPECT_EQ(0, store_.counter("runtime.load_error").value()); EXPECT_EQ(1, store_.counter("runtime.load_success").value()); diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index 48101c09578b..fe6b5e39ab60 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -352,8 +352,8 @@ TEST_F(CdsApiImplTest, FailureSubscription) { setup(); EXPECT_CALL(initialized_, ready()); - cds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - {}); + // onConfigUpdateFailed() should not be called for gRPC stream connection failure + cds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, {}); EXPECT_EQ("", cds_->versionInfo()); } diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 688c162807ff..9247c9d84eb8 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -207,14 +207,6 @@ TEST_F(EdsTest, ValidateFail) { EXPECT_FALSE(initialized_); } -// Validate onConfigUpdate() on stream disconnection. -TEST_F(EdsTest, StreamDisconnection) { - initialize(); - eds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - nullptr); - EXPECT_FALSE(initialized_); -} - // Validate that onConfigUpdate() with unexpected cluster names rejects config. TEST_F(EdsTest, OnConfigUpdateWrongName) { envoy::api::v2::ClusterLoadAssignment cluster_load_assignment; diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 1cedee3abb09..a5ffe1378ace 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -411,8 +411,7 @@ TEST_F(LdsApiTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - lds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, - {}); + lds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, {}); EXPECT_EQ("", lds_->versionInfo()); } From ab71a09a57aaf1f4924b815ca6d77d8880edcb41 Mon Sep 17 00:00:00 2001 From: lhuang8 Date: Fri, 6 Sep 2019 13:37:28 -0700 Subject: [PATCH 2/3] add assert and unify HTTP and gRPC error handling for connection failure Signed-off-by: lhuang8 --- source/common/config/http_subscription_impl.cc | 12 +++++++----- source/common/router/rds_impl.cc | 5 +++-- source/common/router/scoped_rds.h | 3 ++- source/common/router/vhds.cc | 3 ++- source/common/runtime/runtime_impl.cc | 3 ++- source/common/secret/sds_api.cc | 4 +++- source/common/upstream/cds_api_impl.cc | 3 ++- source/common/upstream/eds.cc | 3 ++- source/server/lds_api.cc | 3 ++- test/common/config/delta_subscription_state_test.cc | 8 ++++++++ test/common/config/http_subscription_impl_test.cc | 3 ++- test/common/config/subscription_impl_test.cc | 4 ++++ 12 files changed, 39 insertions(+), 15 deletions(-) diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 4f532e9f2159..a971607718cc 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -105,6 +105,7 @@ void HttpSubscriptionImpl::handleFailure(Config::ConfigUpdateFailureReason reaso case Config::ConfigUpdateFailureReason::FetchTimedout: ENVOY_LOG(warn, "REST config: initial fetch timeout for {}", path_); stats_.init_fetch_timeout_.inc(); + disableInitFetchTimeoutTimer(); break; case Config::ConfigUpdateFailureReason::UpdateRejected: ASSERT(e != nullptr); @@ -114,11 +115,12 @@ void HttpSubscriptionImpl::handleFailure(Config::ConfigUpdateFailureReason reaso break; } - // TODO(l8huang): test case "//test/integration:hotrestart_test" relies - // on Http::AsyncClient::FailureReason::Reset to get server startup without - // any initial CDS/LDS discovery response, so here calls onConfigUpdateFailed() - // even reason is ConnectionFailure. After the test case fixed, - // onConfigUpdateFailed() shouldn't be called for ConnectionFailure. + if (Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure == reason) { + // New requests will be sent again. + // If init_fetch_timeout is non-zero, server will continue startup after it timeout + return; + } + callbacks_.onConfigUpdateFailed(reason, e); } diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 64a60062a30c..f7080c018f09 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -148,8 +148,9 @@ void RdsRouteConfigSubscription::onConfigUpdate( } } -void RdsRouteConfigSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, - const EnvoyException*) { +void RdsRouteConfigSubscription::onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) { + ASSERT(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure != reason); // We need to allow server startup to continue, even if we have a bad // config. init_target_.ready(); diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 878cca0a3280..422551449acd 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -148,8 +148,9 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio void onConfigUpdate(const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& version_info) override; - void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) override { + ASSERT(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure != reason); DeltaConfigSubscriptionInstance::onConfigUpdateFailed(); } std::string resourceName(const ProtobufWkt::Any& resource) override { diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 6cd05ed015f0..93d304647f7d 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -45,8 +45,9 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, *scope_, *this); } -void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, +void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) { + ASSERT(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure != reason); // We need to allow server startup to continue, even if we have a bad // config. init_target_.ready(); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index b1f4494b27a7..220e4305571e 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -543,8 +543,9 @@ void RtdsSubscription::onConfigUpdate( onConfigUpdate(unwrapped_resource, resources[0].version()); } -void RtdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, +void RtdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException*) { + ASSERT(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure != reason); // We need to allow server startup to continue, even if we have a bad // config. init_target_.ready(); diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 7d695040dfa9..4514f5ce0792 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -58,7 +58,9 @@ void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrFieldonFailure(Http::AsyncClient::FailureReason::Reset); EXPECT_TRUE(statsAre(1, 0, 0, 1, 0, 0)); timerTick(); diff --git a/test/common/config/subscription_impl_test.cc b/test/common/config/subscription_impl_test.cc index 622a268c90cb..c45f19bd00e8 100644 --- a/test/common/config/subscription_impl_test.cc +++ b/test/common/config/subscription_impl_test.cc @@ -149,7 +149,11 @@ TEST_P(SubscriptionImplInitFetchTimeoutTest, InitialFetchTimeout) { expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds(1000)); startSubscription({"cluster0", "cluster1"}); statsAre(1, 0, 0, 0, 0, 0); + if (GetParam() == SubscriptionType::Http) { + expectDisableInitFetchTimeoutTimer(); + } expectConfigUpdateFailed(); + callInitFetchTimeoutCb(); statsAre(1, 0, 0, 0, 1, 0); } From e530371b995d86c624e07f70ebd9677d0e974595 Mon Sep 17 00:00:00 2001 From: lhuang8 Date: Wed, 18 Sep 2019 12:02:19 -0700 Subject: [PATCH 3/3] update if condition style Signed-off-by: lhuang8 --- source/common/config/grpc_mux_subscription_impl.cc | 2 +- source/common/config/http_subscription_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/grpc_mux_subscription_impl.cc b/source/common/config/grpc_mux_subscription_impl.cc index bbbe29cca83e..dffab9f0caea 100644 --- a/source/common/config/grpc_mux_subscription_impl.cc +++ b/source/common/config/grpc_mux_subscription_impl.cc @@ -83,7 +83,7 @@ void GrpcMuxSubscriptionImpl::onConfigUpdateFailed(ConfigUpdateFailureReason rea } stats_.update_attempt_.inc(); - if (Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure == reason) { + if (reason == Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure) { // New gRPC stream will be established and send requests again. // If init_fetch_timeout is non-zero, server will continue startup after it timeout return; diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index a971607718cc..dda038436500 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -115,7 +115,7 @@ void HttpSubscriptionImpl::handleFailure(Config::ConfigUpdateFailureReason reaso break; } - if (Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure == reason) { + if (reason == Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure) { // New requests will be sent again. // If init_fetch_timeout is non-zero, server will continue startup after it timeout return;