Adjust route options behaviour for manifest push

Overwrite behaviour for route options is now fixed and tested:
Existing options are not modified if options is nil, {} or not provided
A single option (e.g. loadbalancing-algorithm) can be removed by setting its value to nil

adjust test for manifest push:
options {key:nil} should not modify the existing value

Author: a18e

app/actions/manifest_route_update.rb

@@ -92,7 +92,9 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info)
               domain: existing_domain,
               manifest_triggered: true
             )
-          elsif route[:options] != manifest_route[:options]
+          elsif manifest_route[:options] && route[:options] != manifest_route[:options]
+            # remove nil values from options
+            manifest_route[:options] = manifest_route[:options].compact
             message = RouteUpdateMessage.new({
                                                'options' => manifest_route[:options]
                                              })

app/actions/route_update.rb

@@ -11,6 +11,7 @@ def update(route:, message:)
                             route.options.merge(message.options)
                           end
         end
+
         route.save
         MetadataUpdate.update(route, message)
       end

lib/cloud_controller/app_manifest/manifest_route.rb

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

spec/request/space_manifests_spec.rb

@@ -606,7 +606,7 @@
       before do
         app1_model.web_processes.first.update(state: VCAP::CloudController::ProcessModel::STARTED, instances: 4)
         space.update(space_quota_definition:
-          VCAP::CloudController::SpaceQuotaDefinition.make(organization: space.organization, log_rate_limit: 0))
+                       VCAP::CloudController::SpaceQuotaDefinition.make(organization: space.organization, log_rate_limit: 0))
       end
 
       it 'successfully applies the manifest' do
@@ -625,54 +625,171 @@
     end
 
     describe 'route options' do
-      context 'when an empty route options hash is provided' do
+      context 'when an invalid route option is provided' do
         let(:yml_manifest) do
           {
             'applications' => [
               {
                 'name' => app1_model.name,
                 'routes' => [
                   { 'route' => "https://#{route.host}.#{route.domain.name}",
-                    'options' => {} }
+                    'options' => {
+                      'doesnt-exist' => 'doesnt-exist'
+                    } }
                 ]
               }
             ]
           }.to_yaml
         end
 
-        it 'applies the manifest' do
+        it 'returns a 422' do
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
-          expect(last_response.status).to eq(202)
+          expect(last_response).to have_status_code(422)
+          expect(last_response).to have_error_message('Routes contains invalid route options')
         end
       end
 
-      context 'when an invalid route option is provided' do
-        let(:yml_manifest) do
-          {
+      context 'updating existing route options' do
+        # using loadbalancing-algorithm as an example since it is the only route option currently supported
+        before do
+          yml_manifest = {
             'applications' => [
-              {
-                'name' => app1_model.name,
+              { 'name' => app1_model.name,
                 'routes' => [
-                  { 'route' => "https://#{route.host}.#{route.domain.name}",
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
                     'options' => {
-                      'doesnt-exist' => 'doesnt-exist'
+                      'loadbalancing-algorithm' => 'round-robin'
                     } }
-                ]
-              }
+                ] }
             ]
           }.to_yaml
+
+          # 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
+
+          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
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
         end
 
-        it 'returns a 422' do
+        it 'updates the route option when a new value is provided' do
+          yml_manifest = {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'routes' => [
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                    'options' => {
+                      'loadbalancing-algorithm' => 'least-connections'
+                    } }
+                ] }
+            ]
+          }.to_yaml
+
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
-          expect(last_response).to have_status_code(422)
-          expect(last_response).to have_error_message('Routes contains invalid route options')
+          expect(last_response.status).to eq(202)
+          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
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' })
+        end
+
+        it 'does not modify any route options when the options hash is not provided' do
+          yml_manifest = {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'routes' => [
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}" }
+                ] }
+            ]
+          }.to_yaml
+
+          # 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
+
+          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
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
+        end
+
+        it 'does not modify any route options options: nil is provided' do
+          yml_manifest = {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'routes' => [
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                    'options' => nil }
+                ] }
+            ]
+          }.to_yaml
+
+          # 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
+
+          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
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
+        end
+
+        it 'does not modify any route options if an empty options hash is provided' do
+          yml_manifest = {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'routes' => [
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                    'options' => {} }
+                ] }
+            ]
+          }.to_yaml
+
+          # 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)
+
+          app1_model.reload
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
+        end
+
+        it 'does not modify any option when options: { key: nil } is provided' do
+          yml_manifest = {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'routes' => [
+                  { 'route' => "https://round-robin-app.#{shared_domain.name}",
+                    'options' => {
+                      'loadbalancing-algorithm' => nil
+                    } }
+                ] }
+            ]
+          }.to_yaml
+
+          # 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)
+
+          app1_model.reload
+          expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
         end
       end
 
-      context 'loadbalancing-algorithm' do
+      context 'route-option: loadbalancing-algorithm' do
         context 'when the loadbalancing-algorithm is not supported' do
           let(:yml_manifest) do
             {
@@ -713,7 +830,7 @@
             }.to_yaml
           end
 
-          it 'adds and updates the loadbalancing-algorithm' do
+          it 'adds the loadbalancing-algorithm' do
             post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
             expect(last_response.status).to eq(202)
@@ -724,31 +841,6 @@
 
             app1_model.reload
             expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' })
-
-            ### update the loadbalancing-algorithm from the route
-
-            yml_manifest = {
-              'applications' => [
-                { 'name' => app1_model.name,
-                  'routes' => [
-                    { 'route' => "https://round-robin-app.#{shared_domain.name}",
-                      'options' => {
-                        'loadbalancing-algorithm' => 'least-connections'
-                      } }
-                  ] }
-              ]
-            }.to_yaml
-
-            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
-
-            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
-            expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' })
           end
         end
       end