Prevent deletion of current droplet with foreign key (#3972)

Add foreign key from apps.droplet_guid to droplets.guid.

MySQL only: ensure that the collation of the foreign key column
(apps.droplet_guid) matches the referenced column (droplets.guid).

Remove invalid current droplet relations (i.e. guid does not exist)
before creating the foreign key constraint.

Add tests for ForeignKeyConstraintViolation errors.

Author: philippthun

db/migrations/20240904132000_add_foreign_key_apps_droplet_guid.rb

@@ -0,0 +1,44 @@
+def get_column_definition(table, column)
+  fetch("SHOW FULL COLUMNS FROM `#{table}`;").all.find { |c| c[:Field] == column }
+end
+
+def verify_no_other_modification(definition_before, definition_after)
+  definition_after.each do |key, value|
+    raise "Column definition '#{key}' was changed from '#{definition_before[key]}' to '#{value}'" unless value == definition_before[key]
+  end
+end
+
+Sequel.migration do
+  up do
+    if foreign_key_list(:apps).none? { |fk| fk[:name] == :fk_apps_droplet_guid }
+      self[:apps].exclude(droplet_guid: self[:droplets].select(:guid)).exclude(droplet_guid: nil).update(droplet_guid: nil)
+
+      if database_type == :mysql
+        # Ensure that the collation of the foreign key column (apps.droplet_guid) matches the referenced column (droplets.guid)
+        guid_definition = get_column_definition('droplets', 'guid')
+        droplet_guid_definition = get_column_definition('apps', 'droplet_guid')
+
+        unless droplet_guid_definition[:Collation] == guid_definition[:Collation]
+          run("ALTER TABLE `apps` MODIFY COLUMN `droplet_guid` #{droplet_guid_definition[:Type]} COLLATE #{guid_definition[:Collation]};")
+
+          changed_droplet_guid_definition = get_column_definition('apps', 'droplet_guid')
+          raise 'Collation was not changed!' unless changed_droplet_guid_definition[:Collation] == guid_definition[:Collation]
+
+          verify_no_other_modification(droplet_guid_definition.except(:Collation), changed_droplet_guid_definition.except(:Collation))
+        end
+      end
+
+      alter_table :apps do
+        add_foreign_key [:droplet_guid], :droplets, key: :guid, name: :fk_apps_droplet_guid
+      end
+    end
+  end
+
+  down do
+    if foreign_key_list(:apps).any? { |fk| fk[:name] == :fk_apps_droplet_guid }
+      alter_table :apps do
+        drop_foreign_key [:droplet_guid], name: :fk_apps_droplet_guid
+      end
+    end
+  end
+end

spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb

@@ -0,0 +1,39 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+
+RSpec.describe "migration to add foreign key on column 'droplet_guid' in table 'apps'", isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20240904132000_add_foreign_key_apps_droplet_guid.rb' }
+  end
+
+  describe 'apps table' do
+    after do
+      db[:apps].delete
+    end
+
+    context 'before adding the foreign key' do
+      it 'allows inserts with a droplet_guid that does not exist' do
+        expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.not_to raise_error
+      end
+    end
+
+    context 'after adding the foreign key' do
+      it 'prevents inserts with a droplet_guid that does not exist' do
+        Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
+
+        expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation)
+      end
+
+      it 'removed references to not existing droplets' do
+        db[:droplets].insert(guid: 'droplet_guid', state: 'some_state')
+        db[:apps].insert(guid: 'app_guid', droplet_guid: 'droplet_guid')
+        db[:apps].insert(guid: 'another_app_guid', droplet_guid: 'not_exists')
+
+        Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
+
+        expect(db[:apps].where(guid: 'app_guid').get(:droplet_guid)).to eq('droplet_guid')
+        expect(db[:apps].where(guid: 'another_app_guid').get(:droplet_guid)).to be_nil
+      end
+    end
+  end
+end

spec/unit/actions/app_assign_droplet_spec.rb

@@ -25,7 +25,6 @@ module VCAP::CloudController
       let(:message) { { 'droplet_guid' => droplet_guid } }
 
       before do
-        app_model.add_droplet_by_guid(droplet_guid)
         allow(ProcessCreateFromAppDroplet).to receive(:new).with(user_audit_info).and_return(process_create_from_app_droplet)
         allow(process_create_from_app_droplet).to receive(:create).with(app_model)
 
@@ -86,6 +85,21 @@ module VCAP::CloudController
           end
         end
 
+        context 'when the droplet gets deleted in parallel (race condition)' do
+          before do
+            allow(app_model).to receive(:lock!).and_wrap_original do |original_method|
+              droplet.destroy
+              original_method.call
+            end
+          end
+
+          it 'raises an error' do
+            expect do
+              app_assign_droplet.assign(app_model, droplet)
+            end.to raise_error AppAssignDroplet::InvalidDroplet, 'Unable to assign current droplet. Ensure the droplet exists and belongs to this app.'
+          end
+        end
+
         context 'when we fail to create missing processes' do
           before do
             allow(process_create_from_app_droplet).to receive(:create).and_raise(ProcessCreateFromAppDroplet::ProcessTypesNotFound, 'some message')
@@ -98,7 +112,7 @@ module VCAP::CloudController
           end
         end
 
-        context 'when we fail to create missing processes' do
+        context 'when we fail to create missing sidecars' do
           before do
             allow(process_create_from_app_droplet).to receive(:create).and_raise(SidecarSynchronizeFromAppDroplet::ConflictingSidecarsError, 'some message')
           end

spec/unit/actions/droplet_delete_spec.rb

@@ -71,6 +71,40 @@ module VCAP::CloudController
         expect(job.guid).not_to be_nil
       end
 
+      context 'when droplet is referenced as current droplet by an app' do
+        before do
+          droplet.app.update(droplet_guid: droplet.guid)
+        end
+
+        it 'raises an UnprocessableEntity error' do
+          expect do
+            droplet_delete.delete([droplet])
+          end.to raise_error do |error|
+            expect(error).to be_a(CloudController::Errors::ApiError)
+            expect(error.name).to eq('UnprocessableEntity')
+            expect(error.message).to match(/^The droplet is currently used.*/)
+          end
+        end
+
+        it 'does not delete the droplet' do
+          expect do
+            expect { droplet_delete.delete([droplet]) }.to raise_error(CloudController::Errors::ApiError)
+          end.not_to(change(DropletModel, :count))
+        end
+
+        it 'does not create an audit event' do
+          expect(Repositories::DropletEventRepository).not_to receive(:record_delete)
+
+          expect { droplet_delete.delete([droplet]) }.to raise_error(CloudController::Errors::ApiError)
+        end
+
+        it 'does not schedule a blobstore delete job' do
+          expect do
+            expect { droplet_delete.delete([droplet]) }.to raise_error(CloudController::Errors::ApiError)
+          end.not_to(change(Delayed::Job, :count))
+        end
+      end
+
       context 'when the droplet does not have a blobstore key' do
         before do
           allow(droplet).to receive(:blobstore_key).and_return(nil)

spec/unit/models/runtime/app_model_spec.rb

@@ -277,6 +277,12 @@ module VCAP::CloudController
           app_model.droplet = droplet
           expect(app_model).to be_valid
         end
+
+        it 'raises a ValidationFailed error in case the given droplet_guid does not exist' do
+          expect do
+            app_model.update(droplet_guid: 'not-existing')
+          end.to raise_error(Sequel::ValidationFailed, /droplet presence/)
+        end
       end
     end