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.

Sign up for GitHub

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

protobuf: report field numbers for unknown fields. #7978

Merged
merged 4 commits into from
Aug 21, 2019
Merged

protobuf: report field numbers for unknown fields. #7978

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19d27bb
protobuf: report field numbers for unknown fields.
htuch Aug 20, 2019
a6fda7b
More typo.
htuch Aug 20, 2019
1547c02
Merge remote-tracking branch 'upstream/master' into proto-warning-fields
htuch Aug 21, 2019
7a660d7
Review feedback.
htuch Aug 21, 2019
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
15 changes: 15 additions & 0 deletions source/common/protobuf/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,21 @@ ProtoValidationException::ProtoValidationException(const std::string& validation
ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what());
}

void MessageUtil::checkUnknownFields(const Protobuf::Message& message,
ProtobufMessage::ValidationVisitor& validation_visitor) {
auto& unknown_fields = message.GetReflection()->GetUnknownFields(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done recursively for embedded message fields?
If I remember, this method also causes allocation of metadata and has performance impact. Perhaps, things are better now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a fair point; this is actually a bug in the existing implementation. We should do it (folks can always disable the checking if they don't want the performance impact). Let me fix that in a followup PR, tracking in #7980.

Copy link
Member

Choose a reason for hiding this comment

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

nit: const auto

// If there are no unknown fields, we're done here.
if (unknown_fields.empty()) {
return;
}
std::string error_msg;
for (int n = 0; n < unknown_fields.field_count(); ++n) {
error_msg += absl::StrCat(n > 0 ? ", " : "", unknown_fields.field(n).number());
}
validation_visitor.onUnknownField("type " + message.GetTypeName() + " with unknown field(s) " +
error_msg);
}

void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message,
ProtobufMessage::ValidationVisitor& validation_visitor) {
Protobuf::util::JsonParseOptions options;
Expand Down
6 changes: 1 addition & 5 deletions source/common/protobuf/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,7 @@ class MessageUtil {
}

static void checkUnknownFields(const Protobuf::Message& message,
ProtobufMessage::ValidationVisitor& validation_visitor) {
if (!message.GetReflection()->GetUnknownFields(message).empty()) {
validation_visitor.onUnknownField("type " + message.GetTypeName());
}
}
ProtobufMessage::ValidationVisitor& validation_visitor);

static void loadFromJson(const std::string& json, Protobuf::Message& message,
ProtobufMessage::ValidationVisitor& validation_visitor);
Expand Down
25 changes: 21 additions & 4 deletions test/common/protobuf/utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,31 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoFromFile) {
EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file));
}

// An unknown field (or with wrong type) in a message is rejected.
TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownFieldFromFile) {
ProtobufWkt::Duration source_duration;
source_duration.set_seconds(42);
const std::string filename =
TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString());
envoy::config::bootstrap::v2::Bootstrap proto_from_file;
EXPECT_THROW_WITH_MESSAGE(
TestUtility::loadFromFile(filename, proto_from_file, *api_), EnvoyException,
"Protobuf message (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields");
EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_),
EnvoyException,
"Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with "
Copy link
Member

Choose a reason for hiding this comment

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

nit: this error message is a bit redundant with multiple "unknown fields." Any way to clean it up a bit with the new logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to unknown field set {1, 2}. It's a bit hard to avoid the double unknown, since the JSON throw has an opaque string with its own description that needs wrapping to explain that it relates to unknown fields.

"unknown field(s) 1) has unknown fields");
}

// Multiple unknown fields (or with wrong type) in a message are rejected.
TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownMultipleFieldsFromFile) {
ProtobufWkt::Duration source_duration;
source_duration.set_seconds(42);
source_duration.set_nanos(42);
const std::string filename =
TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString());
envoy::config::bootstrap::v2::Bootstrap proto_from_file;
EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_),
EnvoyException,
"Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with "
"unknown field(s) 1, 2) has unknown fields");
}

TEST_F(ProtobufUtilityTest, LoadTextProtoFromFile) {
Expand Down Expand Up @@ -333,7 +349,8 @@ TEST_F(ProtobufUtilityTest, AnyConvertWrongFields) {
source_any.set_type_url("type.google.com/google.protobuf.Timestamp");
EXPECT_THROW_WITH_MESSAGE(TestUtility::anyConvert<ProtobufWkt::Timestamp>(source_any),
EnvoyException,
"Protobuf message (type google.protobuf.Timestamp) has unknown fields");
"Protobuf message (type google.protobuf.Timestamp with unknown "
"field(s) 1) has unknown fields");
}

TEST_F(ProtobufUtilityTest, JsonConvertSuccess) {
Expand Down
2 changes: 1 addition & 1 deletion test/server/server_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,7 @@ TEST_P(ServerInstanceImplTest, EmptyBootstrap) {
}

// Custom header bootstrap succeeds.
TEST_P(ServerInstanceImplTest, CusomHeaderBoostrap) {
TEST_P(ServerInstanceImplTest, CustomHeaderBootstrap) {
options_.config_path_ = TestEnvironment::writeStringToFileForTest(
"custom.yaml", "header_prefix: \"x-envoy\"\nstatic_resources:\n");
options_.service_cluster_name_ = "some_cluster_name";
Expand Down