Skip to content

Router Check Tool HeaderMatcher and sendLocalReply#12810

Merged
mattklein123 merged 36 commits intoenvoyproxy:masterfrom
kb000:dev/kb000/router_check_contenttype_2
Sep 5, 2020
Merged

Router Check Tool HeaderMatcher and sendLocalReply#12810
mattklein123 merged 36 commits intoenvoyproxy:masterfrom
kb000:dev/kb000/router_check_contenttype_2

Conversation

@kb000
Copy link
Copy Markdown
Contributor

@kb000 kb000 commented Aug 25, 2020 •
edited
Loading

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

Additional Description:
This is a re-do of #10183

Risk Level: Low
Testing: Modified existing test, added negative test.
Docs Changes: Noted deprecation of request_header_fields, response_header_fields, addition of request_header_matches, response_header_matches.
Fixes: #10072, #10229

kb000 added 3 commits August 25, 2020 11:43
@kb000
HeaderMatcher in router check tool
655dc9f 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Exercise SendLocalReply in router check tool
0ec2d35 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
lint
6460b50 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
lint .rst
fecf2e5 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000 kb000 force-pushed the dev/kb000/router_check_contenttype_2 branch from 88ebfdc to fecf2e5 Compare August 25, 2020 22:54
kb000 added 3 commits August 25, 2020 16:10
@kb000
Fix .rst indent
8a66232 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Fix logic errors in present_match
1107791 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
clang_tidy
eba29e6 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000 kb000 marked this pull request as ready for review August 26, 2020 07:08
kb000 added 2 commits August 26, 2020 00:14
@kb000
Describe invert_match
e07cff3 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
clang_tidy again
16ab0ec 
Signed-off-by: Kevin Burek <kburek@lyft.com>
dio
dio reviewed Aug 26, 2020
View reviewed changes
kb000 added 2 commits August 26, 2020 13:28
@kb000
Soft-deprecate request_header_fields and response_header_fields
6d9cc95 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
format
b7c60b4 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Aug 27, 2020

/meow

@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Aug 27, 2020

@dio Do you have any advice on how to keep the deprecated fields in the unit tests but not fail the CI build?

type envoy.RouterCheckToolSchema.ValidationAssert Using deprecated option 'envoy.RouterCheckToolSchema.ValidationAssert.request_header_fields' from file validation.proto.

https://dev.azure.com/cncf/envoy/_build/results?buildId=49507&view=logs&j=ec9f720e-f904-5f65-4ba4-f371a013344f&t=ee47c280-92b1-5c80-3a10-d3d23a58a062&l=4864

@kb000
Remove deprecated fields from router_check_tool unit tests.
eb898b6 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@dio
Copy link
Copy Markdown
Member

dio commented Aug 28, 2020

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Aug 28, 2020

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12810 (comment) was created by @dio.

see: more, trace.

@dio
Copy link
Copy Markdown
Member

dio commented Aug 28, 2020 •
edited
Loading

Our CI, @rbe_ubuntu_clang_libcxx//config:platform fails to compile the following (I'm not sure either since it is fine in my local, cc. @lizan):

test/tools/router_check/router.cc:172:67: error: no viable conversion from 'const (lambda at test/tools/router_check/router.cc:162:32)' to 'std::function<void (ResponseHeaderMap &, Code &, std::string &, absl::string_view &)>' (aka 'function<void (Envoy::Http::ResponseHeaderMap &, Envoy::Http::Code &, basic_string<char, char_traits<char>, allocator<char> > &, basic_string_view<char> &)>')
  Envoy::Http::Utility::EncodeFunctions encode_functions{nullptr, encode_headers, encode_data};
                                                                  ^~~~~~~~~~~~~~
/opt/llvm/bin/../include/c++/v1/functional:2283:5: note: candidate constructor not viable: no known conversion from 'const (lambda at test/tools/router_check/router.cc:162:32)' to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument
    function(nullptr_t) _NOEXCEPT {}
    ^
/opt/llvm/bin/../include/c++/v1/functional:2284:5: note: candidate constructor not viable: no known conversion from 'const (lambda at test/tools/router_check/router.cc:162:32)' to 'const std::__1::function<void (Envoy::Http::ResponseHeaderMap &, Envoy::Http::Code &, std::__1::basic_string<char> &, std::__1::basic_string_view<char, std::__1::char_traits<char> > &)> &' for 1st argument
    function(const function&);
    ^
