Move service binding uniquness validation into ccng (#4521)

* Move service binding uniqueness validation into ccng 

Allows operators to set the number of allowed service bindings via parameter `max_service_credential_bindings_per_app_service_instance`. (currently hard coded to 1)
This will be set to 1 in capi-release to not change the current behavior.
If set to >1 developers can create multiple bindings for the same app and service instance.
This is useful e.g. for rotating the binding credentials without pushing the app again. Restart/restage is sufficient to put the latest binding into the app.


To ensure users cannot run create multiple service bindings during the
bosh deployment (when old and new code is running) we need to first
introduce the new validation logic so that we can safely remove the
unique constraints in the next capi release.

Further details can be found in RFC-0040:
https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0040-service-binding-rotation.md

* Allow only one binding for v2 API

* fix test issues

* Remove db migration and config parameter
e.

* skip tests which require unique constraint removal

* remove config parameter

* Lock service instance to prevent concurrent binding creation

Author: johha

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_binding_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