addressing comments

Signed-off-by: Yanjun Xiang <[email protected]>

source/common/protobuf/utility.cc

@@ -645,34 +645,57 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector<ProtobufWkt::Value>& 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<int64_t>::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);
 }
 

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.

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<ProcessingResponse>&& 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;
   }
 

source/extensions/filters/http/ext_proc/ext_proc.h

@@ -191,7 +191,7 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>,
   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) {

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.