v3(route-binding): delete when another operation is in progress (cloudfoundry#1929)

[#174748134](https://www.pivotaltracker.com/story/show/174748134)

Author: Derik Evangelista

app/actions/service_route_binding_delete.rb

@@ -3,6 +3,8 @@ module V3
     class ServiceRouteBindingDelete
       class UnprocessableDelete < StandardError; end
 
+      class ConcurrencyError < StandardError; end
+
       RequiresAsync = Class.new.freeze
 
       DeleteStatus = Struct.new(:finished, :operation).freeze
@@ -18,9 +20,8 @@ def initialize(service_event_repository)
       end
 
       def delete(binding, async_allowed:)
-        return RequiresAsync.new unless async_allowed || binding.service_instance.user_provided_instance?
-
         operation_in_progress! if binding.service_instance.operation_in_progress?
+        return RequiresAsync.new unless async_allowed || binding.service_instance.user_provided_instance?
 
         result = send_unbind_to_broker(binding)
         if result[:finished]
@@ -31,7 +32,9 @@ def delete(binding, async_allowed:)
 
         return result
       rescue => e
-        update_last_operation(binding, state: 'failed', description: e.message)
+        unless e.is_a? ConcurrencyError
+          update_last_operation(binding, state: 'failed', description: e.message)
+        end
 
         raise e
       end
@@ -65,6 +68,10 @@ def send_unbind_to_broker(binding)
         client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
         details = client.unbind(binding, nil, true)
         details[:async] ? DeleteStarted.call(details[:operation]) : DeleteComplete
+      rescue VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError
+        raise ConcurrencyError.new(
+          'The service broker rejected the request due to an operation being in progress for the service route binding'
+        )
       rescue => err
         raise UnprocessableDelete.new("Service broker failed to delete service binding for instance #{binding.service_instance.name}: #{err.message}")
       end
@@ -88,7 +95,7 @@ def update_last_operation(binding, description: nil, state: 'in progress', opera
           type: 'delete',
           state: state,
           description: description,
-          broker_provided_operation: operation
+          broker_provided_operation: operation || binding.last_operation.broker_provided_operation
         })
       end
 

app/controllers/v3/service_route_bindings_controller.rb

@@ -74,6 +74,8 @@ def destroy
     else
       head :no_content
     end
+  rescue V3::ServiceRouteBindingDelete::UnprocessableDelete => e
+    unprocessable!(e.message)
   end
 
   def parameters

app/jobs/v3/delete_route_binding_job.rb

@@ -61,7 +61,7 @@ def perform
           self.polling_interval_seconds = polling_status[:retry_after]
         end
       rescue => e
-        if route_binding.reload.last_operation.state != 'failed'
+        if route_binding.reload.last_operation.state != 'failed' && !e.is_a?(V3::ServiceRouteBindingDelete::ConcurrencyError)
           save_failure(e.message)
         end
         raise CloudController::Errors::ApiError.new_from_details('UnableToPerform', 'unbind', e.message)

lib/services/service_brokers/v2/client.rb

@@ -162,8 +162,12 @@ def unbind(service_binding, user_guid=nil, accepts_incomplete=false)
         async: async_response?(response),
         operation: parsed_response['operation']
       }
-    rescue VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError
-      raise CloudController::Errors::ApiError.new_from_details('AsyncServiceBindingOperationInProgress', service_binding.app.name, service_binding.service_instance.name)
+    rescue VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError => e
+      if service_binding.is_a? VCAP::CloudController::ServiceBinding
+        raise CloudController::Errors::ApiError.new_from_details('AsyncServiceBindingOperationInProgress', service_binding.app.name, service_binding.service_instance.name)
+      end
+
+      raise e
     end
 
     def update(instance, plan, accepts_incomplete: false, arbitrary_parameters: nil, previous_values: {}, maintenance_info: nil, name: instance.name)

spec/request/service_route_bindings_spec.rb

@@ -935,6 +935,18 @@
         let(:expected_codes_and_responses) { responses_for_space_restricted_async_delete_endpoint }
         let(:db_check) { lambda {} }
         let(:job) { VCAP::CloudController::PollableJobModel.last }
+        let(:broker_base_url) { service_instance.service_broker.broker_url }
+        let(:broker_unbind_url) { "#{broker_base_url}/v2/service_instances/#{service_instance.guid}/service_bindings/#{binding.guid}" }
+        let(:route_service_url) { 'https://route_service_url.com' }
+        let(:broker_unbind_status_code) { 200 }
+        let(:broker_response) { {} }
+        let(:query) do
+          {
+            service_id: service_instance.service_plan.service.unique_id,
+            plan_id: service_instance.service_plan.unique_id,
+            accepts_incomplete: true,
+          }
+        end
 
         it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS
 
@@ -955,19 +967,6 @@
         end
 
         describe 'the pollable job' do
-          let(:broker_base_url) { service_instance.service_broker.broker_url }
-          let(:broker_unbind_url) { "#{broker_base_url}/v2/service_instances/#{service_instance.guid}/service_bindings/#{binding.guid}" }
-          let(:route_service_url) { 'https://route_service_url.com' }
-          let(:broker_unbind_status_code) { 200 }
-          let(:broker_response) { {} }
-          let(:query) do
-            {
-              service_id: service_instance.service_plan.service.unique_id,
-              plan_id: service_instance.service_plan.unique_id,
-              accepts_incomplete: true,
-            }
-          end
-
           before do
             api_call.call(space_dev_headers)
             expect(last_response).to have_status_code(202)
