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 8 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,9 @@ 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) unless service_plan_active?(service_plan)
service_plan_not_visible_in_space!(service_plan, space) 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 +396,26 @@ 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) unless service_plan_active?(service_plan)
service_plan_not_visible_in_space!(service_plan, space) unless service_plan_exists_in_space?(service_plan, service_instance.space)
kathap marked this conversation as resolved.
Show resolved Hide resolved
invalid_service_plan_relation! unless service_plan.service == service_instance.service
end

Expand All @@ -424,6 +435,18 @@ 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)
unprocessable!("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.')
kathap marked this conversation as resolved.
Show resolved Hide resolved
end

def service_plan_not_visible_in_space!(service_plan, space)
unprocessable!('Invalid service plan. This could be due to a space-scoped broker which is offering the service plan ' \
"'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \
kathap marked this conversation as resolved.
Show resolved Hide resolved
'is not enabled in this organization. Ensure that the service plan 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
13 changes: 10 additions & 3 deletions spec/request/service_instances_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1260,7 +1260,10 @@ 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. This could be due to a space-scoped broker which is offering the service plan ' \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have the same tests for the update case (PATCH). If we had those, above bug which results in a 500 would have been detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will add it

"'#{service_plan.name}' with guid '#{service_plan.guid} in another space or that the plan " \
'is not enabled in this organization. Ensure that the service plan is visible in your current space ' \
"'#{space.name}' with guid '#{space.guid}'.",
'title' => 'CF-UnprocessableEntity',
'code' => 10_008
})
Expand All @@ -1276,7 +1279,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 +2418,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
Loading