router: correct cluster name for ads in routes #2449

ramaraochavali · 2018-01-25T11:37:01Z

Signed-off-by: Rama [email protected]
Fix Cluster name for ADS configured RDS in routes

Description:
When admin/routes displays the route table, configured with RDS, it shows the cluster name as "NOT_USING_CLUSTER". Fixed it to display "ADS_CLUSTER"

Risk Level: Low
Testing: Manual
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-25T11:39:01Z

source/common/router/rds_impl.cc

@@ -75,6 +75,8 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl(
  // then there is no actual RDS server, and hence no RDS cluster name.
  if (rds.has_config_source() && rds.config_source().has_api_config_source()) {
    cluster_name_ = rds.config_source().api_config_source().cluster_names()[0];
+  } else if (rds.has_config_source() && rds.config_source().has_ads()) {
+    cluster_name_ = "ADS_CLUSTER";


I tried to get the actual ads cluster name, but looks like it requires a bigger change, like adding ads cluster name while building the ClusterManager and have to introduce a public method in ClusterManager which may be overkill for this. Please let me know if you would like me to do it or if there is a simpler way to get the actual ads cluster name.

This is actually pretty tricky for a few reasons:

We're migrating envoy.api.v2.ApiConfigSource (by which ADS) is configured away from using raw cluster names to envoy.api.v2.GrpcService.

The ADS GrpcService might either use a cluster name (when using Envoy gRPC) or a raw address (when using Google gRPC). There is a stat_prefix in the latter case, which could be used for reporting, but I think this would be confusing semantics here.

I think including cluster name in the admin console is not the way to go long term. What we should do perhaps is include a JSON serialization of rds.config_source().api_config_source(), or elide it completely.

@junr03 can probably comment further on the original intention of this field and what it might be used for in a post-cluster centric view of gRPC endpoints.

Will defer to @junr03 on the final solution here, but in general I think we need to move to a model where we just store the config in proto form and are able to spit it back out. See also #2421

The original purpose of that field was to be able to give a sense to someone operating the admin interface where the RDS config came from, given that the configuration might have come from an LDS response.

In general, #2421 and #2172 show that there is a need to be able to understand what is currently loaded in envoy and where it came from. There are a couple segmented pieces here and there but a comprehensive solution would be great.

As far as this particular change goes, I am ok with it in order to make the output of the /routes admin endpoint correct and more clear for now.

What about just dumping the full rds.config_source.api_config_source() proto instead of cluster name? It seems this would kill multiple birds with one stone.

Sorry, in my comment above I meant it to mean your suggestion of dumping rds.config_source().api_config_source() -- I elided a sentence. However, I would do rds.config_source() so all three sources are covered by default?

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-26T06:30:31Z

source/common/router/rds_impl.h

@@ -130,7 +130,7 @@ class RdsRouteConfigProviderImpl
  Upstream::ClusterManager& cm_;
  std::unique_ptr<Envoy::Config::Subscription<envoy::api::v2::RouteConfiguration>> subscription_;
  ThreadLocal::SlotPtr tls_;
-  std::string cluster_name_;
+  std::string cluster_config_;


@junr03 i have changed this variable name and method name to mean what we are doing here. let me know if you want to use any better names

mattklein123 · 2018-01-26T16:51:58Z

@ramaraochavali this LGTM in general. Can you provide an example of the admin endpoint output from curl? I'm mostly wonder if it looks OK or if we should try to do some pretty printing. (I can't recall whether we have that capability or not easily in the proto conversion path). cc @junr03 @htuch

htuch · 2018-01-26T19:52:20Z

source/common/router/rds_impl.cc

  } else {
-    cluster_name_ = "NOT_USING_CLUSTER";
+    cluster_config_ = "NOT_USING_CLUSTER";
  }
 }


This whole if statement should now just be:

if (rds.has_config_source()) { cluster_source_ = MessageUtil::getJsonStringFromMessage(rds.config_source()); }

htuch · 2018-01-26T19:53:07Z

include/envoy/router/rds.h

   */
-  virtual const std::string& clusterName() const PURE;
+  virtual const std::string& clusterConfig() const PURE;


Rename to configSource(), the cluster aspect is no longer true in general, this could be a non-cluster destination GrpcService.

htuch

