Disallow null in manifest; Cleanup error outputs

Author: hoffmaen

app/actions/route_update.rb

@@ -2,13 +2,7 @@ module VCAP::CloudController
   class RouteUpdate
     def update(route:, message:)
       Route.db.transaction do
-        if message.requested?(:options)
-          route.options = if message.options.nil?
-                            {}
-                          else
-                            route.options.symbolize_keys.merge(message.options).compact
-                          end
-        end
+        route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options)
 
         route.save
         MetadataUpdate.update(route, message)

app/messages/manifest_routes_update_message.rb

@@ -50,11 +50,12 @@ def route_options_are_valid
       return if errors[:routes].present?
 
       routes.any? do |r|
-        next unless r[:options]
+        next unless r.keys.include?(:options)
 
-        return true unless r[:options].is_a?(Hash)
-
-        return false if r[:options].empty?
+        unless r[:options].is_a?(Hash)
+          errors.add(:base, message: "Route '#{r[:route]}': options must be an object")
+          next
+        end
 
         r[:options].each_key do |key|
           RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) &&
@@ -69,13 +70,19 @@ def loadbalancings_are_valid
       return if errors[:routes].present?
 
       routes.each do |r|
-        next unless r[:options] && r[:options][:'loadbalancing']
+        next unless r.keys.include?(:options) && r[:options].is_a?(Hash) && r[:options].keys.include?(:loadbalancing)
 
