Test-server extension: use shared configuration base and test facilities by oschaaf · Pull Request #512 · envoyproxy/nighthawk

oschaaf · 2020-09-05T15:12:32Z

Make the test server use the shared configuration handling code.
Convert its tests to use the new facilities that are shared accross extensions.

Last in a series of PRs to fix #498

This is on par with what we did for the dynamic delay and timing extensions,
but this extension was slightly more complex to begin with, so saving the
best for last ;-)

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Original PR description:

A first step to deduplicate code across the c++ test server extensions, and help us handle
configuration and testing of behaviour consistently.

Intent was for this to be a functional no-op, but added testing uncovered
issues in request post body handling. this solves that, modulo an issue with larger
request bodies vs the dynamic delay filter, which would need further analysis.

Prerequisites:

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Will still fail the test because of mismatched expectations, but this will help merging master periodically in here to verify (almost) all is well. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2020-09-24T17:58:37Z

@pamorgan please review and assign back to me once done.

dubious90 · 2020-09-30T15:58:39Z

Peter is out of office and this PR has been delayed - @qqustc can you please review and assign back to me once done?

dubious90 · 2020-09-30T17:49:07Z

Qin doesn't have the bandwidth for a review right now. @wjuan-AFK can you please review and assign back to me once done?

wjuan-AFK · 2020-10-02T21:02:34Z

source/server/http_test_server_filter.cc

