diff --git a/proxygen/lib/http/codec/ControlMessageRateLimitFilter.h b/proxygen/lib/http/codec/ControlMessageRateLimitFilter.h index f337279ce0..d902ddc0b8 100644 --- a/proxygen/lib/http/codec/ControlMessageRateLimitFilter.h +++ b/proxygen/lib/http/codec/ControlMessageRateLimitFilter.h @@ -24,14 +24,9 @@ constexpr uint32_t kDefaultMaxDirectErrorHandlingPerInterval = 100; constexpr uint32_t kMaxDirectErrorHandlingPerIntervalLowerBound = 50; constexpr std::chrono::milliseconds kDefaultDirectErrorHandlingDuration{100}; -constexpr uint32_t kDefaultMaxHeadersPerInterval = 50000; -constexpr uint32_t kMaxHeadersPerIntervalLowerBound = 100; -constexpr std::chrono::milliseconds kDefaultHeadersDuration{100}; - enum RateLimitTarget { CONTROL_MSGS, DIRECT_ERROR_HANDLING, - HEADERS, }; /** @@ -54,9 +49,6 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { resetDirectErrors_(numDirectErrorHandlingInCurrentInterval_, RateLimitTarget::DIRECT_ERROR_HANDLING, httpSessionStats), - resetHeaders_(numHeadersInCurrentInterval_, - RateLimitTarget::HEADERS, - httpSessionStats), timer_(timer), httpSessionStats_(httpSessionStats) { } @@ -64,21 +56,17 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { void setSessionStats(HTTPSessionStats* httpSessionStats) { httpSessionStats_ = httpSessionStats; resetControlMessages_.httpSessionStats = httpSessionStats; - resetHeaders_.httpSessionStats = httpSessionStats; } - void setParams(uint32_t maxControlMsgsPerInterval, - uint32_t maxDirectErrorHandlingPerInterval, - uint32_t maxHeadersPerInterval, - std::chrono::milliseconds controlMsgIntervalDuration, - std::chrono::milliseconds directErrorHandlingIntervalDuration, - std::chrono::milliseconds headersIntervalDuration) { + void setParams( + uint32_t maxControlMsgsPerInterval, + uint32_t maxDirectErrorHandlingPerInterval, + std::chrono::milliseconds controlMsgIntervalDuration, + std::chrono::milliseconds directErrorHandlingIntervalDuration) { maxControlMsgsPerInterval_ = maxControlMsgsPerInterval; maxDirectErrorHandlingPerInterval_ = maxDirectErrorHandlingPerInterval; controlMsgIntervalDuration_ = controlMsgIntervalDuration; directErrorHandlingIntervalDuration_ = directErrorHandlingIntervalDuration; - maxHeadersPerInterval_ = maxHeadersPerInterval; - headersIntervalDuration_ = headersIntervalDuration; } // Filter functions @@ -110,13 +98,6 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { } } - void onHeadersComplete(StreamID stream, - std::unique_ptr msg) override { - if (!incrementNumHeadersInCurInterval()) { - callback_->onHeadersComplete(stream, std::move(msg)); - } - } - void onError(HTTPCodec::StreamID streamID, const HTTPException& error, bool newTxn) override { @@ -135,7 +116,6 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { void detachThreadLocals() { resetControlMessages_.cancelTimeout(); resetDirectErrors_.cancelTimeout(); - resetHeaders_.cancelTimeout(); timer_ = nullptr; // Free pass when switching threads numControlMsgsInCurrentInterval_ = 0; @@ -172,25 +152,6 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { return false; } - bool incrementNumHeadersInCurInterval() { - if (numHeadersInCurrentInterval_ == 0) { - // The first header (or first after a reset) schedules the next - // reset timer - CHECK(timer_); - timer_->scheduleTimeout(&resetHeaders_, headersIntervalDuration_); - } - - if (++numHeadersInCurrentInterval_ > maxHeadersPerInterval_) { - if (httpSessionStats_) { - httpSessionStats_->recordHeadersRateLimited(); - } - callback_->onGoaway(http2::kMaxStreamID, ErrorCode::NO_ERROR); - return true; - } - - return false; - } - bool incrementDirectErrorHandlingInCurInterval() { if (numDirectErrorHandlingInCurrentInterval_ == 0) { // The first control message (or first after a reset) schedules the next @@ -235,9 +196,6 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { case RateLimitTarget::DIRECT_ERROR_HANDLING: // No stats for this one break; - case RateLimitTarget::HEADERS: - httpSessionStats->recordHeadersInInterval(counter); - break; } } counter = 0; @@ -262,18 +220,13 @@ class ControlMessageRateLimitFilter : public PassThroughHTTPCodecFilter { uint32_t maxDirectErrorHandlingPerInterval_{ kDefaultMaxDirectErrorHandlingPerInterval}; - uint32_t numHeadersInCurrentInterval_{0}; - uint32_t maxHeadersPerInterval_{kDefaultMaxHeadersPerInterval}; - std::chrono::milliseconds controlMsgIntervalDuration_{ kDefaultControlMsgDuration}; std::chrono::milliseconds directErrorHandlingIntervalDuration_{ kDefaultDirectErrorHandlingDuration}; - std::chrono::milliseconds headersIntervalDuration_{kDefaultHeadersDuration}; ResetCounterTimeout resetControlMessages_; ResetCounterTimeout resetDirectErrors_; - ResetCounterTimeout resetHeaders_; folly::HHWheelTimer* timer_{nullptr}; HTTPSessionStats* httpSessionStats_{nullptr}; }; diff --git a/proxygen/lib/http/session/HTTPSession.cpp b/proxygen/lib/http/session/HTTPSession.cpp index 4cbc6fa9ff..1e08ceb3b0 100644 --- a/proxygen/lib/http/session/HTTPSession.cpp +++ b/proxygen/lib/http/session/HTTPSession.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -221,13 +222,16 @@ void HTTPSession::setupCodec() { // if we really support switching from spdy <-> h2, we need to update // existing flow control filter } - if (codec_->supportsParallelRequests() && !controlMessageRateLimitFilter_ && - sock_ && + if (codec_->supportsParallelRequests() && sock_ && codec_->getTransportDirection() == TransportDirection::DOWNSTREAM) { - controlMessageRateLimitFilter_ = new ControlMessageRateLimitFilter( - &getEventBase()->timer(), sessionStats_); - codec_.addFilters(std::unique_ptr( - controlMessageRateLimitFilter_)); + addRateLimitFilter(RateLimitFilter::Type::HEADERS); + + if (!controlMessageRateLimitFilter_) { + controlMessageRateLimitFilter_ = new ControlMessageRateLimitFilter( + &getEventBase()->timer(), sessionStats_); + codec_.addFilters(std::unique_ptr( + controlMessageRateLimitFilter_)); + } } codec_.setCallback(this); diff --git a/proxygen/lib/http/session/HTTPSessionBase.cpp b/proxygen/lib/http/session/HTTPSessionBase.cpp index 106e4808b2..cb2d314dfc 100644 --- a/proxygen/lib/http/session/HTTPSessionBase.cpp +++ b/proxygen/lib/http/session/HTTPSessionBase.cpp @@ -75,10 +75,8 @@ void HTTPSessionBase::setSessionStats(HTTPSessionStats* stats) { void HTTPSessionBase::setControlMessageRateLimitParams( uint32_t maxControlMsgsPerInterval, uint32_t maxDirectErrorHandlingPerInterval, - uint32_t maxHeadersPerInterval, std::chrono::milliseconds controlMsgIntervalDuration, - std::chrono::milliseconds directErrorHandlingIntervalDuration, - std::chrono::milliseconds headersIntervalDuration) { + std::chrono::milliseconds directErrorHandlingIntervalDuration) { if (maxControlMsgsPerInterval < kMaxControlMsgsPerIntervalLowerBound) { XLOG_EVERY_MS(WARNING, 60000) @@ -95,20 +93,12 @@ void HTTPSessionBase::setControlMessageRateLimitParams( kMaxDirectErrorHandlingPerIntervalLowerBound; } - if (maxHeadersPerInterval < kMaxHeadersPerIntervalLowerBound) { - XLOG_EVERY_MS(WARNING, 60000) - << "Invalid maxHeadersPerInterval: " << maxHeadersPerInterval; - maxHeadersPerInterval = kMaxHeadersPerIntervalLowerBound; - } - if (controlMessageRateLimitFilter_) { controlMessageRateLimitFilter_->setParams( maxControlMsgsPerInterval, maxDirectErrorHandlingPerInterval, - maxHeadersPerInterval, controlMsgIntervalDuration, - directErrorHandlingIntervalDuration, - headersIntervalDuration); + directErrorHandlingIntervalDuration); } } @@ -121,6 +111,14 @@ void HTTPSessionBase::setRateLimitParams( << "Out of bounds access to rate limit filter array"; RateLimitFilter* rateLimitFilter = rateLimitFilters_.at(typeIndex); if (rateLimitFilter) { + uint32_t maxEventsPerIntervalLowerBound = + rateLimitFilter->getMaxEventsPerInvervalLowerBound(); + if (maxEventsPerInterval < maxEventsPerIntervalLowerBound) { + LOG(WARNING) << "Invalid maxEventsPerInterval for event " + << RateLimitFilter::toStr(type) << ": " + << maxEventsPerInterval; + maxEventsPerInterval = maxEventsPerIntervalLowerBound; + } rateLimitFilter->setParams(maxEventsPerInterval, intervalDuration); } } diff --git a/proxygen/lib/http/session/HTTPSessionBase.h b/proxygen/lib/http/session/HTTPSessionBase.h index 15848700ab..be309073c6 100644 --- a/proxygen/lib/http/session/HTTPSessionBase.h +++ b/proxygen/lib/http/session/HTTPSessionBase.h @@ -177,13 +177,10 @@ class HTTPSessionBase : public wangle::ManagedConnection { uint32_t maxControlMsgsPerInterval = kDefaultMaxControlMsgsPerInterval, uint32_t maxDirectErrorHandlingPerInterval = kDefaultMaxDirectErrorHandlingPerInterval, - uint32_t maxHeadersPerInterval = kDefaultMaxHeadersPerInterval, std::chrono::milliseconds controlMsgIntervalDuration = kDefaultControlMsgDuration, std::chrono::milliseconds directErrorHandlingIntervalDuration = - kDefaultDirectErrorHandlingDuration, - std::chrono::milliseconds headersIntervalDuration = - kDefaultHeadersDuration); + kDefaultDirectErrorHandlingDuration); void setRateLimitParams(RateLimitFilter::Type type, uint32_t maxEventsPerInterval, diff --git a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp index 089a6ed480..e993cc7a3e 100644 --- a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp @@ -3728,8 +3728,8 @@ TEST_F(HTTP2DownstreamSessionTest, TestPriorityFCBlocked) { } TEST_F(HTTP2DownstreamSessionTest, TestHeadersRateLimitExceeded) { - httpSession_->setControlMessageRateLimitParams( - 1000, 1000, 100, seconds(0), seconds(0), seconds(0)); + httpSession_->setRateLimitParams( + RateLimitFilter::Type::HEADERS, 100, std::chrono::seconds(0)); std::vector>> handlers; for (int i = 0; i < 100; i++) { @@ -3780,7 +3780,7 @@ TEST_F(HTTP2DownstreamSessionTest, TestControlMsgResetRateLimitTouched) { auto streamid = clientCodec_->createStream(); - httpSession_->setControlMessageRateLimitParams(10, 100, 100, milliseconds(0)); + httpSession_->setControlMessageRateLimitParams(10, 100, milliseconds(0)); // Send 97 PRIORITY, 1 SETTINGS, and 2 PING frames. This doesn't exceed the // limit of 10. @@ -3821,7 +3821,7 @@ TEST_F(HTTP2DownstreamSessionTest, TestControlMsgResetRateLimitTouched) { } TEST_F(HTTP2DownstreamSessionTest, DirectErrorHandlingLimitTouched) { - httpSession_->setControlMessageRateLimitParams(100, 10, 100, milliseconds(0)); + httpSession_->setControlMessageRateLimitParams(100, 10, milliseconds(0)); // Send 50 messages, each of which cause direct error handling. Since // this doesn't exceed the limit, this should not cause the connection @@ -3853,7 +3853,7 @@ TEST_F(HTTP2DownstreamSessionTest, DirectErrorHandlingLimitTouched) { } TEST_F(HTTP2DownstreamSessionTest, DirectErrorHandlingLimitExceeded) { - httpSession_->setControlMessageRateLimitParams(100, 10, 100, milliseconds(0)); + httpSession_->setControlMessageRateLimitParams(100, 10, milliseconds(0)); // Send eleven messages, each of which causes direct error handling. Since // this exceeds the limit, the connection should be dropped.