v3(services) Job state set to failed when LO responds with failure (cloudfoundry#1900)

* v3(services) Job state set to failed when LO responds with failure

* fix failing tests

* Fix formatting

Author: FelisiaM

app/actions/v3/service_binding_create.rb

@@ -2,6 +2,9 @@
 
 module VCAP::CloudController
   module V3
+    class LastOperationFailedState < StandardError
+    end
+
     class ServiceBindingCreate
       PollingStatus = Struct.new(:finished, :retry_after).freeze
       PollingFinished = PollingStatus.new(true, nil).freeze
@@ -50,6 +53,10 @@ def poll(binding)
           }
         )
 
+        if details[:last_operation][:state] == 'failed'
+          raise LastOperationFailedState.new(details[:last_operation][:description])
+        end
+
         if binding.reload.terminal_state?
           PollingFinished
         else

spec/request/service_route_bindings_spec.rb

@@ -1069,7 +1069,7 @@
               end
             end
 
-            it_behaves_like 'binding last operation response handling', 'delete'
+            it_behaves_like 'unbinding last operation response handling', 'delete'
           end
 
           context 'when the broker returns a failure' do

spec/support/shared_examples/request/last_operation_response_handling.rb

@@ -1,8 +1,8 @@
-RSpec.shared_examples 'binding last operation response handling' do |type|
-  context 'valid http codes' do
+RSpec.shared_examples 'binding last operation response handling' do |operation_type|
+  context 'failure http codes' do
     valid_responses = [
-      { code: 400, body: { description: 'helpful message' }, expected_description: 'helpful message' },
-      { code: 200, body: { state: 'failed', description: 'something went wrong' }, expected_description: 'something went wrong' }
+      { code: 400, body: { description: 'helpful message' } },
+      { code: 200, body: { state: 'failed', description: 'something went wrong' } }
     ]
 
     valid_responses.each do |response|
@@ -11,21 +11,21 @@
         let(:last_operation_status_code) { response[:code] }
         let(:last_operation_body) { response[:body] }
 
-        it 'updates the binding and job' do
-          execute_all_jobs(expected_successes: 1, expected_failures: 0)
+        it 'updates the binding and job to failed' do
+          execute_all_jobs(expected_successes: 0, expected_failures: 1)
 
           binding.reload
-          expect(binding.last_operation.type).to eq(type)
+          expect(binding.last_operation.type).to eq(operation_type)
           expect(binding.last_operation.state).to eq('failed')
           expect(binding.last_operation.description).to eq(response[:body][:description])
 
-          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::COMPLETE_STATE)
+          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::FAILED_STATE)
         end
       end
     end
   end
 
-  context 'invalid http codes' do
+  context 'keep polling http codes' do
     [404, 500].each do |code|
       context "last operation response is #{code}" do
         let(:last_operation_status_code) { code }
@@ -35,7 +35,7 @@
           execute_all_jobs(expected_successes: 1, expected_failures: 0)
 
           binding.reload
-          expect(binding.last_operation.type).to eq(type)
+          expect(binding.last_operation.type).to eq(operation_type)
           expect(binding.last_operation.state).to eq('in progress')
           expect(binding.last_operation.description).to include("Status Code: #{code}")
 
@@ -57,7 +57,7 @@
       execute_all_jobs(expected_successes: 1, expected_failures: 0)
 
       binding.reload
-      expect(binding.last_operation.type).to eq(type)
+      expect(binding.last_operation.type).to eq(operation_type)
       expect(binding.last_operation.state).to eq('in progress')
 
       expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
@@ -77,7 +77,96 @@
         execute_all_jobs(expected_successes: 1, expected_failures: 0)
 
         binding.reload
