-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
local rate limit: add new rate_limits support to the filter #36099
Changes from 23 commits
0090d86
111aaf7
90da99b
68df70c
e9f5510
cddefa7
0dd7f9b
5c01cd5
26efa09
085b3aa
3638131
a573560
21a569c
cd7fa92
4a776f1
6677aa4
4ad79d2
a3e2f62
de1a61d
135975e
3d5429a
384970f
7cf2a8b
3b8d8b7
e9893ed
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 |
---|---|---|
|
@@ -3,6 +3,7 @@ syntax = "proto3"; | |
package envoy.extensions.filters.http.local_ratelimit.v3; | ||
|
||
import "envoy/config/core/v3/base.proto"; | ||
import "envoy/config/route/v3/route_components.proto"; | ||
import "envoy/extensions/common/ratelimit/v3/ratelimit.proto"; | ||
import "envoy/type/v3/http_status.proto"; | ||
import "envoy/type/v3/token_bucket.proto"; | ||
|
@@ -22,7 +23,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |
// Local Rate limit :ref:`configuration overview <config_http_filters_local_rate_limit>`. | ||
// [#extension: envoy.filters.http.local_ratelimit] | ||
|
||
// [#next-free-field: 17] | ||
// [#next-free-field: 18] | ||
message LocalRateLimit { | ||
// The human readable prefix to use when emitting stats. | ||
string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; | ||
|
@@ -147,4 +148,23 @@ message LocalRateLimit { | |
// of the default ``UNAVAILABLE`` gRPC code for a rate limited gRPC call. The | ||
// HTTP code will be 200 for a gRPC response. | ||
bool rate_limited_as_resource_exhausted = 15; | ||
|
||
// Rate limit configuration that is used to generate a list of descriptor entries based on | ||
// the request context. The generated entries will be used to find one or multiple matched rate | ||
// limit rule from the ``descriptors``. | ||
// If this is set, then | ||
// :ref:`VirtualHost.rate_limits<envoy_v3_api_field_config.route.v3.VirtualHost.rate_limits>` or | ||
// :ref:`RouteAction.rate_limits<envoy_v3_api_field_config.route.v3.RouteAction.rate_limits>` fields | ||
Comment on lines
+156
to
+157
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 should probably be mentioned in those fields as well. 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. Not sure I get it correctly? Did you mean we should add some comments to the 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. Check the fields documents again, the legacy For users who doesn't use this new 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. Maybe it will be possible to add a warning when there are fields that are ignored. 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.
Sorry, could you make this a little more clear? should I add warning in the runtime log or the comments? 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 meant the log. The example I'm thinking of is that if the config contains both the new rate-limit filter config and VirtualHost.rate_limits (for example), then Envoy will just warn that the VirtualHost.rate_limits is ignored in favor of the filter config. |
||
// will be ignored. | ||
// | ||
// .. note:: | ||
// Not all configuration fields of | ||
// :ref:`rate limit config <envoy_v3_api_msg_config.route.v3.RateLimit>` is supported at here. | ||
// Following fields are not supported: | ||
// | ||
// 1. :ref:`rate limit stage <envoy_v3_api_field_config.route.v3.RateLimit.stage>`. | ||
// 2. :ref:`dynamic metadata <envoy_v3_api_field_config.route.v3.RateLimit.Action.dynamic_metadata>`. | ||
// 3. :ref:`disable_key <envoy_v3_api_field_config.route.v3.RateLimit.disable_key>`. | ||
// 4. :ref:`override limit <envoy_v3_api_field_config.route.v3.RateLimit.limit>`. | ||
Comment on lines
+165
to
+168
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 guess some of them are not supported because they are not implemented yet, and not because there's some design blocker. 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. Hmmm, I think all these will be ignored forever. This is why l initially copied whole RateLimit to a new message and removed them in the new message. Because historical reason, at the initial design, the ratelimit configuration is split to two parts: filter configuration and The The The But I think it should not be used as general switch of feature or configuration. XDS is good enough for most cases to update configuration dynamically. 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 it would be good to reject/warn when there's config that has those fields 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 will add a warn log at the implementation. |
||
repeated config.route.v3.RateLimit rate_limits = 17; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
load( | ||
"//bazel:envoy_build_system.bzl", | ||
"envoy_cc_library", | ||
"envoy_extension_package", | ||
) | ||
|
||
licenses(["notice"]) # Apache 2 | ||
|
||
envoy_extension_package() | ||
|
||
envoy_cc_library( | ||
name = "ratelimit_config_lib", | ||
srcs = ["ratelimit_config.cc"], | ||
hdrs = ["ratelimit_config.h"], | ||
deps = [ | ||
"//envoy/ratelimit:ratelimit_interface", | ||
"//source/common/router:router_ratelimit_lib", | ||
"@com_google_absl//absl/container:inlined_vector", | ||
"@com_google_absl//absl/strings", | ||
"@envoy_api//envoy/config/route/v3:pkg_cc_proto", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
#include "source/extensions/filters/common/ratelimit_config/ratelimit_config.h" | ||
|
||
#include "source/common/config/utility.h" | ||
#include "source/common/http/matching/data_impl.h" | ||
#include "source/common/matcher/matcher.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace Filters { | ||
namespace Common { | ||
namespace RateLimit { | ||
|
||
RateLimitPolicy::RateLimitPolicy(const ProtoRateLimit& config, | ||
Server::Configuration::CommonFactoryContext& context, | ||
absl::Status& creation_status, bool no_limit) { | ||
if (config.has_stage() || !config.disable_key().empty()) { | ||
creation_status = | ||
absl::InvalidArgumentError("'stage' field and 'disable_key' field are not supported"); | ||
return; | ||
} | ||
|
||
if (config.has_limit()) { | ||
if (no_limit) { | ||
creation_status = absl::InvalidArgumentError("'limit' field is not supported"); | ||
return; | ||
} | ||
} | ||
|
||
for (const ProtoRateLimit::Action& action : config.actions()) { | ||
switch (action.action_specifier_case()) { | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kSourceCluster: | ||
actions_.emplace_back(new Envoy::Router::SourceClusterAction()); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kDestinationCluster: | ||
actions_.emplace_back(new Envoy::Router::DestinationClusterAction()); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kRequestHeaders: | ||
actions_.emplace_back(new Envoy::Router::RequestHeadersAction(action.request_headers())); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kRemoteAddress: | ||
actions_.emplace_back(new Envoy::Router::RemoteAddressAction()); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kGenericKey: | ||
actions_.emplace_back(new Envoy::Router::GenericKeyAction(action.generic_key())); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kMetadata: | ||
actions_.emplace_back(new Envoy::Router::MetaDataAction(action.metadata())); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kHeaderValueMatch: | ||
actions_.emplace_back( | ||
new Envoy::Router::HeaderValueMatchAction(action.header_value_match(), context)); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kExtension: { | ||
ProtobufMessage::ValidationVisitor& validator = context.messageValidationVisitor(); | ||
auto* factory = | ||
Envoy::Config::Utility::getFactory<Envoy::RateLimit::DescriptorProducerFactory>( | ||
action.extension()); | ||
if (!factory) { | ||
// If no descriptor extension is found, fallback to using HTTP matcher | ||
// input functions. Note that if the same extension name or type was | ||
// dual registered as an extension descriptor and an HTTP matcher input | ||
// function, the descriptor extension takes priority. | ||
Router::RateLimitDescriptorValidationVisitor validation_visitor; | ||
Matcher::MatchInputFactory<Http::HttpMatchingData> input_factory(validator, | ||
validation_visitor); | ||
Matcher::DataInputFactoryCb<Http::HttpMatchingData> data_input_cb = | ||
input_factory.createDataInput(action.extension()); | ||
actions_.emplace_back(std::make_unique<Router::MatchInputRateLimitDescriptor>( | ||
action.extension().name(), data_input_cb())); | ||
break; | ||
} | ||
auto message = Envoy::Config::Utility::translateAnyToFactoryConfig( | ||
action.extension().typed_config(), validator, *factory); | ||
Envoy::RateLimit::DescriptorProducerPtr producer = | ||
factory->createDescriptorProducerFromProto(*message, context); | ||
if (producer) { | ||
actions_.emplace_back(std::move(producer)); | ||
} else { | ||
creation_status = absl::InvalidArgumentError( | ||
absl::StrCat("Rate limit descriptor extension failed: ", action.extension().name())); | ||
return; | ||
} | ||
break; | ||
} | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kMaskedRemoteAddress: | ||
actions_.emplace_back(new Router::MaskedRemoteAddressAction(action.masked_remote_address())); | ||
break; | ||
case ProtoRateLimit::Action::ActionSpecifierCase::kQueryParameterValueMatch: | ||
actions_.emplace_back(new Router::QueryParameterValueMatchAction( | ||
action.query_parameter_value_match(), context)); | ||
break; | ||
default: | ||
creation_status = absl::InvalidArgumentError(fmt::format( | ||
"Unsupported rate limit action: {}", static_cast<int>(action.action_specifier_case()))); | ||
return; | ||
} | ||
} | ||
} | ||
|
||
void RateLimitPolicy::populateDescriptors(const Http::RequestHeaderMap& headers, | ||
const StreamInfo::StreamInfo& stream_info, | ||
const std::string& local_service_cluster, | ||
RateLimitDescriptors& descriptors) const { | ||
Envoy::RateLimit::LocalDescriptor descriptor; | ||
for (const Envoy::RateLimit::DescriptorProducerPtr& action : actions_) { | ||
Envoy::RateLimit::DescriptorEntry entry; | ||
if (!action->populateDescriptor(entry, local_service_cluster, headers, stream_info)) { | ||
return; | ||
} | ||
if (!entry.key_.empty()) { | ||
descriptor.entries_.emplace_back(std::move(entry)); | ||
} | ||
} | ||
descriptors.emplace_back(std::move(descriptor)); | ||
} | ||
|
||
RateLimitConfig::RateLimitConfig(const Protobuf::RepeatedPtrField<ProtoRateLimit>& configs, | ||
Server::Configuration::CommonFactoryContext& context, | ||
absl::Status& creation_status, bool no_limit) { | ||
for (const ProtoRateLimit& config : configs) { | ||
auto descriptor_generator = | ||
std::make_unique<RateLimitPolicy>(config, context, creation_status, no_limit); | ||
if (!creation_status.ok()) { | ||
return; | ||
} | ||
rate_limit_policies_.emplace_back(std::move(descriptor_generator)); | ||
} | ||
} | ||
|
||
void RateLimitConfig::populateDescriptors(const Http::RequestHeaderMap& headers, | ||
const StreamInfo::StreamInfo& stream_info, | ||
const std::string& local_service_cluster, | ||
RateLimitDescriptors& descriptors) const { | ||
for (const auto& generator : rate_limit_policies_) { | ||
generator->populateDescriptors(headers, stream_info, local_service_cluster, descriptors); | ||
} | ||
} | ||
|
||
} // namespace RateLimit | ||
} // namespace Common | ||
} // namespace Filters | ||
} // namespace Extensions | ||
} // namespace Envoy |
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.
I wonder if it would make more sense to move the
RateLimit
message to a different file (out of route_components.proto).Unfortunately I think it must stay in "envoy/config/route/v3/" to avoid a breaking change.
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.
I think it make sense. But not just for RateLimit. There are lots of common base message here that shared by other components because historical reason. I will prefer to do that in an independent PR.