From 1ca9162b8fa31bcffd3a65cae278f2743ee51ebb Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Tue, 25 Feb 2025 14:37:13 +0100 Subject: [PATCH 1/7] Add column 'service_binding_k8s_enabled' to apps table --- ...apps_service_binding_k8s_enabled_column.rb | 5 +++ ...service_binding_k8s_enabled_column_spec.rb | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb create mode 100644 spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb diff --git a/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb b/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb new file mode 100644 index 00000000000..4070e6226c4 --- /dev/null +++ b/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb @@ -0,0 +1,5 @@ +Sequel.migration do + change do + add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false + end +end diff --git a/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb b/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb new file mode 100644 index 00000000000..bcb472b2ab4 --- /dev/null +++ b/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'migration to add service_binding_k8s_enabled column to apps table', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20250225132929_add_apps_service_binding_k8s_enabled_column.rb' } + end + + describe 'apps table' do + subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } + + it 'adds a column `service_binding_k8s_enabled`' do + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + run_migration + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + end + + it 'sets the default value of existing entries to false' do + db[:apps].insert(guid: 'existing_app_guid') + run_migration + expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) + end + + it 'sets the default value of new entries to false' do + run_migration + db[:apps].insert(guid: 'new_app_guid') + expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) + end + + it 'forbids null values' do + run_migration + expect { db[:apps].insert(guid: 'app_guid__nil', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + end + end +end From 4c308e2b3294b1684bc2cc57f06d951e76b073fe Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Tue, 25 Feb 2025 15:22:33 +0100 Subject: [PATCH 2/7] Rename 'file-based-service-bindings' feature to 'service-binding-k8s' --- app/actions/app_feature_update.rb | 4 ++-- app/controllers/v3/app_features_controller.rb | 10 ++++----- ...ased_service_bindings_feature_presenter.rb | 19 ---------------- ...p_service_binding_k8s_feature_presenter.rb | 19 ++++++++++++++++ .../includes/api_resources/_app_features.erb | 4 ++-- .../app_features/_supported_features.md.erb | 2 +- spec/request/app_features_spec.rb | 22 +++++++++---------- spec/unit/actions/app_feature_update_spec.rb | 8 +++---- .../v3/app_features_controller_spec.rb | 14 ++++++------ .../v3/app_feature_presenter_spec.rb | 12 +++++----- 10 files changed, 57 insertions(+), 57 deletions(-) delete mode 100644 app/presenters/v3/app_file_based_service_bindings_feature_presenter.rb create mode 100644 app/presenters/v3/app_service_binding_k8s_feature_presenter.rb diff --git a/app/actions/app_feature_update.rb b/app/actions/app_feature_update.rb index 1fc865f78b0..e6e9c9ced0b 100644 --- a/app/actions/app_feature_update.rb +++ b/app/actions/app_feature_update.rb @@ -6,8 +6,8 @@ def self.update(feature_name, app, message) app.update(enable_ssh: message.enabled) when 'revisions' app.update(revisions_enabled: message.enabled) - when 'file-based-service-bindings' - app.update(file_based_service_bindings_enabled: message.enabled) + when 'service-binding-k8s' + app.update(service_binding_k8s_enabled: message.enabled) end end end diff --git a/app/controllers/v3/app_features_controller.rb b/app/controllers/v3/app_features_controller.rb index 7359e524f76..2cc294838fe 100644 --- a/app/controllers/v3/app_features_controller.rb +++ b/app/controllers/v3/app_features_controller.rb @@ -2,7 +2,7 @@ require 'controllers/v3/mixins/app_sub_resource' require 'presenters/v3/app_ssh_feature_presenter' require 'presenters/v3/app_revisions_feature_presenter' -require 'presenters/v3/app_file_based_service_bindings_feature_presenter' +require 'presenters/v3/app_service_binding_k8s_feature_presenter' require 'presenters/v3/app_ssh_status_presenter' require 'actions/app_feature_update' @@ -11,9 +11,9 @@ class AppFeaturesController < ApplicationController SSH_FEATURE = 'ssh'.freeze REVISIONS_FEATURE = 'revisions'.freeze - FILE_BASED_SERVICE_BINDINGS_FEATURE = 'file-based-service-bindings'.freeze + SERVICE_BINDING_K8S_FEATURE = 'service-binding-k8s'.freeze - TRUSTED_APP_FEATURES = [SSH_FEATURE, FILE_BASED_SERVICE_BINDINGS_FEATURE].freeze + TRUSTED_APP_FEATURES = [SSH_FEATURE, SERVICE_BINDING_K8S_FEATURE].freeze UNTRUSTED_APP_FEATURES = [REVISIONS_FEATURE].freeze APP_FEATURES = (TRUSTED_APP_FEATURES + UNTRUSTED_APP_FEATURES).freeze @@ -83,7 +83,7 @@ def feature_presenter_for(feature_name, app) presenters = { SSH_FEATURE => Presenters::V3::AppSshFeaturePresenter, REVISIONS_FEATURE => Presenters::V3::AppRevisionsFeaturePresenter, - FILE_BASED_SERVICE_BINDINGS_FEATURE => Presenters::V3::AppFileBasedServiceBindingsFeaturePresenter + SERVICE_BINDING_K8S_FEATURE => Presenters::V3::AppServiceBindingK8sFeaturePresenter } presenters[feature_name].new(app) end @@ -92,7 +92,7 @@ def presented_app_features(app) [ Presenters::V3::AppSshFeaturePresenter.new(app), Presenters::V3::AppRevisionsFeaturePresenter.new(app), - Presenters::V3::AppFileBasedServiceBindingsFeaturePresenter.new(app) + Presenters::V3::AppServiceBindingK8sFeaturePresenter.new(app) ] end end diff --git a/app/presenters/v3/app_file_based_service_bindings_feature_presenter.rb b/app/presenters/v3/app_file_based_service_bindings_feature_presenter.rb deleted file mode 100644 index fbd5928c7a3..00000000000 --- a/app/presenters/v3/app_file_based_service_bindings_feature_presenter.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'presenters/v3/base_presenter' - -module VCAP::CloudController::Presenters::V3 - class AppFileBasedServiceBindingsFeaturePresenter < BasePresenter - def to_hash - { - name: 'file-based-service-bindings', - description: 'Enable file-based service bindings for the app', - enabled: app.file_based_service_bindings_enabled - } - end - - private - - def app - @resource - end - end -end diff --git a/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb b/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb new file mode 100644 index 00000000000..11f0425964a --- /dev/null +++ b/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb @@ -0,0 +1,19 @@ +require 'presenters/v3/base_presenter' + +module VCAP::CloudController::Presenters::V3 + class AppServiceBindingK8sFeaturePresenter < BasePresenter + def to_hash + { + name: 'service-binding-k8s', + description: 'Enable k8s service bindings for the app', + enabled: app.service_binding_k8s_enabled + } + end + + private + + def app + @resource + end + end +end diff --git a/docs/v3/source/includes/api_resources/_app_features.erb b/docs/v3/source/includes/api_resources/_app_features.erb index f947f9c0a6c..04846a8e24e 100644 --- a/docs/v3/source/includes/api_resources/_app_features.erb +++ b/docs/v3/source/includes/api_resources/_app_features.erb @@ -20,8 +20,8 @@ "enabled": false }, { - "name": "file-based-service-bindings", - "description": "Enable file-based service bindings for the app", + "name": "service-binding-k8s", + "description": "Enable k8s service bindings for the app", "enabled": false } ], diff --git a/docs/v3/source/includes/resources/app_features/_supported_features.md.erb b/docs/v3/source/includes/resources/app_features/_supported_features.md.erb index 7f8bfede853..7014410d5dc 100644 --- a/docs/v3/source/includes/resources/app_features/_supported_features.md.erb +++ b/docs/v3/source/includes/resources/app_features/_supported_features.md.erb @@ -6,4 +6,4 @@ Name | Description ---- | ----------- **ssh** | Enable SSHing into the app **revisions** | Enable [versioning](#revisions) of an application -**file-based-service-bindings** | Enable file-based service bindings for the app (experimental) +**service-binding-k8s** | Enable k8s service bindings for the app (experimental) diff --git a/spec/request/app_features_spec.rb b/spec/request/app_features_spec.rb index 01abb54e15c..0131cce1eb3 100644 --- a/spec/request/app_features_spec.rb +++ b/spec/request/app_features_spec.rb @@ -7,7 +7,7 @@ let(:admin_header) { admin_headers_for(user) } let(:org) { VCAP::CloudController::Organization.make(created_at: 3.days.ago) } let(:space) { VCAP::CloudController::Space.make(organization: org) } - let(:app_model) { VCAP::CloudController::AppModel.make(space: space, enable_ssh: true, file_based_service_bindings_enabled: true) } + let(:app_model) { VCAP::CloudController::AppModel.make(space: space, enable_ssh: true, service_binding_k8s_enabled: true) } describe 'GET /v3/apps/:guid/features' do context 'getting a list of available features for the app' do @@ -26,8 +26,8 @@ 'enabled' => true }, { - 'name' => 'file-based-service-bindings', - 'description' => 'Enable file-based service bindings for the app', + 'name' => 'service-binding-k8s', + 'description' => 'Enable k8s service bindings for the app', 'enabled' => true } ], @@ -100,12 +100,12 @@ it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS end - context 'file-based-service-bindings app feature' do - let(:api_call) { ->(user_headers) { get "/v3/apps/#{app_model.guid}/features/file-based-service-bindings", nil, user_headers } } + context 'service-binding-k8s app feature' do + let(:api_call) { ->(user_headers) { get "/v3/apps/#{app_model.guid}/features/service-binding-k8s", nil, user_headers } } let(:feature_response_object) do { - 'name' => 'file-based-service-bindings', - 'description' => 'Enable file-based service bindings for the app', + 'name' => 'service-binding-k8s', + 'description' => 'Enable k8s service bindings for the app', 'enabled' => true } end @@ -191,12 +191,12 @@ end end - context 'file-based-service-bindings app feature' do - let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/file-based-service-bindings", request_body.to_json, user_headers } } + context 'service-binding-k8s app feature' do + let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body.to_json, user_headers } } let(:feature_response_object) do { - 'name' => 'file-based-service-bindings', - 'description' => 'Enable file-based service bindings for the app', + 'name' => 'service-binding-k8s', + 'description' => 'Enable k8s service bindings for the app', 'enabled' => false } end diff --git a/spec/unit/actions/app_feature_update_spec.rb b/spec/unit/actions/app_feature_update_spec.rb index 071c00287a9..2920b7636ba 100644 --- a/spec/unit/actions/app_feature_update_spec.rb +++ b/spec/unit/actions/app_feature_update_spec.rb @@ -25,11 +25,11 @@ module VCAP::CloudController end end - context 'when the feature name is file-based-service-bindings' do - it 'updates the file_based_service_bindings_enabled column on the app' do + context 'when the feature name is service-binding-k8s' do + it 'updates the service_binding_k8s_enabled column on the app' do expect do - AppFeatureUpdate.update('file-based-service-bindings', app, message) - end.to change { app.reload.file_based_service_bindings_enabled }.to(true) + AppFeatureUpdate.update('service-binding-k8s', app, message) + end.to change { app.reload.service_binding_k8s_enabled }.to(true) end end end diff --git a/spec/unit/controllers/v3/app_features_controller_spec.rb b/spec/unit/controllers/v3/app_features_controller_spec.rb index f1317edcb39..f0780acc43e 100644 --- a/spec/unit/controllers/v3/app_features_controller_spec.rb +++ b/spec/unit/controllers/v3/app_features_controller_spec.rb @@ -4,14 +4,14 @@ ## NOTICE: Prefer request specs over controller specs as per ADR #0003 ## RSpec.describe AppFeaturesController, type: :controller do - let(:app_model) { VCAP::CloudController::AppModel.make(enable_ssh: true, file_based_service_bindings_enabled: true) } + let(:app_model) { VCAP::CloudController::AppModel.make(enable_ssh: true, service_binding_k8s_enabled: true) } let(:space) { app_model.space } let(:org) { space.organization } let(:user) { VCAP::CloudController::User.make } let(:app_feature_ssh_response) { { 'name' => 'ssh', 'description' => 'Enable SSHing into the app.', 'enabled' => true } } let(:app_feature_revisions_response) { { 'name' => 'revisions', 'description' => 'Enable versioning of an application', 'enabled' => true } } - let(:app_feature_file_based_service_bindings_response) do - { 'name' => 'file-based-service-bindings', 'description' => 'Enable file-based service bindings for the app', 'enabled' => true } + let(:app_feature_service_binding_k8s_response) do + { 'name' => 'service-binding-k8s', 'description' => 'Enable k8s service bindings for the app', 'enabled' => true } end before do @@ -42,7 +42,7 @@ it 'returns app features' do get :index, params: { app_guid: app_model.guid } expect(parsed_body).to eq( - 'resources' => [app_feature_ssh_response, app_feature_revisions_response, app_feature_file_based_service_bindings_response], + 'resources' => [app_feature_ssh_response, app_feature_revisions_response, app_feature_service_binding_k8s_response], 'pagination' => pagination_hash ) end @@ -70,9 +70,9 @@ expect(parsed_body).to eq(app_feature_revisions_response) end - it 'returns the file-based-service-bindings app feature' do - get :show, params: { app_guid: app_model.guid, name: 'file-based-service-bindings' } - expect(parsed_body).to eq(app_feature_file_based_service_bindings_response) + it 'returns the service-binding-k8s app feature' do + get :show, params: { app_guid: app_model.guid, name: 'service-binding-k8s' } + expect(parsed_body).to eq(app_feature_service_binding_k8s_response) end it 'throws 404 for a non-existent feature' do diff --git a/spec/unit/presenters/v3/app_feature_presenter_spec.rb b/spec/unit/presenters/v3/app_feature_presenter_spec.rb index c720a02f9de..afc4822a787 100644 --- a/spec/unit/presenters/v3/app_feature_presenter_spec.rb +++ b/spec/unit/presenters/v3/app_feature_presenter_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' require 'presenters/v3/app_ssh_feature_presenter' -require 'presenters/v3/app_file_based_service_bindings_feature_presenter' +require 'presenters/v3/app_service_binding_k8s_feature_presenter' module VCAP::CloudController::Presenters::V3 RSpec.describe AppSshFeaturePresenter do @@ -16,15 +16,15 @@ module VCAP::CloudController::Presenters::V3 end end - RSpec.describe AppFileBasedServiceBindingsFeaturePresenter do + RSpec.describe AppServiceBindingK8sFeaturePresenter do let(:app) { VCAP::CloudController::AppModel.make } describe '#to_hash' do it 'presents the app feature as json' do - result = AppFileBasedServiceBindingsFeaturePresenter.new(app).to_hash - expect(result[:name]).to eq('file-based-service-bindings') - expect(result[:description]).to eq('Enable file-based service bindings for the app') - expect(result[:enabled]).to eq(app.file_based_service_bindings_enabled) + result = AppServiceBindingK8sFeaturePresenter.new(app).to_hash + expect(result[:name]).to eq('service-binding-k8s') + expect(result[:description]).to eq('Enable k8s service bindings for the app') + expect(result[:enabled]).to eq(app.service_binding_k8s_enabled) end end end From 15421b37c362dca725b66a8b597037c49c06edd9 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Thu, 27 Feb 2025 12:39:24 +0100 Subject: [PATCH 3/7] Introduce 'file-based-vcap-services' app feature --- app/actions/app_feature_update.rb | 2 + app/controllers/v3/app_features_controller.rb | 22 ++- ...e_based_vcap_services_feature_presenter.rb | 19 ++ ...apps_service_binding_k8s_enabled_column.rb | 18 +- .../includes/api_resources/_app_features.erb | 7 +- .../app_features/_supported_features.md.erb | 1 + spec/request/app_features_spec.rb | 180 ++++++++++++++++-- spec/unit/actions/app_feature_update_spec.rb | 10 +- .../v3/app_features_controller_spec.rb | 12 +- .../v3/app_feature_presenter_spec.rb | 14 ++ 10 files changed, 256 insertions(+), 29 deletions(-) create mode 100644 app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb diff --git a/app/actions/app_feature_update.rb b/app/actions/app_feature_update.rb index e6e9c9ced0b..022e7504025 100644 --- a/app/actions/app_feature_update.rb +++ b/app/actions/app_feature_update.rb @@ -8,6 +8,8 @@ def self.update(feature_name, app, message) app.update(revisions_enabled: message.enabled) when 'service-binding-k8s' app.update(service_binding_k8s_enabled: message.enabled) + when 'file-based-vcap-services' + app.update(file_based_vcap_services_enabled: message.enabled) end end end diff --git a/app/controllers/v3/app_features_controller.rb b/app/controllers/v3/app_features_controller.rb index 2cc294838fe..19ccdb5b3ef 100644 --- a/app/controllers/v3/app_features_controller.rb +++ b/app/controllers/v3/app_features_controller.rb @@ -3,6 +3,7 @@ require 'presenters/v3/app_ssh_feature_presenter' require 'presenters/v3/app_revisions_feature_presenter' require 'presenters/v3/app_service_binding_k8s_feature_presenter' +require 'presenters/v3/app_file_based_vcap_services_feature_presenter' require 'presenters/v3/app_ssh_status_presenter' require 'actions/app_feature_update' @@ -12,8 +13,9 @@ class AppFeaturesController < ApplicationController 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].freeze + TRUSTED_APP_FEATURES = [SSH_FEATURE, SERVICE_BINDING_K8S_FEATURE, FILE_BASED_VCAP_SERVICES_FEATURE].freeze UNTRUSTED_APP_FEATURES = [REVISIONS_FEATURE].freeze APP_FEATURES = (TRUSTED_APP_FEATURES + UNTRUSTED_APP_FEATURES).freeze @@ -53,6 +55,10 @@ def update message = VCAP::CloudController::AppFeatureUpdateMessage.new(hashed_params['body']) unprocessable!(message.errors.full_messages) unless message.valid? + if message.enabled && both_service_binding_features_enabled?(app, name) + unprocessable!("'file-based-vcap-services' and 'service-binding-k8s' 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) end @@ -83,7 +89,8 @@ 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 + SERVICE_BINDING_K8S_FEATURE => Presenters::V3::AppServiceBindingK8sFeaturePresenter, + FILE_BASED_VCAP_SERVICES_FEATURE => Presenters::V3::AppFileBasedVcapServicesFeaturePresenter } presenters[feature_name].new(app) end @@ -92,7 +99,16 @@ def presented_app_features(app) [ Presenters::V3::AppSshFeaturePresenter.new(app), Presenters::V3::AppRevisionsFeaturePresenter.new(app), - Presenters::V3::AppServiceBindingK8sFeaturePresenter.new(app) + Presenters::V3::AppServiceBindingK8sFeaturePresenter.new(app), + Presenters::V3::AppFileBasedVcapServicesFeaturePresenter.new(app) ] end + + def both_service_binding_features_enabled?(app, feature_name) + if feature_name == 'file-based-vcap-services' + app.service_binding_k8s_enabled + elsif feature_name == 'service-binding-k8s' + app.file_based_vcap_services_enabled + end + end end diff --git a/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb b/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb new file mode 100644 index 00000000000..456a118ce50 --- /dev/null +++ b/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb @@ -0,0 +1,19 @@ +require 'presenters/v3/base_presenter' + +module VCAP::CloudController::Presenters::V3 + class AppFileBasedVcapServicesFeaturePresenter < BasePresenter + def to_hash + { + name: 'file-based-vcap-services', + description: 'Enable file-based VCAP service bindings for the app', + enabled: app.file_based_vcap_services_enabled + } + end + + private + + def app + @resource + end + end +end diff --git a/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb b/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb index 4070e6226c4..3d527c7a84d 100644 --- a/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb +++ b/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb @@ -1,5 +1,19 @@ Sequel.migration do - change do - add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false + up do + alter_table :apps do + add_column :service_binding_k8s_enabled, :boolean, default: false, null: false + add_column :file_based_vcap_services_enabled, :boolean, default: false, null: false + add_constraint(name: :only_one_sb_feature_enabled) do + Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') + end + end + end + + down do + alter_table :apps do + drop_column :service_binding_k8s_enabled + drop_column :file_based_vcap_services_enabled + drop_constraint(name: :only_one_sb_feature_enabled) + end end end diff --git a/docs/v3/source/includes/api_resources/_app_features.erb b/docs/v3/source/includes/api_resources/_app_features.erb index 04846a8e24e..19704a22ac1 100644 --- a/docs/v3/source/includes/api_resources/_app_features.erb +++ b/docs/v3/source/includes/api_resources/_app_features.erb @@ -23,10 +23,15 @@ "name": "service-binding-k8s", "description": "Enable k8s service bindings for the app", "enabled": false + }, + { + "name": "file-based-vcap-services", + "description": "Enable file-based VCAP service bindings for the app", + "enabled": false } ], "pagination": { - "total_results": 3, + "total_results": 4, "total_pages": 1, "first": { "href": "/v3/apps/05d39de4-2c9e-4c76-8fd6-10417da07e42/features" }, "last": { "href": "/v3/apps/05d39de4-2c9e-4c76-8fd6-10417da07e42/features" }, diff --git a/docs/v3/source/includes/resources/app_features/_supported_features.md.erb b/docs/v3/source/includes/resources/app_features/_supported_features.md.erb index 7014410d5dc..32ca219495f 100644 --- a/docs/v3/source/includes/resources/app_features/_supported_features.md.erb +++ b/docs/v3/source/includes/resources/app_features/_supported_features.md.erb @@ -7,3 +7,4 @@ Name | Description **ssh** | Enable SSHing into the app **revisions** | Enable [versioning](#revisions) of an application **service-binding-k8s** | Enable k8s service bindings for the app (experimental) +**file-based-vcap-services** | Enable file-based VCAP service bindings for the app (experimental) diff --git a/spec/request/app_features_spec.rb b/spec/request/app_features_spec.rb index 0131cce1eb3..e475dade2ef 100644 --- a/spec/request/app_features_spec.rb +++ b/spec/request/app_features_spec.rb @@ -7,7 +7,17 @@ let(:admin_header) { admin_headers_for(user) } let(:org) { VCAP::CloudController::Organization.make(created_at: 3.days.ago) } let(:space) { VCAP::CloudController::Space.make(organization: org) } - let(:app_model) { VCAP::CloudController::AppModel.make(space: space, enable_ssh: true, service_binding_k8s_enabled: true) } + let(:service_binding_k8s_enabled) { true } + let(:file_based_vcap_services_enabled) { false } + let(:request_body_enabled) { { body: { enabled: true } } } + let(:app_model) do + VCAP::CloudController::AppModel.make( + space: space, + enable_ssh: true, + service_binding_k8s_enabled: service_binding_k8s_enabled, + file_based_vcap_services_enabled: file_based_vcap_services_enabled + ) + end describe 'GET /v3/apps/:guid/features' do context 'getting a list of available features for the app' do @@ -29,11 +39,16 @@ 'name' => 'service-binding-k8s', 'description' => 'Enable k8s service bindings for the app', 'enabled' => true + }, + { + 'name' => 'file-based-vcap-services', + 'description' => 'Enable file-based VCAP service bindings for the app', + 'enabled' => false } ], 'pagination' => { - 'total_results' => 3, + 'total_results' => 4, 'total_pages' => 1, 'first' => { 'href' => "/v3/apps/#{app_model.guid}/features" }, 'last' => { 'href' => "/v3/apps/#{app_model.guid}/features" }, @@ -112,6 +127,19 @@ it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS end + + context 'file-based-vcap-services app feature' do + let(:api_call) { ->(user_headers) { get "/v3/apps/#{app_model.guid}/features/file-based-vcap-services", nil, user_headers } } + let(:feature_response_object) do + { + 'name' => 'file-based-vcap-services', + 'description' => 'Enable file-based VCAP service bindings for the app', + 'enabled' => false + } + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end end describe 'PATCH /v3/apps/:guid/features/:name' do @@ -192,36 +220,148 @@ end context 'service-binding-k8s app feature' do - let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body.to_json, user_headers } } - let(:feature_response_object) do - { - 'name' => 'service-binding-k8s', - 'description' => 'Enable k8s service bindings for the app', - 'enabled' => false - } + context 'when feature is enabled' do + let(:service_binding_k8s_enabled) { true } + let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body.to_json, user_headers } } + let(:feature_response_object) do + { + 'name' => 'service-binding-k8s', + 'description' => 'Enable k8s service bindings for the app', + 'enabled' => false + } + end + + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_auditor org_billing_manager].each { |r| h[r] = { code: 404 } } + %w[admin space_developer].each { |r| h[r] = { code: 200, response_object: feature_response_object } } + h + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + + context 'when organization is suspended' do + let(:expected_codes_and_responses) do + h = super() + h['space_developer'] = { code: 403, errors: CF_ORG_SUSPENDED } + h + end + + before do + org.update(status: VCAP::CloudController::Organization::SUSPENDED) + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end end - let(:expected_codes_and_responses) do - h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) - %w[no_role org_auditor org_billing_manager].each { |r| h[r] = { code: 404 } } - %w[admin space_developer].each { |r| h[r] = { code: 200, response_object: feature_response_object } } - h + context 'when feature is disabled' do + let(:service_binding_k8s_enabled) { false } + let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body_enabled.to_json, user_headers } } + + let(:feature_response_object) do + { + 'name' => 'service-binding-k8s', + 'description' => 'Enable k8s service bindings for the app', + 'enabled' => true + } + end + + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_auditor org_billing_manager].each { |r| h[r] = { code: 404 } } + %w[admin space_developer].each { |r| h[r] = { code: 200, response_object: feature_response_object } } + h + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + + context 'when file-based-vcap-services is enabled' do + before do + patch "/v3/apps/#{app_model.guid}/features/file-based-vcap-services", request_body_enabled.to_json, admin_header + end + + it 'returns an error which states that both features cannot be enabled at the same time' do + patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body_enabled.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(parsed_response['errors'][0]['detail']).to eq("'file-based-vcap-services' and 'service-binding-k8s' features cannot be enabled at the same time.") + end + end end + end - it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + context 'file-based-vcap-services app feature' do + context 'when feature is enabled' do + let(:service_binding_k8s_enabled) { false } + let(:file_based_vcap_services_enabled) { true } + let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/file-based-vcap-services", request_body.to_json, user_headers } } + let(:feature_response_object) do + { + 'name' => 'file-based-vcap-services', + 'description' => 'Enable file-based VCAP service bindings for the app', + 'enabled' => false + } + end - context 'when organization is suspended' do let(:expected_codes_and_responses) do - h = super() - h['space_developer'] = { code: 403, errors: CF_ORG_SUSPENDED } + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_auditor org_billing_manager].each { |r| h[r] = { code: 404 } } + %w[admin space_developer].each { |r| h[r] = { code: 200, response_object: feature_response_object } } h end - before do - org.update(status: VCAP::CloudController::Organization::SUSPENDED) + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + + context 'when organization is suspended' do + let(:expected_codes_and_responses) do + h = super() + h['space_developer'] = { code: 403, errors: CF_ORG_SUSPENDED } + h + end + + before do + org.update(status: VCAP::CloudController::Organization::SUSPENDED) + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end + end + + context 'when feature is disabled' do + let(:service_binding_k8s_enabled) { false } + let(:file_based_vcap_services_enabled) { false } + let(:api_call) { ->(user_headers) { patch "/v3/apps/#{app_model.guid}/features/file-based-vcap-services", request_body_enabled.to_json, user_headers } } + + let(:feature_response_object) do + { + 'name' => 'file-based-vcap-services', + 'description' => 'Enable file-based VCAP service bindings for the app', + 'enabled' => true + } + end + + let(:expected_codes_and_responses) do + h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze) + %w[no_role org_auditor org_billing_manager].each { |r| h[r] = { code: 404 } } + %w[admin space_developer].each { |r| h[r] = { code: 200, response_object: feature_response_object } } + h end it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + + context 'when service-binding-k8s is enabled' do + before do + patch "/v3/apps/#{app_model.guid}/features/service-binding-k8s", request_body_enabled.to_json, admin_header + end + + it 'returns an error which states that both features cannot be enabled at the same time' do + patch "/v3/apps/#{app_model.guid}/features/file-based-vcap-services", request_body_enabled.to_json, admin_header + + expect(last_response.status).to eq(422) + expect(parsed_response['errors'][0]['detail']).to eq("'file-based-vcap-services' and 'service-binding-k8s' features cannot be enabled at the same time.") + end + end end end end diff --git a/spec/unit/actions/app_feature_update_spec.rb b/spec/unit/actions/app_feature_update_spec.rb index 2920b7636ba..fea88f16a7c 100644 --- a/spec/unit/actions/app_feature_update_spec.rb +++ b/spec/unit/actions/app_feature_update_spec.rb @@ -5,7 +5,7 @@ module VCAP::CloudController RSpec.describe AppFeatureUpdate do subject(:app_feature_update) { AppFeatureUpdate } - let(:app) { AppModel.make(enable_ssh: false, revisions_enabled: false, file_based_service_bindings_enabled: false) } + let(:app) { AppModel.make(enable_ssh: false, revisions_enabled: false) } let(:message) { AppFeatureUpdateMessage.new(enabled: true) } describe '.update' do @@ -32,6 +32,14 @@ module VCAP::CloudController end.to change { app.reload.service_binding_k8s_enabled }.to(true) end end + + context 'when the feature name is file-based-vcap-services' do + it 'updates the file_based_vcap_services_enabled column on the app' do + expect do + AppFeatureUpdate.update('file-based-vcap-services', app, message) + end.to change { app.reload.file_based_vcap_services_enabled }.to(true) + end + end end end end diff --git a/spec/unit/controllers/v3/app_features_controller_spec.rb b/spec/unit/controllers/v3/app_features_controller_spec.rb index f0780acc43e..8cc85b87466 100644 --- a/spec/unit/controllers/v3/app_features_controller_spec.rb +++ b/spec/unit/controllers/v3/app_features_controller_spec.rb @@ -13,6 +13,9 @@ let(:app_feature_service_binding_k8s_response) do { 'name' => 'service-binding-k8s', 'description' => 'Enable k8s service bindings for the app', 'enabled' => true } end + let(:app_feature_file_based_vcap_services_response) do + { 'name' => 'file-based-vcap-services', 'description' => 'Enable file-based VCAP service bindings for the app', 'enabled' => false } + end before do space.update(allow_ssh: true) @@ -23,7 +26,7 @@ describe '#index' do let(:pagination_hash) do { - 'total_results' => 3, + 'total_results' => 4, 'total_pages' => 1, 'first' => { 'href' => "/v3/apps/#{app_model.guid}/features" }, 'last' => { 'href' => "/v3/apps/#{app_model.guid}/features" }, @@ -42,7 +45,7 @@ it 'returns app features' do get :index, params: { app_guid: app_model.guid } expect(parsed_body).to eq( - 'resources' => [app_feature_ssh_response, app_feature_revisions_response, app_feature_service_binding_k8s_response], + 'resources' => [app_feature_ssh_response, app_feature_revisions_response, app_feature_service_binding_k8s_response, app_feature_file_based_vcap_services_response], 'pagination' => pagination_hash ) end @@ -75,6 +78,11 @@ expect(parsed_body).to eq(app_feature_service_binding_k8s_response) end + it 'returns the file-based-vcap-services feature' do + get :show, params: { app_guid: app_model.guid, name: 'file-based-vcap-services' } + expect(parsed_body).to eq(app_feature_file_based_vcap_services_response) + end + it 'throws 404 for a non-existent feature' do set_current_user_as_role(role: 'admin', org: org, space: space, user: user) diff --git a/spec/unit/presenters/v3/app_feature_presenter_spec.rb b/spec/unit/presenters/v3/app_feature_presenter_spec.rb index afc4822a787..4abc8e972a5 100644 --- a/spec/unit/presenters/v3/app_feature_presenter_spec.rb +++ b/spec/unit/presenters/v3/app_feature_presenter_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' require 'presenters/v3/app_ssh_feature_presenter' require 'presenters/v3/app_service_binding_k8s_feature_presenter' +require 'presenters/v3/app_file_based_vcap_services_feature_presenter' module VCAP::CloudController::Presenters::V3 RSpec.describe AppSshFeaturePresenter do @@ -28,4 +29,17 @@ module VCAP::CloudController::Presenters::V3 end end end + + RSpec.describe AppFileBasedVcapServicesFeaturePresenter do + let(:app) { VCAP::CloudController::AppModel.make } + + describe '#to_hash' do + it 'presents the app feature as json' do + result = AppFileBasedVcapServicesFeaturePresenter.new(app).to_hash + expect(result[:name]).to eq('file-based-vcap-services') + expect(result[:description]).to eq('Enable file-based VCAP service bindings for the app') + expect(result[:enabled]).to eq(app.file_based_vcap_services_enabled) + end + end + end end From c5f92e23259a139d6ef62be6d299b33935a6a759 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Fri, 28 Feb 2025 10:22:02 +0100 Subject: [PATCH 4/7] Improve migration and add migration tests --- ...e_based_service_binding_feature_columns.rb | 40 ++++ ...apps_service_binding_k8s_enabled_column.rb | 19 -- ...ed_service_binding_feature_columns_spec.rb | 205 ++++++++++++++++++ ...service_binding_k8s_enabled_column_spec.rb | 35 --- 4 files changed, 245 insertions(+), 54 deletions(-) create mode 100644 db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb delete mode 100644 db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb create mode 100644 spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb delete mode 100644 spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb diff --git a/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb new file mode 100644 index 00000000000..92207e55014 --- /dev/null +++ b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb @@ -0,0 +1,40 @@ +Sequel.migration do + up do + if database_type == :postgres + alter_table :apps do + add_column :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true + add_column :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true + + unless check_constraint_exists?(@db) + add_constraint(name: :only_one_sb_feature_enabled, if_not_exists: true) do + Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') + end + end + end + + elsif database_type == :mysql + add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false unless schema(:apps).map(&:first).include?(:service_binding_k8s_enabled) + add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false unless schema(:apps).map(&:first).include?(:file_based_vcap_services_enabled) + + unless check_constraint_exists?(self) + run('ALTER TABLE apps ADD CONSTRAINT only_one_sb_feature_enabled CHECK (NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled))') + end + end + end + + down do + alter_table :apps do + drop_constraint :only_one_sb_feature_enabled if check_constraint_exists?(@db) + drop_column :service_binding_k8s_enabled if @db.schema(:apps).map(&:first).include?(:service_binding_k8s_enabled) + drop_column :file_based_vcap_services_enabled if @db.schema(:apps).map(&:first).include?(:file_based_vcap_services_enabled) + end + end +end + +def check_constraint_exists?(database) + if database.database_type == :postgres + database.check_constraints(:apps).include?(:only_one_sb_feature_enabled) + elsif database.database_type == :mysql + database[:information_schema__table_constraints].where(TABLE_NAME: 'apps', CONSTRAINT_TYPE: 'CHECK', CONSTRAINT_NAME: 'only_one_sb_feature_enabled').any? + end +end diff --git a/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb b/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb deleted file mode 100644 index 3d527c7a84d..00000000000 --- a/db/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column.rb +++ /dev/null @@ -1,19 +0,0 @@ -Sequel.migration do - up do - alter_table :apps do - add_column :service_binding_k8s_enabled, :boolean, default: false, null: false - add_column :file_based_vcap_services_enabled, :boolean, default: false, null: false - add_constraint(name: :only_one_sb_feature_enabled) do - Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') - end - end - end - - down do - alter_table :apps do - drop_column :service_binding_k8s_enabled - drop_column :file_based_vcap_services_enabled - drop_constraint(name: :only_one_sb_feature_enabled) - end - end -end diff --git a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb new file mode 100644 index 00000000000..c0616abdf5d --- /dev/null +++ b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb @@ -0,0 +1,205 @@ +require 'spec_helper' +require 'migrations/helpers/migration_shared_context' + +RSpec.describe 'migration to add service_binding_k8s_enabled column to apps table', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20250225132929_add_apps_file_based_service_binding_feature_columns.rb' } + end + + describe 'apps table' do + subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } + + describe 'up' do + describe 'column service_binding_k8s_enabled' do + it 'adds a column `service_binding_k8s_enabled`' do + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + run_migration + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + end + + it 'sets the default value of existing entries to false' do + db[:apps].insert(guid: 'existing_app_guid') + run_migration + expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) + end + + it 'sets the default value of new entries to false' do + run_migration + db[:apps].insert(guid: 'new_app_guid') + expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) + end + + it 'forbids null values' do + run_migration + expect { db[:apps].insert(guid: 'app_guid__nil', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + end + + context 'when it already exists' do + before do + db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true + end + + it 'does not fail' do + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect { run_migration }.not_to raise_error + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(true) + end + end + end + + describe 'column file_based_vcap_services_enabled' do + it 'adds a column `file_based_vcap_services_enabled`' do + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + run_migration + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + end + + it 'sets the default value of existing entries to false' do + db[:apps].insert(guid: 'existing_app_guid') + run_migration + expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_vcap_services_enabled]).to be(false) + end + + it 'sets the default value of new entries to false' do + run_migration + db[:apps].insert(guid: 'new_app_guid') + expect(db[:apps].first(guid: 'new_app_guid')[:file_based_vcap_services_enabled]).to be(false) + end + + it 'forbids null values' do + run_migration + expect { db[:apps].insert(guid: 'app_guid__nil', file_based_vcap_services_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + end + + context 'when it already exists' do + before do + db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true + end + + it 'does not fail' do + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + expect { run_migration }.not_to raise_error + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(true) + end + end + end + + describe 'check constraint' do + it 'adds the check constraint' do + expect(check_constraint_exists?(db)).to be(false) + run_migration + expect(check_constraint_exists?(db)).to be(true) + end + + it 'forbids setting both features to true' do + run_migration + expect { db[:apps].insert(guid: 'some_app', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| + expect(error.inspect).to include('only_one_sb_feature_enabled', 'violate') + end) + end + + context 'when it already exists' do + before do + db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true + db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true + db.alter_table :apps do + add_constraint(name: :only_one_sb_feature_enabled) do + Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') + end + end + end + + it 'does not fail' do + expect { run_migration }.not_to raise_error + end + end + end + end + + describe 'down' do + subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } + + before do + run_migration + end + + describe 'column service_binding_k8s_enabled' do + it 'removes column `service_binding_k8s_enabled`' do + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + run_rollback + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + end + + context 'when it does not exist' do + before do + db.alter_table :apps do + drop_constraint :only_one_sb_feature_enabled + drop_column :service_binding_k8s_enabled + end + end + + it 'does not fail' do + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect { run_rollback }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + end + end + end + + describe 'column file_based_vcap_services_enabled' do + it 'removes column `file_based_vcap_services_enabled`' do + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + run_rollback + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + end + + context 'when it does not exist' do + before do + db.alter_table :apps do + drop_constraint :only_one_sb_feature_enabled + drop_column :file_based_vcap_services_enabled + end + end + + it 'does not fail' do + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect { run_rollback }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + end + end + end + + describe 'check constraint' do + it 'removes the check constraint' do + expect(check_constraint_exists?(db)).to be(true) + run_rollback + expect(check_constraint_exists?(db)).to be(false) + end + + context 'when it does not exist' do + before do + db.alter_table :apps do + drop_constraint :only_one_sb_feature_enabled + end + end + + it 'does not fail' do + expect(check_constraint_exists?(db)).to be(false) + expect { run_rollback }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + end + end + end + end + end +end diff --git a/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb b/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb deleted file mode 100644 index bcb472b2ab4..00000000000 --- a/spec/migrations/20250225132929_add_apps_service_binding_k8s_enabled_column_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper' -require 'migrations/helpers/migration_shared_context' - -RSpec.describe 'migration to add service_binding_k8s_enabled column to apps table', isolation: :truncation, type: :migration do - include_context 'migration' do - let(:migration_filename) { '20250225132929_add_apps_service_binding_k8s_enabled_column.rb' } - end - - describe 'apps table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - it 'adds a column `service_binding_k8s_enabled`' do - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - run_migration - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - end - - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration - expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'sets the default value of new entries to false' do - run_migration - db[:apps].insert(guid: 'new_app_guid') - expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'forbids null values' do - run_migration - expect { db[:apps].insert(guid: 'app_guid__nil', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) - end - end -end From c6dab853a2dd18e6ed23f1d30d52631390da1d46 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Fri, 28 Feb 2025 16:55:09 +0100 Subject: [PATCH 5/7] Fix migration tests for mysql --- ...2929_add_apps_file_based_service_binding_feature_columns.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb index 92207e55014..e82506b8e1e 100644 --- a/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb +++ b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb @@ -35,6 +35,7 @@ def check_constraint_exists?(database) if database.database_type == :postgres database.check_constraints(:apps).include?(:only_one_sb_feature_enabled) elsif database.database_type == :mysql - database[:information_schema__table_constraints].where(TABLE_NAME: 'apps', CONSTRAINT_TYPE: 'CHECK', CONSTRAINT_NAME: 'only_one_sb_feature_enabled').any? + database[:information_schema__table_constraints].where(TABLE_SCHEMA: database.opts[:database], TABLE_NAME: 'apps', CONSTRAINT_TYPE: 'CHECK', + CONSTRAINT_NAME: 'only_one_sb_feature_enabled').any? end end From 3abe847af79c3753877b84d04d94b9bc6a15fc5f Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Fri, 28 Feb 2025 17:16:35 +0100 Subject: [PATCH 6/7] Do not try to create/drop check constraint on mysql < 8 --- ...e_based_service_binding_feature_columns.rb | 10 +- ...ed_service_binding_feature_columns_spec.rb | 98 +++++++++++++------ 2 files changed, 75 insertions(+), 33 deletions(-) diff --git a/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb index e82506b8e1e..9064fcc05eb 100644 --- a/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb +++ b/db/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns.rb @@ -16,7 +16,7 @@ add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false unless schema(:apps).map(&:first).include?(:service_binding_k8s_enabled) add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false unless schema(:apps).map(&:first).include?(:file_based_vcap_services_enabled) - unless check_constraint_exists?(self) + if check_constraint_supported?(self) && !check_constraint_exists?(self) run('ALTER TABLE apps ADD CONSTRAINT only_one_sb_feature_enabled CHECK (NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled))') end end @@ -24,7 +24,7 @@ down do alter_table :apps do - drop_constraint :only_one_sb_feature_enabled if check_constraint_exists?(@db) + drop_constraint :only_one_sb_feature_enabled if check_constraint_supported?(@db) && check_constraint_exists?(@db) drop_column :service_binding_k8s_enabled if @db.schema(:apps).map(&:first).include?(:service_binding_k8s_enabled) drop_column :file_based_vcap_services_enabled if @db.schema(:apps).map(&:first).include?(:file_based_vcap_services_enabled) end @@ -39,3 +39,9 @@ def check_constraint_exists?(database) CONSTRAINT_NAME: 'only_one_sb_feature_enabled').any? end end + +# check constraints are not available in Mysql versions < 8 +# this is also enforced on application level, so it should be fine not to create it on that version +def check_constraint_supported?(database) + database.database_type == :postgres || (database.database_type == :mysql && database.server_version >= 80_000) +end diff --git a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb index c0616abdf5d..7d3f5646362 100644 --- a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb +++ b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require 'migrations/helpers/migration_shared_context' -RSpec.describe 'migration to add service_binding_k8s_enabled column to apps table', isolation: :truncation, type: :migration do +RSpec.describe 'migration to add file-based service binding feature columns to apps table', isolation: :truncation, type: :migration do include_context 'migration' do let(:migration_filename) { '20250225132929_add_apps_file_based_service_binding_feature_columns.rb' } end @@ -44,7 +44,7 @@ expect { run_migration }.not_to raise_error expect(db[:apps].columns).to include(:service_binding_k8s_enabled) expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) + expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) end end end @@ -83,38 +83,56 @@ expect { run_migration }.not_to raise_error expect(db[:apps].columns).to include(:service_binding_k8s_enabled) expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) + expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) end end end describe 'check constraint' do - it 'adds the check constraint' do - expect(check_constraint_exists?(db)).to be(false) - run_migration - expect(check_constraint_exists?(db)).to be(true) - end + context 'when supported' do + before do + skip 'check constraint not supported by db' unless check_constraint_supported?(db) + end - it 'forbids setting both features to true' do - run_migration - expect { db[:apps].insert(guid: 'some_app', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| - expect(error.inspect).to include('only_one_sb_feature_enabled', 'violate') - end) - end + it 'adds the check constraint' do + expect(check_constraint_exists?(db)).to be(false) + run_migration + expect(check_constraint_exists?(db)).to be(true) + end - context 'when it already exists' do - before do - db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true - db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true - db.alter_table :apps do - add_constraint(name: :only_one_sb_feature_enabled) do - Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') + it 'forbids setting both features to true' do + run_migration + expect { db[:apps].insert(guid: 'some_app', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| + expect(error.inspect).to include('only_one_sb_feature_enabled', 'violate') + end) + end + + context 'when it already exists' do + before do + db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true + db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true + db.alter_table :apps do + add_constraint(name: :only_one_sb_feature_enabled) do + Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') + end end end + + it 'does not fail' do + expect { run_migration }.not_to raise_error + end + end + end + + context 'when not supported' do + before do + skip 'check constraint supported by db' if check_constraint_supported?(db) end it 'does not fail' do expect { run_migration }.not_to raise_error + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) end end end @@ -178,25 +196,43 @@ end describe 'check constraint' do - it 'removes the check constraint' do - expect(check_constraint_exists?(db)).to be(true) - run_rollback - expect(check_constraint_exists?(db)).to be(false) + context 'when supported' do + before do + skip 'check constraint not supported by db' unless check_constraint_supported?(db) + end + + it 'removes the check constraint' do + expect(check_constraint_exists?(db)).to be(true) + run_rollback + expect(check_constraint_exists?(db)).to be(false) + end + + context 'when it does not exist' do + before do + db.alter_table :apps do + drop_constraint :only_one_sb_feature_enabled + end + end + + it 'does not fail' do + expect(check_constraint_exists?(db)).to be(false) + expect { run_rollback }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + end + end end - context 'when it does not exist' do + context 'when not supported' do before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - end + skip 'check constraint supported by db' if check_constraint_supported?(db) end it 'does not fail' do - expect(check_constraint_exists?(db)).to be(false) expect { run_rollback }.not_to raise_error expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) end end end From 2ed827670903a88811bd0e7549a327a8077b03c6 Mon Sep 17 00:00:00 2001 From: Sven Krieger <37476281+svkrieger@users.noreply.github.com> Date: Tue, 11 Mar 2025 10:49:14 +0100 Subject: [PATCH 7/7] Use constants for app feature names --- app/actions/app_feature_update.rb | 8 ++++---- app/controllers/v3/app_features_controller.rb | 6 +++--- .../v3/app_file_based_vcap_services_feature_presenter.rb | 2 +- .../v3/app_service_binding_k8s_feature_presenter.rb | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/actions/app_feature_update.rb b/app/actions/app_feature_update.rb index 022e7504025..e2475df8dd1 100644 --- a/app/actions/app_feature_update.rb +++ b/app/actions/app_feature_update.rb @@ -2,13 +2,13 @@ module VCAP::CloudController class AppFeatureUpdate def self.update(feature_name, app, message) case feature_name - when 'ssh' + when AppFeaturesController::SSH_FEATURE app.update(enable_ssh: message.enabled) - when 'revisions' + when AppFeaturesController::REVISIONS_FEATURE app.update(revisions_enabled: message.enabled) - when 'service-binding-k8s' + when AppFeaturesController::SERVICE_BINDING_K8S_FEATURE app.update(service_binding_k8s_enabled: message.enabled) - when 'file-based-vcap-services' + when AppFeaturesController::FILE_BASED_VCAP_SERVICES_FEATURE app.update(file_based_vcap_services_enabled: message.enabled) end end diff --git a/app/controllers/v3/app_features_controller.rb b/app/controllers/v3/app_features_controller.rb index 19ccdb5b3ef..a83812ea914 100644 --- a/app/controllers/v3/app_features_controller.rb +++ b/app/controllers/v3/app_features_controller.rb @@ -56,7 +56,7 @@ 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' and 'service-binding-k8s' features cannot be enabled at the same time.") + unprocessable!("'#{FILE_BASED_VCAP_SERVICES_FEATURE}' and '#{SERVICE_BINDING_K8S_FEATURE}' features cannot be enabled at the same time.") end AppFeatureUpdate.update(hashed_params[:name], app, message) @@ -105,9 +105,9 @@ def presented_app_features(app) end def both_service_binding_features_enabled?(app, feature_name) - if feature_name == 'file-based-vcap-services' + if feature_name == FILE_BASED_VCAP_SERVICES_FEATURE app.service_binding_k8s_enabled - elsif feature_name == 'service-binding-k8s' + elsif feature_name == SERVICE_BINDING_K8S_FEATURE app.file_based_vcap_services_enabled end end diff --git a/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb b/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb index 456a118ce50..66f02be8937 100644 --- a/app/presenters/v3/app_file_based_vcap_services_feature_presenter.rb +++ b/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: 'file-based-vcap-services', + name: AppFeaturesController::FILE_BASED_VCAP_SERVICES_FEATURE, description: 'Enable file-based VCAP service bindings for the app', enabled: app.file_based_vcap_services_enabled } diff --git a/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb b/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb index 11f0425964a..af80df22247 100644 --- a/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb +++ b/app/presenters/v3/app_service_binding_k8s_feature_presenter.rb @@ -4,7 +4,7 @@ module VCAP::CloudController::Presenters::V3 class AppServiceBindingK8sFeaturePresenter < BasePresenter def to_hash { - name: 'service-binding-k8s', + name: AppFeaturesController::SERVICE_BINDING_K8S_FEATURE, description: 'Enable k8s service bindings for the app', enabled: app.service_binding_k8s_enabled }