Add retries to policy checks on failed transport error

Makefile

@@ -21,6 +21,8 @@ BAZEL_STARTUP_ARGS ?=
 BAZEL_BUILD_ARGS ?=
 BAZEL_TEST_ARGS ?=
 BAZEL_TARGETS ?= //...
+# Some tests run so slowly under the santizers that they always timeout.
+SANITIZER_EXCLUSIONS ?= -test/integration:mixer_fault_test
 HUB ?=
 TAG ?=
 ifeq "$(origin CC)" "default"
@@ -49,11 +51,11 @@ test:
 	@bazel shutdown
 
 test_asan:
-	PATH=$(PATH) CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-asan $(BAZEL_TARGETS)
+	PATH=$(PATH) CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-asan -- $(BAZEL_TARGETS) $(SANITIZER_EXCLUSIONS)
 	@bazel shutdown
 
 test_tsan:
-	PATH=$(PATH) CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-tsan $(BAZEL_TARGETS)
+	PATH=$(PATH) CC=$(CC) CXX=$(CXX) bazel $(BAZEL_STARTUP_ARGS) test $(BAZEL_TEST_ARGS) --config=clang-tsan -- $(BAZEL_TARGETS) $(SANITIZER_EXCLUSIONS)
 	@bazel shutdown
 
 check:

include/istio/control/http/request_handler.h