-        expect(binding.last_operation.type).to eq(type)
+        expect(binding.last_operation.type).to eq(operation_type)
+        expect(binding.last_operation.state).to eq('in progress')
+
+        expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
+      end
+    end
+  end
+end
+
+# TODO: Merge these two shared examples into one in https://www.pivotaltracker.com/story/show/174748133
+RSpec.shared_examples 'unbinding last operation response handling' do |operation_type|
+  context 'failure http codes' do
+    valid_responses = [
+      { code: 400, body: { description: 'helpful message' } },
+      { code: 200, body: { state: 'failed', description: 'something went wrong' } }
+    ]
+
+    valid_responses.each do |response|
+      context "last operation response is #{response[:code]}" do
+        let(:state) { response[:state] }
+        let(:last_operation_status_code) { response[:code] }
+        let(:last_operation_body) { response[:body] }
+
+        it 'updates the binding and job to failed' do
+          execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+          binding.reload
+          expect(binding.last_operation.type).to eq(operation_type)
+          expect(binding.last_operation.state).to eq('failed')
+          expect(binding.last_operation.description).to eq(response[:body][:description])
+
+          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::COMPLETE_STATE)
+        end
+      end
+    end
+  end
+
+  context 'keep polling http codes' do
+    [404, 500].each do |code|
+      context "last operation response is #{code}" do
+        let(:last_operation_status_code) { code }
+        let(:last_operation_body) { 'something awful' }
+
+        it 'continues polling' do
+          execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+          binding.reload
+          expect(binding.last_operation.type).to eq(operation_type)
+          expect(binding.last_operation.state).to eq('in progress')
+          expect(binding.last_operation.description).to include("Status Code: #{code}")
+
+          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
+        end
+      end
+    end
+  end
+
+  context 'timeout' do
+    before do
+      stub_request(:get, broker_binding_last_operation_url).
+        with(query: hash_including({
+          operation: operation
+        })).to_timeout
+    end
+
+    it 'continues polling' do
+      execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+      binding.reload
+      expect(binding.last_operation.type).to eq(operation_type)
+      expect(binding.last_operation.state).to eq('in progress')
+
+      expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
+    end
+  end
+
+  context 'connection errors' do
+    [SocketError, Errno::ECONNREFUSED, RuntimeError].each do |error|
+      before do
+        stub_request(:get, broker_binding_last_operation_url).
+          with(query: hash_including({
+            operation: operation
+          })).to_raise(error)
+      end
+
+      it 'continues polling' do
+        execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+        binding.reload
+        expect(binding.last_operation.type).to eq(operation_type)
         expect(binding.last_operation.state).to eq('in progress')
 
         expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)

spec/support/shared_examples/v3_service_binding_create.rb

@@ -229,13 +229,8 @@ class BadError < StandardError; end
     context 'response says failed' do
       let(:state) { 'failed' }
 
-      it 'returns true' do
-        polling_status = action.poll(binding)
-        expect(polling_status[:finished]).to be_truthy
-      end
-
       it 'updates the last operation' do
-        action.poll(binding)
+        expect { action.poll(binding) }.to raise_error(VCAP::CloudController::V3::LastOperationFailedState)
 
         binding.reload
         expect(binding.last_operation.state).to eq('failed')

spec/unit/actions/service_credential_binding_create_spec.rb

@@ -332,8 +332,8 @@ module V3
 
           context 'response says failed' do
             let(:state) { 'failed' }
-            it 'does not notify diego or create an audit event' do
-              action.poll(binding)
+            it 'does not create an audit event' do
+              expect { action.poll(binding) }.to raise_error(VCAP::CloudController::V3::LastOperationFailedState)
 
               expect(@service_binding_event_repository).not_to have_received(:record_create)
             end

spec/unit/actions/service_route_binding_create_spec.rb

@@ -324,7 +324,7 @@ module V3
           context 'response says failed' do
             let(:state) { 'failed' }
             it 'does not notify diego or create an audit event' do
-              action.poll(binding)
+              expect { action.poll(binding) }.to raise_error(VCAP::CloudController::V3::LastOperationFailedState)
 
               expect(messenger).not_to have_received(:send_desire_request)
               expect(event_repository).not_to have_received(:record_service_instance_event)