diff --git a/include/envoy/config/BUILD b/include/envoy/config/BUILD index ab8c3a0899f67..2b2504620c994 100644 --- a/include/envoy/config/BUILD +++ b/include/envoy/config/BUILD @@ -33,6 +33,7 @@ envoy_cc_library( name = "grpc_mux_interface", hdrs = ["grpc_mux.h"], deps = [ + ":subscription_interface", "//include/envoy/stats:stats_macros", "//source/common/protobuf", ], diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 3d5bf2c17c70e..4888e4e7e7161 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -2,6 +2,7 @@ #include "envoy/common/exception.h" #include "envoy/common/pure.h" +#include "envoy/config/subscription.h" #include "envoy/stats/stats_macros.h" #include "common/protobuf/protobuf.h" @@ -42,9 +43,11 @@ class GrpcMuxCallbacks { /** * Called when either the subscription is unable to fetch a config update or when onConfigUpdate * invokes an exception. + * @param reason supplies the update failure reason. * @param e supplies any exception data on why the fetch failed. May be nullptr. */ - virtual void onConfigUpdateFailed(const EnvoyException* e) PURE; + virtual void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) PURE; /** * Obtain the "name" of a v2 API resource in a google.protobuf.Any, e.g. the route config name for diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 822a3eec68a67..bb9861d374ff1 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -13,6 +13,18 @@ namespace Envoy { namespace Config { +/** + * Reason that a config update is failed. + */ +enum class ConfigUpdateFailureReason { + // A connection failure took place and the update could not be fetched. + ConnectionFailure, + // Config fetch timed out. + FetchTimedout, + // Update rejected because there is a problem in applying the update. + UpdateRejected +}; + class SubscriptionCallbacks { public: virtual ~SubscriptionCallbacks() = default; @@ -45,9 +57,10 @@ class SubscriptionCallbacks { /** * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate * invokes an exception. + * @param reason supplies the update failure reason. * @param e supplies any exception data on why the fetch failed. May be nullptr. */ - virtual void onConfigUpdateFailed(const EnvoyException* e) PURE; + virtual void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) PURE; /** * Obtain the "name" of a v2 API resource in a google.protobuf.Any, e.g. the route config name for diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 8aa9d6f0bcbc3..a0dcb5f0b66fb 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -25,7 +25,8 @@ void DeltaSubscriptionState::setInitFetchTimeout(Event::Dispatcher& dispatcher) if (init_fetch_timeout_.count() > 0 && !init_fetch_timeout_timer_) { init_fetch_timeout_timer_ = dispatcher.createTimer([this]() -> void { ENVOY_LOG(warn, "delta config: initial fetch timed out for {}", type_url_); - callbacks_.onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, + nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } @@ -145,14 +146,15 @@ void DeltaSubscriptionState::handleBadResponse(const EnvoyException& e, UpdateAc disableInitFetchTimeoutTimer(); stats_.update_rejected_.inc(); ENVOY_LOG(warn, "delta config for {} rejected: {}", type_url_, e.what()); - callbacks_.onConfigUpdateFailed(&e); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } void DeltaSubscriptionState::handleEstablishmentFailure() { disableInitFetchTimeoutTimer(); stats_.update_failure_.inc(); stats_.update_attempt_.inc(); - callbacks_.onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + nullptr); } envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index c9bcaedd3d7d2..f14e395330750 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -49,11 +49,15 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(warn, "Filesystem config update rejected: {}", e.what()); ENVOY_LOG(debug, "Failed configuration:\n{}", message.DebugString()); stats_.update_rejected_.inc(); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } else { ENVOY_LOG(warn, "Filesystem config update failure: {}", e.what()); stats_.update_failure_.inc(); + // ConnectionFailure is not a meaningful error code for file system but it has been chosen so + // that the behaviour is uniform across all subscription types. + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + &e); } - callbacks_.onConfigUpdateFailed(&e); } } diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index b18bcac597ff2..55ffd4ea1ad6e 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -192,7 +192,8 @@ void GrpcMuxImpl::onDiscoveryResponse( api_state_[type_url].request_.set_version_info(message->version_info()); } catch (const EnvoyException& e) { for (auto watch : api_state_[type_url].watches_) { - watch->callbacks_.onConfigUpdateFailed(&e); + watch->callbacks_.onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } ::google::rpc::Status* error_detail = api_state_[type_url].request_.mutable_error_detail(); error_detail->set_code(Grpc::Status::GrpcStatus::Internal); @@ -213,7 +214,8 @@ void GrpcMuxImpl::onStreamEstablished() { void GrpcMuxImpl::onEstablishmentFailure() { for (const auto& api_state : api_state_) { for (auto watch : api_state.second.watches_) { - watch->callbacks_.onConfigUpdateFailed(nullptr); + watch->callbacks_.onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, nullptr); } } } diff --git a/source/common/config/grpc_mux_subscription_impl.cc b/source/common/config/grpc_mux_subscription_impl.cc index 8383036d4e522..cb50a314dae83 100644 --- a/source/common/config/grpc_mux_subscription_impl.cc +++ b/source/common/config/grpc_mux_subscription_impl.cc @@ -22,8 +22,8 @@ GrpcMuxSubscriptionImpl::GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, void GrpcMuxSubscriptionImpl::start(const std::set& resources) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { - ENVOY_LOG(warn, "gRPC config: initial fetch timed out for {}", type_url_); - callbacks_.onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, + nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } @@ -61,18 +61,27 @@ void GrpcMuxSubscriptionImpl::onConfigUpdate( resources.size(), version_info); } -void GrpcMuxSubscriptionImpl::onConfigUpdateFailed(const EnvoyException* e) { - disableInitFetchTimeoutTimer(); - // TODO(htuch): Less fragile signal that this is failure vs. reject. - if (e == nullptr) { +void GrpcMuxSubscriptionImpl::onConfigUpdateFailed(ConfigUpdateFailureReason reason, + const EnvoyException* e) { + switch (reason) { + case Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure: stats_.update_failure_.inc(); ENVOY_LOG(debug, "gRPC update for {} failed", type_url_); - } else { + break; + case Envoy::Config::ConfigUpdateFailureReason::FetchTimedout: + disableInitFetchTimeoutTimer(); + ENVOY_LOG(warn, "gRPC config: initial fetch timed out for {}", type_url_); + break; + case Envoy::Config::ConfigUpdateFailureReason::UpdateRejected: + // We expect Envoy exception to be thrown when update is rejected. + ASSERT(e != nullptr); + disableInitFetchTimeoutTimer(); stats_.update_rejected_.inc(); ENVOY_LOG(warn, "gRPC config for {} rejected: {}", type_url_, e->what()); + break; } stats_.update_attempt_.inc(); - callbacks_.onConfigUpdateFailed(e); + callbacks_.onConfigUpdateFailed(reason, e); } std::string GrpcMuxSubscriptionImpl::resourceName(const ProtobufWkt::Any& resource) { diff --git a/source/common/config/grpc_mux_subscription_impl.h b/source/common/config/grpc_mux_subscription_impl.h index 10d08ff49f3c6..9fb4cd76407c2 100644 --- a/source/common/config/grpc_mux_subscription_impl.h +++ b/source/common/config/grpc_mux_subscription_impl.h @@ -29,7 +29,8 @@ class GrpcMuxSubscriptionImpl : public Subscription, // Config::GrpcMuxCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override; private: diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 78a5e6caf9c05..4ee6388955781 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -39,7 +39,8 @@ 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_); - callbacks_.onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::FetchTimedout, + nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } @@ -87,7 +88,7 @@ void HttpSubscriptionImpl::parseResponse(const Http::Message& response) { } catch (const EnvoyException& e) { ENVOY_LOG(warn, "REST config update rejected: {}", e.what()); stats_.update_rejected_.inc(); - callbacks_.onConfigUpdateFailed(&e); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } } @@ -101,7 +102,7 @@ void HttpSubscriptionImpl::onFetchFailure(const EnvoyException* e) { void HttpSubscriptionImpl::handleFailure(const EnvoyException* e) { stats_.update_failure_.inc(); - callbacks_.onConfigUpdateFailed(e); + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, e); } void HttpSubscriptionImpl::disableInitFetchTimeoutTimer() { diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 781ce9db15215..e316460e0f09f 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -143,7 +143,8 @@ void RdsRouteConfigSubscription::onConfigUpdate( } } -void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { +void RdsRouteConfigSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad // config. init_target_.ready(); diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 1b627cab27115..6ec2c3e16047c 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -114,7 +114,8 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, void onConfigUpdate(const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_) diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 725c3ec755ead..4fd6c72c3ff86 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -110,8 +110,9 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio const Protobuf::RepeatedPtrField&, const std::string&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - void onConfigUpdateFailed(const EnvoyException*) override { - DeltaConfigSubscriptionInstance::onConfigUpdateFailed(); + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) override { + ConfigSubscriptionCommonBase::onConfigUpdateFailed(); } std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 100c9ef6034e3..f48449f67897d 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -46,7 +46,8 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, *scope_, *this); } -void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { +void VhdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad // config. init_target_.ready(); diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 7c5b370babe1f..0959bc6eca2a3 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -56,7 +56,8 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, } void onConfigUpdate(const Protobuf::RepeatedPtrField&, const Protobuf::RepeatedPtrField&, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_) diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 6ed82c39a37d2..8be5d9a964f85 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -526,7 +526,8 @@ void RtdsSubscription::onConfigUpdate( onConfigUpdate(unwrapped_resource, resources[0].version()); } -void RtdsSubscription::onConfigUpdateFailed(const EnvoyException*) { +void RtdsSubscription::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) { // 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.h b/source/common/runtime/runtime_impl.h index a41260eca02fd..fd1b6d271ade8 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -205,7 +205,8 @@ struct RtdsSubscription : Config::SubscriptionCallbacks, Logger::Loggable& removed_resources, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 7626433b9a25d..1aa00bb8c75b1 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -59,7 +59,7 @@ void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrField&, const Protobuf::RepeatedPtrField&, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_) .name(); diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index caea8d9f571ee..2d0271f56e586 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -98,7 +98,8 @@ void CdsApiImpl::onConfigUpdate( } } -void CdsApiImpl::onConfigUpdateFailed(const EnvoyException*) { +void CdsApiImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) { // We need to allow server startup to continue, even if we have a bad // config. runInitializeCallbackIfAny(); diff --git a/source/common/upstream/cds_api_impl.h b/source/common/upstream/cds_api_impl.h index c9c2a5d1cc784..2825213f31534 100644 --- a/source/common/upstream/cds_api_impl.h +++ b/source/common/upstream/cds_api_impl.h @@ -40,7 +40,8 @@ class CdsApiImpl : public CdsApi, void onConfigUpdate(const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_).name(); } diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index f215651682ddb..7205d5aec7d24 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -251,8 +251,13 @@ bool EdsClusterImpl::updateHostsPerLocality( return false; } -void EdsClusterImpl::onConfigUpdateFailed(const EnvoyException* e) { - UNREFERENCED_PARAMETER(e); +void EdsClusterImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + 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/common/upstream/eds.h b/source/common/upstream/eds.h index 9953555db0e16..c7b3ffaba0241 100644 --- a/source/common/upstream/eds.h +++ b/source/common/upstream/eds.h @@ -35,7 +35,8 @@ class EdsClusterImpl : public BaseDynamicClusterImpl, Config::SubscriptionCallba const std::string& version_info) override; void onConfigUpdate(const Protobuf::RepeatedPtrField&, const Protobuf::RepeatedPtrField&, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_) diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 6f9c5c8c0ef28..581abba24b71b 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -108,7 +108,8 @@ void LdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info) override; - void onConfigUpdateFailed(const EnvoyException* e) override; + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, + const EnvoyException* e) override; std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource, validation_visitor_).name(); } diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 020d6f5260d82..5e7e65eeec709 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -91,7 +91,8 @@ class DummyConfigSubscription : public ConfigSubscriptionInstance, } // Envoy::Config::SubscriptionCallbacks - void onConfigUpdateFailed(const EnvoyException*) override {} + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) override {} // Envoy::Config::SubscriptionCallbacks std::string resourceName(const ProtobufWkt::Any&) override { return ""; } @@ -549,7 +550,8 @@ class DeltaDummyConfigSubscription : public DeltaConfigSubscriptionInstance, const Protobuf::RepeatedPtrField&, const std::string&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - void onConfigUpdateFailed(const EnvoyException*) override { + void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason, + const EnvoyException*) override { ConfigSubscriptionCommonBase::onConfigUpdateFailed(); } std::string resourceName(const ProtobufWkt::Any&) override { @@ -725,7 +727,8 @@ TEST_F(DeltaConfigProviderImplTest, DeltaSubscriptionFailure) { timeSystem().setSystemTime(time); const EnvoyException ex(fmt::format("config failure")); // Verify the failure updates the lastUpdated() timestamp. - subscription.onConfigUpdateFailed(&ex); + subscription.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + &ex); EXPECT_EQ(std::chrono::time_point_cast(provider->lastUpdated()) .time_since_epoch(), time); diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index a2431c154bc81..aada398fd30cd 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -127,7 +127,8 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { if (accept) { expectSendMessage({}, version); } else { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); expectSendMessage({}, {}, Grpc::Status::GrpcStatus::Internal, "bad config", {}); } subscription_->onDiscoveryResponse(std::move(response)); @@ -150,7 +151,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { } void expectConfigUpdateFailed() override { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(nullptr)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, nullptr)); } void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { diff --git a/test/common/config/filesystem_subscription_impl_test.cc b/test/common/config/filesystem_subscription_impl_test.cc index 36c9815d1bf0c..f4ac009e58f37 100644 --- a/test/common/config/filesystem_subscription_impl_test.cc +++ b/test/common/config/filesystem_subscription_impl_test.cc @@ -19,7 +19,8 @@ class FilesystemSubscriptionImplTest : public testing::Test, TEST_F(FilesystemSubscriptionImplTest, BadJsonRecovery) { startSubscription({"cluster0", "cluster1"}); EXPECT_TRUE(statsAre(1, 0, 0, 0, 0)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); updateFile(";!@#badjso n"); EXPECT_TRUE(statsAre(2, 0, 0, 1, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); diff --git a/test/common/config/filesystem_subscription_test_harness.h b/test/common/config/filesystem_subscription_test_harness.h index b34028e40b538..03abeb81e72b8 100644 --- a/test/common/config/filesystem_subscription_test_harness.h +++ b/test/common/config/filesystem_subscription_test_harness.h @@ -81,7 +81,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { if (accept) { version_ = version; } else { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)); } updateFile(file_json); } diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 7a515a37f7f12..05b8356ceec5e 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -147,7 +147,9 @@ TEST_F(GrpcMuxImplTest, ResetStream) { expectSendMessage("baz", {"z"}, ""); grpc_mux_->start(); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)).Times(3); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)) + .Times(3); EXPECT_CALL(random_, random()); ASSERT_TRUE(timer != nullptr); // initialized from dispatcher mock. EXPECT_CALL(*timer, enableTimer(_)); @@ -207,10 +209,11 @@ TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { { invalid_response->set_type_url("foo"); invalid_response->mutable_resources()->Add()->set_type_url("bar"); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)).WillOnce(Invoke([](const EnvoyException* e) { - EXPECT_TRUE( - IsSubstring("", "", "bar does not match foo type URL in DiscoveryResponse", e->what())); - })); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)) + .WillOnce(Invoke([](Envoy::Config::ConfigUpdateFailureReason, const EnvoyException* e) { + EXPECT_TRUE(IsSubstring("", "", "bar does not match foo type URL in DiscoveryResponse", + e->what())); + })); expectSendMessage("foo", {"x", "y"}, "", "", Grpc::Status::GrpcStatus::Internal, fmt::format("bar does not match foo type URL in DiscoveryResponse {}", diff --git a/test/common/config/grpc_subscription_impl_test.cc b/test/common/config/grpc_subscription_impl_test.cc index 35991e88717d3..563054feea277 100644 --- a/test/common/config/grpc_subscription_impl_test.cc +++ b/test/common/config/grpc_subscription_impl_test.cc @@ -15,7 +15,8 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { InSequence s; EXPECT_CALL(*async_client_, startRaw(_, _, _)).WillOnce(Return(nullptr)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); EXPECT_CALL(random_, random()); EXPECT_CALL(*timer_, enableTimer(_)); subscription_->start({"cluster0", "cluster1"}); @@ -37,7 +38,8 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { TEST_F(GrpcSubscriptionImplTest, RemoteStreamClose) { startSubscription({"cluster0", "cluster1"}); EXPECT_TRUE(statsAre(1, 0, 0, 0, 0)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); EXPECT_CALL(*timer_, enableTimer(_)); EXPECT_CALL(random_, random()); subscription_->grpcMux().grpcStreamForTest().onRemoteClose(Grpc::Status::GrpcStatus::Canceled, diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index bcbe968620a1d..96a9aae23b6e7 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -105,7 +105,8 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { expectSendMessage(last_cluster_names_, version); version_ = version; } else { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); expectSendMessage(last_cluster_names_, version_, Grpc::Status::GrpcStatus::Internal, "bad config"); } @@ -132,7 +133,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { } void expectConfigUpdateFailed() override { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(nullptr)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, nullptr)); } void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { diff --git a/test/common/config/http_subscription_impl_test.cc b/test/common/config/http_subscription_impl_test.cc index 6cc81becb889f..114597e2fa417 100644 --- a/test/common/config/http_subscription_impl_test.cc +++ b/test/common/config/http_subscription_impl_test.cc @@ -15,7 +15,8 @@ TEST_F(HttpSubscriptionImplTest, OnRequestReset) { startSubscription({"cluster0", "cluster1"}); EXPECT_CALL(random_gen_, random()).WillOnce(Return(0)); EXPECT_CALL(*timer_, enableTimer(_)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); http_callbacks_->onFailure(Http::AsyncClient::FailureReason::Reset); EXPECT_TRUE(statsAre(1, 0, 0, 1, 0)); timerTick(); @@ -32,7 +33,8 @@ TEST_F(HttpSubscriptionImplTest, BadJsonRecovery) { message->body() = std::make_unique(";!@#badjso n"); EXPECT_CALL(random_gen_, random()).WillOnce(Return(0)); EXPECT_CALL(*timer_, enableTimer(_)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); http_callbacks_->onSuccess(std::move(message)); EXPECT_TRUE(statsAre(1, 0, 0, 1, 0)); request_in_progress_ = false; diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index ecf5c056950e1..7c890973ab382 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -139,7 +139,8 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { .WillOnce(ThrowOnRejectedConfig(accept)); } if (!accept) { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); } EXPECT_CALL(random_gen_, random()).WillOnce(Return(0)); EXPECT_CALL(*timer_, enableTimer(_)); @@ -152,7 +153,7 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { } void expectConfigUpdateFailed() override { - EXPECT_CALL(callbacks_, onConfigUpdateFailed(nullptr)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, nullptr)); } void expectEnableInitFetchTimeoutTimer(std::chrono::milliseconds timeout) override { diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index c72b02d146083..c5cd9c4c771ae 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -186,7 +186,7 @@ TEST_F(SubscriptionFactoryTest, FilesystemSubscription) { auto* watcher = new Filesystem::MockWatcher(); EXPECT_CALL(dispatcher_, createFilesystemWatcher_()).WillOnce(Return(watcher)); EXPECT_CALL(*watcher, addWatch(test_path, _, _)); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)); subscriptionFromConfigSource(config)->start({"foo"}); } @@ -302,7 +302,7 @@ TEST_F(SubscriptionFactoryTest, GrpcSubscription) { })); EXPECT_CALL(random_, random()); EXPECT_CALL(dispatcher_, createTimer_(_)).Times(2); - EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_, _)); subscriptionFromConfigSource(config)->start({"static_cluster"}); } diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 303589eb9750c..e84bb925b367c 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -256,7 +256,8 @@ TEST_F(RdsImplTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - rds_callbacks_->onConfigUpdateFailed({}); + rds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + {}); } class RouteConfigProviderManagerImplTest : public RdsTestBase { diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 1476dac9dce0f..30d3a671ed0a9 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -177,7 +177,8 @@ TEST_F(ScopedRdsTest, ConfigUpdateFailure) { timeSystem().setSystemTime(time); const EnvoyException ex(fmt::format("config failure")); // Verify the failure updates the lastUpdated() timestamp. - subscription_callbacks_->onConfigUpdateFailed(&ex); + subscription_callbacks_->onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &ex); EXPECT_EQ(std::chrono::time_point_cast(provider_->lastUpdated()) .time_since_epoch(), time); diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 41f80ba3b3601..dc3a0c34faee4 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -828,7 +828,8 @@ TEST_F(RtdsLoaderImplTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - rtds_callbacks_[0]->onConfigUpdateFailed({}); + rtds_callbacks_[0]->onConfigUpdateFailed( + Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, {}); 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 da5d5a532a0c5..72c0aeb6fed2f 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -355,7 +355,8 @@ TEST_F(CdsApiImplTest, FailureSubscription) { setup(); EXPECT_CALL(initialized_, ready()); - cds_callbacks_->onConfigUpdateFailed({}); + cds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + {}); EXPECT_EQ("", cds_->versionInfo()); } diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index a3909a8ca79c3..988df7fc7ee4c 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -210,6 +210,14 @@ 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; @@ -217,8 +225,12 @@ TEST_F(EdsTest, OnConfigUpdateWrongName) { Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); initialize(); - EXPECT_THROW(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException); - eds_callbacks_->onConfigUpdateFailed(nullptr); + try { + eds_callbacks_->onConfigUpdate(resources, ""); + } catch (const EnvoyException& e) { + eds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, + &e); + } EXPECT_TRUE(initialized_); } @@ -241,8 +253,12 @@ TEST_F(EdsTest, OnConfigUpdateWrongSize) { Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(cluster_load_assignment); resources.Add()->PackFrom(cluster_load_assignment); - EXPECT_THROW(eds_callbacks_->onConfigUpdate(resources, ""), EnvoyException); - eds_callbacks_->onConfigUpdateFailed(nullptr); + try { + eds_callbacks_->onConfigUpdate(resources, ""); + } catch (const EnvoyException& e) { + eds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, + &e); + } EXPECT_TRUE(initialized_); } diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index e87fdff3f6f4f..3954f0aad69fa 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -394,6 +394,17 @@ TEST_P(AdsIntegrationTest, ClusterWarmingOnNamedResponse) { // i,e. no named EDS response. test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 1); + // Disconnect and reconnect the stream. + xds_stream_->finishGrpcStream(Grpc::Status::Internal); + + AssertionResult result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); + RELEASE_ASSERT(result, result.message()); + xds_stream_->startGrpcStream(); + + // Envoy will not finish warming of the second cluster because of the missing load assignments + // i,e. no named EDS response even after disconnect and reconnect. + test_server_->waitForGaugeEq("cluster_manager.warming_clusters", 1); + // Finish warming the second cluster. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index ee7c9291782b3..b579cb56b7221 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -37,7 +37,8 @@ template class MockSubscriptionCallbacks : public Subscript void(const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& system_version_info)); - MOCK_METHOD1_T(onConfigUpdateFailed, void(const EnvoyException* e)); + MOCK_METHOD2_T(onConfigUpdateFailed, + void(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e)); MOCK_METHOD1_T(resourceName, std::string(const ProtobufWkt::Any& resource)); }; @@ -93,7 +94,8 @@ class MockGrpcMuxCallbacks : public GrpcMuxCallbacks { MOCK_METHOD2(onConfigUpdate, void(const Protobuf::RepeatedPtrField& resources, const std::string& version_info)); - MOCK_METHOD1(onConfigUpdateFailed, void(const EnvoyException* e)); + MOCK_METHOD2(onConfigUpdateFailed, + void(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e)); MOCK_METHOD1(resourceName, std::string(const ProtobufWkt::Any& resource)); }; diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index efbd1c5f8f247..3f54aed53551b 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -412,7 +412,8 @@ TEST_F(LdsApiTest, FailureSubscription) { setup(); EXPECT_CALL(init_watcher_, ready()); - lds_callbacks_->onConfigUpdateFailed({}); + lds_callbacks_->onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, + {}); EXPECT_EQ("", lds_->versionInfo()); }