Update features from manifest

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

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,41 @@
 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
+
+        return if flags.empty?
+
+        check_invalid_combination!(app, flags)
+        app.update(flags)
+      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
+
+      def check_invalid_combination!(app, flags)
+        file_based_vcap_services_enabled = flags[database_column(AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE)].present? || app.file_based_vcap_services_enabled
+        service_binding_k8s_enabled = flags[database_column(AppFeatures::SERVICE_BINDING_K8S_FEATURE)].present? || 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."
+        raise InvalidCombination.new(msg)
       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

docs/v3/source/includes/resources/manifests/_apply.md

@@ -24,7 +24,7 @@ Location: https://api.example.org/v3/jobs/[guid]
 Apply changes specified in a manifest to the named apps and their underlying
 processes. The apps must reside in the space. These changes are additive
 and will not modify any unspecified properties or remove any existing
-environment variables, routes, or services.
+environment variables, app features, routes, or services.
 
 <aside class="notice">
 Apply manifest will only trigger an immediate update for the "instances" property or routing changes. All other properties require an app restart to take effect.

docs/v3/source/includes/resources/manifests/_get.md

@@ -22,6 +22,11 @@ Content-Type: application/x-yaml
 applications:
 - name: my-app
   stack: cflinuxfs4
+  features:
+    ssh: true
+    revisions: true
+    service-binding-k8s: false
+    file-based-vcap-services: false
   services:
   - my-service
   routes:

docs/v3/source/includes/resources/manifests/_object.md.erb

@@ -19,6 +19,11 @@ applications:
   env:
     VAR1: value1
     VAR2: value2
+  features:
+    ssh: true
+    revisions: true
+    service-binding-k8s: false
+    file-based-vcap-services: false
   routes:
   - route: route.example.com
   - route: another-route.example.com
@@ -94,6 +99,7 @@ Name | Type | Description
 **buildpacks** | _list of strings_ | a) An empty array, which will automatically select the appropriate default buildpack according to the coding language (incompatible with **lifecycle: cnb**) <br>b) An array of one or more URLs pointing to buildpacks <br>c) An array of one or more installed buildpack names <br>Replaces the legacy `buildpack` field
 **docker** | _object_ | If present, the created app will have Docker lifecycle type; the value of this key is ignored by the API but may be used by clients to source the registry address of the image and credentials, if needed; the [generate manifest endpoint](#generate-a-manifest-for-an-app) will return the registry address of the image and username provided with this key
 **env** | _object_ | A key-value mapping of environment variables to be used for the app when running
+**features** | _object_ | A key-value mapping of feature names to booleans (true = enabled, false = disabled)
 **no-route** | _boolean_ | When set to `true`, any routes specified with the `routes` attribute will be ignored and any existing routes will be removed
 **processes** | _array of [process configurations](#space-manifest-process-level-configuration)_ | List of configurations for individual process types
 **random-route** | _boolean_ | Creates a random route for the app if `true`; if `routes` is specified, if the app already has routes, or if `no-route` is specified, this field is ignored regardless of its value

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