Skip to content
Merged
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
49 changes: 49 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "envoy/json/json_object.h"
#include "envoy/type/percent.pb.h"

#include "common/common/fmt.h"
#include "common/common/hash.h"
#include "common/common/utility.h"
#include "common/json/json_loader.h"
Expand Down Expand Up @@ -177,6 +178,51 @@ class MessageUtil {
static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
static void loadFromFile(const std::string& path, Protobuf::Message& message);

static void checkForDeprecation(const Protobuf::Message& message, bool fatal) {

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.

move the impl to .cc

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.

Nit: prefer warn_only vs. fatal, since it's not fatal, it's raising an exception which could be handled in other ways at a call-site (even though you probably only plan on using it for fatal..).

const Protobuf::Descriptor* descriptor = message.GetDescriptor();
const Protobuf::Reflection* reflection = message.GetReflection();
for (int i = 0; i < descriptor->field_count(); ++i) {
const auto* field = descriptor->field(i);

// If this field is not in use, continue.
if ((field->is_repeated() && reflection->FieldSize(message, field) == 0) ||
(!field->is_repeated() && !reflection->HasField(message, field))) {
continue;
}

// If this field is deprecated, warn or throw an error.
if (field->options().deprecated()) {
std::string err = fmt::format(
"Using deprecated option '{}'. This configuration will be removed from Envoy soon. "
"Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for "
"details.",
field->name());

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.

full_name() instead of name should be more helpful.

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.

Thanks for the tip - this is going to be even more useful!

if (fatal) {
throw ProtoValidationException(err, message);
} else {
ENVOY_LOG_MISC(warn, "{}", err);

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.

use config logger?

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.

Which logger? The only other log macro in the util.cc used MISC so I didn't think any of the other log macros were suitable.

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.

We could wrap in a class to provide access to the config logger (or maybe there is a more direct way?).

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.

Just make MessageUtil inherit Loggable and it should be good (it only provides static method)

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.

Ah, so you want it to inherit from Logger::LoggableLogger::Id::config

Done, though I think ENVOY_LOG_MISC might make more sense, given that someone could put non-config related proto utilities in source/common/protobuf/utility.h

That said the main uses of proto are config and gRPC which has its own directories, so it's fine for now.

}
}

// If this is a message, recurse to check for deprecated fields in the sub-message.
switch (field->cpp_type()) {

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.

just if (field->cpp_type == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE)?

case Protobuf::FieldDescriptor::CPPTYPE_MESSAGE: {
if (field->is_repeated()) {
const int size = reflection->FieldSize(message, field);
for (int j = 0; j < size; ++j) {
checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), fatal);
}
} else {
checkForDeprecation(reflection->GetMessage(message, field), fatal);
}
break;
}
default:
break;
}
}
}

/**
* Validate protoc-gen-validate constraints on a given protobuf.
* Note the corresponding `.pb.validate.h` for the message has to be included in the source file
Expand All @@ -185,6 +231,9 @@ class MessageUtil {
* @throw ProtoValidationException if the message does not satisfy its type constraints.
*/
template <class MessageType> static void validate(const MessageType& message) {
// Do a (non-fatal) deprecation check to log warnings about deprecated field use.
checkForDeprecation(message, false);

std::string err;
if (!Validate(message, &err)) {
throw ProtoValidationException(err, message);
Expand Down
2 changes: 2 additions & 0 deletions test/common/protobuf/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ envoy_cc_test(
srcs = ["utility_test.cc"],
deps = [
"//source/common/protobuf:utility_lib",
"//test/proto:deprecated_proto_cc",
"//test/test_common:environment_lib",
"//test/test_common:logging_lib",
"//test/test_common:utility_lib",
"@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc",
],
Expand Down
61 changes: 61 additions & 0 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
#include "common/protobuf/protobuf.h"
#include "common/protobuf/utility.h"

#include "test/proto/deprecated.pb.h"
#include "test/test_common/environment.h"
#include "test/test_common/logging.h"
#include "test/test_common/utility.h"

#include "gtest/gtest.h"
Expand Down Expand Up @@ -40,6 +42,65 @@ TEST(UtilityTest, DowncastAndValidate) {
ProtoValidationException);
}

TEST(UtilityTest, ValidateDeprecatedFields) {
{
envoy::test::deprecation_test::Base base;
base.set_not_deprecated("foo");
// Fatal checks for a non-deprecated field should cause no problem.
MessageUtil::checkForDeprecation(base, true);
}
{
envoy::test::deprecation_test::Base base;
base.set_is_deprecated("foo");
// Non-fatal checks for a deprecated field should log rather than throw an exception.
EXPECT_LOG_CONTAINS("warning", "Using deprecated option 'is_deprecated'.",
MessageUtil::checkForDeprecation(base, false));
// Fatal checks for a deprecated field should result in an exception.
EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException,
"Using deprecated option 'is_deprecated'.");
}
{
envoy::test::deprecation_test::Base base;
base.mutable_deprecated_message();
// Fatal checks for a present (unused) deprecated message should result in an exception.
EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException,
"Using deprecated option 'deprecated_message'.");
}

{
envoy::test::deprecation_test::Base base;
base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo");
// Non-fatal checks for a deprecated field shouldn't throw an exception.
MessageUtil::checkForDeprecation(base, false);

base.mutable_not_deprecated_message()->set_inner_deprecated("bar");
// Fatal checks for a deprecated sub-message should result in an exception.
EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException,
"Using deprecated option 'inner_deprecated'.");
}

// Check that repeated sub-messages get validated.

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.

Nit: preference for making these all separate TEST_F, since they don't share state and it's more self-documenting.

{
envoy::test::deprecation_test::Base base;
base.add_repeated_message();
base.add_repeated_message()->set_inner_deprecated("foo");
base.add_repeated_message();

// Fatal checks for a repeated deprecated sub-message should result in an exception.
EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException,
"Using deprecated option 'inner_deprecated'.");
}
// Check that deprecated repeated messages trigger
{
envoy::test::deprecation_test::Base base;
base.add_deprecated_repeated_message();

// Fatal checks for a repeated deprecated sub-message should result in an exception.
EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException,
"Using deprecated option 'deprecated_repeated_message'.");
}
}

TEST(UtilityTest, LoadBinaryProtoFromFile) {
envoy::config::bootstrap::v2::Bootstrap bootstrap;
bootstrap.mutable_cluster_manager()
Expand Down
5 changes: 5 additions & 0 deletions test/proto/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ envoy_package()

exports_files(["bookstore.proto"])

envoy_proto_library(
name = "deprecated_proto",
srcs = [":deprecated.proto"],
)

envoy_proto_library(
name = "helloworld_proto",
srcs = [":helloworld.proto"],
Expand Down
16 changes: 16 additions & 0 deletions test/proto/deprecated.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
syntax = "proto3";

package envoy.test.deprecation_test;

message Base {
string not_deprecated = 1;
string is_deprecated = 2 [deprecated = true];
message InnerMessage {
string inner_not_deprecated = 1;
string inner_deprecated = 2 [deprecated = true];
}
InnerMessage deprecated_message = 3 [deprecated = true];
InnerMessage not_deprecated_message = 4;
repeated InnerMessage repeated_message = 5;
repeated InnerMessage deprecated_repeated_message = 6 [deprecated = true];
}