router: add a route name field in route.Route list by drao9 · Pull Request #6776 · envoyproxy/envoy

drao9 · 2019-05-02T02:24:07Z

Signed-off-by: Divyani Rao dirao@ebay.com

Description: Add a route name field to each route in route.Route list (HTTP route)
Risk Level: Low
Testing: added a test to //test/common/router/config_impl_test.cc and modified some tests in //test/common/router/router_test.cc
Docs Changes: n/a
Release Notes: added a line to version_history.rst
[Optional Fixes #Issue] #6364

zuercher

Thanks for taking this on. I took a quick pass and made some comments, but let's get the compile error fixed before we go further.

/wait

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

source/common/stream_info/stream_info_impl.h

zuercher

Generally looks good. I have one comment related to this change.

Do you plan a follow-up change to allow access to the route name in (for example) access logs or use it in Envoy debug logging?

include/envoy/stream_info/stream_info.h

drao9 · 2019-05-03T18:35:39Z

@zuercher I do plan to follow-up with the access log changes in a separate PR

drao9 · 2019-05-06T20:53:20Z

/retest

repokitteh-read-only · 2019-05-06T20:53:25Z

🔨 rebuilding ci/circleci: mac (failed build)
🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6776 (comment) was created by @drao9.

see: more, trace.

zuercher

I apologize for not being specific enough about the API comments, but I think they need a little more work. Other than it looks good.

include/envoy/router/router.h

include/envoy/stream_info/stream_info.h

drao9 · 2019-05-07T18:00:04Z

@zuercher Thank you so much for your help! I just fixed all the comments.

zuercher

Thanks!

drao9 · 2019-05-08T23:04:45Z

@zuercher The ci/circleci: mac has been failing and it does not seem to be related to these changes. Are you waiting on me to look into this before merging?

mattklein123 · 2019-05-08T23:06:06Z

@drao9 please merge master to pick up CI flake fixes.

/wait

drao9 · 2019-05-09T22:32:57Z

@mattklein123 @zuercher Done with rebasing on master and looks like everything is green! Can this be merged?

mattklein123

Thank you for adding this. A few comments.

/wait

source/common/router/config_impl.h

source/common/http/async_client_impl.h

include/envoy/stream_info/stream_info.h

mattklein123 · 2019-05-09T23:07:58Z

test/common/router/router_test.cc

I don't see any tests in here for actually setting stream info. Can you add them?

@mattklein123 I have added a couple of checks for this. I also made a separate commit to with changes in access logs - this will cover the tests for route name in stream info. Let me know if this works - thanks!

mattklein123

LGTM with 1 comment. Thank you!

/wait

mattklein123 · 2019-05-14T17:28:02Z

source/common/access_log/access_log_formatter.cc

Please add this change to the public docs as well as version history. Also, please make a parallel change to add this to the gRPC access logs.

zuercher · 2019-05-14T17:16:34Z

test/common/access_log/access_log_impl_test.cc

nit: add blank line above

zuercher · 2019-05-14T17:19:06Z

test/common/router/config_impl_test.cc

nit: put the newline back

mattklein123 · 2019-05-19T20:22:34Z

Looks a legit clang-tidy error. @drao9 friendly request to please don't force push. It makes it much harder to review your changes.

/wait

drao9 · 2019-05-20T19:36:56Z

Sorry about that - I will be more careful next time. Will take a look at the clang-tidy error.

mattklein123 · 2019-05-21T19:49:16Z

@drao9 I think you have a messed up merge and also DCO.

/wait

drao9 · 2019-05-21T20:26:48Z

@mattklein123 I sanitized my branch and rebased it against the latest master so the git history looks clean now - however, this means I will have to force push again to my branch. Please let me know if that works or whether you prefer a new PR.

mattklein123 · 2019-05-22T02:27:20Z

Sometimes you gotta force push...

Signed-off-by: Divyani Rao <dirao@ebay.com>

Signed-off-by: Divyani Rao <dirao@ebay.com> Signed-off-by: Divyani Rao <dirao@ebay.com>

Signed-off-by: Divyani Rao <dirao@ebay.com>

mattklein123

Thanks, LGTM. Can you merge master to fix coverage? Thank you!

/wait

Signed-off-by: Divyani Rao <dirao@ebay.com>

* master: test: Add coverage for IsolatedStoreImpl::find* (envoyproxy#7043) server: add ProcessContext (envoyproxy#7018) config: Implement both versions of onConfigUpdate() everywhere (envoyproxy#6879) gzip: add test for various compression strategy and level (envoyproxy#7055) Fix typo in comment for rds.RouteConfiguration.validate_clusters (envoyproxy#7056) mysql_filter: add handling for partial messages (envoyproxy#6885) migrate from v2alpha to v2 (envoyproxy#7044) tests: fix tsan test flake (envoyproxy#7052) upstream: fix HostUtility::healthFlagsToString (envoyproxy#7051) tech debt: eliminate absl::make_unique (envoyproxy#7034) router: add a route name field in route.Route list (envoyproxy#6776) ext_authz: configurable HTTP status code for network errors. (envoyproxy#6669) stats: remove const-cast for symbol-table in edcs_filter_test.cc (envoyproxy#7045) build: bump libevent to 3b1864b. (envoyproxy#7012) stats: improve test-coverage for a few stats-related functions. (envoyproxy#7038) docs: fix csrf filter source origin note (envoyproxy#7041) Fix common typo: grcp -> grpc (envoyproxy#7040) snapshot (envoyproxy#7036) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

zuercher reviewed May 2, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

source/common/stream_info/stream_info_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label May 2, 2019

zuercher self-assigned this May 2, 2019

repokitteh-read-only bot removed the waiting label May 2, 2019

zuercher reviewed May 3, 2019

View reviewed changes

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved

zuercher reviewed May 6, 2019

View reviewed changes

include/envoy/router/router.h Outdated Show resolved Hide resolved

include/envoy/router/router.h Outdated Show resolved Hide resolved

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved

zuercher previously approved these changes May 8, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 8, 2019

drao9 dismissed zuercher’s stale review via b2de45c May 9, 2019 02:03

repokitteh-read-only bot removed the waiting label May 9, 2019

zuercher previously approved these changes May 9, 2019

View reviewed changes

mattklein123 requested changes May 9, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 9, 2019

mattklein123 self-assigned this May 9, 2019

drao9 dismissed zuercher’s stale review via 43e6893 May 13, 2019 23:58

repokitteh-read-only bot removed the waiting label May 13, 2019

mattklein123 requested changes May 14, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 14, 2019

zuercher reviewed May 14, 2019

View reviewed changes

repokitteh-read-only bot removed the waiting label May 18, 2019

repokitteh-read-only bot added the waiting label May 19, 2019

drao9 requested a review from alyssawilk as a code owner May 21, 2019 18:13

repokitteh-read-only bot removed the waiting label May 21, 2019

repokitteh-read-only bot added the waiting label May 21, 2019

Divyani Rao added 9 commits May 21, 2019 19:50

add a route name field in route.Route list

19c49d7

Signed-off-by: Divyani Rao <dirao@ebay.com>

fixing compilation errors and addressing comments

fdca10a

Signed-off-by: Divyani Rao <dirao@ebay.com>

adding comments

11c45f0

Signed-off-by: Divyani Rao <dirao@ebay.com>

fixing comment format

7bdb127

Signed-off-by: Divyani Rao <dirao@ebay.com>

addressing comments & adding checks for StreamInfo

d97ed27

Signed-off-by: Divyani Rao <dirao@ebay.com>

adding access log changes

57bb834

Signed-off-by: Divyani Rao <dirao@ebay.com>

format fix changes

06b503a

Signed-off-by: Divyani Rao <dirao@ebay.com>

route name in gRPC access logger

1c5dae1

Signed-off-by: Divyani Rao <dirao@ebay.com> Signed-off-by: Divyani Rao <dirao@ebay.com>

fixing clang-tidy error

cb11c75

Signed-off-by: Divyani Rao <dirao@ebay.com>

repokitteh-read-only bot removed the waiting label May 22, 2019

mattklein123 approved these changes May 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 22, 2019

Merge branch 'upstream-master' into latest_route_name

c6fec15

Signed-off-by: Divyani Rao <dirao@ebay.com>

repokitteh-read-only bot removed the waiting label May 22, 2019

mattklein123 merged commit 46012ce into envoyproxy:master May 23, 2019

rshriram mentioned this pull request Aug 8, 2019

add names to http routes istio/istio#16138

Closed

derekargueta mentioned this pull request Aug 27, 2019

Add a route name field to each route in route.Route list (HTTP route) #6364

Open

Conversation

drao9 commented May 2, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drao9 commented May 3, 2019

Uh oh!

drao9 commented May 6, 2019

Uh oh!

repokitteh-read-only bot commented May 6, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drao9 commented May 7, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

drao9 commented May 8, 2019

Uh oh!

mattklein123 commented May 8, 2019

Uh oh!

drao9 commented May 9, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 May 9, 2019

Choose a reason for hiding this comment

Uh oh!

drao9 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

zuercher May 14, 2019

Choose a reason for hiding this comment

Uh oh!

zuercher May 14, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 19, 2019

Uh oh!

drao9 commented May 20, 2019

Uh oh!

mattklein123 commented May 21, 2019

Uh oh!

drao9 commented May 21, 2019

Uh oh!

mattklein123 commented May 22, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development