Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error message and support logs for /v3/service_offerings and plans #4213

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions app/controllers/v3/service_instances_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,10 @@ def create_user_provided(message)

def create_managed(message, space:)
service_plan = ServicePlan.first(guid: message.service_plan_guid)
unprocessable_service_plan! unless service_plan_valid?(service_plan, space)
unprocessable_service_plan! unless service_plan_valid?(service_plan)
unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan)
service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \
kathap marked this conversation as resolved.
Show resolved Hide resolved
unless service_plan_exists_in_space?(service_plan, space)

action = V3::ServiceInstanceCreateManaged.new(user_audit_info, message.audit_hash)
VCAP::CloudController::ManagedServiceInstance.db.transaction do
Expand Down Expand Up @@ -394,17 +397,27 @@ def admin?
permission_queryer.can_write_globally?
end

def service_plan_valid?(service_plan, space)
def service_plan_valid?(service_plan)
service_plan &&
visible_to_current_user?(plan: service_plan) &&
service_plan.visible_in_space?(space)
visible_to_current_user?(plan: service_plan)
end

def service_plan_active?(service_plan)
service_plan.active?
end

def service_plan_exists_in_space?(service_plan, space)
service_plan.visible_in_space?(space)
end

def raise_if_invalid_service_plan!(service_instance, message)
return unless message.service_plan_guid

service_plan = ServicePlan.first(guid: message.service_plan_guid)
unprocessable_service_plan! unless service_plan_valid?(service_plan, service_instance.space)
unprocessable_service_plan! unless service_plan_valid?(service_plan)
unavailable_service_plan!(service_plan.name, service_plan.guid) unless service_plan_active?(service_plan)
service_plan_not_visible_in_space!(service_plan.name, service_plan.guid, space.name, space.guid) \
unless service_plan_exists_in_space?(service_plan, service_instance.space)
invalid_service_plan_relation! unless service_plan.service == service_instance.service
end

Expand All @@ -424,6 +437,16 @@ def unprocessable_service_plan!
unprocessable!('Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.')
end

def unavailable_service_plan!(service_plan, service_plan_guid)
unprocessable!("Invalid service plan. The service plan '#{service_plan}' with guid '#{service_plan_guid}' has been removed from the service broker's catalog." \
'It is not possible to create new service instances using this plan.')
kathap marked this conversation as resolved.
Show resolved Hide resolved
end

def service_plan_not_visible_in_space!(service_plan, service_plan_guid, space_name, space_guid)
unprocessable!("Invalid service plan. Ensure that the service plan '#{service_plan}' with guid '#{service_plan_guid}' \
svkrieger marked this conversation as resolved.
Show resolved Hide resolved
is visible in your current space '#{space_name}' with guid '#{space_guid}'.")
end

def invalid_service_plan_relation!
raise CloudController::Errors::ApiError.new_from_details('InvalidRelation', 'service plan relates to a different service offering')
end
Expand Down
11 changes: 8 additions & 3 deletions spec/request/service_instances_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1260,7 +1260,8 @@ def check_filtered_instances(*instances)
expect(last_response).to have_status_code(422)
expect(parsed_response['errors']).to include(
include({
'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.',
'detail' => "Invalid service plan. Ensure that the service plan '#{service_plan.name}' with \
guid '#{service_plan_guid}' is visible in your current space '#{space.name}' with guid '#{space.guid}'.",
'title' => 'CF-UnprocessableEntity',
'code' => 10_008
})
Expand All @@ -1276,7 +1277,9 @@ def check_filtered_instances(*instances)
expect(last_response).to have_status_code(422)
expect(parsed_response['errors']).to include(
include({
'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.',
'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \
"has been removed from the service broker's catalog." \
'It is not possible to create new service instances using this plan.',
'title' => 'CF-UnprocessableEntity',
'code' => 10_008
})
Expand Down Expand Up @@ -2413,7 +2416,9 @@ def check_filtered_instances(*instances)
expect(last_response).to have_status_code(422)
expect(parsed_response['errors']).to include(
include({
'detail' => 'Invalid service plan. Ensure that the service plan exists, is available, and you have access to it.',
'detail' => "Invalid service plan. The service plan '#{service_plan.name}' with guid '#{service_plan.guid}' " \
"has been removed from the service broker's catalog." \
'It is not possible to create new service instances using this plan.',
'title' => 'CF-UnprocessableEntity',
'code' => 10_008
})
Expand Down