Add unique constraint for running security groups (#4617)

Table `security_group_spaces` does not have unique constraint which can result in duplicated entries.
This change adds a migration to replace the existing index with a unique index.
Duplicated entries will be cleaned up during the migration by keeping the oldest combination of `security_group_id` and `space_id` (determined by primary key).
In addition unique constraint errors will be caught to make parallel inserts idempotent.

Author: johha

app/models/runtime/security_group.rb

@@ -12,7 +12,7 @@ class SecurityGroup < Sequel::Model
 
     serialize_attributes :json, :rules
 
-    many_to_many :spaces
+    many_to_many :spaces, ignored_unique_constraint_violation_errors: %w[ignored_unique_constraint_violation_errors]
     many_to_many :staging_spaces,
                  class: 'VCAP::CloudController::Space',
                  join_table: 'staging_security_groups_spaces',

db/migrations/20251016120006_security_groups_spaces_unique_index.rb

@@ -0,0 +1,39 @@
+Sequel.migration do
+  up do
+    transaction do
+      # Remove duplicate entries if they exist
+      duplicates = self[:security_groups_spaces].
+                   select(:security_group_id, :space_id).
+                   group(:security_group_id, :space_id).
+                   having { count(:security_groups_spaces_pk) > 1 }
+
+      duplicates.each do |dup|
+        security_groups_spaces_pks_to_remove = self[:security_groups_spaces].
+                                               where(security_group_id: dup[:security_group_id], space_id: dup[:space_id]).
+                                               select(:security_groups_spaces_pk).
+                                               order(:security_groups_spaces_pk).
+                                               offset(1).
+                                               map(:security_groups_spaces_pk)
+
+        self[:security_groups_spaces].where(security_groups_spaces_pk: security_groups_spaces_pks_to_remove).delete
+      end
+
+      alter_table(:security_groups_spaces) do
+        # Cannot add unique constraint concurrently as it requires a transaction
+        # rubocop:disable Sequel/ConcurrentIndex
+        add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids unless @db.indexes(:security_groups_spaces).key?(:security_groups_spaces_ids)
+        drop_index %i[security_group_id space_id], name: :sgs_spaces_ids if @db.indexes(:security_groups_spaces).key?(:sgs_spaces_ids)
+        # rubocop:enable Sequel/ConcurrentIndex
+      end
+    end
+  end
+
+  down do
+    alter_table(:security_groups_spaces) do
+      # rubocop:disable Sequel/ConcurrentIndex
+      add_index %i[security_group_id space_id], name: :sgs_spaces_ids unless @db.indexes(:security_groups_spaces).key?(:sgs_spaces_ids)
+      drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids if @db.indexes(:security_groups_spaces).key?(:security_groups_spaces_ids)
+      # rubocop:enable Sequel/ConcurrentIndex
+    end
+  end
+end

spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb

@@ -0,0 +1,76 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+
+RSpec.describe 'security groups spaces unique index', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20251016120006_security_groups_spaces_unique_index_spec.rb' }
+  end
+
+  let(:space_1) { VCAP::CloudController::Space.make }
+  let(:space_2) { VCAP::CloudController::Space.make }
+  let(:sec_group_1) { VCAP::CloudController::SecurityGroup.make }
+  let(:sec_group_2) { VCAP::CloudController::SecurityGroup.make }
+
+  describe 'security_groups_spaces table' do
+    context 'up migration' do
+      it 'is in the correct state before migration' do
+        expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids)
+        expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids)
+      end
+
+      it 'removes duplicates and migrates successfully by adding unique index' do
+        db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id)
+        db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id)
+
+        # Count duplicates before migration
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(3)
+
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+
+        # Verify duplicates are removed after migration
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1)
+        expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1)
+
+        # Verify indexes are updated
+        expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids)
+        expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids)
+      end
+
+      it 'does not fail if indexes/constraints are already in desired state' do
+        db.alter_table :security_groups_spaces do
+          add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids
+          drop_index %i[security_group_id space_id], name: :sgs_spaces_ids
+        end
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+      end
+    end
+
+    context 'down migration' do
+      it 'rolls back successfully' do
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
+        expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids)
+        expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids)
+      end
+
+      it 'does not fail if indexes/constraints are already in desired state' do
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+        db.alter_table :security_groups_spaces do
+          add_index %i[security_group_id space_id], name: :sgs_spaces_ids
+          drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids
+        end
+        expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error
+      end
+    end
+  end
+end