Better error message and support logs for /v3/service_offerings and p…

…lans (#4213) * Better error message and support logs for /v3/service_offerings and plans
cloudfoundry · Feb 17, 2025 · eed3d7b · eed3d7b
1 parent cfd3d29
commit eed3d7b
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 8 deletions.
diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb
@@ -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
@@ -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, service_instance.space) unless service_plan_exists_in_space?(service_plan, service_instance.space)
     invalid_service_plan_relation! unless service_plan.service == service_instance.service
   end
 
@@ -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.')
+  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 " \
+                   '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

diff --git a/spec/request/service_instances_spec.rb b/spec/request/service_instances_spec.rb
@@ -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 ' \
+                                    "'#{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
                       })
@@ -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
                       })
@@ -2404,6 +2409,26 @@ def check_filtered_instances(*instances)
           end
         end
 
+        context 'not enabled in that org' do
+          let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: false, active: true) }
+          let(:service_plan_guid) { service_plan.guid }
+
+          it 'fails saying the plan is invalid' do
+            api_call.call(admin_headers)
+            expect(last_response).to have_status_code(422)
+            expect(parsed_response['errors']).to include(
+              include({
+                        'detail' => '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 " \
+                                    '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
+                      })
+            )
+          end
+        end
+
         context 'not available' do
           let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: true, active: false) }
           let(:service_plan_guid) { service_plan.guid }
@@ -2413,7 +2438,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
                       })