Revert ac0bd74 (envoyproxy#4295)

This reverts commit ac0bd74. But leaves the API changes as 'not implemented' in order to not scramble the proto field.

envoyproxy#4073 had a bug. The cause has been identified, and a fix PR is forthcoming. However, in the meantime, we want to leave master clean.

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", "RLSE"]
+    in: ["LH", "UH", "UT", "LR", "UR", "UF", "UC", "UO", "NR", "DI", "FI", "RL", "UAEX"]
   }];
 }

api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto

@@ -35,6 +35,7 @@ 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.

api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto

@@ -27,6 +27,7 @@ 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.

api/envoy/data/accesslog/v2/accesslog.proto

@@ -185,6 +185,7 @@ message ResponseFlags {
   // Indicates if the request was deemed unauthorized and the reason for it.
   Unauthorized unauthorized_details = 13;
 
+  // [#not-implemented-hide:] Hide from docs.
   // Indicates that the request was rejected because there was an error in rate limit service.
   bool rate_limit_service_error = 14;
 }

docs/root/configuration/http_filters/rate_limit_filter.rst

@@ -16,9 +16,6 @@ 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
@@ -111,8 +108,6 @@ 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
 -------

docs/root/configuration/network_filters/rate_limit_filter.rst

@@ -25,8 +25,6 @@ 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
 -------

include/envoy/request_info/request_info.h

