From 69a5d3618227a091dcf2a086bd1abd87b1e0aabb Mon Sep 17 00:00:00 2001 From: Niki Maslarski Date: Wed, 8 Aug 2018 15:51:05 +0100 Subject: [PATCH] Don't orphan mitigate 408 responses * Service Instances Create * Service Bindings Create * Service Keys Create [finishes #159459070] Signed-off-by: Jen Spinney Signed-off-by: William Martin Signed-off-by: Luis Urraca --- .../services/service_binding_state_fetch.rb | 7 +- lib/services/service_brokers/v2/client.rb | 30 +++++-- lib/services/service_brokers/v2/errors.rb | 1 + .../v2/errors/http_client_timeout.rb | 24 ++++++ .../service_brokers/v2/http_client.rb | 2 +- .../broker_api_v2.14_spec.rb | 85 +++++++++++++++++++ .../broker_api_versions_spec.rb | 2 +- spec/acceptance/orphan_mitigation_spec.rb | 2 +- .../service_binding_state_fetch_spec.rb | 21 ++++- .../service_brokers/v2/client_spec.rb | 29 +++---- .../service_brokers/v2/http_client_spec.rb | 2 +- 11 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 lib/services/service_brokers/v2/errors/http_client_timeout.rb diff --git a/app/jobs/services/service_binding_state_fetch.rb b/app/jobs/services/service_binding_state_fetch.rb index e2e6d716be8..cb2fe26405a 100644 --- a/app/jobs/services/service_binding_state_fetch.rb +++ b/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 } diff --git a/lib/services/service_brokers/v2/client.rb b/lib/services/service_brokers/v2/client.rb index ea6b2137186..fc537d680ad 100644 --- a/lib/services/service_brokers/v2/client.rb +++ b/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) diff --git a/lib/services/service_brokers/v2/errors.rb b/lib/services/service_brokers/v2/errors.rb index 6eb9deab147..3e9770bea6c 100644 --- a/lib/services/service_brokers/v2/errors.rb +++ b/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' diff --git a/lib/services/service_brokers/v2/errors/http_client_timeout.rb b/lib/services/service_brokers/v2/errors/http_client_timeout.rb new file mode 100644 index 00000000000..28c6c2fb5b2 --- /dev/null +++ b/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 diff --git a/lib/services/service_brokers/v2/http_client.rb b/lib/services/service_brokers/v2/http_client.rb index 45aa160bfe4..1b873f690fb 100644 --- a/lib/services/service_brokers/v2/http_client.rb +++ b/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 diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb index aae353f6e58..0d1177ff630 100644 --- a/spec/acceptance/broker_api_compatibility/broker_api_v2.14_spec.rb +++ b/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 diff --git a/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb index 5adc4e3641d..0d05da247ce 100644 --- a/spec/acceptance/broker_api_compatibility/broker_api_versions_spec.rb +++ b/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) } diff --git a/spec/acceptance/orphan_mitigation_spec.rb b/spec/acceptance/orphan_mitigation_spec.rb index 5fec66aece7..285e66b09ff 100644 --- a/spec/acceptance/orphan_mitigation_spec.rb +++ b/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 diff --git a/spec/unit/jobs/services/service_binding_state_fetch_spec.rb b/spec/unit/jobs/services/service_binding_state_fetch_spec.rb index eb33e3f3fbb..9c83d141a61 100644 --- a/spec/unit/jobs/services/service_binding_state_fetch_spec.rb +++ b/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) diff --git a/spec/unit/lib/services/service_brokers/v2/client_spec.rb b/spec/unit/lib/services/service_brokers/v2/client_spec.rb index 0bd1a508eaa..e2bb7931b7b 100644 --- a/spec/unit/lib/services/service_brokers/v2/client_spec.rb +++ b/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 diff --git a/spec/unit/lib/services/service_brokers/v2/http_client_spec.rb b/spec/unit/lib/services/service_brokers/v2/http_client_spec.rb index 4a9ad1f9439..efa1be295d8 100644 --- a/spec/unit/lib/services/service_brokers/v2/http_client_spec.rb +++ b/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