Skip to content

Commit

Permalink
ext_proc: runtime guard for timeout error (envoyproxy#35475)
Browse files Browse the repository at this point in the history
<!--
!!!ATTENTION!!!

If you are fixing *any* crash or *any* potential security issue, *do
not*
open a pull request in this repo. Please report the issue via emailing
[email protected] where the issue will be triaged
appropriately.
Thank you in advance for helping to keep Envoy secure.

!!!ATTENTION!!!

For an explanation of how to fill out the fields, please see the
relevant section
in
[PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md)
-->

Commit Message: envoyproxy#34856 fixs the
timeout error but it is a behavior change that could cause the customer
surprises. Adding runtime guard.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: tyxia <[email protected]>
  • Loading branch information
tyxia authored Jul 29, 2024
1 parent eba7bc2 commit 71244bf
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 1 deletion.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ bug_fixes:
- area: websocket
change: |
Fixed a bug where the websocket upgrade filter would not take into account per-filter configs.
- area: ext_proc
change: |
Add runtime guard for timeout error code 504 Gateway Timeout that is returned to downstream. If runtime flag
``envoy.reloadable_features.ext_proc_timeout_error`` is set to false, old error code 500 Internal Server Error will be returned.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ RUNTIME_GUARD(envoy_reloadable_features_edf_lb_locality_scheduler_init_fix);
RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);
RUNTIME_GUARD(envoy_reloadable_features_exclude_host_in_eds_status_draining);
RUNTIME_GUARD(envoy_reloadable_features_ext_proc_timeout_error);
RUNTIME_GUARD(envoy_reloadable_features_gcp_authn_use_fixed_url);
RUNTIME_GUARD(envoy_reloadable_features_grpc_side_stream_flow_control);
RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_delay_reset);
Expand Down
6 changes: 5 additions & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1216,7 +1216,11 @@ void Filter::onMessageTimeout() {
decoding_state_.onFinishProcessorCall(Grpc::Status::DeadlineExceeded);
encoding_state_.onFinishProcessorCall(Grpc::Status::DeadlineExceeded);
ImmediateResponse errorResponse;
errorResponse.mutable_status()->set_code(StatusCode::GatewayTimeout);

errorResponse.mutable_status()->set_code(
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_timeout_error")
? StatusCode::GatewayTimeout
: StatusCode::InternalServerError);
errorResponse.set_details(absl::StrFormat("%s_per-message_timeout_exceeded", ErrorPrefix));
sendImmediateResponse(errorResponse);
}
Expand Down
18 changes: 18 additions & 0 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2395,6 +2395,24 @@ TEST_P(ExtProcIntegrationTest, RequestMessageTimeout) {
verifyDownstreamResponse(*response, 504);
}

TEST_P(ExtProcIntegrationTest, RequestMessageTimeoutOldErrorCode) {
scoped_runtime_.mergeValues({{"envoy.reloadable_features.ext_proc_timeout_error", "false"}});
// ensure 200 ms timeout
proto_config_.mutable_message_timeout()->set_nanos(200000000);
initializeConfig();
HttpIntegrationTest::initialize();
auto response = sendDownstreamRequest(absl::nullopt);
processRequestHeadersMessage(*grpc_upstreams_[0], true,
[this](const HttpHeaders&, HeadersResponse&) {
// Travel forward 400 ms
timeSystem().advanceTimeWaitImpl(400ms);
return false;
});

// We should immediately have an error response now
verifyDownstreamResponse(*response, 500);
}

TEST_P(ExtProcIntegrationTest, RequestMessageTimeoutWithTracing) {
// ensure 200 ms timeout
proto_config_.mutable_message_timeout()->set_nanos(200000000);
Expand Down

0 comments on commit 71244bf

Please sign in to comment.