Consolidate communication with the routing API

* centralize calls out to routing API to instance method with memoized
global variable on the domain
* other minor refactors of where error messages get created

[finishes #169299174]

Authored-by: Mona Mohebbi <mmohebbi@pivotal.io>

Author: monamohebbi

app/actions/route_create.rb

@@ -8,6 +8,8 @@ def initialize(user_audit_info)
     end
 
     def create(message:, space:, domain:, manifest_triggered: false)
+      validate_tcp_route!(domain, message)
+
       route = Route.new(
         host: message.host || '',
         path: message.path || '',
@@ -43,9 +45,15 @@ def create(message:, space:, domain:, manifest_triggered: false)
 
     private
 
+    def validate_tcp_route!(domain, message)
+      if domain.router_group_guid.present? && router_group(domain).nil?
+        error!('Route could not be created because the specified domain does not have a valid router group.')
+      end
+    end
+
     def port(message, domain)
       generated_port = if !message.requested?(:port) && domain.protocols.include?('tcp')
-                         PortGenerator.generate_port(domain.guid)
+                         PortGenerator.generate_port(domain.guid, router_group(domain).reservable_ports)
                        else
                          message.port || 0
                        end
@@ -54,6 +62,10 @@ def port(message, domain)
       generated_port
     end
 
+    def router_group(domain)
+      @router_group ||= domain.router_group
+    end
+
     def route_crd_client
       @route_crd_client ||= CloudController::DependencyLocator.instance.route_crd_client
     end
@@ -135,7 +147,11 @@ def validation_error_host!(error, host, domain)
       end
 
       if error.errors.on(:host)&.include?(:host_and_path_domain_tcp)
-        error!("Routes with protocol 'tcp' do not support paths or hosts.")
+        error!('Hosts are not supported for TCP routes.')
+      end
+
+      if error.errors.on(:path)&.include?(:host_and_path_domain_tcp)
+        error!('Paths are not supported for TCP routes.')
       end
     end
 

app/controllers/runtime/routes_controller.rb

@@ -68,7 +68,7 @@ def self.translate_validation_exception(e, attributes)
           'Try a different port or request a random one be generated for you.'
         )
       end
-      if e.errors.on(:host) == [:host_and_path_domain_tcp]
+      if e.errors.on(:host) == [:host_and_path_domain_tcp] || e.errors.on(:path) == [:host_and_path_domain_tcp]
         return CloudController::Errors::ApiError.new_from_details('RouteInvalid', 'Host and path are not supported, as domain belongs to a TCP router group.')
       end
 

app/controllers/v3/routes_controller.rb

@@ -67,7 +67,7 @@ def create
     unprocessable_space! unless space
     unprocessable_domain! unless domain
     unauthorized! unless permission_queryer.can_write_to_space?(space.guid)
-    validate_domain_for_route_create!(domain, message)
+    unprocessable_wildcard! if domain.shared? && message.wildcard? && !permission_queryer.can_write_globally?
 
     route = RouteCreate.new(user_audit_info).create(message: message, space: space, domain: domain)
 
@@ -236,19 +236,6 @@ def unprocessable_protocol_path!
     unprocessable!('Paths are not supported for TCP routes.')
   end
 
-  def validate_domain_for_route_create!(domain, message)
-    unprocessable_wildcard! if domain.shared? && message.wildcard? && !permission_queryer.can_write_globally?
-
-    if domain.protocols.include?('tcp')
-      unprocessable_protocol_host! if message.host
-      unprocessable_protocol_path! if message.path
-    end
-
-    if domain.router_group_guid && routing_api_client.router_group(domain.router_group_guid).nil?
-      unprocessable!('Route could not be created because the specified domain does not have a valid router group.')
-    end
-  end
-
   def validate_app_guids!(apps_hash, desired_app_guids)
     existing_app_guids = apps_hash.keys
 

app/models/runtime/shared_domain.rb

@@ -73,7 +73,6 @@ def tcp?
 
       if router_group_guid.present?
         if @router_group_type.nil?
-          router_group = routing_api_client.router_group(router_group_guid)
           @router_group_type = router_group.nil? ? '' : router_group.type
         end
 
@@ -84,8 +83,6 @@ def tcp?
     end
 
     def router_group
-      raise RoutingApi::RoutingApiDisabled unless routing_api_client.enabled?
-
       @router_group ||= routing_api_client.router_group(router_group_guid)
     end
 
@@ -99,12 +96,12 @@ def internal?
       !!internal
     end
 
-    private
-
     def routing_api_client
       @routing_api_client ||= CloudController::DependencyLocator.instance.routing_api_client
     end
 
+    private
+
     def validate_internal_domain
       if router_group_guid.present?
         errors.add(:router_group_guid, 'cannot be specified for internal domains')

app/presenters/v3/domain_presenter.rb

@@ -82,8 +82,7 @@ def build_links
         }
       end
 
