Filter active service bindings per instance on dataset level (#4570)

- 'active' means latest in state 'create succeeded' (or without any
  operation)
- filtering is done in SQL, not Ruby
- no duplicate code where filtering is needed

Author: philippthun

app/models/runtime/helpers/service_operation_mixin.rb

@@ -1,5 +1,9 @@
 module VCAP::CloudController
   module ServiceOperationMixin
+    CREATE = 'create'.freeze
+    UPDATE = 'update'.freeze
+    DELETE = 'delete'.freeze
+
     INITIAL = 'initial'.freeze
     IN_PROGRESS = 'in progress'.freeze
     SUCCEEDED = 'succeeded'.freeze
@@ -56,15 +60,15 @@ def last_operation?
     end
 
     def create?
-      last_operation&.type == 'create'
+      last_operation&.type == CREATE
     end
 
     def update?
-      last_operation&.type == 'update'
+      last_operation&.type == UPDATE
     end
 
     def delete?
-      last_operation&.type == 'delete'
+      last_operation&.type == DELETE
     end
 
     def initial?
@@ -83,4 +87,18 @@ def failed?
       last_operation&.state == FAILED
     end
   end
+
+  module ServiceOperationDatasetMixin
+    def create_succeeded
+      last_operation_table = Sequel[last_operation_association]
+
+      no_operation = Sequel.expr(last_operation_table[:id] => nil)
+      create_and_succeeded = Sequel.expr(last_operation_table[:type] => ServiceOperationMixin::CREATE) &
+                             Sequel.expr(last_operation_table[:state] => ServiceOperationMixin::SUCCEEDED)
+
+      association_left_join(last_operation_association).
+        where(no_operation | create_and_succeeded).
+        select_all(first_source_alias)
+    end
+  end
 end

app/models/services/service_binding.rb

@@ -130,5 +130,26 @@ def save_with_new_operation(last_operation, attributes: {})
         service_binding_operation(reload: true)
       end
     end
+
+    dataset_module ServiceOperationDatasetMixin
+
+    dataset_module do
+      def last_operation_association
+        :service_binding_operation
+      end
+
+      # 'active' means latest in state 'create succeeded' (or without any operation)
+      def active_per_instance
+        create_succeeded.
+          from_self.
+          select_append do
+            row_number.function.over(
+              partition: :service_instance_guid,
+              order: [Sequel.desc(:created_at), Sequel.desc(:id)]
+            ).as(:_rn)
+          end.
+          from_self.where(_rn: 1)
+      end
+    end
   end
 end

app/presenters/system_environment/system_env_presenter.rb

@@ -3,9 +3,9 @@
 
 class SystemEnvPresenter
   def initialize(app_or_process)
+    @app_or_process = app_or_process
     @service_binding_k8s_enabled = app_or_process.service_binding_k8s_enabled
     @file_based_vcap_services_enabled = app_or_process.file_based_vcap_services_enabled
-    @service_bindings = app_or_process.service_bindings
   end
 
   def system_env
@@ -22,14 +22,8 @@ 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 = {}
-    latest_bindings.each do |service_binding|
+    @app_or_process.service_bindings_dataset.active_per_instance.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)

lib/cloud_controller/diego/service_binding_files_builder.rb

@@ -13,7 +13,6 @@ def initialize(app_or_process)
         @app_or_process = app_or_process
         @service_binding_k8s_enabled = app_or_process.service_binding_k8s_enabled
         @file_based_vcap_services = app_or_process.file_based_vcap_services_enabled
-        @service_bindings = app_or_process.service_bindings
       end
 
       def build
@@ -27,21 +26,14 @@ 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
 
-        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|
+        @app_or_process.service_bindings_dataset.active_per_instance.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)
@@ -58,8 +50,6 @@ 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
 
         service_binding_files.values

spec/unit/actions/service_credential_binding_app_create_spec.rb

@@ -43,6 +43,8 @@ module V3
 
       describe '#precursor' do
         RSpec.shared_examples 'the credential binding precursor' do
+          before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
+
           it 'returns a service credential binding precursor' do
             binding = action.precursor(service_instance, app:, message:)
 
@@ -69,9 +71,6 @@ module V3
           context 'when a binding already exists' do
             let!(:binding) { ServiceBinding.make(service_instance:, app:) }
 
