Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/envoy/server/filter_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ class CommonFactoryContext {
*/
virtual const LocalInfo::LocalInfo& localInfo() const PURE;

/**
* @return ProtobufMessage::ValidationContext& validation visitor for xDS&static configuration
* messages.
*/
virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE;

/**
* @return RandomGenerator& the random generator for the server.
*/
Expand Down
16 changes: 8 additions & 8 deletions source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ StaticRouteConfigProviderImpl::StaticRouteConfigProviderImpl(
Server::Configuration::FactoryContext& factory_context,
RouteConfigProviderManagerImpl& route_config_provider_manager)
: config_(new ConfigImpl(config, factory_context.getServerFactoryContext(),
factory_context.messageValidationVisitor(), true)),
factory_context.messageValidationContext().staticValidationVisitor(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. You can have a static inline route in a dynamic Listener, for example, and we should be applying dynamic validation logic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert it.
In the long run, I will probably make Listener validator passed down to RDS and its children object. (so that RDS doesn't depend on Listener).

true)),

route_config_proto_{config}, last_updated_(factory_context.timeSource().systemTime()),
route_config_provider_manager_(route_config_provider_manager) {
Expand All @@ -63,11 +64,11 @@ StaticRouteConfigProviderImpl::~StaticRouteConfigProviderImpl() {
RdsRouteConfigSubscription::RdsRouteConfigSubscription(
const envoy::extensions::filters::network::http_connection_manager::v3alpha::Rds& rds,
const uint64_t manager_identifier, Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator, Init::Manager& init_manager,
const std::string& stat_prefix,
Init::Manager& init_manager, const std::string& stat_prefix,
Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager)
: route_config_name_(rds.route_config_name()), factory_context_(factory_context),
validator_(validator), init_manager_(init_manager),
validator_(factory_context.messageValidationContext().dynamicValidationVisitor()),
init_manager_(init_manager),
init_target_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_),
[this]() { subscription_->start({route_config_name_}); }),
scope_(factory_context.scope().createScope(stat_prefix + "rds." + route_config_name_ + ".")),
Expand All @@ -80,7 +81,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription(
rds.config_source(), loadTypeUrl(rds.config_source().resource_api_version()), *scope_,
*this);
config_update_info_ =
std::make_unique<RouteConfigUpdateReceiverImpl>(factory_context.timeSource(), validator);
std::make_unique<RouteConfigUpdateReceiverImpl>(factory_context.timeSource(), validator_);
}