@@ -35,10 +35,13 @@ class RequestHandler {
   // * extract attributes from the config.
   // * if necessary, forward some attributes to downstream
   // * make a Check call.
-  virtual ::istio::mixerclient::CancelFunc Check(
-      CheckData* check_data, HeaderUpdate* header_update,
-      ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::CheckDoneFunc on_done) = 0;
+  virtual void Check(CheckData* check_data, HeaderUpdate* header_update,
+                     const ::istio::mixerclient::TransportCheckFunc& transport,
+                     const ::istio::mixerclient::CheckDoneFunc& on_done) = 0;
+
+  virtual void ResetCancel() = 0;
+
+  virtual void CancelCheck() = 0;
 
   // Make a Report call. It will:
   // * check service config to see if Report is required

include/istio/control/tcp/request_handler.h

@@ -32,8 +32,12 @@ class RequestHandler {
   // Perform a Check call. It will:
   // * extract downstream tcp connection attributes
   // * check config, make a Check call if necessary.
-  virtual ::istio::mixerclient::CancelFunc Check(
-      CheckData* check_data, ::istio::mixerclient::CheckDoneFunc on_done) = 0;
+  virtual void Check(CheckData* check_data,
+                     const ::istio::mixerclient::CheckDoneFunc& on_done) = 0;
+
+  virtual void ResetCancel() = 0;
+
+  virtual void CancelCheck() = 0;
 
   // Make report call.
   virtual void Report(ReportData* report_data,

include/istio/mixerclient/client.h

@@ -133,9 +133,9 @@ class MixerClient {
   // The response data from mixer will be consumed by mixer client.
 
   // A check call.
-  virtual CancelFunc Check(istio::mixerclient::CheckContextSharedPtr& context,
-                           TransportCheckFunc transport,
-                           CheckDoneFunc on_done) = 0;
+  virtual void Check(istio::mixerclient::CheckContextSharedPtr& context,
+                     const TransportCheckFunc& transport,
+                     const CheckDoneFunc& on_done) = 0;
 
   // A report call.
   virtual void Report(

include/istio/mixerclient/options.h

@@ -39,7 +39,17 @@ struct CheckOptions {
   const int num_entries;
 
   // If true, Check is passed for any network failures.
-  bool network_fail_open = true;
+  bool network_fail_open{true};
+
+  // Number of retries on transport error
+  uint32_t retries{0};
+
+  // Base milliseconds to sleep between retries.  Will be adjusted by
+  // exponential backoff and jitter.
+  uint32_t base_retry_ms{80};
+
+  // Max milliseconds to sleep between retries.
+  uint32_t max_retry_ms{1000};
 };
 
 // Options controlling report batch.

istio.deps

@@ -4,7 +4,7 @@
 		"name": "ISTIO_API",
 		"repoName": "api",
 		"file": "repositories.bzl",
-		"lastStableSHA": "92b7ddc0f30b3aab6a5e82a861e54bf55fe249bd"
+		"lastStableSHA": "5945a02236f53ad860d518772f730594709b1234"
 	},
 	{
 		"_comment": "",

repositories.bzl

@@ -99,8 +99,14 @@ cc_library(
             actual = "@googletest_git//:googletest_prod",
         )
 
-ISTIO_API = "92b7ddc0f30b3aab6a5e82a861e54bf55fe249bd"
-ISTIO_API_SHA256 = "03fc53fe2a2ac980d2fbe9eeab9cf5526f8e493c786f72ba456d4c4e78b44e6b"
+#
+# To update these...
+# 1) find the ISTIO_API SHA you want in git
+# 2) wget https://github.com/istio/api/archive/ISTIO_API_SHA.tar.gz
+# 3) sha256sum ISTIO_API_SHA.tar.gz
+#
+ISTIO_API = "5945a02236f53ad860d518772f730594709b1234"
+ISTIO_API_SHA256 = "b75411deda635c70bdbf12cd1d405129d1f23e6a56a5eebbe69c75cfa3d6009e"
 
 def mixerapi_repositories(bind = True):
     BUILD = """

src/envoy/http/mixer/filter.cc

@@ -76,7 +76,7 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) {
                        decoder_callbacks_->connection());
   Utils::HeaderUpdate header_update(&headers);
   headers_ = &headers;
-  cancel_check_ = handler_->Check(
+  handler_->Check(
       &check_data, &header_update,
       control_.GetCheckTransport(decoder_callbacks_->activeSpan()),
       [this](const CheckResponseInfo& info) { completeCheck(info); });
@@ -203,14 +203,12 @@ void Filter::completeCheck(const CheckResponseInfo& info) {
 
 void Filter::onDestroy() {
   ENVOY_LOG(debug, "Called Mixer::Filter : {} state: {}", __func__, state_);
-  if (state_ != Calling) {
-    cancel_check_ = nullptr;
+  if (state_ != Calling && handler_) {
+    handler_->ResetCancel();
   }
   state_ = Responded;
-  if (cancel_check_) {
-    ENVOY_LOG(debug, "Cancelling check call");
-    cancel_check_();
-    cancel_check_ = nullptr;
+  if (handler_) {
+    handler_->CancelCheck();
   }
 }
 

src/envoy/http/mixer/filter.h

@@ -89,8 +89,6 @@ class Filter : public StreamFilter,
   Control& control_;
   // The request handler.
   std::unique_ptr<::istio::control::http::RequestHandler> handler_;
-  // The pending callback object.
-  istio::mixerclient::CancelFunc cancel_check_;
 
   enum State { NotStarted, Calling, Complete, Responded };
   // The state

src/envoy/tcp/mixer/filter.cc

@@ -43,14 +43,12 @@ void Filter::initializeReadFilterCallbacks(
 }
 
 void Filter::cancelCheck() {
-  if (state_ != State::Calling) {
-    cancel_check_ = nullptr;
+  if (state_ != State::Calling && handler_) {
+    handler_->ResetCancel();
   }
   state_ = State::Closed;
-  if (cancel_check_) {
-    ENVOY_LOG(debug, "Cancelling check call");
-    cancel_check_();
-    cancel_check_ = nullptr;
+  if (handler_) {
+    handler_->CancelCheck();
   }
 }
 
@@ -61,7 +59,7 @@ void Filter::callCheck() {
   state_ = State::Calling;
   filter_callbacks_->connection().readDisable(true);
   calling_check_ = true;
-  cancel_check_ = handler_->Check(
+  handler_->Check(
       this, [this](const CheckResponseInfo &info) { completeCheck(info); });
   calling_check_ = false;
 }
@@ -140,7 +138,7 @@ Network::FilterStatus Filter::onNewConnection() {
 void Filter::completeCheck(const CheckResponseInfo &info) {
   const auto &status = info.status();
   ENVOY_LOG(debug, "Called tcp filter completeCheck: {}", status.ToString());
-  cancel_check_ = nullptr;
+  handler_->ResetCancel();
   if (state_ == State::Closed) {
     return;
   }

src/envoy/tcp/mixer/filter.h

@@ -86,8 +86,6 @@ class Filter : public Network::Filter,
   // Cancel the pending Check call.
   void cancelCheck();
 
-  // the cancel check
-  istio::mixerclient::CancelFunc cancel_check_;
   // the control object.
   Control &control_;
   // pre-request handler

src/envoy/utils/utils_test.cc

@@ -51,7 +51,7 @@ TEST(UtilsTest, ParseMessageWithUnknownField) {
 TEST(UtilsTest, CheckResponseInfoToStreamInfo) {
   auto attributes = std::make_shared<::istio::mixerclient::SharedAttributes>();
   ::istio::mixerclient::CheckContext check_response(
-      false /* fail_open */, attributes);  // by default status is unknown
+      0U, false /* fail_open */, attributes);  // by default status is unknown
   Envoy::StreamInfo::MockStreamInfo mock_stream_info;
 
   EXPECT_CALL(

src/istio/control/client_context_base.cc

@@ -31,6 +31,7 @@ using ::istio::mixerclient::MixerClientOptions;
 using ::istio::mixerclient::QuotaOptions;
 using ::istio::mixerclient::ReportOptions;
 using ::istio::mixerclient::Statistics;
+using ::istio::mixerclient::TimerCreateFunc;
 using ::istio::mixerclient::TransportCheckFunc;
 using ::istio::utils::CreateLocalAttributes;
 using ::istio::utils::LocalNode;
@@ -39,6 +40,16 @@ namespace istio {
 namespace control {
 namespace {
 
+static constexpr uint32_t MaxDurationSec = 24 * 60 * 60;
+
+static uint32_t DurationToMsec(const ::google::protobuf::Duration& duration) {
+  uint32_t msec =
+      1000 * (duration.seconds() > MaxDurationSec ? MaxDurationSec
+                                                  : duration.seconds());
+  msec += duration.nanos() / 1000 / 1000;
+  return msec;
+}
+
 CheckOptions GetJustCheckOptions(const TransportConfig& config) {
   if (config.disable_check_cache()) {
     return CheckOptions(0);
@@ -48,9 +59,25 @@ CheckOptions GetJustCheckOptions(const TransportConfig& config) {
 
 CheckOptions GetCheckOptions(const TransportConfig& config) {
   auto options = GetJustCheckOptions(config);
-  if (config.has_network_fail_policy() &&
-      config.network_fail_policy().policy() == NetworkFailPolicy::FAIL_CLOSE) {
-    options.network_fail_open = false;
+  if (config.has_network_fail_policy()) {
+    if (config.network_fail_policy().policy() ==
+        NetworkFailPolicy::FAIL_CLOSE) {
+      options.network_fail_open = false;
+    }
+
+    if (0 <= config.network_fail_policy().max_retry()) {
+      options.retries = config.network_fail_policy().max_retry();
+    }
+
+    if (config.network_fail_policy().has_base_retry_wait()) {
+      options.base_retry_ms =
+          DurationToMsec(config.network_fail_policy().base_retry_wait());
+    }
+
+    if (config.network_fail_policy().has_max_retry_wait()) {
+      options.max_retry_ms =
+          DurationToMsec(config.network_fail_policy().max_retry_wait());
+    }
   }
   return options;
 }
@@ -81,9 +108,10 @@ ClientContextBase::ClientContextBase(const TransportConfig& config,
   mixer_client_ = ::istio::mixerclient::CreateMixerClient(options);
   CreateLocalAttributes(local_node, &local_attributes_);
   network_fail_open_ = options.check_options.network_fail_open;
+  retries_ = options.check_options.retries;
 }
 
-CancelFunc ClientContextBase::SendCheck(
+void ClientContextBase::SendCheck(
     const TransportCheckFunc& transport, const CheckDoneFunc& on_done,
     ::istio::mixerclient::CheckContextSharedPtr& context) {
   MIXER_DEBUG("Check attributes: %s",

src/istio/control/client_context_base.h

@@ -17,6 +17,7 @@
 #define ISTIO_CONTROL_CLIENT_CONTEXT_BASE_H
 
 #include "include/istio/mixerclient/client.h"
+#include "include/istio/mixerclient/timer.h"
 #include "include/istio/utils/attribute_names.h"
 #include "include/istio/utils/local_attributes.h"
 #include "mixer/v1/config/client/client_config.pb.h"
@@ -42,15 +43,15 @@ class ClientContextBase {
       : mixer_client_(std::move(mixer_client)),
         outbound_(outbound),
         local_attributes_(local_attributes),
-        network_fail_open_(false) {}
+        network_fail_open_(false),
+        retries_(0) {}
   // virtual destrutor
   virtual ~ClientContextBase() {}
 
   // Use mixer client object to make a Check call.
-  ::istio::mixerclient::CancelFunc SendCheck(
-      const ::istio::mixerclient::TransportCheckFunc& transport,
-      const ::istio::mixerclient::CheckDoneFunc& on_done,
-      ::istio::mixerclient::CheckContextSharedPtr& check_context);
+  void SendCheck(const ::istio::mixerclient::TransportCheckFunc& transport,
+                 const ::istio::mixerclient::CheckDoneFunc& on_done,
+                 ::istio::mixerclient::CheckContextSharedPtr& check_context);
 
   // Use mixer client object to make a Report call.
   void SendReport(
@@ -66,6 +67,8 @@ class ClientContextBase {
 
   bool NetworkFailOpen() const { return network_fail_open_; }
 
+  uint32_t Retries() const { return retries_; }
+
  private:
   // The mixer client object with check cache and report batch features.
   std::unique_ptr<::istio::mixerclient::MixerClient> mixer_client_;
@@ -77,6 +80,7 @@ class ClientContextBase {
   ::istio::utils::LocalAttributes local_attributes_;
 
   bool network_fail_open_;
+  uint32_t retries_;
 };
 
 }  // namespace control

src/istio/control/http/request_handler_impl.cc

@@ -20,6 +20,7 @@ using ::google::protobuf::util::Status;
 using ::istio::mixerclient::CancelFunc;
 using ::istio::mixerclient::CheckDoneFunc;
 using ::istio::mixerclient::CheckResponseInfo;
+using ::istio::mixerclient::TimerCreateFunc;
 using ::istio::mixerclient::TransportCheckFunc;
 using ::istio::quota_config::Requirement;
 
@@ -31,6 +32,7 @@ RequestHandlerImpl::RequestHandlerImpl(
     std::shared_ptr<ServiceContext> service_context)
     : attributes_(new istio::mixerclient::SharedAttributes()),
       check_context_(new istio::mixerclient::CheckContext(
+          service_context->client_context()->Retries(),
           service_context->client_context()->NetworkFailOpen(), attributes_)),
       service_context_(service_context) {}
 
@@ -61,10 +63,10 @@ void RequestHandlerImpl::AddCheckAttributes(CheckData* check_data) {
   }
 }
 
-CancelFunc RequestHandlerImpl::Check(CheckData* check_data,
-                                     HeaderUpdate* header_update,
-                                     TransportCheckFunc transport,
-                                     CheckDoneFunc on_done) {
+void RequestHandlerImpl::Check(CheckData* check_data,
+                               HeaderUpdate* header_update,
+                               const TransportCheckFunc& transport,
+                               const CheckDoneFunc& on_done) {
   // Forwarded attributes need to be stored regardless Check is needed
   // or not since the header will be updated or removed.
   AddCheckAttributes(check_data);
@@ -75,14 +77,26 @@ CancelFunc RequestHandlerImpl::Check(CheckData* check_data,
   if (!service_context_->enable_mixer_check()) {
     check_context_->setFinalStatus(Status::OK, false);
     on_done(*check_context_);
-    return nullptr;
+    return;
   }
 
   service_context_->AddQuotas(attributes_->attributes(),
                               check_context_->quotaRequirements());
 
-  return service_context_->client_context()->SendCheck(transport, on_done,
-                                                       check_context_);
+  service_context_->client_context()->SendCheck(transport, on_done,
+                                                check_context_);
+}
+
+void RequestHandlerImpl::ResetCancel() {
+  if (check_context_) {
+    check_context_->resetCancel();
+  }
+}
+
+void RequestHandlerImpl::CancelCheck() {
+  if (check_context_) {
+    check_context_->cancel();
+  }
 }
 
 // Make remote report call.

src/istio/control/http/request_handler_impl.h

@@ -30,11 +30,16 @@ class RequestHandlerImpl : public RequestHandler {
  public:
   RequestHandlerImpl(std::shared_ptr<ServiceContext> service_context);
 
+  virtual ~RequestHandlerImpl() = default;
+
   // Makes a Check call.
-  ::istio::mixerclient::CancelFunc Check(
-      CheckData* check_data, HeaderUpdate* header_update,
-      ::istio::mixerclient::TransportCheckFunc transport,
-      ::istio::mixerclient::CheckDoneFunc on_done) override;
+  void Check(CheckData* check_data, HeaderUpdate* header_update,
+             const ::istio::mixerclient::TransportCheckFunc& transport,
+             const ::istio::mixerclient::CheckDoneFunc& on_done) override;
+
+  void ResetCancel() override;
+
+  void CancelCheck() override;
 
   // Make a Report call.
   void Report(CheckData* check_data, ReportData* report_data) override;