Enable proto schema for router_check_tool by jyotima · Pull Request #6992 · envoyproxy/envoy

jyotima · 2019-05-17T22:14:38Z

Description: Enable proto schema in router check tool to prepare future deprecation of json schema.
Risk Level: Low. Adds a new code path for router check tool.
Testing:

Verified that the existing router check tool path is not manipulated at all.
New code path can only be invoked using the --useproto flag, which is false by default.
Ran the router tests on Lyft tests.
Docs Changes:
Release Notes:

mattklein123 · 2019-05-17T22:27:14Z

Thanks for working on this @jyotima this will be great to have. I will take a pass in a couple of days, but in the interim can you work on fixing DCO, fixing format, and adding tests for the new code? Thank you!

/wait

mattklein123 · 2019-05-17T22:28:21Z

Oh also this new feature will need version history notes and documentation here: https://www.envoyproxy.io/docs/envoy/latest/install/tools/route_table_check_tool. Thank you!

mattklein123

Thank you. Looking good. Flushing out some comments. We still need some tests for the new code here https://github.com/envoyproxy/envoy/tree/master/test/tools/router_check/test. @hennna can you potentially also take a look at this change?

/wait

docs/root/intro/version_history.rst

docs/root/install/tools/route_table_check_tool.rst

test/tools/router_check/validation.proto

test/tools/router_check/router_check.cc

test/tools/router_check/router.cc

docs/root/install/tools/route_table_check_tool.rst

router router Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotima · 2019-05-20T21:29:25Z

Thank you. Looking good. Flushing out some comments. We still need some tests for the new code here https://github.com/envoyproxy/envoy/tree/master/test/tools/router_check/test. @hennna can you potentially also take a look at this change?

/wait

@mattklein123 I have modified route_tests.sh to make sure it exercises the new code path. Essentially, the tests are the same, but use a different schema based on the proto.

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

mattklein123 · 2019-05-20T22:11:57Z

@jyotima will take a look. Friendly request to not force push new commits in the future. It makes your change much more difficult to review. Thanks!

mattklein123 · 2019-05-21T19:31:48Z

Is there a way to avoid this?

You might consider setting up the support hooks or finding some other workflow that you like: https://github.com/envoyproxy/envoy/tree/master/support

mattklein123

Thanks looks good. Few small comments and we can ship!

/wait

docs/root/install/tools/route_table_check_tool.rst

docs/root/intro/version_history.rst

test/tools/router_check/validation.proto

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotima · 2019-05-21T20:51:52Z

@mattklein123
took care of all the comments now.

test/tools/router_check/test/route_tests.sh

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

test/tools/router_check/test/route_tests.sh

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

mattklein123

LGTM, thanks for the great work! One small question.

/wait

mattklein123 · 2019-05-22T02:45:17Z

test/tools/router_check/validation.proto


  // This pseudo-header field includes the HTTP method.
  // The options are GET, PUT, or POST. When not specified the field defaults to GET.
-  string method = 4 [(validate.rules).string = {in: ["GET", "POST", "PUT", ""]}];


I think method should still be required? WDYT?

@mattklein123 Based on existing documentation method is optional

:method (optional, string) The request method. If not specified, the default method is GET. The options are GET, PUT, or POST.

We can make this a breaking change too if it brings more clarity in usage. I think defaulting to GET makes sense.

IMO I would make it required. I think the tests should be very specific so they are easy to visually understand.

agree with the part about making it very explicit for consistency. I will make the change right away.

@mattklein123 I made the change to make method mandatory. I have put min_bytes=3 . I didn't want to put the exhaustive method list there, but all methods have a length of 3. Is it a good constraint to have?

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotimahapatra · 2019-05-22T03:46:10Z

@mattklein123 We are hitting Code coverage 97.4 is lower than limit of 97.5 in the coverage CI task. How should we handle that?

mattklein123

Thanks! Coverage is broken in master. Since you didn't add to the problem I will force merge this. Great work!

jyotimahapatra · 2019-05-22T04:20:35Z

@mattklein123 Thank you so much!!!

* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

jyotimahapatra · 2019-06-19T21:00:31Z

#730

jyotima changed the title ~~Fork~~ Enable proto schema for router_check_tool May 17, 2019

mattklein123 self-assigned this May 17, 2019

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

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

jyotima force-pushed the fork branch 3 times, most recently from 02e1075 to 318a08a Compare May 18, 2019 08:05

mattklein123 requested changes May 19, 2019

View reviewed changes

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

hennna reviewed May 20, 2019

View reviewed changes

jyotima force-pushed the fork branch from 318a08a to 88f2daa Compare May 20, 2019 20:55

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

jyotima force-pushed the fork branch 2 times, most recently from 77a93b6 to 413f59b Compare May 20, 2019 21:06

Jyoti Mahapatra added 8 commits May 20, 2019 14:11

router

7b3d1b6

router router Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

router

f66aa65

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

format

a50a055

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

docs

72cf50b

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

empty

db02ad0

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

format

2e1e995

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

docs

f627341

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

comments

721e00b

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotima force-pushed the fork branch from 413f59b to 721e00b Compare May 20, 2019 21:11

correction

39a7129

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

Jyoti Mahapatra added 2 commits May 20, 2019 15:09

files

ed76eaf

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

files

ca1436d

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

mattklein123 requested changes May 21, 2019

View reviewed changes

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

comments

87685b8

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

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

comments

7303ca9

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

Merge branch 'master' into fork

3e29ad4

hennna suggested changes May 21, 2019

View reviewed changes

test/tools/router_check/test/route_tests.sh Show resolved Hide resolved

hennna previously approved these changes May 21, 2019

View reviewed changes

pb

f072ba5

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotima dismissed hennna’s stale review via f072ba5 May 21, 2019 23:37

jyotima commented May 21, 2019

View reviewed changes

test/tools/router_check/test/route_tests.sh Show resolved Hide resolved

pb

bc15738

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

hennna previously approved these changes May 21, 2019

View reviewed changes

mattklein123 requested changes May 22, 2019

View reviewed changes

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

Jyoti Mahapatra added 2 commits May 21, 2019 20:34

method

02d3a09

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

method

de1aff5

Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

jyotima dismissed hennna’s stale review via de1aff5 May 22, 2019 03:35

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

mattklein123 approved these changes May 22, 2019

View reviewed changes

mattklein123 merged commit cab4ce6 into envoyproxy:master May 22, 2019

jyotima deleted the fork branch May 22, 2019 05:58

jyotima restored the fork branch May 22, 2019 05:59

jyotima deleted the fork branch May 22, 2019 05:59

Conversation

jyotima commented May 17, 2019

Uh oh!

mattklein123 commented May 17, 2019

Uh oh!

mattklein123 commented May 17, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyotima commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented May 20, 2019

Uh oh!

mattklein123 commented May 21, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyotima commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

jyotimahapatra May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 May 22, 2019

Choose a reason for hiding this comment

Uh oh!

jyotimahapatra May 22, 2019

Choose a reason for hiding this comment

Uh oh!

jyotimahapatra May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyotimahapatra commented May 22, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jyotimahapatra commented May 22, 2019

Uh oh!

jyotimahapatra commented Jun 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jyotima commented May 20, 2019 •

edited

Loading

jyotima commented May 21, 2019 •

edited

Loading

jyotimahapatra May 22, 2019 •

edited

Loading

jyotimahapatra May 22, 2019 •

edited

Loading