Update features from manifest

- add bulk_update method to AppFeatureUpdate
- add features to manifest message
- call AppFeatureUpdate from AppApplyManifest
- add features to SpaceDiffManifest

Author: philippthun

app/actions/app_apply_manifest.rb

@@ -28,6 +28,8 @@ def apply(app_guid, message)
       app = AppModel.first(guid: app_guid)
       app_instance_not_found!(app_guid) unless app
 
+      AppFeatureUpdate.bulk_update(app, message.manifest_features_update_message)
+
       message.manifest_process_update_messages.each do |manifest_process_update_msg|
         if manifest_process_update_msg.requested_keys == [:type] && manifest_process_update_msg.type == 'web'
           # ignore trivial messages, most likely from manifest

app/actions/app_feature_update.rb

@@ -1,15 +1,34 @@
 module VCAP::CloudController
   class AppFeatureUpdate
-    def self.update(feature_name, app, message)
-      case feature_name
-      when AppFeatures::SSH_FEATURE
-        app.update(enable_ssh: message.enabled)
-      when AppFeatures::REVISIONS_FEATURE
-        app.update(revisions_enabled: message.enabled)
-      when AppFeatures::SERVICE_BINDING_K8S_FEATURE
-        app.update(service_binding_k8s_enabled: message.enabled)
-      when AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE
-        app.update(file_based_vcap_services_enabled: message.enabled)
+    class InvalidCombination < StandardError; end
+
+    class << self
+      def update(feature_name, app, app_feature_update_message)
+        app.update({ database_column(feature_name) => app_feature_update_message.enabled })
+      end
+
+      def bulk_update(app, manifest_features_update_message)
+        flags = {}
+
+        manifest_features_update_message.features&.each do |feature_name, enabled|
+          flags[database_column(feature_name)] = enabled
+        end
+
+        app.update(flags) unless flags.empty?
+      rescue Sequel::CheckConstraintViolation => e
+        raise e unless e.message.include?('only_one_sb_feature_enabled')
+
+        msg = "'#{AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE}' and '#{AppFeatures::SERVICE_BINDING_K8S_FEATURE}' features cannot be enabled at the same time."
+        raise InvalidCombination.new(msg)
+      end
+
+      private
+
+      def database_column(feature_name)
+        column = AppFeatures::DATABASE_COLUMNS_MAPPING[feature_name.to_s]
+        raise "Unknown feature name: #{feature_name}" if column.nil?
+
+        column
       end
     end
   end

app/actions/space_diff_manifest.rb

@@ -51,6 +51,8 @@ def generate_diff(app_manifests, space)
 
             remove_default_missing_fields(existing_value, key, value) if nested_attribute_exists
 
+            remove_unspecified_app_features(existing_value, key, value)
+
             # To preserve backwards compability, we've decided to skip diffs that satisfy this conditon
             next if !nested_attribute_exists && %w[disk_quota disk-quota memory].include?(key)
 
@@ -211,6 +213,11 @@ def remove_default_missing_fields(existing_value, current_key, value)
           end
         end
       end
+
+      def remove_unspecified_app_features(existing_value, key, value)
+        # Only show the diff for features that are specified in the manifest
+        existing_value.slice!(*value.keys) if key == 'features'
+      end
     end
   end
 end

app/jobs/space_apply_manifest_action_job.rb

@@ -26,7 +26,8 @@ def perform
                AppApplyManifest::ServiceBindingError,
                SidecarCreate::InvalidSidecar,
                SidecarUpdate::InvalidSidecar,
-               ProcessScale::SidecarMemoryLessThanProcessMemory => e
+               ProcessScale::SidecarMemoryLessThanProcessMemory,
+               AppFeatureUpdate::InvalidCombination => e
 
           app_name = AppModel.find(guid: app_guid)&.name
           error_message = app_name ? "For application '#{app_name}': #{e.message}" : e.message

app/messages/app_manifest_message.rb

@@ -4,6 +4,7 @@
 require 'messages/manifest_buildpack_message'
 require 'messages/manifest_service_binding_create_message'
 require 'messages/manifest_routes_update_message'
+require 'messages/manifest_features_update_message'
 require 'messages/validators/metadata_validator'
 require 'cloud_controller/app_manifest/byte_converter'
 require 'models/helpers/health_check_types'
@@ -43,6 +44,7 @@ class AppManifestMessage < BaseMessage
       stack
       timeout
       cnb_credentials
+      features
     ]
 
     HEALTH_CHECK_TYPE_MAPPING = { HealthCheckTypes::NONE => HealthCheckTypes::PROCESS }.freeze