-      routing_client = CloudController::DependencyLocator.instance.routing_api_client
-      if domain.router_group_guid && routing_client.enabled?
+      if domain.router_group_guid && domain.routing_api_client.enabled?
         links[:router_group] = {
           href: url_builder.build_url(path: "/routing/v1/router_groups/#{domain.router_group_guid}")
         }

lib/cloud_controller/port_generator.rb

@@ -1,15 +1,14 @@
 module VCAP::CloudController
   class PortGenerator
     class << self
-      def generate_port(domain_guid, possible_ports=nil)
+      def generate_port(domain_guid, possible_ports)
         domain = SharedDomain.where(guid: domain_guid).first
         router_group_guid = domain.router_group_guid
 
         unavailable_ports = Route.join(:domains, id: :domain_id).
                             where(router_group_guid: router_group_guid).
                             select_map(:port)
 
-        possible_ports ||= domain.router_group.reservable_ports
         available_ports = possible_ports - unavailable_ports
 
         size = available_ports.size

lib/cloud_controller/route_validator.rb

@@ -52,7 +52,7 @@ def validate_host_not_included
 
     def validate_path_not_included
       unless route.path.blank?
-        route.errors.add(:host, :host_and_path_domain_tcp)
+        route.errors.add(:path, :host_and_path_domain_tcp)
       end
     end
 

spec/request/routes_spec.rb

@@ -2182,6 +2182,7 @@
 
       context 'when the routing API is unavailable' do
         before do
+          allow(routing_api_client).to receive(:enabled?).and_return true
           allow(routing_api_client).to receive(:router_group).and_raise VCAP::CloudController::RoutingApi::RoutingApiUnavailable
         end
 
@@ -2196,7 +2197,7 @@
       context 'when the routing API is disabled' do
         before do
           allow(routing_api_client).to receive(:enabled?).and_return false
-          allow(routing_api_client).to receive(:router_group).and_return router_group
+          allow(routing_api_client).to receive(:router_group).and_raise VCAP::CloudController::RoutingApi::RoutingApiDisabled
         end
 
         it 'returns a 503 with a helpful error message' do

spec/unit/actions/route_create_spec.rb

@@ -697,7 +697,7 @@ module VCAP::CloudController
             it 'errors with a helpful error message' do
               expect {
                 subject.create(message: message, space: space, domain: domain)
-              }.to raise_error(RouteCreate::Error, "Routes with protocol 'tcp' do not support paths or hosts.")
+              }.to raise_error(RouteCreate::Error, 'Paths are not supported for TCP routes.')
             end
           end
         end

spec/unit/lib/cloud_controller/port_generator_spec.rb

@@ -35,22 +35,6 @@ module VCAP::CloudController
         expect(port).to eq(-1)
       end
 
-      context 'when possible ports are not provided' do
-        let(:router_group1) { double('router_group1', type: router_group_type, guid: router_group_guid1, reservable_ports: Array(1024..65535)) }
-        let(:routing_api_client) { instance_double(VCAP::CloudController::RoutingApi::Client) }
-
-        before do
-          allow_any_instance_of(CloudController::DependencyLocator).to receive(:routing_api_client).and_return(routing_api_client)
-          allow(routing_api_client).to receive(:enabled?).and_return(true)
-          allow(routing_api_client).to receive(:router_group).and_return(router_group1)
-        end
-        it 'generates a port' do
-          port = PortGenerator.generate_port(domain_guid1)
-
-          expect((1024..65535).cover?(port)).to eq(true)
-        end
-      end
-
       context 'when there are multi router groups' do
         let(:router_group_guid2) { 'router-group-guid2' }
         let(:router_group2) { double('router_group2', type: router_group_type, guid: router_group_guid2) }