Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/envoy/config/filter/accesslog/v2/accesslog.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
}];
}
7 changes: 7 additions & 0 deletions api/envoy/config/filter/http/rate_limit/v2/rate_limit.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package envoy.config.filter.http.rate_limit.v2;
option go_package = "v2";

import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this isn't needed anymore


import "validate/validate.proto";
import "gogoproto/gogo.proto";
Expand Down Expand Up @@ -34,4 +35,10 @@ message RateLimit {
// The timeout in milliseconds for the rate limit service RPC. If not
// set, this defaults to 20ms.
google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true];

// The filter's behaviour in case the rate limiting service does
// not respond back. When it is set to true, Envoy will also allow traffic in case of
// communication failure between rate limiting service and the proxy.
// Defaults to true.
google.protobuf.BoolValue failure_mode_allow = 5;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's preferable to use bool instead of g.p.BoolValue. Can you change this to type bool and name failure_mode_deny (so that the default value is false)? Or is there a good reason for making this a tri-state?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no other reason except to be consistent with external_authz proto naming. I will change.

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ option go_package = "v2";

import "envoy/api/v2/ratelimit/ratelimit.proto";
import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


import "validate/validate.proto";
import "gogoproto/gogo.proto";
Expand All @@ -26,4 +27,10 @@ message RateLimit {
// The timeout in milliseconds for the rate limit service RPC. If not
// set, this defaults to 20ms.
google.protobuf.Duration timeout = 4 [(gogoproto.stdduration) = true];

// The filter's behaviour in case the rate limiting service does
// not respond back. When it is set to true, Envoy will also allow traffic in case of
// communication failure between rate limiting service and the proxy.
// Defaults to true.
google.protobuf.BoolValue failure_mode_allow = 5;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
3 changes: 3 additions & 0 deletions api/envoy/data/accesslog/v2/accesslog.proto
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ message ResponseFlags {

// Indicates if the request was deemed unauthorized and the reason for it.
Unauthorized unauthorized_details = 13;

// Indicates that the request was rejected because there was an error in rate limit service.
bool rate_limit_service_error = 14;
}

// [#not-implemented-hide:]
Expand Down
5 changes: 5 additions & 0 deletions docs/root/configuration/http_filters/rate_limit_filter.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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_allow <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failure_mode_deny

set to false, a 500 response is returned.

.. _config_http_filters_rate_limit_composing_actions:

Composing Actions
Expand Down Expand Up @@ -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_allow <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` set to true."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failure_mode_deny


Runtime
-------
Expand Down
2 changes: 2 additions & 0 deletions docs/root/configuration/network_filters/rate_limit_filter.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failure_mode_deny: I'm fine with the stat being named either way, but the doc link needs to be updated.

of :ref:`failure_mode_allow <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` set to true."

Runtime
-------
Expand Down
2 changes: 2 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Version history
* upstream: require opt-in to use the :ref:`x-envoy-orignal-dst-host <config_http_conn_man_headers_x-envoy-original-dst-host>` header
for overriding destination address when using the :ref:`Original Destination <arch_overview_load_balancing_types_original_destination>`
load balancing policy.
* ratelimit: added :ref:`failure_mode_allow <envoy_api_msg_config.filter.http.rate_limit.v2.RateLimit>` option to control traffic flow in
case of rate limit service error.

1.7.0
===============
Expand Down
4 changes: 3 additions & 1 deletion include/envoy/request_info/request_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

/**
Expand Down
8 changes: 7 additions & 1 deletion source/common/request_info/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}

Expand All @@ -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()) {
Expand Down
1 change: 1 addition & 0 deletions source/common/request_info/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 9 additions & 0 deletions source/extensions/filters/http/ratelimit/ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ 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()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change the method name in config because it seems more readable like this. LMK if you think otherwise

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as is

cluster_->statsScope().counter("ratelimit.failure_mode_allowed").inc();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this new counter at all? We already have a counter for ratelimit service failures, and presumably the user knows which way they have this configured (failure_mode_deny value). In one case this counter will always be zero, in the other it will be equal to ratelimit.error. So is there value in this new counter?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would help in general, while looking at stats user do not have to check back on what his configuration is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm fine with that.

callbacks_->continueDecoding();
} else {
state_ = State::Responded;
callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr);
callbacks_->requestInfo().setResponseFlag(RequestInfo::ResponseFlag::RateLimitServiceError);
}
} else if (!initiating_call_) {
callbacks_->continueDecoding();
}
Expand Down
7 changes: 5 additions & 2 deletions source/extensions/filters/http/ratelimit/ratelimit.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_allow_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, failure_mode_allow, true)) {}
const std::string& domain() const { return domain_; }
const LocalInfo::LocalInfo& localInfo() const { return local_info_; }
uint64_t stage() const { return stage_; }
Expand All @@ -47,6 +47,8 @@ class FilterConfig {
Upstream::ClusterManager& cm() { return cm_; }
FilterRequestType requestType() const { return request_type_; }

bool failureModeAllow() const { return failure_mode_allow_; }

private:
static FilterRequestType stringToType(const std::string& request_type) {
if (request_type == "internal") {
Expand All @@ -66,6 +68,7 @@ class FilterConfig {
Stats::Scope& scope_;
Runtime::Loader& runtime_;
Upstream::ClusterManager& cm_;
const bool failure_mode_allow_;
};

typedef std::shared_ptr<FilterConfig> FilterConfigSharedPtr;
Expand Down
13 changes: 10 additions & 3 deletions source/extensions/filters/network/ratelimit/ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +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) {

runtime_(runtime),
failure_mode_allow_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, failure_mode_allow, true)) {
for (const auto& descriptor : config.descriptors()) {
RateLimit::Descriptor new_descriptor;
for (const auto& entry : descriptor.entries()) {
Expand Down Expand Up @@ -85,11 +85,18 @@ 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_) {
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/filters/network/ratelimit/ratelimit.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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_allow_; };

private:
static InstanceStats generateStats(const std::string& name, Stats::Scope& scope);
Expand All @@ -57,6 +59,7 @@ class Config {
std::vector<RateLimit::Descriptor> descriptors_;
const InstanceStats stats_;
Runtime::Loader& runtime_;
const bool failure_mode_allow_;
};

typedef std::shared_ptr<Config> ConfigSharedPtr;
Expand Down
6 changes: 4 additions & 2 deletions test/common/access_log/access_log_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -834,6 +835,7 @@ name: envoy.file_access_log
RequestInfo::ResponseFlag::FaultInjected,
RequestInfo::ResponseFlag::RateLimited,
RequestInfo::ResponseFlag::UnauthorizedExternalService,
RequestInfo::ResponseFlag::RateLimitServiceError,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. This test should fail due to unmatched expectation here, because the new flag was not added to the filter config here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to take a look in the morning.

@ramaraochavali ramaraochavali Aug 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Catch. I see the problem. The hex value I gave for RateLimitServiceError is incorrect so the intersectResponseFlags is returning wrong value. I have corrected it.

};

InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_);
Expand Down Expand Up @@ -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");
}

Expand Down
6 changes: 4 additions & 2 deletions test/common/request_info/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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) {
Expand Down Expand Up @@ -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),
Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
Loading