v3(services): unbind service credential binding of type app (cloudfoundry#1942)

* v3(services): Add NoAdditionalKey validation for get list for route bindings

* WIP: Explicitly show the difference between MSI and UPSI in controller

The "require_async" flag introduced decoupling from the concepts of MSI and USPI, however we can be more intentional in using the domain and use the SI types to distinguish between the behaviours

* Remove unused method

* Extraxted delete route binding job

* Extracted delete binding action so that it can be extended by Route and Credential binding

TODO:
- add audit logging for service credential binding delete
- correct behaviuor on "concurrency error"

* Added audit events for service binding unbind

Made sure to catch concurrency errors for service bindings

Author: FelisiaM

app/actions/service_credential_binding_delete.rb

@@ -1,18 +1,27 @@
+require 'actions/v3/service_binding_delete'
+
 module VCAP::CloudController
   module V3
-    class ServiceCredentialBindingDelete
-      class NotImplementedError < StandardError
+    class ServiceCredentialBindingDelete < V3::ServiceBindingDelete
+      def initialize(user_audit_info)
+        super()
+        @user_audit_info = user_audit_info
       end
 
-      def delete(binding)
-        not_implemented! if binding.service_instance.managed_instance?
+      private
+
+      def perform_delete_actions(binding)
         binding.destroy
+
+        event_repository.record_delete(binding, @user_audit_info)
       end
 
-      private
+      def perform_start_delete_actions(binding)
+        event_repository.record_start_delete(binding, @user_audit_info)
+      end
 
-      def not_implemented!
-        raise NotImplementedError.new
+      def event_repository
+        Repositories::ServiceBindingEventRepository
       end
     end
   end

app/actions/service_route_binding_delete.rb

@@ -1,84 +1,18 @@
+require 'services/service_brokers/service_client_provider'
+require 'actions/v3/service_binding_delete'
+
 module VCAP::CloudController
   module V3
-    class ServiceRouteBindingDelete
-      class UnprocessableDelete < StandardError; end
-
-      class ConcurrencyError < StandardError; end
-
-      RequiresAsync = Class.new.freeze
-
-      DeleteStatus = Struct.new(:finished, :operation).freeze
-      DeleteStarted = ->(operation) { DeleteStatus.new(false, operation) }
-      DeleteComplete = DeleteStatus.new(true, nil).freeze
-
-      PollingStatus = Struct.new(:finished, :retry_after).freeze
-      PollingFinished = PollingStatus.new(true, nil).freeze
-      ContinuePolling = ->(retry_after) { PollingStatus.new(false, retry_after) }
-
+    class ServiceRouteBindingDelete < V3::ServiceBindingDelete
       def initialize(service_event_repository)
+        super()
         @service_event_repository = service_event_repository
       end
 
-      def delete(binding, async_allowed:)
-        operation_in_progress! if binding.service_instance.operation_in_progress?
-        return RequiresAsync.new unless async_allowed || binding.service_instance.user_provided_instance?
-
-        result = send_unbind_to_broker(binding)
-        if result[:finished]
-          perform_delete_actions(binding)
-        else
-          update_last_operation(binding, operation: result[:operation])
-        end
-
-        return result
-      rescue => e
-        unless e.is_a? ConcurrencyError
-          update_last_operation(binding, state: 'failed', description: e.message)
-        end
-
-        raise e
-      end
-
-      def poll(binding)
-        client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
-        details = client.fetch_and_handle_service_binding_last_operation(binding)
-        case details[:last_operation][:state]
-        when 'in progress'
-          update_last_operation(
-            binding,
-            description: details[:last_operation][:description],
-            operation: binding.last_operation.broker_provided_operation)
-          return ContinuePolling.call(details[:retry_after])
-        when 'succeeded'
-          perform_delete_actions(binding)
-          return PollingFinished
-        when 'failed'
-          update_last_operation(binding, state: 'failed', description: details[:last_operation][:description])
-          raise LastOperationFailedState
-        end
-      rescue LastOperationFailedState => e
-        raise e
-      rescue => e
-        update_last_operation(binding, state: 'failed', description: e.message)
-        raise e
-      end
-
       private
 
       attr_reader :service_event_repository
 
-      def send_unbind_to_broker(binding)
-        client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
-        details = client.unbind(binding, nil, true)
-        details[:async] ? DeleteStarted.call(details[:operation]) : DeleteComplete
-      rescue VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError
-        raise ConcurrencyError.new(
-          'The service broker rejected the request due to an operation being in progress for the service route binding'
-        )
-      rescue => err
-        raise UnprocessableDelete.new("Service broker failed to delete service binding for instance #{binding.service_instance.name}: #{err.message}")
-      end
-
       def perform_delete_actions(binding)
         record_audit_event(binding)
         binding.destroy
@@ -92,19 +26,6 @@ def record_audit_event(binding)
           { route_guid: binding.route.guid },
         )
       end
