router: allow propagating attempt count in header #4536

snowp · 2018-09-26T18:21:59Z

Adds the ability to configure x-envoy-attempt-count to be set to the attempt count to allow upstreams to infer how many times the request has been retried.

Risk Level: Medium
Testing: Integration test
Docs Changes: Updated the header documentation to include new header
Release Notes: Added release note
Fixes #4210

Signed-off-by: Snow Pettersen <[email protected]>

junr03

thanks! A few comments

junr03 · 2018-09-28T17:21:24Z

api/envoy/api/v2/route/route.proto

@@ -109,6 +109,10 @@ message VirtualHost {
  // specific; see the :ref:`HTTP filter documentation <config_http_filters>`
  // for if and how it is utilized.
  map<string, google.protobuf.Struct> per_filter_config = 12;
+
+  // Decides whether the x-envoy-attempt-count header should be included in upstream


add what the default is.

junr03 · 2018-09-28T17:53:04Z

source/common/router/config_impl.h

@@ -563,6 +568,7 @@ class RouteEntryImplBase : public RouteEntry,
  HeaderParserPtr response_headers_parser_;
  envoy::api::v2::core::Metadata metadata_;
  const bool match_grpc_;
+  const bool include_attempt_count_;


looks to me like this instance variable in the route entry is not actually being used, as we end up getting the setting from the vhost, right?

junr03 · 2018-09-28T17:56:15Z

test/integration/http_integration.cc

@@ -734,6 +734,47 @@ void HttpIntegrationTest::testRetry() {
  EXPECT_EQ(512U, response->body().size());
 }

+// Tests that the x-envoy-attempt-count header is propagated set on the upstream request


nit: is ~~propagated~~ properly set // propagated ~~set~~. Not sure which one you were going for.

junr03 · 2018-09-28T18:13:03Z

test/integration/integration_test.cc

@@ -109,6 +109,8 @@ TEST_P(IntegrationTest, RouterUpstreamResponseBeforeRequestComplete) {

 TEST_P(IntegrationTest, Retry) { testRetry(); }

+TEST_P(IntegrationTest, RetryAttemptCount) { testRetryAttemptCountHeader(); }


I think we are lacking unit test on making sure the configuration is wired up correctly, probably in test/common/router/config_impl_test.cc.

Less clear to me is if we should also have unit testing at the router level in test/common/router/router_test.cc

dio · 2018-10-02T10:38:33Z

docs/root/intro/version_history.rst

@@ -77,6 +77,7 @@ Version history
 * rbac network filter: a :ref:`role-based access control network filter <config_network_filters_rbac>` has been added.
 * rest-api: added ability to set the :ref:`request timeout <envoy_api_field_core.ApiConfigSource.request_timeout>` for REST API requests.
 * router: added ability to set request/response headers at the :ref:`envoy_api_msg_route.Route` level.
+* router: addded ability to set attempt count in upstream requests, see :ref:`envoy_api_field_route.VirtualHost.include_request_attempt_count`.


nit: s/addded/added.

And also this introduces warning,

Warning, treated as error: /../generated/rst/intro/version_history.rst:80:undefined label: envoy_api_field_route.virtualhost.include_request_attempt_count (if the link has no caption the label must precede a section header)

Probably something like the following might work:

* router: added ability to set attempt count in upstream requests, see :ref:`virtual host's include request attempt count flag <envoy_api_field_route.VirtualHost.include_request_attempt_count>`.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2018-10-04T02:27:02Z

@junr03 This is ready for another review

Signed-off-by: Snow Pettersen <[email protected]>

junr03 · 2018-10-05T15:55:39Z

looks good to me @ggreenway can you take a look?

ggreenway · 2018-10-05T16:27:56Z

This is not my area of expertise. @alyssawilk can you take this one?

ggreenway · 2018-10-05T16:28:25Z

@snowp you have merge conflict :(

mattklein123 · 2018-10-05T16:29:49Z

@alyssawilk is out today. I can take it.

mattklein123

Looks great. A few comments. Thank you!

mattklein123 · 2018-10-05T17:50:56Z

api/envoy/api/v2/route/route.proto

@@ -109,6 +109,10 @@ message VirtualHost {
  // specific; see the :ref:`HTTP filter documentation <config_http_filters>`
  // for if and how it is utilized.
  map<string, google.protobuf.Struct> per_filter_config = 12;
+
+  // Decides whether the x-envoy-attempt-count header should be included in upstream


Can you add to https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/router_filter#http-headers-set and cross link?

mattklein123 · 2018-10-05T17:52:39Z

include/envoy/router/router.h

+  /**
+   * @return bool whether x-envoy-attempt-count should be included on the upstream request.
+   */
+  virtual bool includeAttemptCount() const PURE;


I only see configuration for virtual host. Is this used? Do we need route configuration also? If we don't want this now can you comment that this is pulled from the virtual host config currently?

It's used since I plumb through the option from the vhost. I'll add a comment

Re whether we want it on the route configuration: at least for our purposes we want to enable it on per domain/vhost level, so I'd defer per route configuration until somebody needs it

mattklein123 · 2018-10-05T17:53:52Z

source/common/router/router.cc

@@ -287,6 +287,11 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
    headers.removeEnvoyUpstreamRequestTimeoutAltResponse();
  }

+  include_attempt_count_ = route_entry_->includeAttemptCount();
+  if (include_attempt_count_) {
+    headers.insertEnvoyAttemptCount().value(1);


nit: use attempt_count_ here which you initialize to 1?

Signed-off-by: Snow Pettersen <[email protected]>

…ed-count Signed-off-by: Snow Pettersen <[email protected]>

mattklein123

LGTM, small doc issues. Thank you!

mattklein123 · 2018-10-05T21:20:05Z

docs/root/configuration/http_filters/router_filter.rst

@@ -22,6 +22,16 @@ ingress/response path. They are documented in this section.
 .. contents::
  :local:

+.. _config_http_filters_router_x-envoy-attempt-count:


I think this should be in the "set" section below vs. the "consumed" section?

mattklein123 · 2018-10-05T21:20:28Z

api/envoy/api/v2/route/route.proto

-  // Decides whether the x-envoy-attempt-count header should be included in upstream
+  // <config_http_filters_router_x-envoy-original-path>` header.
+  // Decides whether the :ref:`x-envoy-attempt-count
+  // <config_http_filters_router_x-envoy-attempt-count>` header should be included in upstream


nit: s/in/in the

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123

Thank you!

mattklein123 · 2018-10-08T03:30:05Z

@snowp sorry needs a master merge.

lizan · 2018-10-08T05:30:58Z

api/envoy/api/v2/route/route.proto

+  // Decides whether the :ref:`x-envoy-attempt-count
+  // <config_http_filters_router_x-envoy-attempt-count>` header should be included
+  // in the upstream request. Defaults to false.
+  bool include_request_attempt_count = 14;


nit: update next free id in comment above.

lizan · 2018-10-08T05:33:07Z

api/envoy/api/v2/route/route.proto

@@ -109,6 +109,12 @@ message VirtualHost {
  // specific; see the :ref:`HTTP filter documentation <config_http_filters>`
  // for if and how it is utilized.
  map<string, google.protobuf.Struct> per_filter_config = 12;
+
+  // <config_http_filters_router_x-envoy-original-path>` header.


is this related to x-envoy-original-path?

no it's not, that must have snuck it somehow

lizan · 2018-10-08T05:39:37Z

api/envoy/api/v2/route/route.proto

@@ -109,6 +109,12 @@ message VirtualHost {
  // specific; see the :ref:`HTTP filter documentation <config_http_filters>`
  // for if and how it is utilized.
  map<string, google.protobuf.Struct> per_filter_config = 12;
+
+  // <config_http_filters_router_x-envoy-original-path>` header.
+  // Decides whether the :ref:`x-envoy-attempt-count


How this play with suppress_envoy_headers in router.proto?

Right now it doesn't interact with it at all, not sure if it would be more confusing to have two different flags affect whether it's enabled? My take on suppress_envoy_headers is that it's a way to disable headers that are injected by Envoy by default, although I can see how still getting x-envoy-* headers sent with that true (with include_request_attempt_count_header: true) could be confusing. Thoughts?

I'm fine either way, perhaps document the behavior when both are true? @htuch any suggestion?

Signed-off-by: Snow Pettersen <[email protected]>

alyssawilk · 2018-10-08T15:27:20Z

source/common/router/router.cc

@@ -287,6 +287,11 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e
    headers.removeEnvoyUpstreamRequestTimeoutAltResponse();
  }

+  include_attempt_count_ = route_entry_->includeAttemptCount();
+  if (include_attempt_count_) {
+    headers.insertEnvoyAttemptCount().value(attempt_count_);


My one drive by comment is it's worth documenting how this will work when you have 2 levels of Envoys. It's not uncommon in production to have [L1 Envoy][L2 Envoy][upstream]. Granted, it's often a bad idea to have both levels doing retries as more often than not you end up with exponential retries (L2 retries 3 times, fails, L1 retries to new L1, L2 retries 3 times fails, etc) so I don't think it's a bad thing for the last Envoy in the chain to force-overwritten, but I think it's worth calling out it replaces any existing header.

That sounds reasonable, I'll update the documentation

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123

LGTM, thank you!

mattklein123 · 2018-10-08T17:13:01Z

docs/root/intro/version_history.rst

@@ -3,6 +3,8 @@ Version history

 1.9.0 (pending)
 ===============
+* router: added ability to set attempt count in upstream requests, see :ref:`virtual host's include request


nit: alpha order.

Signed-off-by: Snow Pettersen <[email protected]>

snowp · 2018-10-08T21:14:29Z

@mattklein123 ptal

Noted while skimming through envoyproxy#4536. Signed-off-by: Per Lundberg <[email protected]>

Noted while skimming through #4536. Signed-off-by: Per Lundberg <[email protected]>

Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: Aaltan Ahmad <[email protected]>

Noted while skimming through envoyproxy#4536. Signed-off-by: Per Lundberg <[email protected]> Signed-off-by: Aaltan Ahmad <[email protected]>

router: allow propagating attempt count in header

f0ba9c9

Signed-off-by: Snow Pettersen <[email protected]>

snowp force-pushed the attempted-count branch from cecd861 to f0ba9c9 Compare September 26, 2018 18:22

snowp added 3 commits September 26, 2018 14:22

format

c232954

Signed-off-by: Snow Pettersen <[email protected]>

add release note

6b5196c

Signed-off-by: Snow Pettersen <[email protected]>

fix release

748454d

Signed-off-by: Snow Pettersen <[email protected]>

junr03 self-assigned this Sep 27, 2018

junr03 reviewed Sep 28, 2018

View reviewed changes

dio reviewed Oct 2, 2018

View reviewed changes

snowp added 4 commits October 2, 2018 14:36

nits + add unit test for config

a9a0e7e

Signed-off-by: Snow Pettersen <[email protected]>

actually commit nits

b7e7c30

Signed-off-by: Snow Pettersen <[email protected]>

remove non existant field from init list

8c34b6c

Signed-off-by: Snow Pettersen <[email protected]>

kick

089f360

Signed-off-by: Snow Pettersen <[email protected]>

snowp added 2 commits October 4, 2018 21:25

Merge remote-tracking branch 'origin/master' into attempted-count

2d277e2

Signed-off-by: Snow Pettersen <[email protected]>

move release note

5ccf298

Signed-off-by: Snow Pettersen <[email protected]>

junr03 assigned ggreenway Oct 5, 2018

ggreenway assigned alyssawilk and unassigned ggreenway Oct 5, 2018

mattklein123 self-assigned this Oct 5, 2018

Merge branch 'master' into attempted-count

ccba26f

mattklein123 requested changes Oct 5, 2018

View reviewed changes

snowp added 2 commits October 5, 2018 14:35

nits and additioanl documentation

9139c59

Signed-off-by: Snow Pettersen <[email protected]>

Merge branch 'attempted-count' of github.com:snowp/envoy into attempt…

5d04f1e

…ed-count Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 requested changes Oct 5, 2018

View reviewed changes

snowp added 2 commits October 5, 2018 18:11

doc nit + move header documentation

14b863d

Signed-off-by: Snow Pettersen <[email protected]>

format

d969b65

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 previously approved these changes Oct 5, 2018

View reviewed changes

lizan reviewed Oct 8, 2018

View reviewed changes

snowp added 2 commits October 8, 2018 08:25

update next-available and remove original-host comment

dbea290

Signed-off-by: Snow Pettersen <[email protected]>

Merge remote-tracking branch 'origin/master' into attempted-count

2ce5c0e

Signed-off-by: Snow Pettersen <[email protected]>

snowp dismissed mattklein123’s stale review via 2ce5c0e October 8, 2018 12:25

alyssawilk reviewed Oct 8, 2018

View reviewed changes

snowp added 2 commits October 8, 2018 08:35

clarify how setting header interacts with previous header values

265a86d

Signed-off-by: Snow Pettersen <[email protected]>

spelling

94f8890

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 reviewed Oct 8, 2018

View reviewed changes

snowp and others added 4 commits October 8, 2018 11:55

Merge branch 'master' into attempted-count

9956e67

document interaction with suppress_envoy_headers

71d07e5

Signed-off-by: Snow Pettersen <[email protected]>

fix anchor

66d46e7

Signed-off-by: Snow Pettersen <[email protected]>

fix attempt two

8605e5d

Signed-off-by: Snow Pettersen <[email protected]>

mattklein123 approved these changes Oct 8, 2018

View reviewed changes

mattklein123 merged commit fdfa5bd into envoyproxy:master Oct 8, 2018

snowp deleted the attempted-count branch October 8, 2018 21:40

perlun mentioned this pull request Oct 9, 2018

docs: Fix typo #4649

Merged

perlun added a commit to perlun/envoy that referenced this pull request Oct 9, 2018

docs: Fix typo

3d9e092

Noted while skimming through envoyproxy#4536. Signed-off-by: Per Lundberg <[email protected]>

alyssawilk pushed a commit that referenced this pull request Oct 9, 2018

docs: Fix typo (#4649)

11994c7

Noted while skimming through #4536. Signed-off-by: Per Lundberg <[email protected]>

aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018

router: allow propagating attempt count in header (envoyproxy#4536)

a6cc3e5

Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: Aaltan Ahmad <[email protected]>

aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018

docs: Fix typo (envoyproxy#4649)

670eec5

Noted while skimming through envoyproxy#4536. Signed-off-by: Per Lundberg <[email protected]> Signed-off-by: Aaltan Ahmad <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router: allow propagating attempt count in header #4536

router: allow propagating attempt count in header #4536

snowp commented Sep 26, 2018 •

edited

Loading

junr03 left a comment

junr03 Sep 28, 2018

junr03 Sep 28, 2018

junr03 Sep 28, 2018

junr03 Sep 28, 2018

dio Oct 2, 2018

snowp commented Oct 4, 2018

junr03 commented Oct 5, 2018

ggreenway commented Oct 5, 2018

ggreenway commented Oct 5, 2018

mattklein123 commented Oct 5, 2018

mattklein123 left a comment

mattklein123 Oct 5, 2018

mattklein123 Oct 5, 2018

snowp Oct 5, 2018

mattklein123 Oct 5, 2018

mattklein123 left a comment

mattklein123 Oct 5, 2018

mattklein123 Oct 5, 2018

mattklein123 left a comment

mattklein123 commented Oct 8, 2018

lizan Oct 8, 2018

lizan Oct 8, 2018

snowp Oct 8, 2018

lizan Oct 8, 2018

snowp Oct 8, 2018

lizan Oct 8, 2018

alyssawilk Oct 8, 2018

snowp Oct 8, 2018

mattklein123 left a comment

mattklein123 Oct 8, 2018

snowp commented Oct 8, 2018

		@@ -109,6 +109,8 @@ TEST_P(IntegrationTest, RouterUpstreamResponseBeforeRequestComplete) {

		TEST_P(IntegrationTest, Retry) { testRetry(); }

		TEST_P(IntegrationTest, RetryAttemptCount) { testRetryAttemptCountHeader(); }

router: allow propagating attempt count in header #4536

router: allow propagating attempt count in header #4536

Conversation

snowp commented Sep 26, 2018 • edited Loading

junr03 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

snowp commented Oct 4, 2018

junr03 commented Oct 5, 2018

ggreenway commented Oct 5, 2018

ggreenway commented Oct 5, 2018

mattklein123 commented Oct 5, 2018

mattklein123 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

mattklein123 left a comment

Choose a reason for hiding this 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

mattklein123 commented Oct 8, 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp commented Oct 8, 2018

snowp commented Sep 26, 2018 •

edited

Loading