From 4d2c47e48a1014b66661ed85eb5e4735e379aa4f Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Thu, 13 Oct 2022 19:00:15 +0000 Subject: [PATCH 01/31] intitial commit for xds tracer extension point Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 6 +- envoy/config/BUILD | 13 + envoy/config/subscription.h | 5 + envoy/config/xds_config_tracer.h | 81 +++++ source/common/config/BUILD | 3 + source/common/config/decoded_resource_impl.h | 27 +- source/common/config/grpc_mux_impl.cc | 14 +- source/common/config/grpc_mux_impl.h | 3 + source/common/config/new_grpc_mux_impl.cc | 20 +- source/common/config/new_grpc_mux_impl.h | 5 +- .../config/subscription_factory_impl.cc | 10 +- .../common/config/subscription_factory_impl.h | 5 +- source/common/protobuf/BUILD | 1 + .../common/upstream/cluster_manager_impl.cc | 16 +- source/common/upstream/cluster_manager_impl.h | 1 + test/common/config/BUILD | 5 + .../config/delta_subscription_impl_test.cc | 4 +- .../config/delta_subscription_test_harness.h | 4 +- test/common/config/grpc_mux_impl_test.cc | 5 + .../config/grpc_subscription_test_harness.h | 4 +- test/common/config/new_grpc_mux_impl_test.cc | 4 +- .../config/subscription_factory_impl_test.cc | 4 +- .../multi_connection_base_impl_test.cc | 2 +- test/common/upstream/eds_speed_test.cc | 2 + test/integration/BUILD | 24 ++ .../xds_config_tracer_integration_test.cc | 281 ++++++++++++++++++ test/integration/xds_config_tracer_test.proto | 7 + 27 files changed, 524 insertions(+), 32 deletions(-) create mode 100644 envoy/config/xds_config_tracer.h create mode 100644 test/integration/xds_config_tracer_integration_test.cc create mode 100644 test/integration/xds_config_tracer_test.proto diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index 679505815114b..c528ddd2c6901 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -41,7 +41,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // ` for more detail. // Bootstrap :ref:`configuration overview `. -// [#next-free-field: 36] +// [#next-free-field: 37] message Bootstrap { option (udpa.annotations.versioning).previous_message_type = "envoy.config.bootstrap.v2.Bootstrap"; @@ -342,6 +342,10 @@ message Bootstrap { // TODO(abeyad): Add public-facing documentation. // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_delegate_extension = 35; + + // Optional XdsConfigTracer configuration, which allows tracking xDS resources and responses. + // If a value is not specified, no XdsConfigTracer will be used. + core.v3.TypedExtensionConfig xds_config_tracer_extension = 36; } // Administration interface :ref:`operations documentation diff --git a/envoy/config/BUILD b/envoy/config/BUILD index 5f5a02b4b354d..a116ab68386ef 100644 --- a/envoy/config/BUILD +++ b/envoy/config/BUILD @@ -117,3 +117,16 @@ envoy_cc_library( "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", ], ) + +envoy_cc_library( + name = "xds_config_tracer_interface", + hdrs = ["xds_config_tracer.h"], + deps = [ + ":subscription_interface", + ":typed_config_interface", + "//envoy/protobuf:message_validator_interface", + "//source/common/config:update_ack_lib", + "//source/common/protobuf", + "@com_google_googleapis//google/rpc:status_cc_proto", + ], +) diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h index 3d5c629eec53a..7dc61eec381f7 100644 --- a/envoy/config/subscription.h +++ b/envoy/config/subscription.h @@ -59,6 +59,11 @@ class DecodedResource { * @return bool does the xDS discovery response have a set resource payload? */ virtual bool hasResource() const PURE; + + /** + * @return bool does the xDS discovery respon + */ + virtual absl::optional metadata() const PURE; }; using DecodedResourcePtr = std::unique_ptr; diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h new file mode 100644 index 0000000000000..5479eab895ada --- /dev/null +++ b/envoy/config/xds_config_tracer.h @@ -0,0 +1,81 @@ +#pragma once + +#include +#include + +#include "envoy/common/optref.h" +#include "envoy/config/subscription.h" +#include "envoy/config/typed_config.h" +#include "envoy/protobuf/message_validator.h" + +#include "source/common/protobuf/protobuf.h" + +#include "google/rpc/status.pb.h" + +namespace Envoy { +namespace Config { + +// The status of a Delta/DiscoveryResponse. +enum TraceState { + // Successfully got the resources. + RECEIVE = 0, + // Successfully ingest the resouces. + INGESTED = 1, + // Fail to apply the resources. + FAILED = 2, +}; + +struct TraceDetails { + TraceDetails(const TraceState state) : state_(state){}; + TraceDetails(const TraceState state, const ::google::rpc::Status error_detail) + : state_(state), error_detail_(error_detail){}; + const TraceState state_; + ::google::rpc::Status error_detail_; +}; + +/** + * + */ +class XdsConfigTracer { +public: + virtual ~XdsConfigTracer() = default; + + /** + * Log a decode resources. + * @param type_url The type url of xDS message. + * @param resources List of decoded resources that reflect the new state. + * @param details if the config should be rejected. + */ + virtual void log(const absl::string_view type_url, + const std::vector& resources, + const TraceDetails& details) PURE; + + virtual void log(const envoy::service::discovery::v3::DiscoveryResponse& message, + const TraceDetails& details) PURE; + + virtual void log(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, + const TraceDetails& details) PURE; +}; + +using XdsConfigTracerPtr = std::unique_ptr; +using XdsConfigTracerOptRef = OptRef; + +/** + * A factory abstract class for creating instances of ConfigValidators. + */ +class XdsConfigTracerFactory : public Config::TypedFactory { +public: + ~XdsConfigTracerFactory() override = default; + + /** + * Creates a ConfigValidator using the given config. + */ + virtual XdsConfigTracerPtr + createXdsConfigTracer(const ProtobufWkt::Any& config, + ProtobufMessage::ValidationVisitor& validation_visitor) PURE; + + std::string category() const override { return "envoy.config.xds_tracers"; } +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index d9f1154263626..b022cd750dc26 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -166,6 +166,7 @@ envoy_cc_library( ":xds_source_id_lib", "//envoy/config:grpc_mux_interface", "//envoy/config:subscription_interface", + "//envoy/config:xds_config_tracer_interface", "//envoy/config:xds_resources_delegate_interface", "//envoy/upstream:cluster_manager_interface", "//source/common/common:cleanup_lib", @@ -203,6 +204,7 @@ envoy_cc_library( ":watch_map_lib", ":xds_context_params_lib", ":xds_resource_lib", + "//envoy/config:xds_config_tracer_interface", "//envoy/event:dispatcher_interface", "//envoy/grpc:async_client_interface", "//source/common/memory:utils_lib", @@ -329,6 +331,7 @@ envoy_cc_library( ":xds_resource_lib", "//envoy/config:subscription_factory_interface", "//envoy/config:subscription_interface", + "//envoy/config:xds_config_tracer_interface", "//envoy/config:xds_resources_delegate_interface", "//envoy/server:instance_interface", "//envoy/upstream:cluster_manager_interface", diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index 65c5d9dc6a0c6..ba0fa18103c8b 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -40,7 +40,7 @@ class DecodedResourceImpl : public DecodedResource { return std::unique_ptr(new DecodedResourceImpl( resource_decoder, absl::nullopt, Protobuf::RepeatedPtrField(), resource, true, - version, absl::nullopt)); + version, absl::nullopt, absl::nullopt)); } static DecodedResourceImplPtr @@ -51,21 +51,22 @@ class DecodedResourceImpl : public DecodedResource { DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, const envoy::service::discovery::v3::Resource& resource) - : DecodedResourceImpl(resource_decoder, resource.name(), resource.aliases(), - resource.resource(), resource.has_resource(), resource.version(), - resource.has_ttl() - ? absl::make_optional(std::chrono::milliseconds( - DurationUtil::durationToMilliseconds(resource.ttl()))) - : absl::nullopt) {} + : DecodedResourceImpl( + resource_decoder, resource.name(), resource.aliases(), resource.resource(), + resource.has_resource(), resource.version(), + resource.has_ttl() ? absl::make_optional(std::chrono::milliseconds( + DurationUtil::durationToMilliseconds(resource.ttl()))) + : absl::nullopt, + resource.has_metadata() ? absl::make_optional(resource.metadata()) : absl::nullopt) {} DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, const xds::core::v3::CollectionEntry::InlineEntry& inline_entry) : DecodedResourceImpl(resource_decoder, inline_entry.name(), Protobuf::RepeatedPtrField(), inline_entry.resource(), - true, inline_entry.version(), absl::nullopt) {} + true, inline_entry.version(), absl::nullopt, absl::nullopt) {} DecodedResourceImpl(ProtobufTypes::MessagePtr resource, const std::string& name, const std::vector& aliases, const std::string& version) : resource_(std::move(resource)), has_resource_(true), name_(name), aliases_(aliases), - version_(version), ttl_(absl::nullopt) {} + version_(version), ttl_(absl::nullopt), metadata_(absl::nullopt) {} // Config::DecodedResource const std::string& name() const override { return name_; } @@ -74,15 +75,18 @@ class DecodedResourceImpl : public DecodedResource { const Protobuf::Message& resource() const override { return *resource_; }; bool hasResource() const override { return has_resource_; } absl::optional ttl() const override { return ttl_; } + absl::optional metadata() const override { return metadata_; } private: DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, absl::optional name, const Protobuf::RepeatedPtrField& aliases, const ProtobufWkt::Any& resource, bool has_resource, - const std::string& version, absl::optional ttl) + const std::string& version, absl::optional ttl, + const absl::optional metadata) : resource_(resource_decoder.decodeResource(resource)), has_resource_(has_resource), name_(name ? *name : resource_decoder.resourceName(*resource_)), - aliases_(repeatedPtrFieldToVector(aliases)), version_(version), ttl_(ttl) {} + aliases_(repeatedPtrFieldToVector(aliases)), version_(version), ttl_(ttl), + metadata_(metadata) {} const ProtobufTypes::MessagePtr resource_; const bool has_resource_; @@ -91,6 +95,7 @@ class DecodedResourceImpl : public DecodedResource { const std::string version_; // Per resource TTL. const absl::optional ttl_; + const absl::optional metadata_; }; struct DecodedResourcesWrapper { diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 53c0d01135e5c..1b7d99889068b 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -39,12 +39,13 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, CustomConfigValidatorsPtr&& config_validators, + XdsConfigTracerOptRef xds_config_tracer, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), local_info_(local_info), skip_subsequent_node_(skip_subsequent_node), - config_validators_(std::move(config_validators)), + config_validators_(std::move(config_validators)), xds_config_tracer_(xds_config_tracer), xds_resources_delegate_(xds_resources_delegate), target_xds_authority_(target_xds_authority), first_stream_request_(true), dispatcher_(dispatcher), dynamic_update_callback_handle_(local_info.contextProvider().addDynamicContextUpdateCallback( @@ -284,8 +285,16 @@ void GrpcMuxImpl::onDiscoveryResponse( } } + if (xds_config_tracer_.has_value()) { + xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::RECEIVE)); + } + processDiscoveryResources(resources, api_state, type_url, message->version_info(), /*call_delegate=*/true); + + if (xds_config_tracer_.has_value()) { + xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::INGESTED)); + } } END_TRY catch (const EnvoyException& e) { @@ -296,6 +305,9 @@ void GrpcMuxImpl::onDiscoveryResponse( ::google::rpc::Status* error_detail = api_state.request_.mutable_error_detail(); error_detail->set_code(Grpc::Status::WellKnownGrpcStatus::Internal); error_detail->set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); + if (xds_config_tracer_.has_value()) { + xds_config_tracer_->log(*message, TraceDetails(TraceState::FAILED, *error_detail)); + } } previously_fetched_data_ = true; api_state.request_.set_response_nonce(message->nonce()); diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index ed1dd60dd6cb1..47b6123c7a245 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -8,6 +8,7 @@ #include "envoy/common/time.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" @@ -39,6 +40,7 @@ class GrpcMuxImpl : public GrpcMux, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, CustomConfigValidatorsPtr&& config_validators, + XdsConfigTracerOptRef xds_config_tracer, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority); @@ -182,6 +184,7 @@ class GrpcMuxImpl : public GrpcMux, const LocalInfo::LocalInfo& local_info_; const bool skip_subsequent_node_; CustomConfigValidatorsPtr config_validators_; + XdsConfigTracerOptRef xds_config_tracer_; XdsResourcesDelegateOptRef xds_resources_delegate_; const std::string target_xds_authority_; bool first_stream_request_; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 56d7ae05109b0..b2be1385dbc75 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -40,7 +40,8 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, - CustomConfigValidatorsPtr&& config_validators) + CustomConfigValidatorsPtr&& config_validators, + XdsConfigTracerOptRef xds_config_tracer) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), local_info_(local_info), config_validators_(std::move(config_validators)), @@ -48,7 +49,7 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, [this](absl::string_view resource_type_url) { onDynamicContextUpdate(resource_type_url); })), - dispatcher_(dispatcher) { + dispatcher_(dispatcher), xds_config_tracer_(xds_config_tracer) { AllMuxes::get().insert(this); } @@ -89,6 +90,11 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ControlPlaneStats& control_plane_stats) { ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); + + if (xds_config_tracer_.has_value()) { + xds_config_tracer_->log(*message, TraceDetails(TraceState::RECEIVE)); + } + auto sub = subscriptions_.find(message->type_url()); if (sub == subscriptions_.end()) { ENVOY_LOG(warn, @@ -108,7 +114,15 @@ void NewGrpcMuxImpl::onDiscoveryResponse( } } - kickOffAck(sub->second->sub_state_.handleResponse(*message)); + auto ack = sub->second->sub_state_.handleResponse(*message); + if (xds_config_tracer_.has_value()) { + xds_config_tracer_->log( + *message, TraceDetails(ack.error_detail_.code() == Grpc::Status::WellKnownGrpcStatus::Ok + ? TraceState::INGESTED + : TraceState::FAILED, + ack.error_detail_)); + } + kickOffAck(ack); Memory::Utils::tryShrinkHeap(); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index bcf8dfee0a464..57ba603d87833 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -6,6 +6,7 @@ #include "envoy/common/token_bucket.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/common/logger.h" @@ -34,7 +35,8 @@ class NewGrpcMuxImpl const Protobuf::MethodDescriptor& service_method, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, - CustomConfigValidatorsPtr&& config_validators); + CustomConfigValidatorsPtr&& config_validators, + XdsConfigTracerOptRef xds_config_tracer); ~NewGrpcMuxImpl() override; @@ -178,6 +180,7 @@ class NewGrpcMuxImpl CustomConfigValidatorsPtr config_validators_; Common::CallbackHandlePtr dynamic_update_callback_handle_; Event::Dispatcher& dispatcher_; + XdsConfigTracerOptRef xds_config_tracer_; // True iff Envoy is shutting down; no messages should be sent on the `grpc_stream_` when this is // true because it may contain dangling pointers. diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index 7ed7682a5fdb6..6ad2833c9634d 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -24,10 +24,10 @@ SubscriptionFactoryImpl::SubscriptionFactoryImpl( const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, const Server::Instance& server, - XdsResourcesDelegateOptRef xds_resources_delegate) + XdsResourcesDelegateOptRef xds_resources_delegate, XdsConfigTracerOptRef xds_config_tracer) : local_info_(local_info), dispatcher_(dispatcher), cm_(cm), validation_visitor_(validation_visitor), api_(api), server_(server), - xds_resources_delegate_(xds_resources_delegate) {} + xds_resources_delegate_(xds_resources_delegate), xds_config_tracer_(xds_config_tracer) {} SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, @@ -98,7 +98,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( dispatcher_, sotwGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), api_config_source.set_node_on_first_message_only(), std::move(custom_config_validators), - xds_resources_delegate_, control_plane_id); + xds_config_tracer_, xds_resources_delegate_, control_plane_id); } return std::make_unique( std::move(mux), callbacks, resource_decoder, stats, type_url, dispatcher_, @@ -126,7 +126,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ->createUncachedRawAsyncClient(), dispatcher_, deltaGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, - std::move(custom_config_validators)); + std::move(custom_config_validators), xds_config_tracer_); } return std::make_unique( std::move(mux), callbacks, resource_decoder, stats, type_url, dispatcher_, @@ -192,7 +192,7 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( ->createUncachedRawAsyncClient(), dispatcher_, deltaGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, - std::move(custom_config_validators)), + std::move(custom_config_validators), xds_config_tracer_), callbacks, resource_decoder, stats, dispatcher_, Utility::configSourceInitialFetchTimeout(config), false, options); } diff --git a/source/common/config/subscription_factory_impl.h b/source/common/config/subscription_factory_impl.h index 8bbbaa2219731..7053cbe8d73a3 100644 --- a/source/common/config/subscription_factory_impl.h +++ b/source/common/config/subscription_factory_impl.h @@ -5,6 +5,7 @@ #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/config/subscription.h" #include "envoy/config/subscription_factory.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/server/instance.h" #include "envoy/stats/scope.h" @@ -21,7 +22,8 @@ class SubscriptionFactoryImpl : public SubscriptionFactory, Logger::Loggable( + bootstrap.xds_config_tracer_extension()); + xds_config_tracer_ = + tracer_factory.createXdsConfigTracer(bootstrap.xds_config_tracer_extension().typed_config(), + validation_context.dynamicValidationVisitor()); + } + subscription_factory_ = std::make_unique( local_info, main_thread_dispatcher, *this, validation_context.dynamicValidationVisitor(), api, - server, makeOptRefFromPtr(xds_resources_delegate_.get())); + server, makeOptRefFromPtr(xds_resources_delegate_.get()), + makeOptRefFromPtr(xds_config_tracer_.get())); const auto& dyn_resources = bootstrap.dynamic_resources(); @@ -399,7 +408,7 @@ ClusterManagerImpl::ClusterManagerImpl( "envoy.service.discovery.v3.AggregatedDiscoveryService.DeltaAggregatedResources"), random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info, - std::move(custom_config_validators)); + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracer_.get())); } } else { Config::Utility::checkTransportVersion(dyn_resources.ads_config()); @@ -432,7 +441,8 @@ ClusterManagerImpl::ClusterManagerImpl( random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only(), - std::move(custom_config_validators), xds_delegate_opt_ref, target_xds_authority); + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracer_.get()), + xds_delegate_opt_ref, target_xds_authority); } } } else { diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index a779bef35ad1a..892097b84b282 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -774,6 +774,7 @@ class ClusterManagerImpl : public ClusterManager, ClusterSet primary_clusters_; std::unique_ptr xds_resources_delegate_; + std::unique_ptr xds_config_tracer_; }; } // namespace Upstream diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 818964c0ca805..1dc0075a3b4af 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -36,6 +36,7 @@ envoy_cc_test( srcs = ["delta_subscription_impl_test.cc"], deps = [ ":delta_subscription_test_harness", + "//envoy/config:xds_config_tracer_interface", "//source/common/config:api_version_lib", "//source/common/config:grpc_subscription_lib", "//source/common/config:new_grpc_mux_lib", @@ -153,6 +154,7 @@ envoy_cc_test( name = "grpc_mux_impl_test", srcs = ["grpc_mux_impl_test.cc"], deps = [ + "//envoy/config:xds_config_tracer_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/config:api_version_lib", "//source/common/config:grpc_mux_lib", @@ -262,6 +264,7 @@ envoy_cc_test_library( hdrs = ["grpc_subscription_test_harness.h"], deps = [ ":subscription_test_harness", + "//envoy/config:xds_config_tracer_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/common:hash_lib", "//source/common/config:api_version_lib", @@ -287,6 +290,7 @@ envoy_cc_test_library( hdrs = ["delta_subscription_test_harness.h"], deps = [ ":subscription_test_harness", + "//envoy/config:xds_config_tracer_interface", "//source/common/common:utility_lib", "//source/common/config:new_grpc_mux_lib", "//source/common/config/xds_mux:grpc_mux_lib", @@ -349,6 +353,7 @@ envoy_cc_test( name = "subscription_factory_impl_test", srcs = ["subscription_factory_impl_test.cc"], deps = [ + "//envoy/config:xds_config_tracer_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/config:subscription_factory_lib", "//source/common/config:xds_resource_lib", diff --git a/test/common/config/delta_subscription_impl_test.cc b/test/common/config/delta_subscription_impl_test.cc index 18a578b20be9e..afe4d62258845 100644 --- a/test/common/config/delta_subscription_impl_test.cc +++ b/test/common/config/delta_subscription_impl_test.cc @@ -1,5 +1,6 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/buffer/zero_copy_input_stream_impl.h" @@ -157,7 +158,8 @@ TEST_P(DeltaSubscriptionNoGrpcStreamTest, NoGrpcStream) { xds_context = std::make_shared( std::unique_ptr(async_client), dispatcher, *method_descriptor, random, stats_store, rate_limit_settings, local_info, - std::make_unique>()); + std::make_unique>(), + /*xds_config_tracer=*/XdsConfigTracerOptRef()); } GrpcSubscriptionImplPtr subscription = std::make_unique( diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index e3cfd7fc3fe01..b2bcbed084862 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -5,6 +5,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/config/grpc_subscription_impl.h" @@ -56,7 +57,8 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { xds_context_ = std::make_shared( std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, local_info_, - std::make_unique>()); + std::make_unique>(), + /*xds_config_tracer=*/XdsConfigTracerOptRef()); } subscription_ = std::make_unique( xds_context_, callbacks_, resource_decoder_, stats_, diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index fe0540bb474b4..71e784ee08e72 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -2,6 +2,7 @@ #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -64,6 +65,7 @@ class GrpcMuxImplTestBase : public testing::Test { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::move(config_validators_), + /*xds_config_tracer=*/XdsConfigTracerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } @@ -73,6 +75,7 @@ class GrpcMuxImplTestBase : public testing::Test { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, custom_rate_limit_settings, true, std::move(config_validators_), + /*xds_config_tracer=*/XdsConfigTracerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } @@ -893,6 +896,7 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::make_unique>(), + /*xds_config_tracer=*/XdsConfigTracerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " @@ -908,6 +912,7 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::make_unique>(), + /*xds_config_tracer=*/XdsConfigTracerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index c30f5ebeef649..f95763f2d5a78 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -5,6 +5,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -64,7 +65,8 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { mux_ = std::make_shared( local_info_, std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, true, - std::move(config_validators_), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), + std::move(config_validators_), /*xds_config_tracer=*/XdsConfigTracerOptRef(), + /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } subscription_ = std::make_unique( diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 9ce1ad7222d85..62e4f52fccaf6 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -2,6 +2,7 @@ #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/event/timer.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -72,7 +73,8 @@ class NewGrpcMuxImplTestBase : public testing::TestWithParam { std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), - random_, stats_, rate_limit_settings_, local_info_, std::move(config_validators_)); + random_, stats_, rate_limit_settings_, local_info_, std::move(config_validators_), + /*xds_config_tracer=*/XdsConfigTracerOptRef()); } void expectSendMessage(const std::string& type_url, diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index 89a49e42e0690..2793869eea06f 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -6,6 +6,7 @@ #include "envoy/config/core/v3/config_source.pb.validate.h" #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/stats/scope.h" @@ -47,7 +48,8 @@ class SubscriptionFactoryTest : public testing::Test { http_request_(&cm_.thread_local_cluster_.async_client_), api_(Api::createApiForTest(stats_store_, random_)), subscription_factory_(local_info_, dispatcher_, cm_, validation_visitor_, *api_, server_, - /*xds_resources_delegate=*/XdsResourcesDelegateOptRef()) {} + /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), + /*xds_config_tracer=*/XdsConfigTracerOptRef()) {} SubscriptionPtr subscriptionFromConfigSource(const envoy::config::core::v3::ConfigSource& config) { diff --git a/test/common/network/multi_connection_base_impl_test.cc b/test/common/network/multi_connection_base_impl_test.cc index 034a40fbc07c2..326b0d829a299 100644 --- a/test/common/network/multi_connection_base_impl_test.cc +++ b/test/common/network/multi_connection_base_impl_test.cc @@ -76,7 +76,7 @@ class MultiConnectionBaseImplTest : public testing::Test { void expectConnectionCreation(size_t id) { EXPECT_CALL(*connection_provider_, createNextConnection(_)) - .WillOnce(testing::Invoke([ this, id ](uint64_t conn_id) -> auto{ + .WillOnce(testing::Invoke([ this, id ](uint64_t conn_id) -> auto { EXPECT_EQ(connection_provider_->nextConnection(), id); return connection_provider_->getNextConnection(conn_id); })); diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 613310534192c..13492abace79d 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -5,6 +5,7 @@ #include "envoy/config/core/v3/health_check.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint_components.pb.h" +#include "envoy/config/xds_config_tracer.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/stats/scope.h" @@ -61,6 +62,7 @@ class EdsSpeedTest { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.endpoint.v3.EndpointDiscoveryService.StreamEndpoints"), random_, stats_, {}, true, std::move(config_validators_), + /*xds_config_tracer=*/Config::XdsConfigTracerOptRef(), /*xds_resources_delegate=*/Config::XdsResourcesDelegateOptRef(), /*target_xds_authority=*/"")); } diff --git a/test/integration/BUILD b/test/integration/BUILD index 8bc85eed256db..25e9d4aa7daca 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -2030,3 +2030,27 @@ envoy_proto_library( name = "xds_delegate_test_config_proto", srcs = ["xds_delegate_test_config.proto"], ) + +envoy_cc_test( + name = "xds_config_tracer_integration_test", + srcs = ["xds_config_tracer_integration_test.cc"], + external_deps = ["abseil_strings"], + deps = [ + ":http_integration_lib", + ":xds_config_tracer_test_proto_cc_proto", + "//envoy/config:subscription_interface", + "//envoy/config:xds_config_tracer_interface", + "//envoy/protobuf:message_validator_interface", + "//test/common/grpc:grpc_client_integration_lib", + "//test/config:v2_link_hacks", + "//test/test_common:registry_lib", + "@envoy_api//envoy/config/route/v3:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", + "@envoy_api//envoy/service/runtime/v3:pkg_cc_proto", + ], +) + +envoy_proto_library( + name = "xds_config_tracer_test_proto", + srcs = ["xds_config_tracer_test.proto"], +) diff --git a/test/integration/xds_config_tracer_integration_test.cc b/test/integration/xds_config_tracer_integration_test.cc new file mode 100644 index 0000000000000..8bf00cee8c511 --- /dev/null +++ b/test/integration/xds_config_tracer_integration_test.cc @@ -0,0 +1,281 @@ +#include + +#include "envoy/config/route/v3/route.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracer.h" +#include "envoy/protobuf/message_validator.h" +#include "envoy/service/discovery/v3/discovery.pb.h" +#include "envoy/service/runtime/v3/rtds.pb.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/config/v2_link_hacks.h" +#include "test/integration/http_integration.h" +#include "test/integration/xds_config_tracer_test.pb.h" +#include "test/test_common/registry.h" +#include "test/test_common/utility.h" + +#include "absl/synchronization/mutex.h" +#include "gtest/gtest.h" + +using testing::HasSubstr; + +namespace Envoy { +namespace { + +constexpr char XDS_CLUSTER_NAME[] = "xds_cluster"; + +class TestXdsConfigTracer : public Config::XdsConfigTracer { +public: + TestXdsConfigTracer() { + ReceiveCount = 0; + IngestedCount = 0; + FailureCount = 0; + ErrorMessage = ""; + } + + void log(const absl::string_view, const std::vector&, + const Config::TraceDetails& trace_details) override { + countState(trace_details.state_); + } + + void log(const envoy::service::discovery::v3::DiscoveryResponse&, + const Config::TraceDetails& trace_details) override { + countState(trace_details.state_); + if (trace_details.state_ == Config::TraceState::FAILED) { + ErrorMessage = trace_details.error_detail_.message(); + std::cout << "Boteng " << ErrorMessage << std::endl; + } + } + + void log(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, + const Config::TraceDetails& trace_details) override { + countState(trace_details.state_); + } + + // static std::string getFailedMessage() { return error_message_; }; + + static std::atomic ReceiveCount; + static std::atomic IngestedCount; + static std::atomic FailureCount; + static std::string ErrorMessage; + +private: + void countState(const Config::TraceState& state) { + switch (state) { + case Config::TraceState::RECEIVE: + ++ReceiveCount; + case Config::TraceState::INGESTED: + ++IngestedCount; + case Config::TraceState::FAILED: + ++FailureCount; + }; + } +}; + +std::atomic TestXdsConfigTracer::ReceiveCount; +std::atomic TestXdsConfigTracer::IngestedCount; +std::atomic TestXdsConfigTracer::FailureCount; +std::string TestXdsConfigTracer::ErrorMessage; + +class TestXdsConfigTracerFactory : public Config::XdsConfigTracerFactory { +public: + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return std::make_unique(); + } + + std::string name() const override { return "envoy.config.xds.test_xds_tracer"; }; + + Config::XdsConfigTracerPtr createXdsConfigTracer(const ProtobufWkt::Any&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_unique(); + } +}; + +class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, + public HttpIntegrationTest { +public: + XdsConfigTracerIntegrationTest() + : HttpIntegrationTest(Http::CodecType::HTTP2, ipVersion(), + ConfigHelper::baseConfigNoListeners()) { + + use_lds_ = false; + create_xds_upstream_ = true; + + // Make the default cluster HTTP2. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + ConfigHelper::setHttp2(*bootstrap.mutable_static_resources()->mutable_clusters(0)); + }); + + // Build and add the xDS cluster config. + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* xds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + xds_cluster->MergeFrom(ConfigHelper::buildStaticCluster( + std::string(XDS_CLUSTER_NAME), + /*port=*/0, ipVersion() == Network::Address::IpVersion::v4 ? "127.0.0.1" : "::1")); + ConfigHelper::setHttp2(*xds_cluster); + }); + + // Add static runtime values. + config_helper_.addRuntimeOverride("whatevs", "yar"); + + // Set up the RTDS runtime layer. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* layer = bootstrap.mutable_layered_runtime()->add_layers(); + layer->set_name("some_rtds_layer"); + auto* rtds_layer = layer->mutable_rtds_layer(); + rtds_layer->set_name("some_rtds_layer"); + auto* rtds_config = rtds_layer->mutable_rtds_config(); + rtds_config->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); + auto* api_config_source = rtds_config->mutable_api_config_source(); + api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); + api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); + api_config_source->set_set_node_on_first_message_only(true); + api_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name( + XDS_CLUSTER_NAME); + }); + + // Add test xDS config tracer. + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* tracer_extension = bootstrap.mutable_xds_config_tracer_extension(); + tracer_extension->set_name("envoy.config.xds.test_xds_tracer"); + tracer_extension->mutable_typed_config()->PackFrom( + test::envoy::config::xds::TestXdsConfigTracer()); + }); + } + + void TearDown() override { + if (xds_connection_ != nullptr) { + cleanUpXdsConnection(); + } + } + + void initialize() override { + // The tests infra expects the xDS server to be the second fake upstream, so + // we need a dummy data plane cluster. + setUpstreamCount(1); + setUpstreamProtocol(Http::CodecType::HTTP2); + HttpIntegrationTest::initialize(); + registerTestServerPorts({}); + initial_load_success_ = test_server_->counter("runtime.load_success")->value(); + } + + void acceptXdsConnection() { + createXdsConnection(); + AssertionResult result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); + RELEASE_ASSERT(result, result.message()); + xds_stream_->startGrpcStream(); + } + + std::string getRuntimeKey(const std::string& key) { + auto response = IntegrationUtil::makeSingleRequest( + lookupPort("admin"), "GET", "/runtime?format=json", "", downstreamProtocol(), version_); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(response->body()); + auto entries = loader->getObject("entries"); + if (entries->hasObject(key)) { + return entries->getObject(key)->getString("final_value"); + } + return ""; + } + + void waitforReceiveCount(const int expected_count) { + absl::MutexLock l(&lock_); + const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { + return TestXdsConfigTracer::ReceiveCount == expected_count; + }; + timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), + TestUtility::DefaultTimeout); + } + + void waitforFailureCount(const int expected_count) { + absl::MutexLock l(&lock_); + const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { + return TestXdsConfigTracer::FailureCount >= expected_count; + }; + timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), + TestUtility::DefaultTimeout); + } + + absl::Mutex lock_; + uint32_t initial_load_success_{0}; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTracerIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerSuccessCount) { + TestXdsConfigTracerFactory factory; + Registry::InjectFactory registered(factory); + + initialize(); + acceptXdsConnection(); + + int current_receive_count = TestXdsConfigTracer::ReceiveCount; + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, + {"some_rtds_layer"}, {}, true)); + auto some_rtds_layer = TestUtility::parseYaml(R"EOF( + name: some_rtds_layer + layer: + foo: bar + baz: meh + )EOF"); + sendDiscoveryResponse( + Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); + test_server_->waitForCounterGe("runtime.load_success", initial_load_success_ + 1); + int expected_receive_count = ++current_receive_count; + waitforReceiveCount(expected_receive_count); + + EXPECT_EQ(expected_receive_count, TestXdsConfigTracer::ReceiveCount); + EXPECT_EQ("bar", getRuntimeKey("foo")); + EXPECT_EQ("meh", getRuntimeKey("baz")); + EXPECT_TRUE(TestXdsConfigTracer::IngestedCount == 2); +} + +TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerFailureCount) { + TestXdsConfigTracerFactory factory; + Registry::InjectFactory registered(factory); + + initialize(); + acceptXdsConnection(); + + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, + {"some_rtds_layer"}, {}, true)); + auto some_rtds_layer = TestUtility::parseYaml(R"EOF( + name: different_rtds_layer + layer: + foo: bar + baz: meh + )EOF"); + + auto route_config = TestUtility::parseYaml(R"EOF( + name: my_route + vhds: + config_source: + resource_api_version: V3 + api_config_source: + api_type: GRPC + transport_api_version: V3 + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + FakeStream* stream = xds_stream_.get(); + envoy::service::discovery::v3::DiscoveryResponse discovery_response; + discovery_response.set_version_info("1"); + discovery_response.set_type_url(Config::TypeUrl::get().Runtime); + discovery_response.add_resources()->PackFrom(route_config); + + static int next_nonce_counter = 0; + discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); + + // Message's TypeUrl != Resource's + stream->sendGrpcMessage(discovery_response); + waitforFailureCount(1); + EXPECT_THAT(TestXdsConfigTracer::ErrorMessage, + HasSubstr("does not match the message-wide type URL")); +} + +} // namespace +} // namespace Envoy diff --git a/test/integration/xds_config_tracer_test.proto b/test/integration/xds_config_tracer_test.proto new file mode 100644 index 0000000000000..aa29431b322a8 --- /dev/null +++ b/test/integration/xds_config_tracer_test.proto @@ -0,0 +1,7 @@ +syntax = "proto3"; + +package test.envoy.config.xds; + +// Configuration for TestXdsResourcesDelegate. +message TestXdsConfigTracer { +} From b5234d2479625b308ca6663f19bf68c1150fd72e Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Thu, 13 Oct 2022 19:48:52 +0000 Subject: [PATCH 02/31] clean up Signed-off-by: Boteng Yao --- source/common/protobuf/BUILD | 1 - test/common/network/multi_connection_base_impl_test.cc | 2 +- test/integration/xds_config_tracer_integration_test.cc | 3 --- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index 4ccc026f35a69..fbb16cb51eb12 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -1,4 +1,3 @@ -load("//tools/build_defs/proto/cpp:cc_proto_library.bzl", "cc_proto_library") load("@rules_proto//proto:defs.bzl", "proto_library") load( "//bazel:envoy_build_system.bzl", diff --git a/test/common/network/multi_connection_base_impl_test.cc b/test/common/network/multi_connection_base_impl_test.cc index 326b0d829a299..034a40fbc07c2 100644 --- a/test/common/network/multi_connection_base_impl_test.cc +++ b/test/common/network/multi_connection_base_impl_test.cc @@ -76,7 +76,7 @@ class MultiConnectionBaseImplTest : public testing::Test { void expectConnectionCreation(size_t id) { EXPECT_CALL(*connection_provider_, createNextConnection(_)) - .WillOnce(testing::Invoke([ this, id ](uint64_t conn_id) -> auto { + .WillOnce(testing::Invoke([ this, id ](uint64_t conn_id) -> auto{ EXPECT_EQ(connection_provider_->nextConnection(), id); return connection_provider_->getNextConnection(conn_id); })); diff --git a/test/integration/xds_config_tracer_integration_test.cc b/test/integration/xds_config_tracer_integration_test.cc index 8bf00cee8c511..6e7e8a4239a63 100644 --- a/test/integration/xds_config_tracer_integration_test.cc +++ b/test/integration/xds_config_tracer_integration_test.cc @@ -43,7 +43,6 @@ class TestXdsConfigTracer : public Config::XdsConfigTracer { countState(trace_details.state_); if (trace_details.state_ == Config::TraceState::FAILED) { ErrorMessage = trace_details.error_detail_.message(); - std::cout << "Boteng " << ErrorMessage << std::endl; } } @@ -52,8 +51,6 @@ class TestXdsConfigTracer : public Config::XdsConfigTracer { countState(trace_details.state_); } - // static std::string getFailedMessage() { return error_message_; }; - static std::atomic ReceiveCount; static std::atomic IngestedCount; static std::atomic FailureCount; From ba27917121a4b28dd79154a6d82b647d4cf6dc6c Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Thu, 13 Oct 2022 19:54:01 +0000 Subject: [PATCH 03/31] clean up Signed-off-by: Boteng Yao --- test/integration/xds_config_tracer_test.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/xds_config_tracer_test.proto b/test/integration/xds_config_tracer_test.proto index aa29431b322a8..dd074b930483d 100644 --- a/test/integration/xds_config_tracer_test.proto +++ b/test/integration/xds_config_tracer_test.proto @@ -2,6 +2,6 @@ syntax = "proto3"; package test.envoy.config.xds; -// Configuration for TestXdsResourcesDelegate. +// Configuration for TestXdsConfigTracer. message TestXdsConfigTracer { } From 43b59833592d408859d65944e0b04ae070baf2fe Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Mon, 24 Oct 2022 20:48:23 +0000 Subject: [PATCH 04/31] add some docs Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 4 ++- envoy/config/xds_config_tracer.h | 28 +++++++++++++++---- .../xds_config_tracer_integration_test.cc | 4 ++- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index c528ddd2c6901..f22a92ec40887 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -343,8 +343,10 @@ message Bootstrap { // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_delegate_extension = 35; - // Optional XdsConfigTracer configuration, which allows tracking xDS resources and responses. + // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, + // e.g., external logger, tracer, or monitor. // If a value is not specified, no XdsConfigTracer will be used. + // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_config_tracer_extension = 36; } diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index 5479eab895ada..f2b8706dd5d8e 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -17,7 +17,7 @@ namespace Config { // The status of a Delta/DiscoveryResponse. enum TraceState { - // Successfully got the resources. + // Successfully got the resources or message. RECEIVE = 0, // Successfully ingest the resouces. INGESTED = 1, @@ -34,25 +34,41 @@ struct TraceDetails { }; /** - * + * An interface for hooking into xDS update events to provide the ablility to use some + * external logger or processor in xDS update. + * + * Instance of this interface get invoked on the main Envoy thread. Thus, it is important + * for implementations of this interface to not execute any blocking operations on the same + * thread. */ class XdsConfigTracer { public: virtual ~XdsConfigTracer() = default; /** - * Log a decode resources. + * Log the decoded SotW xDS resources that are about to ingested. * @param type_url The type url of xDS message. * @param resources List of decoded resources that reflect the new state. - * @param details if the config should be rejected. + * @param details The log point state and details. */ virtual void log(const absl::string_view type_url, const std::vector& resources, const TraceDetails& details) PURE; + /** + * Log point for SotW xDS discovery response. + * applied on the Envoy instance, and are about to be ACK'ed. + * @param message The SotW discovery response message body. + * @param details The log point state and details. + */ virtual void log(const envoy::service::discovery::v3::DiscoveryResponse& message, const TraceDetails& details) PURE; + /** + * Log point for Delta xDS discovery response message. + * @param message The Delta discovery response message body. + * @param details The log point state and details. + */ virtual void log(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, const TraceDetails& details) PURE; }; @@ -61,14 +77,14 @@ using XdsConfigTracerPtr = std::unique_ptr; using XdsConfigTracerOptRef = OptRef; /** - * A factory abstract class for creating instances of ConfigValidators. + * A factory abstract class for creating instances of XdsConfigTracer. */ class XdsConfigTracerFactory : public Config::TypedFactory { public: ~XdsConfigTracerFactory() override = default; /** - * Creates a ConfigValidator using the given config. + * Creates an XdsConfigTracer using the given config. */ virtual XdsConfigTracerPtr createXdsConfigTracer(const ProtobufWkt::Any& config, diff --git a/test/integration/xds_config_tracer_integration_test.cc b/test/integration/xds_config_tracer_integration_test.cc index 6e7e8a4239a63..00ac439ae0b3d 100644 --- a/test/integration/xds_config_tracer_integration_test.cc +++ b/test/integration/xds_config_tracer_integration_test.cc @@ -61,10 +61,13 @@ class TestXdsConfigTracer : public Config::XdsConfigTracer { switch (state) { case Config::TraceState::RECEIVE: ++ReceiveCount; + break; case Config::TraceState::INGESTED: ++IngestedCount; + break; case Config::TraceState::FAILED: ++FailureCount; + break; }; } }; @@ -226,7 +229,6 @@ TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerSuccessCount) { EXPECT_EQ(expected_receive_count, TestXdsConfigTracer::ReceiveCount); EXPECT_EQ("bar", getRuntimeKey("foo")); EXPECT_EQ("meh", getRuntimeKey("baz")); - EXPECT_TRUE(TestXdsConfigTracer::IngestedCount == 2); } TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerFailureCount) { From 12bb4f35e558e05564cd7529c81ae77ffbea71d9 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Mon, 24 Oct 2022 20:58:25 +0000 Subject: [PATCH 05/31] clean up Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 3 ++- envoy/config/xds_config_tracer.h | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index f22a92ec40887..a1ef0d79ab844 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -344,7 +344,8 @@ message Bootstrap { core.v3.TypedExtensionConfig xds_delegate_extension = 35; // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, - // e.g., external logger, tracer, or monitor. + // e.g., external logger, tracer, or monitor. It provides the log point when receive, ingest, or fail to + // process xDS response messages. // If a value is not specified, no XdsConfigTracer will be used. // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_config_tracer_extension = 36; diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index f2b8706dd5d8e..82ce4e2f1b7c3 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -36,7 +36,7 @@ struct TraceDetails { /** * An interface for hooking into xDS update events to provide the ablility to use some * external logger or processor in xDS update. - * + * * Instance of this interface get invoked on the main Envoy thread. Thus, it is important * for implementations of this interface to not execute any blocking operations on the same * thread. @@ -48,7 +48,7 @@ class XdsConfigTracer { /** * Log the decoded SotW xDS resources that are about to ingested. * @param type_url The type url of xDS message. - * @param resources List of decoded resources that reflect the new state. + * @param resources List of decoded resources that reflect the latest state. * @param details The log point state and details. */ virtual void log(const absl::string_view type_url, From 51533cf6fd242a29962bb2199bf17a942e5b0f11 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Mon, 24 Oct 2022 21:04:11 +0000 Subject: [PATCH 06/31] clean up Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 2 +- envoy/config/subscription.h | 2 +- envoy/config/xds_config_tracer.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index a1ef0d79ab844..bac82fd913a49 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -345,7 +345,7 @@ message Bootstrap { // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, // e.g., external logger, tracer, or monitor. It provides the log point when receive, ingest, or fail to - // process xDS response messages. + // process xDS resources and messages. // If a value is not specified, no XdsConfigTracer will be used. // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_config_tracer_extension = 36; diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h index 7dc61eec381f7..6daf2d63f2b50 100644 --- a/envoy/config/subscription.h +++ b/envoy/config/subscription.h @@ -61,7 +61,7 @@ class DecodedResource { virtual bool hasResource() const PURE; /** - * @return bool does the xDS discovery respon + * @return absl::optional of a resource. */ virtual absl::optional metadata() const PURE; }; diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index 82ce4e2f1b7c3..02b0b0ccb3f54 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -15,7 +15,7 @@ namespace Envoy { namespace Config { -// The status of a Delta/DiscoveryResponse. +// The status of processing Delta/DiscoveryResponse. enum TraceState { // Successfully got the resources or message. RECEIVE = 0, From 7ac37488e282c2045a337862826360972631abb3 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Mon, 24 Oct 2022 21:13:01 +0000 Subject: [PATCH 07/31] clean up Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracer.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index 02b0b0ccb3f54..0ed42840cfc07 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -19,9 +19,9 @@ namespace Config { enum TraceState { // Successfully got the resources or message. RECEIVE = 0, - // Successfully ingest the resouces. + // Successfully ingested the resouces. INGESTED = 1, - // Fail to apply the resources. + // Failed to apply the resources. FAILED = 2, }; @@ -46,7 +46,7 @@ class XdsConfigTracer { virtual ~XdsConfigTracer() = default; /** - * Log the decoded SotW xDS resources that are about to ingested. + * Log the decoded SotW xDS resources that are about to be ingested. * @param type_url The type url of xDS message. * @param resources List of decoded resources that reflect the latest state. * @param details The log point state and details. @@ -57,7 +57,6 @@ class XdsConfigTracer { /** * Log point for SotW xDS discovery response. - * applied on the Envoy instance, and are about to be ACK'ed. * @param message The SotW discovery response message body. * @param details The log point state and details. */ @@ -65,7 +64,7 @@ class XdsConfigTracer { const TraceDetails& details) PURE; /** - * Log point for Delta xDS discovery response message. + * Log point for Delta xDS discovery response message when received, ingested, and failed. * @param message The Delta discovery response message body. * @param details The log point state and details. */ From a04823a3a566b33c8702eae2069e32d64dd0785b Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 25 Oct 2022 14:21:28 +0000 Subject: [PATCH 08/31] fix format Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index bac82fd913a49..f52c18d2f3c0d 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -343,7 +343,7 @@ message Bootstrap { // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_delegate_extension = 35; - // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, + // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, // e.g., external logger, tracer, or monitor. It provides the log point when receive, ingest, or fail to // process xDS resources and messages. // If a value is not specified, no XdsConfigTracer will be used. From 2ab302bf261d0404d005077a8892543c88abe0b1 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Fri, 28 Oct 2022 14:23:06 +0000 Subject: [PATCH 09/31] Add more comments Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracer.h | 21 ++++++++++++++++++--- source/common/config/grpc_mux_impl.cc | 4 ++++ source/common/config/new_grpc_mux_impl.cc | 3 +++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index 0ed42840cfc07..c2f36fe7c00ac 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -35,7 +35,9 @@ struct TraceDetails { /** * An interface for hooking into xDS update events to provide the ablility to use some - * external logger or processor in xDS update. + * external logger or processor in xDS update. This tracer provides the log point when + * the resources are received , when they are successfully processed and applied, and when + * there is any failure. * * Instance of this interface get invoked on the main Envoy thread. Thus, it is important * for implementations of this interface to not execute any blocking operations on the same @@ -46,7 +48,13 @@ class XdsConfigTracer { virtual ~XdsConfigTracer() = default; /** - * Log the decoded SotW xDS resources that are about to be ingested. + * Log the decoded SotW xDS resources ingestion status. + * + * This is called twice. One is at the time when the resouce is successfully parsed and + * before the ingestion. The other one is when the resource is successfully applied to Envoy + * and are about to be ACK'ed. The TraceDetails imply the status of ingestion process, e.g., + * TraceState::INGESTED. + * * @param type_url The type url of xDS message. * @param resources List of decoded resources that reflect the latest state. * @param details The log point state and details. @@ -57,6 +65,10 @@ class XdsConfigTracer { /** * Log point for SotW xDS discovery response. + * + * This is mainly callded when there is any exception during the parse and ingestion process + * for SotW. The TraceDetails includes the error details and TraceState. + * * @param message The SotW discovery response message body. * @param details The log point state and details. */ @@ -64,7 +76,10 @@ class XdsConfigTracer { const TraceDetails& details) PURE; /** - * Log point for Delta xDS discovery response message when received, ingested, and failed. + * Log point for Delta xDS discovery response message when it is received, successfully applied, + * and is failed to process it. TraceDetailes can be 3 states, RECEIVE, INGESTED, and FAILED, + * including error details. + * * @param message The Delta discovery response message body. * @param details The log point state and details. */ diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 8ca736cd065b4..d4e3591644e86 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -285,6 +285,7 @@ void GrpcMuxImpl::onDiscoveryResponse( } } + // Log point when the resources are successfully parsed. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::RECEIVE)); } @@ -292,6 +293,7 @@ void GrpcMuxImpl::onDiscoveryResponse( processDiscoveryResources(resources, api_state, type_url, message->version_info(), /*call_delegate=*/true); + // Log point when the resources are successfully ingested. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::INGESTED)); } @@ -305,6 +307,8 @@ void GrpcMuxImpl::onDiscoveryResponse( ::google::rpc::Status* error_detail = api_state.request_.mutable_error_detail(); error_detail->set_code(Grpc::Status::WellKnownGrpcStatus::Internal); error_detail->set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); + + // Log point when there is any exception during the parse and ingestion process. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log(*message, TraceDetails(TraceState::FAILED, *error_detail)); } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index b2be1385dbc75..19c4faa70f581 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -91,6 +91,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); + // Log point when DelataDiscoveryResponse is received. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log(*message, TraceDetails(TraceState::RECEIVE)); } @@ -115,6 +116,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( } auto ack = sub->second->sub_state_.handleResponse(*message); + + // Log point after the response is processed, it can be INGESTED or FAILED state based on the ack. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log( *message, TraceDetails(ack.error_detail_.code() == Grpc::Status::WellKnownGrpcStatus::Ok From 340d71e1b5ca56839b8f9bf42725657c05a03f1d Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Fri, 28 Oct 2022 14:49:28 +0000 Subject: [PATCH 10/31] fix typo Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracer.h | 4 ++-- source/common/config/new_grpc_mux_impl.cc | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h index c2f36fe7c00ac..0e463cd9100f2 100644 --- a/envoy/config/xds_config_tracer.h +++ b/envoy/config/xds_config_tracer.h @@ -50,9 +50,9 @@ class XdsConfigTracer { /** * Log the decoded SotW xDS resources ingestion status. * - * This is called twice. One is at the time when the resouce is successfully parsed and + * This is called twice. One is at the time when the resource is successfully parsed and * before the ingestion. The other one is when the resource is successfully applied to Envoy - * and are about to be ACK'ed. The TraceDetails imply the status of ingestion process, e.g., + * and are about to be ACK'ed. The TraceDetails implies the status of ingestion process, e.g., * TraceState::INGESTED. * * @param type_url The type url of xDS message. diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 19c4faa70f581..4646af6c5b214 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -91,7 +91,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); - // Log point when DelataDiscoveryResponse is received. + // Log point when DeltaDiscoveryResponse is received. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log(*message, TraceDetails(TraceState::RECEIVE)); } @@ -117,7 +117,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( auto ack = sub->second->sub_state_.handleResponse(*message); - // Log point after the response is processed, it can be INGESTED or FAILED state based on the ack. + // Log point after the response is processed, and it can be INGESTED or FAILED + // state based on the ack. if (xds_config_tracer_.has_value()) { xds_config_tracer_->log( *message, TraceDetails(ack.error_detail_.code() == Grpc::Status::WellKnownGrpcStatus::Ok From acfb6360e3f0abc74710d57eabe69f3b023adc91 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 8 Nov 2022 05:44:33 +0000 Subject: [PATCH 11/31] make it more generic Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 8 +- envoy/config/BUILD | 4 +- envoy/config/subscription.h | 3 +- envoy/config/xds_config_tracer.h | 111 ----------------- envoy/config/xds_config_tracker.h | 115 ++++++++++++++++++ source/common/config/BUILD | 7 +- source/common/config/decoded_resource_impl.h | 9 +- source/common/config/grpc_mux_impl.cc | 29 +++-- source/common/config/grpc_mux_impl.h | 6 +- source/common/config/new_grpc_mux_impl.cc | 30 +++-- source/common/config/new_grpc_mux_impl.h | 11 +- .../config/subscription_factory_impl.cc | 10 +- .../common/config/subscription_factory_impl.h | 6 +- source/common/config/watch_map.cc | 5 + source/common/config/watch_map.h | 6 +- source/common/config/xds_mux/grpc_mux_impl.cc | 2 +- .../common/upstream/cluster_manager_impl.cc | 18 +-- source/common/upstream/cluster_manager_impl.h | 2 +- test/common/config/BUILD | 11 +- .../config/decoded_resource_impl_test.cc | 22 ++++ .../config/delta_subscription_impl_test.cc | 4 +- .../config/delta_subscription_test_harness.h | 4 +- test/common/config/grpc_mux_impl_test.cc | 10 +- .../config/grpc_subscription_test_harness.h | 4 +- test/common/config/new_grpc_mux_impl_test.cc | 4 +- .../config/subscription_factory_impl_test.cc | 4 +- test/common/config/watch_map_test.cc | 35 ++++-- test/common/upstream/eds_speed_test.cc | 4 +- test/integration/BUILD | 12 +- test/integration/xds_config_tracer_test.proto | 7 -- ...=> xds_config_tracker_integration_test.cc} | 115 ++++++++++-------- .../integration/xds_config_tracker_test.proto | 7 ++ 32 files changed, 344 insertions(+), 281 deletions(-) delete mode 100644 envoy/config/xds_config_tracer.h create mode 100644 envoy/config/xds_config_tracker.h delete mode 100644 test/integration/xds_config_tracer_test.proto rename test/integration/{xds_config_tracer_integration_test.cc => xds_config_tracker_integration_test.cc} (66%) create mode 100644 test/integration/xds_config_tracker_test.proto diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index f52c18d2f3c0d..2676b9c0757be 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -343,12 +343,12 @@ message Bootstrap { // [#not-implemented-hide:] core.v3.TypedExtensionConfig xds_delegate_extension = 35; - // Optional XdsConfigTracer configuration, which allows tracking xDS responses in external components, - // e.g., external logger, tracer, or monitor. It provides the log point when receive, ingest, or fail to + // Optional XdsConfigTracker configuration, which allows tracking xDS responses in external components, + // e.g., external tracer or monitor. It provides the process point when receive, ingest, or fail to // process xDS resources and messages. - // If a value is not specified, no XdsConfigTracer will be used. + // If a value is not specified, no XdsConfigTracker will be used. // [#not-implemented-hide:] - core.v3.TypedExtensionConfig xds_config_tracer_extension = 36; + core.v3.TypedExtensionConfig xds_config_tracker_extension = 36; } // Administration interface :ref:`operations documentation diff --git a/envoy/config/BUILD b/envoy/config/BUILD index a116ab68386ef..d527fea857bfe 100644 --- a/envoy/config/BUILD +++ b/envoy/config/BUILD @@ -119,8 +119,8 @@ envoy_cc_library( ) envoy_cc_library( - name = "xds_config_tracer_interface", - hdrs = ["xds_config_tracer.h"], + name = "xds_config_tracker_interface", + hdrs = ["xds_config_tracker.h"], deps = [ ":subscription_interface", ":typed_config_interface", diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h index 6daf2d63f2b50..c3c814898cd17 100644 --- a/envoy/config/subscription.h +++ b/envoy/config/subscription.h @@ -4,6 +4,7 @@ #include #include "envoy/common/exception.h" +#include "envoy/common/optref.h" #include "envoy/common/pure.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/stats/stats_macros.h" @@ -63,7 +64,7 @@ class DecodedResource { /** * @return absl::optional of a resource. */ - virtual absl::optional metadata() const PURE; + virtual OptRef metadata() const PURE; }; using DecodedResourcePtr = std::unique_ptr; diff --git a/envoy/config/xds_config_tracer.h b/envoy/config/xds_config_tracer.h deleted file mode 100644 index 0e463cd9100f2..0000000000000 --- a/envoy/config/xds_config_tracer.h +++ /dev/null @@ -1,111 +0,0 @@ -#pragma once - -#include -#include - -#include "envoy/common/optref.h" -#include "envoy/config/subscription.h" -#include "envoy/config/typed_config.h" -#include "envoy/protobuf/message_validator.h" - -#include "source/common/protobuf/protobuf.h" - -#include "google/rpc/status.pb.h" - -namespace Envoy { -namespace Config { - -// The status of processing Delta/DiscoveryResponse. -enum TraceState { - // Successfully got the resources or message. - RECEIVE = 0, - // Successfully ingested the resouces. - INGESTED = 1, - // Failed to apply the resources. - FAILED = 2, -}; - -struct TraceDetails { - TraceDetails(const TraceState state) : state_(state){}; - TraceDetails(const TraceState state, const ::google::rpc::Status error_detail) - : state_(state), error_detail_(error_detail){}; - const TraceState state_; - ::google::rpc::Status error_detail_; -}; - -/** - * An interface for hooking into xDS update events to provide the ablility to use some - * external logger or processor in xDS update. This tracer provides the log point when - * the resources are received , when they are successfully processed and applied, and when - * there is any failure. - * - * Instance of this interface get invoked on the main Envoy thread. Thus, it is important - * for implementations of this interface to not execute any blocking operations on the same - * thread. - */ -class XdsConfigTracer { -public: - virtual ~XdsConfigTracer() = default; - - /** - * Log the decoded SotW xDS resources ingestion status. - * - * This is called twice. One is at the time when the resource is successfully parsed and - * before the ingestion. The other one is when the resource is successfully applied to Envoy - * and are about to be ACK'ed. The TraceDetails implies the status of ingestion process, e.g., - * TraceState::INGESTED. - * - * @param type_url The type url of xDS message. - * @param resources List of decoded resources that reflect the latest state. - * @param details The log point state and details. - */ - virtual void log(const absl::string_view type_url, - const std::vector& resources, - const TraceDetails& details) PURE; - - /** - * Log point for SotW xDS discovery response. - * - * This is mainly callded when there is any exception during the parse and ingestion process - * for SotW. The TraceDetails includes the error details and TraceState. - * - * @param message The SotW discovery response message body. - * @param details The log point state and details. - */ - virtual void log(const envoy::service::discovery::v3::DiscoveryResponse& message, - const TraceDetails& details) PURE; - - /** - * Log point for Delta xDS discovery response message when it is received, successfully applied, - * and is failed to process it. TraceDetailes can be 3 states, RECEIVE, INGESTED, and FAILED, - * including error details. - * - * @param message The Delta discovery response message body. - * @param details The log point state and details. - */ - virtual void log(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, - const TraceDetails& details) PURE; -}; - -using XdsConfigTracerPtr = std::unique_ptr; -using XdsConfigTracerOptRef = OptRef; - -/** - * A factory abstract class for creating instances of XdsConfigTracer. - */ -class XdsConfigTracerFactory : public Config::TypedFactory { -public: - ~XdsConfigTracerFactory() override = default; - - /** - * Creates an XdsConfigTracer using the given config. - */ - virtual XdsConfigTracerPtr - createXdsConfigTracer(const ProtobufWkt::Any& config, - ProtobufMessage::ValidationVisitor& validation_visitor) PURE; - - std::string category() const override { return "envoy.config.xds_tracers"; } -}; - -} // namespace Config -} // namespace Envoy diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h new file mode 100644 index 0000000000000..8073ad76bdeb3 --- /dev/null +++ b/envoy/config/xds_config_tracker.h @@ -0,0 +1,115 @@ +#pragma once + +#include +#include + +#include "envoy/common/optref.h" +#include "envoy/config/subscription.h" +#include "envoy/config/typed_config.h" +#include "envoy/protobuf/message_validator.h" + +#include "source/common/protobuf/protobuf.h" + +#include "google/rpc/status.pb.h" + +namespace Envoy { +namespace Config { + +// The status of processing Delta/DiscoveryResponse. +enum ProcessingState { + // Successfully got the resources or message. + RECEIVED = 0, + // Failed to apply the resources. + FAILED = 1, +}; + +struct ProcessingDetails { + ProcessingDetails(const ProcessingState state) : state_(state){}; + ProcessingDetails(const ProcessingState state, const ::google::rpc::Status error_detail) + : state_(state), error_detail_(error_detail){}; + const ProcessingState state_; + ::google::rpc::Status error_detail_; +}; + +/** + * An interface for hooking into xDS update events to provide the ablility to use some external + * processor in xDS update. This traker provides the process point when the discovery response + * is received, when the resoures are successfully processed and applied, and when there is any + * failure. + * + * Instance of this interface get invoked on the main Envoy thread. Thus, it is important + * for implementations of this interface to not execute any blocking operations on the same + * thread. + */ +class XdsConfigTracker { +public: + virtual ~XdsConfigTracker() = default; + + /** + * Invoked when SotW xDS configuration updates have been successfully parsed, applied on + * the Envoy instance, and are about to be ACK'ed. + * + * @param type_url The type url of xDS message. + * @param resources A list of decoded resources to add to the current state. + */ + virtual void onConfigIngested(const absl::string_view type_url, + const std::vector& resources) PURE; + + /** + * Invoked when Delta xDS configuration updates have been successfully parsed, applied on + * the Envoy instance, and are about to be ACK'ed. + * + * @param type_url The type url of xDS message. + * @param added_resources A list of decoded resources to add to the current state. + * @param removed_resources A list of resources to remove from the current state. + */ + virtual void + onConfigIngested(const absl::string_view type_url, + const std::vector& added_resources, + const Protobuf::RepeatedPtrField& removed_resources) PURE; + + /** + * Invoked when received or failed to process the DiscoveryResponse. + * The ProcessingState can be RECIEVED or FALIED. + * + * @param message The SotW discovery response message body. + * @param details The process state and error details. + */ + virtual void + onResponseReceiveOrFail(const envoy::service::discovery::v3::DiscoveryResponse& message, + const ProcessingDetails& details) PURE; + + /** + * Invoked when received or failed to process the DeltaDiscoveryResponse. + * The ProcessingState can be RECIEVED or FALIED. + * + * @param message The Delta discovery response message body. + * @param details The process state and error details. + */ + virtual void + onResponseReceiveOrFail(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, + const ProcessingDetails& details) PURE; +}; + +using XdsConfigTrackerPtr = std::unique_ptr; +using XdsConfigTrackerOptRef = OptRef; + +/** + * A factory abstract class for creating instances of XdsConfigTracker. + */ +class XdsConfigTrackerFactory : public Config::TypedFactory { +public: + ~XdsConfigTrackerFactory() override = default; + + /** + * Creates an XdsConfigTracker using the given config. + */ + virtual XdsConfigTrackerPtr + createXdsConfigTracker(const ProtobufWkt::Any& config, + ProtobufMessage::ValidationVisitor& validation_visitor) PURE; + + std::string category() const override { return "envoy.config.xds_trackers"; } +}; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index b022cd750dc26..8181f7610be05 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -166,7 +166,7 @@ envoy_cc_library( ":xds_source_id_lib", "//envoy/config:grpc_mux_interface", "//envoy/config:subscription_interface", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/config:xds_resources_delegate_interface", "//envoy/upstream:cluster_manager_interface", "//source/common/common:cleanup_lib", @@ -204,7 +204,7 @@ envoy_cc_library( ":watch_map_lib", ":xds_context_params_lib", ":xds_resource_lib", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/event:dispatcher_interface", "//envoy/grpc:async_client_interface", "//source/common/memory:utils_lib", @@ -331,7 +331,7 @@ envoy_cc_library( ":xds_resource_lib", "//envoy/config:subscription_factory_interface", "//envoy/config:subscription_interface", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/config:xds_resources_delegate_interface", "//envoy/server:instance_interface", "//envoy/upstream:cluster_manager_interface", @@ -437,6 +437,7 @@ envoy_cc_library( ":utility_lib", ":xds_resource_lib", "//envoy/config:subscription_interface", + "//envoy/config:xds_config_tracker_interface", "//source/common/common:assert_lib", "//source/common/common:cleanup_lib", "//source/common/common:minimal_logger_lib", diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index ba0fa18103c8b..55936cc5f6526 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/common/optref.h" #include "envoy/config/subscription.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -57,7 +58,7 @@ class DecodedResourceImpl : public DecodedResource { resource.has_ttl() ? absl::make_optional(std::chrono::milliseconds( DurationUtil::durationToMilliseconds(resource.ttl()))) : absl::nullopt, - resource.has_metadata() ? absl::make_optional(resource.metadata()) : absl::nullopt) {} + resource.has_metadata() ? makeOptRef(resource.metadata()) : absl::nullopt) {} DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, const xds::core::v3::CollectionEntry::InlineEntry& inline_entry) : DecodedResourceImpl(resource_decoder, inline_entry.name(), @@ -75,14 +76,14 @@ class DecodedResourceImpl : public DecodedResource { const Protobuf::Message& resource() const override { return *resource_; }; bool hasResource() const override { return has_resource_; } absl::optional ttl() const override { return ttl_; } - absl::optional metadata() const override { return metadata_; } + OptRef metadata() const override { return metadata_; } private: DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, absl::optional name, const Protobuf::RepeatedPtrField& aliases, const ProtobufWkt::Any& resource, bool has_resource, const std::string& version, absl::optional ttl, - const absl::optional metadata) + OptRef metadata) : resource_(resource_decoder.decodeResource(resource)), has_resource_(has_resource), name_(name ? *name : resource_decoder.resourceName(*resource_)), aliases_(repeatedPtrFieldToVector(aliases)), version_(version), ttl_(ttl), @@ -95,7 +96,7 @@ class DecodedResourceImpl : public DecodedResource { const std::string version_; // Per resource TTL. const absl::optional ttl_; - const absl::optional metadata_; + OptRef metadata_; }; struct DecodedResourcesWrapper { diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index d4e3591644e86..6162b48bf9147 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -39,13 +39,13 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, CustomConfigValidatorsPtr&& config_validators, - XdsConfigTracerOptRef xds_config_tracer, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), local_info_(local_info), skip_subsequent_node_(skip_subsequent_node), - config_validators_(std::move(config_validators)), xds_config_tracer_(xds_config_tracer), + config_validators_(std::move(config_validators)), xds_config_tracker_(xds_config_tracker), xds_resources_delegate_(xds_resources_delegate), target_xds_authority_(target_xds_authority), first_stream_request_(true), dispatcher_(dispatcher), dynamic_update_callback_handle_(local_info.contextProvider().addDynamicContextUpdateCallback( @@ -220,6 +220,13 @@ void GrpcMuxImpl::onDiscoveryResponse( ControlPlaneStats& control_plane_stats) { const std::string type_url = message->type_url(); ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url, message->version_info()); + + // Processing point when DiscoveryResponse is received. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onResponseReceiveOrFail(*message, + ProcessingDetails(ProcessingState::RECEIVED)); + } + if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a // protocol violation @@ -285,17 +292,12 @@ void GrpcMuxImpl::onDiscoveryResponse( } } - // Log point when the resources are successfully parsed. - if (xds_config_tracer_.has_value()) { - xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::RECEIVE)); - } - processDiscoveryResources(resources, api_state, type_url, message->version_info(), /*call_delegate=*/true); - // Log point when the resources are successfully ingested. - if (xds_config_tracer_.has_value()) { - xds_config_tracer_->log(type_url, resources, TraceDetails(TraceState::INGESTED)); + // Processing point when resources are successfully ingested. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onConfigIngested(type_url, resources); } } END_TRY @@ -308,9 +310,10 @@ void GrpcMuxImpl::onDiscoveryResponse( error_detail->set_code(Grpc::Status::WellKnownGrpcStatus::Internal); error_detail->set_message(Config::Utility::truncateGrpcStatusMessage(e.what())); - // Log point when there is any exception during the parse and ingestion process. - if (xds_config_tracer_.has_value()) { - xds_config_tracer_->log(*message, TraceDetails(TraceState::FAILED, *error_detail)); + // Processing point when there is any exception during the parse and ingestion process. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onResponseReceiveOrFail( + *message, ProcessingDetails(ProcessingState::FAILED, *error_detail)); } } previously_fetched_data_ = true; diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index fb1b8c220ca3a..2c07c17ba7d60 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -8,7 +8,7 @@ #include "envoy/common/time.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" @@ -40,7 +40,7 @@ class GrpcMuxImpl : public GrpcMux, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, CustomConfigValidatorsPtr&& config_validators, - XdsConfigTracerOptRef xds_config_tracer, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority); @@ -184,7 +184,7 @@ class GrpcMuxImpl : public GrpcMux, const LocalInfo::LocalInfo& local_info_; const bool skip_subsequent_node_; CustomConfigValidatorsPtr config_validators_; - XdsConfigTracerOptRef xds_config_tracer_; + XdsConfigTrackerOptRef xds_config_tracker_; XdsResourcesDelegateOptRef xds_resources_delegate_; const std::string target_xds_authority_; bool first_stream_request_; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 4646af6c5b214..ee05f516768a0 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -41,7 +41,7 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, CustomConfigValidatorsPtr&& config_validators, - XdsConfigTracerOptRef xds_config_tracer) + XdsConfigTrackerOptRef xds_config_tracker) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), local_info_(local_info), config_validators_(std::move(config_validators)), @@ -49,7 +49,7 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, [this](absl::string_view resource_type_url) { onDynamicContextUpdate(resource_type_url); })), - dispatcher_(dispatcher), xds_config_tracer_(xds_config_tracer) { + dispatcher_(dispatcher), xds_config_tracker_(xds_config_tracker) { AllMuxes::get().insert(this); } @@ -91,9 +91,10 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); - // Log point when DeltaDiscoveryResponse is received. - if (xds_config_tracer_.has_value()) { - xds_config_tracer_->log(*message, TraceDetails(TraceState::RECEIVE)); + // Processing point when DeltaDiscoveryResponse is received. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onResponseReceiveOrFail(*message, + ProcessingDetails(ProcessingState::RECEIVED)); } auto sub = subscriptions_.find(message->type_url()); @@ -117,14 +118,11 @@ void NewGrpcMuxImpl::onDiscoveryResponse( auto ack = sub->second->sub_state_.handleResponse(*message); - // Log point after the response is processed, and it can be INGESTED or FAILED - // state based on the ack. - if (xds_config_tracer_.has_value()) { - xds_config_tracer_->log( - *message, TraceDetails(ack.error_detail_.code() == Grpc::Status::WellKnownGrpcStatus::Ok - ? TraceState::INGESTED - : TraceState::FAILED, - ack.error_detail_)); + // Processing point to record error if there is any failure after the response is processed. + if (xds_config_tracker_.has_value() && + ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { + xds_config_tracker_->onResponseReceiveOrFail( + *message, ProcessingDetails(ProcessingState::FAILED, ack.error_detail_)); } kickOffAck(ack); Memory::Utils::tryShrinkHeap(); @@ -251,9 +249,9 @@ void NewGrpcMuxImpl::removeWatch(const std::string& type_url, Watch* watch) { void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use_namespace_matching) { - subscriptions_.emplace( - type_url, std::make_unique(type_url, local_info_, use_namespace_matching, - dispatcher_, *config_validators_.get())); + subscriptions_.emplace(type_url, std::make_unique( + type_url, local_info_, use_namespace_matching, dispatcher_, + *config_validators_.get(), xds_config_tracker_)); subscription_ordering_.emplace_back(type_url); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 57ba603d87833..bafa2b114d094 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -6,7 +6,7 @@ #include "envoy/common/token_bucket.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/common/logger.h" @@ -36,7 +36,7 @@ class NewGrpcMuxImpl Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, CustomConfigValidatorsPtr&& config_validators, - XdsConfigTracerOptRef xds_config_tracer); + XdsConfigTrackerOptRef xds_config_tracker); ~NewGrpcMuxImpl() override; @@ -85,8 +85,9 @@ class NewGrpcMuxImpl struct SubscriptionStuff { SubscriptionStuff(const std::string& type_url, const LocalInfo::LocalInfo& local_info, const bool use_namespace_matching, Event::Dispatcher& dispatcher, - CustomConfigValidators& config_validators) - : watch_map_(use_namespace_matching, type_url, config_validators), + CustomConfigValidators& config_validators, + XdsConfigTrackerOptRef xds_config_tracker) + : watch_map_(use_namespace_matching, type_url, config_validators, xds_config_tracker), sub_state_(type_url, watch_map_, local_info, dispatcher) {} WatchMap watch_map_; @@ -180,7 +181,7 @@ class NewGrpcMuxImpl CustomConfigValidatorsPtr config_validators_; Common::CallbackHandlePtr dynamic_update_callback_handle_; Event::Dispatcher& dispatcher_; - XdsConfigTracerOptRef xds_config_tracer_; + XdsConfigTrackerOptRef xds_config_tracker_; // True iff Envoy is shutting down; no messages should be sent on the `grpc_stream_` when this is // true because it may contain dangling pointers. diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index 6ad2833c9634d..e27786eb112cb 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -24,10 +24,10 @@ SubscriptionFactoryImpl::SubscriptionFactoryImpl( const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, const Server::Instance& server, - XdsResourcesDelegateOptRef xds_resources_delegate, XdsConfigTracerOptRef xds_config_tracer) + XdsResourcesDelegateOptRef xds_resources_delegate, XdsConfigTrackerOptRef xds_config_tracker) : local_info_(local_info), dispatcher_(dispatcher), cm_(cm), validation_visitor_(validation_visitor), api_(api), server_(server), - xds_resources_delegate_(xds_resources_delegate), xds_config_tracer_(xds_config_tracer) {} + xds_resources_delegate_(xds_resources_delegate), xds_config_tracker_(xds_config_tracker) {} SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( const envoy::config::core::v3::ConfigSource& config, absl::string_view type_url, @@ -98,7 +98,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( dispatcher_, sotwGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), api_config_source.set_node_on_first_message_only(), std::move(custom_config_validators), - xds_config_tracer_, xds_resources_delegate_, control_plane_id); + xds_config_tracker_, xds_resources_delegate_, control_plane_id); } return std::make_unique( std::move(mux), callbacks, resource_decoder, stats, type_url, dispatcher_, @@ -126,7 +126,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ->createUncachedRawAsyncClient(), dispatcher_, deltaGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, - std::move(custom_config_validators), xds_config_tracer_); + std::move(custom_config_validators), xds_config_tracker_); } return std::make_unique( std::move(mux), callbacks, resource_decoder, stats, type_url, dispatcher_, @@ -192,7 +192,7 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl( ->createUncachedRawAsyncClient(), dispatcher_, deltaGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, - std::move(custom_config_validators), xds_config_tracer_), + std::move(custom_config_validators), xds_config_tracker_), callbacks, resource_decoder, stats, dispatcher_, Utility::configSourceInitialFetchTimeout(config), false, options); } diff --git a/source/common/config/subscription_factory_impl.h b/source/common/config/subscription_factory_impl.h index 7053cbe8d73a3..2f37e08408e72 100644 --- a/source/common/config/subscription_factory_impl.h +++ b/source/common/config/subscription_factory_impl.h @@ -5,7 +5,7 @@ #include "envoy/config/core/v3/config_source.pb.h" #include "envoy/config/subscription.h" #include "envoy/config/subscription_factory.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/server/instance.h" #include "envoy/stats/scope.h" @@ -23,7 +23,7 @@ class SubscriptionFactoryImpl : public SubscriptionFactory, Logger::Loggablecallbacks_.onConfigUpdate({}, {}, system_version_info); } } + + // Processing point when resources are successfully ingested. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onConfigIngested(type_url_, decoded_resources, removed_resources); + } } void WatchMap::onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) { diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index 7c0da75df7f5b..f39b6ddf32c42 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -5,6 +5,7 @@ #include #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/common/assert.h" @@ -62,9 +63,9 @@ struct Watch { class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable { public: WatchMap(const bool use_namespace_matching, const std::string& type_url, - CustomConfigValidators& config_validators) + CustomConfigValidators& config_validators, XdsConfigTrackerOptRef xds_config_tracker) : use_namespace_matching_(use_namespace_matching), type_url_(type_url), - config_validators_(config_validators) {} + config_validators_(config_validators), xds_config_tracker_(xds_config_tracker) {} // Adds 'callbacks' to the WatchMap, with every possible resource being watched. // (Use updateWatchInterest() to narrow it down to some specific names). @@ -133,6 +134,7 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable::addWatch( watch_map = watch_maps_ .emplace(type_url, std::make_unique(options.use_namespace_matching_, type_url, - *config_validators_.get())) + *config_validators_.get(), absl::nullopt)) .first; subscriptions_.emplace(type_url, subscription_state_factory_->makeSubscriptionState( type_url, *watch_maps_[type_url], resource_decoder, diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index bb5003a214e72..ec41ff11d14d9 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -330,18 +330,18 @@ ClusterManagerImpl::ClusterManagerImpl( validation_context.dynamicValidationVisitor(), api, main_thread_dispatcher); } - if (bootstrap.has_xds_config_tracer_extension()) { - auto& tracer_factory = Config::Utility::getAndCheckFactory( - bootstrap.xds_config_tracer_extension()); - xds_config_tracer_ = - tracer_factory.createXdsConfigTracer(bootstrap.xds_config_tracer_extension().typed_config(), - validation_context.dynamicValidationVisitor()); + if (bootstrap.has_xds_config_tracker_extension()) { + auto& tracer_factory = Config::Utility::getAndCheckFactory( + bootstrap.xds_config_tracker_extension()); + xds_config_tracker_ = tracer_factory.createXdsConfigTracker( + bootstrap.xds_config_tracker_extension().typed_config(), + validation_context.dynamicValidationVisitor()); } subscription_factory_ = std::make_unique( local_info, main_thread_dispatcher, *this, validation_context.dynamicValidationVisitor(), api, server, makeOptRefFromPtr(xds_resources_delegate_.get()), - makeOptRefFromPtr(xds_config_tracer_.get())); + makeOptRefFromPtr(xds_config_tracker_.get())); const auto& dyn_resources = bootstrap.dynamic_resources(); @@ -408,7 +408,7 @@ ClusterManagerImpl::ClusterManagerImpl( "envoy.service.discovery.v3.AggregatedDiscoveryService.DeltaAggregatedResources"), random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info, - std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracer_.get())); + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracker_.get())); } } else { Config::Utility::checkTransportVersion(dyn_resources.ads_config()); @@ -441,7 +441,7 @@ ClusterManagerImpl::ClusterManagerImpl( random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only(), - std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracer_.get()), + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracker_.get()), xds_delegate_opt_ref, target_xds_authority); } } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 892097b84b282..58af3b36340f7 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -774,7 +774,7 @@ class ClusterManagerImpl : public ClusterManager, ClusterSet primary_clusters_; std::unique_ptr xds_resources_delegate_; - std::unique_ptr xds_config_tracer_; + std::unique_ptr xds_config_tracker_; }; } // namespace Upstream diff --git a/test/common/config/BUILD b/test/common/config/BUILD index c9f9a9fd60e78..c45fd0c4c191a 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -36,7 +36,7 @@ envoy_cc_test( srcs = ["delta_subscription_impl_test.cc"], deps = [ ":delta_subscription_test_harness", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//source/common/config:api_version_lib", "//source/common/config:grpc_subscription_lib", "//source/common/config:new_grpc_mux_lib", @@ -155,7 +155,7 @@ envoy_cc_test( name = "grpc_mux_impl_test", srcs = ["grpc_mux_impl_test.cc"], deps = [ - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/config:api_version_lib", "//source/common/config:grpc_mux_lib", @@ -265,7 +265,7 @@ envoy_cc_test_library( hdrs = ["grpc_subscription_test_harness.h"], deps = [ ":subscription_test_harness", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/common:hash_lib", "//source/common/config:api_version_lib", @@ -291,7 +291,7 @@ envoy_cc_test_library( hdrs = ["delta_subscription_test_harness.h"], deps = [ ":subscription_test_harness", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//source/common/common:utility_lib", "//source/common/config:new_grpc_mux_lib", "//source/common/config/xds_mux:grpc_mux_lib", @@ -354,7 +354,7 @@ envoy_cc_test( name = "subscription_factory_impl_test", srcs = ["subscription_factory_impl_test.cc"], deps = [ - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/config:xds_resources_delegate_interface", "//source/common/config:subscription_factory_lib", "//source/common/config:xds_resource_lib", @@ -484,6 +484,7 @@ envoy_cc_test( name = "watch_map_test", srcs = ["watch_map_test.cc"], deps = [ + "//envoy/config:xds_config_tracker_interface", "//source/common/config:watch_map_lib", "//test/mocks/config:config_mocks", "//test/mocks/config:custom_config_validators_mocks", diff --git a/test/common/config/decoded_resource_impl_test.cc b/test/common/config/decoded_resource_impl_test.cc index ef2584b638858..229a0f7960423 100644 --- a/test/common/config/decoded_resource_impl_test.cc +++ b/test/common/config/decoded_resource_impl_test.cc @@ -49,6 +49,28 @@ TEST(DecodedResourceImplTest, All) { EXPECT_EQ("foo", decoded_resource.version()); EXPECT_THAT(decoded_resource.resource(), ProtoEq(ProtobufWkt::Empty())); EXPECT_TRUE(decoded_resource.hasResource()); + EXPECT_FALSE(decoded_resource.metadata().has_value()); + } + + { + envoy::service::discovery::v3::Resource resource_wrapper; + resource_wrapper.set_name("real_name"); + resource_wrapper.mutable_resource()->MergeFrom(some_opaque_resource); + resource_wrapper.set_version("foo"); + auto metadata = resource_wrapper.mutable_metadata(); + metadata->mutable_filter_metadata()->insert( + {"fake_test_domain", MessageUtil::keyValueStruct("fake_test_key", "fake_test_value")}); + EXPECT_CALL(resource_decoder, decodeResource(ProtoEq(some_opaque_resource))) + .WillOnce(InvokeWithoutArgs( + []() -> ProtobufTypes::MessagePtr { return std::make_unique(); })); + EXPECT_CALL(resource_decoder, resourceName(ProtoEq(ProtobufWkt::Empty()))).Times(0); + DecodedResourceImpl decoded_resource(resource_decoder, resource_wrapper); + EXPECT_EQ("real_name", decoded_resource.name()); + EXPECT_EQ("foo", decoded_resource.version()); + EXPECT_THAT(decoded_resource.resource(), ProtoEq(ProtobufWkt::Empty())); + EXPECT_TRUE(decoded_resource.hasResource()); + EXPECT_TRUE(decoded_resource.metadata().has_value()); + EXPECT_EQ(metadata->DebugString(), decoded_resource.metadata()->DebugString()); } { diff --git a/test/common/config/delta_subscription_impl_test.cc b/test/common/config/delta_subscription_impl_test.cc index afe4d62258845..993cdc2f6f598 100644 --- a/test/common/config/delta_subscription_impl_test.cc +++ b/test/common/config/delta_subscription_impl_test.cc @@ -1,6 +1,6 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/buffer/zero_copy_input_stream_impl.h" @@ -159,7 +159,7 @@ TEST_P(DeltaSubscriptionNoGrpcStreamTest, NoGrpcStream) { std::unique_ptr(async_client), dispatcher, *method_descriptor, random, stats_store, rate_limit_settings, local_info, std::make_unique>(), - /*xds_config_tracer=*/XdsConfigTracerOptRef()); + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } GrpcSubscriptionImplPtr subscription = std::make_unique( diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index b2bcbed084862..96077d8619604 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -5,7 +5,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/config/grpc_subscription_impl.h" @@ -58,7 +58,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, local_info_, std::make_unique>(), - /*xds_config_tracer=*/XdsConfigTracerOptRef()); + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } subscription_ = std::make_unique( xds_context_, callbacks_, resource_decoder_, stats_, diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 71e784ee08e72..3636b25f09319 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -2,7 +2,7 @@ #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -65,7 +65,7 @@ class GrpcMuxImplTestBase : public testing::Test { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::move(config_validators_), - /*xds_config_tracer=*/XdsConfigTracerOptRef(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } @@ -75,7 +75,7 @@ class GrpcMuxImplTestBase : public testing::Test { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, custom_rate_limit_settings, true, std::move(config_validators_), - /*xds_config_tracer=*/XdsConfigTracerOptRef(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } @@ -896,7 +896,7 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::make_unique>(), - /*xds_config_tracer=*/XdsConfigTracerOptRef(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " @@ -912,7 +912,7 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, true, std::make_unique>(), - /*xds_config_tracer=*/XdsConfigTracerOptRef(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index f95763f2d5a78..eeb13fbf4d041 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -5,7 +5,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -65,7 +65,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { mux_ = std::make_shared( local_info_, std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, true, - std::move(config_validators_), /*xds_config_tracer=*/XdsConfigTracerOptRef(), + std::move(config_validators_), /*xds_config_tracker=*/XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), /*target_xds_authority=*/""); } diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 62e4f52fccaf6..197abaad19423 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -2,7 +2,7 @@ #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/event/timer.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -74,7 +74,7 @@ class NewGrpcMuxImplTestBase : public testing::TestWithParam { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v3.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, local_info_, std::move(config_validators_), - /*xds_config_tracer=*/XdsConfigTracerOptRef()); + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } void expectSendMessage(const std::string& type_url, diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index 0a785d9aa3a8a..80ca4df3adc6a 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -6,7 +6,7 @@ #include "envoy/config/core/v3/config_source.pb.validate.h" #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/stats/scope.h" @@ -49,7 +49,7 @@ class SubscriptionFactoryTest : public testing::Test { api_(Api::createApiForTest(stats_store_, random_)), subscription_factory_(local_info_, dispatcher_, cm_, validation_visitor_, *api_, server_, /*xds_resources_delegate=*/XdsResourcesDelegateOptRef(), - /*xds_config_tracer=*/XdsConfigTracerOptRef()) {} + /*xds_config_tracker=*/XdsConfigTrackerOptRef()) {} SubscriptionPtr subscriptionFromConfigSource(const envoy::config::core::v3::ConfigSource& config) { diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 76cfe31768367..7ad48061f2bae 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -3,6 +3,7 @@ #include "envoy/common/exception.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/stats/scope.h" @@ -127,7 +128,8 @@ TEST(WatchMapTest, Basic) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); { @@ -201,7 +203,8 @@ TEST(WatchMapTest, Overlap) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -264,7 +267,9 @@ TEST(WatchMapTest, Overlap) { // WatchMap defers deletes and doesn't crash. class SameWatchRemoval : public testing::Test { public: - SameWatchRemoval() : watch_map_(false, "ClusterLoadAssignmentType", config_validators) {} + SameWatchRemoval() + : watch_map_(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()) {} void SetUp() override { envoy::config::endpoint::v3::ClusterLoadAssignment alice; @@ -343,7 +348,8 @@ TEST(WatchMapTest, AddRemoveAdd) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -400,7 +406,8 @@ TEST(WatchMapTest, UninterestingUpdate) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"alice"}); @@ -445,7 +452,8 @@ TEST(WatchMapTest, WatchingEverything) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); /*Watch* watch1 = */ watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); // watch1 never specifies any names, and so is treated as interested in everything. @@ -482,7 +490,8 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); @@ -516,7 +525,8 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { TEST(WatchMapTest, OnConfigUpdateFailed) { NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); // calling on empty map doesn't break watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); @@ -538,7 +548,8 @@ TEST(WatchMapTest, OnConfigUpdateXdsTpGlobCollections) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"xdstp://foo/bar/baz/*?some=thing&thing=some"}); @@ -583,7 +594,8 @@ TEST(WatchMapTest, OnConfigUpdateXdsTpSingletons) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"xdstp://foo/bar/baz?some=thing&thing=some"}); @@ -624,7 +636,8 @@ TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(true, "ClusterLoadAssignmentType", config_validators); + WatchMap watch_map(true, "ClusterLoadAssignmentType", config_validators, + XdsConfigTrackerOptRef()); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 13492abace79d..7951283e1474d 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -5,7 +5,7 @@ #include "envoy/config/core/v3/health_check.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint_components.pb.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/stats/scope.h" @@ -62,7 +62,7 @@ class EdsSpeedTest { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.endpoint.v3.EndpointDiscoveryService.StreamEndpoints"), random_, stats_, {}, true, std::move(config_validators_), - /*xds_config_tracer=*/Config::XdsConfigTracerOptRef(), + /*xds_config_tracker=*/Config::XdsConfigTrackerOptRef(), /*xds_resources_delegate=*/Config::XdsResourcesDelegateOptRef(), /*target_xds_authority=*/"")); } diff --git a/test/integration/BUILD b/test/integration/BUILD index 3acd1670bb000..f725ae90ad8d0 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -2020,14 +2020,14 @@ envoy_proto_library( ) envoy_cc_test( - name = "xds_config_tracer_integration_test", - srcs = ["xds_config_tracer_integration_test.cc"], + name = "xds_config_tracker_integration_test", + srcs = ["xds_config_tracker_integration_test.cc"], external_deps = ["abseil_strings"], deps = [ ":http_integration_lib", - ":xds_config_tracer_test_proto_cc_proto", + ":xds_config_tracker_test_proto_cc_proto", "//envoy/config:subscription_interface", - "//envoy/config:xds_config_tracer_interface", + "//envoy/config:xds_config_tracker_interface", "//envoy/protobuf:message_validator_interface", "//test/common/grpc:grpc_client_integration_lib", "//test/config:v2_link_hacks", @@ -2039,6 +2039,6 @@ envoy_cc_test( ) envoy_proto_library( - name = "xds_config_tracer_test_proto", - srcs = ["xds_config_tracer_test.proto"], + name = "xds_config_tracker_test_proto", + srcs = ["xds_config_tracker_test.proto"], ) diff --git a/test/integration/xds_config_tracer_test.proto b/test/integration/xds_config_tracer_test.proto deleted file mode 100644 index dd074b930483d..0000000000000 --- a/test/integration/xds_config_tracer_test.proto +++ /dev/null @@ -1,7 +0,0 @@ -syntax = "proto3"; - -package test.envoy.config.xds; - -// Configuration for TestXdsConfigTracer. -message TestXdsConfigTracer { -} diff --git a/test/integration/xds_config_tracer_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc similarity index 66% rename from test/integration/xds_config_tracer_integration_test.cc rename to test/integration/xds_config_tracker_integration_test.cc index 00ac439ae0b3d..66d68a4bb1510 100644 --- a/test/integration/xds_config_tracer_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -2,7 +2,7 @@ #include "envoy/config/route/v3/route.pb.h" #include "envoy/config/subscription.h" -#include "envoy/config/xds_config_tracer.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/protobuf/message_validator.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/service/runtime/v3/rtds.pb.h" @@ -10,7 +10,7 @@ #include "test/common/grpc/grpc_client_integration.h" #include "test/config/v2_link_hacks.h" #include "test/integration/http_integration.h" -#include "test/integration/xds_config_tracer_test.pb.h" +#include "test/integration/xds_config_tracker_test.pb.h" #include "test/test_common/registry.h" #include "test/test_common/utility.h" @@ -24,31 +24,39 @@ namespace { constexpr char XDS_CLUSTER_NAME[] = "xds_cluster"; -class TestXdsConfigTracer : public Config::XdsConfigTracer { +class TestXdsConfigTracker : public Config::XdsConfigTracker { public: - TestXdsConfigTracer() { + TestXdsConfigTracker() { ReceiveCount = 0; IngestedCount = 0; FailureCount = 0; ErrorMessage = ""; } - void log(const absl::string_view, const std::vector&, - const Config::TraceDetails& trace_details) override { - countState(trace_details.state_); + void onConfigIngested(const absl::string_view, + const std::vector&) override { + ++IngestedCount; } - void log(const envoy::service::discovery::v3::DiscoveryResponse&, - const Config::TraceDetails& trace_details) override { - countState(trace_details.state_); - if (trace_details.state_ == Config::TraceState::FAILED) { - ErrorMessage = trace_details.error_detail_.message(); + void onConfigIngested(const absl::string_view, const std::vector&, + const Protobuf::RepeatedPtrField&) override { + ++IngestedCount; + } + + void onResponseReceiveOrFail(const envoy::service::discovery::v3::DiscoveryResponse&, + const Config::ProcessingDetails& process_details) override { + countState(process_details.state_); + if (process_details.state_ == Config::ProcessingState::FAILED) { + ErrorMessage = process_details.error_detail_.message(); } } - void log(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, - const Config::TraceDetails& trace_details) override { - countState(trace_details.state_); + void onResponseReceiveOrFail(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, + const Config::ProcessingDetails& process_details) override { + countState(process_details.state_); + if (process_details.state_ == Config::ProcessingState::FAILED) { + ErrorMessage = process_details.error_detail_.message(); + } } static std::atomic ReceiveCount; @@ -57,49 +65,49 @@ class TestXdsConfigTracer : public Config::XdsConfigTracer { static std::string ErrorMessage; private: - void countState(const Config::TraceState& state) { + void countState(const Config::ProcessingState& state) { switch (state) { - case Config::TraceState::RECEIVE: + case Config::ProcessingState::RECEIVED: ++ReceiveCount; break; - case Config::TraceState::INGESTED: - ++IngestedCount; - break; - case Config::TraceState::FAILED: + case Config::ProcessingState::FAILED: ++FailureCount; break; }; } }; -std::atomic TestXdsConfigTracer::ReceiveCount; -std::atomic TestXdsConfigTracer::IngestedCount; -std::atomic TestXdsConfigTracer::FailureCount; -std::string TestXdsConfigTracer::ErrorMessage; +std::atomic TestXdsConfigTracker::ReceiveCount; +std::atomic TestXdsConfigTracker::IngestedCount; +std::atomic TestXdsConfigTracker::FailureCount; +std::string TestXdsConfigTracker::ErrorMessage; -class TestXdsConfigTracerFactory : public Config::XdsConfigTracerFactory { +static std::map ResourcesMap; + +class TestXdsConfigTrackerFactory : public Config::XdsConfigTrackerFactory { public: ProtobufTypes::MessagePtr createEmptyConfigProto() override { - return std::make_unique(); + return std::make_unique(); } std::string name() const override { return "envoy.config.xds.test_xds_tracer"; }; - Config::XdsConfigTracerPtr createXdsConfigTracer(const ProtobufWkt::Any&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_unique(); + Config::XdsConfigTrackerPtr createXdsConfigTracker(const ProtobufWkt::Any&, + ProtobufMessage::ValidationVisitor&) override { + return std::make_unique(); } }; -class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, - public HttpIntegrationTest { +class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, + public HttpIntegrationTest { public: - XdsConfigTracerIntegrationTest() + XdsConfigTrackerIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP2, ipVersion(), ConfigHelper::baseConfigNoListeners()) { use_lds_ = false; create_xds_upstream_ = true; + sotw_or_delta_ = sotwOrDelta(); // Make the default cluster HTTP2. config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { @@ -119,7 +127,7 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe config_helper_.addRuntimeOverride("whatevs", "yar"); // Set up the RTDS runtime layer. - config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* layer = bootstrap.mutable_layered_runtime()->add_layers(); layer->set_name("some_rtds_layer"); auto* rtds_layer = layer->mutable_rtds_layer(); @@ -128,7 +136,9 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe rtds_config->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); auto* api_config_source = rtds_config->mutable_api_config_source(); api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); - api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); + api_config_source->set_api_type(this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw + ? envoy::config::core::v3::ApiConfigSource::GRPC + : envoy::config::core::v3::ApiConfigSource::DELTA_GRPC); api_config_source->set_set_node_on_first_message_only(true); api_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name( XDS_CLUSTER_NAME); @@ -136,10 +146,10 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe // Add test xDS config tracer. config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* tracer_extension = bootstrap.mutable_xds_config_tracer_extension(); + auto* tracer_extension = bootstrap.mutable_xds_config_tracker_extension(); tracer_extension->set_name("envoy.config.xds.test_xds_tracer"); tracer_extension->mutable_typed_config()->PackFrom( - test::envoy::config::xds::TestXdsConfigTracer()); + test::envoy::config::xds::TestXdsConfigTracker()); }); } @@ -182,7 +192,7 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe void waitforReceiveCount(const int expected_count) { absl::MutexLock l(&lock_); const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { - return TestXdsConfigTracer::ReceiveCount == expected_count; + return TestXdsConfigTracker::IngestedCount == expected_count; }; timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), TestUtility::DefaultTimeout); @@ -191,7 +201,7 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe void waitforFailureCount(const int expected_count) { absl::MutexLock l(&lock_); const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { - return TestXdsConfigTracer::FailureCount >= expected_count; + return TestXdsConfigTracker::FailureCount >= expected_count; }; timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), TestUtility::DefaultTimeout); @@ -201,17 +211,17 @@ class XdsConfigTracerIntegrationTest : public Grpc::GrpcClientIntegrationParamTe uint32_t initial_load_success_{0}; }; -INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTracerIntegrationTest, - GRPC_CLIENT_INTEGRATION_PARAMS); +INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTrackerIntegrationTest, + DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); -TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerSuccessCount) { - TestXdsConfigTracerFactory factory; - Registry::InjectFactory registered(factory); +TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { + TestXdsConfigTrackerFactory factory; + Registry::InjectFactory registered(factory); initialize(); acceptXdsConnection(); - int current_receive_count = TestXdsConfigTracer::ReceiveCount; + int current_ingested_count = TestXdsConfigTracker::IngestedCount; EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, {"some_rtds_layer"}, {}, true)); auto some_rtds_layer = TestUtility::parseYaml(R"EOF( @@ -223,17 +233,18 @@ TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerSuccessCount) { sendDiscoveryResponse( Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); test_server_->waitForCounterGe("runtime.load_success", initial_load_success_ + 1); - int expected_receive_count = ++current_receive_count; - waitforReceiveCount(expected_receive_count); + int expected_ingested_count = ++current_ingested_count; + waitforReceiveCount(expected_ingested_count); - EXPECT_EQ(expected_receive_count, TestXdsConfigTracer::ReceiveCount); + EXPECT_EQ(expected_ingested_count, TestXdsConfigTracker::IngestedCount); + EXPECT_EQ(expected_ingested_count, TestXdsConfigTracker::ReceiveCount); EXPECT_EQ("bar", getRuntimeKey("foo")); EXPECT_EQ("meh", getRuntimeKey("baz")); } -TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerFailureCount) { - TestXdsConfigTracerFactory factory; - Registry::InjectFactory registered(factory); +TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { + TestXdsConfigTrackerFactory factory; + Registry::InjectFactory registered(factory); initialize(); acceptXdsConnection(); @@ -272,7 +283,7 @@ TEST_P(XdsConfigTracerIntegrationTest, XdsConfigTracerFailureCount) { // Message's TypeUrl != Resource's stream->sendGrpcMessage(discovery_response); waitforFailureCount(1); - EXPECT_THAT(TestXdsConfigTracer::ErrorMessage, + EXPECT_THAT(TestXdsConfigTracker::ErrorMessage, HasSubstr("does not match the message-wide type URL")); } diff --git a/test/integration/xds_config_tracker_test.proto b/test/integration/xds_config_tracker_test.proto new file mode 100644 index 0000000000000..7a74cc0fed59e --- /dev/null +++ b/test/integration/xds_config_tracker_test.proto @@ -0,0 +1,7 @@ +syntax = "proto3"; + +package test.envoy.config.xds; + +// Configuration for TestXdsConfigTracker. +message TestXdsConfigTracker { +} From 2c11de7e878df1c5cc8136ca17bb1101768df92b Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 8 Nov 2022 14:27:32 +0000 Subject: [PATCH 12/31] clean up Signed-off-by: Boteng Yao --- envoy/config/subscription.h | 4 ++-- source/common/config/decoded_resource_impl.h | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h index c3c814898cd17..0c4d61a924c8e 100644 --- a/envoy/config/subscription.h +++ b/envoy/config/subscription.h @@ -62,9 +62,9 @@ class DecodedResource { virtual bool hasResource() const PURE; /** - * @return absl::optional of a resource. + * @return optional ref of a resource. */ - virtual OptRef metadata() const PURE; + virtual const OptRef metadata() const PURE; }; using DecodedResourcePtr = std::unique_ptr; diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index 55936cc5f6526..b463cdc0d39f2 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -76,14 +76,14 @@ class DecodedResourceImpl : public DecodedResource { const Protobuf::Message& resource() const override { return *resource_; }; bool hasResource() const override { return has_resource_; } absl::optional ttl() const override { return ttl_; } - OptRef metadata() const override { return metadata_; } + const OptRef metadata() const override { return metadata_; } private: DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, absl::optional name, const Protobuf::RepeatedPtrField& aliases, const ProtobufWkt::Any& resource, bool has_resource, const std::string& version, absl::optional ttl, - OptRef metadata) + const OptRef metadata) : resource_(resource_decoder.decodeResource(resource)), has_resource_(has_resource), name_(name ? *name : resource_decoder.resourceName(*resource_)), aliases_(repeatedPtrFieldToVector(aliases)), version_(version), ttl_(ttl), @@ -96,7 +96,7 @@ class DecodedResourceImpl : public DecodedResource { const std::string version_; // Per resource TTL. const absl::optional ttl_; - OptRef metadata_; + const OptRef metadata_; }; struct DecodedResourcesWrapper { From f19005ed6d15da351a500a444d37539ed60b8a4c Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 8 Nov 2022 14:57:30 +0000 Subject: [PATCH 13/31] fix format Signed-off-by: Boteng Yao --- source/common/config/decoded_resource_impl.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index b463cdc0d39f2..ab7a462dc6391 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -76,7 +76,9 @@ class DecodedResourceImpl : public DecodedResource { const Protobuf::Message& resource() const override { return *resource_; }; bool hasResource() const override { return has_resource_; } absl::optional ttl() const override { return ttl_; } - const OptRef metadata() const override { return metadata_; } + const OptRef metadata() const override { + return metadata_; + } private: DecodedResourceImpl(OpaqueResourceDecoder& resource_decoder, absl::optional name, From 8a4b472f209e19ea6a5000cea722eb86424f9ca0 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 9 Nov 2022 19:43:09 +0000 Subject: [PATCH 14/31] changed name to onConfigReceivedOrFailed Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 12 ++++++------ .../xds_config_tracker_integration_test.cc | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index 8073ad76bdeb3..b7692964344ae 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -69,26 +69,26 @@ class XdsConfigTracker { const Protobuf::RepeatedPtrField& removed_resources) PURE; /** - * Invoked when received or failed to process the DiscoveryResponse. + * Invoked when received or failed to decode and apply the xds configs in the response. * The ProcessingState can be RECIEVED or FALIED. * * @param message The SotW discovery response message body. * @param details The process state and error details. */ virtual void - onResponseReceiveOrFail(const envoy::service::discovery::v3::DiscoveryResponse& message, - const ProcessingDetails& details) PURE; + onConfigReceivedOrFailed(const envoy::service::discovery::v3::DiscoveryResponse& message, + const ProcessingDetails& details) PURE; /** - * Invoked when received or failed to process the DeltaDiscoveryResponse. + * Invoked when received or failed to decode and apply the xds configs in the response. * The ProcessingState can be RECIEVED or FALIED. * * @param message The Delta discovery response message body. * @param details The process state and error details. */ virtual void - onResponseReceiveOrFail(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, - const ProcessingDetails& details) PURE; + onConfigReceivedOrFailed(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, + const ProcessingDetails& details) PURE; }; using XdsConfigTrackerPtr = std::unique_ptr; diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 66d68a4bb1510..5393bc0c881e5 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -43,7 +43,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { ++IngestedCount; } - void onResponseReceiveOrFail(const envoy::service::discovery::v3::DiscoveryResponse&, + void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DiscoveryResponse&, const Config::ProcessingDetails& process_details) override { countState(process_details.state_); if (process_details.state_ == Config::ProcessingState::FAILED) { @@ -51,7 +51,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { } } - void onResponseReceiveOrFail(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, + void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, const Config::ProcessingDetails& process_details) override { countState(process_details.state_); if (process_details.state_ == Config::ProcessingState::FAILED) { From 11c8def4d249f69015143f24a67e07f6640f2552 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 9 Nov 2022 19:44:08 +0000 Subject: [PATCH 15/31] changed name to onConfigReceivedOrFailed Signed-off-by: Boteng Yao --- source/common/config/grpc_mux_impl.cc | 4 ++-- source/common/config/new_grpc_mux_impl.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 6162b48bf9147..7cd70ce45f29a 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -223,7 +223,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // Processing point when DiscoveryResponse is received. if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onResponseReceiveOrFail(*message, + xds_config_tracker_->onConfigReceivedOrFailed(*message, ProcessingDetails(ProcessingState::RECEIVED)); } @@ -312,7 +312,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // Processing point when there is any exception during the parse and ingestion process. if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onResponseReceiveOrFail( + xds_config_tracker_->onConfigReceivedOrFailed( *message, ProcessingDetails(ProcessingState::FAILED, *error_detail)); } } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index ee05f516768a0..22ff30f805c37 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -93,7 +93,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( // Processing point when DeltaDiscoveryResponse is received. if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onResponseReceiveOrFail(*message, + xds_config_tracker_->onConfigReceivedOrFailed(*message, ProcessingDetails(ProcessingState::RECEIVED)); } @@ -121,7 +121,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( // Processing point to record error if there is any failure after the response is processed. if (xds_config_tracker_.has_value() && ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { - xds_config_tracker_->onResponseReceiveOrFail( + xds_config_tracker_->onConfigReceivedOrFailed( *message, ProcessingDetails(ProcessingState::FAILED, ack.error_detail_)); } kickOffAck(ack); From a48502a14102495be75306346e18d0a0b1ed5854 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 9 Nov 2022 19:47:09 +0000 Subject: [PATCH 16/31] clean up Signed-off-by: Boteng Yao --- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/new_grpc_mux_impl.cc | 2 +- test/integration/xds_config_tracker_integration_test.cc | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 7cd70ce45f29a..60d3de44d5bd2 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -224,7 +224,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // Processing point when DiscoveryResponse is received. if (xds_config_tracker_.has_value()) { xds_config_tracker_->onConfigReceivedOrFailed(*message, - ProcessingDetails(ProcessingState::RECEIVED)); + ProcessingDetails(ProcessingState::RECEIVED)); } if (api_state_.count(type_url) == 0) { diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 22ff30f805c37..bf8cd8a9ff007 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -94,7 +94,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( // Processing point when DeltaDiscoveryResponse is received. if (xds_config_tracker_.has_value()) { xds_config_tracker_->onConfigReceivedOrFailed(*message, - ProcessingDetails(ProcessingState::RECEIVED)); + ProcessingDetails(ProcessingState::RECEIVED)); } auto sub = subscriptions_.find(message->type_url()); diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 5393bc0c881e5..825f02d3b8529 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -44,7 +44,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { } void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DiscoveryResponse&, - const Config::ProcessingDetails& process_details) override { + const Config::ProcessingDetails& process_details) override { countState(process_details.state_); if (process_details.state_ == Config::ProcessingState::FAILED) { ErrorMessage = process_details.error_detail_.message(); @@ -52,7 +52,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { } void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, - const Config::ProcessingDetails& process_details) override { + const Config::ProcessingDetails& process_details) override { countState(process_details.state_); if (process_details.state_ == Config::ProcessingState::FAILED) { ErrorMessage = process_details.error_detail_.message(); From 3efd2b3eef753c52e47f7586e8f02ab8520de093 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 15 Nov 2022 22:17:42 +0000 Subject: [PATCH 17/31] add unified mux xds Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 47 ++---- .../common/config/delta_subscription_state.cc | 15 +- .../common/config/delta_subscription_state.h | 5 +- source/common/config/grpc_mux_impl.cc | 11 +- .../config/new_delta_subscription_state.cc | 12 +- .../config/new_delta_subscription_state.h | 5 +- source/common/config/new_grpc_mux_impl.cc | 9 +- source/common/config/new_grpc_mux_impl.h | 4 +- .../config/old_delta_subscription_state.cc | 11 +- .../config/old_delta_subscription_state.h | 5 +- .../config/subscription_factory_impl.cc | 6 +- source/common/config/watch_map.cc | 5 - source/common/config/watch_map.h | 6 +- source/common/config/xds_mux/BUILD | 1 + .../xds_mux/delta_subscription_state.cc | 11 +- .../config/xds_mux/delta_subscription_state.h | 7 +- source/common/config/xds_mux/grpc_mux_impl.cc | 20 ++- source/common/config/xds_mux/grpc_mux_impl.h | 7 +- .../config/xds_mux/sotw_subscription_state.cc | 19 ++- .../config/xds_mux/sotw_subscription_state.h | 6 +- .../config/xds_mux/subscription_state.h | 10 +- .../common/upstream/cluster_manager_impl.cc | 7 +- .../config/delta_subscription_impl_test.cc | 3 +- .../delta_subscription_state_old_test.cc | 5 +- .../config/delta_subscription_state_test.cc | 9 +- .../config/delta_subscription_test_harness.h | 3 +- .../config/grpc_subscription_test_harness.h | 2 +- test/common/config/new_grpc_mux_impl_test.cc | 3 +- .../config/sotw_subscription_state_test.cc | 4 +- test/common/config/watch_map_test.cc | 35 ++--- test/common/config/xds_grpc_mux_impl_test.cc | 14 +- test/common/grpc/grpc_client_integration.h | 6 + .../xds_config_tracker_integration_test.cc | 137 +++++++----------- 33 files changed, 220 insertions(+), 230 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index b7692964344ae..3ef80a4dc9017 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -7,6 +7,7 @@ #include "envoy/config/subscription.h" #include "envoy/config/typed_config.h" #include "envoy/protobuf/message_validator.h" +#include "envoy/stats/scope.h" #include "source/common/protobuf/protobuf.h" @@ -15,22 +16,6 @@ namespace Envoy { namespace Config { -// The status of processing Delta/DiscoveryResponse. -enum ProcessingState { - // Successfully got the resources or message. - RECEIVED = 0, - // Failed to apply the resources. - FAILED = 1, -}; - -struct ProcessingDetails { - ProcessingDetails(const ProcessingState state) : state_(state){}; - ProcessingDetails(const ProcessingState state, const ::google::rpc::Status error_detail) - : state_(state), error_detail_(error_detail){}; - const ProcessingState state_; - ::google::rpc::Status error_detail_; -}; - /** * An interface for hooking into xDS update events to provide the ablility to use some external * processor in xDS update. This traker provides the process point when the discovery response @@ -52,7 +37,7 @@ class XdsConfigTracker { * @param type_url The type url of xDS message. * @param resources A list of decoded resources to add to the current state. */ - virtual void onConfigIngested(const absl::string_view type_url, + virtual void onConfigAccepted(const absl::string_view type_url, const std::vector& resources) PURE; /** @@ -63,32 +48,29 @@ class XdsConfigTracker { * @param added_resources A list of decoded resources to add to the current state. * @param removed_resources A list of resources to remove from the current state. */ - virtual void - onConfigIngested(const absl::string_view type_url, - const std::vector& added_resources, - const Protobuf::RepeatedPtrField& removed_resources) PURE; + virtual void onConfigAccepted( + const absl::string_view type_url, + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources) PURE; /** - * Invoked when received or failed to decode and apply the xds configs in the response. - * The ProcessingState can be RECIEVED or FALIED. + * Invoked when xds configs are rejected during xDS ingestion. * * @param message The SotW discovery response message body. * @param details The process state and error details. */ - virtual void - onConfigReceivedOrFailed(const envoy::service::discovery::v3::DiscoveryResponse& message, - const ProcessingDetails& details) PURE; + virtual void onConfigRejected(const envoy::service::discovery::v3::DiscoveryResponse& message, + const absl::string_view error_detail) PURE; /** - * Invoked when received or failed to decode and apply the xds configs in the response. - * The ProcessingState can be RECIEVED or FALIED. + * Invoked when xds configs are rejected during xDS ingestion. * * @param message The Delta discovery response message body. * @param details The process state and error details. */ virtual void - onConfigReceivedOrFailed(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, - const ProcessingDetails& details) PURE; + onConfigRejected(const envoy::service::discovery::v3::DeltaDiscoveryResponse& message, + const absl::string_view error_detail) PURE; }; using XdsConfigTrackerPtr = std::unique_ptr; @@ -106,9 +88,10 @@ class XdsConfigTrackerFactory : public Config::TypedFactory { */ virtual XdsConfigTrackerPtr createXdsConfigTracker(const ProtobufWkt::Any& config, - ProtobufMessage::ValidationVisitor& validation_visitor) PURE; + ProtobufMessage::ValidationVisitor& validation_visitor, + Event::Dispatcher& dispatcher, Stats::Scope& stats) PURE; - std::string category() const override { return "envoy.config.xds_trackers"; } + std::string category() const override { return "envoy.config.xds_tracker"; } }; } // namespace Config diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 39429f88b4a5a..850dd565238d0 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -9,13 +9,16 @@ namespace { DeltaSubscriptionStateVariant getState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher) { + Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker) { if (Runtime::runtimeFeatureEnabled("envoy.restart_features.explicit_wildcard_resource")) { return DeltaSubscriptionStateVariant(absl::in_place_type, - std::move(type_url), watch_map, local_info, dispatcher); + std::move(type_url), watch_map, local_info, dispatcher, + xds_config_tracker); } else { return DeltaSubscriptionStateVariant(absl::in_place_type, - std::move(type_url), watch_map, local_info, dispatcher); + std::move(type_url), watch_map, local_info, dispatcher, + xds_config_tracker); } } @@ -24,8 +27,10 @@ DeltaSubscriptionStateVariant getState(std::string type_url, DeltaSubscriptionState::DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher) - : state_(getState(std::move(type_url), watch_map, local_info, dispatcher)) {} + Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker) + : state_(getState(std::move(type_url), watch_map, local_info, dispatcher, xds_config_tracker)) { +} void DeltaSubscriptionState::updateSubscriptionInterest( const absl::flat_hash_set& cur_added, diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 6b613ade0b4fa..ec57f555280c9 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/local_info/local_info.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -20,7 +21,8 @@ using DeltaSubscriptionStateVariant = class DeltaSubscriptionState : public Logger::Loggable { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker); void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, const absl::flat_hash_set& cur_removed); @@ -37,6 +39,7 @@ class DeltaSubscriptionState : public Logger::Loggable { private: DeltaSubscriptionStateVariant state_; + XdsConfigTrackerOptRef xds_config_tracker_; }; } // namespace Config diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 60d3de44d5bd2..6f730dc950348 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -221,12 +221,6 @@ void GrpcMuxImpl::onDiscoveryResponse( const std::string type_url = message->type_url(); ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url, message->version_info()); - // Processing point when DiscoveryResponse is received. - if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onConfigReceivedOrFailed(*message, - ProcessingDetails(ProcessingState::RECEIVED)); - } - if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a // protocol violation @@ -297,7 +291,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // Processing point when resources are successfully ingested. if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onConfigIngested(type_url, resources); + xds_config_tracker_->onConfigAccepted(type_url, resources); } } END_TRY @@ -312,8 +306,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // Processing point when there is any exception during the parse and ingestion process. if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onConfigReceivedOrFailed( - *message, ProcessingDetails(ProcessingState::FAILED, *error_detail)); + xds_config_tracker_->onConfigRejected(*message, error_detail->message()); } } previously_fetched_data_ = true; diff --git a/source/common/config/new_delta_subscription_state.cc b/source/common/config/new_delta_subscription_state.cc index 6b495b7bbba50..9cf27f53feda5 100644 --- a/source/common/config/new_delta_subscription_state.cc +++ b/source/common/config/new_delta_subscription_state.cc @@ -14,7 +14,8 @@ namespace Config { NewDeltaSubscriptionState::NewDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher) + Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker) // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on // empty resources as updates. : supports_heartbeats_(type_url != "envoy.config.route.v3.VirtualHost"), @@ -36,7 +37,8 @@ NewDeltaSubscriptionState::NewDeltaSubscriptionState(std::string type_url, watch_map_.onConfigUpdate({}, removed_resources, ""); }, dispatcher, dispatcher.timeSource()), - type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info) {} + type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), + xds_config_tracker_(xds_config_tracker) {} void NewDeltaSubscriptionState::updateSubscriptionInterest( const absl::flat_hash_set& cur_added, @@ -232,6 +234,12 @@ void NewDeltaSubscriptionState::handleGoodResponse( watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(), message.system_version_info()); + // Processing point when resources are successfully ingested. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources, + message.removed_resources()); + } + if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.delta_xds_subscription_state_tracking_fix")) { const auto scoped_update = ttl_.scopedTtlUpdate(); diff --git a/source/common/config/new_delta_subscription_state.h b/source/common/config/new_delta_subscription_state.h index 9ef841cffb22f..177de30f168ed 100644 --- a/source/common/config/new_delta_subscription_state.h +++ b/source/common/config/new_delta_subscription_state.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" #include "envoy/local_info/local_info.h" @@ -77,7 +78,8 @@ namespace Config { class NewDeltaSubscriptionState : public Logger::Loggable { public: NewDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker); // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, @@ -163,6 +165,7 @@ class NewDeltaSubscriptionState : public Logger::Loggable { const std::string type_url_; UntypedConfigUpdateCallbacks& watch_map_; const LocalInfo::LocalInfo& local_info_; + XdsConfigTrackerOptRef xds_config_tracker_; bool in_initial_legacy_wildcard_{true}; bool any_request_sent_yet_in_current_stream_{}; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index bf8cd8a9ff007..daf2c9f479f9f 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -91,12 +91,6 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); - // Processing point when DeltaDiscoveryResponse is received. - if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onConfigReceivedOrFailed(*message, - ProcessingDetails(ProcessingState::RECEIVED)); - } - auto sub = subscriptions_.find(message->type_url()); if (sub == subscriptions_.end()) { ENVOY_LOG(warn, @@ -121,8 +115,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( // Processing point to record error if there is any failure after the response is processed. if (xds_config_tracker_.has_value() && ack.error_detail_.code() != Grpc::Status::WellKnownGrpcStatus::Ok) { - xds_config_tracker_->onConfigReceivedOrFailed( - *message, ProcessingDetails(ProcessingState::FAILED, ack.error_detail_)); + xds_config_tracker_->onConfigRejected(*message, ack.error_detail_.message()); } kickOffAck(ack); Memory::Utils::tryShrinkHeap(); diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index bafa2b114d094..60625f717263d 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -87,8 +87,8 @@ class NewGrpcMuxImpl const bool use_namespace_matching, Event::Dispatcher& dispatcher, CustomConfigValidators& config_validators, XdsConfigTrackerOptRef xds_config_tracker) - : watch_map_(use_namespace_matching, type_url, config_validators, xds_config_tracker), - sub_state_(type_url, watch_map_, local_info, dispatcher) {} + : watch_map_(use_namespace_matching, type_url, config_validators), + sub_state_(type_url, watch_map_, local_info, dispatcher, xds_config_tracker) {} WatchMap watch_map_; DeltaSubscriptionState sub_state_; diff --git a/source/common/config/old_delta_subscription_state.cc b/source/common/config/old_delta_subscription_state.cc index 993727feaa768..69461ae9158b0 100644 --- a/source/common/config/old_delta_subscription_state.cc +++ b/source/common/config/old_delta_subscription_state.cc @@ -14,7 +14,8 @@ namespace Config { OldDeltaSubscriptionState::OldDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, const LocalInfo::LocalInfo& local_info, - Event::Dispatcher& dispatcher) + Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker) // TODO(snowp): Hard coding VHDS here is temporary until we can move it away from relying on // empty resources as updates. : supports_heartbeats_(type_url != "envoy.config.route.v3.VirtualHost"), @@ -30,7 +31,7 @@ OldDeltaSubscriptionState::OldDeltaSubscriptionState(std::string type_url, }, dispatcher, dispatcher.timeSource()), type_url_(std::move(type_url)), watch_map_(watch_map), local_info_(local_info), - dispatcher_(dispatcher) {} + dispatcher_(dispatcher), xds_config_tracker_(xds_config_tracker) {} void OldDeltaSubscriptionState::updateSubscriptionInterest( const absl::flat_hash_set& cur_added, @@ -138,6 +139,12 @@ void OldDeltaSubscriptionState::handleGoodResponse( watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(), message.system_version_info()); + // Processing point when resources are successfully ingested. + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources, + message.removed_resources()); + } + // If a resource is gone, there is no longer a meaningful version for it that makes sense to // provide to the server upon stream reconnect: either it will continue to not exist, in which // case saying nothing is fine, or the server will bring back something new, which we should diff --git a/source/common/config/old_delta_subscription_state.h b/source/common/config/old_delta_subscription_state.h index f8aef137f1337..f1efcb5544450 100644 --- a/source/common/config/old_delta_subscription_state.h +++ b/source/common/config/old_delta_subscription_state.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" #include "envoy/local_info/local_info.h" @@ -25,7 +26,8 @@ namespace Config { class OldDeltaSubscriptionState : public Logger::Loggable { public: OldDeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher); + const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker); // Update which resources we're interested in subscribing to. void updateSubscriptionInterest(const absl::flat_hash_set& cur_added, @@ -107,6 +109,7 @@ class OldDeltaSubscriptionState : public Logger::Loggable { UntypedConfigUpdateCallbacks& watch_map_; const LocalInfo::LocalInfo& local_info_; Event::Dispatcher& dispatcher_; + XdsConfigTrackerOptRef xds_config_tracker_; std::chrono::milliseconds init_fetch_timeout_; bool any_request_sent_yet_in_current_stream_{}; diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index e27786eb112cb..d08dce8d3efae 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -88,7 +88,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( dispatcher_, sotwGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, api_config_source.set_node_on_first_message_only(), std::move(custom_config_validators), - xds_resources_delegate_, control_plane_id); + xds_config_tracker_, xds_resources_delegate_, control_plane_id); } else { mux = std::make_shared( local_info_, @@ -117,8 +117,8 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ->createUncachedRawAsyncClient(), dispatcher_, deltaGrpcMethod(type_url), api_.randomGenerator(), scope, Utility::parseRateLimitSettings(api_config_source), local_info_, - api_config_source.set_node_on_first_message_only(), - std::move(custom_config_validators)); + api_config_source.set_node_on_first_message_only(), std::move(custom_config_validators), + xds_config_tracker_); } else { mux = std::make_shared( Config::Utility::factoryForGrpcApiConfigSource(cm_.grpcAsyncClientManager(), diff --git a/source/common/config/watch_map.cc b/source/common/config/watch_map.cc index e86681a36e570..803bc729d210c 100644 --- a/source/common/config/watch_map.cc +++ b/source/common/config/watch_map.cc @@ -242,11 +242,6 @@ void WatchMap::onConfigUpdate( cur_watch->callbacks_.onConfigUpdate({}, {}, system_version_info); } } - - // Processing point when resources are successfully ingested. - if (xds_config_tracker_.has_value()) { - xds_config_tracker_->onConfigIngested(type_url_, decoded_resources, removed_resources); - } } void WatchMap::onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) { diff --git a/source/common/config/watch_map.h b/source/common/config/watch_map.h index f39b6ddf32c42..7c0da75df7f5b 100644 --- a/source/common/config/watch_map.h +++ b/source/common/config/watch_map.h @@ -5,7 +5,6 @@ #include #include "envoy/config/subscription.h" -#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/common/assert.h" @@ -63,9 +62,9 @@ struct Watch { class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable { public: WatchMap(const bool use_namespace_matching, const std::string& type_url, - CustomConfigValidators& config_validators, XdsConfigTrackerOptRef xds_config_tracker) + CustomConfigValidators& config_validators) : use_namespace_matching_(use_namespace_matching), type_url_(type_url), - config_validators_(config_validators), xds_config_tracker_(xds_config_tracker) {} + config_validators_(config_validators) {} // Adds 'callbacks' to the WatchMap, with every possible resource being watched. // (Use updateWatchInterest() to narrow it down to some specific names). @@ -134,7 +133,6 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::LoggableonConfigAccepted(message.type_url(), non_heartbeat_resources, + message.removed_resources()); + } + if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.delta_xds_subscription_state_tracking_fix")) { const auto scoped_update = ttl_.scopedTtlUpdate(); diff --git a/source/common/config/xds_mux/delta_subscription_state.h b/source/common/config/xds_mux/delta_subscription_state.h index 64daec95baea5..22d9ee371245f 100644 --- a/source/common/config/xds_mux/delta_subscription_state.h +++ b/source/common/config/xds_mux/delta_subscription_state.h @@ -20,7 +20,7 @@ class DeltaSubscriptionState envoy::service::discovery::v3::DeltaDiscoveryRequest> { public: DeltaSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& watch_map, - Event::Dispatcher& dispatcher); + Event::Dispatcher& dispatcher, XdsConfigTrackerOptRef xds_config_tracker); ~DeltaSubscriptionState() override; @@ -114,10 +114,11 @@ class DeltaSubscriptionStateFactory : public SubscriptionStateFactory makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, - OpaqueResourceDecoderSharedPtr, + OpaqueResourceDecoderSharedPtr, XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef /*xds_resources_delegate*/, const std::string& /*target_xds_authority*/) override { - return std::make_unique(type_url, callbacks, dispatcher_); + return std::make_unique(type_url, callbacks, dispatcher_, + xds_config_tracker); } private: diff --git a/source/common/config/xds_mux/grpc_mux_impl.cc b/source/common/config/xds_mux/grpc_mux_impl.cc index aa8f23d8f9161..9848295fd7ce8 100644 --- a/source/common/config/xds_mux/grpc_mux_impl.cc +++ b/source/common/config/xds_mux/grpc_mux_impl.cc @@ -42,7 +42,8 @@ GrpcMuxImpl::GrpcMuxImpl( Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, CustomConfigValidatorsPtr&& config_validators, - XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority) + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, + const std::string& target_xds_authority) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), subscription_state_factory_(std::move(subscription_state_factory)), @@ -51,7 +52,7 @@ GrpcMuxImpl::GrpcMuxImpl( [this](absl::string_view resource_type_url) { onDynamicContextUpdate(resource_type_url); })), - config_validators_(std::move(config_validators)), + config_validators_(std::move(config_validators)), xds_config_tracker_(xds_config_tracker), xds_resources_delegate_(xds_resources_delegate), target_xds_authority_(target_xds_authority) { Config::Utility::checkLocalInfo("ads", local_info); AllMuxes::get().insert(this); @@ -87,11 +88,12 @@ Config::GrpcMuxWatchPtr GrpcMuxImpl::addWatch( watch_map = watch_maps_ .emplace(type_url, std::make_unique(options.use_namespace_matching_, type_url, - *config_validators_.get(), absl::nullopt)) + *config_validators_.get())) .first; subscriptions_.emplace(type_url, subscription_state_factory_->makeSubscriptionState( type_url, *watch_maps_[type_url], resource_decoder, - xds_resources_delegate_, target_xds_authority_)); + xds_config_tracker_, xds_resources_delegate_, + target_xds_authority_)); subscription_ordering_.emplace_back(type_url); } @@ -364,10 +366,11 @@ GrpcMuxDelta::GrpcMuxDelta(Grpc::RawAsyncClientPtr&& async_client, Event::Dispat Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, bool skip_subsequent_node, - CustomConfigValidatorsPtr&& config_validators) + CustomConfigValidatorsPtr&& config_validators, + XdsConfigTrackerOptRef xds_config_tracker) : GrpcMuxImpl(std::make_unique(dispatcher), skip_subsequent_node, local_info, std::move(async_client), dispatcher, service_method, random, scope, - rate_limit_settings, std::move(config_validators)) {} + rate_limit_settings, std::move(config_validators), xds_config_tracker) {} // GrpcStreamCallbacks for GrpcMuxDelta void GrpcMuxDelta::requestOnDemandUpdate(const std::string& type_url, @@ -386,12 +389,13 @@ GrpcMuxSotw::GrpcMuxSotw(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatch const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, bool skip_subsequent_node, CustomConfigValidatorsPtr&& config_validators, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority) : GrpcMuxImpl(std::make_unique(dispatcher), skip_subsequent_node, local_info, std::move(async_client), dispatcher, service_method, random, scope, - rate_limit_settings, std::move(config_validators), xds_resources_delegate, - target_xds_authority) {} + rate_limit_settings, std::move(config_validators), xds_config_tracker, + xds_resources_delegate, target_xds_authority) {} Config::GrpcMuxWatchPtr NullGrpcMuxImpl::addWatch(const std::string&, const absl::flat_hash_set&, diff --git a/source/common/config/xds_mux/grpc_mux_impl.h b/source/common/config/xds_mux/grpc_mux_impl.h index 9e115358b0cab..5f960fbaebf54 100644 --- a/source/common/config/xds_mux/grpc_mux_impl.h +++ b/source/common/config/xds_mux/grpc_mux_impl.h @@ -9,6 +9,7 @@ #include "envoy/common/token_bucket.h" #include "envoy/config/grpc_mux.h" #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/event/dispatcher.h" #include "envoy/grpc/status.h" @@ -64,6 +65,7 @@ class GrpcMuxImpl : public GrpcStreamCallbacks, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, CustomConfigValidatorsPtr&& config_validators, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate = absl::nullopt, const std::string& target_xds_authority = ""); @@ -206,6 +208,7 @@ class GrpcMuxImpl : public GrpcStreamCallbacks, const LocalInfo::LocalInfo& local_info_; Common::CallbackHandlePtr dynamic_update_callback_handle_; CustomConfigValidatorsPtr config_validators_; + XdsConfigTrackerOptRef xds_config_tracker_; XdsResourcesDelegateOptRef xds_resources_delegate_; const std::string target_xds_authority_; @@ -222,7 +225,8 @@ class GrpcMuxDelta : public GrpcMuxImplonConfigAccepted(message.type_url(), non_heartbeat_resources); + } + // Send the resources to the xDS delegate, if configured. if (xds_resources_delegate_.has_value()) { XdsConfigSourceId source_id{target_xds_authority_, message.type_url()}; diff --git a/source/common/config/xds_mux/sotw_subscription_state.h b/source/common/config/xds_mux/sotw_subscription_state.h index 603d2f440663e..fb063ada022f8 100644 --- a/source/common/config/xds_mux/sotw_subscription_state.h +++ b/source/common/config/xds_mux/sotw_subscription_state.h @@ -23,6 +23,7 @@ class SotwSubscriptionState SotwSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& callbacks, Event::Dispatcher& dispatcher, OpaqueResourceDecoderSharedPtr resource_decoder, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority); ~SotwSubscriptionState() override; @@ -74,11 +75,12 @@ class SotwSubscriptionStateFactory : public SubscriptionStateFactory makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, OpaqueResourceDecoderSharedPtr resource_decoder, + XdsConfigTrackerOptRef xds_config_tracker, XdsResourcesDelegateOptRef xds_resources_delegate, const std::string& target_xds_authority) override { return std::make_unique(type_url, callbacks, dispatcher_, - resource_decoder, xds_resources_delegate, - target_xds_authority); + resource_decoder, xds_config_tracker, + xds_resources_delegate, target_xds_authority); } private: diff --git a/source/common/config/xds_mux/subscription_state.h b/source/common/config/xds_mux/subscription_state.h index d165eb9b96e4b..41223d0829d44 100644 --- a/source/common/config/xds_mux/subscription_state.h +++ b/source/common/config/xds_mux/subscription_state.h @@ -5,6 +5,7 @@ #include "envoy/common/pure.h" #include "envoy/config/subscription.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/config/xds_resources_delegate.h" #include "envoy/event/dispatcher.h" #include "envoy/service/discovery/v3/discovery.pb.h" @@ -34,12 +35,13 @@ class BaseSubscriptionState : public SubscriptionState, // Note that, outside of tests, we expect callbacks to always be a WatchMap. BaseSubscriptionState(std::string type_url, UntypedConfigUpdateCallbacks& callbacks, Event::Dispatcher& dispatcher, + XdsConfigTrackerOptRef xds_config_tracker = absl::nullopt, XdsResourcesDelegateOptRef xds_resources_delegate = absl::nullopt, const std::string& target_xds_authority = "") : ttl_([this](const std::vector& expired) { ttlExpiryCallback(expired); }, dispatcher, dispatcher.timeSource()), type_url_(std::move(type_url)), callbacks_(callbacks), dispatcher_(dispatcher), - xds_resources_delegate_(xds_resources_delegate), + xds_config_tracker_(xds_config_tracker), xds_resources_delegate_(xds_resources_delegate), target_xds_authority_(target_xds_authority) {} virtual ~BaseSubscriptionState() = default; @@ -68,6 +70,10 @@ class BaseSubscriptionState : public SubscriptionState, TRY_ASSERT_MAIN_THREAD { handleGoodResponse(response); } END_TRY catch (const EnvoyException& e) { + if (xds_config_tracker_.has_value()) { + xds_config_tracker_->onConfigRejected(response, + Config::Utility::truncateGrpcStatusMessage(e.what())); + } handleBadResponse(e, ack); } previously_fetched_data_ = true; @@ -121,6 +127,7 @@ class BaseSubscriptionState : public SubscriptionState, Event::Dispatcher& dispatcher_; bool dynamic_context_changed_{}; std::string control_plane_identifier_{}; + XdsConfigTrackerOptRef xds_config_tracker_; XdsResourcesDelegateOptRef xds_resources_delegate_; const std::string target_xds_authority_; bool previously_fetched_data_{}; @@ -133,6 +140,7 @@ template class SubscriptionStateFactory { virtual std::unique_ptr makeSubscriptionState(const std::string& type_url, UntypedConfigUpdateCallbacks& callbacks, OpaqueResourceDecoderSharedPtr resource_decoder, + XdsConfigTrackerOptRef xds_config_tracker = absl::nullopt, XdsResourcesDelegateOptRef xds_resources_delegate = absl::nullopt, const std::string& target_xds_authority = "") PURE; }; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index ec41ff11d14d9..cac426014ba4e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -335,7 +335,7 @@ ClusterManagerImpl::ClusterManagerImpl( bootstrap.xds_config_tracker_extension()); xds_config_tracker_ = tracer_factory.createXdsConfigTracker( bootstrap.xds_config_tracker_extension().typed_config(), - validation_context.dynamicValidationVisitor()); + validation_context.dynamicValidationVisitor(), main_thread_dispatcher, stats); } subscription_factory_ = std::make_unique( @@ -397,7 +397,7 @@ ClusterManagerImpl::ClusterManagerImpl( random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info, dyn_resources.ads_config().set_node_on_first_message_only(), - std::move(custom_config_validators)); + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracker_.get())); } else { ads_mux_ = std::make_shared( Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, @@ -428,7 +428,8 @@ ClusterManagerImpl::ClusterManagerImpl( random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info, bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only(), - std::move(custom_config_validators), xds_delegate_opt_ref, target_xds_authority); + std::move(custom_config_validators), makeOptRefFromPtr(xds_config_tracker_.get()), + xds_delegate_opt_ref, target_xds_authority); } else { ads_mux_ = std::make_shared( local_info, diff --git a/test/common/config/delta_subscription_impl_test.cc b/test/common/config/delta_subscription_impl_test.cc index 993cdc2f6f598..eeef0307b7a78 100644 --- a/test/common/config/delta_subscription_impl_test.cc +++ b/test/common/config/delta_subscription_impl_test.cc @@ -153,7 +153,8 @@ TEST_P(DeltaSubscriptionNoGrpcStreamTest, NoGrpcStream) { xds_context = std::make_shared( std::unique_ptr(async_client), dispatcher, *method_descriptor, random, stats_store, rate_limit_settings, local_info, false, - std::make_unique>()); + std::make_unique>(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } else { xds_context = std::make_shared( std::unique_ptr(async_client), dispatcher, *method_descriptor, diff --git a/test/common/config/delta_subscription_state_old_test.cc b/test/common/config/delta_subscription_state_old_test.cc index fa564c6ca21ad..f5fa768901c3e 100644 --- a/test/common/config/delta_subscription_state_old_test.cc +++ b/test/common/config/delta_subscription_state_old_test.cc @@ -1,6 +1,7 @@ #include #include "envoy/config/cluster/v3/cluster.pb.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/config/delta_subscription_state.h" @@ -42,8 +43,8 @@ class OldDeltaSubscriptionStateTestBase : public testing::Test { scoped_runtime.mergeValues({ {"envoy.restart_features.explicit_wildcard_resource", "false"}, }); - state_ = std::make_unique(type_url, callbacks_, - local_info_, dispatcher_); + state_ = std::make_unique( + type_url, callbacks_, local_info_, dispatcher_, XdsConfigTrackerOptRef()); } updateSubscriptionInterest(initial_resources, {}); auto cur_request = getNextRequestAckless(); diff --git a/test/common/config/delta_subscription_state_test.cc b/test/common/config/delta_subscription_state_test.cc index 9f04784d07cd1..4f113cb57762e 100644 --- a/test/common/config/delta_subscription_state_test.cc +++ b/test/common/config/delta_subscription_state_test.cc @@ -1,6 +1,7 @@ #include #include "envoy/config/cluster/v3/cluster.pb.h" +#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "source/common/config/delta_subscription_state.h" @@ -59,11 +60,11 @@ class DeltaSubscriptionStateTestBase : public testing::TestWithParam(type_url, callbacks_, - dispatcher_); + state_ = std::make_unique( + type_url, callbacks_, dispatcher_, XdsConfigTrackerOptRef()); } else { - state_ = std::make_unique(type_url, callbacks_, - local_info_, dispatcher_); + state_ = std::make_unique( + type_url, callbacks_, local_info_, dispatcher_, XdsConfigTrackerOptRef()); } } diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index 96077d8619604..c542426deab96 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -52,7 +52,8 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { xds_context_ = std::make_shared( std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, local_info_, false, - std::make_unique>()); + std::make_unique>(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } else { xds_context_ = std::make_shared( std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index eeb13fbf4d041..4f30aff773045 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -60,7 +60,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { mux_ = std::make_shared( std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, random_, stats_store_, rate_limit_settings_, local_info_, true, - std::move(config_validators_)); + std::move(config_validators_), /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } else { mux_ = std::make_shared( local_info_, std::unique_ptr(async_client_), dispatcher_, diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 197abaad19423..7157acdce7f86 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -66,7 +66,8 @@ class NewGrpcMuxImplTestBase : public testing::TestWithParam { std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - random_, stats_, rate_limit_settings_, local_info_, false, std::move(config_validators_)); + random_, stats_, rate_limit_settings_, local_info_, false, std::move(config_validators_), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); return; } grpc_mux_ = std::make_unique( diff --git a/test/common/config/sotw_subscription_state_test.cc b/test/common/config/sotw_subscription_state_test.cc index 7bc5592175c8e..462949e96c3cf 100644 --- a/test/common/config/sotw_subscription_state_test.cc +++ b/test/common/config/sotw_subscription_state_test.cc @@ -100,8 +100,8 @@ class SotwSubscriptionStateTest : public testing::Test { xds_resources_delegate_ = std::make_unique(); state_ = std::make_unique( Config::getTypeUrl(), callbacks_, - dispatcher_, resource_decoder_, *xds_resources_delegate_, - /*target_xds_authority=*/"some_random_xds_server"); + dispatcher_, resource_decoder_, /*xds_config_tracker=*/XdsConfigTrackerOptRef(), + *xds_resources_delegate_, /*target_xds_authority=*/"some_random_xds_server"); state_->updateSubscriptionInterest({"name1", "name2", "name3"}, {}); auto cur_request = getNextDiscoveryRequestAckless(); EXPECT_THAT(cur_request->resource_names(), UnorderedElementsAre("name1", "name2", "name3")); diff --git a/test/common/config/watch_map_test.cc b/test/common/config/watch_map_test.cc index 7ad48061f2bae..76cfe31768367 100644 --- a/test/common/config/watch_map_test.cc +++ b/test/common/config/watch_map_test.cc @@ -3,7 +3,6 @@ #include "envoy/common/exception.h" #include "envoy/config/endpoint/v3/endpoint.pb.h" #include "envoy/config/endpoint/v3/endpoint.pb.validate.h" -#include "envoy/config/xds_config_tracker.h" #include "envoy/service/discovery/v3/discovery.pb.h" #include "envoy/stats/scope.h" @@ -128,8 +127,7 @@ TEST(WatchMapTest, Basic) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); { @@ -203,8 +201,7 @@ TEST(WatchMapTest, Overlap) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -267,9 +264,7 @@ TEST(WatchMapTest, Overlap) { // WatchMap defers deletes and doesn't crash. class SameWatchRemoval : public testing::Test { public: - SameWatchRemoval() - : watch_map_(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()) {} + SameWatchRemoval() : watch_map_(false, "ClusterLoadAssignmentType", config_validators) {} void SetUp() override { envoy::config::endpoint::v3::ClusterLoadAssignment alice; @@ -348,8 +343,7 @@ TEST(WatchMapTest, AddRemoveAdd) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); @@ -406,8 +400,7 @@ TEST(WatchMapTest, UninterestingUpdate) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"alice"}); @@ -452,8 +445,7 @@ TEST(WatchMapTest, WatchingEverything) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); /*Watch* watch1 = */ watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); // watch1 never specifies any names, and so is treated as interested in everything. @@ -490,8 +482,7 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); @@ -525,8 +516,7 @@ TEST(WatchMapTest, DeltaOnConfigUpdate) { TEST(WatchMapTest, OnConfigUpdateFailed) { NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); // calling on empty map doesn't break watch_map.onConfigUpdateFailed(ConfigUpdateFailureReason::UpdateRejected, nullptr); @@ -548,8 +538,7 @@ TEST(WatchMapTest, OnConfigUpdateXdsTpGlobCollections) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"xdstp://foo/bar/baz/*?some=thing&thing=some"}); @@ -594,8 +583,7 @@ TEST(WatchMapTest, OnConfigUpdateXdsTpSingletons) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(false, "ClusterLoadAssignmentType", config_validators); Watch* watch = watch_map.addWatch(callbacks, resource_decoder); watch_map.updateWatchInterest(watch, {"xdstp://foo/bar/baz?some=thing&thing=some"}); @@ -636,8 +624,7 @@ TEST(WatchMapTest, OnConfigUpdateUsingNamespaces) { TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); NiceMock config_validators; - WatchMap watch_map(true, "ClusterLoadAssignmentType", config_validators, - XdsConfigTrackerOptRef()); + WatchMap watch_map(true, "ClusterLoadAssignmentType", config_validators); Watch* watch1 = watch_map.addWatch(callbacks1, resource_decoder); Watch* watch2 = watch_map.addWatch(callbacks2, resource_decoder); Watch* watch3 = watch_map.addWatch(callbacks3, resource_decoder); diff --git a/test/common/config/xds_grpc_mux_impl_test.cc b/test/common/config/xds_grpc_mux_impl_test.cc index 4147c14a9531f..3a139697dcc29 100644 --- a/test/common/config/xds_grpc_mux_impl_test.cc +++ b/test/common/config/xds_grpc_mux_impl_test.cc @@ -63,7 +63,8 @@ class GrpcMuxImplTestBase : public testing::Test { std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - random_, stats_, rate_limit_settings_, local_info_, true, std::move(config_validators_)); + random_, stats_, rate_limit_settings_, local_info_, true, std::move(config_validators_), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } void setup(const RateLimitSettings& custom_rate_limit_settings) { @@ -72,7 +73,7 @@ class GrpcMuxImplTestBase : public testing::Test { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, custom_rate_limit_settings, local_info_, true, - std::move(config_validators_)); + std::move(config_validators_), /*xds_config_tracker=*/XdsConfigTrackerOptRef()); } void expectSendMessage(const std::string& type_url, @@ -895,7 +896,8 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, local_info_, true, - std::make_unique>()), + std::make_unique>(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " "--service-node and --service-cluster options."); @@ -909,7 +911,8 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, local_info_, true, - std::make_unique>()), + std::make_unique>(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()), EnvoyException, "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " "--service-node and --service-cluster options."); @@ -1021,7 +1024,8 @@ TEST_F(GrpcMuxImplTest, AllMuxesStateTest) { *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), random_, stats_, rate_limit_settings_, local_info_, true, - std::make_unique>()); + std::make_unique>(), + /*xds_config_tracker=*/XdsConfigTrackerOptRef()); Config::XdsMux::GrpcMuxSotw::shutdownAll(); diff --git a/test/common/grpc/grpc_client_integration.h b/test/common/grpc/grpc_client_integration.h index 4bec40b1e7ec9..bc3092d419c5f 100644 --- a/test/common/grpc/grpc_client_integration.h +++ b/test/common/grpc/grpc_client_integration.h @@ -150,6 +150,12 @@ class DeltaSotwIntegrationParamTest testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ testing::ValuesIn(TestEnvironment::getsGrpcVersionsForTest()), \ testing::Values(Grpc::LegacyOrUnified::Legacy, Grpc::LegacyOrUnified::Unified)) +#define DELTA_SOTW_UNIFIED_GRPC_CLIENT_INTEGRATION_PARAMS \ + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), \ + testing::ValuesIn(TestEnvironment::getsGrpcVersionsForTest()), \ + testing::Values(Grpc::SotwOrDelta::Sotw, Grpc::SotwOrDelta::Delta, \ + Grpc::SotwOrDelta::UnifiedSotw, \ + Grpc::SotwOrDelta::UnifiedDelta)) } // namespace Grpc } // namespace Envoy diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 825f02d3b8529..e6274ab62a6c1 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -24,77 +24,73 @@ namespace { constexpr char XDS_CLUSTER_NAME[] = "xds_cluster"; +/** + * All stats for this xds tracker. @see stats_macros.h + */ +#define ALL_TEST_XDS_TRACKER_STATS(COUNTER) \ + COUNTER(on_config_accepted) \ + COUNTER(on_config_rejected) + +/** + * Struct definition for stats. @see stats_macros.h + */ +struct TestXdsTrackerStats { + ALL_TEST_XDS_TRACKER_STATS(GENERATE_COUNTER_STRUCT) +}; + class TestXdsConfigTracker : public Config::XdsConfigTracker { public: - TestXdsConfigTracker() { - ReceiveCount = 0; - IngestedCount = 0; - FailureCount = 0; + TestXdsConfigTracker(Stats::Scope& scope) : stats_(generateStats("test_xds_tracker", scope)) { ErrorMessage = ""; } - void onConfigIngested(const absl::string_view, + void onConfigAccepted(const absl::string_view, const std::vector&) override { - ++IngestedCount; + stats_.on_config_accepted_.inc(); } - void onConfigIngested(const absl::string_view, const std::vector&, + void onConfigAccepted(const absl::string_view, + const Protobuf::RepeatedPtrField&, const Protobuf::RepeatedPtrField&) override { - ++IngestedCount; + stats_.on_config_accepted_.inc(); } - void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DiscoveryResponse&, - const Config::ProcessingDetails& process_details) override { - countState(process_details.state_); - if (process_details.state_ == Config::ProcessingState::FAILED) { - ErrorMessage = process_details.error_detail_.message(); - } + void onConfigRejected(const envoy::service::discovery::v3::DiscoveryResponse&, + const absl::string_view error_detail) override { + stats_.on_config_rejected_.inc(); + ErrorMessage = std::string(error_detail); } - void onConfigReceivedOrFailed(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, - const Config::ProcessingDetails& process_details) override { - countState(process_details.state_); - if (process_details.state_ == Config::ProcessingState::FAILED) { - ErrorMessage = process_details.error_detail_.message(); - } + void onConfigRejected(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, + const absl::string_view error_detail) override { + stats_.on_config_rejected_.inc(); + ErrorMessage = std::string(error_detail); } - static std::atomic ReceiveCount; - static std::atomic IngestedCount; - static std::atomic FailureCount; static std::string ErrorMessage; private: - void countState(const Config::ProcessingState& state) { - switch (state) { - case Config::ProcessingState::RECEIVED: - ++ReceiveCount; - break; - case Config::ProcessingState::FAILED: - ++FailureCount; - break; - }; + TestXdsTrackerStats generateStats(const std::string& prefix, Stats::Scope& scope) { + return {ALL_TEST_XDS_TRACKER_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; } + TestXdsTrackerStats stats_; }; -std::atomic TestXdsConfigTracker::ReceiveCount; -std::atomic TestXdsConfigTracker::IngestedCount; -std::atomic TestXdsConfigTracker::FailureCount; std::string TestXdsConfigTracker::ErrorMessage; -static std::map ResourcesMap; - class TestXdsConfigTrackerFactory : public Config::XdsConfigTrackerFactory { public: ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); } - std::string name() const override { return "envoy.config.xds.test_xds_tracer"; }; + std::string name() const override { return "envoy.config.xds.test_xds_tracker"; }; Config::XdsConfigTrackerPtr createXdsConfigTracker(const ProtobufWkt::Any&, - ProtobufMessage::ValidationVisitor&) override { - return std::make_unique(); + ProtobufMessage::ValidationVisitor&, + Event::Dispatcher&, + Stats::Scope& stats) override { + return std::make_unique(stats); } }; @@ -109,6 +105,12 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe create_xds_upstream_ = true; sotw_or_delta_ = sotwOrDelta(); + config_helper_.addRuntimeOverride("envoy.reloadable_features.unified_mux", + (this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw || + this->sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw) + ? "true" + : "false"); + // Make the default cluster HTTP2. config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { ConfigHelper::setHttp2(*bootstrap.mutable_static_resources()->mutable_clusters(0)); @@ -136,7 +138,8 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe rtds_config->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); auto* api_config_source = rtds_config->mutable_api_config_source(); api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); - api_config_source->set_api_type(this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw + api_config_source->set_api_type((this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw || + this->sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw) ? envoy::config::core::v3::ApiConfigSource::GRPC : envoy::config::core::v3::ApiConfigSource::DELTA_GRPC); api_config_source->set_set_node_on_first_message_only(true); @@ -166,7 +169,7 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe setUpstreamProtocol(Http::CodecType::HTTP2); HttpIntegrationTest::initialize(); registerTestServerPorts({}); - initial_load_success_ = test_server_->counter("runtime.load_success")->value(); + initial_xds_update_ = test_server_->counter("test_xds_tracker.on_config_accepted")->value(); } void acceptXdsConnection() { @@ -176,43 +179,11 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe xds_stream_->startGrpcStream(); } - std::string getRuntimeKey(const std::string& key) { - auto response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/runtime?format=json", "", downstreamProtocol(), version_); - EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().getStatusValue()); - Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(response->body()); - auto entries = loader->getObject("entries"); - if (entries->hasObject(key)) { - return entries->getObject(key)->getString("final_value"); - } - return ""; - } - - void waitforReceiveCount(const int expected_count) { - absl::MutexLock l(&lock_); - const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { - return TestXdsConfigTracker::IngestedCount == expected_count; - }; - timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), - TestUtility::DefaultTimeout); - } - - void waitforFailureCount(const int expected_count) { - absl::MutexLock l(&lock_); - const auto reached_expected_count = [expected_count]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { - return TestXdsConfigTracker::FailureCount >= expected_count; - }; - timeSystem().waitFor(lock_, absl::Condition(&reached_expected_count), - TestUtility::DefaultTimeout); - } - - absl::Mutex lock_; - uint32_t initial_load_success_{0}; + uint32_t initial_xds_update_{0}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTrackerIntegrationTest, - DELTA_SOTW_GRPC_CLIENT_INTEGRATION_PARAMS); + DELTA_SOTW_UNIFIED_GRPC_CLIENT_INTEGRATION_PARAMS); TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { TestXdsConfigTrackerFactory factory; @@ -221,7 +192,6 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { initialize(); acceptXdsConnection(); - int current_ingested_count = TestXdsConfigTracker::IngestedCount; EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, {"some_rtds_layer"}, {}, true)); auto some_rtds_layer = TestUtility::parseYaml(R"EOF( @@ -232,14 +202,9 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { )EOF"); sendDiscoveryResponse( Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); - test_server_->waitForCounterGe("runtime.load_success", initial_load_success_ + 1); - int expected_ingested_count = ++current_ingested_count; - waitforReceiveCount(expected_ingested_count); - - EXPECT_EQ(expected_ingested_count, TestXdsConfigTracker::IngestedCount); - EXPECT_EQ(expected_ingested_count, TestXdsConfigTracker::ReceiveCount); - EXPECT_EQ("bar", getRuntimeKey("foo")); - EXPECT_EQ("meh", getRuntimeKey("baz")); + test_server_->waitForCounterEq("test_xds_tracker.on_config_accepted", initial_xds_update_ + 1); + EXPECT_EQ(test_server_->counter("test_xds_tracker.on_config_accepted")->value(), + initial_xds_update_ + 1); } TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { @@ -282,7 +247,7 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { // Message's TypeUrl != Resource's stream->sendGrpcMessage(discovery_response); - waitforFailureCount(1); + test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); EXPECT_THAT(TestXdsConfigTracker::ErrorMessage, HasSubstr("does not match the message-wide type URL")); } From 67964199a9f226c0589503e079aff7b250ce2c92 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 15 Nov 2022 22:43:55 +0000 Subject: [PATCH 18/31] add comments Signed-off-by: Boteng Yao --- source/common/config/decoded_resource_impl.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index ab7a462dc6391..4a28d300dd8d9 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -98,6 +98,9 @@ class DecodedResourceImpl : public DecodedResource { const std::string version_; // Per resource TTL. const absl::optional ttl_; + + // This is the metadata info under the Resource wrapper. + // It is intented to be consumed in the xds_config_tracker extension. const OptRef metadata_; }; From 7aec7d1559d524fc59bd8ab9771731a28c334a4d Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 15 Nov 2022 23:14:11 +0000 Subject: [PATCH 19/31] fix spell Signed-off-by: Boteng Yao --- source/common/config/decoded_resource_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/decoded_resource_impl.h b/source/common/config/decoded_resource_impl.h index 4a28d300dd8d9..405cf1a1a4fa8 100644 --- a/source/common/config/decoded_resource_impl.h +++ b/source/common/config/decoded_resource_impl.h @@ -100,7 +100,7 @@ class DecodedResourceImpl : public DecodedResource { const absl::optional ttl_; // This is the metadata info under the Resource wrapper. - // It is intented to be consumed in the xds_config_tracker extension. + // It is intended to be consumed in the xds_config_tracker extension. const OptRef metadata_; }; From adaf2446842c6a6f8cc8f90874b4ffeb865a7b14 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 16 Nov 2022 14:50:13 +0000 Subject: [PATCH 20/31] fix ci Signed-off-by: Boteng Yao --- test/common/upstream/eds_speed_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/upstream/eds_speed_test.cc b/test/common/upstream/eds_speed_test.cc index 7951283e1474d..81d6a4a74fffb 100644 --- a/test/common/upstream/eds_speed_test.cc +++ b/test/common/upstream/eds_speed_test.cc @@ -54,7 +54,8 @@ class EdsSpeedTest { std::unique_ptr(async_client_), server_context_.dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.endpoint.v3.EndpointDiscoveryService.StreamEndpoints"), - random_, stats_, {}, local_info_, true, std::move(config_validators_))); + random_, stats_, {}, local_info_, true, std::move(config_validators_), + /*xds_config_tracker=*/Config::XdsConfigTrackerOptRef())); } else { grpc_mux_.reset(new Config::GrpcMuxImpl( local_info_, std::unique_ptr(async_client_), From a144dbd5759302c2c594fc2083fdbe2b12788a7f Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 16 Nov 2022 18:50:43 +0000 Subject: [PATCH 21/31] fix ci Signed-off-by: Boteng Yao --- .../xds_config_tracker_integration_test.cc | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index e6274ab62a6c1..4b368d29e982c 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -40,9 +40,7 @@ struct TestXdsTrackerStats { class TestXdsConfigTracker : public Config::XdsConfigTracker { public: - TestXdsConfigTracker(Stats::Scope& scope) : stats_(generateStats("test_xds_tracker", scope)) { - ErrorMessage = ""; - } + TestXdsConfigTracker(Stats::Scope& scope) : stats_(generateStats("test_xds_tracker", scope)) {} void onConfigAccepted(const absl::string_view, const std::vector&) override { @@ -56,19 +54,15 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { } void onConfigRejected(const envoy::service::discovery::v3::DiscoveryResponse&, - const absl::string_view error_detail) override { + const absl::string_view) override { stats_.on_config_rejected_.inc(); - ErrorMessage = std::string(error_detail); } void onConfigRejected(const envoy::service::discovery::v3::DeltaDiscoveryResponse&, - const absl::string_view error_detail) override { + const absl::string_view) override { stats_.on_config_rejected_.inc(); - ErrorMessage = std::string(error_detail); } - static std::string ErrorMessage; - private: TestXdsTrackerStats generateStats(const std::string& prefix, Stats::Scope& scope) { return {ALL_TEST_XDS_TRACKER_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; @@ -76,8 +70,6 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker { TestXdsTrackerStats stats_; }; -std::string TestXdsConfigTracker::ErrorMessage; - class TestXdsConfigTrackerFactory : public Config::XdsConfigTrackerFactory { public: ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -170,6 +162,7 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe HttpIntegrationTest::initialize(); registerTestServerPorts({}); initial_xds_update_ = test_server_->counter("test_xds_tracker.on_config_accepted")->value(); + initial_xds_reject_ = test_server_->counter("test_xds_tracker.on_config_rejected")->value(); } void acceptXdsConnection() { @@ -180,6 +173,7 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe } uint32_t initial_xds_update_{0}; + uint32_t initial_xds_reject_{0}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTrackerIntegrationTest, @@ -247,9 +241,9 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { // Message's TypeUrl != Resource's stream->sendGrpcMessage(discovery_response); - test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); - EXPECT_THAT(TestXdsConfigTracker::ErrorMessage, - HasSubstr("does not match the message-wide type URL")); + test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", initial_xds_reject_ + 1); + EXPECT_EQ(test_server_->counter("test_xds_tracker.on_config_rejected")->value(), + initial_xds_reject_ + 1); } } // namespace From bf8105c448136401758964de9b70e671d293004f Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Thu, 17 Nov 2022 14:38:57 +0000 Subject: [PATCH 22/31] fix clang Signed-off-by: Boteng Yao --- test/integration/xds_config_tracker_integration_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 4b368d29e982c..64b219888b372 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -17,8 +17,6 @@ #include "absl/synchronization/mutex.h" #include "gtest/gtest.h" -using testing::HasSubstr; - namespace Envoy { namespace { From 91dc0027224fcf3ece6926bcf3f30d5833433dfe Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Fri, 18 Nov 2022 16:26:15 +0000 Subject: [PATCH 23/31] fix intergration test Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 9 ++- .../config/decoded_resource_impl_test.cc | 3 +- .../xds_config_tracker_integration_test.cc | 70 +++++++++++-------- 3 files changed, 49 insertions(+), 33 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index 3ef80a4dc9017..1144100d5d2a2 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -34,6 +34,10 @@ class XdsConfigTracker { * Invoked when SotW xDS configuration updates have been successfully parsed, applied on * the Envoy instance, and are about to be ACK'ed. * + * For SotW, resources are all the resouces except for the heart-beat ones in the original + * message. The call of this method means there is a subscriber for this type_url and the type of + * resource is same as the message's type_url. + * * @param type_url The type url of xDS message. * @param resources A list of decoded resources to add to the current state. */ @@ -41,9 +45,12 @@ class XdsConfigTracker { const std::vector& resources) PURE; /** - * Invoked when Delta xDS configuration updates have been successfully parsed, applied on + * Invoked when Delta xDS configuration updates have been successfully accepted, applied on * the Envoy instance, and are about to be ACK'ed. * + * For Delta, added_resources are all the received added resouces except for the heart-beat ones + * in the original message, and the removed resouces are the same in the xDS message. + * * @param type_url The type url of xDS message. * @param added_resources A list of decoded resources to add to the current state. * @param removed_resources A list of resources to remove from the current state. diff --git a/test/common/config/decoded_resource_impl_test.cc b/test/common/config/decoded_resource_impl_test.cc index 229a0f7960423..e494695d2a075 100644 --- a/test/common/config/decoded_resource_impl_test.cc +++ b/test/common/config/decoded_resource_impl_test.cc @@ -52,11 +52,11 @@ TEST(DecodedResourceImplTest, All) { EXPECT_FALSE(decoded_resource.metadata().has_value()); } + // To verify the metadata is decoded as expected. { envoy::service::discovery::v3::Resource resource_wrapper; resource_wrapper.set_name("real_name"); resource_wrapper.mutable_resource()->MergeFrom(some_opaque_resource); - resource_wrapper.set_version("foo"); auto metadata = resource_wrapper.mutable_metadata(); metadata->mutable_filter_metadata()->insert( {"fake_test_domain", MessageUtil::keyValueStruct("fake_test_key", "fake_test_value")}); @@ -66,7 +66,6 @@ TEST(DecodedResourceImplTest, All) { EXPECT_CALL(resource_decoder, resourceName(ProtoEq(ProtobufWkt::Empty()))).Times(0); DecodedResourceImpl decoded_resource(resource_decoder, resource_wrapper); EXPECT_EQ("real_name", decoded_resource.name()); - EXPECT_EQ("foo", decoded_resource.version()); EXPECT_THAT(decoded_resource.resource(), ProtoEq(ProtobufWkt::Empty())); EXPECT_TRUE(decoded_resource.hasResource()); EXPECT_TRUE(decoded_resource.metadata().has_value()); diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 64b219888b372..07fb9200540d8 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -36,6 +36,10 @@ struct TestXdsTrackerStats { ALL_TEST_XDS_TRACKER_STATS(GENERATE_COUNTER_STRUCT) }; +/** + * A test implementation of the XdsConfigTracker extension. + * It just increases the test counter when a related method is called. + */ class TestXdsConfigTracker : public Config::XdsConfigTracker { public: TestXdsConfigTracker(Stats::Scope& scope) : stats_(generateStats("test_xds_tracker", scope)) {} @@ -96,7 +100,7 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe sotw_or_delta_ = sotwOrDelta(); config_helper_.addRuntimeOverride("envoy.reloadable_features.unified_mux", - (this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw || + (this->sotwOrDelta() == Grpc::SotwOrDelta::UnifiedDelta || this->sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw) ? "true" : "false"); @@ -159,8 +163,6 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe setUpstreamProtocol(Http::CodecType::HTTP2); HttpIntegrationTest::initialize(); registerTestServerPorts({}); - initial_xds_update_ = test_server_->counter("test_xds_tracker.on_config_accepted")->value(); - initial_xds_reject_ = test_server_->counter("test_xds_tracker.on_config_rejected")->value(); } void acceptXdsConnection() { @@ -169,9 +171,6 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe RELEASE_ASSERT(result, result.message()); xds_stream_->startGrpcStream(); } - - uint32_t initial_xds_update_{0}; - uint32_t initial_xds_reject_{0}; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTrackerIntegrationTest, @@ -186,7 +185,7 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, {"some_rtds_layer"}, {}, true)); - auto some_rtds_layer = TestUtility::parseYaml(R"EOF( + const auto some_rtds_layer = TestUtility::parseYaml(R"EOF( name: some_rtds_layer layer: foo: bar @@ -194,9 +193,8 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { )EOF"); sendDiscoveryResponse( Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); - test_server_->waitForCounterEq("test_xds_tracker.on_config_accepted", initial_xds_update_ + 1); - EXPECT_EQ(test_server_->counter("test_xds_tracker.on_config_accepted")->value(), - initial_xds_update_ + 1); + test_server_->waitForCounterEq("test_xds_tracker.on_config_accepted", 1); + EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_accepted")->value()); } TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { @@ -208,14 +206,9 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, {"some_rtds_layer"}, {}, true)); - auto some_rtds_layer = TestUtility::parseYaml(R"EOF( - name: different_rtds_layer - layer: - foo: bar - baz: meh - )EOF"); - auto route_config = TestUtility::parseYaml(R"EOF( + const auto route_config = + TestUtility::parseYaml(R"EOF( name: my_route vhds: config_source: @@ -229,19 +222,36 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { )EOF"); FakeStream* stream = xds_stream_.get(); - envoy::service::discovery::v3::DiscoveryResponse discovery_response; - discovery_response.set_version_info("1"); - discovery_response.set_type_url(Config::TypeUrl::get().Runtime); - discovery_response.add_resources()->PackFrom(route_config); - - static int next_nonce_counter = 0; - discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); - - // Message's TypeUrl != Resource's - stream->sendGrpcMessage(discovery_response); - test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", initial_xds_reject_ + 1); - EXPECT_EQ(test_server_->counter("test_xds_tracker.on_config_rejected")->value(), - initial_xds_reject_ + 1); + if (sotw_or_delta_ == Grpc::SotwOrDelta::Sotw || + sotw_or_delta_ == Grpc::SotwOrDelta::UnifiedSotw) { + envoy::service::discovery::v3::DiscoveryResponse discovery_response; + discovery_response.set_version_info("1"); + discovery_response.set_type_url(Config::TypeUrl::get().Runtime); + discovery_response.add_resources()->PackFrom(route_config); + + static int next_nonce_counter = 0; + discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); + + // Message's TypeUrl != Resource's + stream->sendGrpcMessage(discovery_response); + } else { + std::vector resources; + envoy::service::discovery::v3::Resource resource; + resource.mutable_resource()->PackFrom(route_config); + resource.set_name("my_route"); + resource.set_version("1"); + resources.emplace_back(resource); + const auto delta_discovery_response = createExplicitResourcesDeltaDiscoveryResponse( + Config::TypeUrl::get().Runtime, resources, {}); + + // Message's TypeUrl != Resource's + stream->sendGrpcMessage(delta_discovery_response); + } + + test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); + ENVOY_LOG_MISC(warn, "Boteng test {}", + test_server_->counter("test_xds_tracker.on_config_rejected")->value()); + EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); } } // namespace From b8977216095d58e5e74a3672cc661e0d17ad26ff Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Fri, 18 Nov 2022 16:38:00 +0000 Subject: [PATCH 24/31] clean up Signed-off-by: Boteng Yao --- test/integration/xds_config_tracker_integration_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 07fb9200540d8..4d1f537340f70 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -249,8 +249,6 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { } test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); - ENVOY_LOG_MISC(warn, "Boteng test {}", - test_server_->counter("test_xds_tracker.on_config_rejected")->value()); EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); } From b805acb28866389454fdee702c3e29d6ecdbeefa Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Wed, 23 Nov 2022 14:01:13 +0000 Subject: [PATCH 25/31] fix comments Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index 1144100d5d2a2..d936541aff431 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -18,8 +18,8 @@ namespace Config { /** * An interface for hooking into xDS update events to provide the ablility to use some external - * processor in xDS update. This traker provides the process point when the discovery response - * is received, when the resoures are successfully processed and applied, and when there is any + * processor in xDS update. This tracker provides the process point when the discovery response + * is received, when the resources are successfully processed and applied, and when there is any * failure. * * Instance of this interface get invoked on the main Envoy thread. Thus, it is important @@ -34,9 +34,9 @@ class XdsConfigTracker { * Invoked when SotW xDS configuration updates have been successfully parsed, applied on * the Envoy instance, and are about to be ACK'ed. * - * For SotW, resources are all the resouces except for the heart-beat ones in the original - * message. The call of this method means there is a subscriber for this type_url and the type of - * resource is same as the message's type_url. + * For SotW, the passed resources contain all the received resources except for the heart-beat + * ones in the original message. The call of this method means there is a subscriber for this + * type_url and the type of resource is same as the message's type_url. * * @param type_url The type url of xDS message. * @param resources A list of decoded resources to add to the current state. @@ -48,8 +48,8 @@ class XdsConfigTracker { * Invoked when Delta xDS configuration updates have been successfully accepted, applied on * the Envoy instance, and are about to be ACK'ed. * - * For Delta, added_resources are all the received added resouces except for the heart-beat ones - * in the original message, and the removed resouces are the same in the xDS message. + * For Delta, added_resources contains all the received added resources except for the heart-beat + * ones in the original message, and the removed resouces are the same in the xDS message. * * @param type_url The type url of xDS message. * @param added_resources A list of decoded resources to add to the current state. From 9b23b37ddcd90a07988987a5042dfd0d54f1a684 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Mon, 5 Dec 2022 16:10:31 +0000 Subject: [PATCH 26/31] simplify test Signed-off-by: Boteng Yao --- .../xds_config_tracker_integration_test.cc | 29 ++----------------- 1 file changed, 3 insertions(+), 26 deletions(-) diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 4d1f537340f70..5f41032fbcacf 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -221,32 +221,9 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { cluster_name: xds_cluster )EOF"); - FakeStream* stream = xds_stream_.get(); - if (sotw_or_delta_ == Grpc::SotwOrDelta::Sotw || - sotw_or_delta_ == Grpc::SotwOrDelta::UnifiedSotw) { - envoy::service::discovery::v3::DiscoveryResponse discovery_response; - discovery_response.set_version_info("1"); - discovery_response.set_type_url(Config::TypeUrl::get().Runtime); - discovery_response.add_resources()->PackFrom(route_config); - - static int next_nonce_counter = 0; - discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); - - // Message's TypeUrl != Resource's - stream->sendGrpcMessage(discovery_response); - } else { - std::vector resources; - envoy::service::discovery::v3::Resource resource; - resource.mutable_resource()->PackFrom(route_config); - resource.set_name("my_route"); - resource.set_version("1"); - resources.emplace_back(resource); - const auto delta_discovery_response = createExplicitResourcesDeltaDiscoveryResponse( - Config::TypeUrl::get().Runtime, resources, {}); - - // Message's TypeUrl != Resource's - stream->sendGrpcMessage(delta_discovery_response); - } + // Message's TypeUrl != Resource's + sendDiscoveryResponse( + Config::TypeUrl::get().Runtime, {route_config}, {route_config}, {}, "1"); test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); From 27d1072a3c814580b2fe685a620234d22f9120a2 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 6 Dec 2022 03:54:09 +0000 Subject: [PATCH 27/31] add more integration tests Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 4 + test/config/utility.cc | 14 +- test/config/utility.h | 3 + .../xds_config_tracker_integration_test.cc | 121 +++++++++--------- 4 files changed, 79 insertions(+), 63 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index d936541aff431..14c8c2440a840 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -38,6 +38,8 @@ class XdsConfigTracker { * ones in the original message. The call of this method means there is a subscriber for this * type_url and the type of resource is same as the message's type_url. * + * Note: this method is called when *all* the resouces in a response are accepted. + * * @param type_url The type url of xDS message. * @param resources A list of decoded resources to add to the current state. */ @@ -51,6 +53,8 @@ class XdsConfigTracker { * For Delta, added_resources contains all the received added resources except for the heart-beat * ones in the original message, and the removed resouces are the same in the xDS message. * + * Note: this method is called when *all* the resouces in a response are accepted. + * * @param type_url The type url of xDS message. * @param added_resources A list of decoded resources to add to the current state. * @param removed_resources A list of resources to remove from the current state. diff --git a/test/config/utility.cc b/test/config/utility.cc index 13f4add5e4104..c884060403d52 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -348,8 +348,7 @@ name: squash )EOF"; } -// TODO(#6327) cleaner approach to testing with static config. -std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_type) { +std::string ConfigHelper::clustersNoListenerBootstrap(const std::string& api_type) { return fmt::format( R"EOF( admin: @@ -389,6 +388,14 @@ std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_typ socket_address: address: 127.0.0.1 port_value: 0 +)EOF", + Platform::null_device_path, api_type); +} + +// TODO(#6327) cleaner approach to testing with static config. +std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_type) { + return absl::StrCat(clustersNoListenerBootstrap(api_type), + R"EOF( listeners: name: http address: @@ -421,8 +428,7 @@ std::string ConfigHelper::discoveredClustersBootstrap(const std::string& api_typ match: prefix: "/cluster2" domains: "*" -)EOF", - Platform::null_device_path, api_type); +)EOF"); } // TODO(#6327) cleaner approach to testing with static config. diff --git a/test/config/utility.h b/test/config/utility.h index 615d3326189a6..0f12fc3fb3640 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -196,6 +196,9 @@ class ConfigHelper { // Configuration for L7 proxying, with clusters cluster_1 and cluster_2 meant to be added via CDS. // api_type should be REST, GRPC, or DELTA_GRPC. static std::string discoveredClustersBootstrap(const std::string& api_type); + // Configuration for L7 proxying, with clusters cluster_1 and cluster_2 meant to be added via CDS. + // but there are no listeners. + static std::string clustersNoListenerBootstrap(const std::string& api_type); static std::string adsBootstrap(const std::string& api_type); // Builds a standard Cluster config fragment, with a single endpoint (at address:port). static envoy::config::cluster::v3::Cluster diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index 5f41032fbcacf..d51546ec598ab 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -20,7 +20,10 @@ namespace Envoy { namespace { -constexpr char XDS_CLUSTER_NAME[] = "xds_cluster"; +const char ClusterName1[] = "cluster_1"; +const char ClusterName2[] = "cluster_2"; +const int UpstreamIndex1 = 1; +const int UpstreamIndex2 = 2; /** * All stats for this xds tracker. @see stats_macros.h @@ -93,10 +96,13 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe public: XdsConfigTrackerIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP2, ipVersion(), - ConfigHelper::baseConfigNoListeners()) { + ConfigHelper::clustersNoListenerBootstrap( + sotwOrDelta() == Grpc::SotwOrDelta::Sotw || + sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw + ? "GRPC" + : "DELTA_GRPC")) { use_lds_ = false; - create_xds_upstream_ = true; sotw_or_delta_ = sotwOrDelta(); config_helper_.addRuntimeOverride("envoy.reloadable_features.unified_mux", @@ -105,42 +111,6 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe ? "true" : "false"); - // Make the default cluster HTTP2. - config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - ConfigHelper::setHttp2(*bootstrap.mutable_static_resources()->mutable_clusters(0)); - }); - - // Build and add the xDS cluster config. - config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* xds_cluster = bootstrap.mutable_static_resources()->add_clusters(); - xds_cluster->MergeFrom(ConfigHelper::buildStaticCluster( - std::string(XDS_CLUSTER_NAME), - /*port=*/0, ipVersion() == Network::Address::IpVersion::v4 ? "127.0.0.1" : "::1")); - ConfigHelper::setHttp2(*xds_cluster); - }); - - // Add static runtime values. - config_helper_.addRuntimeOverride("whatevs", "yar"); - - // Set up the RTDS runtime layer. - config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - auto* layer = bootstrap.mutable_layered_runtime()->add_layers(); - layer->set_name("some_rtds_layer"); - auto* rtds_layer = layer->mutable_rtds_layer(); - rtds_layer->set_name("some_rtds_layer"); - auto* rtds_config = rtds_layer->mutable_rtds_config(); - rtds_config->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); - auto* api_config_source = rtds_config->mutable_api_config_source(); - api_config_source->set_transport_api_version(envoy::config::core::v3::ApiVersion::V3); - api_config_source->set_api_type((this->sotwOrDelta() == Grpc::SotwOrDelta::Sotw || - this->sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw) - ? envoy::config::core::v3::ApiConfigSource::GRPC - : envoy::config::core::v3::ApiConfigSource::DELTA_GRPC); - api_config_source->set_set_node_on_first_message_only(true); - api_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name( - XDS_CLUSTER_NAME); - }); - // Add test xDS config tracer. config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { auto* tracer_extension = bootstrap.mutable_xds_config_tracker_extension(); @@ -162,15 +132,32 @@ class XdsConfigTrackerIntegrationTest : public Grpc::DeltaSotwIntegrationParamTe setUpstreamCount(1); setUpstreamProtocol(Http::CodecType::HTTP2); HttpIntegrationTest::initialize(); + + // Create the regular (i.e. not an xDS server) upstreams. + addFakeUpstream(Http::CodecType::HTTP2); + addFakeUpstream(Http::CodecType::HTTP2); + cluster1_ = ConfigHelper::buildStaticCluster( + ClusterName1, fake_upstreams_[UpstreamIndex1]->localAddress()->ip()->port(), + Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN"); + cluster2_ = ConfigHelper::buildStaticCluster( + ClusterName2, fake_upstreams_[UpstreamIndex2]->localAddress()->ip()->port(), + Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN"); + + acceptXdsConnection(); registerTestServerPorts({}); } void acceptXdsConnection() { - createXdsConnection(); - AssertionResult result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); + AssertionResult result = // xds_connection_ is filled with the new FakeHttpConnection. + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, xds_connection_); + RELEASE_ASSERT(result, result.message()); + result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); RELEASE_ASSERT(result, result.message()); xds_stream_->startGrpcStream(); } + + envoy::config::cluster::v3::Cluster cluster1_; + envoy::config::cluster::v3::Cluster cluster2_; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, XdsConfigTrackerIntegrationTest, @@ -181,18 +168,15 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerSuccessCount) { Registry::InjectFactory registered(factory); initialize(); - acceptXdsConnection(); - - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, - {"some_rtds_layer"}, {}, true)); - const auto some_rtds_layer = TestUtility::parseYaml(R"EOF( - name: some_rtds_layer - layer: - foo: bar - baz: meh - )EOF"); - sendDiscoveryResponse( - Config::TypeUrl::get().Runtime, {some_rtds_layer}, {some_rtds_layer}, {}, "1"); + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); + + sendDiscoveryResponse( + Config::TypeUrl::get().Cluster, {cluster1_, cluster2_}, {cluster1_, cluster2_}, {}, "1"); + + // 3 because the statically specified CDS server itself counts as a cluster. + test_server_->waitForGaugeGe("cluster_manager.active_clusters", 3); + + // onConfigAccepted is called when all the resources are accepted. test_server_->waitForCounterEq("test_xds_tracker.on_config_accepted", 1); EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_accepted")->value()); } @@ -202,10 +186,7 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { Registry::InjectFactory registered(factory); initialize(); - acceptXdsConnection(); - - EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Runtime, "", {"some_rtds_layer"}, - {"some_rtds_layer"}, {}, true)); + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); const auto route_config = TestUtility::parseYaml(R"EOF( @@ -221,12 +202,34 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerFailureCount) { cluster_name: xds_cluster )EOF"); - // Message's TypeUrl != Resource's sendDiscoveryResponse( - Config::TypeUrl::get().Runtime, {route_config}, {route_config}, {}, "1"); + Config::TypeUrl::get().Cluster, {route_config}, {route_config}, {}, "3"); + + // Resources are rejected because Message's TypeUrl != Resource's + test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); + EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); +} +TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerPartialUpdate) { + TestXdsConfigTrackerFactory factory; + Registry::InjectFactory registered(factory); + + initialize(); + // The first of duplicates has already been successfully applied, and a duplicate exception should + // threw. Only the first cluster is added. + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); + sendDiscoveryResponse( + Config::TypeUrl::get().Cluster, {cluster1_, cluster1_, cluster2_}, + {cluster1_, cluster1_, cluster2_}, {}, "4"); + + test_server_->waitForCounterGe("cluster_manager.cluster_added", 1); test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); + + // onConfigRejected is called even if a subset of the resources are added. EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); + + // onConfigAccepted is called only when all the resources in a response are successfully ingested. + EXPECT_EQ(0, test_server_->counter("test_xds_tracker.on_config_accepted")->value()); } } // namespace From cf96f4b36de851528f84df6f6da620ccc3f1481d Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 6 Dec 2022 03:56:35 +0000 Subject: [PATCH 28/31] fix typo Signed-off-by: Boteng Yao --- test/integration/xds_config_tracker_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index d51546ec598ab..c2fc3a97dc06c 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -216,7 +216,7 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerPartialUpdate) { initialize(); // The first of duplicates has already been successfully applied, and a duplicate exception should - // threw. Only the first cluster is added. + // be threw. Only the first cluster is added. EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); sendDiscoveryResponse( Config::TypeUrl::get().Cluster, {cluster1_, cluster1_, cluster2_}, From cb9dddb72bb79df8323106a6bded477d026725db Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Fri, 9 Dec 2022 14:55:53 +0000 Subject: [PATCH 29/31] modify comments and make test specific Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 4 ++-- .../xds_config_tracker_integration_test.cc | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index 14c8c2440a840..fdf1a02988891 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -38,7 +38,7 @@ class XdsConfigTracker { * ones in the original message. The call of this method means there is a subscriber for this * type_url and the type of resource is same as the message's type_url. * - * Note: this method is called when *all* the resouces in a response are accepted. + * Note: this method is called when *all* the resources in a response are accepted. * * @param type_url The type url of xDS message. * @param resources A list of decoded resources to add to the current state. @@ -51,7 +51,7 @@ class XdsConfigTracker { * the Envoy instance, and are about to be ACK'ed. * * For Delta, added_resources contains all the received added resources except for the heart-beat - * ones in the original message, and the removed resouces are the same in the xDS message. + * ones in the original message, and the removed resources are the same in the xDS message. * * Note: this method is called when *all* the resouces in a response are accepted. * diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc index c2fc3a97dc06c..96f58bdaf79cb 100644 --- a/test/integration/xds_config_tracker_integration_test.cc +++ b/test/integration/xds_config_tracker_integration_test.cc @@ -215,17 +215,24 @@ TEST_P(XdsConfigTrackerIntegrationTest, XdsConfigTrackerPartialUpdate) { Registry::InjectFactory registered(factory); initialize(); - // The first of duplicates has already been successfully applied, and a duplicate exception should - // be threw. Only the first cluster is added. + // The first of duplicates has already been successfully applied, + // and a duplicate exception should be threw. EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); sendDiscoveryResponse( Config::TypeUrl::get().Cluster, {cluster1_, cluster1_, cluster2_}, - {cluster1_, cluster1_, cluster2_}, {}, "4"); + {cluster1_, cluster1_, cluster2_}, {}, "5"); + + // For Delta, the response will be rejected when checking the message due to the duplication. + // For SotW, both clusters are accepted, but the internal exception is not empty. + if (sotw_or_delta_ == Grpc::SotwOrDelta::Delta || + sotw_or_delta_ == Grpc::SotwOrDelta::UnifiedDelta) { + test_server_->waitForCounterGe("cluster_manager.cluster_added", 1); + } else { + test_server_->waitForCounterGe("cluster_manager.cluster_added", 3); + } - test_server_->waitForCounterGe("cluster_manager.cluster_added", 1); + // onConfigRejected is called if there is any exception even some resources are accepted. test_server_->waitForCounterEq("test_xds_tracker.on_config_rejected", 1); - - // onConfigRejected is called even if a subset of the resources are added. EXPECT_EQ(1, test_server_->counter("test_xds_tracker.on_config_rejected")->value()); // onConfigAccepted is called only when all the resources in a response are successfully ingested. From 348aa7f717ca49cd9f5b813b162858c3f3b33c9d Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Sat, 10 Dec 2022 00:15:00 +0000 Subject: [PATCH 30/31] fix typo Signed-off-by: Boteng Yao --- envoy/config/xds_config_tracker.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h index fdf1a02988891..202ae4f49506a 100644 --- a/envoy/config/xds_config_tracker.h +++ b/envoy/config/xds_config_tracker.h @@ -53,7 +53,7 @@ class XdsConfigTracker { * For Delta, added_resources contains all the received added resources except for the heart-beat * ones in the original message, and the removed resources are the same in the xDS message. * - * Note: this method is called when *all* the resouces in a response are accepted. + * Note: this method is called when *all* the resources in a response are accepted. * * @param type_url The type url of xDS message. * @param added_resources A list of decoded resources to add to the current state. From 7a90bccae8b7d0f72588f6ea308c8019e40074dc Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 13 Dec 2022 00:07:45 +0000 Subject: [PATCH 31/31] modify api comments and add release notes Signed-off-by: Boteng Yao --- api/envoy/config/bootstrap/v3/bootstrap.proto | 11 ++++++++--- changelogs/current.yaml | 4 ++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index 2676b9c0757be..eaf0911c64b66 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -345,9 +345,14 @@ message Bootstrap { // Optional XdsConfigTracker configuration, which allows tracking xDS responses in external components, // e.g., external tracer or monitor. It provides the process point when receive, ingest, or fail to - // process xDS resources and messages. - // If a value is not specified, no XdsConfigTracker will be used. - // [#not-implemented-hide:] + // process xDS resources and messages. If a value is not specified, no XdsConfigTracker will be used. + // + // .. note:: + // + // There are no in-repo extensions currently, and the :repo:`XdsConfigTracker ` + // interface should be implemented before using. + // See :repo:`xds_config_tracker_integration_test ` + // for an example usage of the interface. core.v3.TypedExtensionConfig xds_config_tracker_extension = 36; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ba6cfd6b429e8..bb435b32e2817 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -237,5 +237,9 @@ new_features: :ref:`source_address `. This allows setting :ref:`socket options ` when using the default unspecified bind address is desired. +- area: xds + change: | + added an api configuration :ref:`xds_config_tracker_extension ` in the bootstrap + to allow tracking xDS responses in external components, and provided the extension interface. deprecated: