Support api config source without cluster names by ambuc · Pull Request #3030 · envoyproxy/envoy

ambuc · 2018-04-09T16:40:47Z

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

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

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

htuch · 2018-04-09T17:23:30Z

source/common/config/utility.cc

+    }
+  } else {
+    if (api_config_source.grpc_services().size() != 0) {
+      ENVOY_LOG_MISC(warn, "envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "


This should still throw, non-gRPC services should never have been configured this way.

I'll roll this case and its test back to throwing an error.

htuch · 2018-04-09T17:24:06Z

source/common/config/utility.cc

+  if (is_grpc) {
+    // Some ApiConfigSources of type GRPC won't have a cluster name, such as if
+    // they've been configured with google_grpc.
+    if (api_config_source.grpc_services()[0].has_envoy_grpc()) {


You'll need to deal with the case where cluster_names.size() == 1 here and there are no gRPC services.

I think this case gets caught when we call Utility::checkApiConfigSourceNames(api_config_source) at the top of this function.

Addressed in a34526e.

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

htuch

Looks good, but just one missing test case and related implementation issue.

htuch · 2018-04-09T22:05:28Z

source/common/config/utility.cc

+  if (is_grpc) {
+    if (api_config_source.cluster_names().size() != 0) {
+      ENVOY_LOG_MISC(
+          warn, "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified");


Change this to "Setting a cluster name for API config source type envoy::api::v2::core::ConfigSource::GRPC is deprecated".

htuch · 2018-04-09T22:06:10Z

source/common/config/utility.cc

+      ENVOY_LOG_MISC(
+          warn, "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified");
+    }
+    if (api_config_source.grpc_services().size() != 1) {


Shouldn't this be an else if (how come the tests didn't catch this?).

I think this shouldn't be an else if. The first block checks for non-zero cluster_names, which would be bad because setting a cluster name for a gRPC is deprecated. This second block independently checks for non-singleton grpc_services, which are unsupported.

You'll need to change the check to > 1 then, to support when grpc_services.size() == 0 and they are using deprecated cluster_names.

Oh, nice catch. We're sort of overloading !=1 here to catch both empty grpc_services (deprecated) and >1 grpc_services (not supported).

htuch · 2018-04-09T22:09:56Z

test/common/config/utility_test.cc

+    // clusters.
+    api_config_source->add_grpc_services();
  }
+  api_config_source->add_cluster_names("foo_cluster");


You'll probably want another standalone test for the case where cluster_names.size() != 0 for gRPC but grpc_services.size() == 0.

Reworked this in d62249f.

… REST and gRPC APIs Signed-off-by: James Buckland <jbuckland@google.com>

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

htuch · 2018-04-11T00:14:13Z

source/common/config/utility.cc

+      ENVOY_LOG_MISC(warn, "Setting a cluster name for API config source type "
+                           "envoy::api::v2::core::ConfigSource::GRPC is deprecated");
+    }
+    if (api_config_source.cluster_names().size() > 1) {


I think this one can be dropped, the above warning is sufficient, unless this makes testing easier somehow.

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

htuch · 2018-04-11T20:21:42Z

LGTM. One last thing, can you update DEPRECATED.md, since 1.7.0 is when we do the real removal?

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

This reverts commit b8e2eee. Signed-off-by: Matt Klein <mklein@lyft.com>

…yproxy#3030)"" This reverts commit f0a7c4e. Signed-off-by: James Buckland <jbuckland@google.com>

ambuc added 2 commits April 9, 2018 10:08

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>

Warn instead of throw on REST/gRPC config mismatches

77b292c

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

htuch suggested changes Apr 9, 2018

View reviewed changes

ambuc added 2 commits April 9, 2018 13:46

Throw on gRPC API configs without any gRPC services

b751815

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

Refactor utility_test.cc to better test a range of API config types

a34526e

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

htuch suggested changes Apr 9, 2018

View reviewed changes

htuch self-assigned this Apr 9, 2018

ambuc added 3 commits April 10, 2018 09:28

Refactor utility_test.cc to better test API config mismatches between…

d62249f

… REST and gRPC APIs Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

c917128

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

Better tests for API configs of type gRPC with cluster names

fdcc8b3

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

htuch previously approved these changes Apr 11, 2018

View reviewed changes

Remove duplicate warn

a5f573e

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

ambuc dismissed htuch’s stale review via a5f573e April 11, 2018 16:33

Documentation change

0cd4ab0

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

Modify DEPRECATED.md to reflect API config warnings

d511a59

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

htuch approved these changes Apr 11, 2018

View reviewed changes

htuch merged commit b8e2eee into envoyproxy:master Apr 11, 2018

mattklein123 added a commit that referenced this pull request Apr 12, 2018

Revert "Support api config source without cluster names (#3030)"

f0a7c4e

This reverts commit b8e2eee. Signed-off-by: Matt Klein <mklein@lyft.com>

ambuc deleted the fix-support_api_config_source branch April 12, 2018 17:38

mattklein123 added a commit that referenced this pull request Apr 12, 2018

Revert "Support api config source without cluster names (#3030)" (#3063)

8b4c2d6

This reverts commit b8e2eee. Signed-off-by: Matt Klein <mklein@lyft.com>

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

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

e0161a2

…yproxy#3030)"" This reverts commit f0a7c4e. Signed-off-by: James Buckland <jbuckland@google.com>

ambuc mentioned this pull request Apr 12, 2018

Support api config source without cluster names #3067

Merged

Conversation

ambuc commented Apr 9, 2018

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!

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 commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

htuch commented Apr 11, 2018 •

edited

Loading