From f79685371b9cadc2fe8eb56b912a27163609e4d4 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 10 May 2023 01:09:57 +0000 Subject: [PATCH] addressing comments Signed-off-by: Yanjun Xiang --- source/common/protobuf/utility.cc | 41 +++++++++++++++---- source/common/protobuf/utility.h | 9 ++++ .../filters/http/ext_proc/ext_proc.cc | 24 +++++------ .../filters/http/ext_proc/ext_proc.h | 2 +- .../ext_proc/ext_proc_integration_test.cc | 2 +- 5 files changed, 55 insertions(+), 23 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 7cdc7d5042a8..963587c39d9e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -645,34 +645,57 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector& v namespace { -void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { +void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value, + bool throw_exception, bool& error) { + error = false; if (duration.seconds() < 0 || duration.nanos() < 0) { - throw DurationUtil::OutOfRangeException( - fmt::format("Expected positive duration: {}", duration.DebugString())); + if (throw_exception) { + throw DurationUtil::OutOfRangeException( + fmt::format("Expected positive duration: {}", duration.DebugString())); + } else { + error = true; + return; + } } if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); + if (throw_exception) { + throw DurationUtil::OutOfRangeException( + fmt::format("Duration out-of-range: {}", duration.DebugString())); + } else { + error = true; + } } } void validateDuration(const ProtobufWkt::Duration& duration) { - validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds); + bool error = false; + validateDuration(duration, Protobuf::util::TimeUtil::kDurationMaxSeconds, true, error); } -void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) { +void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration, bool throw_exception, + bool& error) { // Apply stricter max boundary to the `seconds` value to avoid overflow. // Note that protobuf internally converts to nanoseconds. // The kMaxInt64Nanoseconds = 9223372036, which is about 300 years. constexpr int64_t kMaxInt64Nanoseconds = std::numeric_limits::max() / (1000 * 1000 * 1000); - validateDuration(duration, kMaxInt64Nanoseconds); + validateDuration(duration, kMaxInt64Nanoseconds, throw_exception, error); } } // namespace uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) { - validateDurationAsMilliseconds(duration); + bool error = false; + validateDurationAsMilliseconds(duration, true, error); + return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); +} + +uint64_t DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, + bool& error) { + validateDurationAsMilliseconds(duration, false, error); + if (error) { + return 0; + } return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 7f5847e310a7..080abc2ecd3b 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -683,6 +683,15 @@ class DurationUtil { */ static uint64_t durationToMilliseconds(const ProtobufWkt::Duration& duration); + /** + * Same as DurationUtil::durationToMilliseconds but does not throw an exception. + * Instead, it returns error if duration is out-of-range. + * @param duration protobuf. + * @param error bool, set into true in case of duration out-of-range; otherwise false. + * @return duration in milliseconds. + */ + static uint64_t durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration, bool& error); + /** * Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic. * Specifically, we ensure that the duration is positive. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 92f9606afb00..a538bcc67054 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -527,12 +527,21 @@ void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers stats_.stream_msgs_sent_.inc(); } -void Filter::onNewTimeout(const uint32_t message_timeout_ms) { +void Filter::onNewTimeout(const ProtobufWkt::Duration& override_message_timeout) { + bool error = false; + uint32_t message_timeout_ms = + DurationUtil::durationToMillisecondsNoThrow(override_message_timeout, error); + if (error) { + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of duration range. " + "Ignoring the message."); + stats_.override_message_timeout_ignored_.inc(); + return; + } // The new timeout has to be >=1ms and <= max_message_timeout configured in filter. const uint32_t min_timeout_ms = 1; const uint32_t max_timeout_ms = config_->maxMessageTimeout(); if (message_timeout_ms < min_timeout_ms || message_timeout_ms > max_timeout_ms) { - ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of range. " + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of config range. " "Ignoring the message."); stats_.override_message_timeout_ignored_.inc(); return; @@ -559,16 +568,7 @@ void Filter::onReceiveMessage(std::unique_ptr&& r) { // Check whether the server is asking to extend the timer. if (response->has_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(); - } + onNewTimeout(response->override_message_timeout()); return; } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 0bc052997e88..2824cb4d1944 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -191,7 +191,7 @@ class Filter : public Logger::Loggable, void onGrpcClose() override; void onMessageTimeout(); - void onNewTimeout(const uint32_t message_timeout_ms); + void onNewTimeout(const ProtobufWkt::Duration& override_message_timeout); void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state, bool end_stream) { diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 1adb3a327463..1d4565a56470 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -1960,7 +1960,7 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc newTimeoutWrongConfigTest(500); } -// Send the new timeout to be an extremely large number, which will trigger exception being thrown. +// Send the new timeout to be an extremely large number, which is out-of-range of duration. // Verify the code appropriately handled it. TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) { // Config max_message_timeout proto to 100ms to enable the new timeout API.