-
-      def update_last_operation(binding, description: nil, state: 'in progress', operation: nil)
-        binding.save_with_new_operation({}, {
-          type: 'delete',
-          state: state,
-          description: description,
-          broker_provided_operation: operation || binding.last_operation.broker_provided_operation
-        })
-      end
-
-      def operation_in_progress!
-        raise UnprocessableDelete.new('There is an operation in progress for the service instance')
-      end
     end
   end
 end

app/actions/v3/service_binding_create.rb

@@ -94,22 +94,6 @@ def bindings_retrievable?(binding)
       def not_retrievable!
         raise BindingNotRetrievable.new('The broker responded asynchronously but does not support fetching binding data')
       end
-
-      def fetch_last_operation(client, binding)
-        client.fetch_service_binding_last_operation(binding)
-      rescue VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerBadResponse,
-             VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerRequestRejected,
-             HttpRequestError => e
-        binding.save_with_attributes_and_new_operation(
-          {},
-          {
-          type: 'create',
-          state: 'in progress',
-          description: e.message,
-        })
-
-        return nil
-      end
     end
   end
 end

app/actions/v3/service_binding_delete.rb

@@ -0,0 +1,103 @@
+require 'services/service_brokers/service_client_provider'
+
+module VCAP::CloudController
+  module V3
+    class ServiceBindingDelete
+      class UnprocessableDelete < StandardError; end
+
+      class ConcurrencyError < StandardError; end
+      class OperationCancelled < StandardError; end
+
+      DeleteStatus = Struct.new(:finished, :operation).freeze
+      DeleteStarted = ->(operation) { DeleteStatus.new(false, operation) }
+      DeleteComplete = DeleteStatus.new(true, nil).freeze
+
+      PollingStatus = Struct.new(:finished, :retry_after).freeze
+      PollingFinished = PollingStatus.new(true, nil).freeze
+      ContinuePolling = ->(retry_after) { PollingStatus.new(false, retry_after) }
+
+      def delete(binding)
+        result = send_unbind_to_client(binding)
+        if result[:finished]
+          perform_delete_actions(binding)
+        else
+          perform_start_delete_actions(binding)
+          update_last_operation(binding, operation: result[:operation])
+        end
+
+        return result
+      rescue => e
+        unless e.is_a? ConcurrencyError
+          update_last_operation(binding, state: 'failed', description: e.message)
+        end
+
+        raise e
+      end
+
+      def poll(binding)
+        client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
+        details = client.fetch_and_handle_service_binding_last_operation(binding)
+
+        case details[:last_operation][:state]
+        when 'in progress'
+          update_last_operation(
+            binding,
+            description: details[:last_operation][:description],
+            operation: binding.last_operation.broker_provided_operation)
+          return ContinuePolling.call(details[:retry_after])
+        when 'succeeded'
+          perform_delete_actions(binding)
+          return PollingFinished
+        when 'failed'
+          update_last_operation(binding, state: 'failed', description: details[:last_operation][:description])
+          raise LastOperationFailedState
+        end
+      rescue LastOperationFailedState => e
+        raise e
+      rescue => e
+        update_last_operation(binding, state: 'failed', description: e.message)
+        raise e
+      end
+
+      class BindingNotRetrievable < StandardError; end
+
+      private
+
+      def perform_start_delete_actions(binding); end
+
+      def send_unbind_to_client(binding)
+        client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
+        details = client.unbind(binding, nil, true)
+        details[:async] ? DeleteStarted.call(details[:operation]) : DeleteComplete
+      rescue VCAP::Services::ServiceBrokers::V2::Errors::ConcurrencyError
+        raise ConcurrencyError.new(
+          'The service broker rejected the request due to an operation being in progress for the service route binding'
+        )
+      rescue CloudController::Errors::ApiError => err
+        if err.name == 'AsyncServiceBindingOperationInProgress'
+          raise ConcurrencyError.new(
+            'The service broker rejected the request due to an operation being in progress for the service route binding'
+          )
+        end
+        raise unprocessable!(binding, err)
+      rescue => err
+        raise unprocessable!(binding, err)
+      end
+
+      def unprocessable!(binding, err)
+        UnprocessableDelete.new("Service broker failed to delete service binding for instance #{binding.service_instance.name}: #{err.message}")
+      end
+
+      def update_last_operation(binding, description: nil, state: 'in progress', operation: nil)
+        binding.save_with_attributes_and_new_operation(
+          {},
+          {
+          type: 'delete',
+          state: state,
+          description: description,
+          broker_provided_operation: operation || binding.last_operation&.broker_provided_operation
+        })
+      end
+    end
+  end
+end

app/controllers/v3/service_credential_bindings_controller.rb

@@ -10,6 +10,7 @@
 require 'decorators/include_binding_app_decorator'
 require 'decorators/include_binding_service_instance_decorator'
 require 'jobs/v3/create_service_credential_binding_job_actor'