-        loadbalancing = r[:options][:'loadbalancing']
+        loadbalancing = r[:options][:loadbalancing]
+        unless loadbalancing.is_a?(String)
+          errors.add(:base,
+                     message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \
+Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
+          next
+        end
         RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(loadbalancing) &&
           errors.add(:base,
-                     message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{loadbalancing}'. \
-Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
+                     message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \
+Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
       end
     end
 

app/messages/route_options_message.rb

@@ -9,7 +9,7 @@ class RouteOptionsMessage < BaseMessage
     register_allowed_keys VALID_ROUTE_OPTIONS
     validates_with NoAdditionalKeysValidator
     validates :loadbalancing,
-              inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%<value>s' is not supported" },
+              inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "must be one of '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}' if present" },
               presence: true,
               allow_nil: true
   end

app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb

@@ -12,7 +12,7 @@ def to_hash(route_mappings:, app:, **_)
 
               if route_mapping.route.options
                 route_hash[:options] = {}
-                route_hash[:options][:'loadbalancing'] = route_mapping.route.options[:loadbalancing] if route_mapping.route.options[:loadbalancing]
+                route_hash[:options][:loadbalancing] = route_mapping.route.options[:loadbalancing] if route_mapping.route.options[:loadbalancing]
               end
 
               route_hash

docs/v3/source/includes/resources/routes/_route_options_object.md.erb

@@ -7,6 +7,6 @@ Example Route-Options object
 <%= yield_content :route_options %>
 ```
 
-| Name        | Type     | Description                                                                                              |
-|-------------|----------|----------------------------------------------------------------------------------------------------------|
-| **loadbalancing** | _string_ | The load balancer associated with this route. Valid algorithms are 'round-robin' and 'least-connections' |
+| Name              | Type     | Description                                                                                          |
+|-------------------|----------|------------------------------------------------------------------------------------------------------|
+| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin' and 'least-connections' |

lib/cloud_controller/app_manifest/manifest_route.rb

@@ -17,7 +17,7 @@ def self.parse(route, options=nil)
 
       attrs[:full_route] = route
       attrs[:options] = {}
-      attrs[:options][:loadbalancing] = options[:'loadbalancing'] if options && options.key?(:'loadbalancing')
+      attrs[:options][:loadbalancing] = options[:loadbalancing] if options && options.key?(:loadbalancing)
 
       ManifestRoute.new(attrs)
     end

spec/request/space_manifests_spec.rb

@@ -658,7 +658,7 @@
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}",
                     'options' => {
                       'loadbalancing' => 'round-robin'
                     } }
@@ -683,7 +683,7 @@
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}",
                     'options' => {
                       'loadbalancing' => 'least-connections'
                     } }
@@ -707,7 +707,7 @@
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}" }
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}" }
                 ] }
             ]
           }.to_yaml
@@ -724,12 +724,12 @@
           expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' })
         end
 
-        it 'does not modify any route options options: nil is provided' do
+        it 'returns 422 when options: null is provided' do
           yml_manifest = {
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}",
                     'options' => nil }
                 ] }
             ]
@@ -738,9 +738,11 @@
           # apply the manifest with the route option
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
-          expect(last_response.status).to eq(202)
-          job_guid = VCAP::CloudController::PollableJobModel.last.guid
+          expect(last_response.status).to eq(422)
+          expect(last_response).to have_error_message("For application '#{app1_model.name}': \
+Route 'https://#{route.host}.#{route.domain.name}': options must be an object")
 
+          job_guid = VCAP::CloudController::PollableJobModel.last.guid
           Delayed::Worker.new.work_off
           expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error
           app1_model.reload
@@ -752,7 +754,7 @@
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}",
                     'options' => {} }
                 ] }
             ]
@@ -767,12 +769,12 @@
           expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' })
         end
 
-        it 'does not modify any option when options: { key: nil } is provided' do
+        it 'returns 422 when { loadbalancing: null } is provided' do
           yml_manifest = {
             'applications' => [
               { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                  { 'route' => "https://#{route.host}.#{shared_domain.name}",
                     'options' => {
                       'loadbalancing' => nil
                     } }
@@ -783,7 +785,9 @@
           # apply the manifest with the route option
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
-          expect(last_response.status).to eq(202)
+          expect(last_response.status).to eq(422)
+          expect(last_response).to have_error_message("For application '#{app1_model.name}': \
+Invalid value for 'loadbalancing' for Route 'https://#{route.host}.#{route.domain.name}'; Valid values are: 'round-robin, least-connections'")
 
           app1_model.reload
           expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' })
@@ -813,7 +817,7 @@
 
             expect(last_response).to have_status_code(422)
             expect(last_response).to have_error_message("For application '#{app1_model.name}': \
-Route 'https://#{route.host}.#{route.domain.name}' contains invalid load-balancing algorithm 'unsupported-lb-algorithm'. Valid algorithms: 'round-robin, least-connections'")
+Cannot use loadbalancing value 'unsupported-lb-algorithm' for Route 'https://#{route.host}.#{route.domain.name}'; Valid values are: 'round-robin, least-connections'")
           end
         end
 
@@ -823,7 +827,7 @@
               'applications' => [
                 { 'name' => app1_model.name,
                   'routes' => [
-                    { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                    { 'route' => "https://#{route.host}.#{shared_domain.name}",
                       'options' => {
                         'loadbalancing' => 'round-robin'
                       } }

spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb

@@ -176,7 +176,7 @@ module VCAP::CloudController
       end
 
       it 'parses a route with a loadbalancing into route components' do
-        route = ManifestRoute.parse('http://potato.sub.some-domain.com', { 'loadbalancing': 'round-robin' })
+        route = ManifestRoute.parse('http://potato.sub.some-domain.com', { loadbalancing: 'round-robin' })
 
         expect(route.to_hash).to eq({
                                       candidate_host_domain_pairs: [

spec/unit/messages/manifest_routes_update_message_spec.rb

@@ -237,7 +237,8 @@ module VCAP::CloudController
         it 'returns true' do
           msg = ManifestRoutesUpdateMessage.new(body)
 
-          expect(msg.valid?).to be(true)
+          expect(msg.valid?).to be(false)
+          expect(msg.errors.full_messages).to include("Route 'existing.example.com': options must be an object")
         end
       end
 
@@ -291,7 +292,8 @@ module VCAP::CloudController
         it 'returns true' do
           msg = ManifestRoutesUpdateMessage.new(body)
 
-          expect(msg.valid?).to be(true)
+          expect(msg.valid?).to be(false)
+          expect(msg.errors.full_messages).to include("Invalid value for 'loadbalancing' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connections'")
         end
       end
 
@@ -311,8 +313,7 @@ module VCAP::CloudController
 
           expect(msg.valid?).to be(false)
           expect(msg.errors.errors.length).to eq(1)
-          expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid load-balancing algorithm 'sushi'. \
-Valid algorithms: 'round-robin, least-connections'")
+          expect(msg.errors.full_messages).to include("Cannot use loadbalancing value 'sushi' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connections'")
         end
       end
     end

spec/unit/messages/route_create_message_spec.rb

@@ -492,7 +492,7 @@ module VCAP::CloudController
 
             it 'is not valid' do
               expect(subject).not_to be_valid
-              expect(subject.errors[:options]).to include("Loadbalancing 'random' is not supported")
+              expect(subject.errors[:options]).to include("Loadbalancing must be one of 'round-robin, least-connections' if present")
             end
           end
         end