From 8e3f4d75a9a0db38e79df87eea6479e756fc1b02 Mon Sep 17 00:00:00 2001
From: "Dzmitry (Dima) Meshcharakou"
<12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Wed, 12 Feb 2025 17:19:54 +0000
Subject: [PATCH] Change cleanup policies to ignore protected tags
---
.../container_registry/protection/tag_rule.rb | 9 ++
.../cleanup_tags_base_service.rb | 23 +++++
.../gitlab/cleanup_tags_service.rb | 7 ++
.../delete_container_repository_worker.rb | 3 +-
doc/api/container_registry.md | 1 +
.../protection/tag_rule_spec.rb | 64 +++++++++++++-
.../gitlab/cleanup_tags_service_spec.rb | 85 +++++++++++++++++--
.../cleanup_tags_service_shared_examples.rb | 22 +++++
8 files changed, 202 insertions(+), 12 deletions(-)
diff --git a/app/models/container_registry/protection/tag_rule.rb b/app/models/container_registry/protection/tag_rule.rb
index 562cbbf2ac273..cd0be80788a04 100644
--- a/app/models/container_registry/protection/tag_rule.rb
+++ b/app/models/container_registry/protection/tag_rule.rb
@@ -7,6 +7,7 @@ class TagRule < ApplicationRecord
ACCESS_LEVELS = Gitlab::Access.sym_options_with_admin.slice(:maintainer, :owner, :admin).freeze
MAX_TAG_RULES_PER_PROJECT = 5
+ DELETE_ACTIONS = ['delete'].freeze
enum :minimum_access_level_for_delete, ACCESS_LEVELS, prefix: true
enum :minimum_access_level_for_push, ACCESS_LEVELS, prefix: true
@@ -26,6 +27,14 @@ class TagRule < ApplicationRecord
where(conditions.reduce(:or))
}
+ scope :for_delete_and_access, ->(access_level) do
+ for_actions_and_access(DELETE_ACTIONS, access_level)
+ end
+
+ scope :tag_name_patterns_for_project, ->(project_id) do
+ select(:tag_name_pattern).where(project_id: project_id)
+ end
+
def push_restricted?(access_level)
Gitlab::Access.sym_options_with_admin[minimum_access_level_for_push.to_sym] > access_level
end
diff --git a/app/services/projects/container_repository/cleanup_tags_base_service.rb b/app/services/projects/container_repository/cleanup_tags_base_service.rb
index 45557d0350297..41cfbcc4a67e1 100644
--- a/app/services/projects/container_repository/cleanup_tags_base_service.rb
+++ b/app/services/projects/container_repository/cleanup_tags_base_service.rb
@@ -21,6 +21,29 @@ def filter_by_name!(tags)
end
end
+ def filter_out_protected!(tags)
+ return if Feature.disabled?(:container_registry_protected_tags, project)
+
+ tag_rules = ::ContainerRegistry::Protection::TagRule.tag_name_patterns_for_project(project.id)
+
+ if current_user
+ return if current_user.can_admin_all_resources?
+
+ user_access_level = current_user.max_member_access_for_project(project.id)
+ tag_rules = tag_rules.for_delete_and_access(user_access_level)
+ end
+
+ return if tag_rules.blank?
+
+ patterns = tag_rules.map { |rule| ::Gitlab::UntrustedRegexp.new(rule.tag_name_pattern) }
+
+ tags.reject! do |tag|
+ patterns.detect do |pattern|
+ pattern.match?(tag.name)
+ end
+ end
+ end
+
# Should return [tags_to_delete, tags_to_keep]
def partition_by_keep_n(tags)
return [tags, []] unless keep_n
diff --git a/app/services/projects/container_repository/gitlab/cleanup_tags_service.rb b/app/services/projects/container_repository/gitlab/cleanup_tags_service.rb
index 714a9d4333342..6db66e4bc79c3 100644
--- a/app/services/projects/container_repository/gitlab/cleanup_tags_service.rb
+++ b/app/services/projects/container_repository/gitlab/cleanup_tags_service.rb
@@ -34,6 +34,9 @@ def execute_for_tags(tags, overall_result)
tags = filter_by_keep_n(tags)
tags = filter_by_older_than(tags)
+ # As a most expensive operation it should follow after the other filters.
+ filter_out_protected!(tags) unless skip_protected_tags?
+
overall_result[:before_delete_size] += tags.size
overall_result[:original_size] += original_size
@@ -76,6 +79,10 @@ def pushed_at(tag)
def timeout_disabled?
params['disable_timeout'] || false
end
+
+ def skip_protected_tags?
+ !!params['skip_protected_tags']
+ end
end
end
end
diff --git a/app/workers/container_registry/delete_container_repository_worker.rb b/app/workers/container_registry/delete_container_repository_worker.rb
index 5acd1a395a1dd..566158bee0dcb 100644
--- a/app/workers/container_registry/delete_container_repository_worker.rb
+++ b/app/workers/container_registry/delete_container_repository_worker.rb
@@ -19,7 +19,8 @@ class DeleteContainerRepositoryWorker
CLEANUP_TAGS_SERVICE_PARAMS = {
'name_regex_delete' => '.*',
'keep_latest' => false,
- 'container_expiration_policy' => true # to avoid permissions checks
+ 'container_expiration_policy' => true, # to avoid permissions checks
+ 'skip_protected_tags' => true
}.freeze
def perform_work
diff --git a/doc/api/container_registry.md b/doc/api/container_registry.md
index c9522401d3226..3399a1e5d73ed 100644
--- a/doc/api/container_registry.md
+++ b/doc/api/container_registry.md
@@ -352,6 +352,7 @@ if successful, and performs the following operations:
- It keeps N latest matching tags (if `keep_n` is specified).
- It only removes tags that are older than X amount of time (if `older_than` is
specified).
+- It excludes [protected tags](../user/packages/container_registry/protected_container_tags.md).
- It schedules the asynchronous job to be executed in the background.
These operations are executed asynchronously and can take time to get executed.
diff --git a/spec/models/container_registry/protection/tag_rule_spec.rb b/spec/models/container_registry/protection/tag_rule_spec.rb
index 3b40a16ac9db8..a34f39275f02c 100644
--- a/spec/models/container_registry/protection/tag_rule_spec.rb
+++ b/spec/models/container_registry/protection/tag_rule_spec.rb
@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe ContainerRegistry::Protection::TagRule, type: :model, feature_category: :container_registry do
+ using RSpec::Parameterized::TableSyntax
+
it_behaves_like 'having unique enum values'
describe 'relationships' do
@@ -141,8 +143,6 @@
rule_four
end
- using RSpec::Parameterized::TableSyntax
-
where(:user_access_level, :actions, :expected_rules) do
Gitlab::Access::DEVELOPER | ['push'] | lazy { [rule_one, rule_two, rule_three, rule_four] }
Gitlab::Access::DEVELOPER | ['delete'] | lazy { [rule_one, rule_two, rule_three, rule_four] }
@@ -169,6 +169,66 @@
end
end
+ describe '.for_delete_and_access' do
+ let_it_be(:rule_one) do
+ create(:container_registry_protection_tag_rule,
+ tag_name_pattern: 'one',
+ minimum_access_level_for_push: :maintainer,
+ minimum_access_level_for_delete: :maintainer)
+ end
+
+ let_it_be(:rule_two) do
+ create(:container_registry_protection_tag_rule,
+ tag_name_pattern: 'two',
+ minimum_access_level_for_push: :owner,
+ minimum_access_level_for_delete: :maintainer)
+ end
+
+ let_it_be(:rule_three) do
+ create(:container_registry_protection_tag_rule,
+ tag_name_pattern: 'three',
+ minimum_access_level_for_push: :maintainer,
+ minimum_access_level_for_delete: :admin)
+ end
+
+ let_it_be(:rule_four) do
+ create(:container_registry_protection_tag_rule,
+ tag_name_pattern: 'four',
+ minimum_access_level_for_push: :admin,
+ minimum_access_level_for_delete: :admin)
+ end
+
+ where(:user_access_level, :expected_rules) do
+ Gitlab::Access::DEVELOPER | [ref(:rule_one), ref(:rule_two), ref(:rule_three), ref(:rule_four)]
+ Gitlab::Access::MAINTAINER | [ref(:rule_three), ref(:rule_four)]
+ Gitlab::Access::OWNER | [ref(:rule_three), ref(:rule_four)]
+ Gitlab::Access::ADMIN | []
+ end
+
+ with_them do
+ subject { described_class.for_delete_and_access(user_access_level) }
+
+ it 'returns the expected rules' do
+ is_expected.to match_array(expected_rules)
+ end
+ end
+ end
+
+ describe '.tag_name_patterns_for_projects' do
+ let_it_be(:rule) { create(:container_registry_protection_tag_rule) }
+ let_it_be(:rule2) { create(:container_registry_protection_tag_rule) }
+
+ subject(:result) { described_class.tag_name_patterns_for_project(rule.project_id) }
+
+ it 'contains matched rule' do
+ expect(result.pluck(:tag_name_pattern)).to contain_exactly(rule.tag_name_pattern)
+ end
+
+ it 'selects only the tag_name_pattern' do
+ expect(result.select_values).to contain_exactly(:tag_name_pattern)
+ end
+ end
+
describe '#push_restricted?' do
let(:rule) do
create(
diff --git a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb
index d2f8dcad1a4ef..e186795187f51 100644
--- a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb
+++ b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb
@@ -7,13 +7,12 @@
include_context 'for a cleanup tags service'
- let_it_be(:user) { create(:user) }
let_it_be(:user) { create(:user) }
let_it_be(:project, reload: true) { create(:project, :private) }
let(:repository) { create(:container_repository, :root, project: project) }
let(:service) { described_class.new(container_repository: repository, current_user: user, params: params) }
- let(:tags) { %w[latest A Ba Bb C D E] }
+ let(:tags) { %w[latest A Ba Bb C D 17-8-stable] }
before do
project.add_maintainer(user) if user
@@ -37,7 +36,7 @@
'Bb' => six_days_ago,
'C' => one_month_ago,
'D' => nil,
- 'E' => nil
+ '17-8-stable' => nil
}
)
end
@@ -49,17 +48,17 @@
let(:tags_page_size) { 2 }
it_behaves_like 'when regex matching everything is specified',
- delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[E]]
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
it_behaves_like 'when regex matching everything is specified and latest is not kept',
- delete_expectations: [%w[latest A], %w[Ba Bb], %w[C D], %w[E]]
+ delete_expectations: [%w[latest A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
it_behaves_like 'when delete regex matching specific tags is used'
it_behaves_like 'when delete regex matching specific tags is used with overriding allow regex'
it_behaves_like 'with allow regex value',
- delete_expectations: [%w[A], %w[C D], %w[E]]
+ delete_expectations: [%w[A], %w[C D], %w[17-8-stable]]
it_behaves_like 'when keeping only N tags',
delete_expectations: [%w[Bb]]
@@ -87,6 +86,40 @@
it_behaves_like 'when running a container_expiration_policy',
delete_expectations: [%w[Bb], %w[C]]
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
+
+ context 'with admin minimum_access_level_for_delete' do
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D]],
+ minimum_access_level_for_delete: :admin
+ end
+
+ context 'without user' do
+ let(:user) { nil }
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D]]
+ end
+
+ context 'with the skip_protected_tags param' do
+ let(:params) do
+ { 'skip_protected_tags' => true }
+ end
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
+ end
+
+ context 'with the container_registry_protected_tags disabled' do
+ before do
+ stub_feature_flags(container_registry_protected_tags: false)
+ end
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
+ end
+
context 'with a timeout' do
let(:params) do
{ 'name_regex_delete' => '.*' }
@@ -113,7 +146,7 @@
end
it_behaves_like 'when regex matching everything is specified',
- delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[E]]
+ delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[17-8-stable]]
end
end
end
@@ -122,14 +155,14 @@
let(:tags_page_size) { 1000 }
it_behaves_like 'when regex matching everything is specified',
- delete_expectations: [%w[A Ba Bb C D E]]
+ delete_expectations: [%w[A Ba Bb C D 17-8-stable]]
it_behaves_like 'when delete regex matching specific tags is used'
it_behaves_like 'when delete regex matching specific tags is used with overriding allow regex'
it_behaves_like 'with allow regex value',
- delete_expectations: [%w[A C D E]]
+ delete_expectations: [%w[A C D 17-8-stable]]
it_behaves_like 'when keeping only N tags',
delete_expectations: [%w[Ba Bb C]]
@@ -148,6 +181,40 @@
it_behaves_like 'when running a container_expiration_policy',
delete_expectations: [%w[Ba Bb C]]
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A Ba Bb C D 17-8-stable]]
+
+ context 'with admin minimum_access_level_for_delete' do
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A Ba Bb C D]],
+ minimum_access_level_for_delete: :admin
+ end
+
+ context 'without user' do
+ let(:user) { nil }
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A Ba Bb C D]]
+ end
+
+ context 'with the skip_protected_tags param' do
+ let(:params) do
+ { 'skip_protected_tags' => true }
+ end
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A Ba Bb C D 17-8-stable]]
+ end
+
+ context 'with the container_registry_protected_tags disabled' do
+ before do
+ stub_feature_flags(container_registry_protected_tags: false)
+ end
+
+ it_behaves_like 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$',
+ delete_expectations: [%w[A Ba Bb C D 17-8-stable]]
+ end
end
context 'with no tags page' do
diff --git a/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb b/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
index f9f8435c211ca..94b7fe3719572 100644
--- a/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
+++ b/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
@@ -194,6 +194,28 @@
end
end
+RSpec.shared_examples 'with protected rule having pattern ^\d{1,2}-\d{1,2}-stable$' do
+ |delete_expectations:, service_response_extra: {}, supports_caching: false,
+ minimum_access_level_for_delete: :maintainer|
+ let_it_be(:rule) do
+ create(
+ :container_registry_protection_tag_rule,
+ tag_name_pattern: '^\d{1,2}-\d{1,2}-stable$',
+ project: project,
+ minimum_access_level_for_delete: minimum_access_level_for_delete
+ )
+ end
+
+ let(:params) do
+ { 'name_regex_delete' => '.*' }
+ end
+
+ it_behaves_like 'removing the expected tags',
+ service_response_extra: service_response_extra,
+ supports_caching: supports_caching,
+ delete_expectations: delete_expectations
+end
+
RSpec.shared_examples 'not removing anything' do |service_response_extra: {}, supports_caching: false|
it 'does not remove anything' do
expect(Projects::ContainerRepository::DeleteTagsService).not_to receive(:new)
--
GitLab