Converting mongo proxy to v2 api by rshriram · Pull Request #1952 · envoyproxy/envoy

rshriram · 2017-10-26T21:52:39Z

No description provided.

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

htuch

Looks good! Thanks for taking on this task, it was one of the last mile things we need for v2 completeness.

htuch · 2017-10-27T03:16:34Z

source/common/config/filter_json.cc

+    delay->set_type(envoy::api::v2::filter::FaultDelay::FaultDelayType::FIXED);
+    delay->set_percent(static_cast<uint32_t>(json_fault->getInteger("percent")));
+    delay->mutable_fixed_delay()->CopyFrom(
+        Protobuf::util::TimeUtil::MillisecondsToDuration(json_fault->getInteger("duration_ms")));


JSON_UTIL_SET_DURATION, we probably need a variant where the old/new field names are different as well.

htuch · 2017-10-27T03:19:22Z

source/server/config/network/mongo_proxy.h

  // NamedNetworkFilterConfigFactory
  NetworkFilterFactoryCb createFilterFactory(const Json::Object& config,
                                             FactoryContext& context) override;
+  NetworkFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message& config,


You will also need to provider the factory https://github.com/envoyproxy/envoy/blob/master/include/envoy/server/filter_config.h#L173.

Can we make sure we have tests that cover both the legacy and factory config flows also when you fix this. Thank you!

I have not touched any existing tests other than fixing up the class names. And the existing tests use JSON config for testing. i.e. they exercise the conversion and tests pass.

Right my point is can you please make sure that the native proto loading path for v2 config is covered, and not just the conversion path. You may need to add new tests. I think this is important as we add the rest of the filters.

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram · 2017-10-27T19:05:15Z

Added tests for protos. PTAL.

mattklein123

looks great. thank for doing this work. Some small comments/nits.

mattklein123 · 2017-10-27T19:46:38Z

source/common/config/filter_json.h

+
+  /**
+   * Translate a v1 JSON Mongo proxy object to v2
+   * envoy::api::v2::filter::network::MongoProxy.


nit: flow to 100 col

mattklein123 · 2017-10-27T19:48:03Z

source/server/config/network/BUILD

        "//include/envoy/server:filter_config_interface",
        "//source/common/config:well_known_names",
-        "//source/common/json:config_schemas_lib",
        "//source/common/json:json_loader_lib",


mattklein123 · 2017-10-27T19:48:51Z

source/server/config/network/mongo_proxy.cc

+    const envoy::api::v2::filter::network::MongoProxy& mongo_proxy, FactoryContext& context) {
+
+  if (mongo_proxy.stat_prefix().empty()) {
+    throw MissingFieldException("stat_prefix", mongo_proxy);


@htuch should we include this since this would be covered by validators? My thinking is to remove this check and possibly just have an ASSERT.

We can't run some negative tests without these. Thats the reason for adding these.

But if it's code we don't need why would we add it? I would just put in an assert and remove the tests?

mattklein123 · 2017-10-27T19:49:10Z

source/server/config/network/mongo_proxy.cc

+  if (mongo_proxy.has_delay()) {
+    auto delay = mongo_proxy.delay();
+    if (!delay.has_fixed_delay()) {
+      throw MissingFieldException("delay.fixed_delay", mongo_proxy);


mattklein123 · 2017-10-27T19:49:42Z

source/server/config/network/mongo_proxy.h

 * Config registration for the mongo proxy filter. @see NamedNetworkFilterConfigFactory.
 */
 class MongoProxyFilterConfigFactory : public NamedNetworkFilterConfigFactory {
+  NetworkFilterFactoryCb


if this is meant to be private, should go in private section below public section.

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 · 2017-10-28T17:48:41Z

source/server/config/network/mongo_proxy.cc


-#include "common/json/config_schemas.h"
+#include "common/config/filter_json.h"
+#include "common/json/json_loader.h"


nit: do we need this include now? (I think you just need interface JSON include). If so please fix BUILD file also.

mattklein123 · 2017-10-28T17:49:26Z

source/server/config/network/mongo_proxy.h

 * Config registration for the mongo proxy filter. @see NamedNetworkFilterConfigFactory.
 */
 class MongoProxyFilterConfigFactory : public NamedNetworkFilterConfigFactory {
+


nit: del newline

htuch

LGTM modulo comments.

htuch · 2017-10-29T18:01:12Z

source/common/config/json_utility.h

+
+// Set a google.protobuf.Duration field in a protobuf message with the specified field's
+// milliseconds value from a JSON object if the field is set in the JSON object.
+#define JSON_UTIL_SET_DURATION_FROM_FIELD(json, message, dst_field, src_field)                     \


Nit: probably should define JSON_UTIL_SET_DURATION in terms of this macro now.

can I do this in next PR for redis?

Yes that's fine. Please do in the next PR.

htuch · 2017-10-29T18:05:30Z

test/server/config/network/mongo_proxy_test.cc

  cb(connection);
 }

+TEST(MongoFilterConfigTest, CorrectProtoConfigurationNoFaults) {


Nit: I would rename "correct" to "valid".

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Shriram Rajagopalan added 3 commits October 26, 2017 16:51

first cut

49bac68

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

formatting

85d1d3a

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

nit

40ebeb7

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

htuch reviewed Oct 27, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 27, 2017 10:51

compilation fixes

943804a

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master' into mongo_proxy_v2

17f45ef

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

rshriram changed the title ~~[wip] Converting mongo proxy to v2 api~~ Converting mongo proxy to v2 api Oct 27, 2017

Shriram Rajagopalan added 2 commits October 27, 2017 15:03

proto tests

a6e6b0e

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

formatting

a43e50c

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 reviewed Oct 27, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 27, 2017 17:17

nits

097e4d4

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

remove validation checks

33ecf79

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master' into mongo_proxy_v2

8d66953

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 reviewed Oct 28, 2017

View reviewed changes

htuch reviewed Oct 29, 2017

View reviewed changes

htuch mentioned this pull request Oct 29, 2017

Add access logging to tcp_proxy #1953

Merged

Shriram Rajagopalan added 2 commits October 29, 2017 19:38

nits

b434fa0

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

Merge remote-tracking branch 'upstream/master' into mongo_proxy_v2

7f6f4fa

Signed-off-by: Shriram Rajagopalan <shriram@us.ibm.com>

mattklein123 approved these changes Oct 30, 2017

View reviewed changes

mattklein123 merged commit d5260a5 into envoyproxy:master Oct 30, 2017

Conversation

rshriram commented Oct 26, 2017

Uh oh!

htuch 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!

mattklein123 Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Oct 27, 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!

htuch 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattklein123 Oct 27, 2017 •

edited

Loading