Support api config source without cluster names #2999

ambuc · 2018-04-04T19:47:29Z

To support the deprecation of cluster_names in the api_config_source (#2860), I

added a more verbose error in utility.cc for GRPC api_config_sources with any named clusters
hunted down places in tests where api_config_source.cluster_names()[0] was implicitly used (assuming that API would be GRPC and would have cluster names).
renamed factoryForApiConfigSource to factoryForGrpcApiConfigSource, as it already implicitly returns a Grpc::AsyncClientFactoryPtr, and gave a more verbose error for when the user passes a config with cluster_names set.
separated out checkApiConfigSourceSubscriptionBackingCluster into checkApiConfigSourceNames, which does the gRPC services v. cluster names validation, and checkApiConfigSourceSubscriptionBackingCluster, which actually validates the cluster name against the clusters map.
more completely covered the tests for the branching cases for non-gRPC API configs which happened to have grpc services defined.

Risk Level: Low
Testing: bazel test test/... all passed.

Fixed #2680
Fixed #2902

Signed-off-by: James Buckland jbuckland@google.com

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc · 2018-04-04T20:01:11Z

test/integration/header_integration_test.cc

@@ -5,6 +5,7 @@
 #include "common/config/resources.h"
 #include "common/protobuf/protobuf.h"

+#include "test/integration/fake_upstream.h"


Oops, I'll remove this. I was at one point attempting to mock an upstream target_uri.

ambuc · 2018-04-04T20:02:01Z

test/common/config/subscription_factory_test.cc

@@ -190,18 +188,26 @@ INSTANTIATE_TEST_CASE_P(SubscriptionFactoryTestApiConfigSource,
                                          envoy::api::v2::core::ApiConfigSource::REST,
                                          envoy::api::v2::core::ApiConfigSource::GRPC));

+// TODO(jbuckland) add test case for ApiConfigSource::GRPC


Oops, I'll remove this. I had at one point removed envoy::api::v2::core::ApiConfigSource::GRPC from the values block above.

htuch · 2018-04-04T20:02:29Z

source/common/config/utility.cc

-        "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
-  }
+  if (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC) {
+    if (api_config_source.cluster_names().size() != 0) {


We should also throw an exception if grpc_services().size() != 1.

htuch · 2018-04-04T20:05:11Z

source/common/config/utility.cc

-        "envoy::api::v2::core::ConfigSource must have a statically "
-        "defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
-        cluster_name));
+    const auto& cluster_name = api_config_source.cluster_names()[0];


I see what you meant here. You should have a condition here; if it's api_type() == GRPC && api_config_soures.grpc_service()[0].has_envoy_grpc() then you can verify api_Config.grpc_Services()[0].envou_grpc().cluster_name().

htuch · 2018-04-04T20:08:10Z

test/common/config/subscription_factory_test.cc

-                            "non-EDS cluster: 'eds_cluster' "
-                            "does not exist, was added via api, or is an EDS cluster");
+  if (api_config_source->api_type() == envoy::api::v2::core::ApiConfigSource::GRPC) {
+    EXPECT_THROW_WITH_MESSAGE(


I think for subscription_factory_test, we should go in and add additional tests to cover all the branches possible now in checkApiConfigSourceSubscriptionBackingCluster

Done in d149e7f.

Signed-off-by: James Buckland <jbuckland@google.com>

…urceSubscriptionBackingCluster Signed-off-by: James Buckland <jbuckland@google.com>

…ps://github.com/ambuc/envoy into support-api_config_source-without-cluster_names Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…g_source-without-cluster_names Signed-off-by: James Buckland <jbuckland@google.com>

…ster_names Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

htuch · 2018-04-05T18:42:03Z

source/common/config/utility.cc

-    // TODO(htuch): Add support for multiple clusters, #1170.
-    throw EnvoyException(
-        "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
+  bool is_grpc = (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);


Nit: const bool

htuch · 2018-04-05T18:42:37Z

source/common/config/utility.cc

+    }
+    if (api_config_source.grpc_services().size() != 1) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource::GRPC must have a singleton grpc service specified");


Nit: either "singleton grpc_services" or "single gRPC service".

htuch · 2018-04-05T18:42:51Z

source/common/config/utility.cc

+  } else {
+    if (api_config_source.grpc_services().size() != 0) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource must not have a grpc service specified");


htuch · 2018-04-05T18:43:36Z

source/common/config/utility.cc

  }

-  const auto& cluster_name = api_config_source.cluster_names()[0];
+  const std::string& cluster_name =
+      is_grpc ? api_config_source.grpc_services()[0].envoy_grpc().cluster_name()


There needs to be another condition here on has_envoy_grpc(). Some gRPC services will be configured with google_grpc and won't have a cluster name.

htuch · 2018-04-05T18:44:51Z

source/common/config/utility.cc

  }
