hds: prevent timer reset on every update (#5977)

If we push frequent updates but with the same interval, we may never get responses back because the timer gets reset on every update.
This change makes a reset only if the interval has changed or a disconnect ocurred.

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

source/common/upstream/health_discovery_service.cc

@@ -166,12 +166,15 @@ void HdsDelegate::onReceiveMessage(
   hds_clusters_.clear();
 
   // Set response
-  server_response_ms_ = PROTOBUF_GET_MS_REQUIRED(*message, interval);
+  auto server_response_ms = PROTOBUF_GET_MS_REQUIRED(*message, interval);
 
   // Process the HealthCheckSpecifier message
   processMessage(std::move(message));
 
-  setHdsStreamResponseTimer();
+  if (server_response_ms_ != server_response_ms) {
+    server_response_ms_ = server_response_ms;
+    setHdsStreamResponseTimer();
+  }
 }
 
 void HdsDelegate::onReceiveTrailingMetadata(Http::HeaderMapPtr&& metadata) {
@@ -182,6 +185,7 @@ void HdsDelegate::onRemoteClose(Grpc::Status::GrpcStatus status, const std::stri
   ENVOY_LOG(warn, "gRPC config stream closed: {}, {}", status, message);
   hds_stream_response_timer_->disableTimer();
   stream_ = nullptr;
+  server_response_ms_ = 0;
   handleFailure();
 }
 

test/integration/hds_integration_test.cc

@@ -687,5 +687,45 @@ TEST_P(HdsIntegrationTest, TestUpdateMessage) {
   cleanupHdsConnection();
 }
 
+// Tests Envoy HTTP health checking a single endpoint, receiving an update
+// message from the management server and reporting in a new interval
+TEST_P(HdsIntegrationTest, TestUpdateChangesTimer) {
+  initialize();
+
+  // Server <--> Envoy
+  waitForHdsStream();
+  ASSERT_TRUE(hds_stream_->waitForGrpcMessage(*dispatcher_, envoy_msg_));
+
+  // Server asks for health checking
+  server_health_check_specifier_ = makeHttpHealthCheckSpecifier();
+  hds_stream_->startGrpcStream();
+  hds_stream_->sendGrpcMessage(server_health_check_specifier_);
+  test_server_->waitForCounterGe("hds_delegate.requests", ++hds_requests_);
+
+  healthcheckEndpoints();
+
+  // an update should be received after interval
+  ASSERT_TRUE(
+      hds_stream_->waitForGrpcMessage(*dispatcher_, response_, std::chrono::milliseconds(250)));
+
+  // New HealthCheckSpecifier message
+  server_health_check_specifier_.mutable_interval()->set_nanos(300000000); // 0.3 seconds
+
+  // Server asks for health checking with the new message
+  hds_stream_->sendGrpcMessage(server_health_check_specifier_);
+  test_server_->waitForCounterGe("hds_delegate.requests", ++hds_requests_);
+
+  // A response should not be received until the new timer is completed
+  ASSERT_FALSE(
+      hds_stream_->waitForGrpcMessage(*dispatcher_, response_, std::chrono::milliseconds(250)));
+  // Response should be received now
+  ASSERT_TRUE(
+      hds_stream_->waitForGrpcMessage(*dispatcher_, response_, std::chrono::milliseconds(250)));
+
+  // Clean up connections
+  cleanupHostConnections();
+  cleanupHdsConnection();
+}
+
 } // namespace
 } // namespace Envoy