-
Notifications
You must be signed in to change notification settings - Fork 92
Test-server extension: use shared configuration base and test facilities #512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c226497
c6697de
28e0d34
94ad984
933c589
a1691ec
740d0ca
1afe2f1
ae0c08e
3487e19
1d1f03a
a9d99eb
0aa8630
db01e40
5e48513
d9569cb
cf1170c
421feb8
af148ea
e813e75
505495c
5405dc9
d858b55
3cf32b8
35aee47
027c00f
70502e1
a55bbe7
d199313
a16d8dd
f3008d9
7774e2d
937fd88
8152829
64cb19c
af60b8d
3996c65
228934a
00b6861
7cf2edd
68ed58a
01e30e5
74f9717
699b156
54ca814
594bbbe
e36e9c1
86f5d98
eae77cc
dddc710
b1993a1
6778723
809715e
f5f92ab
83e7001
8b3db6c
44496b9
4022a79
3861456
9d333ac
7f432b8
87bd08d
988d62a
9345b66
9be5141
9f0229a
c31a93e
c32c92b
5cdb2a0
cf13178
a7a6aca
3292e2b
079cd01
746ad1a
2b945c2
a9617b7
79b2508
815478d
838f366
2529ea6
2bcbb2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,60 +13,53 @@ namespace Nighthawk { | |
| namespace Server { | ||
|
|
||
| HttpTestServerDecoderFilterConfig::HttpTestServerDecoderFilterConfig( | ||
| nighthawk::server::ResponseOptions proto_config) | ||
| : server_config_(std::move(proto_config)) {} | ||
| const nighthawk::server::ResponseOptions& proto_config) | ||
| : FilterConfigurationBase(proto_config, "test-server") {} | ||
|
|
||
| HttpTestServerDecoderFilter::HttpTestServerDecoderFilter( | ||
| HttpTestServerDecoderFilterConfigSharedPtr config) | ||
| : config_(std::move(config)) {} | ||
|
|
||
| void HttpTestServerDecoderFilter::onDestroy() {} | ||
|
|
||
| void HttpTestServerDecoderFilter::sendReply() { | ||
| if (!json_merge_error_) { | ||
| std::string response_body(base_config_.response_body_size(), 'a'); | ||
| if (request_headers_dump_.has_value()) { | ||
| response_body += *request_headers_dump_; | ||
| } | ||
| decoder_callbacks_->sendLocalReply( | ||
| static_cast<Envoy::Http::Code>(200), response_body, | ||
| [this](Envoy::Http::ResponseHeaderMap& direct_response_headers) { | ||
| Configuration::applyConfigToResponseHeaders(direct_response_headers, base_config_); | ||
| }, | ||
| absl::nullopt, ""); | ||
| } else { | ||
| decoder_callbacks_->sendLocalReply( | ||
| static_cast<Envoy::Http::Code>(500), | ||
| fmt::format("test-server didn't understand the request: {}", error_message_), nullptr, | ||
| absl::nullopt, ""); | ||
| void HttpTestServerDecoderFilter::sendReply(const nighthawk::server::ResponseOptions& options) { | ||
| std::string response_body(options.response_body_size(), 'a'); | ||
| if (request_headers_dump_.has_value()) { | ||
| response_body += *request_headers_dump_; | ||
| } | ||
| decoder_callbacks_->sendLocalReply( | ||
| static_cast<Envoy::Http::Code>(200), response_body, | ||
| [options](Envoy::Http::ResponseHeaderMap& direct_response_headers) { | ||
| Configuration::applyConfigToResponseHeaders(direct_response_headers, options); | ||
| }, | ||
| absl::nullopt, ""); | ||
| } | ||
|
|
||
| Envoy::Http::FilterHeadersStatus | ||
| HttpTestServerDecoderFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers, | ||
| bool end_stream) { | ||
| // TODO(oschaaf): Add functionality to clear fields | ||
| base_config_ = config_->server_config(); | ||
| const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); | ||
| if (request_config_header) { | ||
| json_merge_error_ = !Configuration::mergeJsonConfig( | ||
| request_config_header->value().getStringView(), base_config_, error_message_); | ||
| } | ||
| if (base_config_.echo_request_headers()) { | ||
| std::stringstream headers_dump; | ||
| headers_dump << "\nRequest Headers:\n" << headers; | ||
| request_headers_dump_ = headers_dump.str(); | ||
| } | ||
| config_->computeEffectiveConfiguration(headers); | ||
| if (end_stream) { | ||
| sendReply(); | ||
| if (!config_->maybeSendErrorReply(*decoder_callbacks_)) { | ||
| const absl::StatusOr<EffectiveFilterConfigurationPtr> effective_config = | ||
| config_->getEffectiveConfiguration(); | ||
| if (effective_config.value()->echo_request_headers()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check that the value is ok instead of just calling value() directly? Or do we prefer to throw an exception here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, got it. I had missed the connection between getEffective config and maybeSendError. |
||
| std::stringstream headers_dump; | ||
| headers_dump << "\nRequest Headers:\n" << headers; | ||
| request_headers_dump_ = headers_dump.str(); | ||
| } | ||
| sendReply(*effective_config.value()); | ||
| } | ||
| } | ||
| return Envoy::Http::FilterHeadersStatus::StopIteration; | ||
| } | ||
|
|
||
| Envoy::Http::FilterDataStatus HttpTestServerDecoderFilter::decodeData(Envoy::Buffer::Instance&, | ||
| bool end_stream) { | ||
| if (end_stream) { | ||
| sendReply(); | ||
| if (!config_->maybeSendErrorReply(*decoder_callbacks_)) { | ||
| sendReply(*config_->getEffectiveConfiguration().value()); | ||
| } | ||
| } | ||
| return Envoy::Http::FilterDataStatus::StopIterationNoBuffer; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that this would easier to read if
bool maybeSendErrorReply()would be split up intobool haveErrorReply()andvoid sendErrorReply()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. I'm not sure if I'm alone in feeling uncertain on which boolean value to expect from a maybe naming scheme though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we called it validateOrSendError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tovalidateOrSendError()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.