v3(bindings): orphan mitigation on create failures (cloudfoundry#1914)

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

Co-authored-by: Felisia Martini <fmartini@pivotal.io>

Author: Derik Evangelista

spec/request/service_credential_bindings_spec.rb

@@ -1398,6 +1398,14 @@ def check_filtered_bindings(*bindings)
             end
           end
         end
+
+        context 'orphan mitigation' do
+          it_behaves_like 'create binding orphan mitigation' do
+            let(:bind_url) { broker_bind_url }
+            let(:plan_id) { plan.unique_id }
+            let(:offering_id) { offering.unique_id }
+          end
+        end
       end
     end
   end

spec/support/shared_examples/request/bindings_orphan_mitigation.rb

@@ -1,5 +1,7 @@
 RSpec.shared_examples 'create binding orphan mitigation' do
   let(:job) { VCAP::CloudController::PollableJobModel.last }
+  let(:bind_response_body) { '{}' }
+  let(:broker_bind_status_code) { 200 }
 
   before do
     stub_request(:delete, bind_url).
@@ -8,133 +10,136 @@
         plan_id: plan_id,
         service_id: offering_id,
       }).to_return(status: 200, body: {}.to_json)
+
+    stub_request(:put, bind_url).
+      with(query: { accepts_incomplete: true },
+        body: client_body).to_return(status: broker_bind_status_code, body: bind_response_body)
   end
 
-  context 'broker returns a success code' do
-    codes = [200, 201]
-    codes.each do |code|
-      context "response is #{code}" do
-        before do
-          stub_request(:put, bind_url).
-            with(query: {
-              accepts_incomplete: true,
-            },
-              body: client_body).to_return(status: code, body: {}.to_json)
-        end
+  context 'should not be performed' do
+    before do
+      stub_request(:get, "#{bind_url}/last_operation").
+        with({ query: { plan_id: plan_id, service_id: offering_id } }).
+        to_return(status: 200, body: '{"state": "in progress"}')
+    end
+
+    context 'broker returns valid 200, 201, 202' do
+      [200, 201, 202].each do |code|
+        context "response is #{code}" do
+          let(:broker_bind_status_code) { code }
 
-        it 'updates the binding and job' do
-          execute_all_jobs(expected_successes: 1, expected_failures: 0)
+          it 'succeeds the job and does not perform orphan mitigation' do
+            execute_all_jobs(expected_successes: 1, expected_failures: 0)
+            assert_no_orphan_mitigation_performed(plan_id, offering_id)
+          end
+        end
+      end
+    end
 
-          expect(binding.last_operation.type).to eq('create')
-          expect(binding.last_operation.state).to eq('succeeded')
+    context 'response is 200 with malformed response' do
+      let(:broker_bind_status_code) { 200 }
+      let(:bind_response_body) { nil }
 
-          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::COMPLETE_STATE)
+      it 'fails the job and does not perform orphan mitigation' do
+        execute_all_jobs(expected_successes: 0, expected_failures: 1)
 
-          expect(
-            a_request(:delete, bind_url).
-              with(
-                query: {
-                  accepts_incomplete: true,
-                  plan_id: plan_id,
-                  service_id: offering_id,
-                },
-              )
-          ).not_to have_been_made
-        end
+        assert_failed_job(binding, job)
+        assert_no_orphan_mitigation_performed(plan_id, offering_id)
       end
     end
 
-    context 'response is 201 with malformed response' do
+    context 'response for last operation is 200 state failed' do
+      let(:broker_bind_status_code) { 202 }
       before do
-        stub_request(:put, bind_url).
-          with(query: {
-            accepts_incomplete: true,
-          },
-            body: client_body).to_return(status: 201, body: nil)
+        stub_request(:get, "#{bind_url}/last_operation").
+          with({ query: { plan_id: plan_id, service_id: offering_id } }).
+          to_return(status: 200, body: '{"state": "failed"}')
       end
 
-      it 'updates the binding and performs orphan mitigation' do
-        execute_all_jobs(expected_successes: 1, expected_failures: 1)
+      it 'fails the job and does not perform orphan mitigation' do
+        execute_all_jobs(expected_successes: 0, expected_failures: 1)
 
         assert_failed_job(binding, job)
-        assert_orphan_mitigation_performed(plan_id, offering_id)
+        assert_no_orphan_mitigation_performed(plan_id, offering_id)
       end
     end
-  end
 
-  context 'broker does not process the request' do
-    [400, 401, 408, 409, *411..431].each do |code|
-      context "response is #{code}" do
-        before do
-          stub_request(:put, bind_url).
-            with(query: {
-              accepts_incomplete: true,
-            },
-              body: client_body).to_return(status: code, body: { error: 'ConcurrencyError', description: 'some description' }.to_json)
-        end
+    context 'broker returns a 4xx code' do
+      [400, 401, 408, 409, *411..431].each do |code|
+        context "response is #{code}" do
+          let(:broker_bind_status_code) { code }
+          let(:bind_response_body) { '{ "error": "ConcurrencyError", "description": "some description" }' }
 
