From 416e7a64d1d3f72bf3309938e22ca5f006fe429f Mon Sep 17 00:00:00 2001 From: code Date: Sat, 29 Jun 2024 10:46:43 +0800 Subject: [PATCH] fix a bug of local rate limit about the x-ratelimited-* headers setting (#34917) Signed-off-by: wbpcode Co-authored-by: wbpcode --- changelogs/current.yaml | 5 + .../http/local_ratelimit/local_ratelimit.cc | 92 +++++++++---------- .../http/local_ratelimit/local_ratelimit.h | 6 +- .../http/local_ratelimit/filter_test.cc | 16 ++++ 4 files changed, 67 insertions(+), 52 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 348a1eb46d7a..d4fa242f22c0 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -214,6 +214,11 @@ bug_fixes: Validate http service path_prefix :ref:`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 ` + 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 ` diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index e92274029cec..ed759c2ea48d 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -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(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 descriptors; - if (config->hasDescriptors()) { + if (used_config_->hasDescriptors()) { populateDescriptors(descriptors, headers); } @@ -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()); @@ -203,37 +212,32 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers } bool Filter::requestAllowed(absl::Span 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 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 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 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( @@ -241,9 +245,9 @@ const Filters::Common::LocalRateLimit::LocalRateLimiterImpl& Filter::getPerConne if (typed_state == nullptr) { auto limiter = std::make_shared( - 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, @@ -284,35 +288,23 @@ void Filter::populateDescriptors(std::vector& descri void Filter::populateDescriptors(const Router::RateLimitPolicy& rate_limit_policy, std::vector& 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(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; diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 6bad1e57636b..7640af4835bd 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -163,7 +163,7 @@ using FilterConfigSharedPtr = std::shared_ptr; */ class Filter : public Http::PassThroughFilter, Logger::Loggable { public: - Filter(FilterConfigSharedPtr config) : config_(config) {} + Filter(FilterConfigSharedPtr config) : config_(config), used_config_(config_.get()) {} // Http::StreamDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, @@ -188,8 +188,10 @@ class Filter : public Http::PassThroughFilter, Logger::Loggable request_descriptors); int64_t remainingFillInterval(absl::Span 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> stored_descriptors_; VhRateLimitOptions vh_rate_limits_; diff --git a/test/extensions/filters/http/local_ratelimit/filter_test.cc b/test/extensions/filters/http/local_ratelimit/filter_test.cc index 6994dc46d034..607eb82ad92f 100644 --- a/test/extensions/filters/http/local_ratelimit/filter_test.cc +++ b/test/extensions/filters/http/local_ratelimit/filter_test.cc @@ -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: