Skip to content
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

Envoy rate limit descriptor fix #11153

Merged
Merged
Show file tree
Hide file tree
Changes from 13 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
5 changes: 5 additions & 0 deletions api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,11 @@ message RateLimit {

// The key to use in the descriptor entry.
string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}];

// If set to true, Envoy skips the descriptor while calling rate limiting service
// when header is not present in the request. Defaults to false, which skips calling
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// when header is not present in the request. Defaults to false, which skips calling
// when header is not present in the request. By default this skips calling

i.e. it's redundant to say that the default of a primitive proto3 type is false, but your description of the actual behavior is very helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, @htuch! Pushed the changes as you suggested! Thanks!

// rate limiting service if this header is not present in the request.
bool skip_if_absent = 3;
}

// The following descriptor entry is appended to the descriptor and is populated using the
Expand Down
5 changes: 5 additions & 0 deletions api/envoy/config/route/v4alpha/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1393,6 +1393,11 @@ message RateLimit {

// The key to use in the descriptor entry.
string descriptor_key = 2 [(validate.rules).string = {min_bytes: 1}];

// If set to true, Envoy skips the descriptor while calling rate limiting service
// when header is not present in the request. Defaults to false, which skips calling
// rate limiting service if this header is not present in the request.
bool skip_if_absent = 3;
}

// The following descriptor entry is appended to the descriptor and is populated using the
Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Changes
tracing is not forced.
* router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`.
* router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded <config_http_filters_router_x-envoy-overloaded_set>`.
* router: allow Rate Limiting service to be called in case of missing request header for a descriptor if the :ref:`skip_if_absent <envoy_v3_api_field_config.route.v3.RateLimit.Action.RequestHeaders.skip_if_absent>` field is set to true.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Rate Limiting SErvice

* runtime: add new gauge :ref:`deprecated_feature_seen_since_process_start <runtime_stats>` that gets reset across hot restarts.
* stats: added the option to :ref:`report counters as deltas <envoy_v3_api_field_config.metrics.v3.MetricsServiceConfig.report_counters_as_deltas>` to the metrics service stats sink.
* tracing: tracing configuration has been made fully dynamic and every HTTP connection manager
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions source/common/router/router_ratelimit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,11 @@ bool RequestHeadersAction::populateDescriptor(const Router::RouteEntry&,
const Http::HeaderMap& headers,
const Network::Address::Instance&) const {
const Http::HeaderEntry* header_value = headers.get(header_name_);

// If header is not present and skip_if_absent_ is not set to true, short circuit here and return.
Copy link
Member

Choose a reason for hiding this comment

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

nit: this comment is not quite accurate, as you return no matter what. Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @mattklein123 - Yes, you are right, I agree it can be misleading. Let me rephrase it. What I was going for was "do not add the descriptor". Thanks for pointing it out!

if (!header_value) {
return false;
return skip_if_absent_;
}

descriptor.entries_.push_back(
{descriptor_key_, std::string(header_value->value().getStringView())});
return true;
Expand Down
4 changes: 3 additions & 1 deletion source/common/router/router_ratelimit.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ class DestinationClusterAction : public RateLimitAction {
class RequestHeadersAction : public RateLimitAction {
public:
RequestHeadersAction(const envoy::config::route::v3::RateLimit::Action::RequestHeaders& action)
: header_name_(action.header_name()), descriptor_key_(action.descriptor_key()) {}
: header_name_(action.header_name()), descriptor_key_(action.descriptor_key()),
skip_if_absent_(action.skip_if_absent()) {}

// Router::RateLimitAction
bool populateDescriptor(const Router::RouteEntry& route, RateLimit::Descriptor& descriptor,
Expand All @@ -52,6 +53,7 @@ class RequestHeadersAction : public RateLimitAction {
private:
const Http::LowerCaseString header_name_;
const std::string descriptor_key_;
const bool skip_if_absent_;
};

/**
Expand Down
47 changes: 47 additions & 0 deletions test/common/router/router_ratelimit_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,53 @@ TEST_F(RateLimitPolicyEntryTest, RequestHeaders) {
testing::ContainerEq(descriptors_));
}

// Validate that a descriptor is added if the missing request header
// has skip_if_absent set to true
TEST_F(RateLimitPolicyEntryTest, RequestHeadersWithSkipIfAbsent) {
const std::string yaml = R"EOF(
actions:
- request_headers:
header_name: x-header-name
descriptor_key: my_header_name
skip_if_absent: false
- request_headers:
header_name: x-header
descriptor_key: my_header
skip_if_absent: true
)EOF";

setupTest(yaml);
Http::TestHeaderMapImpl header{{"x-header-name", "test_value"}};

rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header,
default_remote_address_);
EXPECT_THAT(std::vector<Envoy::RateLimit::Descriptor>({{{{"my_header_name", "test_value"}}}}),
testing::ContainerEq(descriptors_));
}

// Tests if the descriptors are added if one of the headers is missing
// and skip_if_absent is set to default value which is false
TEST_F(RateLimitPolicyEntryTest, RequestHeadersWithDefaultSkipIfAbsent) {
const std::string yaml = R"EOF(
actions:
- request_headers:
header_name: x-header-name
descriptor_key: my_header_name
skip_if_absent: false
- request_headers:
header_name: x-header
descriptor_key: my_header
skip_if_absent: false
)EOF";

setupTest(yaml);
Http::TestHeaderMapImpl header{{"x-header-test", "test_value"}};

rate_limit_entry_->populateDescriptors(route_, descriptors_, "service_cluster", header,
default_remote_address_);
EXPECT_TRUE(descriptors_.empty());
}

TEST_F(RateLimitPolicyEntryTest, RequestHeadersNoMatch) {
const std::string yaml = R"EOF(
actions:
Expand Down