-    sendReply();
+    if (!config_->maybeSendErrorReply(*decoder_callbacks_)) {
+      const auto effective_config = config_->getEffectiveConfiguration();
+      if (effective_config.value()->echo_request_headers()) {


Should we check that the value is ok instead of just calling value() directly? Or do we prefer to throw an exception here?

maybeSendErrorReply() will have returned true if the value wasn't OK, so we can't hit this.

Ah, got it. I had missed the connection between getEffective config and maybeSendError.

wjuan-AFK · 2020-10-02T21:08:50Z

source/server/http_test_server_filter.cc

+  config_->computeEffectiveConfiguration(headers);
  if (end_stream) {
-    sendReply();
+    if (!config_->maybeSendErrorReply(*decoder_callbacks_)) {


I'm not sure that this is worth bothering with, but I did find it mildly confusing that the result of maybeSendErrorReply is true if it did send an error reply.

Do you think that this would easier to read if bool maybeSendErrorReply() would be split up into bool haveErrorReply() and void sendErrorReply()?

Probably. I'm not sure if I'm alone in feeling uncertain on which boolean value to expect from a maybe naming scheme though.

What if we called it validateOrSendError.

I was following up on this, I like the suggestion, but while doing that, I realised that the return value would have to be inverted. The maybeXXX "naming scheme" usually returns true iff it did what it said it might do (sendErrorReply() in this case). When we rename to validateOrSendError() the method should probably return true if the configuration was valid? I think this would still be a good change to make, but it would become a little intrusive as there's a bunch of places where this is used already. The good part is that when this PR gets merged, we are in a much better to position to make mechanical refactors like this across all extensions. Filed #558 to track following up.

wjuan-AFK · 2020-10-02T21:49:08Z

test/server/http_test_server_filter_integration_test.cc


 TEST_P(HttpTestServerIntegrationTest, TestMaxBoundaryLengthRequest) {
+  initializeFilterConfiguration(kDefaultProto);
  const int max = 1024 * 1024 * 4;


Why do we define max this way? Is there another expression that would be equivalent?

This equates to / could be expressed as 4194304, or 0x400000, but I was hoping that the way it is expressed now would be easiest to grok it as being the equivalent of 4MB for readers of this code?

Makes sense.

wjuan-AFK · 2020-10-02T22:01:32Z

test/server/http_test_server_filter_integration_test.cc

-
 // Here we test config-level merging as well as its application at the response-header level.
-TEST_F(HttpTestServerDecoderFilterTest, HeaderMerge) {
+TEST(HttpTestServerDecoderFilterTest, HeaderMerge) {


I might be misunderstanding how this test works. Is there any chance this could be split into a couple separate cases being tested? It feels like it's trying to test several different kind of header merge.

You're right, this test has organically grown as different contributors added to it, and splitting this up makes sense.
As this is pre-existing, I prefer to file an issue and address this in a follow up, would you be ok with that?

…refactoring-1 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

wjuan-AFK · 2020-10-05T14:45:36Z

LGTM

…refactoring-1 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

Looks good, just one nit really.

mum4k · 2020-10-06T04:01:20Z

source/server/http_test_server_filter.cc

  if (end_stream) {
-    sendReply();
+    if (!config_->maybeSendErrorReply(*decoder_callbacks_)) {
+      const auto effective_config = config_->getEffectiveConfiguration();


Can we replace the "auto" with a type for improved readability?

done in 2529ea6

mum4k · 2020-10-06T04:04:34Z

test/server/http_test_server_filter_integration_test.cc

-    return makeSingleRequest(addr, method, url, body, type, host, content_type,
-                             request_header_delegate);
-  }
+const std::string kDefaultProto = R"EOF(


(optional) It is typically not recommended to create globals of object that have non-trivial destructors, since c++ doesn't guarantee or specify the order in which these objects get destructed. So it is very easy to end up in an undefined state where object A depends on B, but B gets destructed before A. We had a lot of crashes of servers while attempting proper shutdown due to this.

A good trick around it is to ensure that the global never gets destructed. One way of doing that is what the original code had, i.e. a constexpr absl::string_view instead of an std::string.

Doesn't really matter here, since this is a test file, so optional. It's just something that triggers my muscle memory.

done in 2529ea6

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-10-06T07:10:51Z

Also, added 2bcbb2b, which raises the coverage thresholds to match where we are now.

oschaaf added 30 commits June 15, 2020 17:31

Save state on origin timeing

c226497

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

c6697de

Fix clang-tidy, format, and test build

28e0d34

Will still fail the test because of mismatched expectations, but this will help merging master periodically in here to verify (almost) all is well. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

94ad984

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

stats naming

933c589

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

a1691ec

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Track request receipt deltas. Configure response header name.

740d0ca

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Save state before context switching

1afe2f1

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

ae0c08e

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

3487e19

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix format

1d1f03a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Suppress clang-tidy on MUTABLE_CONSTRUCT_ON_FIRST_USE

a9d99eb

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Proper locking, add TODOs and doc comments.

0aa8630

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

db01e40

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Small fixes

5e48513

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add unit test for StreamDecoder

d9569cb

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

cf1170c

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

check_format introduced proto comment issues. Add punctuation.

421feb8

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

af148ea

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

e813e75

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Save state on splitting out a separate extension and stopwatch

505495c

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

5405dc9

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Small corrections

d858b55

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add threaded spam test for stopwatch. Add doc comments.

3cf32b8

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

clang-tidy: fix for loop

35aee47

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into origin-timings

027c00f

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback: doc comments

70502e1

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Wire up proto option

a55bbe7

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Wire up TCLAP, add OptionImpl tests, regen CLI docs.

d199313

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Address clang-tidy nit, better CLI/proto option description.

a16d8dd

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added the waiting-for-review A PR waiting for a review. label Sep 24, 2020

mum4k requested a review from pamorgan September 24, 2020 17:58

dubious90 requested a review from qqustc September 30, 2020 15:57

dubious90 removed the request for review from pamorgan September 30, 2020 15:58

dubious90 requested review from wjuan-AFK and removed request for qqustc September 30, 2020 17:49

wjuan-AFK reviewed Oct 2, 2020

View reviewed changes

wjuan-AFK 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 Oct 2, 2020

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Oct 2, 2020

Merge remote-tracking branch 'upstream/master' into server-extension-…

815478d

…refactoring-1 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

wjuan-AFK assigned dubious90 Oct 5, 2020

mum4k assigned mum4k and unassigned dubious90 Oct 5, 2020

Merge remote-tracking branch 'upstream/master' into server-extension-…

838f366

…refactoring-1 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k requested changes Oct 6, 2020

View reviewed changes

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 Oct 6, 2020

Review feedback

2529ea6

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Oct 6, 2020

bump coverage thresholds

2bcbb2b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k approved these changes Oct 6, 2020

View reviewed changes

mum4k merged commit 9d77161 into envoyproxy:master Oct 6, 2020

Conversation

oschaaf commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mum4k commented Sep 24, 2020

Uh oh!

dubious90 commented Sep 30, 2020

Uh oh!

dubious90 commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjuan-AFK commented Oct 5, 2020

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oschaaf commented Sep 5, 2020 •

edited

Loading