Skip to content

Draft: Refactor ComplianceFramework::FRAMEWORKS to ActiveRecord Model

Max Woolf requested to merge 251113-add_compliance_framework into master

What does this MR do?

  • Creates a new model: ComplianceManagement::Framework.
  • Renames the framework attribute on ComplianceManagement::ComplianceFrameworks::ProjectSettings to be a foreign key to ComplianceManagement::Framework. This allows us to maintain backwards compatibility with existing compliance-enabled projects. (h/t to @tancnle for that idea!)
  • Adds default compliance frameworks to the database via migrations
  • Refactors all frontend and controller instances to use new foreign relation, rather than enum.

Adds 3 new migrations (database review)

Expand migration details (database review)

AddComplianceFrameworkModel

  • Adds a new model ComplianceManagement::Framework.

Migrate

== 20200922075244 AddComplianceFrameworkModel: migrating ======================
-- table_exists?(:compliance_management_frameworks)
   -> 0.0006s
-- create_table(:compliance_management_frameworks)
   -> 0.0071s
-- transaction_open?()
   -> 0.0000s
-- execute("ALTER TABLE compliance_management_frameworks\nADD CONSTRAINT check_ab00bc2193\nCHECK ( char_length(name) <= 255 )\nNOT VALID;\n")
   -> 0.0007s
-- execute("SET statement_timeout TO 0")
   -> 0.0001s
-- execute("ALTER TABLE compliance_management_frameworks VALIDATE CONSTRAINT check_ab00bc2193;")
   -> 0.0004s
-- execute("RESET ALL")
   -> 0.0001s
-- transaction_open?()
   -> 0.0000s
-- execute("ALTER TABLE compliance_management_frameworks\nADD CONSTRAINT check_1617e0b87e\nCHECK ( char_length(description) <= 255 )\nNOT VALID;\n")
   -> 0.0002s
-- execute("ALTER TABLE compliance_management_frameworks VALIDATE CONSTRAINT check_1617e0b87e;")
   -> 0.0004s
-- transaction_open?()
   -> 0.0000s
-- execute("ALTER TABLE compliance_management_frameworks\nADD CONSTRAINT check_08cd34b2c2\nCHECK ( char_length(color) <= 7 )\nNOT VALID;\n")
   -> 0.0003s
-- execute("ALTER TABLE compliance_management_frameworks VALIDATE CONSTRAINT check_08cd34b2c2;")
   -> 0.0004s
== 20200922075244 AddComplianceFrameworkModel: migrated (0.0208s) =============

Rollback

== 20200922075244 AddComplianceFrameworkModel: reverting ======================
-- drop_table(:compliance_management_frameworks, {:force=>:cascade})
   -> 0.0055s
== 20200922075244 AddComplianceFrameworkModel: reverted (0.0056s) =============

AddDefaultComplianceFrameworks

  • Renames ComplianceManagement::ComplianceFramework::ProjectSettings.framework to ComplianceManagement::ComplianceFramework::ProjectSettings.framework_id
  • Adds a foreign key constraint.
  • Adds an index to the newly renamed column.
  • Adds records to the database that match the old compliance frameworks.
  • Sets the auto_increment value to 6 so that custom frameworks can be added in to the future.

Migrate

== 20200922094625 AddDefaultComplianceFrameworks: migrating ===================
-- column_exists?(:project_compliance_framework_settings, :project_id)
   -> 0.0011s
-- transaction_open?()
   -> 0.0000s
-- columns(:project_compliance_framework_settings)
   -> 0.0007s
-- add_column(:project_compliance_framework_settings, :framework_id, :integer, {:limit=>2, :precision=>nil, :scale=>nil})
   -> 0.0005s
-- transaction_open?()
   -> 0.0000s
-- exec_query("SELECT COUNT(*) AS count FROM \"project_compliance_framework_settings\"")
   -> 0.0004s