@@ -1113,6 +1112,75 @@
             end
           end
         end
+
+        context 'when the service instance has an operation in progress' do
+          it 'responds with 422' do
+            service_instance.save_with_new_operation({}, { type: 'guacamole', state: 'in progress' })
+
+            api_call.call admin_headers
+            expect(last_response).to have_status_code(422)
+            expect(parsed_response['errors']).to include(include({
+              'detail' => include('There is an operation in progress for the service instance'),
+              'title' => 'CF-UnprocessableEntity',
+              'code' => 10008,
+            }))
+          end
+        end
+
+        context 'when the route binding is still creating' do
+          before do
+            binding.save_with_new_operation(
+              {},
+              { type: 'create', state: 'in progress', broker_provided_operation: 'very important info' }
+            )
+          end
+
+          context 'but the broker accepts the delete request' do
+            before do
+              @delete_stub = stub_request(:delete, broker_unbind_url).
+                             with(query: query).
+                             to_return(status: 202, body: '{"operation": "very important delete info"}', headers: {})
+
+              @last_op_stub = stub_request(:get, "#{broker_unbind_url}/last_operation").
+                              with(query: hash_including({
+                  operation: 'very important delete info'
+                })).
+                              to_return(status: 200, body: '{"state": "in progress"}', headers: {})
+            end
+
+            it 'starts the route binding deletion' do
+              api_call.call(admin_headers)
+              expect(last_response).to have_status_code(202)
+              execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+              expect(@delete_stub).to have_been_requested.once
+              expect(@last_op_stub).to have_been_requested
+
+              binding.reload
+              expect(binding.last_operation.type).to eq('delete')
+              expect(binding.last_operation.state).to eq('in progress')
+              expect(binding.last_operation.broker_provided_operation).to eq('very important delete info')
+            end
+          end
+
+          context 'and the broker rejects the delete request' do
+            before do
+              stub_request(:delete, broker_unbind_url).
+                with(query: query).
+                to_return(status: 422, body: '{"error": "ConcurrencyError"}', headers: {})
+
+              api_call.call(admin_headers)
+              execute_all_jobs(expected_successes: 0, expected_failures: 1)
+              binding.reload
+            end
+
+            it 'leaves the route binding in its current state' do
+              expect(binding.last_operation.type).to eq('create')
+              expect(binding.last_operation.state).to eq('in progress')
+              expect(binding.last_operation.broker_provided_operation).to eq('very important info')
+            end
+          end
+        end
       end
     end
 

spec/unit/actions/service_route_binding_delete_spec.rb

@@ -151,7 +151,7 @@ module V3
               end
             end
 
-            context 'broker returns an error' do
+            context 'broker returns a generic error' do
               let(:broker_client) do
                 dbl = instance_double(VCAP::Services::ServiceBrokers::V2::Client)
                 allow(dbl).to receive(:unbind).and_raise(StandardError, 'awful thing')
@@ -171,6 +171,38 @@ module V3
                 expect(route_binding.last_operation.description).to eq("Service broker failed to delete service binding for instance #{service_instance.name}: awful thing")
               end
             end
+
+            context 'broker returns a concurrency error' do
+              let(:broker_client) do
+                dbl = instance_double(VCAP::Services::ServiceBrokers::V2::Client)
+                allow(dbl).to receive(:unbind).and_raise(
+                  VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError.new(
+                    'foo',
+                    :delete,
+                    double(code: '500', reason: '', body: '')
+                  )
+                )
+                dbl
+              end
+
+              before do
+                route_binding.save_with_new_operation({}, { type: 'create', state: 'in progress', description: 'doing stuff' })
+              end
+
+              it 'fails with an appropriate error and does not alter the binding' do
+                expect {
+                  delete_binding
+                }.to raise_error(
+                  described_class::ConcurrencyError,
+                  'The service broker rejected the request due to an operation being in progress for the service route binding',
+                )
+
+                route_binding.reload
+                expect(route_binding.last_operation.type).to eq('create')
+                expect(route_binding.last_operation.state).to eq('in progress')
+                expect(route_binding.last_operation.description).to eq('doing stuff')
+              end
+            end
           end
         end
 

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

@@ -1541,6 +1541,25 @@ module VCAP::Services::ServiceBrokers::V2
           end
         end
       end
+
+      context 'ConcurrencyError error' do
+        let(:code) { 422 }
+        let(:response_body) { '{"error":"ConcurrencyError"}' }
+
+        context 'for app bindings' do
+          it 'propagates the error as an API error' do
+            expect { client.unbind(binding) }.to raise_error(CloudController::Errors::ApiError, /An operation for the service binding between app/)
+          end
+        end
+
+        context 'for a route binding' do
+          let(:binding) { VCAP::CloudController::RouteBinding.make }
+
+          it 'propagates the error as a ConcurrencyError' do
+            expect { client.unbind(binding) }.to raise_error(Errors::ConcurrencyError)
+          end
+        end
+      end
     end
 
     describe '#deprovision' do