From 189f4b6df96b09c7bad87d583f47cd9a3afc809f Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Thu, 18 Sep 2025 00:20:35 +0200 Subject: [PATCH] Filter active service bindings per instance on dataset level - '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 --- .../helpers/service_operation_mixin.rb | 24 ++++++++++++++++--- app/models/services/service_binding.rb | 21 ++++++++++++++++ .../system_env_presenter.rb | 10 ++------ .../diego/service_binding_files_builder.rb | 12 +--------- ...vice_credential_binding_app_create_spec.rb | 24 ++++--------------- .../service_binding_files_builder_spec.rb | 14 +++++++++-- 6 files changed, 62 insertions(+), 43 deletions(-) diff --git a/app/models/runtime/helpers/service_operation_mixin.rb b/app/models/runtime/helpers/service_operation_mixin.rb index 8d49e0cf5e1..2cdb0f5f7bc 100644 --- a/app/models/runtime/helpers/service_operation_mixin.rb +++ b/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 diff --git a/app/models/services/service_binding.rb b/app/models/services/service_binding.rb index 1124ceac94b..46e7ea33dec 100644 --- a/app/models/services/service_binding.rb +++ b/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 diff --git a/app/presenters/system_environment/system_env_presenter.rb b/app/presenters/system_environment/system_env_presenter.rb index 749f4071129..67f01bd6d65 100644 --- a/app/presenters/system_environment/system_env_presenter.rb +++ b/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) diff --git a/lib/cloud_controller/diego/service_binding_files_builder.rb b/lib/cloud_controller/diego/service_binding_files_builder.rb index 6bdd43e24cd..2c03762f75b 100644 --- a/lib/cloud_controller/diego/service_binding_files_builder.rb +++ b/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,7 +26,6 @@ def build private - # rubocop:disable Metrics/CyclomaticComplexity def build_service_binding_k8s return nil unless @service_binding_k8s_enabled @@ -35,13 +33,7 @@ def build_service_binding_k8s 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 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 df4311b1c3f..ada86711e0c 100644 --- a/spec/unit/actions/service_credential_binding_app_create_spec.rb +++ b/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' }) diff --git a/spec/unit/lib/cloud_controller/diego/service_binding_files_builder_spec.rb b/spec/unit/lib/cloud_controller/diego/service_binding_files_builder_spec.rb index 3ce51f5acee..196fe570567 100644 --- a/spec/unit/lib/cloud_controller/diego/service_binding_files_builder_spec.rb +++ b/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