Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: allow propagating attempt count in header #4536

Merged
merged 23 commits into from
Oct 8, 2018
Merged

router: allow propagating attempt count in header #4536

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f0ba9c9
router: allow propagating attempt count in header
snowp Sep 26, 2018
c232954
format
snowp Sep 26, 2018
6b5196c
add release note
snowp Sep 26, 2018
748454d
fix release
snowp Sep 26, 2018
a9a0e7e
nits + add unit test for config
snowp Oct 2, 2018
b7e7c30
actually commit nits
snowp Oct 2, 2018
8c34b6c
remove non existant field from init list
snowp Oct 2, 2018
089f360
kick
snowp Oct 2, 2018
2d277e2
Merge remote-tracking branch 'origin/master' into attempted-count
snowp Oct 5, 2018
5ccf298
move release note
snowp Oct 5, 2018
ccba26f
Merge branch 'master' into attempted-count
snowp Oct 5, 2018
9139c59
nits and additioanl documentation
snowp Oct 5, 2018
5d04f1e
Merge branch 'attempted-count' of github.com:snowp/envoy into attempt…
snowp Oct 5, 2018
14b863d
doc nit + move header documentation
snowp Oct 5, 2018
d969b65
format
snowp Oct 5, 2018
dbea290
update next-available and remove original-host comment
snowp Oct 8, 2018
2ce5c0e
Merge remote-tracking branch 'origin/master' into attempted-count
snowp Oct 8, 2018
265a86d
clarify how setting header interacts with previous header values
snowp Oct 8, 2018
94f8890
spelling
snowp Oct 8, 2018
9956e67
Merge branch 'master' into attempted-count
snowp Oct 8, 2018
71d07e5
document interaction with suppress_envoy_headers
snowp Oct 8, 2018
66d46e7
fix anchor
snowp Oct 8, 2018
8605e5d
fix attempt two
snowp Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/envoy/api/v2/route/route.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ message VirtualHost {
// 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
// <config_http_filters_router_x-envoy-original-path>` header.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Decides whether the :ref:`x-envoy-attempt-count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this play with suppress_envoy_headers in router.proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// <config_http_filters_router_x-envoy-attempt-count>` header should be included in upstream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/in/in the

// request. Defaults to false.
bool include_request_attempt_count = 14;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update next free id in comment above.

}
Expand Down
10 changes: 10 additions & 0 deletions docs/root/configuration/http_filters/router_filter.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ ingress/response path. They are documented in this section.
.. contents::
:local:

.. _config_http_filters_router_x-envoy-attempt-count:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


x-envoy-attempt-count
^^^^^^^^^^^^^^^^^^^^^

Sent to the upstream to indicate which attempt the current request is in a series of retries. The value
will be "1" on the initial request, incremeting by one for each retry. Only set if the
:ref:`include_attempt_count_header <envoy_api_field_route.VirtualHost.include_request_attempt_count>`
flag is set to true.

.. _config_http_filters_router_x-envoy-max-retries:

x-envoy-max-retries
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/router/router.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ class RouteEntry : public ResponseEntry {
};

/**
* True if the virtual host this RouteEntry belongs to is configured to include the attempt
* count header.
* @return bool whether x-envoy-attempt-count should be included on the upstream request.
*/
virtual bool includeAttemptCount() const PURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
2 changes: 1 addition & 1 deletion source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

include_attempt_count_ = route_entry_->includeAttemptCount();
if (include_attempt_count_) {
headers.insertEnvoyAttemptCount().value(1);
headers.insertEnvoyAttemptCount().value(attempt_count_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable, I'll update the documentation

}

route_entry_->finalizeRequestHeaders(headers, callbacks_->streamInfo(),
Expand Down