Skip to content

Commit 86a7e70

Browse files
kathapkathap
authored
Adjust rate limits for service-related endpoints. (#4088)
* Adjust rate limits for service-related endpoints. Most if not all service broker operations for the CF API v3 have been moved into background jobs and are asynchronous operations. This means that the service broker operation can't block anymore the CF API v3 requests (which returns a 202 and a link to the polling job). After reviewing /v3/service related endpoints, we adapted the rate limiting: - v3 service related endpoints that never interact with SBs synchronously are removed from rate limiting (POST, PATCH, DELETE /v3/[service_instances|service_credential_bindings|service_route_bindings]) - v3 GET methods that interact with SBs synchronously are added to rate limiting (/v3/(service_instances|service_credential_bindings|service_route_bindings)/:guid/parameters)
1 parent 67ed31d commit 86a7e70

File tree

2 files changed

+85
-26
lines changed

2 files changed

+85
-26
lines changed

‎middleware/service_broker_rate_limiter.rb‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,13 @@
33

44
module CloudFoundry
55
module Middleware
6+
RateLimitEndpoint = Struct.new(:endpoint_pattern, :request_methods)
7+
8+
RATE_LIMITED_ENDPOINTS = [
9+
RateLimitEndpoint.new(%r{\A/v2/(service_instances|service_bindings|service_keys)}, %w[PUT POST DELETE]),
10+
RateLimitEndpoint.new(%r{\A/v3/(service_instances|service_credential_bindings|service_route_bindings)/.+/parameters\z}, %w[GET])
11+
].freeze
12+
613
class ConcurrentRequestCounter
714
def initialize(key_prefix, redis_connection_pool_size: nil)
815
@key_prefix = key_prefix
@@ -106,26 +113,17 @@ def call(env)
106113

107114
def apply_rate_limiting?(env)
108115
request = ActionDispatch::Request.new(env)
109-
!admin? && is_service_request?(request) && rate_limit_method?(request)
116+
!admin? && is_rate_limited_service_request?(request)
110117
end
111118

112119
def admin?
113120
VCAP::CloudController::SecurityContext.admin? || VCAP::CloudController::SecurityContext.admin_read_only?
114121
end
115122

116-
def is_service_request?(request)
117-
[
118-
%r{\A/v2/service_instances},
119-
%r{\A/v2/service_bindings},
120-
%r{\A/v2/service_keys},
121-
%r{\A/v3/service_instances},
122-
%r{\A/v3/service_credential_bindings},
123-
%r{\A/v3/service_route_bindings}
124-
].any? { |re| request.fullpath.match(re) }
125-
end
126-
127-
def rate_limit_method?(request)
128-
%w[PATCH PUT POST DELETE].include?(request.method)
123+
def is_rate_limited_service_request?(request)
124+
RATE_LIMITED_ENDPOINTS.any? do |endpoint|
125+
endpoint.endpoint_pattern.match?(request.fullpath) && endpoint.request_methods.include?(request.method)
126+
end
129127
end
130128

131129
def suggested_retry_after

‎spec/unit/middleware/service_broker_rate_limiter_spec.rb‎

Lines changed: 73 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@ module Middleware
55
RSpec.describe ServiceBrokerRateLimiter do
66
let(:app) { double(:app) }
77
let(:logger) { double }
8-
let(:path_info) { '/v3/service_instances' }
8+
let(:instance) { VCAP::CloudController::Service.make(instances_retrievable: true) }
9+
let(:path_info) { '/v2/service_instances' }
910
let(:user_guid) { SecureRandom.uuid }
1011
let(:user_env) { { 'cf.user_guid' => user_guid, 'PATH_INFO' => path_info } }
11-
let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: '/v3/service_instances', method: 'POST') }
12+
let(:env) { { 'cf.user_guid' => user_guid, 'PATH_INFO' => path, 'REQUEST_METHOD' => request_method } }
13+
let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: '/v2/service_instances', method: 'POST') }
1214
let(:max_concurrent_requests) { 1 }
1315
let(:broker_timeout) { 60 }
1416
let(:middleware) do
@@ -25,6 +27,8 @@ module Middleware
2527
end
2628

