Move service binding uniquness validation into ccng

app/actions/service_credential_binding_app_create.rb

@@ -19,37 +19,47 @@ def initialize(user_audit_info, audit_hash, manifest_triggered: false)
 
       def precursor(service_instance, message:, app: nil, volume_mount_services_enabled: false)
         validate_service_instance!(app, service_instance, volume_mount_services_enabled)
-        binding = ServiceBinding.first(service_instance:, app:)
-        validate_binding!(binding)
 
-        binding_details = {
+        new_binding_details = {
           service_instance: service_instance,
           name: message.name,
           app: app,
           type: 'app',
           credentials: {}
         }
 
-        ServiceBinding.new.tap do |b|
+        ServiceBinding.new.tap do |new_binding|
           ServiceBinding.db.transaction do
-            if binding
-              binding.destroy
-              VCAP::Services::ServiceBrokers::V2::OrphanMitigator.new.cleanup_failed_bind(binding)
+            # 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)
+
+            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)
+                next
+              end
+
+              validate_binding!(binding, desired_binding_name: message.name)
+              num_valid_bindings += 1
             end
-            b.save_with_attributes_and_new_operation(
-              binding_details,
+
+            validate_number_of_bindings!(num_valid_bindings)
+
+            new_binding.save_with_attributes_and_new_operation(
+              new_binding_details,
               CREATE_INITIAL_OPERATION
             )
-            MetadataUpdate.update(b, message)
+            MetadataUpdate.update(new_binding, message)
           end
         end
       rescue Sequel::ValidationFailed => e
-        if e.message.include?('app_guid and name unique')
-          raise UnprocessableCreate.new("The binding name is invalid. App binding names must be unique. The app already has a binding with name '#{message.name}'.")
-        elsif e.message.include?('service_instance_guid and app_guid unique')
-          raise UnprocessableCreate.new('The app is already bound to the service instance.')
-        end
-
         raise UnprocessableCreate.new(e.full_message)
       end
 
@@ -66,11 +76,25 @@ def validate_service_instance!(app, service_instance, volume_mount_services_enab
         operation_in_progress! if service_instance.operation_in_progress?
       end
 
-      def validate_binding!(binding)
-        return unless binding
+      def validate_binding!(binding, desired_binding_name:)
+        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
+
+      def validate_number_of_bindings!(number_of_bindings)
+        too_many_bindings! if number_of_bindings >= max_bindings_per_app_service_instance
+      end
+
+      def validate_app_guid_name_uniqueness!(target_app_guid, desired_binding_name, target_service_instance_guid)
+        return if desired_binding_name.nil?
 
-        already_bound! if binding.create_succeeded? || binding.create_in_progress?
-        incomplete_deletion! if binding.delete_failed? || binding.delete_in_progress?
+        dataset = ServiceBinding.where(app_guid: target_app_guid, name: desired_binding_name)
+
+        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
       end
 
       def permitted_binding_attributes
@@ -87,6 +111,15 @@ def event_repository
         )
       end
 
+      def max_bindings_per_app_service_instance
+        1
+        # NOTE: This is hard-coded to 1 for now to preserve the old uniqueness behavior.
+        # TODO: Once the DB migration that drops the unique constraints for service bindings has been released,
+        #       this should be switched to read from config:
+        #       VCAP::CloudController::Config.config.get(:max_service_credential_bindings_per_app_service_instance)
+        # TODO: Also remove skips in related specs.
+      end
+
       def app_is_required!
         raise UnprocessableCreate.new('No app was specified')
       end
@@ -95,6 +128,27 @@ 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
+
+      def too_many_bindings!
+        raise UnprocessableCreate.new(
+          "The app has too many bindings to this service instance (limit: #{max_bindings_per_app_service_instance}). Consider deleting existing/orphaned bindings."
+        )
+      end
+
+      def name_cannot_be_changed!
+        raise UnprocessableCreate.new('The binding name cannot be changed for the same app and service instance')
+      end
+
+      def name_uniqueness_violation!(name)
+        msg = 'The binding name is invalid. Binding names must be unique for a given service instance and app.'
+        msg += " The app already has a binding with name '#{name}'." unless name.nil? || name.empty?
+
+        raise UnprocessableCreate.new(msg)
+      end
+
       def already_bound!
         raise UnprocessableCreate.new('The app is already bound to the service instance')
       end

app/actions/v2/services/service_binding_create.rb

@@ -29,6 +29,7 @@ def create(app, service_instance, message, volume_mount_services_enabled, accept
       raise SpaceMismatch unless bindable_in_space?(service_instance, app.space)
 
       raise_if_instance_locked(service_instance)
+      raise_if_binding_already_exists(service_instance, app)
 
       binding = ServiceBinding.new(
         service_instance: service_instance,
@@ -82,6 +83,12 @@ def bindable_in_space?(service_instance, app_space)
       service_instance.space == app_space || service_instance.shared_spaces.include?(app_space)
     end
 
+    def raise_if_binding_already_exists(service_instance, app)
+      return unless ServiceBinding.where(service_instance:, app:).any?
+
+      raise CloudController::Errors::ApiError.new_from_details('ServiceBindingAppServiceTaken', 'The app is already bound to the service.')
+    end
+
     def logger
       @logger ||= Steno.logger('cc.action.service_binding_create')
     end

app/controllers/services/service_instances_controller.rb

@@ -415,7 +415,7 @@ def initialize(service_instance)
       end
 
       def serialize(_controller, space, _opts, _orphans=nil)
-        bound_app_count = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(@service_instance.guid, space.guid).count
+        bound_app_count = ServiceBindingListFetcher.fetch_service_instance_bindings_in_space(@service_instance.guid, space.guid).select(:app_guid).distinct.count
         CloudController::Presenters::V2::ServiceInstanceSharedToPresenter.new.to_hash(space, bound_app_count)
       end
     end

app/models/services/service_binding.rb

@@ -35,28 +35,11 @@ class ServiceBinding < Sequel::Model
 
     delegate :service, :service_plan, to: :service_instance
 
-    def around_save
-      yield
-    rescue Sequel::UniqueConstraintViolation => e
-      if e.message.include?('unique_service_binding_app_guid_name')
-        errors.add(%i[app_guid name], :unique)
-        raise validation_failed_error
-      elsif e.message.include?('unique_service_binding_service_instance_guid_app_guid')
-        errors.add(%i[service_instance_guid app_guid], :unique)
-        raise validation_failed_error
-      else
-        raise e
-      end
-    end
-
     def validate
       validates_presence :app
       validates_presence :service_instance
       validates_presence :type
 
-      validates_unique %i[app_guid service_instance_guid], message: Sequel.lit('The app is already bound to the service.')
-      validates_unique %i[app_guid name], message: Sequel.lit("The binding name is invalid. App binding names must be unique. The app already has a binding with name '#{name}'.")
-
       validate_space_match
       validate_cannot_change_binding
 

app/models/services/service_instance.rb

@@ -148,7 +148,7 @@ def as_summary_json
       {
         'guid' => guid,
         'name' => name,
-        'bound_app_count' => service_bindings_dataset.count,
+        'bound_app_count' => service_bindings_dataset.select(:app_guid).distinct.count,
         'type' => type
       }
     end

app/presenters/system_environment/system_env_presenter.rb

@@ -22,12 +22,19 @@ def vcap_services
   private
 
   def service_binding_env_variables
+    latest_bindings = @service_bindings.
+                      select(&:create_succeeded?).
+                      group_by(&:service_instance_guid).
+                      values.
+                      map { |list| list.max_by(&:created_at) }
+
     services_hash = {}
-    @service_bindings.select(&:create_succeeded?).each do |service_binding|
+    latest_bindings.each do |service_binding|
       service_name = service_binding_label(service_binding)
       services_hash[service_name.to_sym] ||= []
       services_hash[service_name.to_sym] << service_binding_env_values(service_binding)
     end
+
     services_hash
   end
 

lib/cloud_controller/diego/service_binding_files_builder.rb

@@ -27,14 +27,21 @@ def build
 
       private
 
+      # rubocop:disable Metrics/CyclomaticComplexity
       def build_service_binding_k8s
         return nil unless @service_binding_k8s_enabled
 
         service_binding_files = {}
         names = Set.new # to check for duplicate binding names
         total_bytesize = 0 # to check the total bytesize
 
-        @service_bindings.select(&:create_succeeded?).each do |service_binding|
+        latest_bindings = @service_bindings.
+                          select(&:create_succeeded?).
+                          group_by(&:service_instance_guid).
+                          values.
+                          map { |list| list.max_by(&:created_at) }
+
+        latest_bindings.each do |service_binding|
           sb_hash = ServiceBindingPresenter.new(service_binding, include_instance: true).to_hash
           name = sb_hash[:name]
           raise IncompatibleBindings.new("Invalid binding name: '#{name}'. Name must match #{binding_naming_convention.inspect}") unless valid_name?(name)
@@ -52,6 +59,7 @@ def build_service_binding_k8s
           total_bytesize += add_file(service_binding_files, name, 'type', label)
           total_bytesize += add_file(service_binding_files, name, 'provider', label)
         end
+        # rubocop:enable Metrics/CyclomaticComplexity
 
         raise IncompatibleBindings.new("Bindings exceed the maximum allowed bytesize of #{MAX_ALLOWED_BYTESIZE}: #{total_bytesize}") if total_bytesize > MAX_ALLOWED_BYTESIZE
 

spec/request/service_credential_bindings_spec.rb

@@ -1274,18 +1274,22 @@ def check_filtered_bindings(*bindings)
                                                                }))
         end
 
-        it 'returns 422 when the binding already exists' do
-          api_call.call admin_headers
-          expect(last_response.status).to eq(201).or eq(202)
+        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)
 
-          api_call.call admin_headers
+              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
-                                                               }))
+              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
         end
 
         context 'when the service instance does not exist' do