Skip to content

Commit

Permalink
Adding exception handling for response with large override_message_t…
Browse files Browse the repository at this point in the history
…imeout

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
  • Loading branch information
yanjunxiang-google committed May 9, 2023
1 parent adb6e5a commit 2554501
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 7 deletions.
5 changes: 1 addition & 4 deletions api/envoy/service/ext_proc/v3/external_processor.proto
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,7 @@ message ProcessingResponse {
// :ref:`max_message_timeout <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExternalProcessor.max_message_timeout>`
// Such message can be sent at most once in a particular Envoy ext_proc filter processing state.
// To enable this API, one has to set ``max_message_timeout`` to a number >= 1ms.
google.protobuf.Duration override_message_timeout = 10 [(validate.rules).duration = {
lte {seconds: 3600}
gte {nanos: 1000000}
}];
google.protobuf.Duration override_message_timeout = 10;
}

// The following are messages that are sent to the server.
Expand Down
11 changes: 10 additions & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,16 @@ void Filter::onReceiveMessage(std::unique_ptr<ProcessingResponse>&& r) {

// Check whether the server is asking to extend the timer.
if (response->has_override_message_timeout()) {
onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout()));
// The override_message_timeout in response may be too big for duration, which leads
// to exception been thrown during durationToMilliseconds() check.
// Needs to properly handle this case.
try {
onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout()));
} catch (const DurationUtil::OutOfRangeException& e) {
ENVOY_LOG(warn, "override_message_timeout value out-of-range: {}. Ignoring the message.",
e.what());
stats_.override_message_timeout_ignored_.inc();
}
return;
}

Expand Down
13 changes: 11 additions & 2 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,

// ext_proc server sends back a response to tell Envoy to stop the
// original timer and start a new timer.
void serverSendNewTimeout(const uint32_t timeout_ms) {
void serverSendNewTimeout(const uint64_t timeout_ms) {
ProcessingResponse response;
if (timeout_ms < 1000) {
response.mutable_override_message_timeout()->set_nanos(timeout_ms * 1000000);
Expand All @@ -327,7 +327,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,

// The new timeout message is ignored by Envoy due to different reasons, like
// new_timeout setting is out-of-range, or max_message_timeout is not configured.
void newTimeoutWrongConfigTest(const uint32_t timeout_ms) {
void newTimeoutWrongConfigTest(const uint64_t timeout_ms) {
// Set envoy filter timeout to be 200ms.
proto_config_.mutable_message_timeout()->set_nanos(200000000);
// Config max_message_timeout proto to enable the new timeout API.
Expand Down Expand Up @@ -1960,4 +1960,13 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc
newTimeoutWrongConfigTest(500);
}

// Send the new timeout to be an extremely large number, which will trigger exception being thrown.
// Verify the code appropriately handled it.
TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) {
// Config max_message_timeout proto to 100ms to enable the new timeout API.
max_message_timeout_ms_ = 100;
const uint64_t override_message_timeout = 1000000000000000;
newTimeoutWrongConfigTest(override_message_timeout);
}

} // namespace Envoy

0 comments on commit 2554501

Please sign in to comment.