Set suggested retry delay to between 0.5x and 1.5x broker timeout

This commit changes the previously proposed change (which suggested a random retry delay between 30 and 90 seconds) to one that is between 0.5x and 1.5x of the configurable value broker_client_timeout_seconds. That field's default value is 60, and therefore the average deployment will in any event send service broker rate limiter Retry-After headers with a suggested retry time between 30 and 90 seconds in the future.

lib/cloud_controller/rack_app_builder.rb

@@ -40,7 +40,10 @@ def build(config, request_metrics, request_logs)
         end
         if config.get(:max_concurrent_service_broker_requests) > 0
           CloudFoundry::Middleware::ServiceBrokerRequestCounter.instance.limit = config.get(:max_concurrent_service_broker_requests)
-          use CloudFoundry::Middleware::ServiceBrokerRateLimiter, logger: Steno.logger('cc.service_broker_rate_limiter')
+          use CloudFoundry::Middleware::ServiceBrokerRateLimiter, {
+            logger: Steno.logger('cc.service_broker_rate_limiter'),
+            broker_timeout_seconds: config.get(:broker_client_timeout_seconds),
+          }
         end
 
         if config.get(:security_event_logging, :enabled)

middleware/service_broker_rate_limiter.rb

@@ -26,6 +26,7 @@ class ServiceBrokerRateLimiter
       def initialize(app, opts)
         @app                               = app
         @logger                            = opts[:logger]
+        @broker_timeout_seconds            = opts[:broker_timeout_seconds]
         @request_counter = ServiceBrokerRequestCounter.instance
       end
 
@@ -78,7 +79,8 @@ def user_token?(env)
       end
 
       def suggested_retry_time
-        Time.now.utc + rand(30..90).second
+        delay_range = @broker_timeout_seconds * 0.5..@broker_timeout_seconds * 1.5
+        Time.now.utc + rand(delay_range).to_i.second
       end
 
       def too_many_requests!(env, user_guid)

spec/unit/lib/cloud_controller/rack_app_builder_spec.rb

@@ -105,6 +105,7 @@ module VCAP::CloudController
             expect(CloudFoundry::Middleware::ServiceBrokerRateLimiter).to have_received(:new).with(
               anything,
               logger: instance_of(Steno::Logger),
+              broker_timeout_seconds: TestConfig.config_instance.get(:broker_client_timeout_seconds)
             )
           end
         end

spec/unit/middleware/service_broker_rate_limiter_spec.rb

@@ -9,9 +9,10 @@ module Middleware
       let(:user_env) { { 'cf.user_guid' => 'user_guid', 'PATH_INFO' => path_info } }
       let(:fake_request) { instance_double(ActionDispatch::Request, fullpath: '/v3/service_instances', method: 'POST') }
       let(:concurrent_limit) { 1 }
+      let(:broker_timeout) { 60 }
       let(:middleware) {
         ServiceBrokerRequestCounter.instance.limit = concurrent_limit
-        ServiceBrokerRateLimiter.new(app, logger: logger)
+        ServiceBrokerRateLimiter.new(app, logger: logger, broker_timeout_seconds: broker_timeout)
       }
 
       before(:each) do
@@ -67,7 +68,7 @@ module Middleware
                 'description' => 'Service broker concurrent request limit exceeded',
                 'error_code' => 'CF-ServiceBrokerRateLimitExceeded',
               )
-              expect(response_headers['Retry-After']).to be_within(90.second).of Time.now
+              expect(response_headers['Retry-After']).to be_between(Time.now + broker_timeout * 0.5, Time.now + broker_timeout * 1.5)
             end
           end
 
@@ -82,7 +83,20 @@ module Middleware
                 'detail' => 'Service broker concurrent request limit exceeded',
                 'title' => 'CF-ServiceBrokerRateLimitExceeded',
               )
-              expect(response_headers['Retry-After']).to be_within(90.second).of Time.now
+              expect(response_headers['Retry-After']).to be_between(Time.now + broker_timeout * 0.5, Time.now + broker_timeout * 1.5)
+            end
+          end
+
+          context 'when broker_client_timeout_seconds is reduced' do
+            let(:broker_timeout) { 3 }
+            let(:middleware) {
+              ServiceBrokerRequestCounter.instance.limit = concurrent_limit
+              ServiceBrokerRateLimiter.new(app, logger: logger, broker_timeout_seconds: broker_timeout)
+            }
+
+            it 'reduces the suggested delay in the Retry-After header' do
+              _, response_headers, _ = middleware.call(user_env)
+              expect(response_headers['Retry-After']).to be_between(Time.now + broker_timeout * 0.5, Time.now + broker_timeout * 1.5)
             end
           end
         end