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

Jump to bottom

router: add x-envoy-attempt-count on downstream responses #10325

Merged
merged 20 commits into from
Mar 16, 2020
Merged

router: add x-envoy-attempt-count on downstream responses #10325

merged 20 commits into from
Mar 16, 2020

Conversation

junr03
Copy link
Member

@junr03 junr03 commented Mar 10, 2020 •
edited
Loading

Description: this PR adds the x-envoy-attempt-count header to downstream responses.
Risk Level: low, used via new config value that defaults to false.
Testing: updated tests to verify that the header is being set. New unit tests
Docs Changes: updated docs
Release Notes: added.

Signed-off-by: Jose Nino jnino@lyft.com

Jose Nino added 5 commits March 10, 2020 15:36
router: add x-envoy-attemp-count on downstream responses
1b5b280 
Signed-off-by: Jose Nino <jnino@lyft.com>
spelling
dba0039 
Signed-off-by: Jose Nino <jnino@lyft.com>
missed version
e8daae8 
Signed-off-by: Jose Nino <jnino@lyft.com>
docs
eb6b28e 
Signed-off-by: Jose Nino <jnino@lyft.com>
docs
eaaabea 
Signed-off-by: Jose Nino <jnino@lyft.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10325 was opened by junr03.

see: more, trace.

@junr03
Copy link
Member Author

junr03 commented Mar 10, 2020

I made some decisions about this change that are open for debate:

  1. The presence of the header is controlled by the same config value as the presence of the header in upstream requests. I could see us wanting two different config settings.
  2. The presence of the header is not guarded by suppress envoy headers to mirror the behavior for upstream requests
  3. The header is only added when Envoy actually receives a response from upstream. Although I could see us wanting to add the header for local replies to let the client know how many attempts were made before the local reply happened.

docs
87dbfd1 
Signed-off-by: Jose Nino <jnino@lyft.com>
alyssawilk
alyssawilk reviewed Mar 11, 2020
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Cool, it's nice to have better debug info for what's going on in this case!

source/common/router/router.cc Outdated Show resolved Hide resolved
@junr03 junr03 assigned htuch and alyssawilk and unassigned snowp and mattklein123 Mar 11, 2020
Jose Nino added 9 commits March 11, 2020 13:04
add new field to proto
ef0f925 
Signed-off-by: Jose Nino <jnino@lyft.com>
rename functions/vars to include 'request'
bd86b9f 
Signed-off-by: Jose Nino <jnino@lyft.com>
add attempt count to stream info
78186f3 
Signed-off-by: Jose Nino <jnino@lyft.com>
add config setting in vhost and route
edb76dc 
Signed-off-by: Jose Nino <jnino@lyft.com>
Revert "add attempt count to stream info"
fd45d77 
This reverts commit 78186f3.

Signed-off-by: Jose Nino <jnino@lyft.com>
update
9386ac0 
Signed-off-by: Jose Nino <jnino@lyft.com>
name change
25f183f 
Signed-off-by: Jose Nino <jnino@lyft.com>
fmt
f406804 
Signed-off-by: Jose Nino <jnino@lyft.com>
tag rename
bfb08e8 
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Mar 12, 2020

@htuch this is ready for another pass (going to leave alyssa out from mentions given she is ooo today).

@junr03 junr03 mentioned this pull request Mar 12, 2020
Closed
alyssawilk
alyssawilk reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Looking good overall!

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved
source/common/router/router.cc Show resolved Hide resolved
test/common/router/router_test.cc Outdated Show resolved Hide resolved
@junr03 junr03 mentioned this pull request Mar 13, 2020
Merged
Jose Nino added 2 commits March 13, 2020 12:17
comments
7b7222e 
Signed-off-by: Jose Nino <jnino@lyft.com>
Merge branch 'master' into attempt-count
cd7f929 
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Mar 13, 2020

@alyssawilk updated!

@htuch htuch mentioned this pull request Mar 13, 2020
Closed
Jose Nino added 3 commits March 13, 2020 16:33
move field
5ae3a12 
Signed-off-by: Jose Nino <jnino@lyft.com>
Merge branch 'master' into attempt-count
fc4cc78 
Signed-off-by: Jose Nino <jnino@lyft.com>
move
43de1e2 
Signed-off-by: Jose Nino <jnino@lyft.com>
@mattklein123 mattklein123 self-assigned this Mar 16, 2020
@htuch
Copy link
Member

htuch commented Mar 16, 2020

/lgtm api

mattklein123
mattklein123 approved these changes Mar 16, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

api/envoy/api/v2/route/route_components.proto
@@ -143,8 +143,20 @@ message VirtualHost {
// This header is unaffected by the
// :ref:`suppress_envoy_headers
// <envoy_api_field_config.filter.http.router.v2.Router.suppress_envoy_headers>` flag.
//
// [#next-major-version: rename to include_attempt_count_in_request.]
Copy link
Member

Choose a reason for hiding this comment

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

You can do this now with [(udpa.annotations.field_migrate).rename right? Or is the issue that this will right now change it in v3 and we have to circle back and do it after we stop v2 updates?

Copy link
Member Author

Choose a reason for hiding this comment

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

there's a wrinkle with the annotation right now #10325 (comment). I am keeping a note so that when this is fix we can change to use the annotation.

include/envoy/http/header_map.h
@@ -344,6 +343,7 @@ class HeaderEntry {
HEADER_FUNC(Connection) \
HEADER_FUNC(ContentLength) \
HEADER_FUNC(ContentType) \
HEADER_FUNC(EnvoyAttemptCount) \
Copy link
Member

Choose a reason for hiding this comment

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

It makes me happy to see this working properly. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is awesome! Hard/tedious work is now paying off :) thanks for doing this. I took the liberty to narrow some more types in this PR to ensure static checking, really rad.

@junr03
Copy link
Member Author

junr03 commented Mar 16, 2020

I am going to go ahead an merge this PR, in spite of the macOS failure. The same failure has been seen on master as well https://github.com/envoyproxy/envoy/runs/509585590

cc @danzh2010

@junr03 junr03 merged commit 1db15db into envoyproxy:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowp snowp snowp left review comments

@htuch htuch htuch left review comments

@alyssawilk alyssawilk alyssawilk left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

@htuch htuch

@alyssawilk alyssawilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@junr03 @htuch @snowp @mattklein123 @alyssawilk