diff --git a/app/actions/service_credential_binding_app_create.rb b/app/actions/service_credential_binding_app_create.rb index 5ac22684606..d876b0a8fc9 100644 --- a/app/actions/service_credential_binding_app_create.rb +++ b/app/actions/service_credential_binding_app_create.rb @@ -19,10 +19,8 @@ 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, @@ -30,26 +28,38 @@ def precursor(service_instance, message:, app: nil, volume_mount_services_enable 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 diff --git a/app/actions/v2/services/service_binding_create.rb b/app/actions/v2/services/service_binding_create.rb index d2871b58276..cdb06c2807d 100644 --- a/app/actions/v2/services/service_binding_create.rb +++ b/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 diff --git a/app/controllers/services/service_instances_controller.rb b/app/controllers/services/service_instances_controller.rb index 283bf771543..e2da22ec1a0 100644 --- a/app/controllers/services/service_instances_controller.rb +++ b/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 diff --git a/app/models/services/service_binding.rb b/app/models/services/service_binding.rb index e0d7f38fe32..1124ceac94b 100644 --- a/app/models/services/service_binding.rb +++ b/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 diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index c3444708161..6bb76d633e2 100644 --- a/app/models/services/service_instance.rb +++ b/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 diff --git a/app/presenters/system_environment/system_env_presenter.rb b/app/presenters/system_environment/system_env_presenter.rb index b3f27e9fd66..749f4071129 100644 --- a/app/presenters/system_environment/system_env_presenter.rb +++ b/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 diff --git a/lib/cloud_controller/diego/service_binding_files_builder.rb b/lib/cloud_controller/diego/service_binding_files_builder.rb index 20ba186f4ef..fb25a604cd7 100644 --- a/lib/cloud_controller/diego/service_binding_files_builder.rb +++ b/lib/cloud_controller/diego/service_binding_files_builder.rb @@ -27,6 +27,7 @@ def build private + # rubocop:disable Metrics/CyclomaticComplexity def build_service_binding_k8s return nil unless @service_binding_k8s_enabled @@ -34,7 +35,13 @@ def build_service_binding_k8s 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 diff --git a/spec/request/service_credential_bindings_spec.rb b/spec/request/service_credential_bindings_spec.rb index ae5431b7dce..c96d0e5cf36 100644 --- a/spec/request/service_credential_bindings_spec.rb +++ b/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 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 4baf85ba3dc..4d8def3b848 100644 --- a/spec/unit/actions/service_credential_binding_app_create_spec.rb +++ b/spec/unit/actions/service_credential_binding_app_create_spec.rb @@ -69,6 +69,9 @@ 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( @@ -158,22 +161,20 @@ 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 action.precursor(service_instance, app:, message:) end.not_to raise_error - # Mock the validation for the second request to simulate the race condition and trigger a - # unique constraint violation on app_guid + name - allow_any_instance_of(ServiceBinding).to receive(:validate).and_return(true) - allow(ServiceBinding).to receive(:first).with(service_instance: service_instance2, app: app).and_return(nil) - # Second request, should fail with correct error expect do action.precursor(service_instance2, app:, message:) end.to raise_error(ServiceBindingCreate::UnprocessableCreate, - "The binding name is invalid. App binding names must be unique. The app already has a binding with name 'foo'.") + "The binding name is invalid. Binding names must be unique for a given service instance and app. The app already has a binding with name 'foo'.") end end @@ -187,21 +188,34 @@ 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 action.precursor(service_instance, app:, message:) end.not_to raise_error - # Mock the validation for the second request to simulate the race condition and trigger a - # unique constraint violation on service_instance_guid + app_guid - allow_any_instance_of(ServiceBinding).to receive(:validate).and_return(true) - allow(ServiceBinding).to receive(:first).with(service_instance:, app:).and_return(nil) - # Second request, should fail with correct error expect do action.precursor(service_instance, app: app, message: message2) - end.to raise_error(ServiceBindingCreate::UnprocessableCreate, 'The app is already bound to the service instance.') + end.to raise_error(ServiceBindingCreate::UnprocessableCreate, 'The app is already bound to the service instance') + end + end + + context 'app guid name uniqueness validation without name' do + let(:name) { nil } + 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 + + it 'does not raise an error' do + expect { action.precursor(other_service_instance, app:, message:) }.not_to raise_error end end @@ -213,6 +227,106 @@ module V3 'The service instance and the app are in different spaces' ) end + + 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) } + + def attempt_precursor + action.precursor(service_instance, app:, message:) + :ok + rescue StandardError => e + e + end + + it 'allows only one binding when two creates run in parallel' do + results = [Thread.new { attempt_precursor }, Thread.new { attempt_precursor }].map(&:value) + + expect(ServiceBinding.where(app:, service_instance:).count).to eq(1) + expect(results.count(:ok)).to eq(1) + expect(results.count { |r| r.is_a?(VCAP::CloudController::V3::ServiceBindingCreate::UnprocessableCreate) }).to eq(1) + expect(results.grep(Exception).map(&:message)).to include('The app is already bound to the service instance') + end + end + + context 'when multiple bindings are allowed' do + 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:) + 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' }) + end + + it 'creates multiple bindings for the same app and service instance' do + expect { action.precursor(service_instance, app:, message:) }.not_to raise_error + expect(ServiceBinding.where(app:, service_instance:).count).to eq(3) + end + + it 'raises an error if there are too many bindings' do + TestConfig.override(max_service_credential_bindings_per_app_service_instance: 2) + + expect { action.precursor(service_instance, app:, message:) }.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + 'The app has too many bindings to this service instance (limit: 2). Consider deleting existing/orphaned bindings.' + ) + 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' }) + + expect do + action.precursor(service_instance, app:, message:) + end.to raise_error(ServiceCredentialBindingAppCreate::UnprocessableCreate, 'The binding is getting deleted or its deletion failed') + end + + it 'raises an error if the binding name changes' do + new_message = VCAP::CloudController::ServiceCredentialAppBindingCreateMessage.new({ name: 'new_name' }) + expect { action.precursor(service_instance, app: app, message: new_message) }.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + 'The binding name cannot be changed for the same app and service instance' + ) + end + + it 'raises an error if a previous binding is in progress' do + binding_1.save_with_attributes_and_new_operation({}, { type: 'create', state: 'in progress' }) + + expect { action.precursor(service_instance, app:, message:) }.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + "There is already a binding in progress for this service instance and app (binding guid: #{binding_1.guid})" + ) + 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' }) + + expect do + action.precursor(service_instance, app:, message:) # uses 'foo' + end.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + 'The binding name cannot be changed for the same app and service instance' + ) + end + + it 'raises an error if a binding for the same app but a different service instance exists with the same name' do + expect do + action.precursor(other_service_instance, app:, message:) + end.to raise_error( + ServiceCredentialBindingAppCreate::UnprocessableCreate, + "The binding name is invalid. Binding names must be unique for a given service instance and app. The app already has a binding with name 'foo'." + ) + end + end end context 'user-provided service instance' do @@ -224,7 +338,9 @@ module V3 syslog_drain_url: 'https://drain.syslog.example.com/runlolarun' } end + let(:service_instance) { UserProvidedServiceInstance.make(**si_details) } + let(:other_service_instance) { UserProvidedServiceInstance.make(**si_details, name: 'other_instance_name') } it_behaves_like 'the credential binding precursor' end @@ -237,6 +353,7 @@ module V3 end let(:service_instance) { ManagedServiceInstance.make(**si_details) } + let(:other_service_instance) { ManagedServiceInstance.make(**si_details, name: 'other_service_instance') } context 'validations' do context 'when plan is not bindable' do @@ -321,6 +438,8 @@ module V3 end end + let(:other_service_instance) { ManagedServiceInstance.make(space: space, name: 'other_service_instance') } + it_behaves_like 'the credential binding precursor' end end diff --git a/spec/unit/controllers/runtime/apps_controller_spec.rb b/spec/unit/controllers/runtime/apps_controller_spec.rb index 18ee3d9d58c..46f3bb1200e 100644 --- a/spec/unit/controllers/runtime/apps_controller_spec.rb +++ b/spec/unit/controllers/runtime/apps_controller_spec.rb @@ -2496,18 +2496,6 @@ def delete_app ServiceBinding.make(service_instance: si2, app: process2.app, name: 'free') end - it 'binding si2 to process1 with a name in use by process1 is not ok' do - expect do - ServiceBinding.make(service_instance: si2, app: process1.app, name: 'out') - end.to raise_error(Sequel::ValidationFailed, /App binding names must be unique\./) - end - - it 'binding si1 to process1 with a new name is not ok' do - expect do - ServiceBinding.make(service_instance: si1, app: process1.app, name: 'gravy') - end.to raise_error(Sequel::ValidationFailed, 'The app is already bound to the service.') - end - it 'binding si2 to process1 with a name in use by process2 is ok' do ServiceBinding.make(service_instance: si2, app: process1.app, name: 'free') get "/v2/apps/#{process1.app.guid}/service_bindings?results-per-page=2&page=1&q=name:free" 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 fe716bd526f..edbbe652e39 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 @@ -16,6 +16,34 @@ module VCAP::CloudController::Diego 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 + + context 'when there are multiple bindings with the same name for the same app and service instance' do + 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' + end + + let(:newer_binding_created_at) { Time.now.utc - 2.minutes } + + let!(:newer_binding) do + # Create binding eagerly to ensure it exists when ServiceBindingFilesBuilder queries app.service_bindings + VCAP::CloudController::ServiceBinding.make( + name: binding_name, + app: app, + credentials: credentials, + service_instance: instance, + syslog_drain_url: syslog_drain_url, + volume_mounts: volume_mounts, + created_at: newer_binding_created_at + ) + 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}/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 RSpec.shared_examples 'mapping of instance metadata' do |instance_name| @@ -67,10 +95,13 @@ module VCAP::CloudController::Diego end RSpec.describe ServiceBindingFilesBuilder do + let(:space) { VCAP::CloudController::Space.make } + let(:app) { VCAP::CloudController::AppModel.make(space:) } let(:service) { VCAP::CloudController::Service.make(label: 'service-name', tags: %w[a-service-tag another-service-tag]) } let(:plan) { VCAP::CloudController::ServicePlan.make(name: 'plan-name', service: service) } - let(:instance) { VCAP::CloudController::ManagedServiceInstance.make(name: 'instance-name', tags: %w[an-instance-tag another-instance-tag], service_plan: plan) } + let(:instance) { VCAP::CloudController::ManagedServiceInstance.make(name: 'instance-name', space: space, tags: %w[an-instance-tag another-instance-tag], service_plan: plan) } let(:binding_name) { 'binding-name' } + let(:binding_created_at) { Time.now.utc - 3.minutes } let(:credentials) do { string: 'a string', @@ -84,20 +115,25 @@ module VCAP::CloudController::Diego end let(:syslog_drain_url) { nil } let(:volume_mounts) { nil } - let(:binding) do + let!(:binding) do + # Create binding eagerly to ensure it exists when ServiceBindingFilesBuilder queries app.service_bindings VCAP::CloudController::ServiceBinding.make( name: binding_name, + app: app, credentials: credentials, service_instance: instance, syslog_drain_url: syslog_drain_url, - volume_mounts: volume_mounts + volume_mounts: volume_mounts, + created_at: binding_created_at ) end - let(:app) { binding.app } + let(:directory) { 'binding-name' } describe '#build' do - subject(:build) { ServiceBindingFilesBuilder.build(app) } + subject(:build) do + ServiceBindingFilesBuilder.build(app) + end context 'when service-binding-k8s feature is enabled' do before do @@ -197,7 +233,7 @@ module VCAP::CloudController::Diego end context 'when the instance is user-provided' do - let(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(name: 'upsi', tags: %w[an-upsi-tag another-upsi-tag]) } + let(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(name: 'upsi', space: space, tags: %w[an-upsi-tag another-upsi-tag]) } include_examples 'mapping of type and provider', 'user-provided' include_examples 'mapping of binding metadata' diff --git a/spec/unit/models/services/service_binding_spec.rb b/spec/unit/models/services/service_binding_spec.rb index 52959e54fbf..fde838b2224 100644 --- a/spec/unit/models/services/service_binding_spec.rb +++ b/spec/unit/models/services/service_binding_spec.rb @@ -25,7 +25,6 @@ module VCAP::CloudController it { is_expected.to validate_presence :app } it { is_expected.to validate_presence :service_instance } it { is_expected.to validate_db_presence :credentials } - it { is_expected.to validate_uniqueness %i[app_guid service_instance_guid], message: 'The app is already bound to the service.' } it { is_expected.to validate_presence [:type] } it 'validates max length of name' do @@ -80,19 +79,6 @@ module VCAP::CloudController let(:app) { AppModel.make } let(:service_instance) { ServiceInstance.make(space: app.space) } - context 'and the name is not null' do - let(:existing_binding) do - ServiceBinding.make(app: app, name: 'some-name', service_instance: service_instance, type: 'app') - end - - it 'adds a uniqueness error' do - other_service_instance = ServiceInstance.make(space: existing_binding.space) - conflict = ServiceBinding.new(app: existing_binding.app, name: existing_binding.name, service_instance: other_service_instance, type: 'app') - expect(conflict.valid?).to be(false) - expect(conflict.errors.full_messages).to eq(['The binding name is invalid. App binding names must be unique. The app already has a binding with name \'some-name\'.']) - end - end - context 'and the name is null' do let(:existing_binding) do ServiceBinding.make(app: app, name: nil, service_instance: service_instance, type: 'app') diff --git a/spec/unit/presenters/system_environment/system_env_presenter_spec.rb b/spec/unit/presenters/system_environment/system_env_presenter_spec.rb index 7b734293a8e..dfd53ca417f 100644 --- a/spec/unit/presenters/system_environment/system_env_presenter_spec.rb +++ b/spec/unit/presenters/system_environment/system_env_presenter_spec.rb @@ -130,6 +130,21 @@ module VCAP::CloudController end end + context 'when there are multiple service bindings for the same service instance' do + 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' + end + + it 'includes only the latest binding' do + newer_binding = ServiceBinding.make(app: app, service_instance: service_instance, syslog_drain_url: 'logs.go-here.com', created_at: Time.now.utc + 10.seconds) + + bindings = system_env_presenter.system_env[:VCAP_SERVICES][service.label.to_sym] + expect(bindings).to have(1).items + expect(bindings.first.to_hash[:binding_guid]).to eq(newer_binding.guid) + end + end + describe 'volume mounts' do context 'when the service binding has volume mounts' do let!(:service_binding) do