File-based-service-bindings: improved errors (#4693)

* Convert IncompatibleBindings error to UnprocessableEntity (ApiError)

The ServiceBindingFilesBuilder can raise an IncompatibleBindings
exception in case of entries violating the specification for file names
or if the overall file size is too big. When pushing an app these errors
were shown as StagerErrors (with the correct error message); for other
operations - i.e. restart, restage - an UnknownError was returned. This
change ensures that in any case an UnprocessableEntity (ApiError) is
returned (with the concrete error message).

* Fix validation of conflicting manifest features

Switching from service-binding-k8s to file-based-vcap-services via
manifest entries (features) raised an error. This change fixes the wrong
validation for this corner case.

Author: philippthun

app/actions/app_feature_update.rb

@@ -30,8 +30,8 @@ def feature_column_name(feature_name)
       end
 
       def check_invalid_combination!(app, flags)
-        file_based_vcap_services_enabled = flags[feature_column_name(AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE)].present? || app.file_based_vcap_services_enabled
-        service_binding_k8s_enabled = flags[feature_column_name(AppFeatures::SERVICE_BINDING_K8S_FEATURE)].present? || app.service_binding_k8s_enabled
+        file_based_vcap_services_enabled = flags.fetch(feature_column_name(AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE), app.file_based_vcap_services_enabled)
+        service_binding_k8s_enabled = flags.fetch(feature_column_name(AppFeatures::SERVICE_BINDING_K8S_FEATURE), app.service_binding_k8s_enabled)
         return unless file_based_vcap_services_enabled && service_binding_k8s_enabled
 
         msg = "'#{AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE}' and '#{AppFeatures::SERVICE_BINDING_K8S_FEATURE}' features cannot be enabled at the same time."

lib/cloud_controller/diego/app_recipe_builder.rb

@@ -105,6 +105,8 @@ def app_lrp_arguments
           image_password: process.desired_droplet.docker_receipt_password,
           volume_mounted_files: ServiceBindingFilesBuilder.build(process)
         }.compact
+      rescue ServiceBindingFilesBuilder::IncompatibleBindings => e
+        raise CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', "Cannot build service binding files for app - #{e.message}")
       end
 
       def metric_tags(process)

lib/cloud_controller/diego/task_recipe_builder.rb

@@ -56,6 +56,8 @@ def build_app_task(config, task)
           image_password: task.droplet.docker_receipt_password,
           volume_mounted_files: ServiceBindingFilesBuilder.build(task.app)
         }.compact)
+      rescue ServiceBindingFilesBuilder::IncompatibleBindings => e
+        raise CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', "Cannot build service binding files for app task - #{e.message}")
       end
 
       def build_staging_task(config, staging_details)
@@ -95,6 +97,8 @@ def build_staging_task(config, staging_details)
           image_password: staging_details.package.docker_password,
           volume_mounted_files: ServiceBindingFilesBuilder.build(staging_details.package.app)
         }.compact)
+      rescue ServiceBindingFilesBuilder::IncompatibleBindings => e
+        raise CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', "Cannot build service binding files for staging task - #{e.message}")
       end
 
       private

spec/unit/actions/app_feature_update_spec.rb

@@ -122,6 +122,27 @@ module VCAP::CloudController
         end
       end
 
+      context 'when switching from service-binding-k8s to file-based-vcap-services' do
+        before do
+          app.update(service_binding_k8s_enabled: true)
+          features[:'service-binding-k8s'] = false
+          features[:'file-based-vcap-services'] = true
+        end
+
+        it 'updates the corresponding columns in a single database call' do
+          expect(app.service_binding_k8s_enabled).to be(true)
+          expect(app.file_based_vcap_services_enabled).to be(false)
+
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to have_queried_db_times(/update .apps./i, 1)
+
+          app.reload
+          expect(app.service_binding_k8s_enabled).to be(false)
+          expect(app.file_based_vcap_services_enabled).to be(true)
+        end
+      end
+
       context 'when conflicting features are specified' do
         before do
           features[:'service-binding-k8s'] = true

spec/unit/lib/cloud_controller/diego/app_recipe_builder_spec.rb

@@ -57,6 +57,20 @@ module Diego
         end
       end
 
+      shared_examples 'IncompatibleBindings error handling' do
+        let(:incompatible_bindings_error) { VCAP::CloudController::Diego::ServiceBindingFilesBuilder::IncompatibleBindings.new('something is wrong') }
+
+        context 'when an IncompatibleBindings error is raised' do
+          before do
+            allow_any_instance_of(VCAP::CloudController::Diego::ServiceBindingFilesBuilder).to receive(:build).and_raise(incompatible_bindings_error)
+          end
+
+          it 'results in an UnprocessableEntity error' do
+            expect { _build }.to raise_error(CloudController::Errors::ApiError, /Cannot build service binding files .* - something is wrong/)
+          end
+        end
+      end
+
       shared_examples 'k8s service bindings' do
         context 'when k8s service bindings are enabled' do
           before do
@@ -69,6 +83,10 @@ module Diego
             lrp = builder.build_app_lrp
             expect(lrp.volume_mounted_files.size).to be > 1
           end
+
+          include_examples 'IncompatibleBindings error handling' do
+            let(:_build) { builder.build_app_lrp }
+          end
         end
       end
 
@@ -85,6 +103,10 @@ module Diego
             expect(lrp.volume_mounted_files.size).to eq(1)
             expect(lrp.volume_mounted_files[0].path).to eq('vcap_services')
           end
