-
Notifications
You must be signed in to change notification settings - Fork 5.5k
ratelimit: add support for failure_mode_allow configuration #4073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 13 commits
f0a6a88
84b081a
88e49e6
83e75ec
ef281ad
91439ea
2934cc4
37b6913
a6ca282
6296e00
3289db0
cf357e2
d561b19
4404df2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
|
|
||
| import "validate/validate.proto"; | ||
| import "gogoproto/gogo.proto"; | ||
|
|
@@ -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 not allow traffic in case of | ||
| // communication failure between rate limiting service and the proxy. | ||
| // Defaults to false. | ||
| bool failure_mode_deny = 5; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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." | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. failure_mode_deny |
||
|
|
||
| Runtime | ||
| ------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| ------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to take a look in the morning.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great Catch. I see the problem. The hex value I gave for |
||
| }; | ||
|
|
||
| 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"); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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