/opt/llvm/bin/../include/c++/v1/functional:2285:5: note: candidate constructor not viable: no known conversion from 'const (lambda at test/tools/router_check/router.cc:162:32)' to 'std::__1::function<void (Envoy::Http::ResponseHeaderMap &, Envoy::Http::Code &, std::__1::basic_string<char> &, std::__1::basic_string_view<char, std::__1::char_traits<char> > &)> &&' for 1st argument
    function(function&&) _NOEXCEPT;
    ^
/opt/llvm/bin/../include/c++/v1/functional:2287:5: note: candidate template ignored: requirement '__callable<(lambda at test/tools/router_check/router.cc:162:32), false>::value' was not satisfied [with _Fp = (lambda at test/tools/router_check/router.cc:162:32)]
    function(_Fp);
    ^

How about to put those functions inline?

  Envoy::Http::Utility::EncodeFunctions encode_functions{
      nullptr,
      [&](Http::ResponseHeaderMapPtr&& headers, bool end_stream) -> void {
        UNREFERENCED_PARAMETER(end_stream);
        Http::HeaderMapImpl::copyFrom(*tool_config.response_headers_->header_map_, *headers);
      },
      [&](Buffer::Instance& data, bool end_stream) -> void {
        UNREFERENCED_PARAMETER(data);
        UNREFERENCED_PARAMETER(end_stream);
      }};

dio
dio suggested changes Aug 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Looks good overall, some comments:

kb000 added 8 commits August 27, 2020 22:19
@kb000
Revert "Remove deprecated fields from router_check_tool unit tests."
471edad 
This reverts commit eb898b6.

Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Inline encode_functions
e49a07e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Bring back docs for deprecated *_header_fields
e252976 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Come on linux_x64 why don't you understand me
5457fe0 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
again
b9d73ed 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
again
990cee3 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Merge branch 'master' into dev/kb000/router_check_contenttype_2
695b89e 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Merge branch 'master' into dev/kb000/router_check_contenttype_2
000fdfe 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Use HeaderUtility::matchHeaders
f88f9cd 
Signed-off-by: Kevin Burek <kburek@lyft.com>
kb000 added 4 commits September 2, 2020 21:04
@kb000
reorder headers
c44d461 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
Remove deprecated fields
012e9e6 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
clang_tidy
f2578be 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
clang_tidy
a28d068 
Signed-off-by: Kevin Burek <kburek@lyft.com>
mattklein123
mattklein123 requested changes Sep 3, 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.

Awesome, thanks for the changes, much more future proof. Just a few small comments and let's ship!

/wait

kb000 added 2 commits September 3, 2020 18:00
@kb000
Address comments
16c1226 
Remove default case
Test failure strings

Signed-off-by: Kevin Burek <kburek@lyft.com>
@kb000
link router_check_tool from release notes
f2cc096 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 4, 2020

Awesome, thanks, looks great. Can you merge main? Main was broken for a bit.

/wait

@kb000
Merge branch 'master' into dev/kb000/router_check_contenttype_2
3cfdd90 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 4, 2020

Sorry gcc compile error looks legit.

/wait

@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Sep 4, 2020

The compile error doesn't make sense.

test/tools/router_check/router.cc: In function 'const string {anonymous}::toString(envoy::config::route::v3::HeaderMatcher::HeaderMatchSpecifierCase)':
test/tools/router_check/router.cc:51:1: error: control reaches end of non-void function [-Werror=return-type]
   51 | }
      | ^

That switch statement treats all cases. This build error is exactly the safety mechanism we want to future-proof this method.
https://github.com/envoyproxy/envoy/pull/12810/files#diff-be4db2a080dea0422cf3ad8c0ceb7fd3R21-R51

@kb000
Add default case to toString
9b827ed 
Signed-off-by: Kevin Burek <kburek@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 4, 2020

Remove the default case and put a NOT_REACHED at the end of the function (outside of the switch) and it will compile. Sorry for the trouble.

/wait

@kb000
NOT_REACHED outside switch
f34dc24 
Signed-off-by: Kevin Burek <kburek@lyft.com>
mattklein123
mattklein123 approved these changes Sep 5, 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!

@mattklein123 mattklein123 merged commit 7f659a0 into envoyproxy:master Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@dio dio dio left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dio dio

@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

3 participants

@kb000 @dio @mattklein123