+
+          include_examples 'IncompatibleBindings error handling' do
+            let(:_build) { builder.build_app_lrp }
+          end
         end
       end
 

spec/unit/lib/cloud_controller/diego/task_recipe_builder_spec.rb

@@ -8,6 +8,53 @@ module Diego
       let(:space) { Space.make(name: 'MySpace', guid: 'space-guid', organization: org) }
       let(:app) { AppModel.make(name: 'MyApp', guid: 'banana-guid', space: space) }
 
+      shared_examples 'IncompatibleBindings error handling' do
+        let(:incompatible_bindings_error) { VCAP::CloudController::Diego::ServiceBindingFilesBuilder::IncompatibleBindings.new('something is wrong') }
+
+        context 'when an IncompatibleBindings error is raised' do
+          before do
+            allow_any_instance_of(VCAP::CloudController::Diego::ServiceBindingFilesBuilder).to receive(:build).and_raise(incompatible_bindings_error)
+          end
+
+          it 'results in an UnprocessableEntity error' do
+            expect { _build }.to raise_error(CloudController::Errors::ApiError, /Cannot build service binding files .* - something is wrong/)
+          end
+        end
+      end
+
+      shared_examples 'k8s service bindings' do
+        context 'when k8s service bindings are enabled' do
+          before do
+            _app.update(service_binding_k8s_enabled: true)
+            VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: _app.space), app: _app)
+          end
+
+          it 'includes volume mounted files' do
+            result = _build
+            expect(result.volume_mounted_files.size).to be > 1
+          end
+
+          include_examples 'IncompatibleBindings error handling'
+        end
+      end
+
+      shared_examples 'file-based VCAP service bindings' do
+        context 'when file-based VCAP service bindings are enabled' do
+          before do
+            _app.update(file_based_vcap_services_enabled: true)
+            VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: _app.space), app: _app)
+          end
+
+          it 'includes the vcap_services file' do
+            result = _build
+            expect(result.volume_mounted_files.size).to eq(1)
+            expect(result.volume_mounted_files[0].path).to eq('vcap_services')
+          end
+
+          include_examples 'IncompatibleBindings error handling'
+        end
+      end
+
       describe '#build_staging_task' do
         let(:staging_details) do
           Diego::StagingDetails.new.tap do |details|
@@ -201,31 +248,14 @@ module Diego
             end
           end
 
-          context 'when k8s service bindings are enabled' do
-            before do
-              app = staging_details.package.app
-              app.update(service_binding_k8s_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_staging_task(config, staging_details)
-              expect(result.volume_mounted_files.size).to be > 1
-            end
+          include_examples 'k8s service bindings' do
+            let(:_app) { staging_details.package.app }
+            let(:_build) { task_recipe_builder.build_staging_task(config, staging_details) }
           end
 
-          context 'when file-based VCAP service bindings are enabled' do
-            before do
-              app = staging_details.package.app
-              app.update(file_based_vcap_services_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_staging_task(config, staging_details)
-              expect(result.volume_mounted_files.size).to eq(1)
-              expect(result.volume_mounted_files[0].path).to eq('vcap_services')
-            end
+          include_examples 'file-based VCAP service bindings' do
+            let(:_app) { staging_details.package.app }
+            let(:_build) { task_recipe_builder.build_staging_task(config, staging_details) }
           end
         end
 
@@ -615,31 +645,14 @@ module Diego
             end
           end
 
-          context 'when k8s service bindings are enabled' do
-            before do
-              app = task.app
-              app.update(service_binding_k8s_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_app_task(config, task)
-              expect(result.volume_mounted_files.size).to be > 1
-            end
+          include_examples 'k8s service bindings' do
+            let(:_app) { task.app }
+            let(:_build) { task_recipe_builder.build_app_task(config, task) }
           end
 
-          context 'when file-based VCAP service bindings are enabled' do
-            before do
-              app = task.app
-              app.update(file_based_vcap_services_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_app_task(config, task)
-              expect(result.volume_mounted_files.size).to eq(1)
-              expect(result.volume_mounted_files[0].path).to eq('vcap_services')
-            end
+          include_examples 'file-based VCAP service bindings' do
+            let(:_app) { task.app }
+            let(:_build) { task_recipe_builder.build_app_task(config, task) }
           end
         end
 
@@ -797,31 +810,14 @@ module Diego
             end
           end
 
-          context 'when k8s service bindings are enabled' do
-            before do
-              app = task.app
-              app.update(service_binding_k8s_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_app_task(config, task)
-              expect(result.volume_mounted_files.size).to be > 1
-            end
+          include_examples 'k8s service bindings' do
+            let(:_app) { task.app }
+            let(:_build) { task_recipe_builder.build_app_task(config, task) }
           end
 
-          context 'when file-based VCAP service bindings are enabled' do
-            before do
-              app = task.app
-              app.update(file_based_vcap_services_enabled: true)
-              VCAP::CloudController::ServiceBinding.make(service_instance: ManagedServiceInstance.make(space: app.space), app: app)
-            end
-
-            it 'includes volume mounted files' do
-              result = task_recipe_builder.build_app_task(config, task)
-              expect(result.volume_mounted_files.size).to eq(1)
-              expect(result.volume_mounted_files[0].path).to eq('vcap_services')
-            end
+          include_examples 'file-based VCAP service bindings' do
+            let(:_app) { task.app }
+            let(:_build) { task_recipe_builder.build_app_task(config, task) }
           end
         end
       end