From 4ab23c1baf621af7fad956636b530edcb0c54b62 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Thu, 7 Apr 2022 13:10:36 +0200 Subject: [PATCH 1/5] Fix state handling for POST /v3/spaces/:guid/actions/apply_manifest - Return an error when service instance has state `create failed` - Return an error when last operation is `delete in progress` --- app/actions/app_apply_manifest.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 8b3bd86b0b1..b4e6c13e456 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -12,6 +12,7 @@ module VCAP::CloudController class AppApplyManifest class Error < StandardError; end class NoDefaultDomain < StandardError; end + class ServiceInstanceError < StandardError; end class ServiceBindingError < StandardError; end class ServiceBrokerRespondedAsyncWhenNotAllowed < StandardError; end SERVICE_BINDING_TYPE = 'app'.freeze @@ -140,6 +141,12 @@ def create_service_bindings(manifest_service_bindings_message, app) manifest_service_bindings_message.manifest_service_bindings.each do |manifest_service_binding| service_instance = app.space.find_visible_service_instance_by_name(manifest_service_binding.name) service_instance_not_found!(manifest_service_binding.name) unless service_instance + + if service_instance.type == 'managed_service_instance' + service_instance_not_found! if service_instance.create_failed? + delete_in_progress_or_failed!(service_instance) if service_instance.last_operation_is_delete? + end + binding = ServiceBinding.first(service_instance: service_instance, app: app) next if binding&.create_succeeded? @@ -214,6 +221,10 @@ def service_instance_not_found!(name) raise CloudController::Errors::NotFound.new_from_details('ResourceNotFound', "Service instance '#{name}' not found") end + def delete_in_progress_or_failed!(service_instance) + raise ServiceInstanceError.new("The service instance '#{service_instance.name}' is getting deleted or its deletion failed. Therefore, no binding can be created.") + end + def volume_services_enabled? VCAP::CloudController::Config.config.get(:volume_services_enabled) end From c3225af3db5d856cadb6a9eeff580f152f00ba7b Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Thu, 7 Apr 2022 13:46:30 +0200 Subject: [PATCH 2/5] Add tests for service instance state handling in apply_manifest --- app/actions/app_apply_manifest.rb | 6 +-- spec/unit/actions/app_apply_manifest_spec.rb | 39 ++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index b4e6c13e456..4d467056caf 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -12,7 +12,6 @@ module VCAP::CloudController class AppApplyManifest class Error < StandardError; end class NoDefaultDomain < StandardError; end - class ServiceInstanceError < StandardError; end class ServiceBindingError < StandardError; end class ServiceBrokerRespondedAsyncWhenNotAllowed < StandardError; end SERVICE_BINDING_TYPE = 'app'.freeze @@ -143,7 +142,7 @@ def create_service_bindings(manifest_service_bindings_message, app) service_instance_not_found!(manifest_service_binding.name) unless service_instance if service_instance.type == 'managed_service_instance' - service_instance_not_found! if service_instance.create_failed? + service_instance_not_found!(service_instance.name) if service_instance.create_failed? delete_in_progress_or_failed!(service_instance) if service_instance.last_operation_is_delete? end @@ -222,7 +221,8 @@ def service_instance_not_found!(name) end def delete_in_progress_or_failed!(service_instance) - raise ServiceInstanceError.new("The service instance '#{service_instance.name}' is getting deleted or its deletion failed. Therefore, no binding can be created.") + error_message = 'The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.' + raise_binding_error!(service_instance, error_message) end def volume_services_enabled? diff --git a/spec/unit/actions/app_apply_manifest_spec.rb b/spec/unit/actions/app_apply_manifest_spec.rb index aa9e72b4219..453137e2904 100644 --- a/spec/unit/actions/app_apply_manifest_spec.rb +++ b/spec/unit/actions/app_apply_manifest_spec.rb @@ -931,6 +931,45 @@ module VCAP::CloudController /For service 'si-name': A binding failed to be deleted. Resolve the issue with this binding before retrying this operation./) end end + + context 'when the last operation state of the service instance is create failed' do + before do + service_instance.save_with_new_operation({}, { type: 'create', state: 'failed' }) + end + + it 'fails with a service binding error' do + expect { + app_apply_manifest.apply(app.guid, message) + }.to raise_error(CloudController::Errors::NotFound, + "Service instance '#{service_instance.name}' not found") + end + end + + context 'when the last operation state of the service instance is delete in progress' do + before do + service_instance.save_with_new_operation({}, { type: 'delete', state: 'in progress' }) + end + + it 'fails with a service binding error' do + expect { + app_apply_manifest.apply(app.guid, message) + }.to raise_error(AppApplyManifest::ServiceBindingError, + "For service '#{service_instance.name}': The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.") + end + end + + context 'when the last operation state of the service instance is delete failed' do + before do + service_instance.save_with_new_operation({}, { type: 'delete', state: 'failed' }) + end + + it 'fails with a service binding error' do + expect { + app_apply_manifest.apply(app.guid, message) + }.to raise_error(AppApplyManifest::ServiceBindingError, + "For service '#{service_instance.name}': The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.") + end + end end end end From 587a385f1d2c0d058216a96a173a329cf14d3386 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Thu, 7 Apr 2022 13:53:22 +0200 Subject: [PATCH 3/5] Outsource service instance checks into own method --- app/actions/app_apply_manifest.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 4d467056caf..4ea8e728dea 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -139,12 +139,7 @@ def validate_name_dns_compliant!(name) def create_service_bindings(manifest_service_bindings_message, app) manifest_service_bindings_message.manifest_service_bindings.each do |manifest_service_binding| service_instance = app.space.find_visible_service_instance_by_name(manifest_service_binding.name) - service_instance_not_found!(manifest_service_binding.name) unless service_instance - - if service_instance.type == 'managed_service_instance' - service_instance_not_found!(service_instance.name) if service_instance.create_failed? - delete_in_progress_or_failed!(service_instance) if service_instance.last_operation_is_delete? - end + validate_service_instance!(service_instance) binding = ServiceBinding.first(service_instance: service_instance, app: app) next if binding&.create_succeeded? @@ -197,6 +192,15 @@ def create_binding_message(service_instance_guid, app_guid, manifest_service_bin ) end + def validate_service_instance!(service_instance) + service_instance_not_found!(manifest_service_binding.name) unless service_instance + + if service_instance.type == 'managed_service_instance' + service_instance_not_found!(service_instance.name) if service_instance.create_failed? + delete_in_progress_or_failed!(service_instance) if service_instance.last_operation_is_delete? + end + end + def raise_binding_error!(service_instance, message) error_message = "For service '#{service_instance.name}': #{message}" raise ServiceBindingError.new(error_message) From 608af52cfd5ada45266f084f122dba061aa7af38 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Fri, 24 Jun 2022 09:46:17 +0200 Subject: [PATCH 4/5] Allow service binding creations for state `delete failed` - Allow service binding creations for service instances with state `delete failed` - Add more tests to cover all service instance states --- app/actions/app_apply_manifest.rb | 8 +- spec/unit/actions/app_apply_manifest_spec.rb | 93 +++++++++++++++++--- 2 files changed, 87 insertions(+), 14 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 4ea8e728dea..1f060d0db58 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -195,9 +195,9 @@ def create_binding_message(service_instance_guid, app_guid, manifest_service_bin def validate_service_instance!(service_instance) service_instance_not_found!(manifest_service_binding.name) unless service_instance - if service_instance.type == 'managed_service_instance' + if service_instance.managed_instance? service_instance_not_found!(service_instance.name) if service_instance.create_failed? - delete_in_progress_or_failed!(service_instance) if service_instance.last_operation_is_delete? + delete_in_progress!(service_instance) if service_instance.delete_in_progress? end end @@ -224,8 +224,8 @@ def service_instance_not_found!(name) raise CloudController::Errors::NotFound.new_from_details('ResourceNotFound', "Service instance '#{name}' not found") end - def delete_in_progress_or_failed!(service_instance) - error_message = 'The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.' + def delete_in_progress!(service_instance) + error_message = 'The service instance is getting deleted. Therefore, no binding can be created.' raise_binding_error!(service_instance, error_message) end diff --git a/spec/unit/actions/app_apply_manifest_spec.rb b/spec/unit/actions/app_apply_manifest_spec.rb index 453137e2904..4ea995c3a2a 100644 --- a/spec/unit/actions/app_apply_manifest_spec.rb +++ b/spec/unit/actions/app_apply_manifest_spec.rb @@ -931,8 +931,31 @@ module VCAP::CloudController /For service 'si-name': A binding failed to be deleted. Resolve the issue with this binding before retrying this operation./) end end + end + + context 'different service instance states' do + context 'when the last operation state is create succeeded' do + before do + service_instance.save_with_new_operation({}, { type: 'create', state: 'succeeded' }) + end + + it 'creates the binding' do + service_binding_1 = instance_double(ServiceBinding) + service_binding_2 = instance_double(ServiceBinding) + + allow(service_cred_binding_create).to receive(:precursor).and_return( + service_binding_1, + service_binding_2, + ) + + app_apply_manifest.apply(app.guid, message) + + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_1, parameters: nil) + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_2, parameters: { 'foo' => 'bar' }) + end + end - context 'when the last operation state of the service instance is create failed' do + context 'when the last operation state is create failed' do before do service_instance.save_with_new_operation({}, { type: 'create', state: 'failed' }) end @@ -941,11 +964,53 @@ module VCAP::CloudController expect { app_apply_manifest.apply(app.guid, message) }.to raise_error(CloudController::Errors::NotFound, - "Service instance '#{service_instance.name}' not found") + "Service instance '#{service_instance.name}' not found") end end - context 'when the last operation state of the service instance is delete in progress' do + context 'when the last operation state is update succeeded' do + before do + service_instance.save_with_new_operation({}, { type: 'update', state: 'succeeded' }) + end + + it 'creates the binding' do + service_binding_1 = instance_double(ServiceBinding) + service_binding_2 = instance_double(ServiceBinding) + + allow(service_cred_binding_create).to receive(:precursor).and_return( + service_binding_1, + service_binding_2, + ) + + app_apply_manifest.apply(app.guid, message) + + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_1, parameters: nil) + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_2, parameters: { 'foo' => 'bar' }) + end + end + + context 'when the last operation state is update failed' do + before do + service_instance.save_with_new_operation({}, { type: 'update', state: 'failed' }) + end + + it 'creates the binding' do + service_binding_1 = instance_double(ServiceBinding) + service_binding_2 = instance_double(ServiceBinding) + + allow(service_cred_binding_create).to receive(:precursor).and_return( + service_binding_1, + service_binding_2, + ) + + app_apply_manifest.apply(app.guid, message) + + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_1, parameters: nil) + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_2, parameters: { 'foo' => 'bar' }) + end + end + + context 'when the last operation state is delete in progress' do before do service_instance.save_with_new_operation({}, { type: 'delete', state: 'in progress' }) end @@ -954,20 +1019,28 @@ module VCAP::CloudController expect { app_apply_manifest.apply(app.guid, message) }.to raise_error(AppApplyManifest::ServiceBindingError, - "For service '#{service_instance.name}': The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.") + "For service '#{service_instance.name}': The service instance is getting deleted. Therefore, no binding can be created.") end end - context 'when the last operation state of the service instance is delete failed' do + context 'when the last operation state is delete failed' do before do service_instance.save_with_new_operation({}, { type: 'delete', state: 'failed' }) end - it 'fails with a service binding error' do - expect { - app_apply_manifest.apply(app.guid, message) - }.to raise_error(AppApplyManifest::ServiceBindingError, - "For service '#{service_instance.name}': The service instance is getting deleted or its deletion failed. Therefore, no binding can be created.") + it 'creates the binding' do + service_binding_1 = instance_double(ServiceBinding) + service_binding_2 = instance_double(ServiceBinding) + + allow(service_cred_binding_create).to receive(:precursor).and_return( + service_binding_1, + service_binding_2, + ) + + app_apply_manifest.apply(app.guid, message) + + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_1, parameters: nil) + expect(service_cred_binding_create).to have_received(:bind).with(service_binding_2, parameters: { 'foo' => 'bar' }) end end end From 0244841c90772a4923c4e3c1abe3ecf780ca3f56 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Mon, 4 Jul 2022 08:50:10 +0200 Subject: [PATCH 5/5] Small refactoring --- app/actions/app_apply_manifest.rb | 16 +------- .../service_credential_binding_app_create.rb | 5 +++ spec/unit/actions/app_apply_manifest_spec.rb | 39 +++++++++++++++++-- ...vice_credential_binding_app_create_spec.rb | 13 +++++++ 4 files changed, 55 insertions(+), 18 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 1f060d0db58..74b7c125bf7 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -139,7 +139,7 @@ def validate_name_dns_compliant!(name) def create_service_bindings(manifest_service_bindings_message, app) manifest_service_bindings_message.manifest_service_bindings.each do |manifest_service_binding| service_instance = app.space.find_visible_service_instance_by_name(manifest_service_binding.name) - validate_service_instance!(service_instance) + service_instance_not_found!(manifest_service_binding.name) unless service_instance binding = ServiceBinding.first(service_instance: service_instance, app: app) next if binding&.create_succeeded? @@ -192,15 +192,6 @@ def create_binding_message(service_instance_guid, app_guid, manifest_service_bin ) end - def validate_service_instance!(service_instance) - service_instance_not_found!(manifest_service_binding.name) unless service_instance - - if service_instance.managed_instance? - service_instance_not_found!(service_instance.name) if service_instance.create_failed? - delete_in_progress!(service_instance) if service_instance.delete_in_progress? - end - end - def raise_binding_error!(service_instance, message) error_message = "For service '#{service_instance.name}': #{message}" raise ServiceBindingError.new(error_message) @@ -224,11 +215,6 @@ def service_instance_not_found!(name) raise CloudController::Errors::NotFound.new_from_details('ResourceNotFound', "Service instance '#{name}' not found") end - def delete_in_progress!(service_instance) - error_message = 'The service instance is getting deleted. Therefore, no binding can be created.' - raise_binding_error!(service_instance, error_message) - end - def volume_services_enabled? VCAP::CloudController::Config.config.get(:volume_services_enabled) end diff --git a/app/actions/service_credential_binding_app_create.rb b/app/actions/service_credential_binding_app_create.rb index 70e9d3806b2..070caef53d6 100644 --- a/app/actions/service_credential_binding_app_create.rb +++ b/app/actions/service_credential_binding_app_create.rb @@ -56,6 +56,7 @@ def validate_service_instance!(app, service_instance, volume_mount_services_enab service_not_bindable! unless service_instance.service_plan.bindable? service_not_available! unless service_instance.service_plan.active? volume_mount_not_enabled! if service_instance.volume_service? && !volume_mount_services_enabled + service_instance_not_found! if service_instance.create_failed? operation_in_progress! if service_instance.operation_in_progress? end end @@ -84,6 +85,10 @@ def operation_in_progress! raise UnprocessableCreate.new('There is an operation in progress for the service instance') end + def service_instance_not_found! + raise UnprocessableCreate.new('Service instance not found') + end + def app_is_required! raise UnprocessableCreate.new('No app was specified') end diff --git a/spec/unit/actions/app_apply_manifest_spec.rb b/spec/unit/actions/app_apply_manifest_spec.rb index 4ea995c3a2a..4bca8d26222 100644 --- a/spec/unit/actions/app_apply_manifest_spec.rb +++ b/spec/unit/actions/app_apply_manifest_spec.rb @@ -934,6 +934,21 @@ module VCAP::CloudController end context 'different service instance states' do + context 'when the last operation state is create in progress' do + before do + service_instance.save_with_new_operation({}, { type: 'create', state: 'in progress' }) + allow(service_cred_binding_create).to receive(:precursor).and_raise(V3::ServiceCredentialBindingAppCreate::UnprocessableCreate, +'There is an operation in progress for the service instance') + end + + it 'fails with a service binding error' do + expect { + app_apply_manifest.apply(app.guid, message) + }.to raise_error(AppApplyManifest::ServiceBindingError, + "For service '#{service_instance.name}': There is an operation in progress for the service instance") + end + end + context 'when the last operation state is create succeeded' do before do service_instance.save_with_new_operation({}, { type: 'create', state: 'succeeded' }) @@ -958,13 +973,29 @@ module VCAP::CloudController context 'when the last operation state is create failed' do before do service_instance.save_with_new_operation({}, { type: 'create', state: 'failed' }) + allow(service_cred_binding_create).to receive(:precursor).and_raise(V3::ServiceCredentialBindingAppCreate::UnprocessableCreate, 'Service instance not found') end it 'fails with a service binding error' do expect { app_apply_manifest.apply(app.guid, message) - }.to raise_error(CloudController::Errors::NotFound, - "Service instance '#{service_instance.name}' not found") + }.to raise_error(AppApplyManifest::ServiceBindingError, + "For service '#{service_instance.name}': Service instance not found") + end + end + + context 'when the last operation state is update in progress' do + before do + service_instance.save_with_new_operation({}, { type: 'update', state: 'in progress' }) + allow(service_cred_binding_create).to receive(:precursor).and_raise(V3::ServiceCredentialBindingAppCreate::UnprocessableCreate, +'There is an operation in progress for the service instance') + end + + it 'fails with a service binding error' do + expect { + app_apply_manifest.apply(app.guid, message) + }.to raise_error(AppApplyManifest::ServiceBindingError, + "For service '#{service_instance.name}': There is an operation in progress for the service instance") end end @@ -1013,13 +1044,15 @@ module VCAP::CloudController context 'when the last operation state is delete in progress' do before do service_instance.save_with_new_operation({}, { type: 'delete', state: 'in progress' }) + allow(service_cred_binding_create).to receive(:precursor).and_raise(V3::ServiceCredentialBindingAppCreate::UnprocessableCreate, +'There is an operation in progress for the service instance') end it 'fails with a service binding error' do expect { app_apply_manifest.apply(app.guid, message) }.to raise_error(AppApplyManifest::ServiceBindingError, - "For service '#{service_instance.name}': The service instance is getting deleted. Therefore, no binding can be created.") + "For service '#{service_instance.name}': There is an operation in progress for the service instance") end end diff --git a/spec/unit/actions/service_credential_binding_app_create_spec.rb b/spec/unit/actions/service_credential_binding_app_create_spec.rb index e5116256a92..610a21bef2c 100644 --- a/spec/unit/actions/service_credential_binding_app_create_spec.rb +++ b/spec/unit/actions/service_credential_binding_app_create_spec.rb @@ -230,6 +230,19 @@ module V3 end end + context "when the service instance is in state 'create failed'" do + it 'raises an error' do + service_instance.save_with_new_operation({}, { type: 'create', state: 'failed' }) + + expect { + action.precursor(service_instance, app: app, volume_mount_services_enabled: false, message: message) + }.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + 'Service instance not found' + ) + end + end + context 'when the service is a volume service and service volume mounting is enabled' do let(:service_instance) { ManagedServiceInstance.make(:volume_mount, **si_details) }