-            # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
-            # before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
-
             context 'when no last binding operation exists' do
               it 'raises an error' do
                 expect { action.precursor(service_instance, app:, message:) }.to raise_error(
@@ -161,9 +160,6 @@ module V3
             end
             let(:service_instance2) { ManagedServiceInstance.make(**si_details) }
 
-            # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
-            # before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
-
             it 'raises an error when the binding name already exists' do
               # First request, should succeed
               expect do
@@ -188,9 +184,6 @@ module V3
               )
             end
 
-            # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
-            # before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
-
             it 'raises an error when the app is already bound to the service instance' do
               # First request, should succeed
               expect do
@@ -209,8 +202,6 @@ module V3
             let(:binding_1) { ServiceBinding.make(service_instance: service_instance, app: app, name: nil) }
 
             before do
-              # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
-              # TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1)
               binding_1.save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
             end
 
@@ -231,9 +222,6 @@ module V3
           context 'concurrent credential binding creation' do
             let(:name) { nil }
 
-            # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
-            # before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
-
             it 'allows only one binding when two creates run in parallel' do
               # This test simulates a race condition for concurrent binding creation using a spy on `service_instance`.
               # We mock that a second binding is created after the first one acquires a lock and expect an `UnprocessableCreate` error.
@@ -251,11 +239,13 @@ module V3
           end
 
           context 'when multiple bindings are allowed' do
+            let(:binding_1) { ServiceBinding.make(service_instance:, app:, name:) }
+
             before do
               # TODO: Remove skip when the service bindings unique constraints are removed
               skip 'this test can be enabled when the service bindings unique constraints are removed and max_bindings_per_app_service_instance can be configured'
 
-              binding_1 = ServiceBinding.make(service_instance:, app:, name:)
+              TestConfig.override(max_service_credential_bindings_per_app_service_instance: 3)
               binding_2 = ServiceBinding.make(service_instance:, app:, name:)
               binding_1.save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
               binding_2.save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
@@ -276,8 +266,6 @@ module V3
             end
 
             it 'raises an error if one of the bindings is in a failed state' do
-              TestConfig.override(max_service_credential_bindings_per_app_service_instance: 2)
-
               ServiceBinding.make(service_instance:, app:, name:).save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
               ServiceBinding.make(service_instance:, app:, name:).save_with_attributes_and_new_operation({}, { type: 'delete', state: 'failed' })
 
@@ -304,8 +292,6 @@ module V3
             end
 
             it 'raises an error if an existing binding has a different name' do
-              TestConfig.override(max_service_credential_bindings_per_app_service_instance: 4)
-
               ServiceBinding.make(service_instance: service_instance, app: app, name: 'other-name').
                 save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
 

spec/unit/lib/cloud_controller/diego/service_binding_files_builder_spec.rb

@@ -39,9 +39,19 @@ module VCAP::CloudController::Diego
       end
 
       it 'uses the most recent binding' do
-        expect(service_binding_files.find { |f| f.path == "#{directory}/binding-guid" }).to have_attributes(content: newer_binding.guid)
+        expect(service_binding_files.find { |f| f.path == "#{directory}/binding_guid" }).to have_attributes(content: newer_binding.guid)
         expect(service_binding_files.find { |f| f.path == "#{directory}/name" }).to have_attributes(content: name || 'binding-name')
-        expect(service_binding_files.find { |f| f.path == "#{directory}/binding-name" }).to have_attributes(content: 'binding-name') if name.nil?
+        expect(service_binding_files.find { |f| f.path == "#{directory}/binding_name" }).to have_attributes(content: 'binding-name') if name.nil?
+      end
+
+      context 'when the bindings have the same created_at timestamp' do
+        let(:newer_binding_created_at) { binding_created_at }
+
+        it 'uses the most recent binding determined by highest id' do
+          expect(service_binding_files.find { |f| f.path == "#{directory}/binding_guid" }).to have_attributes(content: newer_binding.guid)
+          expect(service_binding_files.find { |f| f.path == "#{directory}/name" }).to have_attributes(content: name || 'binding-name')
+          expect(service_binding_files.find { |f| f.path == "#{directory}/binding_name" }).to have_attributes(content: 'binding-name') if name.nil?
+        end
       end
     end
   end