Skip to content
This repository was archived by the owner on Jun 2, 2021. It is now read-only.

Commit bb73f26

Browse files
Derik Evangelistabutzopower
Derik Evangelista
and
butzopower
authored
v3(services): cannot delete shared SIs (cloudfoundry#1710)
or when there are route bindings present [#171726582](https://www.pivotaltracker.com/story/show/171726582) Co-authored-by: Brian Butz <butzopower@gmail.com>
1 parent 9732ed2 commit bb73f26

7 files changed

Lines changed: 231 additions & 120 deletions

File tree

app/actions/service_instance_delete_user_provided.rb renamed to app/actions/service_instance_delete.rb

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,22 @@
11
module VCAP::CloudController
2-
class ServiceInstanceDeleteUserProvided
2+
class ServiceInstanceDelete
33
class AssociationNotEmptyError < StandardError; end
4+
class InstanceSharedError < StandardError; end
5+
class NotImplementedError < StandardError; end
46

57
def initialize(event_repo)
68
@service_event_repository = event_repo
79
end
810

911
def delete(service_instance)
10-
association_not_empty! if service_instance.has_bindings? || service_instance.has_keys?
12+
association_not_empty! if service_instance.has_bindings? || service_instance.has_keys? || service_instance.has_routes?
13+
14+
cannot_delete_shared_instances! if service_instance.shared?
15+
16+
case service_instance
17+
when ManagedServiceInstance
18+
raise NotImplementedError
19+
end
1120

1221
service_instance.db.transaction do
1322
service_instance.lock!
@@ -19,7 +28,11 @@ def delete(service_instance)
1928
private
2029

2130
def association_not_empty!
22-
raise AssociationNotEmptyError.new
31+
raise AssociationNotEmptyError
32+
end
33+
34+
def cannot_delete_shared_instances!
35+
raise InstanceSharedError
2336
end
2437

2538
attr_reader :service_event_repository

‎app/controllers/v3/service_instances_controller.rb‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
require 'actions/service_instance_update_managed'
1616
require 'actions/service_instance_update_user_provided'
1717
require 'actions/service_instance_create_user_provided'
18-
require 'actions/service_instance_delete_user_provided'
18+
require 'actions/service_instance_delete'
1919
require 'actions/service_instance_create_managed'
2020
require 'fetchers/service_instance_list_fetcher'
2121
require 'decorators/field_service_instance_space_decorator'
@@ -109,16 +109,16 @@ def destroy
109109

110110
unauthorized! unless can_write_space?(service_instance.space)
111111

112-
case service_instance
113-
when UserProvidedServiceInstance
114-
service_event_repository = VCAP::CloudController::Repositories::ServiceEventRepository::WithUserActor.new(user_audit_info)
115-
ServiceInstanceDeleteUserProvided.new(service_event_repository).delete(service_instance)
116-
head :no_content
117-
else
118-
head :not_implemented
119-
end
120-
rescue ServiceInstanceDeleteUserProvided::AssociationNotEmptyError
112+
service_event_repository = VCAP::CloudController::Repositories::ServiceEventRepository::WithUserActor.new(user_audit_info)
113+
ServiceInstanceDelete.new(service_event_repository).delete(service_instance)
114+
115+
head :no_content
116+
rescue ServiceInstanceDelete::AssociationNotEmptyError
121117
associations_not_empty!
118+
rescue ServiceInstanceDelete::InstanceSharedError
119+
cannot_delete_shared_instances!(service_instance.name)
120+
rescue ServiceInstanceDelete::NotImplementedError
121+
head :not_implemented
122122
end
123123

124124
def share_service_instance
@@ -353,4 +353,8 @@ def associations_not_empty!
353353
new_from_details('AssociationNotEmpty', associations, :service_instances).
354354
with_response_code(422)
355355
end
356+
357+
def cannot_delete_shared_instances!(name)
358+
raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceDeletionSharesExists', name)
359+
end
356360
end

‎app/models/services/service_instance.rb‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,11 +210,15 @@ def shared?
210210
end
211211

212212
def has_bindings?
213-
!service_bindings.empty?
213+
service_bindings.present?
214214
end
215215

216216
def has_keys?
217-
!service_keys.empty?
217+
service_keys.present?
218+
end
219+
220+
def has_routes?
221+
routes.present?
218222
end
219223

220224
def self.managed_organizations_spaces_dataset(managed_organizations_dataset)

‎spec/request/service_instances_spec.rb‎

Lines changed: 69 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2146,17 +2146,12 @@ def check_filtered_instances(*instances)
21462146
describe 'DELETE /v3/service_instances/:guid' do
21472147
let(:api_call) { lambda { |user_headers| delete "/v3/service_instances/#{instance.guid}", '{}', user_headers } }
21482148

2149-
context 'user provided service instance' do
2149+
context 'permissions' do
21502150
let!(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(space: space) }
2151-
21522151
let(:db_check) {
2153-
lambda do
2154-
get "/v3/service_instances/#{instance.guid}", {}, admin_headers
2155-
expect(last_response.status).to eq(404)
2156-
2157-
expect(VCAP::CloudController::ServiceInstanceLabelModel.where(service_instance: instance).all).to be_empty
2158-
expect(VCAP::CloudController::ServiceInstanceAnnotationModel.where(service_instance: instance).all).to be_empty
2159-
end
2152+
lambda {
2153+
expect(VCAP::CloudController::ServiceInstance.all).to be_empty
2154+
}
21602155
}
21612156

21622157
let(:expected_codes_and_responses) do
@@ -2170,30 +2165,63 @@ def check_filtered_instances(*instances)
21702165
end
21712166

21722167
it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS
2168+
end
21732169

2174-
context 'when associations are not empty' do
2175-
it 'returns a 422 Unprocessable Entity when there are service bindings' do
2176-
VCAP::CloudController::ServiceBinding.make(service_instance: instance)
2170+
context 'user provided service instances' do
2171+
let!(:instance) { VCAP::CloudController::UserProvidedServiceInstance.make(space: space) }
21772172

2178-
api_call.call(admin_headers)
2179-
expect(last_response).to have_status_code(422)
2180-
response = parsed_response['errors'].first
2181-
expect(response).to include('title' => 'CF-AssociationNotEmpty')
2182-
expect(response).to include('detail' => 'Please delete the service_bindings, service_keys, and routes associations for your service_instances.')
2183-
end
2173+
before do
2174+
VCAP::CloudController::ServiceInstanceLabelModel.make(key_name: 'fruit', value: 'banana', service_instance: instance)
2175+
VCAP::CloudController::ServiceInstanceLabelModel.make(key_name: 'fruit', value: 'avocado', service_instance: instance)
2176+
VCAP::CloudController::ServiceInstanceAnnotationModel.make(key_name: 'contact', value: 'marie', service_instance: instance)
2177+
VCAP::CloudController::ServiceInstanceAnnotationModel.make(key_name: 'email', value: 'some@example.com', service_instance: instance)
2178+
end
21842179

2185-
it 'returns a 422 Unprocessable Entity when there are service keys' do
2186-
VCAP::CloudController::ServiceKey.make(service_instance: instance)
2180+
it 'deletes the instance and removes any labels or annotations' do
2181+
api_call.call(admin_headers)
2182+
expect(last_response).to have_status_code(204)
2183+
2184+
get "/v3/service_instances/#{instance.guid}", {}, admin_headers
2185+
expect(last_response.status).to eq(404)
2186+
expect(VCAP::CloudController::ServiceInstanceLabelModel.where(service_instance: instance).all).to be_empty
2187+
expect(VCAP::CloudController::ServiceInstanceAnnotationModel.where(service_instance: instance).all).to be_empty
2188+
end
2189+
end
2190+
2191+
context 'managed service instance' do
2192+
let!(:instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) }
21872193

2194+
context 'when it is shared' do
2195+
let(:other_space) { VCAP::CloudController::Space.make }
2196+
2197+
before do
2198+
share_service_instance(instance, other_space)
2199+
end
2200+
2201+
it 'returns a 422 Unprocessable Entity' do
21882202
api_call.call(admin_headers)
21892203
expect(last_response).to have_status_code(422)
21902204
response = parsed_response['errors'].first
2191-
expect(response).to include('title' => 'CF-AssociationNotEmpty')
2192-
expect(response).to include('detail' => 'Please delete the service_bindings, service_keys, and routes associations for your service_instances.')
2205+
expect(response).to include('title' => 'CF-ServiceInstanceDeletionSharesExists')
2206+
expect(response).to include('detail' => include('Service instances must be unshared before they can be deleted.'))
21932207
end
21942208
end
21952209
end
21962210

2211+
context 'when associations are not empty' do
2212+
let(:instance) { VCAP::CloudController::ServiceInstance.make(space: space) }
2213+
2214+
it 'returns a 422 Unprocessable Entity' do
2215+
VCAP::CloudController::ServiceBinding.make(service_instance: instance)
2216+
2217+
api_call.call(admin_headers)
2218+
expect(last_response).to have_status_code(422)
2219+
response = parsed_response['errors'].first
2220+
expect(response).to include('title' => 'CF-AssociationNotEmpty')
2221+
expect(response).to include('detail' => 'Please delete the service_bindings, service_keys, and routes associations for your service_instances.')
2222+
end
2223+
end
2224+
21972225
context 'when the service instance does not exist' do
21982226
let(:instance) { Struct.new(:guid).new('some-fake-guid') }
21992227
it 'returns a 404' do
@@ -2304,6 +2332,25 @@ def expect_metadata(instance, annotations: [], labels: [])
23042332
expect(l).to match_array(labels)
23052333
end
23062334

2335+
def share_service_instance(instance, target_space)
2336+
enable_sharing!
2337+
2338+
share_request = {
2339+
'data' => [
2340+
{ 'guid' => target_space.guid }
2341+
]
2342+
}
2343+
2344+
post "/v3/service_instances/#{instance.guid}/relationships/shared_spaces", share_request.to_json, admin_headers
2345+
expect(last_response.status).to eq(200)
2346+
end
2347+
2348+
def enable_sharing!
2349+
VCAP::CloudController::FeatureFlag.
2350+
find_or_create(name: 'service_instance_sharing') { |ff| ff.enabled = true }.
2351+
update(enabled: true)
2352+
end
2353+
23072354
describe 'unrefactored' do
23082355
let(:user_email) { 'user@email.example.com' }
23092356
let(:user_name) { 'username' }

‎spec/unit/actions/service_instance_delete_spec.rb‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
require 'spec_helper'
2+
require 'actions/service_instance_delete'
3+
4+
module VCAP::CloudController
5+
RSpec.describe ServiceInstanceDelete do
6+
describe '#delete' do
7+
subject(:action) { described_class.new(event_repository) }
8+
let(:event_repository) do
9+
dbl = double(Repositories::ServiceEventRepository::WithUserActor)
10+
allow(dbl).to receive(:record_user_provided_service_instance_event)
11+
dbl
12+
end
13+
14+
let!(:service_instance) do
15+
si = VCAP::CloudController::UserProvidedServiceInstance.make(
16+
name: 'foo',
17+
credentials: {
18+
foo: 'bar',
19+
baz: 'qux'
20+
},
21+
syslog_drain_url: 'https://foo.com',
22+
route_service_url: 'https://bar.com',
23+
tags: %w(accounting mongodb)
24+
)
25+
si.label_ids = [
26+
VCAP::CloudController::ServiceInstanceLabelModel.make(key_prefix: 'pre.fix', key_name: 'to_delete', value: 'value'),
27+
VCAP::CloudController::ServiceInstanceLabelModel.make(key_prefix: 'pre.fix', key_name: 'tail', value: 'fluffy')
28+
]
29+
si.annotation_ids = [
30+
VCAP::CloudController::ServiceInstanceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'to_delete', value: 'value').id,
31+
VCAP::CloudController::ServiceInstanceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'fox', value: 'bushy').id
32+
]
33+
si
34+
end
35+
36+
it 'deletes an user provided service instance from the database' do
37+
subject.delete(service_instance)
38+
39+
expect {
40+
service_instance.reload
41+
}.to raise_error(Sequel::Error, 'Record not found')
42+
expect(VCAP::CloudController::ServiceInstanceLabelModel.where(service_instance: service_instance)).to be_empty
43+
expect(VCAP::CloudController::ServiceInstanceAnnotationModel.where(service_instance: service_instance)).to be_empty
44+
end
45+
46+
it 'fails to delete managed service instances' do
47+
instance = VCAP::CloudController::ManagedServiceInstance.make
48+
expect { subject.delete(instance) }.to raise_error(ServiceInstanceDelete::NotImplementedError)
49+
expect { instance.reload }.not_to raise_error
50+
end
51+
52+
it 'creates an audit event' do
53+
subject.delete(service_instance)
54+
55+
expect(event_repository).
56+
to have_received(:record_user_provided_service_instance_event).
57+
with(:delete, instance_of(UserProvidedServiceInstance), {})
58+
end
59+
60+
describe 'invalid pre-conditions' do
61+
context 'when there are associated service bindings' do
62+
before do
63+
VCAP::CloudController::ServiceBinding.make(service_instance: service_instance)
64+
end
65+
66+
it 'does not delete the service instance' do
67+
expect { subject.delete(service_instance) }.to raise_error(ServiceInstanceDelete::AssociationNotEmptyError)
68+
expect { service_instance.reload }.not_to raise_error
69+
end
70+
end
71+
72+
context 'when there are associated service keys' do
73+
before do
74+
VCAP::CloudController::ServiceKey.make(service_instance: service_instance)
75+
end
76+
77+
it 'does not delete the service instance' do
78+
expect { subject.delete(service_instance) }.to raise_error(ServiceInstanceDelete::AssociationNotEmptyError)
79+
expect { service_instance.reload }.not_to raise_error
80+
end
81+
end
82+
83+
context 'when there are associated route bindings' do
84+
before do
85+
VCAP::CloudController::RouteBinding.make(
86+
service_instance: service_instance,
87+
route: VCAP::CloudController::Route.make(space: service_instance.space)
88+
)
89+
end
90+
91+
it 'does not delete the service instance' do
92+
expect { subject.delete(service_instance) }.to raise_error(ServiceInstanceDelete::AssociationNotEmptyError)
93+
expect { service_instance.reload }.not_to raise_error
94+
end
95+
end
96+
97+
context 'when the service instance is shared' do
98+
let(:space) { VCAP::CloudController::Space.make }
99+
let(:other_space) { VCAP::CloudController::Space.make }
100+
let!(:service_instance) {
101+
si = VCAP::CloudController::ServiceInstance.make(space: space)
102+
si.shared_space_ids = [other_space.id]
103+
si
104+
}
105+
106+
it 'does not delete the service instance' do
107+
expect { subject.delete(service_instance) }.to raise_error(ServiceInstanceDelete::InstanceSharedError)
108+
expect { service_instance.reload }.not_to raise_error
109+
end
110+
end
111+
end
112+
end
113+
end
114+
end

0 commit comments

Comments
 (0)