Route policies follow ups

app/actions/route_policy_create.rb

@@ -1,29 +1,44 @@
+require 'repositories/route_policy_event_repository'
+
 module VCAP::CloudController
   class RoutePolicyCreate
     class Error < StandardError
     end
 
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
     def create(route:, message:)
       validate_scope!(route.domain, message.source)
 
-      RoutePolicy.db.transaction do
+      route_policy = RoutePolicy.db.transaction do
         # Lock the parent route row to serialize concurrent creates.
         # SELECT ... FOR UPDATE on an empty policies table acquires no row locks,
         # so two concurrent transactions can both read [] and both pass cf:any
         # exclusivity validation. Locking the route row (which always exists)
         # ensures they serialize regardless of how many policies currently exist.
         Route.where(id: route.id).for_update.first or raise Error.new("Route '#{route.guid}' not found.")
 
-        existing_policies = RoutePolicy.where(route_id: route.id).all
-        validate_source_exclusivity(existing_policies, message.source)
-
         RoutePolicy.create(
           source: message.source,
           route_id: route.id
         )
       end
+
+      route_policy.notify_diego
+
+      Repositories::RoutePolicyEventRepository.new.record_route_policy_create(
+        route_policy,
+        @user_audit_info,
+        { 'source' => message.source, 'route_guid' => route.guid }
+      )
+
+      route_policy
     rescue Sequel::UniqueConstraintViolation
       raise Error.new("A route policy with source '#{message.source}' already exists for this route.")
+    rescue Sequel::ValidationFailed => e
+      raise Error.new(e.errors.full_messages.join(', '))
     end
 
     private
@@ -34,14 +49,5 @@ def validate_scope!(domain, source)
 
       raise Error.new("Source '#{source}' is not allowed: domain's route_policies_scope is 'space'.")
     end
-
-    def validate_source_exclusivity(locked_policies, source)
-      existing_sources = locked_policies.map(&:source)
-
-      # Enforce cf:any exclusivity: if new policy is cf:any, reject if route already has any policies;
-      # if route already has a cf:any policy, reject new policies.
-      raise Error.new("Cannot add 'cf:any' source when other route policies already exist for this route.") if source == 'cf:any' && existing_sources.any?
-      raise Error.new("Cannot add source '#{source}': route already has a 'cf:any' policy.") if existing_sources.include?('cf:any')
-    end
   end
 end

app/actions/route_policy_destroy.rb

@@ -0,0 +1,21 @@
+require 'repositories/route_policy_event_repository'
+
+module VCAP::CloudController
+  class RoutePolicyDestroy
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
+    def delete(route_policy)
+      route_policy.destroy
+      route_policy.notify_diego
+
+      Repositories::RoutePolicyEventRepository.new.record_route_policy_delete(
+        route_policy,
+        @user_audit_info
+      )
+
+      nil
+    end
+  end
+end

app/actions/route_policy_update.rb

@@ -0,0 +1,21 @@
+require 'repositories/route_policy_event_repository'
+
+module VCAP::CloudController
+  class RoutePolicyUpdate
+    def initialize(user_audit_info)
+      @user_audit_info = user_audit_info
+    end
+
+    def update(route_policy, message)
+      MetadataUpdate.update(route_policy, message)
+
+      Repositories::RoutePolicyEventRepository.new.record_route_policy_update(
+        route_policy,
+        @user_audit_info,
+        message.audit_hash
+      )
+
+      route_policy
+    end
+  end
+end

app/controllers/v3/route_policies_controller.rb

@@ -6,7 +6,8 @@
 require 'decorators/include_route_policy_source_decorator'
 require 'decorators/include_route_policy_route_decorator'
 require 'actions/route_policy_create'
-require 'repositories/route_policy_event_repository'
+require 'actions/route_policy_update'
+require 'actions/route_policy_destroy'
 require 'fetchers/label_selector_query_generator'
 
 class RoutePoliciesController < ApplicationController
@@ -53,13 +54,7 @@ def create
     route = find_and_authorize_route(message.route_guid)
     validate_route_domain(route)
 
-    route_policy = VCAP::CloudController::RoutePolicyCreate.new.create(route: route, message: message)
-
-    route_policy_event_repository.record_route_policy_create(
-      route_policy,
-      user_audit_info,
-      { 'source' => message.source, 'route_guid' => message.route_guid }
-    )
+    route_policy = VCAP::CloudController::RoutePolicyCreate.new(user_audit_info).create(route: route, message: message)
 
     render status: :created, json: Presenters::V3::RoutePolicyPresenter.new(route_policy)
   rescue VCAP::CloudController::RoutePolicyCreate::Error => e