-        it 'updates the binding and job' do
-          execute_all_jobs(expected_successes: 0, expected_failures: 1)
-
-          assert_failed_job(binding, job)
-
-          expect(
-            a_request(:delete, bind_url).
-              with(
-                query: {
-                  accepts_incomplete: true,
-                  plan_id: plan_id,
-                  service_id: offering_id,
-                },
-              )
-          ).not_to have_been_made
+          it 'fails the job and updates the binding and job' do
+            execute_all_jobs(expected_successes: 0, expected_failures: 1)
+
+            assert_failed_job(binding, job)
+            assert_no_orphan_mitigation_performed(plan_id, offering_id)
+          end
         end
       end
     end
   end
 
-  context 'broker fails to bind with an error not specified in osbapi' do
-    [*500..511, *203..208].sample(4).each do |code|
-      context "response is #{code}" do
-        before do
-          stub_request(:put, bind_url).
-            with(query: {
-              accepts_incomplete: true,
-            },
-              body: client_body).to_return(status: code, body: {}.to_json)
+  context 'should be performed' do
+    context 'broker returns a 2xx code' do
+      [201, 202].each do |code|
+        context "response is #{code} with malformed response" do
+          let(:broker_bind_status_code) { code }
+          let(:bind_response_body) { nil }
+
+          it 'performs orphan mitigation' do
+            execute_all_jobs(expected_successes: 1, expected_failures: 1)
+
+            assert_failed_job(binding, job)
+            assert_orphan_mitigation_performed(plan_id, offering_id)
+          end
         end
+      end
 
-        it 'does orphan mitigation and fails the job' do
-          execute_all_jobs(expected_successes: 1, expected_failures: 1)
+      [203, 204, 205, 206, 206, 208, 226].each do |code|
+        context "response is #{code}" do
+          let(:broker_bind_status_code) { code }
+          let(:bind_response_body) { '{}' }
 
-          assert_failed_job(binding, job)
-          assert_orphan_mitigation_performed(plan_id, offering_id)
+          it 'performs orphan mitigation' do
+            execute_all_jobs(expected_successes: 1, expected_failures: 1)
+            assert_orphan_mitigation_performed(plan_id, offering_id)
+          end
         end
       end
     end
-  end
 
-  context 'timeout' do
-    before do
-      stub_request(:put, bind_url).
-        with(query: {
-          accepts_incomplete: true,
-        },
-          body: client_body).to_timeout
-
-      stub_request(:delete, bind_url).
-        with(query: {
-          accepts_incomplete: true,
-          plan_id: plan_id,
-          service_id: offering_id,
-        }).to_return(status: 200, body: {}.to_json)
+    context 'broker returns a 5xx code' do
+      [*500..511].each do |code|
+        context "response is #{code}" do
+          let(:broker_bind_status_code) { code }
+
+          it 'does orphan mitigation and fails the job' do
+            execute_all_jobs(expected_successes: 1, expected_failures: 1)
+
+            assert_failed_job(binding, job)
+            assert_orphan_mitigation_performed(plan_id, offering_id)
+          end
+        end
+      end
     end
 
-    it 'does orphan mitigation and fails the job' do
-      execute_all_jobs(expected_successes: 1, expected_failures: 1)
+    context 'timeout' do
+      before do
+        stub_request(:put, bind_url).
+          with(query: {
+            accepts_incomplete: true,
+          },
+            body: client_body).to_timeout
+      end
 
-      assert_failed_job(binding, job)
-      assert_orphan_mitigation_performed(plan_id, offering_id)
+      it 'does orphan mitigation and fails the job' do
+        execute_all_jobs(expected_successes: 1, expected_failures: 1)
+
+        assert_failed_job(binding, job)
+        assert_orphan_mitigation_performed(plan_id, offering_id)
+      end
     end
   end
 end
@@ -147,14 +152,20 @@ def assert_failed_job(binding, job)
 end
 
 def assert_orphan_mitigation_performed(plan_id, offering_id)
-  expect(
-    a_request(:delete, bind_url).
-      with(
-        query: {
-          accepts_incomplete: true,
-          plan_id: plan_id,
-          service_id: offering_id,
-        },
-      )
-  ).to have_been_made.once
+  expect(delete_request(plan_id, offering_id)).to have_been_made.once
+end
+
+def assert_no_orphan_mitigation_performed(plan_id, offering_id)
+  expect(delete_request(plan_id, offering_id)).to_not have_been_made
+end
+
+def delete_request(plan_id, offering_id)
+  a_request(:delete, bind_url).
+    with(
+      query: {
+        accepts_incomplete: true,
+        plan_id: plan_id,
+        service_id: offering_id,
+      },
+    )
 end