Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
55 changes: 54 additions & 1 deletion api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "envoy/config/core/v3/base.proto";
import "envoy/config/core/v3/extension.proto";
import "envoy/config/core/v3/proxy_protocol.proto";
import "envoy/config/core/v3/substitution_format_string.proto";
import "envoy/type/matcher/v3/address.proto";
import "envoy/type/matcher/v3/filter_state.proto";
import "envoy/type/matcher/v3/metadata.proto";
import "envoy/type/matcher/v3/regex.proto";
Expand Down Expand Up @@ -2084,7 +2085,7 @@ message VirtualCluster {
message RateLimit {
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.route.RateLimit";

// [#next-free-field: 13]
// [#next-free-field: 14]
message Action {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.route.RateLimit.Action";
Expand Down Expand Up @@ -2465,6 +2466,54 @@ message RateLimit {
[(validate.rules).repeated = {min_items: 1}];
}

// The following descriptor entry is appended to the descriptor:
//
// .. code-block:: cpp
//
// ("remote_address_match", "<descriptor_value>")
// [#not-implemented-hide:]
message RemoteAddressMatch {
// Descriptor value of entry.
//
// The same :ref:`format specifier <config_access_log_format>` as used for
// :ref:`HTTP access logging <config_access_log>` applies here, however
// unknown specifier values are replaced with the empty string instead of ``-``.
//
// .. note::
//
// The format string can contain multiple valid substitution fields. If multiple substitution
// fields are present, their results will be concatenated to form the final descriptor value.
// If it contains no substitution fields, the value will be used as is.
// All substitution fields will be evaluated and their results concatenated.
// If the final concatenated result is empty and ``default_value`` is set, the ``default_value`` will be used.
// If ``default_value`` is not set and the result is empty, this descriptor will be skipped
// and not included in the rate limit call.
//
// For example, ``static_value`` will be used as is since there are no substitution fields.
// ``%REQ(:method)%`` will be replaced with the HTTP method, and
// ``%REQ(:method)%%REQ(:path)%`` will be replaced with the concatenation of the HTTP method and path.
// ``%CEL(request.headers['user-id'])%`` will use CEL to extract the user ID from request headers.
//
string descriptor_value = 1 [(validate.rules).string = {min_len: 1}];

// The key to use in the descriptor entry.
//
// Defaults to ``remote_address_match``.
string descriptor_key = 2;

// An optional value to use if the final concatenated ``descriptor_value`` result is empty.
string default_value = 3;

// Specifies an address matcher that controls whether the rate limit action is applied.
// The matcher checks the remote address (trusted address from
// :ref:`x-forwarded-for <config_http_conn_man_headers_x-forwarded-for>`)
// against the specified CIDR ranges. The rate limit action will be applied if
// the remote address matches any of the CIDR ranges (or does not match any if
// ``invert_match`` is set to true in the address matcher).
type.matcher.v3.AddressMatcher address_matcher = 4
[(validate.rules).message = {required: true}];
}

oneof action_specifier {
option (validate.required) = true;

Expand Down Expand Up @@ -2517,6 +2566,10 @@ message RateLimit {

// Rate limit on the existence of query parameters.
QueryParameterValueMatch query_parameter_value_match = 11;

// [#not-implemented-hide:]
// Rate limit on remote address match.
RemoteAddressMatch remote_address_match = 13;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this new field is confusing because there are already remote_address and remote_address_match. I think calrifying the reasoning behind this feature is the first step. I'm not an expert in this domain, but I would suggest one of the following:

  1. consider adding options to RemoteAddress and MaskedRemoteAddress that enables the matching (the reasoning behind this is that I think that matching is more generic and orthogonal to whether the matched object is a remote address, a request header, etc).
  2. See if this should be a typed-extension (using the extension field).

cc @wbpcode due to the comment in #36442

Copy link
Member Author

Choose a reason for hiding this comment

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

@adisuissa thanks for the comment. This is supposed to handle all the remote address matching usecases in a generic way by leveraging CEL and substitution formatting in descriptor_value. For example:

  • Using %DOWNSTREAM_REMOTE_ADDRESS% here will handle the existing RemoteAddress usecase with CIDR matching.
  • We might need to introduce formatter like %MASKED_DOWNSTREAM_REMOTE_ADDRESS(len)% to handle MaskedRemoteAddress usecase with CIDR matching. See feat: add formatters for masked IP addresses #42969 .

@wbpcode is aligned with the RemoteAddressMatch approach (see comments above), if the name is confusing I will think of some alternatives.

Copy link
Member Author

Choose a reason for hiding this comment

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

^ bump @adisuissa @wbpcode
How does remote_address_range_match / remote_address_value_match sound instead of remote_address_match?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wbpcode is the expert on substitution formatters. I'm just presenting my thoughts on design and combination of features.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, thanks for your inputs!

}
}

Expand Down
4 changes: 4 additions & 0 deletions api/envoy/type/matcher/v3/address.proto
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// filter state object as an IP.
message AddressMatcher {
repeated xds.core.v3.CidrRange ranges = 1;

// [#not-implemented-hide:]
// If true, the match result will be inverted. Defaults to false.
bool invert_match = 2;
}
4 changes: 4 additions & 0 deletions source/common/router/router_ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ RateLimitPolicyEntryImpl::RateLimitPolicyEntryImpl(
action.query_parameter_value_match(), context, std::move(formatter_or_error.value())));
break;
}
case envoy::config::route::v3::RateLimit::Action::ActionSpecifierCase::kRemoteAddressMatch:
// [#not-implemented-hide:] RemoteAddressMatch is not yet implemented.
PANIC("RemoteAddressMatch rate limit action is not yet implemented");
break;
case envoy::config::route::v3::RateLimit::Action::ActionSpecifierCase::ACTION_SPECIFIER_NOT_SET:
PANIC_DUE_TO_CORRUPT_ENUM;
}
Expand Down
Loading