Lock app when creating service bindings (#4577)

A "FOR UPDATE" lock on the app instance together with checks implemented
in Ruby ensure the same behavior as two unique constraints on database
level, which need to be removed to support multiple bindings between an
app and a service instance.

The two checks are:
- the number of bindings from an app to a service instance is below the
  configured limit
- no other binding (to another service instance) for this app has the
  same name

The validate_app_guid_name_uniqueness! method has been moved after the
validation of already existing bindings - this simplifies its
implementation.

Method incomplete_deletion! raises a different (more precise) error when
multiple bindings are active.

Some unused methods have been removed.

A test in spec/request/service_credential_bindings_spec.rb was not
executed ('it' inside 'before').

Tests in spec/unit/actions/service_credential_binding_app_create_spec.rb
have been reworked for consistence. Some unneeded (redundant) tests have
been removed.

Author: philippthun

app/actions/service_credential_binding_app_create.rb

@@ -30,16 +30,14 @@ def precursor(service_instance, message:, app: nil, volume_mount_services_enable
 
         ServiceBinding.new.tap do |new_binding|
           ServiceBinding.db.transaction do
-            # Lock the service instance to prevent multiple bindings being created when not allowed
-            service_instance.lock! if max_bindings_per_app_service_instance == 1
-
-            validate_app_guid_name_uniqueness!(app.guid, message.name, service_instance.guid)
+            # Lock the app to ensure no other bindings are created concurrently and rules can be enforced
+            # - number of bindings per service instance is below limit
+            # - no other binding (to other service instance) for this app has the same name
+            app.lock!
 
             num_valid_bindings = 0
             existing_bindings = ServiceBinding.where(service_instance:, app:)
             existing_bindings.each do |binding|
-              binding.lock!
-
               if binding.create_failed?
                 binding.destroy
                 VCAP::Services::ServiceBrokers::V2::OrphanMitigator.new.cleanup_failed_bind(binding)
@@ -51,6 +49,7 @@ def precursor(service_instance, message:, app: nil, volume_mount_services_enable
             end
 
             validate_number_of_bindings!(num_valid_bindings)
+            validate_app_guid_name_uniqueness!(app.guid, message.name, service_instance.guid)
 
             new_binding.save_with_attributes_and_new_operation(
               new_binding_details,
@@ -77,10 +76,9 @@ def validate_service_instance!(app, service_instance, volume_mount_services_enab
       end
 
       def validate_binding!(binding, desired_binding_name:)
-        already_bound! if (max_bindings_per_app_service_instance == 1) && (binding.create_succeeded? || binding.create_in_progress?)
+        already_bound! if max_bindings_per_app_service_instance == 1 && (binding.create_succeeded? || binding.create_in_progress?)
         binding_in_progress!(binding.guid) if binding.create_in_progress?
         incomplete_deletion! if binding.delete_in_progress? || binding.delete_failed?
-
         name_cannot_be_changed! if binding.name != desired_binding_name
       end
 
@@ -91,10 +89,9 @@ def validate_number_of_bindings!(number_of_bindings)
       def validate_app_guid_name_uniqueness!(target_app_guid, desired_binding_name, target_service_instance_guid)
         return if desired_binding_name.nil?
 
-        dataset = ServiceBinding.where(app_guid: target_app_guid, name: desired_binding_name)
+        return unless ServiceBinding.where(app_guid: target_app_guid, name: desired_binding_name).exclude(service_instance_guid: target_service_instance_guid).any?
 
-        name_uniqueness_violation!(desired_binding_name) if max_bindings_per_app_service_instance == 1 && dataset.first
-        name_uniqueness_violation!(desired_binding_name) if dataset.exclude(service_instance_guid: target_service_instance_guid).first
+        name_uniqueness_violation!(desired_binding_name)
       end
 
       def permitted_binding_attributes
@@ -124,10 +121,6 @@ def app_is_required!
         raise UnprocessableCreate.new('No app was specified')
       end
 
-      def not_supported!
-        raise Unimplemented.new('Cannot create credential bindings for managed service instances')
-      end
-
       def binding_in_progress!(binding_guid)
         raise UnprocessableCreate.new("There is already a binding in progress for this service instance and app (binding guid: #{binding_guid})")
       end
@@ -154,7 +147,9 @@ def already_bound!
       end
 
       def incomplete_deletion!
-        raise UnprocessableCreate.new('The binding is getting deleted or its deletion failed')
+        raise UnprocessableCreate.new('The binding is getting deleted or its deletion failed') if max_bindings_per_app_service_instance == 1
+
+        raise UnprocessableCreate.new('A binding for this service instance and app is getting deleted or its deletion failed')
       end
 
       def space_mismatch!
@@ -165,10 +160,6 @@ def service_not_bindable!
         raise UnprocessableCreate.new('Service plan does not allow bindings')
       end
 
-      def service_not_available!
-        raise UnprocessableCreate.new('Service plan is not available')
-      end
-
       def volume_mount_not_enabled!
         raise UnprocessableCreate.new('Support for volume mount services is disabled')
       end

app/actions/service_credential_binding_key_create.rb

@@ -92,10 +92,6 @@ def service_not_bindable!
         raise UnprocessableCreate.new('Service plan does not allow bindings.')
       end
 
-      def service_not_available!
-        raise UnprocessableCreate.new('Service plan is not available.')
-      end
-
       def volume_mount_not_enabled!
         raise UnprocessableCreate.new('Support for volume mount services is disabled.')
       end

spec/request/service_credential_bindings_spec.rb

@@ -1275,20 +1275,20 @@ def check_filtered_bindings(*bindings)
         end
 
         context 'when only one binding per app and service instance is allowed' do
-          before do
-            it 'returns 422 when the binding already exists' do
-              api_call.call admin_headers
-              expect(last_response.status).to eq(201).or eq(202)
+          before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
 
-              api_call.call admin_headers
+          it 'returns 422 when the binding already exists' do
+            api_call.call admin_headers
+            expect(last_response.status).to eq(201).or eq(202)
 
-              expect(last_response).to have_status_code(422)
-              expect(parsed_response['errors']).to include(include({
-                                                                     'detail' => include('The app is already bound to the service instance'),
-                                                                     'title' => 'CF-UnprocessableEntity',
-                                                                     'code' => 10_008
-                                                                   }))
-            end
+            api_call.call admin_headers
+
+            expect(last_response).to have_status_code(422)
+            expect(parsed_response['errors']).to include(include({
+                                                                   'detail' => include('The app is already bound to the service instance'),
+                                                                   'title' => 'CF-UnprocessableEntity',
+                                                                   'code' => 10_008
+                                                                 }))
           end
         end