+  grpc_service.MergeFrom(api_config_source.grpc_services(0));


You can skip this line now and pass grpc_service(0) directly into the below call to the factory method.

htuch · 2018-04-05T18:46:08Z

source/common/config/utility.cc

+
+  if (api_config_source.grpc_services().empty()) {
+    throw EnvoyException(
+        fmt::format("Missing gRPC services in envoy::api::v2::core::ApiConfigSource: {}",


Seems we have the same log messages as in checkApiConfigSourceSubscriptionBackingCluster. Can we refactor? Alternatively, can we expect checkApiConfigSourceSubscriptionBackingCluster to always be called before we get here and just put in some ASSERTs?

I don't think so. in source/common/upstream/cluster_manager_impl.cc, we run factoryFor... to reset load_stats_reporter_, but we don't have a list of clusters to validate against. I could split up checkApiConfig... into two bits, the first of which checks cluster_names() vs. grpc_services() (no list of clusters necessary), and the second of which does the actual validation. Does that sound like the right solution?

htuch · 2018-04-05T18:49:00Z

test/common/config/subscription_factory_test.cc

+      "bar");
+  cluster_map.emplace("bar", cluster);
+  envoy::api::v2::core::GrpcService expected_grpc_service_bar;
+  expected_grpc_service_bar.mutable_envoy_grpc()->set_cluster_name("bar");


How is this used?

Good catch, this is a stub from when I was using EXPECT_CALL(cm_.async_client_manager_, factoryForGrpcService(ProtoEq(expected_grpc_service)...)) in all the test cases. I'll get rid of it here...

htuch · 2018-04-05T18:49:05Z

test/common/config/subscription_factory_test.cc

+      "foo");
+  cluster_map.emplace("foo", cluster);
+  envoy::api::v2::core::GrpcService expected_grpc_service_foo;
+  expected_grpc_service_foo.mutable_envoy_grpc()->set_cluster_name("foo");


How is this used?

... and here.

htuch · 2018-04-05T18:49:30Z

test/common/config/subscription_factory_test.cc

  Upstream::ClusterManager::ClusterInfoMap cluster_map;
  Upstream::MockCluster cluster;
-  cluster_map.emplace("eds_cluster", cluster);
+  cluster_map.emplace("foo", cluster);


Not sure if this lends clarity, seems a gratuitous rename.

This isn't actually an eds cluster by default -- because it's a mock object, it defaults to the first enum in envoy::api::v2::Cluster::, which is STATIC. https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/cds.proto#L67

I'd make it static_cluster then for clarity.

htuch · 2018-04-05T18:50:39Z

test/common/config/subscription_factory_test.cc

+  if (api_config_source->api_type() == envoy::api::v2::core::ApiConfigSource::GRPC) {
+    EXPECT_THROW_WITH_MESSAGE(
+        subscriptionFromConfigSource(config)->start({"foo"}, callbacks_), EnvoyException,
+        "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified");


I think we want to setup this test to actually get the below behavior, by using grpc_services.envoy_grpc.cluster_name to specify cluster name in the gRPC case.

I'll refactor all the SubscriptionFactoryTestApiConfigSource/* tests to better fit this.

Signed-off-by: James Buckland <jbuckland@google.com>

…n-statically defined error instead Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…iptionBackingCluster into checkApiConfigSourceNames and use it to validate inside factoryForGrpcApiConfigSource And various assorted testing changes to make this possible Signed-off-by: James Buckland <jbuckland@google.com>

htuch

LGTM modulo nits and the missing coverage case. This is looking like a really nice cleanup.

htuch · 2018-04-06T16:15:28Z

source/common/config/utility.cc

+  const bool is_grpc =
+      (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
+
+  // we ought to validate the cluster name if and only if there is a cluster name.


Super tiny nit: Prefer having comment sentences begin with capital letters (here and below).

htuch · 2018-04-06T16:16:47Z

source/common/config/utility.h

@@ -121,6 +121,13 @@ class Utility {
   */
  static void checkFilesystemSubscriptionBackingPath(const std::string& path);

+  /**
+   * Check the grpc_services and cluster_names for API config sanity. Throws on error.


You can use Doxygeen @throw syntax here, grep around for examples.

Done in 421a420.

htuch · 2018-04-06T16:21:04Z

test/common/config/subscription_factory_test.cc

+  EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map));
+  EXPECT_THROW_WITH_MESSAGE(
+      subscriptionFromConfigSource(config), EnvoyException,
+      "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");


I don't think you need to change anything now that we have these tests, but I would probably approach testing by doing full branch coverage in a dedicated unit test for Utility::checkApiConfigSourceNames() and then just do specific point tests in SubscriptionFactoryTest for where behavior is more complex. I think what you have is fine though.

This is a good catch. The coverage report actually shows a hole in utility.cc for a REST API with defined gRPC services. I won't refactor this entirely but I am going to add coverage for that case.

Done in bb137e7.

…rpc services Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

htuch · 2018-04-06T17:25:02Z

One more pro tip. You can add "Fixed #2680" and "Fixes #2902" to the commit message at the top to close out the issues automatically when this merges.

htuch

Rad, this is a really great cleanup.

)" This reverts commit 01e75f9. Discuession on #envoy-users revealed that this is going to break operationally for folks who are trying to roll forward binaries with the same config, there's no config that can work both before/after. Signed-off-by: Harvey Tuch <htuch@google.com>

This reverts commit 01e75f9. Discuession on #envoy-users revealed that this is going to break operationally for folks who are trying to roll forward binaries with the same config, there's no config that can work both before/after. Signed-off-by: Harvey Tuch <htuch@google.com>

…yproxy#2999)" (envoyproxy#3018)" This reverts commit fdcbe10. Signed-off-by: James Buckland <jbuckland@google.com>

This reverts #3018, which reverted #2999. Therefore this PR is similar to #2999, except that 77b292c allows REST/gRPC config mismatch errors to warn instead of throwing an error. Hopefully this solves the issue for users who were unable to roll forward their binaries while keeping the same config. Changelist for #2999: To support the deprecation of cluster_names in the api_config_source (#2860), I added a more verbose error in utility.cc for GRPC api_config_sources with any named clusters hunted down places in tests where api_config_source.cluster_names()[0] was implicitly used (assuming that API would be GRPC and would have cluster names). renamed factoryForApiConfigSource to factoryForGrpcApiConfigSource, as it already implicitly returns a Grpc::AsyncClientFactoryPtr, and gave a more verbose error for when the user passes a config with cluster_names set. separated out checkApiConfigSourceSubscriptionBackingCluster into checkApiConfigSourceNames, which does the gRPC services v. cluster names validation, and - checkApiConfigSourceSubscriptionBackingCluster, which actually validates the cluster name against the clusters map. more completely covered the tests for the branching cases for non-gRPC API configs which happened to have grpc services defined. Risk Level: Low Testing: bazel test test/... all passed. Fixed #2680 Fixed #2902 Signed-off-by: James Buckland <jbuckland@google.com>

htuch self-assigned this Apr 4, 2018

Support api_config_source without cluster_names

6cafac9

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc force-pushed the support-api_config_source-without-cluster_names branch from 8f7fc1b to 6cafac9 Compare April 4, 2018 19:53

ambuc commented Apr 4, 2018

View reviewed changes

htuch suggested changes Apr 4, 2018

View reviewed changes

ambuc added 8 commits April 5, 2018 13:17

Support api_config_source without cluster_names

7b3a6fb

Signed-off-by: James Buckland <jbuckland@google.com>

Rename eds_cluster to foo, since the mock object is unopinionated

3c0761e

Signed-off-by: James Buckland <jbuckland@google.com>

Add additional tests to explicitly cover branches in checkApiConfigSo…

d149e7f

…urceSubscriptionBackingCluster Signed-off-by: James Buckland <jbuckland@google.com>

Merge branch 'support-api_config_source-without-cluster_names' of htt…

5aba580

…ps://github.com/ambuc/envoy into support-api_config_source-without-cluster_names Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

06f27b2

Signed-off-by: James Buckland <jbuckland@google.com>

Merge remote-tracking branch 'upstream/master' into support-api_confi…

89b93a0

…g_source-without-cluster_names Signed-off-by: James Buckland <jbuckland@google.com>

Merge branch 'saverebased' into support-api_config_source-without-clu…

904b284

…ster_names Signed-off-by: James Buckland <jbuckland@google.com>

Remove unnecessary include

1add00e

Signed-off-by: James Buckland <jbuckland@google.com>

htuch suggested changes Apr 5, 2018

View reviewed changes

ambuc added 7 commits April 5, 2018 16:51

Clean up language around gRPC services

db08d97

Signed-off-by: James Buckland <jbuckland@google.com>

Handle google_grpc condition for cluster_name validation

5c18c68

Signed-off-by: James Buckland <jbuckland@google.com>

Remove remnants of earlier abandoned work

d023dc5

Signed-off-by: James Buckland <jbuckland@google.com>

Assorted nits

6bcd4ef

Signed-off-by: James Buckland <jbuckland@google.com>

Refactor SubscriptionFactoryTestApiConfigSource tests to throw the no…

01d2d41

…n-statically defined error instead Signed-off-by: James Buckland <jbuckland@google.com>

Rename foo to static_cluster for clarity

ef1ebfa

Signed-off-by: James Buckland <jbuckland@google.com>

htuch reviewed Apr 6, 2018

View reviewed changes

ambuc added 2 commits April 6, 2018 13:06

More explicit coverage for exceptions for non-GRPC API configs with g…

bb137e7

…rpc services Signed-off-by: James Buckland <jbuckland@google.com>

Documentation style fixes

421a420

Signed-off-by: James Buckland <jbuckland@google.com>

htuch approved these changes Apr 6, 2018

View reviewed changes

htuch merged commit 01e75f9 into envoyproxy:master Apr 6, 2018

htuch mentioned this pull request Apr 6, 2018

Support api_config_source without cluster_names #2860

Closed

ambuc deleted the support-api_config_source-without-cluster_names branch April 6, 2018 19:56

ambuc added a commit to ambuc/envoy that referenced this pull request Apr 9, 2018

Revert "Revert "Support api config source without cluster names (envo…

fa72e61

…yproxy#2999)" (envoyproxy#3018)" This reverts commit fdcbe10. Signed-off-by: James Buckland <jbuckland@google.com>

ambuc mentioned this pull request Apr 9, 2018

Support api config source without cluster names #3030

Merged

ambuc mentioned this pull request Apr 12, 2018

Support api config source without cluster names #3067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support api config source without cluster names #2999

Support api config source without cluster names #2999

ambuc commented Apr 4, 2018 •

edited

Loading

ambuc Apr 4, 2018

ambuc Apr 4, 2018

htuch Apr 4, 2018

htuch Apr 4, 2018

htuch Apr 4, 2018

ambuc Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

ambuc Apr 5, 2018

htuch Apr 5, 2018

htuch Apr 5, 2018

ambuc Apr 5, 2018

htuch Apr 5, 2018

ambuc Apr 5, 2018

htuch Apr 5, 2018

ambuc Apr 5, 2018 •

edited

Loading

htuch Apr 5, 2018

htuch Apr 5, 2018

ambuc Apr 5, 2018

htuch left a comment

htuch Apr 6, 2018

htuch Apr 6, 2018

ambuc Apr 6, 2018

htuch Apr 6, 2018

ambuc Apr 6, 2018

ambuc Apr 6, 2018

htuch commented Apr 6, 2018

htuch left a comment

		}
		grpc_service.MergeFrom(api_config_source.grpc_services(0));

Support api config source without cluster names #2999

Support api config source without cluster names #2999

Conversation

ambuc commented Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambuc Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Apr 6, 2018

htuch left a comment

Choose a reason for hiding this comment

ambuc commented Apr 4, 2018 •

edited

Loading

ambuc Apr 5, 2018 •

edited

Loading