Skip to content

Commit

Permalink
fix a bug of local rate limit about the x-ratelimited-* headers setti…
Browse files Browse the repository at this point in the history 
…ng (#34917)

Signed-off-by: wbpcode <[email protected]>
Co-authored-by: wbpcode <[email protected]>
  • Loading branch information
@wbpcode
wbpcode and wbpcode committed Jun 29, 2024
1 parent 3feff04 commit 416e7a6
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 52 deletions.
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ bug_fixes:
Validate http service path_prefix
:ref:`path_prefix <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.HttpService.path_prefix>`,
Validate http service path_prefix configuration must start with ``/``.
- area: local_ratelimit
change: |
Fixed a bug where the local rate limit filter would crash when the
:ref:`enable_x_ratelimit_headers <envoy_v3_api_msg_extensions.filters.http.ratelimit.v3.RateLimit>`
is set to ``DRAFT_VERSION_03`` and a send local reply is triggered before the rate limit filter is executed.
- area: admin
change: |
Fixed missing :ref:`additional addresses <envoy_v3_api_msg_config.endpoint.v3.Endpoint.AdditionalAddress>`
Expand Down
92 changes: 42 additions & 50 deletions source/extensions/filters/http/local_ratelimit/local_ratelimit.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,25 @@ bool FilterConfig::enforced() const {
}

Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
const auto* config = getConfig();
const auto* route_config =
Http::Utility::resolveMostSpecificPerFilterConfig<FilterConfig>(decoder_callbacks_);

// We can never assume that the configuration/route will not change between
// decodeHeaders() and encodeHeaders().
// Store the configuration because we will use it later in encodeHeaders().
if (route_config != nullptr) {
ASSERT(used_config_ == config_.get());
used_config_ = route_config; // Overwrite the used configuration.
}

if (!config->enabled()) {
if (!used_config_->enabled()) {
return Http::FilterHeadersStatus::Continue;
}

config->stats().enabled_.inc();
used_config_->stats().enabled_.inc();

std::vector<RateLimit::LocalDescriptor> descriptors;
if (config->hasDescriptors()) {
if (used_config_->hasDescriptors()) {
populateDescriptors(descriptors, headers);
}

Expand All @@ -158,35 +167,35 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
}

if (requestAllowed(descriptors)) {
config->stats().ok_.inc();
used_config_->stats().ok_.inc();
return Http::FilterHeadersStatus::Continue;
}

config->stats().rate_limited_.inc();
used_config_->stats().rate_limited_.inc();

if (!config->enforced()) {
config->requestHeadersParser().evaluateHeaders(headers, decoder_callbacks_->streamInfo());
if (!used_config_->enforced()) {
used_config_->requestHeadersParser().evaluateHeaders(headers, decoder_callbacks_->streamInfo());
return Http::FilterHeadersStatus::Continue;
}

config->stats().enforced_.inc();
used_config_->stats().enforced_.inc();

decoder_callbacks_->sendLocalReply(
config->status(), "local_rate_limited",
[this, config](Http::HeaderMap& headers) {
config->responseHeadersParser().evaluateHeaders(headers, decoder_callbacks_->streamInfo());
used_config_->status(), "local_rate_limited",
[this](Http::HeaderMap& headers) {
used_config_->responseHeadersParser().evaluateHeaders(headers,
decoder_callbacks_->streamInfo());
},
config->rateLimitedGrpcStatus(), "local_rate_limited");
used_config_->rateLimitedGrpcStatus(), "local_rate_limited");
decoder_callbacks_->streamInfo().setResponseFlag(StreamInfo::CoreResponseFlag::RateLimited);

return Http::FilterHeadersStatus::StopIteration;
}

Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) {
const auto* config = getConfig();

if (config->enabled() && config->enableXRateLimitHeaders()) {
ASSERT(stored_descriptors_.has_value());
// We can never assume the decodeHeaders() was called before encodeHeaders().
if (used_config_->enabled() && used_config_->enableXRateLimitHeaders() &&
stored_descriptors_.has_value()) {
auto limit = maxTokens(stored_descriptors_.value());
auto remaining = remainingTokens(stored_descriptors_.value());
auto reset = remainingFillInterval(stored_descriptors_.value());
Expand All @@ -203,47 +212,42 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers
}

bool Filter::requestAllowed(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) {
const auto* config = getConfig();
return config->rateLimitPerConnection()
return used_config_->rateLimitPerConnection()
? getPerConnectionRateLimiter().requestAllowed(request_descriptors)
: config->requestAllowed(request_descriptors);
: used_config_->requestAllowed(request_descriptors);
}

uint32_t Filter::maxTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) {
const auto* config = getConfig();
return config->rateLimitPerConnection()
return used_config_->rateLimitPerConnection()
? getPerConnectionRateLimiter().maxTokens(request_descriptors)
: config->maxTokens(request_descriptors);
: used_config_->maxTokens(request_descriptors);
}

uint32_t Filter::remainingTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) {
const auto* config = getConfig();
return config->rateLimitPerConnection()
return used_config_->rateLimitPerConnection()
? getPerConnectionRateLimiter().remainingTokens(request_descriptors)
: config->remainingTokens(request_descriptors);
: used_config_->remainingTokens(request_descriptors);
}

int64_t
Filter::remainingFillInterval(absl::Span<const RateLimit::LocalDescriptor> request_descriptors) {
const auto* config = getConfig();
return config->rateLimitPerConnection()
return used_config_->rateLimitPerConnection()
? getPerConnectionRateLimiter().remainingFillInterval(request_descriptors)
: config->remainingFillInterval(request_descriptors);
: used_config_->remainingFillInterval(request_descriptors);
}

const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConnectionRateLimiter() {
const auto* config = getConfig();
ASSERT(config->rateLimitPerConnection());
ASSERT(used_config_->rateLimitPerConnection());

auto typed_state =
decoder_callbacks_->streamInfo().filterState()->getDataReadOnly<PerConnectionRateLimiter>(
PerConnectionRateLimiter::key());

if (typed_state == nullptr) {
auto limiter = std::make_shared<PerConnectionRateLimiter>(
config->fillInterval(), config->maxTokens(), config->tokensPerFill(),
decoder_callbacks_->dispatcher(), config->descriptors(),
config->consumeDefaultTokenBucket());
used_config_->fillInterval(), used_config_->maxTokens(), used_config_->tokensPerFill(),
decoder_callbacks_->dispatcher(), used_config_->descriptors(),
used_config_->consumeDefaultTokenBucket());

decoder_callbacks_->streamInfo().filterState()->setData(
PerConnectionRateLimiter::key(), limiter, StreamInfo::FilterState::StateType::ReadOnly,
Expand Down Expand Up @@ -284,35 +288,23 @@ void Filter::populateDescriptors(std::vector<RateLimit::LocalDescriptor>& descri
void Filter::populateDescriptors(const Router::RateLimitPolicy& rate_limit_policy,
std::vector<RateLimit::LocalDescriptor>& descriptors,
Http::RequestHeaderMap& headers) {
const auto* config = getConfig();
for (const Router::RateLimitPolicyEntry& rate_limit :
rate_limit_policy.getApplicableRateLimit(config->stage())) {
rate_limit_policy.getApplicableRateLimit(used_config_->stage())) {
const std::string& disable_key = rate_limit.disableKey();

if (!disable_key.empty()) {
continue;
}
rate_limit.populateLocalDescriptors(descriptors, config->localInfo().clusterName(), headers,
decoder_callbacks_->streamInfo());
rate_limit.populateLocalDescriptors(descriptors, used_config_->localInfo().clusterName(),
headers, decoder_callbacks_->streamInfo());
}
}

const FilterConfig* Filter::getConfig() const {
const auto* config =
Http::Utility::resolveMostSpecificPerFilterConfig<FilterConfig>(decoder_callbacks_);
if (config) {
return config;
}

return config_.get();
}

VhRateLimitOptions Filter::getVirtualHostRateLimitOption(const Router::RouteConstSharedPtr& route) {
if (route->routeEntry()->includeVirtualHostRateLimits()) {
vh_rate_limits_ = VhRateLimitOptions::Include;
} else {
const auto* config = getConfig();
switch (config->virtualHostRateLimits()) {
switch (used_config_->virtualHostRateLimits()) {
PANIC_ON_PROTO_ENUM_SENTINEL_VALUES;
case envoy::extensions::common::ratelimit::v3::INCLUDE:
vh_rate_limits_ = VhRateLimitOptions::Include;
Expand Down
6 changes: 4 additions & 2 deletions source/extensions/filters/http/local_ratelimit/local_ratelimit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>;
*/
class Filter : public Http::PassThroughFilter, Logger::Loggable<Logger::Id::filter> {
public:
Filter(FilterConfigSharedPtr config) : config_(config) {}
Filter(FilterConfigSharedPtr config) : config_(config), used_config_(config_.get()) {}

// Http::StreamDecoderFilter
Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers,
Expand All @@ -188,8 +188,10 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable<Logger::Id::filt
uint32_t remainingTokens(absl::Span<const RateLimit::LocalDescriptor> request_descriptors);
int64_t remainingFillInterval(absl::Span<const RateLimit::LocalDescriptor> request_descriptors);

const FilterConfig* getConfig() const;
FilterConfigSharedPtr config_;
// Actual config used for the current request. Is config_ by default, but can be overridden by
// per-route config.
const FilterConfig* used_config_{};

absl::optional<std::vector<RateLimit::LocalDescriptor>> stored_descriptors_;
VhRateLimitOptions vh_rate_limits_;
Expand Down
16 changes: 16 additions & 0 deletions test/extensions/filters/http/local_ratelimit/filter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,22 @@ TEST_F(FilterTest, RequestRateLimitedXRateLimitHeaders) {
EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.rate_limited"));
}

TEST_F(FilterTest, RequestRateLimitedXRateLimitHeadersWithoutRunningDecodeHeaders) {
setup(fmt::format(config_yaml, "false", "1", "false", "DRAFT_VERSION_03"));

auto response_headers = Http::TestResponseHeaderMapImpl();

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(response_headers, false));
EXPECT_EQ("", response_headers.get_("x-ratelimit-limit"));
EXPECT_EQ("", response_headers.get_("x-ratelimit-remaining"));
EXPECT_EQ("", response_headers.get_("x-ratelimit-reset"));

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_2_->encodeHeaders(response_headers, false));
EXPECT_EQ("", response_headers.get_("x-ratelimit-limit"));
EXPECT_EQ("", response_headers.get_("x-ratelimit-remaining"));
EXPECT_EQ("", response_headers.get_("x-ratelimit-reset"));
}

static constexpr absl::string_view descriptor_config_yaml = R"(
stat_prefix: test
token_bucket:
Expand Down

0 comments on commit 416e7a6

Please sign in to comment.