Skip to content
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

Envoy ext_proc filter throw exception when received response timeout Duration is too large #27260

Merged
merged 7 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@ message ExternalProcessor {
// to the processing mode) if the timer expires before a matching response
// is received. There is no timeout when the filter is running in asynchronous
// mode. Default is 200 milliseconds.
google.protobuf.Duration message_timeout = 7;
google.protobuf.Duration message_timeout = 7 [(validate.rules).duration = {
lte {seconds: 3600}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
gte {}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
}];

// Optional additional prefix to use when emitting statistics. This allows to distinguish
// emitted statistics between configured *ext_proc* filters in an HTTP filter chain.
Expand All @@ -167,7 +170,10 @@ message ExternalProcessor {
// Specify the upper bound of
// :ref:`override_message_timeout <envoy_v3_api_field_service.ext_proc.v3.ProcessingResponse.override_message_timeout>`
// If not specified, by default it is 0, which will effectively disable the ``override_message_timeout`` API.
google.protobuf.Duration max_message_timeout = 10;
google.protobuf.Duration max_message_timeout = 10 [(validate.rules).duration = {
lte {seconds: 3600}
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
gte {}
}];

// Prevents clearing the route-cache when the
// :ref:`clear_route_cache <envoy_v3_api_field_service.ext_proc.v3.CommonResponse.clear_route_cache>`
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 {
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout()));
} catch (const DurationUtil::OutOfRangeException& e) {
yanjunxiang-google marked this conversation as resolved.
Show resolved Hide resolved
ENVOY_LOG(warn, "override_message_timeout value out-of-range: {}. Ignoring the message.",
e.what());
stats_.override_message_timeout_ignored_.inc();
}
return;
}

Expand Down
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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.