Skip to content

Commit

Permalink
Throw an error on sb bind when route binding status is delete_failed …
Browse files Browse the repository at this point in the history
…or delete_in_progess

This is a fix as part of #2637. It contains changes for:
POST /v3/service_route_bindings

Two behaviours change with this PR:
1: When a service route binding is in state delete_in_progess or delete_failed, we want an
error different from "Route is already bound to the service instance" to be thrown. It will now throw an error with "The binding is getting deleted or its deletion failed".
2: When a service route binding is in state create_failed, it should recreate the binding and not reject the request with "Route is already bound to the service instance".

With regard to point 1: it's important to note that the initial statement may not be entirely accurate,
as there's a possibility that orphan mitigation processes may have already removed the binding in question.
The Cloud Foundry Command Line Interface (CF CLI) correctly interprets the situation as a success.
This is because the message conveyed to the CF CLI indicates that the task it was instructed to perform
as already been completed. As a result, if the CF CLI is attempting to create a service route binding but
finds that a binding already exists in a state where it's either failed deletion or is pending deletion,
the CF CLI will consider this a failure.
  • Loading branch information
kathap committed Oct 10, 2023
1 parent ce3ee7b commit f4a26e6
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 12 deletions.
21 changes: 17 additions & 4 deletions app/actions/service_route_binding_create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ def initialize(user_audit_info, audit_hash)
PERMITTED_BINDING_ATTRIBUTES = [:route_service_url].freeze

def precursor(service_instance, route, message:)
validate!(service_instance, route)
validate_service_instance!(service_instance, route)
binding = RouteBinding.first(service_instance:, route:)
validate_binding!(binding)

binding_details = {
service_instance:,
Expand All @@ -24,6 +26,7 @@ def precursor(service_instance, route, message:)

RouteBinding.new.tap do |b|
RouteBinding.db.transaction do
binding.destroy if binding
b.save_with_attributes_and_new_operation(
binding_details,
CREATE_INITIAL_OPERATION
Expand All @@ -43,19 +46,25 @@ def event_repository
)
end

def validate!(service_instance, route)
def validate_service_instance!(service_instance, route)
not_supported! unless service_instance.route_service?
not_bindable! unless service_instance.bindable?
route_is_internal! if route.try(:internal?)
space_mismatch! unless route.space == service_instance.space
already_exists! if route.service_instance == service_instance
already_bound! if route.service_instance
already_bound! if route.service_instance && route.service_instance != service_instance
return unless service_instance.managed_instance?

service_instance_not_found! if service_instance.create_failed?
operation_in_progress! if service_instance.operation_in_progress?
end

def validate_binding!(binding)
return unless binding

already_exists! if binding.create_succeeded? || binding.create_in_progress?
incomplete_deletion! if binding.delete_failed? || binding.delete_in_progress?
end

def permitted_binding_attributes
PERMITTED_BINDING_ATTRIBUTES
end
Expand Down Expand Up @@ -87,6 +96,10 @@ def not_bindable!
def already_exists!
raise RouteBindingAlreadyExists.new('The route and service instance are already bound')
end

def incomplete_deletion!
raise UnprocessableCreate.new('The binding is getting deleted or its deletion failed')
end
end
end
end
84 changes: 76 additions & 8 deletions spec/unit/actions/service_route_binding_create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,83 @@ module V3
end

context 'route binding already exists' do
it 'raises an error' do
RouteBinding.make(service_instance:, route:)
let!(:binding) { RouteBinding.make(service_instance:, route:) }

context 'when no last service route binding operation exists' do
it 'raises an error' do
expect do
action.precursor(service_instance, route, message:)
end.to raise_error(
ServiceRouteBindingCreate::RouteBindingAlreadyExists,
'The route and service instance are already bound'
)
end
end

expect do
action.precursor(service_instance, route, message:)
end.to raise_error(
ServiceRouteBindingCreate::RouteBindingAlreadyExists,
'The route and service instance are already bound'
)
context "when the last service route binding operation is in 'create succeeded' state" do
before do
binding.save_with_attributes_and_new_operation({}, { type: 'create', state: 'succeeded' })
end

it 'raises an error' do
expect { action.precursor(service_instance, route, message:) }.to raise_error(
ServiceRouteBindingCreate::RouteBindingAlreadyExists,
'The route and service instance are already bound'
)
end
end

context "when the last service route binding operation is in 'create in progress' state" do
before do
binding.save_with_attributes_and_new_operation({}, { type: 'create', state: 'in progress' })
end

it 'raises an error' do
expect { action.precursor(service_instance, route, message:) }.to raise_error(
ServiceRouteBindingCreate::RouteBindingAlreadyExists,
'The route and service instance are already bound'
)
end
end

context "when the last service route binding operation is in 'create failed' state" do
before do
binding.save_with_attributes_and_new_operation({}, { type: 'create', state: 'failed' })
end

it 'deletes the existing binding and creates a new one' do
b = action.precursor(service_instance, route, message:)

expect(b.guid).not_to eq(binding.guid)
expect(b).to be_create_in_progress
expect { binding.reload }.to raise_error Sequel::NoExistingObject
end
end

context "when the last service route binding operation is in 'delete in progress' state" do
before do
binding.save_with_attributes_and_new_operation({}, { type: 'delete', state: 'in progress' })
end

it 'raises an error' do
expect { action.precursor(service_instance, route, message:) }.to raise_error(
ServiceRouteBindingCreate::UnprocessableCreate,
'The binding is getting deleted or its deletion failed'
)
end
end

context "when the last service route binding operation is in 'delete failed' state" do
before do
binding.save_with_attributes_and_new_operation({}, { type: 'delete', state: 'failed' })
end

it 'raises an error' do
expect { action.precursor(service_instance, route, message:) }.to raise_error(
ServiceRouteBindingCreate::UnprocessableCreate,
'The binding is getting deleted or its deletion failed'
)
end
end
end

Expand Down

0 comments on commit f4a26e6

Please sign in to comment.