Skip to content

Set content-type in router_check_tool#10183

Closed
kb000 wants to merge 23 commits intoenvoyproxy:masterfrom
kb000:dev/kb000/router_check_contenttype
Closed

Set content-type in router_check_tool#10183
kb000 wants to merge 23 commits intoenvoyproxy:masterfrom
kb000:dev/kb000/router_check_contenttype

Conversation

@kb000
Copy link
Copy Markdown
Contributor

@kb000 kb000 commented Feb 27, 2020

Description:
Set the content-type header in router_check_tool to better match what the real router does.

Risk Level: Low
Testing: Modified existing test, added negative test.
Docs Changes: None
Release Notes: None
Fixes: #10072

@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype branch from f011156 to f823749 Compare February 27, 2020 00:51
@dio dio assigned jyotimahapatra and unassigned jyotimahapatra Feb 27, 2020
@dio
Copy link
Copy Markdown
Member

dio commented Feb 27, 2020

cc. @LisaLudique

LisaLudique
LisaLudique suggested changes Feb 27, 2020
View reviewed changes
@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype branch from 0a8dfb4 to 35c9de5 Compare February 27, 2020 23:16
@kb000
setReferenceContentType in router check tool
15cf2eb 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype branch from 35c9de5 to 7ffc27a Compare February 28, 2020 01:16
kb000 added 3 commits February 27, 2020 17:18
@kb000
Headers only on nonempty response body
75614e0 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
use sendLocalReply in router_check_tool
1cda91e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
rebase
550376e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype branch from 7ffc27a to 550376e Compare February 28, 2020 01:18
@kb000
format
a2963b2 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype branch from 70db9a9 to a2963b2 Compare February 28, 2020 19:10
@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Feb 28, 2020

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Feb 28, 2020

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #10183 (comment) was created by @kb000.

see: more, trace.

LisaLudique
LisaLudique previously approved these changes Feb 28, 2020
View reviewed changes
mattklein123
mattklein123 reviewed Feb 28, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Thanks, LGTM w/ 1 question. Nice work!

/wait-any

test/tools/router_check/test/config/DirectResponse.golden.proto.json Outdated
"response_header_fields": [
{
"key": "content-type",
"value": ""
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Feb 28, 2020

Choose a reason for hiding this comment

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

Is this goal here to test for absence? It would be a much stronger test if we had an explicit way of testing for this?

Copy link
Copy Markdown
Contributor Author

@kb000 kb000 Mar 3, 2020

Choose a reason for hiding this comment

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

Yes the goal is to test for absence of the content-type header.
Looking at https://tools.ietf.org/html/rfc7230#section-3.2.6, it is possible for field values to be implicitly empty strings (whitespace (trimmed) + CRLF), or explicitly double-quoted empty strings.
It would additionally be stronger from a human readable perspective to be able to say, for instance:

validate:
  response_header_fields:
    - key: content-type
      absent: true

Copy link
Copy Markdown
Contributor Author

@kb000 kb000 Mar 3, 2020

Choose a reason for hiding this comment

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

I filed #10229 for future improvement.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Mar 3, 2020

Choose a reason for hiding this comment

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

Let's please just fix this now. Sorry I should have commented on this in @LisaLudique's last PR, but since we just switched the proto to split request/response headers for matching, we should use the HeaderMatcher proto that we already have which makes this type of matching easily possible. Thank you!

Copy link
Copy Markdown
Contributor Author

@kb000 kb000 Mar 3, 2020

Choose a reason for hiding this comment

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

I don't like the scope creep, but ok.
What's the policy on proto changes? Do I need to rename the fields and reserve the old field numbers or am I ok to change it inline because there wasn't a version cut in between?

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Mar 3, 2020

Choose a reason for hiding this comment

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

We are being a bit loose on this tool, so I think it's OK to just change it since we just changed it already within the last week. I would talk to @LisaLudique about it.

@mattklein123 mattklein123 self-assigned this Feb 28, 2020
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Mar 3, 2020

@kb000 thanks for working on this. One small comment about using header match. Additionally, please do not force push as it makes reviews more difficult. Just add commits and merge master. Thank you!

/wait

kb000 and others added 13 commits March 6, 2020 15:21
@kb000
Update router_check.rst
0822718
@kb000
route checker tool: differentiate between request and response headers (
a25e108 
envoyproxy#10168)

Signed-off-by: Lisa Lu <lisalu@lyft.com>
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
setReferenceContentType in router check tool
36a608e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Headers only on nonempty response body
b231b02 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
use sendLocalReply in router_check_tool
f84dd2b 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
rebase
78793c3 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
format
6fbe434 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
request_header_matches
6bb707b 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
HeaderMatcher working with copy/pasted code
0ec95fe 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
a little templating
9076fc2 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
a little more templating
c19803e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
format
04141d8 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
merge doc edits
15fef11
@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Mar 6, 2020

Merge commits and the signoff tool have completely destroyed this branch. I'll create a new PR on monday.

@kb000 kb000 closed this Mar 6, 2020
@kb000 kb000 mentioned this pull request Aug 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

+1 more reviewer

@LisaLudique LisaLudique LisaLudique left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

router_check_tool can't see response_headers_to_add

5 participants

@kb000 @dio @mattklein123 @LisaLudique @jyotimahapatra