+require 'jobs/v3/delete_binding_job'
 
 class ServiceCredentialBindingsController < ApplicationController
   def index
@@ -68,10 +69,22 @@ def destroy
     not_found! unless service_credential_binding.present?
     unauthorized! unless can_write_to_space?(binding_space)
 
-    V3::ServiceCredentialBindingDelete.new.delete(service_credential_binding)
-    head :no_content
-  rescue V3::ServiceCredentialBindingDelete::NotImplementedError
-    head :not_implemented
+    if service_credential_binding.is_a?(ServiceKey)
+      head :not_implemented
+      return
+    end
+
+    operation_in_progress! if service_credential_binding.service_instance.operation_in_progress?
+
+    case service_credential_binding.service_instance
+    when ManagedServiceInstance
+      pollable_job_guid = enqueue_unbind_job(service_credential_binding.guid)
+      head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{pollable_job_guid}")
+    when UserProvidedServiceInstance
+      action = V3::ServiceCredentialBindingDelete.new(user_audit_info)
+      action.delete(service_credential_binding)
+      head :no_content
+    end
   end
 
   def details
@@ -121,6 +134,16 @@ def enqueue_bind_job(binding_guid, message)
     pollable_job.guid
   end
 
+  def enqueue_unbind_job(binding_guid)
+    bind_job = VCAP::CloudController::V3::DeleteBindingJob.new(
+      :credential,
+      binding_guid,
+      user_audit_info: user_audit_info,
+    )
+    pollable_job = Jobs::Enqueuer.new(bind_job, queue: Jobs::Queues.generic).enqueue_pollable
+    pollable_job.guid
+  end
+
   def resource_not_accessible!(resource, guid)
     unprocessable!("The #{resource} could not be found: '#{guid}'")
   end
@@ -195,10 +218,6 @@ def ensure_service_credential_binding_is_accessible!
     not_found! unless service_credential_binding_exists?
   end
 
-  def not_found!
-    resource_not_found!(:service_credential_binding)
-  end
-
   def service_credential_binding_exists?
     !!service_credential_binding
   end
@@ -230,4 +249,12 @@ def fetcher
   def query_params
     request.query_parameters.with_indifferent_access
   end
+
+  def operation_in_progress!
+    unprocessable!('There is an operation in progress for the service instance.')
+  end
+
+  def not_found!
+    resource_not_found!(:service_credential_binding)
+  end
 end

app/controllers/v3/service_route_bindings_controller.rb

@@ -4,7 +4,7 @@
 require 'actions/service_route_binding_create'
 require 'actions/service_route_binding_delete'
 require 'jobs/v3/create_binding_async_job'
-require 'jobs/v3/delete_route_binding_job'
+require 'jobs/v3/delete_binding_job'
 require 'presenters/v3/paginated_list_presenter'
 require 'presenters/v3/service_route_binding_presenter'
 require 'fetchers/route_binding_list_fetcher'
@@ -64,14 +64,15 @@ def create
 
   def destroy
     route_binding_not_found! unless @route_binding && can_read_space?(@route_binding.route.space)
+    operation_in_progress! if @route_binding.service_instance.operation_in_progress?
 
-    action = V3::ServiceRouteBindingDelete.new(service_event_repository)
-    result = action.delete(@route_binding, async_allowed: false)
-
-    if result.is_a? V3::ServiceRouteBindingDelete::RequiresAsync
+    case @route_binding.service_instance
+    when ManagedServiceInstance
       pollable_job_guid = enqueue_unbind_job(@route_binding.guid)
       head :accepted, 'Location' => url_builder.build_url(path: "/v3/jobs/#{pollable_job_guid}")
-    else
+    when UserProvidedServiceInstance
+      action = V3::ServiceRouteBindingDelete.new(service_event_repository)
+      action.delete(@route_binding)
       head :no_content
     end
   rescue V3::ServiceRouteBindingDelete::UnprocessableDelete => e
@@ -144,7 +145,8 @@ def enqueue_bind_job(binding_guid, parameters)
   end
 
   def enqueue_unbind_job(binding_guid)
-    bind_job = VCAP::CloudController::V3::DeleteRouteBindingJob.new(
+    bind_job = VCAP::CloudController::V3::DeleteBindingJob.new(
+      :route,
       binding_guid,
       user_audit_info: user_audit_info,
     )
@@ -237,6 +239,10 @@ def already_exists!
     raise CloudController::Errors::ApiError.new_from_details('ServiceInstanceAlreadyBoundToSameRoute').with_response_code(422)
   end
 
+  def operation_in_progress!
+    unprocessable!('There is an operation in progress for the service instance.')
+  end
+
   def set_route_binding
     @route_binding = RouteBinding.first(guid: hashed_params[:guid])
   end