implement rds api by mattklein123 · Pull Request #464 · envoyproxy/envoy

mattklein123 · 2017-02-10T23:49:15Z

fixes #298
fixes #463

fixes #298

mattklein123 · 2017-02-10T23:49:47Z

@lyft/network-team @rshriram @GregHanson

ccaraman · 2017-02-11T00:57:10Z

docs/configuration/http_conn_man/rds.rst

+
+route_config_name
+  *(required, string)* The name of the route table. This name will be passed to the
+  :ref:`RDS HTTP API <config_http_conn_man_rds_api>` so that different route tables can be used by


What does it mean to be used by independent filters?

I think you meant independent listeners?

ccaraman · 2017-02-11T00:58:38Z

include/envoy/router/rds.h

+
+  /**
+   * @return Router::ConfigPtr a route configuration for use during a single request. The returned
+   *         config may be different on a subsequent call so a new config should be acquired for


comma before so

ccaraman · 2017-02-11T01:05:03Z

source/common/json/config_schemas.cc

      "generate_request_id" : {"type" : "boolean"}
    },
-    "required" : ["codec_type", "stat_prefix", "route_config", "filters"],
+    "required" : ["codec_type", "stat_prefix", "filters"],


I think you should keep route_config. At line 222, it only checks for an object, so empty will pass. And the documentation above makes it seem like it should always be there.

@ccaraman if that line in the documentation is removed, it aligns well with the rest of the checks in rds_impl.cc. WDYT?

It's not required anymore. I'm fixing the docs.

rshriram

Some general doc comments.
Also need a note saying that existing connections to older routes will continue to have same retry/timeout semantics as they were specified during connection initiation.

Given that the combination of RDS/CDS and SDS is (almost) effectively a hot reload with new config, do any of the drain timeout semantics apply? Most probably they don't, but it is worth mentioning in doc, as one liner. WDYT?

rshriram · 2017-02-11T17:22:36Z

docs/configuration/http_conn_man/rds.rst

+=======================
+
+The route discovery service (RDS) API is an optional API that Envoy will call to dynamically fetch
+route tables. Each :ref:`HTTP connection manager filter <config_http_conn_man>` can independently


s/route tables/HTTP route entries/ for clarity ?

Or even better: dynamically fetch configurations for virtual hosts and routes within the virtual host.

rshriram · 2017-02-11T17:26:04Z

docs/configuration/http_conn_man/rds.rst

+
+route_config_name
+  *(required, string)* The name of the route table. This name will be passed to the
+  :ref:`RDS HTTP API <config_http_conn_man_rds_api>` so that different route tables can be used by


I think you meant independent listeners?

rshriram · 2017-02-11T17:41:52Z

docs/configuration/http_conn_man/http_conn_man.rst

+  *(sometimes required, object)* The connection manager configuration must specify one of *rds* or
+  *route_config*. If *route_config* is specified, the :ref:`route table <arch_overview_http_routing>`
+  for the connection manager is static and is specified in this property. All connection managers must have
+  a route table, even if it is empty.


I would remove this line. It sounds like if you use rds, you still need a dummy empty route_config block

rshriram · 2017-02-11T17:45:21Z

source/common/json/config_schemas.cc

      "generate_request_id" : {"type" : "boolean"}
    },
-    "required" : ["codec_type", "stat_prefix", "route_config", "filters"],
+    "required" : ["codec_type", "stat_prefix", "filters"],


@ccaraman if that line in the documentation is removed, it aligns well with the rest of the checks in rds_impl.cc. WDYT?

rshriram · 2017-02-11T17:50:25Z

source/common/router/rds_impl.cc

+  log_debug("rds: parsing response");
+  Json::ObjectPtr response_json = Json::Factory::LoadFromString(response.bodyAsString());
+  uint64_t new_hash = response_json->hash();
+  if (new_hash != last_config_hash_ || !initialized_) {


I think this warrants a comment somewhere in the docs. Something like The new/old routes will be compared based on their hashes. If they differ, then the newer route will be loaded. So care must be taken by the RDS provider to ensure that the order of routes in a RDS response remains unchanged across API calls.
WDYT ?
Most probably, the same applies to CDS as well?

I will add some detail in the docs. It's just a performance optimization for the route config to now reload internally, so I don't think it's worth guiding developers to not reorder things.

Sorry, yeah you are right. It was more for istio, where we are making sure that the config remains as deterministic as possible between every refresh.

rshriram · 2017-02-11T17:56:08Z

source/server/http/admin.h


+  /**
+   * Implementation of RouteConfigProvider that returns a static null route config.
+   */