@@ -75,13 +70,7 @@ def update
     message = RoutePolicyUpdateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    VCAP::CloudController::MetadataUpdate.update(route_policy, message)
-
-    route_policy_event_repository.record_route_policy_update(
-      route_policy.reload,
-      user_audit_info,
-      message.audit_hash
-    )
+    VCAP::CloudController::RoutePolicyUpdate.new(user_audit_info).update(route_policy, message)
 
     render status: :ok, json: Presenters::V3::RoutePolicyPresenter.new(route_policy.reload)
   end
@@ -92,21 +81,13 @@ def destroy
 
     find_and_authorize_route_for_policy(route_policy)
 
-    route_policy.destroy
+    VCAP::CloudController::RoutePolicyDestroy.new(user_audit_info).delete(route_policy)
 
-    route_policy_event_repository.record_route_policy_delete(
-      route_policy,
-      user_audit_info
-    )
     head :no_content
   end
 
   private
 
-  def route_policy_event_repository
-    @route_policy_event_repository ||= Repositories::RoutePolicyEventRepository.new
-  end
-
   def find_and_authorize_route(route_guid)
     route = VCAP::CloudController::Route.find(guid: route_guid)
     resource_not_found!(:route) unless route && permission_queryer.can_read_from_space?(route.space.id, route.space.organization_id)

app/models/runtime/route_policy.rb

@@ -33,27 +33,29 @@ def source=(val)
     def validate
       validates_presence :source_type
       validates_presence :route_id
+      validate_cf_any_exclusivity
     end
 
-    def after_create
-      super
-      notify_processes_of_route_update
-    end
+    def notify_diego
+      return unless route
 
-    def after_destroy
-      super
-      notify_processes_of_route_update
+      route.apps.each do |process|
+        ProcessRouteHandler.new(process).notify_backend_of_route_update
+      end
     end
 
     private
 
-    def notify_processes_of_route_update
-      return unless route
+    def validate_cf_any_exclusivity
+      return unless route_id
+
+      siblings = RoutePolicy.where(route_id: route_id).exclude(id: id)
+      return if siblings.empty?
 
-      db.after_commit do
-        route.apps.each do |process|
-          ProcessRouteHandler.new(process).notify_backend_of_route_update
-        end
+      if source_type == 'any'
+        errors.add(:source, "'cf:any' cannot coexist with other route policies on the same route")
+      elsif siblings.where(source_type: 'any').any?
+        errors.add(:source, "cannot coexist with the existing 'cf:any' policy on this route")
       end
     end
   end

spec/request/route_policies_spec.rb

@@ -580,14 +580,15 @@ def expected_rule_json(rule)
     end
 
     describe 'filtering by label_selector' do
-      let(:another_route) { create(:route, space: space, domain: mtls_domain) }
+      let(:second_route) { create(:route, space: space, domain: mtls_domain) }
+      let(:third_route) { create(:route, space: space, domain: mtls_domain) }
       let!(:labelled_policy) do
-        policy = VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: mtls_route.id)
+        policy = VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: second_route.id)
         VCAP::CloudController::RoutePolicyLabelModel.create(resource_guid: policy.guid, key_name: 'env', value: 'prod')
         policy
       end
       let!(:unlabelled_policy) do
-        VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: another_route.id)
+        VCAP::CloudController::RoutePolicy.create(source: 'cf:any', route_id: third_route.id)
       end
 
       it 'returns only policies matching the label selector' do
@@ -715,7 +716,7 @@ def expected_rule_json(rule)
       let!(:rule_on_route1) do
         VCAP::CloudController::RoutePolicy.create(
           guid: SecureRandom.uuid,
-          source: 'cf:any',
+          source: "cf:app:#{SecureRandom.uuid}",
           route_id: mtls_route.id
         )
       end

spec/unit/actions/route_policy_create_spec.rb

@@ -3,8 +3,9 @@
 
 module VCAP::CloudController
   RSpec.describe RoutePolicyCreate do
-    subject(:action) { RoutePolicyCreate.new }
+    subject(:action) { RoutePolicyCreate.new(user_audit_info) }
 
+    let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') }
     let(:space) { create(:space) }
     let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) }
     let(:route) { create(:route, space:, domain:) }
@@ -33,6 +34,20 @@ module VCAP::CloudController
         expect(policy.source_guid).to eq('')
       end
 
