Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eds: optionalize counting of unknown fields #10982

Merged
merged 13 commits into from
May 12, 2020
5 changes: 4 additions & 1 deletion api/envoy/admin/v3/server_info.proto
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ message ServerInfo {
CommandLineOptions command_line_options = 6;
}

// [#next-free-field: 30]
// [#next-free-field: 31]
message CommandLineOptions {
option (udpa.annotations.versioning).previous_message_type =
"envoy.admin.v2alpha.CommandLineOptions";
Expand Down Expand Up @@ -97,6 +97,9 @@ message CommandLineOptions {
// See :option:`--reject-unknown-dynamic-fields` for details.
bool reject_unknown_dynamic_fields = 26;

// See :option:`--ignore_unknown_dynamic_fields` for details.
bool ignore_unknown_dynamic_fields = 30;

// See :option:`--admin-address-path` for details.
string admin_address_path = 6;

Expand Down
5 changes: 4 additions & 1 deletion api/envoy/admin/v4alpha/server_info.proto
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ message ServerInfo {
CommandLineOptions command_line_options = 6;
}

// [#next-free-field: 30]
// [#next-free-field: 31]
message CommandLineOptions {
option (udpa.annotations.versioning).previous_message_type = "envoy.admin.v3.CommandLineOptions";

Expand Down Expand Up @@ -96,6 +96,9 @@ message CommandLineOptions {
// See :option:`--reject-unknown-dynamic-fields` for details.
bool reject_unknown_dynamic_fields = 26;

// See :option:`--ignore_unknown_dynamic_fields` for details.
pgenera marked this conversation as resolved.
Show resolved Hide resolved
bool ignore_unknown_dynamic_fields = 30;

// See :option:`--admin-address-path` for details.
string admin_address_path = 6;

Expand Down
1 change: 1 addition & 0 deletions docs/root/operations/admin.rst
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ modify different aspects of the server:
"service_zone": "",
"mode": "Serve",
"disable_hot_restart": false,
"disable_eds_validation": false,
pgenera marked this conversation as resolved.
Show resolved Hide resolved
"enable_mutex_tracing": false,
"restart_epoch": 0,
"file_flush_interval": "10s",
Expand Down
5 changes: 5 additions & 0 deletions docs/root/operations/cli.rst
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,11 @@ following are the command line options that Envoy supports.
and these occurrences are counted in the :ref:`server.dynamic_unknown_fields <server_statistics>`
statistic.

.. option:: --ignore-unknown-dynamic-fields

*(optional)* This flag disables validation of protobuf configuration
for unknown fields in dynamic configuration, and ignores GENERA write more.
pgenera marked this conversation as resolved.
Show resolved Hide resolved

.. option:: --disable-extensions <extension list>

*(optional)* This flag disabled the provided list of comma-separated extension names. Disabled
Expand Down
5 changes: 4 additions & 1 deletion generated_api_shadow/envoy/admin/v3/server_info.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion generated_api_shadow/envoy/admin/v4alpha/server_info.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions include/envoy/protobuf/message_validator.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,16 @@ class ValidationVisitor {
* @param description human readable description of the field
*/
virtual void onUnknownField(absl::string_view description) PURE;

/**
* If true, skip this validation visitor in the interest of speed when
* possible.
**/
virtual bool skipValidation() { return false; }
pgenera marked this conversation as resolved.
Show resolved Hide resolved
};

class NullValidationVisitor : public ValidationVisitor {};

class ValidationContext {
public:
virtual ~ValidationContext() = default;
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/server/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ class Options {
*/
virtual bool rejectUnknownDynamicFields() const PURE;

/**
* GENERA write the comment.
pgenera marked this conversation as resolved.
Show resolved Hide resolved
**/
virtual bool ignoreUnknownDynamicFields() const PURE;

/**
* @return const std::string& the admin address output file.
*/
Expand Down
12 changes: 9 additions & 3 deletions source/common/protobuf/message_validator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
namespace Envoy {
namespace ProtobufMessage {

class NullValidationVisitorImpl : public ValidationVisitor {
class NullValidationVisitorImpl : public NullValidationVisitor {
public:
// Envoy::ProtobufMessage::ValidationVisitor
void onUnknownField(absl::string_view) override {}

// Envoy::ProtobufMessage::ValidationVisitor
bool skipValidation() override { return true; }
};

ValidationVisitor& getNullValidationVisitor();
Expand Down Expand Up @@ -62,11 +65,14 @@ class ValidationContextImpl : public ValidationContext {

class ProdValidationContextImpl : public ValidationContextImpl {
public:
ProdValidationContextImpl(bool allow_unknown_static_fields, bool allow_unknown_dynamic_fields)
ProdValidationContextImpl(bool allow_unknown_static_fields, bool allow_unknown_dynamic_fields,
bool ignore_unknown_dynamic_fields)
: ValidationContextImpl(allow_unknown_static_fields ? static_warning_validation_visitor_
: getStrictValidationVisitor(),
allow_unknown_dynamic_fields
? dynamic_warning_validation_visitor_
? (ignore_unknown_dynamic_fields
? ProtobufMessage::getNullValidationVisitor()
: dynamic_warning_validation_visitor_)
: ProtobufMessage::getStrictValidationVisitor()) {}

ProtobufMessage::WarningValidationVisitorImpl& static_warning_validation_visitor() {
Expand Down
12 changes: 9 additions & 3 deletions source/common/upstream/eds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,15 @@ void EdsClusterImpl::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt
if (!validateUpdateSize(resources.size())) {
return;
}
auto cluster_load_assignment =
MessageUtil::anyConvertAndValidate<envoy::config::endpoint::v3::ClusterLoadAssignment>(
resources[0], validation_visitor_);
envoy::config::endpoint::v3::ClusterLoadAssignment cluster_load_assignment;
if (validation_visitor_.skipValidation()) {
cluster_load_assignment =
MessageUtil::anyConvert<envoy::config::endpoint::v3::ClusterLoadAssignment>(resources[0]);
} else {
cluster_load_assignment =
MessageUtil::anyConvertAndValidate<envoy::config::endpoint::v3::ClusterLoadAssignment>(
resources[0], validation_visitor_);
pgenera marked this conversation as resolved.
Show resolved Hide resolved
}
if (cluster_load_assignment.cluster_name() != cluster_name_) {
throw EnvoyException(fmt::format("Unexpected EDS cluster (expecting {}): {}", cluster_name_,
cluster_load_assignment.cluster_name()));
Expand Down
12 changes: 8 additions & 4 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1415,16 +1415,18 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,

// Remove hosts from current_priority_hosts that were matched to an existing host in the previous
// loop.
for (auto itr = current_priority_hosts.begin(); itr != current_priority_hosts.end();) {
auto erase_from_itr = current_priority_hosts.end();
for (auto itr = current_priority_hosts.begin(); itr != erase_from_itr;) {
auto existing_itr = existing_hosts_for_current_priority.find((*itr)->address()->asString());

if (existing_itr != existing_hosts_for_current_priority.end()) {
existing_hosts_for_current_priority.erase(existing_itr);
itr = current_priority_hosts.erase(itr);
*itr = *(--erase_from_itr);
} else {
itr++;
}
}
current_priority_hosts.erase(erase_from_itr, current_priority_hosts.end());
pgenera marked this conversation as resolved.
Show resolved Hide resolved

// If we saw existing hosts during this iteration from a different priority, then we've moved
// a host from another priority into this one, so we should mark the priority as having changed.
Expand All @@ -1442,7 +1444,8 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
const bool dont_remove_healthy_hosts =
health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval();
if (!current_priority_hosts.empty() && dont_remove_healthy_hosts) {
for (auto i = current_priority_hosts.begin(); i != current_priority_hosts.end();) {
erase_from_itr = current_priority_hosts.end();
for (auto i = current_priority_hosts.begin(); i != erase_from_itr;) {
if (!((*i)->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC) ||
(*i)->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH))) {
if ((*i)->weight() > max_host_weight) {
Expand All @@ -1452,11 +1455,12 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(const HostVector& new_hosts,
final_hosts.push_back(*i);
updated_hosts[(*i)->address()->asString()] = *i;
(*i)->healthFlagSet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL);
i = current_priority_hosts.erase(i);
*i = *(--erase_from_itr);
} else {
i++;
}
}
current_priority_hosts.erase(erase_from_itr, current_priority_hosts.end());
}

// At this point we've accounted for all the new hosts as well the hosts that previously
Expand Down
3 changes: 2 additions & 1 deletion source/server/config_validation/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ ValidationInstance::ValidationInstance(
Thread::BasicLockable& access_log_lock, ComponentFactory& component_factory,
Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system)
: options_(options), validation_context_(options_.allowUnknownStaticFields(),
!options.rejectUnknownDynamicFields()),
!options.rejectUnknownDynamicFields(),
!options.ignoreUnknownDynamicFields()),
stats_store_(store),
api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system)),
dispatcher_(api_->allocateDispatcher("main_thread")),
Expand Down
4 changes: 4 additions & 0 deletions source/server/options_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
TCLAP::SwitchArg reject_unknown_dynamic_fields("", "reject-unknown-dynamic-fields",
"reject unknown fields in dynamic configuration",
cmd, false);
TCLAP::SwitchArg ignore_unknown_dynamic_fields("", "ignore-unknown-dynamic-fields",
"GENERA more words.", cmd, false);
pgenera marked this conversation as resolved.
Show resolved Hide resolved

TCLAP::ValueArg<std::string> admin_address_path("", "admin-address-path", "Admin address path",
false, "", "string", cmd);
Expand Down Expand Up @@ -235,6 +237,7 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
allow_unknown_static_fields_ =
allow_unknown_static_fields.getValue() || allow_unknown_fields.getValue();
reject_unknown_dynamic_fields_ = reject_unknown_dynamic_fields.getValue();
ignore_unknown_dynamic_fields_ = ignore_unknown_dynamic_fields.getValue();
admin_address_path_ = admin_address_path.getValue();
log_path_ = log_path.getValue();
restart_epoch_ = restart_epoch.getValue();
Expand Down Expand Up @@ -321,6 +324,7 @@ Server::CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const {
command_line_options->set_config_yaml(configYaml());
command_line_options->set_allow_unknown_static_fields(allow_unknown_static_fields_);
command_line_options->set_reject_unknown_dynamic_fields(reject_unknown_dynamic_fields_);
command_line_options->set_ignore_unknown_dynamic_fields(ignore_unknown_dynamic_fields_);
command_line_options->set_admin_address_path(adminAddressPath());
command_line_options->set_component_log_level(component_log_level_str_);
command_line_options->set_log_level(spdlog::level::to_string_view(logLevel()).data(),
Expand Down
6 changes: 6 additions & 0 deletions source/server/options_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
void setRejectUnknownFieldsDynamic(bool reject_unknown_dynamic_fields) {
reject_unknown_dynamic_fields_ = reject_unknown_dynamic_fields;
}
void setIgnoreUnknownFieldsDynamic(bool ignore_unknown_dynamic_fields) {
ignore_unknown_dynamic_fields_ = ignore_unknown_dynamic_fields;
}

void setFakeSymbolTableEnabled(bool fake_symbol_table_enabled) {
fake_symbol_table_enabled_ = fake_symbol_table_enabled;
}
Expand All @@ -107,6 +111,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
const std::string& configYaml() const override { return config_yaml_; }
bool allowUnknownStaticFields() const override { return allow_unknown_static_fields_; }
bool rejectUnknownDynamicFields() const override { return reject_unknown_dynamic_fields_; }
bool ignoreUnknownDynamicFields() const override { return ignore_unknown_dynamic_fields_; }
const std::string& adminAddressPath() const override { return admin_address_path_; }
Network::Address::IpVersion localAddressIpVersion() const override {
return local_address_ip_version_;
Expand Down Expand Up @@ -161,6 +166,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
std::string config_yaml_;
bool allow_unknown_static_fields_{false};
bool reject_unknown_dynamic_fields_{false};
bool ignore_unknown_dynamic_fields_{false};
std::string admin_address_path_;
Network::Address::IpVersion local_address_ip_version_;
spdlog::level::level_enum log_level_;
Expand Down
5 changes: 3 additions & 2 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ InstanceImpl::InstanceImpl(
Filesystem::Instance& file_system, std::unique_ptr<ProcessContext> process_context)
: init_manager_(init_manager), workers_started_(false), live_(false), shutdown_(false),
options_(options), validation_context_(options_.allowUnknownStaticFields(),
!options.rejectUnknownDynamicFields()),
!options.rejectUnknownDynamicFields(),
options.ignoreUnknownDynamicFields()),
time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)),
original_start_time_(start_time_), stats_store_(store), thread_local_(tls),
api_(new Api::Impl(thread_factory, store, time_system, file_system,
Expand Down Expand Up @@ -712,4 +713,4 @@ ProtobufTypes::MessagePtr InstanceImpl::dumpBootstrapConfig() {
}

} // namespace Server
} // namespace Envoy
} // namespace Envoy
31 changes: 31 additions & 0 deletions test/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,37 @@ envoy_cc_test(
],
)

envoy_cc_benchmark_binary(
name = "eds_speed_test",
srcs = ["eds_speed_test.cc"],
external_deps = [
"benchmark",
],
deps = [
":utility_lib",
"//source/common/config:utility_lib",
"//source/common/upstream:eds_lib",
"//source/extensions/transport_sockets/raw_buffer:config",
"//source/server:transport_socket_config_lib",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:server_mocks",
"//test/mocks/ssl:ssl_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto",
"@envoy_api//envoy/service/discovery/v3:pkg_cc_proto",
],
)

envoy_benchmark_test(
name = "eds_speed_test_benchmark_test",
benchmark_binary = "eds_speed_test",
)

envoy_cc_test(
name = "health_checker_impl_test",
srcs = ["health_checker_impl_test.cc"],
Expand Down
Loading