From 03f22054673039eced17a5009c2e60d237c4091e Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 4 Oct 2023 16:36:42 +0200 Subject: [PATCH 1/4] feat: Introducing basics for protecting containers This MR introduces the basics for protecting containers, e.g. database migration, data model, etc. The scope of the basic functionality was discussed in - Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/424367 - Epic https://gitlab.com/groups/gitlab-org/-/epics/9825 Changelog: added --- app/models/container_registry/protection.rb | 9 +++++ .../container_registry/protection/rule.rb | 16 ++++++++ app/models/project.rb | 1 + .../container_registry_protection_rules.yml | 10 +++++ ...ate_container_registry_protection_rules.rb | 17 ++++++++ db/schema_migrations/20231004100000 | 1 + db/structure.sql | 29 ++++++++++++++ .../container_registry/protection/rules.rb | 9 +++++ .../protection/rule_spec.rb | 40 +++++++++++++++++++ spec/models/project_spec.rb | 1 + 10 files changed, 133 insertions(+) create mode 100644 app/models/container_registry/protection.rb create mode 100644 app/models/container_registry/protection/rule.rb create mode 100644 db/docs/container_registry_protection_rules.yml create mode 100644 db/migrate/20231004100000_create_container_registry_protection_rules.rb create mode 100644 db/schema_migrations/20231004100000 create mode 100644 spec/factories/container_registry/protection/rules.rb create mode 100644 spec/models/container_registry/protection/rule_spec.rb diff --git a/app/models/container_registry/protection.rb b/app/models/container_registry/protection.rb new file mode 100644 index 00000000000000..33c94c0c89382c --- /dev/null +++ b/app/models/container_registry/protection.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + def self.table_name_prefix + 'container_registry_protection_' + end + end +end diff --git a/app/models/container_registry/protection/rule.rb b/app/models/container_registry/protection/rule.rb new file mode 100644 index 00000000000000..bfdf1dff4276ae --- /dev/null +++ b/app/models/container_registry/protection/rule.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + class Rule < ApplicationRecord + enum push_protected_up_to_access_level: + Gitlab::Access.sym_options_with_owner.slice(:maintainer, :owner, :developer), + _prefix: :push_protected_up_to + + belongs_to :project, inverse_of: :container_registry_protection_rules + + validates :container_path_pattern, presence: true, uniqueness: { scope: :project_id }, length: { maximum: 255 } + validates :push_protected_up_to_access_level, presence: true + end + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 2e331c391b10d2..5ee11c463a1d1a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -390,6 +390,7 @@ def self.integration_association_name(name) has_many :alert_management_alerts, class_name: 'AlertManagement::Alert', inverse_of: :project has_many :alert_management_http_integrations, class_name: 'AlertManagement::HttpIntegration', inverse_of: :project + has_many :container_registry_protection_rules, class_name: 'ContainerRegistry::Protection::Rule', inverse_of: :project # Container repositories need to remove data from the container registry, # which is not managed by the DB. Hence we're still using dependent: :destroy # here. diff --git a/db/docs/container_registry_protection_rules.yml b/db/docs/container_registry_protection_rules.yml new file mode 100644 index 00000000000000..4dc27bb2f793c5 --- /dev/null +++ b/db/docs/container_registry_protection_rules.yml @@ -0,0 +1,10 @@ +--- +table_name: container_registry_protection_rules +classes: +- ContainerRegistry::Protection::Rule +feature_categories: +- container_registry +description: Represents container protection rules for container registry. +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133297 +milestone: '16.5' +gitlab_schema: gitlab_main diff --git a/db/migrate/20231004100000_create_container_registry_protection_rules.rb b/db/migrate/20231004100000_create_container_registry_protection_rules.rb new file mode 100644 index 00000000000000..18cfc0edb4eeb2 --- /dev/null +++ b/db/migrate/20231004100000_create_container_registry_protection_rules.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreateContainerRegistryProtectionRules < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def change + create_table :container_registry_protection_rules do |t| + t.references :project, null: false, index: false, foreign_key: { on_delete: :cascade } + t.timestamps_with_timezone null: false + t.integer :push_protected_up_to_access_level, null: false + t.text :container_path_pattern, limit: 255, null: false + + t.index [:project_id, :container_path_pattern], unique: true, + name: :i_container_protection_unique_project_id_container_path_pattern + end + end +end diff --git a/db/schema_migrations/20231004100000 b/db/schema_migrations/20231004100000 new file mode 100644 index 00000000000000..19e14253ed92bf --- /dev/null +++ b/db/schema_migrations/20231004100000 @@ -0,0 +1 @@ +11730ae4a1acf49c31b7a35e67753b53d8ade1f1c99b937c4cb95e3804777e3c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 0373c51c155f90..cdb9f402d6619e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14693,6 +14693,25 @@ CREATE TABLE container_registry_data_repair_details ( status smallint DEFAULT 0 NOT NULL ); +CREATE TABLE container_registry_protection_rules ( + id bigint NOT NULL, + project_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + push_protected_up_to_access_level integer NOT NULL, + container_path_pattern text NOT NULL, + CONSTRAINT check_96811ef9dc CHECK ((char_length(container_path_pattern) <= 255)) +); + +CREATE SEQUENCE container_registry_protection_rules_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE container_registry_protection_rules_id_seq OWNED BY container_registry_protection_rules.id; + CREATE TABLE container_repositories ( id integer NOT NULL, project_id integer NOT NULL, @@ -26096,6 +26115,8 @@ ALTER TABLE ONLY commit_user_mentions ALTER COLUMN id SET DEFAULT nextval('commi ALTER TABLE ONLY compliance_management_frameworks ALTER COLUMN id SET DEFAULT nextval('compliance_management_frameworks_id_seq'::regclass); +ALTER TABLE ONLY container_registry_protection_rules ALTER COLUMN id SET DEFAULT nextval('container_registry_protection_rules_id_seq'::regclass); + ALTER TABLE ONLY container_repositories ALTER COLUMN id SET DEFAULT nextval('container_repositories_id_seq'::regclass); ALTER TABLE ONLY content_blocked_states ALTER COLUMN id SET DEFAULT nextval('content_blocked_states_id_seq'::regclass); @@ -28088,6 +28109,9 @@ ALTER TABLE ONLY container_expiration_policies ALTER TABLE ONLY container_registry_data_repair_details ADD CONSTRAINT container_registry_data_repair_details_pkey PRIMARY KEY (project_id); +ALTER TABLE ONLY container_registry_protection_rules + ADD CONSTRAINT container_registry_protection_rules_pkey PRIMARY KEY (id); + ALTER TABLE ONLY container_repositories ADD CONSTRAINT container_repositories_pkey PRIMARY KEY (id); @@ -30906,6 +30930,8 @@ CREATE INDEX i_compliance_violations_on_project_id_severity_and_id ON merge_requ CREATE INDEX i_compliance_violations_on_project_id_title_and_id ON merge_requests_compliance_violations USING btree (target_project_id, title, id); +CREATE UNIQUE INDEX i_container_protection_unique_project_id_container_path_pattern ON container_registry_protection_rules USING btree (project_id, container_path_pattern); + CREATE INDEX i_custom_email_verifications_on_triggered_at_and_state_started ON service_desk_custom_email_verifications USING btree (triggered_at) WHERE (state = 0); CREATE INDEX i_dast_pre_scan_verification_steps_on_pre_scan_verification_id ON dast_pre_scan_verification_steps USING btree (dast_pre_scan_verification_id); @@ -38965,6 +38991,9 @@ ALTER TABLE ONLY dast_profiles_tags ALTER TABLE ONLY resource_iteration_events ADD CONSTRAINT fk_rails_abf5d4affa FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; +ALTER TABLE ONLY container_registry_protection_rules + ADD CONSTRAINT fk_rails_ac331fcba9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY clusters ADD CONSTRAINT fk_rails_ac3a663d79 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE SET NULL; diff --git a/spec/factories/container_registry/protection/rules.rb b/spec/factories/container_registry/protection/rules.rb new file mode 100644 index 00000000000000..47e160c0344782 --- /dev/null +++ b/spec/factories/container_registry/protection/rules.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :container_registry_protection_rule, class: 'ContainerRegistry::Protection::Rule' do + project + container_path_pattern { '@my_scope/my_container' } + push_protected_up_to_access_level { Gitlab::Access::DEVELOPER } + end +end diff --git a/spec/models/container_registry/protection/rule_spec.rb b/spec/models/container_registry/protection/rule_spec.rb new file mode 100644 index 00000000000000..8a35e54a2c904e --- /dev/null +++ b/spec/models/container_registry/protection/rule_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::Rule, type: :model, feature_category: :container_registry do + it_behaves_like 'having unique enum values' + + describe 'relationships' do + it { is_expected.to belong_to(:project).inverse_of(:container_registry_protection_rules) } + end + + describe 'enums' do + describe '#push_protected_up_to_access_level' do + it { + is_expected.to( + define_enum_for(:push_protected_up_to_access_level) + .with_values( + developer: Gitlab::Access::DEVELOPER, + maintainer: Gitlab::Access::MAINTAINER, + owner: Gitlab::Access::OWNER + ) + .with_prefix(:push_protected_up_to) + ) + } + end + end + + describe 'validations' do + subject { build(:container_registry_protection_rule) } + + describe '#container_path_pattern' do + it { is_expected.to validate_presence_of(:container_path_pattern) } + it { is_expected.to validate_length_of(:container_path_pattern).is_at_most(255) } + end + + describe '#push_protected_up_to_access_level' do + it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 23306e462373b2..902463751d18da 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -103,6 +103,7 @@ it { is_expected.to have_one(:mock_monitoring_integration) } it { is_expected.to have_one(:service_desk_custom_email_verification).class_name('ServiceDesk::CustomEmailVerification') } it { is_expected.to have_one(:container_registry_data_repair_detail).class_name('ContainerRegistry::DataRepairDetail') } + it { is_expected.to have_many(:container_registry_protection_rules).class_name('ContainerRegistry::Protection::Rule') } it { is_expected.to have_many(:commit_statuses) } it { is_expected.to have_many(:ci_pipelines) } it { is_expected.to have_many(:ci_refs) } -- GitLab From 01b0975f580e63c92ec2977349929eac51c9706a Mon Sep 17 00:00:00 2001 From: Marcel Amirault Date: Thu, 5 Oct 2023 08:33:15 +0000 Subject: [PATCH 2/4] refactor: Apply reviewing suggestions from @marcel.amirault --- db/docs/container_registry_protection_rules.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/docs/container_registry_protection_rules.yml b/db/docs/container_registry_protection_rules.yml index 4dc27bb2f793c5..1764cbc8cda6ff 100644 --- a/db/docs/container_registry_protection_rules.yml +++ b/db/docs/container_registry_protection_rules.yml @@ -4,7 +4,7 @@ classes: - ContainerRegistry::Protection::Rule feature_categories: - container_registry -description: Represents container protection rules for container registry. +description: Represents container protection rules for the container registry. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133297 milestone: '16.5' gitlab_schema: gitlab_main -- GitLab From ed7d9f027c23663d3fe60c7b742af82ebdec55ad Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sat, 7 Oct 2023 14:49:51 +0200 Subject: [PATCH 3/4] refactor: Fix failing tests regarding new schema --- ...20231004100000_create_container_registry_protection_rules.rb | 2 +- db/structure.sql | 2 +- spec/lib/gitlab/import_export/all_models.yml | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/db/migrate/20231004100000_create_container_registry_protection_rules.rb b/db/migrate/20231004100000_create_container_registry_protection_rules.rb index 18cfc0edb4eeb2..9265541856998a 100644 --- a/db/migrate/20231004100000_create_container_registry_protection_rules.rb +++ b/db/migrate/20231004100000_create_container_registry_protection_rules.rb @@ -7,7 +7,7 @@ def change create_table :container_registry_protection_rules do |t| t.references :project, null: false, index: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false - t.integer :push_protected_up_to_access_level, null: false + t.integer :push_protected_up_to_access_level, null: false, limit: 2 t.text :container_path_pattern, limit: 255, null: false t.index [:project_id, :container_path_pattern], unique: true, diff --git a/db/structure.sql b/db/structure.sql index cdb9f402d6619e..ffdaae2087adea 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14698,7 +14698,7 @@ CREATE TABLE container_registry_protection_rules ( project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - push_protected_up_to_access_level integer NOT NULL, + push_protected_up_to_access_level smallint NOT NULL, container_path_pattern text NOT NULL, CONSTRAINT check_96811ef9dc CHECK ((char_length(container_path_pattern) <= 255)) ); diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 4a6924b43a89d0..a3fa733975d959 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -669,6 +669,7 @@ project: - statistics - container_repositories - container_registry_data_repair_detail +- container_registry_protection_rules - uploads - file_uploads - import_state -- GitLab From a6dc8240ec1cabc065bf39b59dbf133b9ac46dab Mon Sep 17 00:00:00 2001 From: Gerardo Date: Mon, 9 Oct 2023 15:36:52 +0200 Subject: [PATCH 4/4] refactor: Apply reviewing suggestions from @jaime, @Quintasan This commit includes: - Adding the column, model and tests for the field `delete_protected_up_to_access_level` --- .../container_registry/protection/rule.rb | 4 ++ ...ate_container_registry_protection_rules.rb | 1 + db/structure.sql | 1 + .../container_registry/protection/rules.rb | 3 +- .../protection/rule_spec.rb | 40 +++++++++++++------ 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/app/models/container_registry/protection/rule.rb b/app/models/container_registry/protection/rule.rb index bfdf1dff4276ae..a91f3633d75cb6 100644 --- a/app/models/container_registry/protection/rule.rb +++ b/app/models/container_registry/protection/rule.rb @@ -3,6 +3,9 @@ module ContainerRegistry module Protection class Rule < ApplicationRecord + enum delete_protected_up_to_access_level: + Gitlab::Access.sym_options_with_owner.slice(:maintainer, :owner, :developer), + _prefix: :delete_protected_up_to enum push_protected_up_to_access_level: Gitlab::Access.sym_options_with_owner.slice(:maintainer, :owner, :developer), _prefix: :push_protected_up_to @@ -10,6 +13,7 @@ class Rule < ApplicationRecord belongs_to :project, inverse_of: :container_registry_protection_rules validates :container_path_pattern, presence: true, uniqueness: { scope: :project_id }, length: { maximum: 255 } + validates :delete_protected_up_to_access_level, presence: true validates :push_protected_up_to_access_level, presence: true end end diff --git a/db/migrate/20231004100000_create_container_registry_protection_rules.rb b/db/migrate/20231004100000_create_container_registry_protection_rules.rb index 9265541856998a..30a90a8391d15f 100644 --- a/db/migrate/20231004100000_create_container_registry_protection_rules.rb +++ b/db/migrate/20231004100000_create_container_registry_protection_rules.rb @@ -7,6 +7,7 @@ def change create_table :container_registry_protection_rules do |t| t.references :project, null: false, index: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false + t.integer :delete_protected_up_to_access_level, null: false, limit: 2 t.integer :push_protected_up_to_access_level, null: false, limit: 2 t.text :container_path_pattern, limit: 255, null: false diff --git a/db/structure.sql b/db/structure.sql index ffdaae2087adea..3b46918481635a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14698,6 +14698,7 @@ CREATE TABLE container_registry_protection_rules ( project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, + delete_protected_up_to_access_level smallint NOT NULL, push_protected_up_to_access_level smallint NOT NULL, container_path_pattern text NOT NULL, CONSTRAINT check_96811ef9dc CHECK ((char_length(container_path_pattern) <= 255)) diff --git a/spec/factories/container_registry/protection/rules.rb b/spec/factories/container_registry/protection/rules.rb index 47e160c0344782..cbd5c9d86524e3 100644 --- a/spec/factories/container_registry/protection/rules.rb +++ b/spec/factories/container_registry/protection/rules.rb @@ -4,6 +4,7 @@ factory :container_registry_protection_rule, class: 'ContainerRegistry::Protection::Rule' do project container_path_pattern { '@my_scope/my_container' } - push_protected_up_to_access_level { Gitlab::Access::DEVELOPER } + delete_protected_up_to_access_level { :developer } + push_protected_up_to_access_level { :developer } end end diff --git a/spec/models/container_registry/protection/rule_spec.rb b/spec/models/container_registry/protection/rule_spec.rb index 8a35e54a2c904e..9f162736efd539 100644 --- a/spec/models/container_registry/protection/rule_spec.rb +++ b/spec/models/container_registry/protection/rule_spec.rb @@ -10,19 +10,29 @@ end describe 'enums' do - describe '#push_protected_up_to_access_level' do - it { - is_expected.to( - define_enum_for(:push_protected_up_to_access_level) - .with_values( - developer: Gitlab::Access::DEVELOPER, - maintainer: Gitlab::Access::MAINTAINER, - owner: Gitlab::Access::OWNER - ) - .with_prefix(:push_protected_up_to) - ) - } - end + it { + is_expected.to( + define_enum_for(:push_protected_up_to_access_level) + .with_values( + developer: Gitlab::Access::DEVELOPER, + maintainer: Gitlab::Access::MAINTAINER, + owner: Gitlab::Access::OWNER + ) + .with_prefix(:push_protected_up_to) + ) + } + + it { + is_expected.to( + define_enum_for(:delete_protected_up_to_access_level) + .with_values( + developer: Gitlab::Access::DEVELOPER, + maintainer: Gitlab::Access::MAINTAINER, + owner: Gitlab::Access::OWNER + ) + .with_prefix(:delete_protected_up_to) + ) + } end describe 'validations' do @@ -33,6 +43,10 @@ it { is_expected.to validate_length_of(:container_path_pattern).is_at_most(255) } end + describe '#delete_protected_up_to_access_level' do + it { is_expected.to validate_presence_of(:delete_protected_up_to_access_level) } + end + describe '#push_protected_up_to_access_level' do it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } end -- GitLab