I like the improvement happening here, thanks @ramaraochavali!

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-27T05:56:41Z

@htuch changed it to configSource also changed the text in /routes to config_source . PTAL and let me if any further changes are required.

ramaraochavali · 2018-01-27T05:57:05Z

@mattklein123 Here is the curl output

[
{
"version_info": "1",
"route_config_name": "ingress_http2_route",
"config_source": {"ads":{}},
"route_table_dump": {"name":"ingress_http2_route","virtual_hosts":[{"name":"ingress_http2_route","domains":["*"],"routes":[{"match":{"prefix":"/"},"route":{"cluster":"local_service_grpc","retry_policy":{"retry_on":"5xx,connect-failure,refused-stream","num_retries":3}}}]}]}
}
,{
"version_info": "1",
"route_config_name": "egress_http2_route",
"config_source": {"ads":{}},
"route_table_dump": {"name":"egress_http2_route","virtual_hosts":[{"name":"egress_http2_route","domains":["*"],"routes":[{"match":{"prefix":"/"},"route":{"cluster_header":":authority","retry_policy":{"retry_on":"5xx,connect-failure,refused-stream,cancelled,deadline-exceeded,resource-exhausted","num_retries":3}}}]}]}
}
,{
"version_info": "1",
"route_config_name": "ingress_http1_route",
"config_source": {"ads":{}},
"route_table_dump": {"name":"ingress_http1_route","virtual_hosts":[{"name":"ingress_http1_route","domains":["*"],"routes":[{"match":{"prefix":"/"},"route":{"cluster":"local_service","retry_policy":{"retry_on":"5xx,connect-failure,refused-stream","num_retries":3}}}]}]}
}
]

Signed-off-by: Rama <[email protected]>

mattklein123 · 2018-01-27T17:45:21Z

OK I guess it's no worse than the route table dump output. In the future I wonder if we should pretty print, they I suppose the output can easily be piped through a JSON formatter.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-28T13:57:23Z

@mattklein123 implemented the pretty print support for JSON. Here is the output of /routes with. can you PTAL at the review?

[
{
"version_info": "1",
"route_config_name": "ingress_http2_route",
"config_source": {
 "ads": {}
}
,
"route_table_dump": {
 "name": "ingress_http2_route",
 "virtual_hosts": [
  {
   "name": "ingress_http2_route",
   "domains": [
    "*"
   ],
   "routes": [
    {
     "match": {
      "prefix": "/"
     },
     "route": {
      "cluster": "local_service_grpc",
      "retry_policy": {
       "retry_on": "5xx,connect-failure,refused-stream",
       "num_retries": 3
      }
     }
    }
   ]
  }
 ]
}

}
,{
"version_info": "1",
"route_config_name": "egress_http2_route",
"config_source": {
 "ads": {}
}
,
"route_table_dump": {
 "name": "egress_http2_route",
 "virtual_hosts": [
  {
   "name": "egress_http2_route",
   "domains": [
    "*"
   ],
   "routes": [
    {
     "match": {
      "prefix": "/"
     },
     "route": {
      "cluster_header": ":authority",
      "retry_policy": {
       "retry_on": "5xx,connect-failure,refused-stream,cancelled,deadline-exceeded,resource-exhausted",
       "num_retries": 3
      }
     }
    }
   ]
  }
 ]
}

}
,{
"version_info": "1",
"route_config_name": "ingress_http1_route",
"config_source": {
 "ads": {}
}
,
"route_table_dump": {
 "name": "ingress_http1_route",
 "virtual_hosts": [
  {
   "name": "ingress_http1_route",
   "domains": [
    "*"
   ],
   "routes": [
    {
     "match": {
      "prefix": "/"
     },
     "route": {
      "cluster": "local_service",
      "retry_policy": {
       "retry_on": "5xx,connect-failure,refused-stream",
       "num_retries": 3
      }
     }
    }
   ]
  }
 ]
}

}
]

ramaraochavali · 2018-01-28T14:36:43Z

@mattklein123 once we agree on the format and code changes I will make changes to the rds_impl test to adhere to this format

htuch

LGTM modulo minor rename.

htuch · 2018-01-28T18:00:03Z

