ratelimit: revert revert rate limit failure mode config and add tests (envoyproxy#4303)

ramaraochavali · ggreenway · commit a8572192f2e5 · 2018-08-31T09:54:00.000-07:00
Signed-off-by: Rama &lt;rama.rao@salesforce.com&gt;
diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto
@@ -162,6 +162,6 @@ message ResponseFlagFilter {
   // This field is optional. If it is not specified, then any response flag will pass
   // the filter check.
   repeated string flags = 1 [(validate.rules).repeated .items.string = {
-    in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"]
+    in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX", "RLSE"]
   }];
 }
diff --git a/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto b/api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto
@@ -35,7 +35,6 @@ message RateLimit {
   // set, this defaults to 20ms.
   google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true];
 
-  // [#not-implemented-hide:] Hide from docs.
   // The filter's behaviour in case the rate limiting service does
   // not respond back. When it is set to true, Envoy will not allow traffic in case of
   // communication failure between rate limiting service and the proxy.
diff --git a/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto b/api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto
@@ -27,7 +27,6 @@ message RateLimit {
   // set, this defaults to 20ms.
   google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true];
 
-  // [#not-implemented-hide:] Hide from docs.
   // The filter's behaviour in case the rate limiting service does
   // not respond back. When it is set to true, Envoy will not allow traffic in case of
   // communication failure between rate limiting service and the proxy.
diff --git a/docs/root/configuration/http_filters/rate_limit_filter.rst b/docs/root/configuration/http_filters/rate_limit_filter.rst
@@ -16,6 +16,9 @@ apply to a request. Each configuration results in a descriptor being sent to the
 If the rate limit service is called, and the response for any of the descriptors is over limit, a
 429 response is returned.
 
+If there is an error in calling rate limit service or rate limit service returns an error and :ref:`failure_mode_deny <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` is 
+set to true, a 500 response is returned.
+
 .. _config_http_filters_rate_limit_composing_actions:
 
 Composing Actions
@@ -108,6 +111,8 @@ The buffer filter outputs statistics in the *cluster.<route target cluster>.rate
   ok, Counter, Total under limit responses from the rate limit service
   error, Counter, Total errors contacting the rate limit service
   over_limit, Counter, total over limit responses from the rate limit service
+  failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because
+  of :ref:`failure_mode_deny <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` set to false."
 
 Runtime
 -------
diff --git a/docs/root/configuration/network_filters/rate_limit_filter.rst b/docs/root/configuration/network_filters/rate_limit_filter.rst
@@ -25,6 +25,8 @@ following statistics:
   ok, Counter, Total under limit responses from the rate limit service
   cx_closed, Counter, Total connections closed due to an over limit response from the rate limit service
   active, Gauge, Total active requests to the rate limit service
+  failure_mode_allowed, Counter, "Total requests that were error(s) but were allowed through because
+  of :ref:`failure_mode_deny <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` set to false."
 
 Runtime
 -------
diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h
@@ -48,8 +48,10 @@ enum ResponseFlag {
   RateLimited = 0x800,
   // Request was unauthorized by external authorization service.
   UnauthorizedExternalService = 0x1000,
+  // Unable to call Ratelimit service.
+  RateLimitServiceError = 0x2000,
   // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG.
-  LastFlag = UnauthorizedExternalService
+  LastFlag = RateLimitServiceError
 };
 
 /**
diff --git a/source/common/request_info/utility.cc b/source/common/request_info/utility.cc
@@ -19,6 +19,7 @@ const std::string ResponseFlagUtils::DELAY_INJECTED = "DI";
 const std::string ResponseFlagUtils::FAULT_INJECTED = "FI";
 const std::string ResponseFlagUtils::RATE_LIMITED = "RL";
 const std::string ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE = "UAEX";
+const std::string ResponseFlagUtils::RATELIMIT_SERVICE_ERROR = "RLSE";
 
 void ResponseFlagUtils::appendString(std::string& result, const std::string& append) {
   if (result.empty()) {
@@ -31,7 +32,7 @@ void ResponseFlagUtils::appendString(std::string& result, const std::string& app
 const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_info) {
   std::string result;
 
-  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code.");
 
   if (request_info.hasResponseFlag(ResponseFlag::FailedLocalHealthCheck)) {
     appendString(result, FAILED_LOCAL_HEALTH_CHECK);
@@ -85,6 +86,10 @@ const std::string ResponseFlagUtils::toShortString(const RequestInfo& request_in
     appendString(result, UNAUTHORIZED_EXTERNAL_SERVICE);
   }
 
+  if (request_info.hasResponseFlag(ResponseFlag::RateLimitServiceError)) {
+    appendString(result, RATELIMIT_SERVICE_ERROR);
+  }
+
   return result.empty() ? NONE : result;
 }
 
@@ -104,6 +109,7 @@ absl::optional<ResponseFlag> ResponseFlagUtils::toResponseFlag(const std::string
       {ResponseFlagUtils::FAULT_INJECTED, ResponseFlag::FaultInjected},
       {ResponseFlagUtils::RATE_LIMITED, ResponseFlag::RateLimited},
       {ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE, ResponseFlag::UnauthorizedExternalService},
+      {ResponseFlagUtils::RATELIMIT_SERVICE_ERROR, ResponseFlag::RateLimitServiceError},
   };
   const auto& it = map.find(flag);
   if (it != map.end()) {
diff --git a/source/common/request_info/utility.h b/source/common/request_info/utility.h
@@ -34,6 +34,7 @@ class ResponseFlagUtils {
   const static std::string FAULT_INJECTED;
   const static std::string RATE_LIMITED;
   const static std::string UNAUTHORIZED_EXTERNAL_SERVICE;
+  const static std::string RATELIMIT_SERVICE_ERROR;
 };
 
 /**
diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc
@@ -85,7 +85,7 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags(
     envoy::data::accesslog::v2::AccessLogCommon& common_access_log,
     const RequestInfo::RequestInfo& request_info) {
 
-  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,
+  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x2000,
                 "A flag has been added. Fix this code.");
 
   if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) {
@@ -141,6 +141,10 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags(
         envoy::data::accesslog::v2::ResponseFlags_Unauthorized_Reason::
             ResponseFlags_Unauthorized_Reason_EXTERNAL_SERVICE);
   }
+
+  if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError)) {
+    common_access_log.mutable_response_flags()->set_rate_limit_service_error(true);
+  }
 }
 
 void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers,
diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc
@@ -147,6 +147,17 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&& header
     callbacks_->sendLocalReply(Http::Code::TooManyRequests, "",
                                [this](Http::HeaderMap& headers) { addHeaders(headers); });
     callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimited);
+  } else if (status == RateLimit::LimitStatus::Error) {
+    if (config_->failureModeAllow()) {
+      cluster_->statsScope().counter("ratelimit.failure_mode_allowed").inc();
+      if (!initiating_call_) {
+        callbacks_->continueDecoding();
+      }
+    } else {
+      state_ = State::Responded;
+      callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr);
+      callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError);
+    }
   } else if (!initiating_call_) {
     callbacks_->continueDecoding();
   }
diff --git a/source/extensions/filters/http/ratelimit/ratelimit.h b/source/extensions/filters/http/ratelimit/ratelimit.h
@@ -37,8 +37,8 @@ class FilterConfig {
       : domain_(config.domain()), stage_(static_cast<uint64_t>(config.stage())),
         request_type_(config.request_type().empty() ? stringToType("both")
                                                     : stringToType(config.request_type())),
-        local_info_(local_info), scope_(scope), runtime_(runtime), cm_(cm) {}
-
+        local_info_(local_info), scope_(scope), runtime_(runtime), cm_(cm),
+        failure_mode_deny_(config.failure_mode_deny()) {}
   const std::string& domain() const { return domain_; }
   const LocalInfo::LocalInfo& localInfo() const { return local_info_; }
   uint64_t stage() const { return stage_; }
@@ -47,6 +47,8 @@ class FilterConfig {
   Upstream::ClusterManager& cm() { return cm_; }
   FilterRequestType requestType() const { return request_type_; }
 
+  bool failureModeAllow() const { return !failure_mode_deny_; }
+
 private:
   static FilterRequestType stringToType(const std::string& request_type) {
     if (request_type == "internal") {
@@ -66,6 +68,7 @@ class FilterConfig {
   Stats::Scope& scope_;
   Runtime::Loader& runtime_;
   Upstream::ClusterManager& cm_;
+  const bool failure_mode_deny_;
 };
 
 typedef std::shared_ptr<FilterConfig> FilterConfigSharedPtr;
diff --git a/source/extensions/filters/network/ratelimit/ratelimit.cc b/source/extensions/filters/network/ratelimit/ratelimit.cc
@@ -16,8 +16,7 @@ namespace RateLimitFilter {
 Config::Config(const envoy::config::filter::network::rate_limit::v2::RateLimit& config,
                Stats::Scope& scope, Runtime::Loader& runtime)
     : domain_(config.domain()), stats_(generateStats(config.stat_prefix(), scope)),
-      runtime_(runtime) {
-
+      runtime_(runtime), failure_mode_deny_(config.failure_mode_deny()) {
   for (const auto& descriptor : config.descriptors()) {
     RateLimit::Descriptor new_descriptor;
     for (const auto& entry : descriptor.entries()) {
@@ -85,11 +84,20 @@ void Filter::complete(RateLimit::LimitStatus status, Http::HeaderMapPtr&&) {
     break;
   }
 
-  // We fail open if there is an error contacting the service.
   if (status == RateLimit::LimitStatus::OverLimit &&
       config_->runtime().snapshot().featureEnabled("ratelimit.tcp_filter_enforcing", 100)) {
     config_->stats().cx_closed_.inc();
     filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
+  } else if (status == RateLimit::LimitStatus::Error) {
+    if (config_->failureModeAllow()) {
+      config_->stats().failure_mode_allowed_.inc();
+      if (!calling_limit_) {
+        filter_callbacks_->continueReading();
+      }
+    } else {
+      config_->stats().cx_closed_.inc();
+      filter_callbacks_->connection().close(Network::ConnectionCloseType::NoFlush);
+    }
   } else {
     // We can get completion inline, so only call continue if that isn't happening.
     if (!calling_limit_) {
diff --git a/source/extensions/filters/network/ratelimit/ratelimit.h b/source/extensions/filters/network/ratelimit/ratelimit.h
@@ -27,6 +27,7 @@ namespace RateLimitFilter {
   COUNTER(error)                                                                                   \
   COUNTER(over_limit)                                                                              \
   COUNTER(ok)                                                                                      \
+  COUNTER(failure_mode_allowed)                                                                    \
   COUNTER(cx_closed)                                                                               \
   GAUGE  (active)
 // clang-format on
@@ -49,6 +50,7 @@ class Config {
   const std::vector<RateLimit::Descriptor>& descriptors() { return descriptors_; }
   Runtime::Loader& runtime() { return runtime_; }
   const InstanceStats& stats() { return stats_; }
+  bool failureModeAllow() const { return !failure_mode_deny_; };
 
 private:
   static InstanceStats generateStats(const std::string& name, Stats::Scope& scope);
@@ -57,6 +59,7 @@ class Config {
   std::vector<RateLimit::Descriptor> descriptors_;
   const InstanceStats stats_;
   Runtime::Loader& runtime_;
+  const bool failure_mode_deny_;
 };
 
 typedef std::shared_ptr<Config> ConfigSharedPtr;
diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc
@@ -813,11 +813,12 @@ name: envoy.file_access_log
       - FI
       - RL
       - UAEX
+      - RLSE
 config:
   path: /dev/null
   )EOF";
 
-  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,
+  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x2000,
                 "A flag has been added. Fix this code.");
 
   std::vector<RequestInfo::ResponseFlag> all_response_flags = {
@@ -834,6 +835,7 @@ name: envoy.file_access_log
       RequestInfo::ResponseFlag::FaultInjected,
       RequestInfo::ResponseFlag::RateLimited,
       RequestInfo::ResponseFlag::UnauthorizedExternalService,
+      RequestInfo::ResponseFlag::RateLimitServiceError,
   };
 
   InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_);
@@ -863,7 +865,7 @@ name: envoy.file_access_log
       "Proto constraint validation failed (AccessLogFilterValidationError.ResponseFlagFilter: "
       "[\"embedded message failed validation\"] | caused by "
       "ResponseFlagFilterValidationError.Flags[i]: [\"value must be in list \" [\"LH\" \"UH\" "
-      "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\"]]): "
+      "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\" \"RLSE\"]]): "
       "response_flag_filter {\n  flags: \"UnsupportedFlag\"\n}\n");
 }
 
diff --git a/test/common/request_info/utility_test.cc b/test/common/request_info/utility_test.cc
@@ -14,7 +14,7 @@ namespace Envoy {
 namespace RequestInfo {
 
 TEST(ResponseFlagUtilsTest, toShortStringConversion) {
-  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code.");
 
   std::vector<std::pair<ResponseFlag, std::string>> expected = {
       std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"),
@@ -30,6 +30,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) {
       std::make_pair(ResponseFlag::FaultInjected, "FI"),
       std::make_pair(ResponseFlag::RateLimited, "RL"),
       std::make_pair(ResponseFlag::UnauthorizedExternalService, "UAEX"),
+      std::make_pair(ResponseFlag::RateLimitServiceError, "RLSE"),
   };
 
   for (const auto& test_case : expected) {
@@ -58,7 +59,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) {
 }
 
 TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) {
-  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code.");
 
   std::vector<std::pair<std::string, ResponseFlag>> expected = {
       std::make_pair("LH", ResponseFlag::FailedLocalHealthCheck),
@@ -74,6 +75,7 @@ TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) {
       std::make_pair("FI", ResponseFlag::FaultInjected),
       std::make_pair("RL", ResponseFlag::RateLimited),
       std::make_pair("UAEX", ResponseFlag::UnauthorizedExternalService),
+      std::make_pair("RLSE", ResponseFlag::RateLimitServiceError),
   };
 
   EXPECT_FALSE(ResponseFlagUtils::toResponseFlag("NonExistentFlag").has_value());
diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc
@@ -434,6 +434,7 @@ TEST(responseFlagsToAccessLogResponseFlagsTest, All) {
   common_access_log_expected.mutable_response_flags()->mutable_unauthorized_details()->set_reason(
       envoy::data::accesslog::v2::ResponseFlags_Unauthorized_Reason::
           ResponseFlags_Unauthorized_Reason_EXTERNAL_SERVICE);
+  common_access_log_expected.mutable_response_flags()->set_rate_limit_service_error(true);
 
   EXPECT_EQ(common_access_log_expected.DebugString(), common_access_log.DebugString());
 }
diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc
diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc
diff --git a/test/integration/ratelimit_integration_test.cc b/test/integration/ratelimit_integration_test.cc

Original file line number	Diff line number	Diff line change
`@@ -162,6 +162,6 @@ message ResponseFlagFilter {`
`162`	`162`	`// This field is optional. If it is not specified, then any response flag will pass`
`163`	`163`	`// the filter check.`
`164`	`164`	`repeated string flags = 1 [(validate.rules).repeated .items.string = {`
`165`		`- in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"]`
	`165`	`+ in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX", "RLSE"]`
`166`	`166`	`}];`
`167`	`167`	`}`