-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: bootstrap v1 JSON -> proto translation. #1521
Conversation
Also removed deprecated statsd_local_udp_port config option, since this was deprecated in 1.4.0 and this PR will merge after the release.
Nice. I'll put the static stats registration on top of this once it gets merged. |
@htuch I was going to review this while you were out, but this needs mega merge, so I'm not. Sorry. :) |
@alyssawilk Check out |
@zuercher check out https://circleci.com/gh/turbinelabs/envoy/254. Looks like some kind of clock skew issue on CircleCI, bizzaro. |
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.
very nice. few questions/comments.
JSON_UTIL_SET_DURATION(json_config, *watchdog, kill_timeout); | ||
JSON_UTIL_SET_DURATION(json_config, *watchdog, multikill_timeout); | ||
|
||
if (json_config.hasObject("tracing")) { |
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: This could be slightly less nested by doing getObject("tracing", true)
, etc.
@@ -68,6 +68,14 @@ class MessageUtil { | |||
static void loadFromFile(const std::string& path, Protobuf::Message& message); | |||
|
|||
/** | |||
* Convert between two protobufs via a JSON round-trip. This is used to translate arbitrary |
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.
Question (mostly curious): Is this really the best way of doing this? Isn't there some internal way that the library can do this using reflection? Seems silly to have to go to JSON and back.
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'm checking with some folks on this, but I think this is the most straightforward using the utilities available. I will add a TODO, it seems it should be possible to do something similar to the serialization utils but producing a Struct instead of string and vice versa.
config.getObject("outlier_detection")->getString("event_log_path", ""); | ||
const auto& cm_config = bootstrap.cluster_manager(); | ||
if (cm_config.has_outlier_detection()) { | ||
std::string event_log_file_path = cm_config.outlier_detection().event_log_path(); |
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.
Is the reason this is not const std::string&
string compat issues? (could at least make const)
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 think that was accidental, but now that you mention it, we probably should be doing that, so all good :)
@@ -64,12 +65,13 @@ void ValidationInstance::initialize(Options& options, | |||
if (!options.bootstrapPath().empty()) { | |||
MessageUtil::loadFromFile(options.bootstrapPath(), bootstrap); | |||
} | |||
Config::BootstrapJson::translateBootstrap(*config_json, bootstrap); // TODO(htuch): only if -c. |
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 expand TODO? Not sure what it means exactly.
source/server/server.cc
Outdated
@@ -151,18 +152,20 @@ void InstanceImpl::initialize(Options& options, | |||
restarter_.version()); | |||
|
|||
// Handle configuration that needs to take place prior to the main configuration load. | |||
Json::ObjectSharedPtr config_json = Json::Factory::loadFromFile(options.configPath()); | |||
envoy::api::v2::Bootstrap bootstrap; | |||
if (!options.bootstrapPath().empty()) { |
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.
Aren't we at parity now? Do we need two options? Can we autodetect somehow and just switch back to -c only?
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 do you propose doing this? Attempt to parse into a proto, if this fails then print a warning and switch to v1 JSON? We will have both v1 and v2 .json
files.
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.
Yeah, that's what I would do. For now, we could not have a warning, but as we move towards v1 deprecation and we have docs, we could start printing a warning that says something like "v1 configuration detected, please switch to v2" etc.
// Fake upstream. | ||
fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); | ||
|
||
// Admin access log path and address. |
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.
FWIW I find the following substantially harder to read vs. YAML/JSON. Is there any particular reason to build the config this way? (I know we have talked about making integration tests better, but to me that's mostly about reducing code duplication in tests. For configs IMO it's a lot simpler to read a config as is, even if there is duplication).
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 don't think it would be bad to have a "default config" in string JSON/YAML as long as we're (eventually) reading it into in-memory structs and allowing C++ style manipulation before re-serializing.
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.
+1, there is no reason why tests couldn't manipulate configs once read in. IMO that would be much easier to read/grok.
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 was trying to demonstrate how we can dynamically build from scratch a complete config. I think we will want to do this to reduce boilerplate in integration tests long term. I'll add a TODO on potentially factoring part of this out to a static file.
The idea is we will only have this kind of construction happening in one IntegrationTestConfig
class that is appropriately parameterized, we won't be copying+pasting this across tests.
namespace Envoy { | ||
namespace { | ||
|
||
class BindIntegrationTest : public BaseIntegrationTest, |
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.
Preference for leaving the test in the same file, with a TODO for me to pull out all the annoying config gorp into a helper class in a follow-up
If you'd prefer having a separate file let's make it generic YamlIntegrationTest or ProtoIntegrationTest where we encourage others adding new features to drop them here, with a TODO for me to pull the bind-specific logic into just the Bind test.
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 will make this ProtoIntegrationTest
and add a TODO for you.
Was broken by interaction of envoyproxy#1521 and envoyproxy#1570.
Also removed deprecated statsd_local_udp_port config option, since this was deprecated in 1.4.0 and this PR will merge after the release.