+      it 'notifies diego after the transaction commits' do
+        expect_any_instance_of(RoutePolicy).to receive(:notify_diego).once
+
+        action.create(route:, message:)
+      end
+
+      it 'records a route_policy_create audit event' do
+        expect_any_instance_of(Repositories::RoutePolicyEventRepository).
+          to receive(:record_route_policy_create).
+          with(instance_of(RoutePolicy), user_audit_info, { 'source' => "cf:app:#{app_guid}", 'route_guid' => route.guid })
+
+        action.create(route:, message:)
+      end
+
       context 'when the same source already exists for the route' do
         before do
           RoutePolicy.create(source: "cf:app:#{app_guid}", route_id: route.id)
@@ -55,7 +70,7 @@ module VCAP::CloudController
         it 'raises an error' do
           expect do
             action.create(route:, message:)
-          end.to raise_error(RoutePolicyCreate::Error, /cannot add 'cf:any'/i)
+          end.to raise_error(RoutePolicyCreate::Error, /'cf:any' cannot coexist with other route policies/)
         end
       end
 
@@ -68,7 +83,7 @@ module VCAP::CloudController
           other_message = instance_double(RoutePolicyCreateMessage, source: "cf:app:#{SecureRandom.uuid}")
           expect do
             action.create(route: route, message: other_message)
-          end.to raise_error(RoutePolicyCreate::Error, /already has a 'cf:any' policy/)
+          end.to raise_error(RoutePolicyCreate::Error, /cannot coexist with the existing 'cf:any' policy/)
         end
       end
 

spec/unit/actions/route_policy_destroy_spec.rb

@@ -0,0 +1,35 @@
+require 'spec_helper'
+require 'actions/route_policy_destroy'
+
+module VCAP::CloudController
+  RSpec.describe RoutePolicyDestroy do
+    subject(:action) { RoutePolicyDestroy.new(user_audit_info) }
+
+    let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') }
+    let(:space) { create(:space) }
+    let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) }
+    let(:route) { create(:route, space:, domain:) }
+    let(:app_guid) { SecureRandom.uuid }
+    let!(:route_policy) { RoutePolicy.create(source: "cf:app:#{app_guid}", route: route) }
+
+    describe '#delete' do
+      it 'destroys the route policy' do
+        expect { action.delete(route_policy) }.to change(RoutePolicy, :count).by(-1)
+      end
+
+      it 'notifies diego after destroying' do
+        expect(route_policy).to receive(:notify_diego).once
+
+        action.delete(route_policy)
+      end
+
+      it 'records a route_policy_delete audit event' do
+        expect_any_instance_of(Repositories::RoutePolicyEventRepository).
+          to receive(:record_route_policy_delete).
+          with(route_policy, user_audit_info)
+
+        action.delete(route_policy)
+      end
+    end
+  end
+end

spec/unit/actions/route_policy_update_spec.rb

@@ -0,0 +1,39 @@
+require 'spec_helper'
+require 'actions/route_policy_update'
+
+module VCAP::CloudController
+  RSpec.describe RoutePolicyUpdate do
+    subject(:action) { RoutePolicyUpdate.new(user_audit_info) }
+
+    let(:user_audit_info) { UserAuditInfo.new(user_email: 'user@example.com', user_guid: 'some-user-guid') }
+    let(:space) { create(:space) }
+    let(:domain) { create(:shared_domain, name: 'apps.identity', enforce_route_policies: true) }
+    let(:route) { create(:route, space:, domain:) }
+    let(:app_guid) { SecureRandom.uuid }
+    let(:route_policy) { RoutePolicy.create(source: "cf:app:#{app_guid}", route: route) }
+    let(:message) do
+      RoutePolicyUpdateMessage.new(metadata: { labels: { 'key' => 'value' }, annotations: { 'a' => 'note' } })
+    end
+
+    describe '#update' do
+      it 'applies the metadata update' do
+        action.update(route_policy, message)
+
+        expect(route_policy.reload.labels.map { |l| [l.key_name, l.value] }).to include(%w[key value])
+        expect(route_policy.annotations.map { |a| [a.key_name, a.value] }).to include(%w[a note])
+      end
+
+      it 'records a route_policy_update audit event' do
+        expect_any_instance_of(Repositories::RoutePolicyEventRepository).
+          to receive(:record_route_policy_update).
+          with(route_policy, user_audit_info, message.audit_hash)
+
+        action.update(route_policy, message)
+      end
+
+      it 'returns the route policy' do
+        expect(action.update(route_policy, message)).to eq(route_policy)
+      end
+    end
+  end
+end