@@ -82,6 +84,7 @@ def self.underscore_keys(hash)
         record.requested?(:random_route) ||
         record.requested?(:default_route)
     }
+    validate :validate_features!, if: ->(record) { record.requested?(:features) }
 
     def initialize(original_yaml, attrs={})
       super(attrs)
@@ -116,6 +119,10 @@ def manifest_routes_update_message
       @manifest_routes_update_message ||= ManifestRoutesUpdateMessage.new(routes_attribute_mapping)
     end
 
+    def manifest_features_update_message
+      @manifest_features_update_message ||= ManifestFeaturesUpdateMessage.new(features_attribute_mapping)
+    end
+
     def audit_hash
       override_env = original_yaml['env'] ? { 'env' => Presenters::Censorship::PRIVATE_DATA_HIDDEN } : {}
       override_cnb = original_yaml['cnb-credentials'] ? { 'cnb-credentials' => Presenters::Censorship::PRIVATE_DATA_HIDDEN } : {}
@@ -295,6 +302,12 @@ def service_bindings_attribute_mapping
       mapping
     end
 
+    def features_attribute_mapping
+      mapping = {}
+      mapping[:features] = features if requested?(:features)
+      mapping
+    end
+
     def should_autodetect?(buildpack)
       buildpack == 'default' || buildpack == 'null' || buildpack.nil?
     end
@@ -464,6 +477,13 @@ def validate_docker_buildpacks_combination!
       errors.add(:base, 'Cannot specify both buildpack(s) and docker keys')
     end
 
