Add echo_request_headers to the test server filter ResponseOptions. by efimki · Pull Request #338 · envoyproxy/nighthawk

efimki · 2020-06-01T20:42:04Z

If echo_request_headers option is set to true, then request headers dump is appended to the response body.

efimki · 2020-06-01T20:42:30Z

@htuch PTAL when you have a chance.

Requests with this path get response with the dump of request headers instead of generated output. Signed-off-by: Misha Efimov <mef@google.com>

Signed-off-by: Misha Efimov <mef@google.com>

oschaaf

Some drive-by comments:
This looks like a great feature to me, some thoughts:

Maybe an option to allow people to configure /echoheaders would be nice-to-have.
(It would also allow an escape latch in the (highly unlikely) event someone needs this path to function like it did before this change)
A note in README.md would help make this feature discoverable by others.

efimki · 2020-06-02T13:43:58Z

This looks like a great feature to me, some thoughts:

Excellent, thanks for your suggestions!

Maybe an option to allow people to configure /echoheaders would be nice-to-have.
(It would also allow an escape latch in the (highly unlikely) event someone needs this path to function like it did before this change)

The current implementation is inspired by Chromium's Embedded Test Server [1] that is mostly used in net and cronet [2] unit tests.

Your suggestion made me realize that in Nighthawk it will be better to configure special handlers using config instead of pre-defined request path. This way echo behaviors can be configured per individual request, applied to any request path, and combined if necessary (for example, echo headers and echo metadata). I'll update the PR.

A note in README.md would help make this feature discoverable by others.

Absolutely! I just wanted to flesh out the implementation before updating the docs.

[1] https://source.chromium.org/chromium/chromium/src/+/master:net/test/embedded_test_server/embedded_test_server.h
[2] https://source.chromium.org/chromium/chromium/src/+/master:components/cronet/test/test_server.h

Signed-off-by: Misha Efimov <mef@google.com>

htuch

LGTM, but do you want to talk about the motivation in the README.md? I.e. why would I set this as a Nighthawk user?

Signed-off-by: Misha Efimov <mef@google.com>

efimki · 2020-06-03T14:06:54Z

Thanks for the suggestion!
I've added an example to README.md.
WDYT?

efimki · 2020-06-03T14:14:16Z

On related note, after this PR I'd like to add more response options that I think might be useful:

echo_request_metadata - include the dump of request metadata in the response body
echo_request_body - include the body of POST request in the response body
status_code - set http status code to arbitrary value, like 204 or 302

WDYT?

mum4k

Thank you for adding this feature and improving the documentation.

source/server/http_test_server_filter.h

test/server/http_test_server_filter_integration_test.cc

htuch · 2020-06-04T00:12:27Z

@efimki LGTM modulo the comments from @mum4k. Re: future directions, do you want to eventually have a more structured body. E.g. make it a proto defined JSON format or something like that? To what extent is this going to be human interpreted vs. machine read?

Signed-off-by: Misha Efimov <mef@google.com>

efimki

Thanks for your comments, please take a look.

source/server/http_test_server_filter.h

test/server/http_test_server_filter_integration_test.cc

efimki · 2020-06-04T14:55:40Z

@efimki LGTM modulo the comments from @mum4k. Re: future directions, do you want to eventually have a more structured body. E.g. make it a proto defined JSON format or something like that? To what extent is this going to be human interpreted vs. machine read?

Maybe? I haven't really thought this through yet.
The chromium test server that I know is very simple and doesn't do any special formatting.
My current use case is fine with plain text dump.
I would follow YAGNI spirit and don't decide on this until we have a clear use case for something more elaborate.

mum4k · 2020-06-04T19:58:23Z

Thank you @efimki for addressing my comments, this looks good in regards to test coverage.

I think @htuch raises a good point. I do agree that we shouldn't implement anything prematurely, however this might be the only time to introduce any structure, since we have a backward compatibility guarantee in the Nighthawk repository. Should we consider at least adding an inner structure (proto definition?) that will for now contain these headers only?

mum4k · 2020-06-04T20:51:49Z

@efimki can you please push a commit (or two) to kick the CI jobs (automated check)? We will want them to be green before we proceed.

Signed-off-by: Misha Efimov <mef@google.com>

efimki · 2020-06-04T20:56:25Z

Hopefully this will do. If not, I'll try again later.

oschaaf · 2020-06-04T21:42:37Z

FWIW I suspect CircleCI is hogged, we may just need to wait for that to clear up. Other PRs also seem subject to this now

htuch

LGTM. As long as it is just the request headers, I think it's totally fine to dump. If we start building a more structured response, with other fields like request body etc., I think it's time to add a proper structured format.

Signed-off-by: Misha Efimov <mef@google.com>

efimki · 2020-06-08T13:31:09Z

Trying to close / reopen to get ci unstuck.

Signed-off-by: Misha Efimov <mef@google.com>

mum4k · 2020-06-08T23:54:15Z