@@ -48,10 +48,8 @@ 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 = RateLimitServiceError
+  LastFlag = UnauthorizedExternalService
 };
 
 /**

source/common/request_info/utility.cc

@@ -19,7 +19,6 @@ 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()) {
@@ -32,7 +31,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 == 0x2000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
 
   if (request_info.hasResponseFlag(ResponseFlag::FailedLocalHealthCheck)) {
     appendString(result, FAILED_LOCAL_HEALTH_CHECK);
@@ -86,10 +85,6 @@ 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;
 }
 
@@ -109,7 +104,6 @@ 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()) {

source/common/request_info/utility.h

@@ -34,7 +34,6 @@ 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;
 };
 
 /**

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 == 0x2000,
+  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,
                 "A flag has been added. Fix this code.");
 
   if (request_info.hasResponseFlag(RequestInfo::ResponseFlag::FailedLocalHealthCheck)) {
@@ -141,10 +141,6 @@ 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,

source/extensions/filters/http/ratelimit/ratelimit.cc

@@ -147,15 +147,6 @@ 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();
-      callbacks_->continueDecoding();
-    } else {
-      state_ = State::Responded;
-      callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr);
-      callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError);
-    }
   } else if (!initiating_call_) {
     callbacks_->continueDecoding();
   }

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),
-        failure_mode_deny_(config.failure_mode_deny()) {}
+        local_info_(local_info), scope_(scope), runtime_(runtime), cm_(cm) {}
+
   const std::string& domain() const { return domain_; }
   const LocalInfo::LocalInfo& localInfo() const { return local_info_; }
   uint64_t stage() const { return stage_; }
@@ -47,8 +47,6 @@ 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") {
@@ -68,7 +66,6 @@ class FilterConfig {
   Stats::Scope& scope_;
   Runtime::Loader& runtime_;
   Upstream::ClusterManager& cm_;
-  const bool failure_mode_deny_;
 };
 
 typedef std::shared_ptr<FilterConfig> FilterConfigSharedPtr;

source/extensions/filters/network/ratelimit/ratelimit.cc

@@ -16,7 +16,8 @@ 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), failure_mode_deny_(config.failure_mode_deny()) {
+      runtime_(runtime) {
+
   for (const auto& descriptor : config.descriptors()) {
     RateLimit::Descriptor new_descriptor;
     for (const auto& entry : descriptor.entries()) {
@@ -84,18 +85,11 @@ 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();
-      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_) {

source/extensions/filters/network/ratelimit/ratelimit.h

@@ -27,7 +27,6 @@ namespace RateLimitFilter {
   COUNTER(error)                                                                                   \
   COUNTER(over_limit)                                                                              \
   COUNTER(ok)                                                                                      \
-  COUNTER(failure_mode_allowed)                                                                    \
   COUNTER(cx_closed)                                                                               \
   GAUGE  (active)
 // clang-format on
@@ -50,7 +49,6 @@ 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);
@@ -59,7 +57,6 @@ class Config {
   std::vector<RateLimit::Descriptor> descriptors_;
   const InstanceStats stats_;
   Runtime::Loader& runtime_;
-  const bool failure_mode_deny_;
 };
 
 typedef std::shared_ptr<Config> ConfigSharedPtr;

test/common/access_log/access_log_impl_test.cc

@@ -813,12 +813,11 @@ name: envoy.file_access_log
       - FI
       - RL
       - UAEX
-      - RLSE
 config:
   path: /dev/null
   )EOF";
 
-  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x2000,
+  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,
                 "A flag has been added. Fix this code.");
 
   std::vector<RequestInfo::ResponseFlag> all_response_flags = {
@@ -835,7 +834,6 @@ name: envoy.file_access_log
       RequestInfo::ResponseFlag::FaultInjected,
       RequestInfo::ResponseFlag::RateLimited,
       RequestInfo::ResponseFlag::UnauthorizedExternalService,
-      RequestInfo::ResponseFlag::RateLimitServiceError,
   };
 
   InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_);
@@ -865,7 +863,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\" \"RLSE\"]]): "
+      "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\"]]): "
       "response_flag_filter {\n  flags: \"UnsupportedFlag\"\n}\n");
 }
 

test/common/request_info/utility_test.cc

@@ -14,7 +14,7 @@ namespace Envoy {
 namespace RequestInfo {
 
 TEST(ResponseFlagUtilsTest, toShortStringConversion) {
-  static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
 
   std::vector<std::pair<ResponseFlag, std::string>> expected = {
       std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"),
@@ -30,7 +30,6 @@ 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) {
@@ -59,7 +58,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) {
 }
 
 TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) {
-  static_assert(ResponseFlag::LastFlag == 0x2000, "A flag has been added. Fix this code.");
+  static_assert(ResponseFlag::LastFlag == 0x1000, "A flag has been added. Fix this code.");
 
   std::vector<std::pair<std::string, ResponseFlag>> expected = {
       std::make_pair("LH", ResponseFlag::FailedLocalHealthCheck),
@@ -75,7 +74,6 @@ 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());

test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc

@@ -434,7 +434,6 @@ 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());
 }

test/extensions/filters/http/ratelimit/ratelimit_test.cc

@@ -46,10 +46,10 @@ class HttpRateLimitFilterTest : public testing::Test {
         .WillByDefault(Return(true));
   }
 
-  void SetUpTest(const std::string& yaml) {
+  void SetUpTest(const std::string json) {
+    Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json);
     envoy::config::filter::http::rate_limit::v2::RateLimit proto_config{};
-    MessageUtil::loadFromYaml(yaml, proto_config);
-
+    Config::FilterJson::translateHttpRateLimitFilter(*json_config, proto_config);
     config_.reset(new FilterConfig(proto_config, local_info_, stats_store_, runtime_, cm_));
 
     client_ = new RateLimit::MockClient();
@@ -64,13 +64,10 @@ class HttpRateLimitFilterTest : public testing::Test {
         .emplace_back(vh_rate_limit_);
   }
 
-  const std::string fail_close_config_ = R"EOF(
-  domain: foo
-  failure_mode_deny: true
-  )EOF";
-
   const std::string filter_config_ = R"EOF(
-  domain: foo
+  {
+    "domain": "foo"
+  }
   )EOF";
 
   FilterConfigSharedPtr config_;
@@ -331,37 +328,6 @@ TEST_F(HttpRateLimitFilterTest, ErrorResponse) {
   EXPECT_EQ(
       1U,
       cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value());
-  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("ratelimit.failure_mode_allowed")
-                    .value());
-}
-
-TEST_F(HttpRateLimitFilterTest, ErrorResponseWithFailureModeAllowOff) {
-  SetUpTest(fail_close_config_);
-  InSequence s;
-
-  EXPECT_CALL(route_rate_limit_, populateDescriptors(_, _, _, _, _))
-      .WillOnce(SetArgReferee<1>(descriptor_));
-  EXPECT_CALL(*client_, limit(_, _, _, _))
-      .WillOnce(WithArgs<0>(Invoke([&](RateLimit::RequestCallbacks& callbacks) -> void {
-        request_callbacks_ = &callbacks;
-      })));
-
-  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
-            filter_->decodeHeaders(request_headers_, false));
-
-  request_callbacks_->complete(RateLimit::LimitStatus::Error, nullptr);
-
-  EXPECT_CALL(filter_callbacks_.request_info_,
-              setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError))
-      .Times(0);
-
-  EXPECT_EQ(
-      1U,
-      cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("ratelimit.error").value());
-  EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("ratelimit.failure_mode_allowed")
-                    .value());
 }
 
 TEST_F(HttpRateLimitFilterTest, LimitResponse) {