Merge pull request #1202 from cloudfoundry-incubator/pr-dont-mitigate…

…-408

Don't orphan mitigate when Service Brokers return with 408 status code

[finishes #159731068]

app/jobs/services/service_binding_state_fetch.rb

@@ -29,7 +29,10 @@ def perform
           end
 
           retry_job unless service_binding.terminal_state?
-        rescue HttpResponseError, Sequel::Error, VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerApiTimeout => e
+        rescue HttpResponseError,
+               Sequel::Error,
+               VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerApiTimeout,
+               VCAP::Services::ServiceBrokers::V2::Errors::HttpClientTimeout => e
           logger.error("There was an error while fetching the service binding operation state: #{e}")
           retry_job
         end
@@ -46,7 +49,7 @@ def process_create_operation(logger, service_binding, last_operation_result)
 
             begin
               binding_response = client.fetch_service_binding(service_binding)
-            rescue HttpResponseError, VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerApiTimeout => e
+            rescue HttpResponseError, VCAP::Services::ServiceBrokers::V2::Errors::HttpClientTimeout => e
               set_binding_failed_state(service_binding, logger)
               logger.error("There was an error while fetching the service binding details: #{e}")
               return { finished: true }

lib/services/service_brokers/v2/client.rb

@@ -31,7 +31,13 @@ def provision(instance, arbitrary_parameters: {}, accepts_incomplete: false)
       }
 
       body[:parameters] = arbitrary_parameters if arbitrary_parameters.present?
-      response          = @http_client.put(path, body)
+
+      begin
+        response = @http_client.put(path, body)
+      rescue Errors::HttpClientTimeout => e
+        @orphan_mitigator.cleanup_failed_provision(@attrs, instance)
+        raise e
+      end
 
       parsed_response     = @response_parser.parse_provision(path, response)
       last_operation_hash = parsed_response['last_operation'] || {}
@@ -51,7 +57,7 @@ def provision(instance, arbitrary_parameters: {}, accepts_incomplete: false)
       return_values[:last_operation][:state] = state || 'succeeded'
 
       return_values
-    rescue Errors::ServiceBrokerApiTimeout, Errors::ServiceBrokerBadResponse => e
+    rescue Errors::ServiceBrokerBadResponse => e
       @orphan_mitigator.cleanup_failed_provision(@attrs, instance)
       raise e
     rescue Errors::ServiceBrokerResponseMalformed => e
@@ -89,11 +95,17 @@ def create_service_key(key, arbitrary_parameters: {})
 
       body[:parameters] = arbitrary_parameters if arbitrary_parameters.present?
 
-      response        = @http_client.put(path, body)
+      begin
+        response = @http_client.put(path, body)
+      rescue Errors::HttpClientTimeout => e
+        @orphan_mitigator.cleanup_failed_key(@attrs, key)
+        raise e
+      end
+
       parsed_response = @response_parser.parse_bind(path, response, service_guid: key.service.guid)
 
       { credentials: parsed_response['credentials'] }
-    rescue Errors::ServiceBrokerApiTimeout, Errors::ServiceBrokerBadResponse => e
+    rescue Errors::ServiceBrokerBadResponse => e
       @orphan_mitigator.cleanup_failed_key(@attrs, key)
       raise e
     end
@@ -110,7 +122,12 @@ def bind(binding, arbitrary_parameters, accepts_incomplete=false)
       body              = body.reject { |_, v| v.nil? }
       body[:parameters] = arbitrary_parameters if arbitrary_parameters.present?
 
-      response = @http_client.put(path, body)
+      begin
+        response = @http_client.put(path, body)
+      rescue Errors::HttpClientTimeout => e
+        @orphan_mitigator.cleanup_failed_bind(@attrs, binding)
+        raise e
+      end
 
       parsed_response = @response_parser.parse_bind(path, response, service_guid: binding.service.guid)
 
@@ -135,8 +152,7 @@ def bind(binding, arbitrary_parameters, accepts_incomplete=false)
         binding: attributes,
         operation: parsed_response['operation']
       }
