config: bootstrap v1 JSON -> proto translation. by htuch · Pull Request #1521 · envoyproxy/envoy

htuch · 2017-08-23T19:12:06Z

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.

htuch · 2017-08-23T19:13:05Z

@mrice32 @rfaulk @rohitbhoj

mrice32 · 2017-08-23T19:25:35Z

Nice. I'll put the static stats registration on top of this once it gets merged.

mattklein123 · 2017-08-25T16:28:00Z

@htuch I was going to review this while you were out, but this needs mega merge, so I'm not. Sorry. :)

htuch · 2017-08-29T21:50:30Z

@alyssawilk Check out bind_integration_test and let me know what you think. @mattklein123 mega-merge done.

htuch · 2017-08-30T13:37:59Z

@zuercher check out https://circleci.com/gh/turbinelabs/envoy/254. Looks like some kind of clock skew issue on CircleCI, bizzaro.

mattklein123

very nice. few questions/comments.

mattklein123 · 2017-08-30T15:56:25Z

source/common/config/bootstrap_json.cc

+  JSON_UTIL_SET_DURATION(json_config, *watchdog, kill_timeout);
+  JSON_UTIL_SET_DURATION(json_config, *watchdog, multikill_timeout);
+
+  if (json_config.hasObject("tracing")) {


nit: This could be slightly less nested by doing getObject("tracing", true), etc.

mattklein123 · 2017-08-30T16:00:09Z

source/common/protobuf/utility.h

  static void loadFromJson(const std::string& json, Protobuf::Message& message);
  static void loadFromFile(const std::string& path, Protobuf::Message& message);

+  /**


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.

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.

mattklein123 · 2017-08-30T16:02:24Z

source/common/upstream/cluster_manager_impl.cc

-        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();


Is the reason this is not const std::string& string compat issues? (could at least make const)

I think that was accidental, but now that you mention it, we probably should be doing that, so all good :)

mattklein123 · 2017-08-30T16:04:13Z

source/server/config_validation/server.cc

  if (!options.bootstrapPath().empty()) {
    MessageUtil::loadFromFile(options.bootstrapPath(), bootstrap);
  }
+  Config::BootstrapJson::translateBootstrap(*config_json, bootstrap); // TODO(htuch): only if -c.


Can you expand TODO? Not sure what it means exactly.

mattklein123 · 2017-08-30T16:06:28Z

source/server/server.cc

  // 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()) {


Aren't we at parity now? Do we need two options? Can we autodetect somehow and just switch back to -c only?

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.

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.

mattklein123 · 2017-08-30T16:08:18Z

test/integration/bind_integration_test.cc

+    // Fake upstream.
+    fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_));
+
+    // Admin access log path and address.


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).

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.

+1, there is no reason why tests couldn't manipulate configs once read in. IMO that would be much easier to read/grok.

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.

alyssawilk · 2017-08-30T16:10:18Z

test/integration/bind_integration_test.cc

+namespace Envoy {
+namespace {
+
+class BindIntegrationTest : public BaseIntegrationTest,


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.

I will make this ProtoIntegrationTest and add a TODO for you.

Was broken by interaction of envoyproxy#1521 and envoyproxy#1570.

Was broken by interaction of #1521 and #1570.

…lude thought is true (#1521) **Description** When `includeThought` is true, Gemini would also generate summary of thinking process. We need to parse out this kind of data to users. Otherwise, we would return thought process together with output to users. Depends/base or replace envoyproxy/ai-gateway#1461 --------- Signed-off-by: yxia216 <yxia216@bloomberg.net>

config: bootstrap v1 JSON -> proto translation.

28f3a04

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.

mrice32 mentioned this pull request Aug 25, 2017

Made stats sinks a statically registered component #1506

Merged

htuch added 2 commits August 29, 2017 12:25

Merge remote-tracking branch 'upstream/master' into bootstrap-v1-parity

47258cb

Generate bind integration test bootstrap proto instead of overlay.

7328bd4

mattklein123 reviewed Aug 30, 2017

View reviewed changes

alyssawilk reviewed Aug 30, 2017

View reviewed changes

htuch added 3 commits August 30, 2017 12:25

Merge remote-tracking branch 'upstream/master' into bootstrap-v1-parity

b8c179e

Review feedback.

542def2

Remove bootstrap -b CLI flag.

2d2c373

mattklein123 approved these changes Aug 31, 2017

View reviewed changes

htuch merged commit 34c0098 into envoyproxy:master Aug 31, 2017

htuch deleted the bootstrap-v1-parity branch August 31, 2017 18:28

htuch added a commit to htuch/envoy that referenced this pull request Aug 31, 2017

test: fix //test/common/upstream:cluster_manager_impl_test_lib.

7357f1e

Was broken by interaction of envoyproxy#1521 and envoyproxy#1570.

htuch mentioned this pull request Aug 31, 2017

test: fix //test/common/upstream:cluster_manager_impl_test_lib. #1580

Merged

mattklein123 pushed a commit that referenced this pull request Aug 31, 2017

test: fix //test/common/upstream:cluster_manager_impl_test_lib. (#1580)

4de12ad

Was broken by interaction of #1521 and #1570.

Conversation

htuch commented Aug 23, 2017

Uh oh!

htuch commented Aug 23, 2017

Uh oh!

mrice32 commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Aug 25, 2017

Uh oh!

htuch commented Aug 29, 2017

Uh oh!

htuch commented Aug 30, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mrice32 commented Aug 23, 2017 •

edited

Loading