2729
describe 'included requests' do
30+
let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: '/v2/service_instances', method: 'PUT') }
31+
2832
it 'allows a service broker request within the limit' do
2933
status, = middleware.call(user_env)
3034
expect(status).to eq(200)
@@ -37,16 +41,6 @@ module Middleware
3741
expect(status).to eq(200)
3842
end
3943

40-
it 'does not allow more than the max number of concurrent requests' do
41-
threads = 2.times.map { Thread.new { Thread.current[:status], = middleware.call(user_env) } }
42-
statuses = threads.map { |t| t.join[:status] }
43-
44-
expect(statuses).to include(200)
45-
expect(statuses).to include(429)
46-
expect(app).to have_received(:call).once
47-
expect(logger).to have_received(:info).with("Service broker concurrent rate limit exceeded for user '#{user_guid}'")
48-
end
49-
5044
it 'counts concurrent requests per user' do
5145
other_user_env = { 'cf.user_guid' => 'other_user_guid', 'PATH_INFO' => path_info }
5246
threads = [user_env, other_user_env].map do |env|
@@ -67,6 +61,73 @@ module Middleware
6761
expect(status).to eq(200)
6862
end
6963

64+
describe 'service endpoints' do
65+
context 'v2 and v3 rate limited service endpoints' do
66+
rate_limited_service_endpoints = [
67+
%w[/v2/service_instances POST],
68+
%w[/v2/service_bindings POST],
69+
%w[/v2/service_keys POST],
70+
%w[/v2/service_instances PUT],
71+
%w[/v2/service_bindings PUT],
72+
%w[/v2/service_keys PUT],
73+
%w[/v2/service_instances DELETE],
74+
%w[/v2/service_bindings DELETE],
75+
%w[/v2/service_keys DELETE],
76+
%w[/v3/service_instances/:guid/parameters GET],
77+
%w[/v3/service_credential_bindings/:guid/parameters GET],
78+
%w[/v3/service_route_bindings/:guid/parameters GET]
79+
]
80+
rate_limited_service_endpoints.each do |endpoint, method|
81+
context "#{endpoint} #{method} - rate-limited" do
82+
let(:path) { endpoint.gsub(':guid', instance.guid.to_s) }
83+
let(:request_method) { method }
84+
let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: endpoint, method: method) }
85+
86+
it 'does not allow more than the max number of concurrent requests' do
87+
threads = 2.times.map { Thread.new { middleware.call(env) } }
88+
statuses = threads.map(&:join).map(&:value).map(&:first)
89+
90+
expect(statuses).to include(200)
91+
expect(statuses).to include(429)
92+
expect(app).to have_received(:call).once
93+
expect(logger).to have_received(:info).with("Service broker concurrent rate limit exceeded for user '#{user_guid}'")
94+
end
95+
end
96+
end
97+
end
98+
99+
context 'v3 non rate limited service endpoints' do
100+
v3_not_rate_limited_service_endpoints = [
101+
%w[/v3/service_instances POST],
102+
%w[/v3/service_credential_bindings POST],
103+
%w[/v3/service_route_bindings POST],
104+
%w[/v3/service_instances PATCH],
105+
%w[/v3/service_credential_bindings PATCH],
106+
%w[/v3/service_route_bindings PATCH],
107+
%w[/v3/service_instances DELETE],
108+
%w[/v3/service_credential_bindings DELETE],
109+
%w[/v3/service_route_bindings DELETE]
110+
]
111+
112+
v3_not_rate_limited_service_endpoints.each do |endpoint, method|
113+
context "#{endpoint} #{method} - non-rate-limited" do
114+
let(:path) { endpoint }
115+
let(:request_method) { method }
116+
let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: endpoint, method: method) }
117+
118+
it 'allows concurrent requests without limits' do
119+
threads = 2.times.map { Thread.new { middleware.call(env) } }
120+
statuses = threads.map(&:join).map(&:value).map(&:first)
121+
122+
expect(statuses).to all(eq(200))
123+
expect(app).to have_received(:call).twice
124+
expect(logger).not_to have_received(:info)
125+
end
126+
end
127+
end
128+
end
129+
end
130+
70131
describe 'errors' do
71132
let(:max_concurrent_requests) { 0 }
72133

0 commit comments

Comments
 (0)