-    rescue Errors::ServiceBrokerApiTimeout,
-           Errors::ServiceBrokerBadResponse,
+    rescue Errors::ServiceBrokerBadResponse,
            Errors::ServiceBrokerInvalidVolumeMounts,
            Errors::ServiceBrokerInvalidSyslogDrainUrl => e
       @orphan_mitigator.cleanup_failed_bind(@attrs, binding)

lib/services/service_brokers/v2/errors.rb

@@ -1,5 +1,6 @@
 require 'services/service_brokers/v2/errors/service_broker_api_unreachable'
 require 'services/service_brokers/v2/errors/service_broker_api_timeout'
+require 'services/service_brokers/v2/errors/http_client_timeout'
 require 'services/service_brokers/v2/errors/service_broker_response_malformed'
 require 'services/service_brokers/v2/errors/service_broker_api_authentication_failed'
 require 'services/service_brokers/v2/errors/service_broker_bad_response'

lib/services/service_brokers/v2/errors/http_client_timeout.rb

@@ -0,0 +1,24 @@
+require 'net/http'
+
+module VCAP::Services
+  module ServiceBrokers
+    module V2
+      module Errors
+        class HttpClientTimeout < HttpRequestError
+          def initialize(uri, method, source)
+            super(
+              "The request to the service broker timed out: #{uri}",
+              uri,
+              method,
+              source
+            )
+          end
+
+          def response_code
+            504
+          end
+        end
+      end
+    end
+  end
+end

lib/services/service_brokers/v2/http_client.rb

@@ -147,7 +147,7 @@ def make_request(method, uri, body=nil, options={})
       rescue SocketError, Errno::ECONNREFUSED => error
         raise Errors::ServiceBrokerApiUnreachable.new(uri.to_s, method, error)
       rescue HTTPClient::TimeoutError => error
-        raise Errors::ServiceBrokerApiTimeout.new(uri.to_s, method, error)
+        raise Errors::HttpClientTimeout.new(uri.to_s, method, error)
       rescue => error
         raise HttpRequestError.new(error.message, uri.to_s, method, error)
       end

spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb

@@ -353,5 +353,90 @@
         end
       end
     end
