V2 Rate Limiter: improve code formatting

- Replace 'skip_rate_limiting' with 'apply_rate_limiting'
- Code cleanup
- Improve signatures & code structure
- Use shared test examples
- Fix admin test

lib/cloud_controller/config_schemas/base/api_schema.rb

@@ -295,7 +295,7 @@ class ApiSchema < VCAP::Config
             max_concurrent_service_broker_requests: Integer,
             shared_isolation_segment_name: String,
 
-            rate_limiter_v2_api: {
+            optional(:rate_limiter_v2_api) => {
               enabled: bool,
               per_process_general_limit: Integer,
               global_general_limit: Integer,

middleware/base_rate_limiter.rb

@@ -1,16 +1,14 @@
+require 'mixins/client_ip'
+require 'mixins/user_reset_interval'
+
 module CloudFoundry
   module Middleware
-    RequestCount = Struct.new(:requests, :valid_until)
-
-    class BaseRequestCounter
+    class RequestCounter
       include CloudFoundry::Middleware::UserResetInterval
 
-      def initialize
-        reset
-      end
+      RequestCount = Struct.new(:requests, :valid_until)
 
-      # needed for testing
-      def reset
+      def initialize
         @mutex = Mutex.new
         @data = {}
       end
@@ -58,7 +56,7 @@ def initialize(suffix)
         @remaining = nil
       end
 
-      def as_hash
+      def to_hash
         return {} if [@limit, @reset, @remaining].all?(&:nil?)
 
         { "#{@prefix}-Limit#{@suffix}" => @limit, "#{@prefix}-Reset#{@suffix}" => @reset, "#{@prefix}-Remaining#{@suffix}" => @remaining }
@@ -81,7 +79,7 @@ def call(env)
 
         request = ActionDispatch::Request.new(env)
 
-        unless skip_rate_limiting?(env, request)
+        if apply_rate_limiting?(env)
           user_guid = user_token?(env) ? env['cf.user_guid'] : client_ip(request)
 
           count, valid_until = @request_counter.get(user_guid, @reset_interval, @logger)
@@ -97,21 +95,25 @@ def call(env)
         end
 
         status, headers, body = @app.call(env)
-        [status, headers.merge(rate_limit_headers.as_hash), body]
+        [status, headers.merge(rate_limit_headers.to_hash), body]
       end
 
       private
 
-      def skip_rate_limiting?(env, request)
-        raise NotImplementedError
+      def apply_rate_limiting?(env)
+        raise 'method should be implemented in concrete class'
       end
 
       def global_request_limit(env)
-        raise NotImplementedError
+        raise 'method should be implemented in concrete class'
       end
 
       def rate_limit_error(env)
-        raise NotImplementedError
+        raise 'method should be implemented in concrete class'
+      end
+
+      def per_process_request_limit(env)
+        raise 'method should be implemented in concrete class'
       end
 
       def exceeded_rate_limit(count, env)
@@ -131,8 +133,9 @@ def user_token?(env)
         !!env['cf.user_guid']
       end
 
-      def basic_auth?(auth)
-        (auth.provided? && auth.basic?)
+      def basic_auth?(env)
+        auth = Rack::Auth::Basic::Request.new(env)
+        auth.provided? && auth.basic?
       end
 
       def admin?
@@ -145,7 +148,7 @@ def too_many_requests!(env, rate_limit_headers)
         headers['Content-Type'] = 'text/plain; charset=utf-8'
         message = rate_limit_error(env).to_json
         headers['Content-Length'] = message.length.to_s
-        [429, rate_limit_headers.as_hash.merge(headers), [message]]
+        [429, rate_limit_headers.to_hash.merge(headers), [message]]
       end
     end
   end

middleware/rate_limiter.rb

@@ -1,12 +1,10 @@
-require 'mixins/client_ip'
-require 'mixins/user_reset_interval'
 require 'base_rate_limiter'
 
 module CloudFoundry
   module Middleware
-    REQUEST_COUNTER = BaseRequestCounter.new
-
     class RateLimiter < BaseRateLimiter
+      REQUEST_COUNTER = RequestCounter.new
+
       def initialize(app, opts)
         @per_process_general_limit         = opts[:per_process_general_limit]
         @global_general_limit              = opts[:global_general_limit]
@@ -17,9 +15,9 @@ def initialize(app, opts)
 
       private
 
-      def skip_rate_limiting?(env, request)
-        auth = Rack::Auth::Basic::Request.new(env)
-        basic_auth?(auth) || internal_api?(request) || root_api?(request) || admin?
+      def apply_rate_limiting?(env)
+        request = ActionDispatch::Request.new(env)
+        !basic_auth?(env) && !internal_api?(request) && !root_api?(request) && !admin?
       end
 
       def root_api?(request)

middleware/rate_limiter_v2_api.rb

@@ -1,25 +1,27 @@
-require 'mixins/client_ip'
-require 'mixins/user_reset_interval'
 require 'base_rate_limiter'
 
 module CloudFoundry
   module Middleware
-    REQUEST_COUNTER_V2_API = BaseRequestCounter.new
-
     class RateLimiterV2API < BaseRateLimiter
+      REQUEST_COUNTER = RequestCounter.new
+
       def initialize(app, opts)
         @per_process_general_limit = opts[:per_process_general_limit]
         @global_general_limit      = opts[:global_general_limit]
         @per_process_admin_limit   = opts[:per_process_admin_limit]
         @global_admin_limit        = opts[:global_admin_limit]
-        super(app, opts[:logger], REQUEST_COUNTER_V2_API, opts[:interval], 'V2-API')
+        super(app, opts[:logger], REQUEST_COUNTER, opts[:interval], 'V2-API')
       end
 
       private
 
-      def skip_rate_limiting?(env, request)
-        auth = Rack::Auth::Basic::Request.new(env)
-        basic_auth?(auth) || !request.fullpath.match(%r{\A/v2/(?!(info)).+})
+      def apply_rate_limiting?(env)
+        !basic_auth?(env) && v2_api?(env)
+      end
+
+      def v2_api?(env)
+        request = ActionDispatch::Request.new(env)
+        request.fullpath.match(%r{\A/v2/(?!(info)).+})
       end
 
       def global_request_limit(env)
@@ -31,8 +33,7 @@ def per_process_request_limit(env)
       end
 
       def rate_limit_error(env)
-        error_name = 'RateLimitV2APIExceeded'
-        api_error = CloudController::Errors::ApiError.new_from_details(error_name)
+        api_error = CloudController::Errors::ApiError.new_from_details('RateLimitV2APIExceeded')
         ErrorPresenter.new(api_error, Rails.env.test?, V2ErrorHasher.new(api_error)).to_hash
       end
     end

spec/support/shared_examples/middleware/rate_limiting.rb

@@ -0,0 +1,11 @@
+RSpec.shared_examples_for 'endpoint exempts from rate limiting' do |header_suffix=''|
+  it 'exempts them from rate limiting' do
+    allow(ActionDispatch::Request).to receive(:new).and_return(fake_request)
+    _, response_headers, _ = middleware.call(env)
+    expect(request_counter).not_to have_received(:get)
+    expect(request_counter).not_to have_received(:increment)
+    expect(response_headers["X-RateLimit-Limit#{header_suffix}"]).to be_nil
+    expect(response_headers["X-RateLimit-Remaining#{header_suffix}"]).to be_nil
+    expect(response_headers["X-RateLimit-Reset#{header_suffix}"]).to be_nil
+  end
+end