If nothing else helps, you can consider closing this PR and creating a new one with the same content. Since we seem to be out of ideas :(

Edit - I see now that you tried just that and it didn't seem to help. @oschaaf any ideas how to move this forward?

efimki · 2020-06-09T01:33:56Z

FWIW I don't seek the fame of authoring this PR. :)
Maybe somebody else could create a new PR based on it that will be not be ignored by CI?

oschaaf · 2020-06-09T10:33:58Z

I mirrored this PR into #349, and CI is running fine over there.
@efimki I'm sorry this is such a hassle.
I have checked everything that I have access to with respect to project-settings in github and CircleCI, but couldn't find anything I could relate to CI not running here. So I still can only speculate about potential causes.

oschaaf · 2020-06-09T16:15:36Z

So .. all tests are now passing here

efimki · 2020-06-09T17:00:04Z

Oh, nice, thanks for your help!
I think I've addressed @mum4k change request, but I don't know whether I should somehow explicitly mark it as addressed. Any ideas!

oschaaf · 2020-06-09T17:24:21Z

I think the last step is that @mum4k needs to explicitly add an approving review before this PR becomes mergeable. Apparently the CI run of my clone PR was able to green-light CI for this one by flagging the last (shared) commit as OK for all jobs.

mum4k · 2020-06-09T18:03:12Z

Magic, thanks @oschaaf.

@efimki would you mind closing all the related duplicates?

efimki · 2020-06-09T18:40:36Z

I've closed obsolete PRs #345 and #346 and opened #351 to discuss structure of additional echo responses.

Add /echoheaders request path handler to test-server filter.

e537816

Requests with this path get response with the dump of request headers instead of generated output. Signed-off-by: Misha Efimov <mef@google.com>

efimki force-pushed the echoheaders branch from 6bd32d8 to e537816 Compare June 1, 2020 21:03

Fix format.

ca88dac

Signed-off-by: Misha Efimov <mef@google.com>

oschaaf reviewed Jun 1, 2020

View reviewed changes

eric846 added the waiting-for-review A PR waiting for a review. label Jun 1, 2020

eric846 assigned mum4k Jun 1, 2020

Use echo_request_headers ResponseOption instead of /echoheaders path.

58dcb1f

Signed-off-by: Misha Efimov <mef@google.com>

efimki changed the title ~~Add /echoheaders request path handler to test-server filter.~~ Add echo_request_headers to the test server filter ResponseOptions. Jun 2, 2020

Update README.md

fcf2e0b

Signed-off-by: Misha Efimov <mef@google.com>

htuch reviewed Jun 2, 2020

View reviewed changes

efimki added 2 commits June 3, 2020 10:01

Add an example to README.md

9eb59d4

Signed-off-by: Misha Efimov <mef@google.com>

Fix markdown.

77e8ff8

Signed-off-by: Misha Efimov <mef@google.com>

mum4k requested changes Jun 3, 2020

View reviewed changes

source/server/http_test_server_filter.h Show resolved Hide resolved

test/server/http_test_server_filter_integration_test.cc Show resolved Hide resolved

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jun 3, 2020

Add unique_header to verify that echoed headers vary per request.

7d58a67

Signed-off-by: Misha Efimov <mef@google.com>

efimki commented Jun 4, 2020

View reviewed changes

source/server/http_test_server_filter.h Show resolved Hide resolved

test/server/http_test_server_filter_integration_test.cc Show resolved Hide resolved

efimki requested a review from mum4k June 4, 2020 15:37

Kick CI

bda8c6e

Signed-off-by: Misha Efimov <mef@google.com>

htuch approved these changes Jun 5, 2020

View reviewed changes

Kick CI

6fad7ec

Signed-off-by: Misha Efimov <mef@google.com>

efimki closed this Jun 8, 2020

efimki reopened this Jun 8, 2020

Kick CI

40d530b

Signed-off-by: Misha Efimov <mef@google.com>

This was referenced Jun 8, 2020

Add echo_request_headers to the test server filter ResponseOptions. #345

Closed

Add echo_request_headers to the test server filter ResponseOptions. #346

Closed

oschaaf mentioned this pull request Jun 9, 2020

Echoheaders #349

Closed

oschaaf approved these changes Jun 9, 2020

View reviewed changes

mum4k approved these changes Jun 9, 2020

View reviewed changes

mum4k merged commit adfc004 into envoyproxy:master Jun 9, 2020

efimki deleted the echoheaders branch June 9, 2020 18:26

efimki mentioned this pull request Jun 9, 2020

Add more echo responders to test server filter ResponseOptions. #351

Open

Conversation

efimki commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efimki commented Jun 1, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

efimki commented Jun 2, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

efimki commented Jun 3, 2020

Uh oh!

efimki commented Jun 3, 2020

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented Jun 4, 2020

Uh oh!

efimki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

efimki commented Jun 4, 2020

Uh oh!

mum4k commented Jun 4, 2020

Uh oh!

mum4k commented Jun 4, 2020

Uh oh!

efimki commented Jun 4, 2020

Uh oh!

oschaaf commented Jun 4, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

efimki commented Jun 8, 2020

Uh oh!

mum4k commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efimki commented Jun 9, 2020

Uh oh!

oschaaf commented Jun 9, 2020

Uh oh!

oschaaf commented Jun 9, 2020

Uh oh!

efimki commented Jun 9, 2020

Uh oh!

oschaaf commented Jun 9, 2020

Uh oh!

mum4k commented Jun 9, 2020

Uh oh!

efimki commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

efimki commented Jun 1, 2020 •

edited

Loading

mum4k commented Jun 8, 2020 •

edited

Loading