RdsRouteConfigSubscription::~RdsRouteConfigSubscription() {
Expand Down Expand Up @@ -227,7 +228,7 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl(
: subscription_(std::move(subscription)),
config_update_info_(subscription_->routeConfigUpdate()),
factory_context_(factory_context.getServerFactoryContext()),
validator_(factory_context.messageValidationVisitor()),
validator_(factory_context.messageValidationContext().dynamicValidationVisitor()),
tls_(factory_context.threadLocal().allocateSlot()) {
ConfigConstSharedPtr initial_config;
if (config_update_info_->configInfo().has_value()) {
Expand Down Expand Up @@ -293,8 +294,7 @@ Router::RouteConfigProviderSharedPtr RouteConfigProviderManagerImpl::createRdsRo
// of simplicity.
RdsRouteConfigSubscriptionSharedPtr subscription(new RdsRouteConfigSubscription(
rds, manager_identifier, factory_context.getServerFactoryContext(),
factory_context.messageValidationVisitor(), factory_context.initManager(), stat_prefix,
*this));
factory_context.initManager(), stat_prefix, *this));
init_manager.add(subscription->init_target_);
std::shared_ptr<RdsRouteConfigProviderImpl> new_provider{
new RdsRouteConfigProviderImpl(std::move(subscription), factory_context)};
Expand Down
3 changes: 1 addition & 2 deletions source/common/router/rds_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
RdsRouteConfigSubscription(
const envoy::extensions::filters::network::http_connection_manager::v3alpha::Rds& rds,
const uint64_t manager_identifier,
Server::Configuration::ServerFactoryContext& factory_context,
ProtobufMessage::ValidationVisitor& validator, Init::Manager& init_manager,
Server::Configuration::ServerFactoryContext& factory_context, Init::Manager& init_manager,
const std::string& stat_prefix,
RouteConfigProviderManagerImpl& route_config_provider_manager);

Expand Down
11 changes: 6 additions & 5 deletions source/common/router/scoped_rds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription(
scope_(factory_context.scope().createScope(stat_prefix + "scoped_rds." + name + ".")),
stats_({ALL_SCOPED_RDS_STATS(POOL_COUNTER(*scope_))}),
rds_config_source_(std::move(rds_config_source)),
validation_visitor_(factory_context.messageValidationVisitor()), stat_prefix_(stat_prefix),
route_config_provider_manager_(route_config_provider_manager) {
validation_visitor_(factory_context.messageValidationContext().dynamicValidationVisitor()),
stat_prefix_(stat_prefix), route_config_provider_manager_(route_config_provider_manager) {
subscription_ =
factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource(
scoped_rds.scoped_rds_config_source(),
Expand Down Expand Up @@ -290,9 +290,10 @@ void ScopedRdsConfigSubscription::onRdsConfigUpdate(const std::string& scope_nam
fmt::format("trying to update route config for non-existing scope {}", scope_name));
auto new_scoped_route_info = std::make_shared<ScopedRouteInfo>(
envoy::config::route::v3alpha::ScopedRouteConfiguration(iter->second->configProto()),
std::make_shared<ConfigImpl>(rds_subscription.routeConfigUpdate()->routeConfiguration(),
factory_context_.getServerFactoryContext(),
factory_context_.messageValidationVisitor(), false));
std::make_shared<ConfigImpl>(
rds_subscription.routeConfigUpdate()->routeConfiguration(),
factory_context_.getServerFactoryContext(),
factory_context_.messageValidationContext().dynamicValidationVisitor(), false));
applyConfigUpdate([new_scoped_route_info](ConfigProvider::ConfigConstSharedPtr config)
-> ConfigProvider::ConfigConstSharedPtr {
auto* thread_local_scoped_config =
Expand Down
4 changes: 4 additions & 0 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,10 @@ class FactoryContextImpl : public Server::Configuration::CommonFactoryContext {
ThreadLocal::SlotAllocator& threadLocal() override { return tls_; }
Server::Admin& admin() override { return admin_; }
TimeSource& timeSource() override { return api().timeSource(); }
ProtobufMessage::ValidationContext& messageValidationContext() override {
// Not used.
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
}
Api::Api& api() override { return api_; }

private:
Expand Down
3 changes: 3 additions & 0 deletions source/server/filter_chain_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ envoy::config::core::v3alpha::TrafficDirection FilterChainFactoryContextImpl::di
return parent_context_.direction();
}

ProtobufMessage::ValidationContext& FilterChainFactoryContextImpl::messageValidationContext() {
return parent_context_.messageValidationContext();
}
ProtobufMessage::ValidationVisitor& FilterChainFactoryContextImpl::messageValidationVisitor() {
return parent_context_.messageValidationVisitor();
}
Expand Down
1 change: 1 addition & 0 deletions source/server/filter_chain_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class FilterChainFactoryContextImpl : public Configuration::FilterChainFactoryCo
envoy::config::core::v3alpha::TrafficDirection direction() const override;
TimeSource& timeSource() override;
ProtobufMessage::ValidationVisitor& messageValidationVisitor() override;
ProtobufMessage::ValidationContext& messageValidationContext() override;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both validation visitor and context as peers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both validation visitor and context as peers?

removing of validator() will be in another PR, lots of files will be modified as I plan to pass the signal "added_via_api" (or a vlidator parameter) down from Listener to everythin.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to figure out how this works.. do you have a draft PR to see the end results? As mentioned off-line, I'd like to:

  1. Thread validators around, vs. context+flag unless the flag is needed for some other purpose.
  2. Ensure that it's easy for consumers to automatically "do the right thing", without having to think too hard, otherwise we get folks picking random validators via copy+paste.

^^ guided the existing design. We need a bunch of comments to explain this interim solution in any case, as it's pretty confusing why both are there right now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9779 I have it now.
That PR doesn't include the removal of FactoryContext::messageValidationVisitor() tho.
So if we want to remove that later, we will need to pipe a picked validator from listenerImpl down to HCM::Config.

Api::Api& api() override;
ServerLifecycleNotifier& lifecycleNotifier() override;
OptProcessContextRef processContext() override;
Expand Down
13 changes: 9 additions & 4 deletions source/server/listener_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,7 @@ Network::SocketSharedPtr ListenSocketFactoryImpl::getListenSocket() {
ListenerImpl::ListenerImpl(const envoy::config::listener::v3alpha::Listener& config,
const std::string& version_info, ListenerManagerImpl& parent,
const std::string& name, bool added_via_api, bool workers_started,
uint64_t hash, ProtobufMessage::ValidationVisitor& validation_visitor,
uint32_t concurrency)
uint64_t hash, uint32_t concurrency)
: parent_(parent), address_(Network::Address::resolveProtoAddress(config.address())),
filter_chain_manager_(address_, *this),
global_scope_(parent_.server_.stats().createScope("")),
Expand All @@ -139,7 +138,10 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3alpha::Listener& con
per_connection_buffer_limit_bytes_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)),
listener_tag_(parent_.factory_.nextListenerTag()), name_(name), added_via_api_(added_via_api),
workers_started_(workers_started), hash_(hash), validation_visitor_(validation_visitor),
workers_started_(workers_started), hash_(hash),
validation_visitor_(
added_via_api_ ? parent_.server_.messageValidationContext().dynamicValidationVisitor()
: parent_.server_.messageValidationContext().staticValidationVisitor()),
dynamic_init_manager_(fmt::format("Listener {}", name)),
init_watcher_(std::make_unique<Init::WatcherImpl>(
"ListenerImpl", [this] { parent_.onListenerWarmed(*this); })),
Expand Down Expand Up @@ -226,7 +228,7 @@ ListenerImpl::ListenerImpl(const envoy::config::listener::v3alpha::Listener& con
parent_.server_.admin(), parent_.server_.sslContextManager(), *listener_scope_,
parent_.server_.clusterManager(), parent_.server_.localInfo(), parent_.server_.dispatcher(),
parent_.server_.random(), parent_.server_.stats(), parent_.server_.singletonManager(),
parent_.server_.threadLocal(), validation_visitor, parent_.server_.api());
parent_.server_.threadLocal(), validation_visitor_, parent_.server_.api());
transport_factory_context.setInitManager(initManager());
// The init manager is a little messy. Will refactor when filter chain manager could accept
// network filter chain update.
Expand Down Expand Up @@ -342,6 +344,9 @@ envoy::config::core::v3alpha::TrafficDirection ListenerImpl::direction() const {
TimeSource& ListenerImpl::timeSource() { return api().timeSource(); }

const Network::ListenerConfig& ListenerImpl::listenerConfig() const { return *this; }
ProtobufMessage::ValidationContext& ListenerImpl::messageValidationContext() {
return getServerFactoryContext().messageValidationContext();
}
ProtobufMessage::ValidationVisitor& ListenerImpl::messageValidationVisitor() {
return validation_visitor_;
}
Expand Down
3 changes: 2 additions & 1 deletion source/server/listener_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class ListenerImpl : public Network::ListenerConfig,
ListenerImpl(const envoy::config::listener::v3alpha::Listener& config,
const std::string& version_info, ListenerManagerImpl& parent,
const std::string& name, bool added_via_api, bool workers_started, uint64_t hash,
ProtobufMessage::ValidationVisitor& validation_visitor, uint32_t concurrency);
uint32_t concurrency);
~ListenerImpl() override;

/**
Expand Down Expand Up @@ -173,6 +173,7 @@ class ListenerImpl : public Network::ListenerConfig,
envoy::config::core::v3alpha::TrafficDirection direction() const override;
TimeSource& timeSource() override;
const Network::ListenerConfig& listenerConfig() const override;
ProtobufMessage::ValidationContext& messageValidationContext() override;
ProtobufMessage::ValidationVisitor& messageValidationVisitor() override;
Api::Api& api() override;
ServerLifecycleNotifier& lifecycleNotifier() override;
Expand Down
8 changes: 3 additions & 5 deletions source/server/listener_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,9 @@ bool ListenerManagerImpl::addOrUpdateListenerInternal(
return false;
}

ListenerImplPtr new_listener(
new ListenerImpl(config, version_info, *this, name, added_via_api, workers_started_, hash,
added_via_api ? server_.messageValidationContext().dynamicValidationVisitor()
: server_.messageValidationContext().staticValidationVisitor(),
server_.options().concurrency()));
ListenerImplPtr new_listener(new ListenerImpl(config, version_info, *this, name, added_via_api,
workers_started_, hash,
server_.options().concurrency()));
ListenerImpl& new_listener_ref = *new_listener;

// We mandate that a listener with the same name must have the same configured address. This
Expand Down
3 changes: 3 additions & 0 deletions source/server/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ class ServerFactoryContextImpl : public Configuration::ServerFactoryContext {
Upstream::ClusterManager& clusterManager() override { return server_.clusterManager(); }
Event::Dispatcher& dispatcher() override { return server_.dispatcher(); }
const LocalInfo::LocalInfo& localInfo() const override { return server_.localInfo(); }
ProtobufMessage::ValidationContext& messageValidationContext() override {
return server_.messageValidationContext();
}
Envoy::Runtime::RandomGenerator& random() override { return server_.random(); }
Envoy::Runtime::Loader& runtime() override { return server_.runtime(); }
Stats::Scope& scope() override { return *server_scope_; }
Expand Down
2 changes: 2 additions & 0 deletions test/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ envoy_cc_test(
"//source/common/router:rds_lib",
"//source/server/http:admin_lib",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/server:server_mocks",
"//test/mocks/thread_local:thread_local_mocks",
"//test/mocks/upstream:upstream_mocks",
Expand Down Expand Up @@ -112,6 +113,7 @@ envoy_cc_test(
"//source/server/http:admin_lib",
"//test/mocks/config:config_mocks",
"//test/mocks/init:init_mocks",
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/router:router_mocks",
"//test/mocks/server:server_mocks",
"//test/test_common:simulated_time_system_lib",
Expand Down
8 changes: 8 additions & 0 deletions test/common/router/rds_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "test/mocks/init/mocks.h"
#include "test/mocks/local_info/mocks.h"
#include "test/mocks/protobuf/mocks.h"
#include "test/mocks/server/mocks.h"
#include "test/mocks/thread_local/mocks.h"
#include "test/mocks/upstream/mocks.h"
Expand Down Expand Up @@ -54,6 +55,12 @@ class RdsTestBase : public testing::Test {
.WillByDefault(ReturnRef(server_factory_context_));
ON_CALL(server_factory_context_, scope()).WillByDefault(ReturnRef(scope_));
ON_CALL(mock_factory_context_, scope()).WillByDefault(ReturnRef(scope_));
ON_CALL(server_factory_context_, messageValidationContext())
.WillByDefault(ReturnRef(validation_context_));
ON_CALL(mock_factory_context_, messageValidationContext())
.WillByDefault(ReturnRef(validation_context_));
EXPECT_CALL(validation_context_, dynamicValidationVisitor())
.WillRepeatedly(ReturnRef(ProtobufMessage::getStrictValidationVisitor()));

ON_CALL(mock_factory_context_, initManager()).WillByDefault(ReturnRef(outer_init_manager_));
ON_CALL(outer_init_manager_, add(_)).WillByDefault(Invoke([this](const Init::Target& target) {
Expand All @@ -67,6 +74,7 @@ class RdsTestBase : public testing::Test {
Event::SimulatedTimeSystem& timeSystem() { return time_system_; }

Event::SimulatedTimeSystem time_system_;
NiceMock<ProtobufMessage::MockValidationContext> validation_context_;
NiceMock<Server::Configuration::MockFactoryContext> mock_factory_context_;
NiceMock<Init::MockManager> outer_init_manager_;
NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
Expand Down
10 changes: 10 additions & 0 deletions test/common/router/scoped_rds_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@

#include "common/config/api_version.h"
#include "common/config/grpc_mux_impl.h"
#include "common/protobuf/message_validator_impl.h"
#include "common/router/scoped_rds.h"

#include "test/mocks/config/mocks.h"
#include "test/mocks/protobuf/mocks.h"
#include "test/mocks/router/mocks.h"
#include "test/mocks/server/mocks.h"
#include "test/test_common/simulated_time_system.h"
Expand All @@ -34,6 +36,7 @@ using testing::Invoke;
using testing::IsNull;
using testing::NiceMock;
using testing::Return;
using testing::ReturnRef;

namespace Envoy {
namespace Router {
Expand Down Expand Up @@ -67,6 +70,12 @@ class ScopedRoutesTestBase : public testing::Test {
ON_CALL(factory_context_, initManager()).WillByDefault(ReturnRef(context_init_manager_));
ON_CALL(factory_context_, getServerFactoryContext())
.WillByDefault(ReturnRef(server_factory_context_));
ON_CALL(factory_context_, messageValidationContext())
.WillByDefault(ReturnRef(validation_context_));
ON_CALL(server_factory_context_, messageValidationContext())
.WillByDefault(ReturnRef(validation_context_));
EXPECT_CALL(validation_context_, dynamicValidationVisitor())
.WillRepeatedly(ReturnRef(ProtobufMessage::getStrictValidationVisitor()));

EXPECT_CALL(factory_context_.admin_.config_tracker_, add_("routes", _));
route_config_provider_manager_ =
Expand Down Expand Up @@ -101,6 +110,7 @@ class ScopedRoutesTestBase : public testing::Test {
NiceMock<Init::MockManager> context_init_manager_;
// factory_context_ is used by srds
NiceMock<Server::Configuration::MockFactoryContext> factory_context_;
NiceMock<ProtobufMessage::MockValidationContext> validation_context_;
// server_factory_context_ is used by rds
NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
std::unique_ptr<RouteConfigProviderManager> route_config_provider_manager_;
Expand Down
2 changes: 2 additions & 0 deletions test/mocks/server/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ MockServerFactoryContext::MockServerFactoryContext()
ON_CALL(*this, admin()).WillByDefault(ReturnRef(admin_));
ON_CALL(*this, api()).WillByDefault(ReturnRef(api_));
ON_CALL(*this, timeSource()).WillByDefault(ReturnRef(time_system_));
ON_CALL(*this, messageValidationContext()).WillByDefault(ReturnRef(validation_context_));
ON_CALL(*this, messageValidationVisitor())
.WillByDefault(ReturnRef(ProtobufMessage::getStrictValidationVisitor()));
ON_CALL(*this, api()).WillByDefault(ReturnRef(api_));
Expand All @@ -230,6 +231,7 @@ MockFactoryContext::MockFactoryContext()
ON_CALL(*this, api()).WillByDefault(ReturnRef(api_));
ON_CALL(*this, timeSource()).WillByDefault(ReturnRef(time_system_));
ON_CALL(*this, overloadManager()).WillByDefault(ReturnRef(overload_manager_));
ON_CALL(*this, messageValidationContext()).WillByDefault(ReturnRef(validation_context_));
ON_CALL(*this, messageValidationVisitor())
.WillByDefault(ReturnRef(ProtobufMessage::getStrictValidationVisitor()));
ON_CALL(*this, api()).WillByDefault(ReturnRef(api_));
Expand Down
Loading