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
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.

6 changes: 6 additions & 0 deletions include/envoy/protobuf/message_validator.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ 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() PURE;
};

class ValidationContext {
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;

/**
* @return bool ignore unknown fields in the dynamic configuration?
**/
virtual bool ignoreUnknownDynamicFields() const PURE;

/**
* @return const std::string& the admin address output file.
*/
Expand Down
16 changes: 14 additions & 2 deletions source/common/protobuf/message_validator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ class NullValidationVisitorImpl : public ValidationVisitor {
public:
// Envoy::ProtobufMessage::ValidationVisitor
void onUnknownField(absl::string_view) override {}

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

ValidationVisitor& getNullValidationVisitor();
Expand All @@ -26,6 +29,9 @@ class WarningValidationVisitorImpl : public ValidationVisitor,
// Envoy::ProtobufMessage::ValidationVisitor
void onUnknownField(absl::string_view description) override;

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

private:
// Track hashes of descriptions we've seen, to avoid log spam. A hash is used here to avoid
// wasting memory with unused strings.
Expand All @@ -40,6 +46,9 @@ class StrictValidationVisitorImpl : public ValidationVisitor {
public:
// Envoy::ProtobufMessage::ValidationVisitor
void onUnknownField(absl::string_view description) override;

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

ValidationVisitor& getStrictValidationVisitor();
Expand All @@ -62,11 +71,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
4 changes: 3 additions & 1 deletion source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,9 @@ class MessageUtil {
static void validate(const MessageType& message,
ProtobufMessage::ValidationVisitor& validation_visitor) {
// Log warnings or throw errors if deprecated fields or unknown fields are in use.
checkForUnexpectedFields(message, validation_visitor);
if (!validation_visitor.skipValidation()) {
checkForUnexpectedFields(message, validation_visitor);
}

std::string err;
if (!Validate(message, &err)) {
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
5 changes: 5 additions & 0 deletions source/server/options_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ 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",
"ignore unknown fields in dynamic configuration",
cmd, false);

TCLAP::ValueArg<std::string> admin_address_path("", "admin-address-path", "Admin address path",
false, "", "string", cmd);
Expand Down Expand Up @@ -235,6 +238,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 +325,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
4 changes: 4 additions & 0 deletions test/common/protobuf/message_validator_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ namespace {
// The null validation visitor doesn't do anything on unknown fields.
TEST(NullValidationVisitorImpl, UnknownField) {
NullValidationVisitorImpl null_validation_visitor;
EXPECT_TRUE(null_validation_visitor.skipValidation());
EXPECT_NO_THROW(null_validation_visitor.onUnknownField("foo"));
}

Expand All @@ -24,6 +25,8 @@ TEST(WarningValidationVisitorImpl, UnknownField) {
Stats::TestUtil::TestStore stats;
Stats::Counter& counter = stats.counter("counter");
WarningValidationVisitorImpl warning_validation_visitor;
// we want to be executed.
EXPECT_FALSE(warning_validation_visitor.skipValidation());
// First time around we should log.
EXPECT_LOG_CONTAINS("warn", "Unknown field: foo",
warning_validation_visitor.onUnknownField("foo"));
Expand All @@ -46,6 +49,7 @@ TEST(WarningValidationVisitorImpl, UnknownField) {
// The strict validation visitor throws on unknown fields.
TEST(StrictValidationVisitorImpl, UnknownField) {
StrictValidationVisitorImpl strict_validation_visitor;
EXPECT_FALSE(strict_validation_visitor.skipValidation());
EXPECT_THROW_WITH_MESSAGE(strict_validation_visitor.onUnknownField("foo"),
UnknownProtoFieldException,
"Protobuf message (foo) has unknown fields");
Expand Down
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