source/common/router/rds_impl.cc

  } else {
-    cluster_name_ = "NOT_USING_CLUSTER";
+    config_source_ = "NOT_USING_CLUSTER";


Change this to NO_CONFIG_SOURCE.

mattklein123

Awesome! LGTM other than small comments.

mattklein123 · 2018-01-28T19:44:55Z

source/common/protobuf/utility.h

+   * @return std::string of formatted JSON object.
+   */
+  static std::string getJsonStringFromMessage(const Protobuf::Message& message,
+                                              const bool pretty_print);


nit: I would just add pretty_print to the other definition and default it to false, then just remove this version.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-29T05:20:33Z

@mattklein123 @htuch addressed the minor comments. Fixed the test case to validate against the new format. Should be good to go. let me know if any thing else is missing on this.

mattklein123

very nice, thanks. Tiny nit.

mattklein123 · 2018-01-29T17:56:20Z

include/envoy/router/rds.h

@@ -51,10 +51,10 @@ class RdsRouteConfigProvider : public RouteConfigProvider {
  virtual const std::string& routeConfigName() const PURE;

  /**
-   * @return const std::string& the name of the cluster the RdsRouteConfigProvider is issuing RDS
-   * requests to.
+   * @return const std::string& the configuration of service the RdsRouteConfigProvider is


tiny nit: "the service"

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-30T03:44:36Z

@mattklein123 changed. PTAL.

htuch

Sorry, just one more thing..

htuch · 2018-01-30T03:45:48Z

source/common/protobuf/utility.h

   */
-  static std::string getJsonStringFromMessage(const Protobuf::Message& message);
+  static std::string getJsonStringFromMessage(const Protobuf::Message& message,
+                                              const bool pretty_print = false);


Nit: const on method declarations doesn't do anything (it's OK in the definition though).

htuch · 2018-01-30T03:46:57Z

source/common/router/rds_impl.cc

  } else {
-    cluster_name_ = "NOT_USING_CLUSTER";
+    config_source_ = "NO_CONFIG_SOURCE";


Should this actually be an empty dictionary {} on second thoughts; JSON parsing will barf on this won't it?

ramaraochavali · 2018-01-30T04:00:41Z

@htuch made those changes..PTAL.

ramaraochavali · 2018-01-30T04:03:52Z

@htuch wait a min. looks like some problem. will correct that in a bit.

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-01-30T07:33:04Z

@mattklein123 @htuch should be good to go now.PTAL.

mattklein123 · 2018-01-30T18:42:49Z

source/common/router/rds_impl.cc

  } else {
-    cluster_name_ = "NOT_USING_CLUSTER";
+    config_source_ = "{}";


Sorry quick question: Is there a test that should have caught this (switching to {})? Is it easy to add one?

@mattklein123 there is no test. Will look in to adding it. One more thing, /routes is not working for routes defined statically. It is only iterating through the RdsRouteConfigProviders. https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L215. Also it does not store the StaticRouteConfigProviders any where. Is this intentional or a bug? @htuch your thoughts?

and the else block changing config_source = {}, I think we can get rid of the else block completely because if rds is configured, with out config_source, EnvoyException will be thrown. And that is validated with this test https://github.com/envoyproxy/envoy/blob/master/test/common/router/rds_impl_test.cc#L126.

Also it does not store the StaticRouteConfigProviders any where. Is this intentional or a bug?

It just wasn't implemented. In the context of #2421 we will want to revisit this.

and the else block changing config_source = {}, I think we can get rid of the else block completely because if rds is configured, with out config_source, EnvoyException will be thrown.

Yup, great catch. If the code can't be hit just delete it and remove the if statement entirely. Thanks!

Signed-off-by: Rama <[email protected]>

ramaraochavali · 2018-02-01T05:18:41Z

@mattklein123 fixed. removed the comment as well as it is confusing there. PTAL. So /routes was implemented recently and for rds only?

junr03 · 2018-02-01T06:21:43Z

@ramaraochavali I can give context. yes /routes was implemented only for RDS given that you could get a dump of the static routes by looking at your config file. As Matt mentioned in the issue above. We would like to unify all of these disparate outputs into a more comprehensive solution.

mattklein123

Thank you! Very nice.

This file is often skipped when updating Envoy SHA in WORKSPACE. Signed-off-by: Piotr Sikora <[email protected]>

Signed-off-by: GitHub Action <[email protected]> Co-authored-by: jpsim <[email protected]> Signed-off-by: JP Simard <[email protected]>

correct cluster name for ads in routes

82527a1

Signed-off-by: Rama <[email protected]>

ramaraochavali commented Jan 25, 2018

View reviewed changes

htuch assigned junr03 and htuch Jan 25, 2018

ramaraochavali added 2 commits January 26, 2018 10:33

dumped the proto

de59b03

Signed-off-by: Rama <[email protected]>

changed variable and method names, fixed tests

faf0d8c

Signed-off-by: Rama <[email protected]>

ramaraochavali commented Jan 26, 2018

View reviewed changes

htuch reviewed Jan 26, 2018

View reviewed changes

changed cluster_config to config_source

ccec8ed

Signed-off-by: Rama <[email protected]>

fixed tests

6af94cb

Signed-off-by: Rama <[email protected]>

added pretty print support for protobuf json

6bc4587

Signed-off-by: Rama <[email protected]>

htuch previously approved these changes Jan 28, 2018

View reviewed changes

mattklein123 reviewed Jan 28, 2018

View reviewed changes

changed rds_impl_test.cc to adhere to new formatted print

195f256

Signed-off-by: Rama <[email protected]>

ramaraochavali dismissed htuch’s stale review via 195f256 January 29, 2018 04:34

mattklein123 reviewed Jan 29, 2018

View reviewed changes

changed the doc

a4df1fb

Signed-off-by: Rama <[email protected]>

htuch reviewed Jan 30, 2018

View reviewed changes

htuch previously approved these changes Jan 30, 2018

View reviewed changes

ramaraochavali dismissed htuch’s stale review via 5a6aadb January 30, 2018 04:40

ramaraochavali added 2 commits January 30, 2018 11:19

added DCO signoff

b53faa9

Signed-off-by: Rama <[email protected]>

added DCO signoff

8b8078d

Signed-off-by: Rama <[email protected]>

ramaraochavali force-pushed the fix/rds-cluster-name branch from 5a6aadb to 8b8078d Compare January 30, 2018 05:51

ramaraochavali changed the title ~~correct cluster name for ads in routes~~ router: correct cluster name for ads in routes Jan 30, 2018

htuch previously approved these changes Jan 30, 2018

View reviewed changes

mattklein123 reviewed Jan 30, 2018

View reviewed changes

mattklein123 mentioned this pull request Jan 31, 2018

Improve legibility of admin pages by indenting JSON. #2492

Closed

removed the unnecessary if and else part

67dc1cf

Signed-off-by: Rama <[email protected]>

ramaraochavali dismissed htuch’s stale review via 67dc1cf February 1, 2018 04:53

mattklein123 approved these changes Feb 1, 2018

View reviewed changes

mattklein123 merged commit 2bce55f into envoyproxy:master Feb 1, 2018

ramaraochavali deleted the fix/rds-cluster-name branch February 2, 2018 05:13

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Sync .bazelrc with upstream. (envoyproxy#2449)

3601310

This file is often skipped when updating Envoy SHA in WORKSPACE. Signed-off-by: Piotr Sikora <[email protected]>

jpsim added a commit that referenced this pull request Nov 28, 2022

Update Envoy (#2449)

4733884

Signed-off-by: GitHub Action <[email protected]> Co-authored-by: jpsim <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim added a commit that referenced this pull request Nov 29, 2022

Update Envoy (#2449)

13c97c9

Signed-off-by: GitHub Action <[email protected]> Co-authored-by: jpsim <[email protected]> Signed-off-by: JP Simard <[email protected]>

router: correct cluster name for ads in routes #2449

router: correct cluster name for ads in routes #2449

Conversation

ramaraochavali commented Jan 25, 2018

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

mattklein123 commented Jan 26, 2018

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

ramaraochavali commented Jan 27, 2018

ramaraochavali commented Jan 27, 2018

mattklein123 commented Jan 27, 2018

ramaraochavali commented Jan 28, 2018

ramaraochavali commented Jan 28, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Jan 29, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Jan 30, 2018

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

ramaraochavali commented Jan 30, 2018

ramaraochavali commented Jan 30, 2018

ramaraochavali commented Jan 30, 2018

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

ramaraochavali commented Feb 1, 2018

junr03 commented Feb 1, 2018

mattklein123 left a comment

Choose a reason for hiding this comment