File-based-service-bindings: improved errors

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