schema: enforce cluster name length and invalid chars#667
schema: enforce cluster name length and invalid chars#667mattklein123 merged 6 commits intomasterfrom
Conversation
|
tests |
|
@kyessenov FYI |
| try { | ||
| create(*loader); | ||
| } catch (Json::Exception& e) { | ||
| EXPECT_EQ("JSON object doesn't conform to schema.\n Invalid schema: #/properties/name.\n " |
There was a problem hiding this comment.
just validate against Json::Exception and do not do precise check on the message. The message can change with the library update, etc. I think it should be sufficient to check that it's not successful creation.
There was a problem hiding this comment.
I disagree, this is a better test. Otherwise a JSON exception could be thrown for any number of reasons. If you don't want to do an exact string match, you could do a find(), or even change the Exception type to be a JsonSchemaException which derives from JsonException.
Also, as an aside, you need to fail in the case where you don't throw an exception. Optimally you would be doing something like EXPECT_THROW(). It would be nice to actually make a variant of EXPECT_THROW() that can check against a message also.
There was a problem hiding this comment.
"I disagree, this is a better test." yup, we quickly chatted with Jose on this offline and he was expressing similar concerns, which is a valid point.
There was a problem hiding this comment.
Yeah I was looking for something like that in gtest.
| Json::ObjectPtr loader = Json::Factory::LoadFromString(json); | ||
| try { | ||
| create(*loader); | ||
| ADD_FAILURE() << "Json::Exception should take place. It did not."; |
There was a problem hiding this comment.
This is a common pattern that we will do again (and should do more). make a macro like EXPECT_THROW(), EXPECT_THROW_WITH_MESSAGE(), put it somewhere common, then use it.
|
Does this apply to CDS/RDS supplied clusters? |
|
@kyessenov yes |
| "JSON object doesn't conform to schema.\n Invalid schema: " | ||
| "#/properties/name.\n Invalid keyword: maxLength.\n Invalid document " | ||
| "key: #/name"); | ||
| factory_.tls_.shutdownThread(); |
test/test_common/utility.h
Outdated
|
|
||
| #include "common/http/header_map_impl.h" | ||
|
|
||
| using testing::_; |
There was a problem hiding this comment.
don't do "using" in a header file like this. Remove and be explicit below.
This unbreaks some cases where patching complains about too short functions to patch. What happens is we first locate one of CRT-s (like ucrt or msvcrt) and patch __expand there, redirecting to our implementation. Then "static" __expand replacement is patched, but it is usually imported from that same C runtime DLL. And through several jmp redirections we end up at our own __expand from libc<1>. Patching that (and other cases) is wrong, but I am unsure how to fix it properly. So we do most simple workaround. I found that when it not fails is either in debug builds where empty expand is not too short or when MSVC deduplicates multiple identical __expand implementations into single function, or when 64-bit patching has to do extra trampoline thingy. And then our patching code checks if we're trying to replace some function with itself. So we "just" take advantage of that and get immediate issue fixed, while punting on more general "duplicate" patching for later. Update github issue envoyproxy#667
@lyft/network-team
Right now the longest stat prefix and trailing stat that we output for clusters is
cluster.<cluster_name>.outlier_detection.ejections_consecutive_5xx, which is 52 characters long. In order to have a bit more room to grow 60 characters might be a good limit to have on cluster name length right now.In addition we can use regex in the schema to enforce the absence of invalid characters,
:for now.fixes #573