Add feature flags to v1 app manifests (#4358)

* Add feature flags to v1 app manifests (read-only)

- introduce AppFeatures helper containing constants and the mapping to
  database columns
- add FeaturesPresenter
- wire into AppManifestPresenter
- update request and unit tests for new 'features' hash

* 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 AppFeaturesController::SSH_FEATURE
-        app.update(enable_ssh: message.enabled)
-      when AppFeaturesController::REVISIONS_FEATURE
-        app.update(revisions_enabled: message.enabled)
-      when AppFeaturesController::SERVICE_BINDING_K8S_FEATURE
-        app.update(service_binding_k8s_enabled: message.enabled)
-      when AppFeaturesController::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({ feature_column_name(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[feature_column_name(feature_name)] = enabled
+        end
+
+        return if flags.empty?
+
+        check_invalid_combination!(app, flags)
+        app.update(flags)
+      end
+
+      private
+
+      def feature_column_name(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[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
+        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/controllers/v3/app_features_controller.rb

@@ -6,17 +6,14 @@
 require 'presenters/v3/app_file_based_vcap_services_feature_presenter'
 require 'presenters/v3/app_ssh_status_presenter'
 require 'actions/app_feature_update'
+require 'models/helpers/app_features'
 
 class AppFeaturesController < ApplicationController
   include AppSubResource
 
-  SSH_FEATURE = 'ssh'.freeze
-  REVISIONS_FEATURE = 'revisions'.freeze
-  SERVICE_BINDING_K8S_FEATURE = 'service-binding-k8s'.freeze
-  FILE_BASED_VCAP_SERVICES_FEATURE = 'file-based-vcap-services'.freeze
-
-  TRUSTED_APP_FEATURES = [SSH_FEATURE, SERVICE_BINDING_K8S_FEATURE, FILE_BASED_VCAP_SERVICES_FEATURE].freeze
-  UNTRUSTED_APP_FEATURES = [REVISIONS_FEATURE].freeze
+  TRUSTED_APP_FEATURES = [VCAP::CloudController::AppFeatures::SSH_FEATURE, VCAP::CloudController::AppFeatures::SERVICE_BINDING_K8S_FEATURE,
+                          VCAP::CloudController::AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE].freeze
+  UNTRUSTED_APP_FEATURES = [VCAP::CloudController::AppFeatures::REVISIONS_FEATURE].freeze
   APP_FEATURES = (TRUSTED_APP_FEATURES + UNTRUSTED_APP_FEATURES).freeze
 
   def index
@@ -56,11 +53,12 @@ def update
     unprocessable!(message.errors.full_messages) unless message.valid?
 
     if message.enabled && both_service_binding_features_enabled?(app, name)
-      unprocessable!("'#{FILE_BASED_VCAP_SERVICES_FEATURE}' and '#{SERVICE_BINDING_K8S_FEATURE}' features cannot be enabled at the same time.")
+      unprocessable!("'#{VCAP::CloudController::AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE}' and '#{VCAP::CloudController::AppFeatures::SERVICE_BINDING_K8S_FEATURE}' " \
+                     'features cannot be enabled at the same time.')
     end
 
-    AppFeatureUpdate.update(hashed_params[:name], app, message)
-    render status: :ok, json: feature_presenter_for(hashed_params[:name], app)
+    AppFeatureUpdate.update(name, app, message)
+    render status: :ok, json: feature_presenter_for(name, app)
   end
 
   def ssh_enabled
@@ -87,10 +85,10 @@ def present_unpagination_hash(result, path)
 
   def feature_presenter_for(feature_name, app)
     presenters = {
-      SSH_FEATURE => Presenters::V3::AppSshFeaturePresenter,
-      REVISIONS_FEATURE => Presenters::V3::AppRevisionsFeaturePresenter,
-      SERVICE_BINDING_K8S_FEATURE => Presenters::V3::AppServiceBindingK8sFeaturePresenter,
-      FILE_BASED_VCAP_SERVICES_FEATURE => Presenters::V3::AppFileBasedVcapServicesFeaturePresenter
+      VCAP::CloudController::AppFeatures::SSH_FEATURE => Presenters::V3::AppSshFeaturePresenter,
+      VCAP::CloudController::AppFeatures::REVISIONS_FEATURE => Presenters::V3::AppRevisionsFeaturePresenter,
+      VCAP::CloudController::AppFeatures::SERVICE_BINDING_K8S_FEATURE => Presenters::V3::AppServiceBindingK8sFeaturePresenter,
+      VCAP::CloudController::AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE => Presenters::V3::AppFileBasedVcapServicesFeaturePresenter
     }
     presenters[feature_name].new(app)
   end
@@ -105,9 +103,9 @@ def presented_app_features(app)
   end
 
   def both_service_binding_features_enabled?(app, feature_name)
-    if feature_name == FILE_BASED_VCAP_SERVICES_FEATURE
+    if feature_name == VCAP::CloudController::AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE
       app.service_binding_k8s_enabled
-    elsif feature_name == SERVICE_BINDING_K8S_FEATURE
+    elsif feature_name == VCAP::CloudController::AppFeatures::SERVICE_BINDING_K8S_FEATURE
       app.file_based_vcap_services_enabled
     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

app/models/helpers/app_features.rb

@@ -0,0 +1,19 @@
+module VCAP::CloudController
+  class AppFeatures
+    SSH_FEATURE = 'ssh'.freeze
+    REVISIONS_FEATURE = 'revisions'.freeze
+    SERVICE_BINDING_K8S_FEATURE = 'service-binding-k8s'.freeze
+    FILE_BASED_VCAP_SERVICES_FEATURE = 'file-based-vcap-services'.freeze
+
+    DATABASE_COLUMNS_MAPPING = {
+      SSH_FEATURE => :enable_ssh,
+      REVISIONS_FEATURE => :revisions_enabled,
+      SERVICE_BINDING_K8S_FEATURE => :service_binding_k8s_enabled,
+      FILE_BASED_VCAP_SERVICES_FEATURE => :file_based_vcap_services_enabled
+    }.freeze
+
+    def self.all_features
+      [SSH_FEATURE, REVISIONS_FEATURE, SERVICE_BINDING_K8S_FEATURE, FILE_BASED_VCAP_SERVICES_FEATURE]
+    end
+  end
+end

app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb

@@ -4,7 +4,7 @@ module VCAP::CloudController::Presenters::V3
   class AppFileBasedVcapServicesFeaturePresenter < BasePresenter
     def to_hash
       {
-        name: AppFeaturesController::FILE_BASED_VCAP_SERVICES_FEATURE,
+        name: VCAP::CloudController::AppFeatures::FILE_BASED_VCAP_SERVICES_FEATURE,
         description: 'Enable file-based VCAP service bindings for the app',
         enabled: app.file_based_vcap_services_enabled
       }

app/presenters/v3/app_manifest_presenter.rb

@@ -7,6 +7,7 @@
 require 'presenters/v3/app_manifest_presenters/process_properties_presenter'
 require 'presenters/v3/app_manifest_presenters/sidecar_properties_presenter'
 require 'presenters/v3/app_manifest_presenters/lifecycle_presenter'
+require 'presenters/v3/app_manifest_presenters/features_presenter'
 
 module VCAP::CloudController
   module Presenters
@@ -17,6 +18,7 @@ class AppManifestPresenter
           AppManifestPresenters::LifecyclePresenter.new,
           AppManifestPresenters::DockerPresenter.new,
           AppManifestPresenters::BuildpackPresenter.new,
+          AppManifestPresenters::FeaturesPresenter.new,
           AppManifestPresenters::ServicesPropertiesPresenter.new,
           AppManifestPresenters::RoutePropertiesPresenter.new,
           AppManifestPresenters::MetadataPresenter.new,