-- indexes(:project_compliance_framework_settings)
   -> 0.0018s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, ["framework_id"], {:unique=>false, :name=>"index_project_compliance_framework_id_settings_framework_id", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0013s
-- add_index(:project_compliance_framework_settings, ["framework_id"], {:unique=>false, :name=>"index_project_compliance_framework_id_settings_framework_id", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0017s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, ["framework_id"], {:unique=>false, :name=>"index_project_compliance_framework_id_settings_on_framework_id", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0014s
-- add_index(:project_compliance_framework_settings, ["framework_id"], {:unique=>false, :name=>"index_project_compliance_framework_id_settings_on_framework_id", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0016s
-- foreign_keys(:project_compliance_framework_settings)
   -> 0.0020s
-- quote_table_name(:project_compliance_framework_settings)
   -> 0.0000s
-- quote_column_name(:framework)
   -> 0.0000s
-- quote_column_name(:framework_id)
   -> 0.0000s
-- execute("CREATE OR REPLACE FUNCTION trigger_8b88fb98bb6e()\nRETURNS trigger AS\n$BODY$\nBEGIN\n  NEW.\"framework_id\" := NEW.\"framework\";\n  RETURN NEW;\nEND;\n$BODY$\nLANGUAGE 'plpgsql'\nVOLATILE\n")
   -> 0.0014s
-- execute("DROP TRIGGER IF EXISTS trigger_8b88fb98bb6e\nON \"project_compliance_framework_settings\"\n")
NOTICE:  trigger "trigger_8b88fb98bb6e" for relation "project_compliance_framework_settings" does not exist, skipping
   -> 0.0002s
-- execute("CREATE TRIGGER trigger_8b88fb98bb6e\nBEFORE INSERT OR UPDATE\nON \"project_compliance_framework_settings\"\nFOR EACH ROW\nEXECUTE FUNCTION trigger_8b88fb98bb6e()\n")
   -> 0.0004s
-- execute("ALTER SEQUENCE compliance_management_frameworks_id_seq RESTART WITH 6;")
   -> 0.0016s
-- transaction_open?()
   -> 0.0000s
-- foreign_keys(:project_compliance_framework_settings)
   -> 0.0022s
-- execute("ALTER TABLE project_compliance_framework_settings\nADD CONSTRAINT fk_be413374a9\nFOREIGN KEY (framework_id)\nREFERENCES compliance_management_frameworks (id)\nON DELETE CASCADE\nNOT VALID;\n")
   -> 0.0015s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, :framework_id, {:name=>"index_project_compliance_framework_settings_framework_id", :algorithm=>:concurrently})
   -> 0.0020s
-- add_index(:project_compliance_framework_settings, :framework_id, {:name=>"index_project_compliance_framework_settings_framework_id", :algorithm=>:concurrently})
   -> 0.0018s
== 20200922094625 AddDefaultComplianceFrameworks: migrated (0.0382s) ==========

Rollback

== 20200922094625 AddDefaultComplianceFrameworks: reverting ===================
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, :framework_id, {:name=>"index_project_compliance_framework_settings_framework_id", :algorithm=>:concurrently})
   -> 0.0025s
-- execute("SET statement_timeout TO 0")
   -> 0.0001s
-- remove_index(:project_compliance_framework_settings, {:name=>"index_project_compliance_framework_settings_framework_id", :algorithm=>:concurrently, :column=>:framework_id})
   -> 0.0048s
-- execute("RESET ALL")
   -> 0.0002s
-- foreign_keys(:project_compliance_framework_settings)
   -> 0.0025s
-- remove_foreign_key(:project_compliance_framework_settings, {:column=>:framework_id})
   -> 0.0028s
-- execute("DROP TRIGGER IF EXISTS trigger_8b88fb98bb6e ON project_compliance_framework_settings")
   -> 0.0004s
-- execute("DROP FUNCTION IF EXISTS trigger_8b88fb98bb6e()")
   -> 0.0003s
-- remove_column(:project_compliance_framework_settings, :framework_id)
   -> 0.0010s
== 20200922094625 AddDefaultComplianceFrameworks: reverted (0.0263s) ==========

RenameComplianceFrameworkForeignKey

  • Post migration to clean up column renaming

Migrate

== 20200924091958 RenameComplianceFrameworkForeignKey: migrating ==============
-- execute("DROP TRIGGER IF EXISTS trigger_8b88fb98bb6e ON project_compliance_framework_settings")
   -> 0.0004s
-- execute("DROP FUNCTION IF EXISTS trigger_8b88fb98bb6e()")
   -> 0.0003s
-- remove_column(:project_compliance_framework_settings, :framework)
   -> 0.0010s
== 20200924091958 RenameComplianceFrameworkForeignKey: migrated (0.0021s) =====

Rollback

== 20200924091958 RenameComplianceFrameworkForeignKey: reverting ==============
-- column_exists?(:project_compliance_framework_settings, :project_id)
   -> 0.0022s
-- transaction_open?()
   -> 0.0000s
-- columns(:project_compliance_framework_settings)
   -> 0.0008s
-- add_column(:project_compliance_framework_settings, :framework, :integer, {:limit=>2, :precision=>nil, :scale=>nil})
   -> 0.0035s
-- transaction_open?()
   -> 0.0000s
-- exec_query("SELECT COUNT(*) AS count FROM \"project_compliance_framework_settings\"")
   -> 0.0028s
-- indexes(:project_compliance_framework_settings)
   -> 0.0025s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, ["framework"], {:unique=>false, :name=>"index_project_compliance_framework_settings_framework", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0018s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- add_index(:project_compliance_framework_settings, ["framework"], {:unique=>false, :name=>"index_project_compliance_framework_settings_framework", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0063s
-- execute("RESET ALL")
   -> 0.0001s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, ["framework"], {:unique=>false, :name=>"index_project_compliance_framework_settings_on_framework", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0018s
-- add_index(:project_compliance_framework_settings, ["framework"], {:unique=>false, :name=>"index_project_compliance_framework_settings_on_framework", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0019s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:project_compliance_framework_settings, ["framework"], {:unique=>false, :name=>"index_project_compliance_framework_settings_framework", :length=>{}, :order=>{}, :using=>:btree, :algorithm=>:concurrently})
   -> 0.0019s
-- foreign_keys(:project_compliance_framework_settings)
   -> 0.0046s
-- transaction_open?()
   -> 0.0000s
-- foreign_keys("project_compliance_framework_settings")
   -> 0.0015s
-- execute("ALTER TABLE project_compliance_framework_settings\nADD CONSTRAINT fk_17c8da60e0\nFOREIGN KEY (framework)\nREFERENCES compliance_management_frameworks (id)\nON DELETE CASCADE\nNOT VALID;\n")
   -> 0.0075s
-- execute("ALTER TABLE project_compliance_framework_settings VALIDATE CONSTRAINT fk_17c8da60e0;")
   -> 0.0011s
-- quote_table_name(:project_compliance_framework_settings)
   -> 0.0000s
-- quote_column_name(:framework)
   -> 0.0000s
-- quote_column_name(:framework_id)
   -> 0.0000s
-- execute("CREATE OR REPLACE FUNCTION trigger_8b88fb98bb6e()\nRETURNS trigger AS\n$BODY$\nBEGIN\n  NEW.\"framework_id\" := NEW.\"framework\";\n  RETURN NEW;\nEND;\n$BODY$\nLANGUAGE 'plpgsql'\nVOLATILE\n")
   -> 0.0060s
-- execute("DROP TRIGGER IF EXISTS trigger_8b88fb98bb6e\nON \"project_compliance_framework_settings\"\n")
NOTICE:  trigger "trigger_8b88fb98bb6e" for relation "project_compliance_framework_settings" does not exist, skipping
   -> 0.0002s
-- execute("CREATE TRIGGER trigger_8b88fb98bb6e\nBEFORE INSERT OR UPDATE\nON \"project_compliance_framework_settings\"\nFOR EACH ROW\nEXECUTE FUNCTION trigger_8b88fb98bb6e()\n")
   -> 0.0008s
== 20200924091958 RenameComplianceFrameworkForeignKey: reverted (0.0573s) =====

TODO:

  • Consider renaming framework to framework_id now that we're referring to a foreign key. Although this might be a lot of work and require downtime, I'm not sure yet. Turns out this is a hard requirement.
  • Adds production db fixtures for new gitlab installs which use structure.sql and don't run migrations!
  • Thoroughly test migration path to make sure existing projects are unaffected\
  • Add missing specs from new features
  • Mop up a lot of failing tests
  • Remove migration down sequence. It doesn't do what you think it does, Max.
  • Remove some more, now, defunct code
  • Handover to @djensen to guide this MR through (a completely seamless 😬) review once I'm OOO.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Mentions #251113 (closed)

Edited by Max Woolf

Merge request reports