s/RouteConfigProvider/NullRouteConfigProvider/ ?

mattklein123 · 2017-02-11T22:37:05Z

@ccaraman @rshriram updated from comments. Also, I opened #468 to do a full overview of all of the configuration options now available. I will do this as an independent change.

This makes all of the REST API configs consistent. fixes #463

rshriram

Thanks @mattklein123 . LGTM. We will start testing this soon

ccaraman · 2017-02-13T18:28:15Z

docs/configuration/network_filters/client_ssl_auth_filter.rst

      "auth_api_cluster": "...",
      "stat_prefix": "...",
-      "refresh_interval_ms": "...",
+      "refresh_delay_ms": "...",


I just realized that CLIENT_SSL_NETWORK_FILTER_SCHEMA doesn't have refresh_delay_ms. Can you please add that?

junr03 · 2017-02-13T18:33:59Z

docs/configuration/http_conn_man/rds.rst

+
+.. http:get:: /v1/routes/(string: route_config_name)/(string: service_cluster)/(string: service_node)
+
+Asks the discovery service to return the route configuration for a particular `route_config_name`,


Asks the route dicovery service... People might get confused.

ccaraman · 2017-02-13T19:22:20Z

test/common/router/config_impl_test.cc

+  NiceMock<Upstream::MockClusterManager> cm;
+  EXPECT_CALL(cm, get("www2")).WillRepeatedly(Return(nullptr));
+
+  ConfigImpl(*loader, runtime, cm, false);


nit: add a EXPECT_NO_THROW

Support Grpc initial metadata in Wasm.

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Commit Message** This removes the unnecessary fields from the expected output of translation test case. **Related Issues/PRs (if applicable)** Contributes to #412 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

implement rds api

b3c48b3

fixes #298

fix

6ad8955

ccaraman reviewed Feb 11, 2017

View reviewed changes

rshriram reviewed Feb 11, 2017

View reviewed changes

mattklein123 added 2 commits February 11, 2017 14:11

Merge branch 'master' into rds

c8ec260

comments

d2f8aba

config: rename refresh_interval_ms -> refresh_delay_ms

77b9d36

This makes all of the REST API configs consistent. fixes #463

rshriram approved these changes Feb 12, 2017

View reviewed changes

comment

1765042

ccaraman reviewed Feb 13, 2017

View reviewed changes

mattklein123 added 2 commits February 13, 2017 10:31

Merge remote-tracking branch 'origin/master' into rds

acf3a58

comment

6063d35

junr03 reviewed Feb 13, 2017

View reviewed changes

comment

133dc1c

ccaraman reviewed Feb 13, 2017

View reviewed changes

ccaraman approved these changes Feb 13, 2017

View reviewed changes

mattklein123 merged commit a1aceb8 into master Feb 13, 2017

mattklein123 deleted the rds branch February 13, 2017 20:19

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

rename target to destination (envoyproxy#464)

892d76c

lizan pushed a commit to lizan/envoy that referenced this pull request Apr 24, 2020

Merge pull request envoyproxy#464 from jplevyak/grpc-metadata

80d36d2

Support Grpc initial metadata in Wasm.

wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021

zh-translation: /configuration/overview/versioning.rst (envoyproxy#464)

6cba02d

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

dispatcher: explicitly null-initialize pointers (#464)

7722700

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

dispatcher: explicitly null-initialize pointers (#464)

d845dfd

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>


		.. http:get:: /v1/routes/(string: route_config_name)/(string: service_cluster)/(string: service_node)

		Asks the discovery service to return the route configuration for a particular `route_config_name`,

Conversation

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

Uh oh!

Uh oh!

mattklein123 commented Feb 10, 2017

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!

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

mattklein123 commented Feb 11, 2017

Uh oh!

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

4 participants

mattklein123 commented Feb 10, 2017 •

edited

Loading