-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
config: allow unknown fields flag (take 2) #4096
config: allow unknown fields flag (take 2) #4096
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we aren't using proto reflection to prohibit unknown fields in proto input in this version?
source/common/protobuf/utility.cc
Outdated
} | ||
} | ||
|
||
void MessageUtil::loadFromJsonLenient(const std::string& json, Protobuf::Message& message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could factor out some of the body of these two to a shared util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way of doing this is to have one function, and make the parameter for leniency be an enum instead of bool. That would be my vote as you would share code and have callsite clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I combined the logic into one function which is only called from two places (json convert and a test). The old loadFromJson
used in many places in tests, which makes me hesitant to change its signature.
@@ -92,4 +91,5 @@ proto_doc_aspect = aspect( | |||
cfg = "host", | |||
), | |||
}, | |||
implementation = _proto_doc_aspect_impl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is just some Buildifier artifact..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not sure why check_format
is eager to refactor all bazel files on my machine.
cluster: "cluster_0" | ||
- match: | ||
method_name: "poke" | ||
- match: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Could you revert the original and switch method_name to method? That should eliminate the unknown fields. (I'll circle back and add a second cluster to this test and use it for the one of the routes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that'll just make half the tests fail. You can leave this and I'll fix it myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am getting errors if I put it back...
@@ -52,7 +52,14 @@ class Utility { | |||
Protobuf::RepeatedPtrField<ResourceType> typed_resources; | |||
for (const auto& resource : response.resources()) { | |||
auto* typed_resource = typed_resources.Add(); | |||
resource.UnpackTo(typed_resource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we were ignoring mismatched type errors during unpacking.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think this is close to the complete solution, just some refactor cleanups requested. Thanks.
source/common/protobuf/utility.cc
Outdated
@@ -68,6 +78,11 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa | |||
if (StringUtil::endsWith(path, ".pb")) { | |||
// Attempt to parse the binary format. | |||
if (message.ParseFromString(contents)) { | |||
if (!MessageUtil::allow_unknown_fields && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you factor this out to a common utility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: Kuat Yessenov <kuat@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two minor comments.
source/common/protobuf/utility.h
Outdated
static void checkUnknownFields(const Protobuf::Message& message) { | ||
if (!MessageUtil::allow_unknown_fields && | ||
!message.GetReflection()->GetUnknownFields(message).empty()) { | ||
throw EnvoyException("Protobuf Any (type " + message.GetTypeName() + ") has unknown fields"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also used when loading from file, so not always Any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
test/common/protobuf/utility_test.cc
Outdated
const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value"); | ||
envoy::config::bootstrap::v2::Bootstrap bootstrap; | ||
EXPECT_NO_THROW(MessageUtil::jsonConvert(obj, bootstrap)); | ||
MessageUtil::allow_unknown_fields = false; | ||
} | ||
|
||
TEST(UtilityTest, JsonConvertFail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally also add a test to ensure server options have the desired effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks great. Happy to see us iterate on this. Please add docs and release notes for the new CLI flag.
source/common/protobuf/utility.cc
Outdated
} | ||
} | ||
|
||
void MessageUtil::loadFromJsonLenient(const std::string& json, Protobuf::Message& message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way of doing this is to have one function, and make the parameter for leniency be an enum instead of bool. That would be my vote as you would share code and have callsite clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback.
source/common/protobuf/utility.cc
Outdated
} | ||
} | ||
|
||
void MessageUtil::loadFromJsonLenient(const std::string& json, Protobuf::Message& message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I combined the logic into one function which is only called from two places (json convert and a test). The old loadFromJson
used in many places in tests, which makes me hesitant to change its signature.
source/common/protobuf/utility.cc
Outdated
@@ -48,9 +48,16 @@ ProtoValidationException::ProtoValidationException(const std::string& validation | |||
ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what()); | |||
} | |||
|
|||
bool MessageUtil::allow_unknown_fields = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity, can we make this an enum with 2 states? Then the function can take the enum value like I mentioned before (and CLI can convert to enum).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, let me know if I need to change the names of anything to conform with the style.
source/common/protobuf/utility.cc
Outdated
MessageUtil::loadFromJsonCustom(json, message, false); | ||
} | ||
|
||
void MessageUtil::loadFromJsonCustom(const std::string& json, Protobuf::Message& message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadFromJsonEx
? (Not sure what custom means exactly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -158,7 +160,19 @@ class MessageUtil { | |||
return HashUtil::xxHash64(text); | |||
} | |||
|
|||
static ProtoUnknownFieldsMode proto_unknown_fields; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For readability, can you do an explicit initialization to Strict here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give me an example how to initialize a non-const static in the header?
I have explicit initialization in the .cc file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that, LGTM, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than @htuch comment. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>
Description: Add a flag
allow-unknown-fields
that controls whether proto conversion ignores unknown fields. Proto structs are uniformly used for extension configuration. A smaller fix than #4094.The validation is applied in the following places:
Risk Level: Medium. Allow unknown fields is set to false by default, meaning unknown fields will be rejected by envoy. A missing check for xDS type checking was discovered.
Testing: Unit tests were updated to have it strict by default. Some wrong filter configs were discovered.
Docs Changes: None
Release Notes:
Add a flag
allow-unknown-fields
to control validation of protobuf configurations for unknown fields.Fixes #4053
/cc @PiotrSikora