+
+    describe 'orphan mitigation strategy for 408s' do
+      context 'when provisioning' do
+        context 'when the broker responds with 408 (Client Timeout)' do
+          before do
+            stub_request(:put, %r{broker-url/v2/service_instances/[[:alnum:]-]+}).
+              to_return(status: 408)
+
+            body = {
+              name: 'test-service',
+              space_guid: @space_guid,
+              service_plan_guid: @plan_guid
+            }
+
+            post('/v2/service_instances',
+                 body.to_json,
+                 admin_headers)
+
+            Delayed::Worker.new.work_off
+          end
+
+          it 'should not orphan mitigate' do
+            expect(
+              a_request(:delete, %r{/v2/service_instances/[[:alnum:]-]+})
+            ).not_to have_been_made
+          end
+        end
+      end
+
+      context 'when binding' do
+        before do
+          provision_service
+          create_app
+        end
+
+        context 'when the broker responds with 408 (Client Timeout)' do
+          before do
+            stub_request(:put, %r{/v2/service_instances/#{@service_instance_guid}/service_bindings/[[:alnum:]-]+}).
+              to_return(status: 408)
+
+            body = { app_guid: @app_guid, service_instance_guid: @service_instance_guid }
+
+            post('/v2/service_bindings',
+                 body.to_json,
+                 admin_headers)
+
+            Delayed::Worker.new.work_off
+          end
+
+          it 'should not orphan mitigate' do
+            expect(
+              a_request(:delete, %r{/v2/service_instances/#{@service_instance_guid}/service_bindings/[[:alnum:]-]+})
+            ).not_to have_been_made
+          end
+        end
+      end
+
+      context 'when service key' do
+        before do
+          provision_service
+          create_app
+        end
+
+        context 'when the broker responds with 408 (Client Timeout)' do
+          before do
+            stub_request(:put, %r{/v2/service_instances/#{@service_instance_guid}/service_bindings/[[:alnum:]-]+}).
+              to_return(status: 408)
+
+            body = { name: 'service-key', service_instance_guid: @service_instance_guid }
+
+            post('/v2/service_keys',
+                 body.to_json,
+                 admin_headers)
+
+            Delayed::Worker.new.work_off
+          end
+
+          it 'should not orphan mitigate' do
+            expect(
+              a_request(:delete, %r{/v2/service_instances/#{@service_instance_guid}/service_bindings/[[:alnum:]-]+})
+            ).not_to have_been_made
+          end
+        end
+      end
+    end
   end
 end

spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb

@@ -17,7 +17,7 @@
       'broker_api_v2.11_spec.rb' => '99e61dc50ceb635b09b3bd16901a4fa6',
       'broker_api_v2.12_spec.rb' => '45db41892336b8bf8748fd5ae484bc29',
       'broker_api_v2.13_spec.rb' => '06b4683ffd7f69d800c3b9097bc9cd73',
-      'broker_api_v2.14_spec.rb' => '56701a44910092ab3ff15eb860974bb8',
+      'broker_api_v2.14_spec.rb' => '5334b31daee43e714f6a91d0a1da82c8',
     }
   end
   let(:digester) { Digester.new(algorithm: Digest::MD5) }

spec/acceptance/orphan_mitigation_spec.rb

@@ -69,7 +69,7 @@ module VCAP::CloudController
           admin_headers)
       end
 
-      it 'makes the request to the broker and deprovisions' do
+      it 'makes the request to the broker and unbinds' do
         expect(a_request(:put, %r{http://broker-url/v2/service_instances/#{service_instance_guid}/service_bindings/#{guid_pattern}}).with(basic_auth: basic_auth)).
           to have_been_made
 

spec/unit/jobs/services/service_binding_state_fetch_spec.rb

@@ -251,7 +251,7 @@ def run_job(job)
                   )
                 }
                 let(:binding_response) { { 'credentials': '{}' } }
-                let(:timeout_exception) { VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerApiTimeout.new(nil, nil, broker_response) }
+                let(:timeout_exception) { VCAP::Services::ServiceBrokers::V2::Errors::HttpClientTimeout.new(nil, nil, broker_response) }
 
                 before do
                   allow(client).to receive(:fetch_service_binding).with(service_binding).and_raise(timeout_exception)
@@ -556,7 +556,7 @@ def run_job(job)
             end
           end
 
-          context 'when calling last operation times out' do
+          context 'when last operation request times out on the broker' do
             before do
               err = VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerApiTimeout.new('uri', 'GET', {})
               allow(client).to receive(:fetch_service_binding_last_operation).and_raise(err)
@@ -573,6 +573,23 @@ def run_job(job)
             end
           end
 
+          context 'when the http call for last operation times out' do
+            before do
+              err = VCAP::Services::ServiceBrokers::V2::Errors::HttpClientTimeout.new('uri', 'GET', {})
+              allow(client).to receive(:fetch_service_binding_last_operation).and_raise(err)
+              run_job(job)
+            end
+
+            it 'should enqueue another fetch job' do
+              expect(Delayed::Job.count).to eq 1
+            end
+
+            it 'maintains the service binding last operation details' do
+              service_binding.reload
+              expect(service_binding.last_operation.state).to eq('in progress')
+            end
+          end
+
           context 'when a database operation fails' do
             before do
               allow(ServiceBinding).to receive(:first).and_raise(Sequel::Error)

spec/unit/lib/services/service_brokers/v2/client_spec.rb

@@ -267,13 +267,13 @@ module VCAP::Services::ServiceBrokers::V2
             allow(http_client).to receive(:put).and_raise(error)
           end
 
-          context 'Errors::ServiceBrokerApiTimeout error' do
-            let(:error) { Errors::ServiceBrokerApiTimeout.new(uri, :put, Timeout::Error.new) }
+          context 'Errors::HttpClientTimeout error' do
+            let(:error) { Errors::HttpClientTimeout.new(uri, :put, Timeout::Error.new) }
 
             it 'propagates the error and follows up with a deprovision request' do
               expect {
                 client.provision(instance)
-              }.to raise_error(Errors::ServiceBrokerApiTimeout)
+              }.to raise_error(Errors::HttpClientTimeout)
 
               expect(orphan_mitigator).to have_received(:cleanup_failed_provision).with(client_attrs, instance)
             end
@@ -291,13 +291,12 @@ module VCAP::Services::ServiceBrokers::V2
           context 'Errors::ServiceBrokerApiTimeout error' do
             let(:error) { Errors::ServiceBrokerApiTimeout.new(uri, :put, Timeout::Error.new) }
 
-            it 'propagates the error and follows up with a deprovision request' do
+            it 'propagates the error and does not follow up with a deprovision request' do
               expect {
                 client.provision(instance)
               }.to raise_error(Errors::ServiceBrokerApiTimeout)
 
-              expect(orphan_mitigator).to have_received(:cleanup_failed_provision).
-                with(client_attrs, instance)
+              expect(orphan_mitigator).not_to have_received(:cleanup_failed_provision)
             end
           end
 
@@ -878,12 +877,12 @@ module VCAP::Services::ServiceBrokers::V2
           end
 
           context 'Errors::ServiceBrokerApiTimeout error' do
-            let(:error) { Errors::ServiceBrokerApiTimeout.new(uri, :put, Timeout::Error.new) }
+            let(:error) { Errors::HttpClientTimeout.new(uri, :put, Timeout::Error.new) }
 
             it 'propagates the error and follows up with a deprovision request' do
               expect {
                 client.create_service_key(key)
-              }.to raise_error(Errors::ServiceBrokerApiTimeout)
+              }.to raise_error(Errors::HttpClientTimeout)
 
               expect(orphan_mitigator).to have_received(:cleanup_failed_key).
                 with(client_attrs, key)
@@ -907,7 +906,7 @@ module VCAP::Services::ServiceBrokers::V2
                 client.create_service_key(key)
               }.to raise_error(Errors::ServiceBrokerApiTimeout)
 
-              expect(orphan_mitigator).to have_received(:cleanup_failed_key).with(client_attrs, key)
+              expect(orphan_mitigator).not_to have_received(:cleanup_failed_key)
             end
           end
 
@@ -1170,13 +1169,13 @@ module VCAP::Services::ServiceBrokers::V2
             allow(http_client).to receive(:put).and_raise(error)
           end
 
-          context 'Errors::ServiceBrokerApiTimeout error' do
-            let(:error) { Errors::ServiceBrokerApiTimeout.new(uri, :put, Timeout::Error.new) }
+          context 'Errors::HttpClientTimeout error' do
+            let(:error) { Errors::HttpClientTimeout.new(uri, :put, Timeout::Error.new) }
 
-            it 'propagates the error and follows up with a deprovision request' do
+            it 'propagates the error and cleans up the failed binding' do
               expect {
                 client.bind(binding, arbitrary_parameters)
-              }.to raise_error(Errors::ServiceBrokerApiTimeout)
+              }.to raise_error(Errors::HttpClientTimeout)
 
               expect(orphan_mitigator).to have_received(:cleanup_failed_bind).
                 with(client_attrs, binding)
@@ -1195,12 +1194,12 @@ module VCAP::Services::ServiceBrokers::V2
           context 'Errors::ServiceBrokerApiTimeout error' do
             let(:error) { Errors::ServiceBrokerApiTimeout.new(uri, :put, Timeout::Error.new) }
 
-            it 'propagates the error and follows up with a deprovision request' do
+            it 'propagates the error but does not clean up the binding' do
               expect {
                 client.bind(binding, arbitrary_parameters)
               }.to raise_error(Errors::ServiceBrokerApiTimeout)
 
-              expect(orphan_mitigator).to have_received(:cleanup_failed_bind).with(client_attrs, binding)
+              expect(orphan_mitigator).not_to have_received(:cleanup_failed_bind)
             end
           end
 

spec/unit/lib/services/service_brokers/v2/http_client_spec.rb

@@ -210,7 +210,7 @@ module VCAP::Services::ServiceBrokers::V2
 
           it 'raises a timeout error' do
             expect { request }.
-              to raise_error(Errors::ServiceBrokerApiTimeout)
+              to raise_error(Errors::HttpClientTimeout)
           end
         end
       end