+    def validate_features!
+      manifest_features_update_message.valid?
+      manifest_features_update_message.errors.full_messages.each do |error_message|
+        errors.add(:base, error_message)
+      end
+    end
+
     def add_process_error!(error_message, type)
       errors.add(:base, %(Process "#{type}": #{error_message}))
     end

app/messages/manifest_features_update_message.rb

@@ -0,0 +1,25 @@
+require 'messages/base_message'
+
+module VCAP::CloudController
+  class ManifestFeaturesUpdateMessage < BaseMessage
+    register_allowed_keys [:features]
+
+    validates_with NoAdditionalKeysValidator
+
+    validate :features do
+      errors.add(:features, 'must be a map of valid feature names to booleans (true = enabled, false = disabled)') unless is_valid(features)
+    end
+
+    private
+
+    def is_valid(features)
+      return false unless features.is_a?(Hash) && features.any?
+
+      features.all? { |feature, enabled| valid_feature?(feature) && [true, false].include?(enabled) }
+    end
+
+    def valid_feature?(feature)
+      AppFeatures.all_features.include?(feature.to_s)
+    end
+  end
+end

spec/unit/actions/app_apply_manifest_spec.rb

@@ -14,6 +14,7 @@ module VCAP::CloudController
       let(:process_create) { instance_double(ProcessCreate) }
       let(:service_cred_binding_create) { instance_double(V3::ServiceCredentialBindingAppCreate) }
       let(:random_route_generator) { instance_double(RandomRouteGenerator, route: 'spiffy/donut') }
+      let(:app_feature_update) { instance_double(AppFeatureUpdate) }
 
       describe '#apply' do
         before do
@@ -52,6 +53,8 @@ module VCAP::CloudController
           allow(AppPatchEnvironmentVariables).
             to receive(:new).and_return(app_patch_env)
           allow(app_patch_env).to receive(:patch)
+
+          allow(AppFeatureUpdate).to receive(:bulk_update)
         end
 
         describe 'scaling a process' do
@@ -1387,6 +1390,23 @@ module VCAP::CloudController
             end.to raise_error(CloudController::Errors::NotFound, "App with guid '#{app_guid}' not found")
           end
         end
+
+        describe 'updating app features' do
+          let(:message) { AppManifestMessage.create_from_yml({ features: { ssh: true } }) }
+          let(:manifest_features_update_message) { message.manifest_features_update_message }
+          let(:app) { AppModel.make }
+
+          it 'returns the app' do
+            expect(
+              app_apply_manifest.apply(app.guid, message)
+            ).to eq(app)
+          end
+
+          it 'calls AppFeatureUpdate.bulk_update with the correct arguments' do
+            app_apply_manifest.apply(app.guid, message)
+            expect(AppFeatureUpdate).to have_received(:bulk_update).with(app, manifest_features_update_message)
+          end
+        end
       end
     end
 

spec/unit/actions/app_feature_update_spec.rb

@@ -5,10 +5,11 @@
 module VCAP::CloudController
   RSpec.describe AppFeatureUpdate do
     subject(:app_feature_update) { AppFeatureUpdate }
-    let(:app) { AppModel.make(enable_ssh: false, revisions_enabled: false) }
-    let(:message) { AppFeatureUpdateMessage.new(enabled: true) }
+    let(:app) { AppModel.make(enable_ssh: false, revisions_enabled: false, service_binding_k8s_enabled: false, file_based_vcap_services_enabled: false) }
 
     describe '.update' do
+      let(:message) { AppFeatureUpdateMessage.new(enabled: true) }
+
       context 'when the feature name is ssh' do
         it 'updates the enable_ssh column on the app' do
           expect do
@@ -41,5 +42,98 @@ module VCAP::CloudController
         end
       end
     end
+
+    describe '.bulk_update' do
+      let(:features) do
+        {
+          ssh: false,
+          revisions: false,
+          'service-binding-k8s': false,
+          'file-based-vcap-services': false
+        }
+      end
+      let(:message) { ManifestFeaturesUpdateMessage.new(features:) }
+
+      context 'when the ssh feature is specified' do
+        before do
+          features[:ssh] = true
+        end
+
+        it 'updates the enable_ssh column on the app' do
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to change { app.reload.enable_ssh }.to(true)
+        end
+      end
+
+      context 'when the revisions feature is specified' do
+        before do
+          features[:revisions] = true
+        end
+
+        it 'updates the revisions_enabled column on the app' do
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to change { app.reload.revisions_enabled }.to(true)
+        end
+      end
+
+      context 'when the service-binding-k8s feature is specified' do
+        before do
+          features[:'service-binding-k8s'] = true
+        end
+
+        it 'updates the service_binding_k8s_enabled column on the app' do
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to change { app.reload.service_binding_k8s_enabled }.to(true)
+        end
+      end
+
+      context 'when the file-based-vcap-services feature is specified' do
+        before do
+          features[:'file-based-vcap-services'] = true
+        end
+
+        it 'updates the file_based_vcap_services_enabled column on the app' do
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to change { app.reload.file_based_vcap_services_enabled }.to(true)
+        end
+      end
+
+      context 'when multiple features are specified' do
+        before do
+          features[:ssh] = true
+          features[:'service-binding-k8s'] = true
+        end
+
+        it 'updates the corresponding columns in a single database call' do
+          expect(app.enable_ssh).to be(false)
+          expect(app.service_binding_k8s_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.enable_ssh).to be(true)
+          expect(app.service_binding_k8s_enabled).to be(true)
+        end
+      end
+
+      context 'when conflicting features are specified' do
+        before do
+          features[:'service-binding-k8s'] = true
+          features[:'file-based-vcap-services'] = true
+        end
+
+        it 'raises an error' do
+          expect do
+            AppFeatureUpdate.bulk_update(app, message)
+          end.to raise_error(AppFeatureUpdate::InvalidCombination, /'file-based-vcap-services' and 'service-binding-k8s' features cannot be enabled at the same time/)
+        end
+      end
+    end
   end
 end

spec/unit/actions/space_diff_manifest_spec.rb

@@ -503,6 +503,28 @@ module VCAP::CloudController
           end
         end
       end
+
+      context 'when the given manifest contains features' do
+        before do
+          default_manifest['applications'][0]['features'] = {
+            'ssh' => true,                 # unchanged
+            'service-binding-k8s' => true  # changed
+          }
+        end
+
+        it 'does not show unchanged features' do
+          expect(subject).not_to include(hash_including('path' => '/applications/0/features/ssh'))
+        end
+
+        it 'shows changed features' do
+          expect(subject).to include({ 'op' => 'replace', 'path' => '/applications/0/features/service-binding-k8s', 'value' => true, 'was' => false })
+        end
+
+        it 'does not show unspecified features' do
+          expect(subject).not_to include(hash_including('path' => '/applications/0/features/revisions'))
+          expect(subject).not_to include(hash_including('path' => '/applications/0/features/file-based-vcap-services'))
+        end
+      end
     end
   end
 end

spec/unit/jobs/space_apply_manifest_action_job_spec.rb

@@ -58,7 +58,8 @@ module Jobs
         AppApplyManifest::ServiceBindingError,
         SidecarCreate::InvalidSidecar,
         SidecarUpdate::InvalidSidecar,
-        ProcessScale::SidecarMemoryLessThanProcessMemory
+        ProcessScale::SidecarMemoryLessThanProcessMemory,
+        AppFeatureUpdate::InvalidCombination
       ].each do |klass|
         it "wraps a #{klass} in an ApiError" do
           allow(apply_manifest_action).to receive(:apply).