Merge branch '15398-admin-user-for-policies' into 'master'

Allow Container Expiration Policy to run tag cleanup

See merge request gitlab-org/gitlab!24424

app/assets/javascripts/registry/shared/components/expiration_policy_fields.vue

@@ -82,7 +82,7 @@ export default {
     regexHelpText() {
       return sprintf(
         s__(
-          'ContainerRegistry|Wildcards such as %{codeStart}*-stable%{codeEnd} or %{codeStart}production/*%{codeEnd} are supported.  To select all tags, use %{codeStart}.*%{codeEnd}',
+          'ContainerRegistry|Wildcards such as %{codeStart}.*-stable%{codeEnd} or %{codeStart}production/.*%{codeEnd} are supported.  To select all tags, use %{codeStart}.*%{codeEnd}',
         ),
         {
           codeStart: '<code>',

app/helpers/projects_helper.rb

@@ -717,6 +717,6 @@ module ProjectsHelper
   def settings_container_registry_expiration_policy_available?(project)
     Feature.enabled?(:registry_retention_policies_settings, project) &&
       Gitlab.config.registry.enabled &&
-      can?(current_user, :read_container_image, project)
+      can?(current_user, :destroy_container_image, project)
   end
 end

app/services/container_expiration_policy_service.rb

@@ -6,9 +6,11 @@ class ContainerExpirationPolicyService < BaseService
 
     container_expiration_policy.container_repositories.find_each do |container_repository|
       CleanupContainerRepositoryWorker.perform_async(
-        current_user.id,
+        nil,
         container_repository.id,
-        container_expiration_policy.attributes.except("created_at", "updated_at")
+        container_expiration_policy.attributes
+          .except('created_at', 'updated_at')
+          .merge(container_expiration_policy: true)
       )
     end
   end

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -5,7 +5,7 @@ module Projects
     class CleanupTagsService < BaseService
       def execute(container_repository)
         return error('feature disabled') unless can_use?
-        return error('access denied') unless can_admin?
+        return error('access denied') unless can_destroy?
 
         tags = container_repository.tags
         tags_by_digest = group_by_digest(tags)
@@ -82,8 +82,10 @@ module Projects
         end
       end
 
-      def can_admin?
-        can?(current_user, :admin_container_image, project)
+      def can_destroy?
+        return true if params['container_expiration_policy']
+
+        can?(current_user, :destroy_container_image, project)
       end
 
       def can_use?

app/workers/cleanup_container_repository_worker.rb

@@ -11,6 +11,7 @@ class CleanupContainerRepositoryWorker
   def perform(current_user_id, container_repository_id, params)
     @current_user = User.find_by_id(current_user_id)
     @container_repository = ContainerRepository.find_by_id(container_repository_id)
+    @params = params
 
     return unless valid?
 
@@ -22,9 +23,15 @@ class CleanupContainerRepositoryWorker
   private
 
   def valid?
+    return true if run_by_container_expiration_policy?
+
     current_user && container_repository && project
   end
 
+  def run_by_container_expiration_policy?
+    @params['container_expiration_policy'] && container_repository && project
+  end
+
   def project
     container_repository&.project
   end

lib/api/project_container_repositories.rb

@@ -80,7 +80,7 @@ module API
         render_api_error!(message, 400) unless obtain_new_cleanup_container_lease
 
         CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id,
-          declared_params.except(:repository_id))
+          declared_params.except(:repository_id).merge(container_expiration_policy: false))
 
         track_event('delete_tag_bulk')
 

locale/gitlab.pot

@@ -5184,7 +5184,7 @@ msgstr ""
 msgid "ContainerRegistry|We are having trouble connecting to Docker, which could be due to an issue with your project name or path. %{docLinkStart}More Information%{docLinkEnd}"
 msgstr ""
 
-msgid "ContainerRegistry|Wildcards such as %{codeStart}*-stable%{codeEnd} or %{codeStart}production/*%{codeEnd} are supported.  To select all tags, use %{codeStart}.*%{codeEnd}"
+msgid "ContainerRegistry|Wildcards such as %{codeStart}.*-stable%{codeEnd} or %{codeStart}production/.*%{codeEnd} are supported.  To select all tags, use %{codeStart}.*%{codeEnd}"
 msgstr ""
 
 msgid "ContainerRegistry|With the Container Registry, every project can have its own space to store its Docker images. %{docLinkStart}More Information%{docLinkEnd}"

spec/requests/api/project_container_repositories_spec.rb

@@ -142,7 +142,8 @@ describe API::ProjectContainerRepositories do
         let(:worker_params) do
           { name_regex: 'v10.*',
             keep_n: 100,
-            older_than: '1 day' }
+            older_than: '1 day',
+            container_expiration_policy: false }
         end
 
         let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" }

spec/services/container_expiration_policy_service_spec.rb

@@ -17,7 +17,7 @@ describe ContainerExpirationPolicyService do
 
     it 'kicks off a cleanup worker for the container repository' do
       expect(CleanupContainerRepositoryWorker).to receive(:perform_async)
-        .with(user.id, container_repository.id, anything)
+        .with(nil, container_repository.id, hash_including(container_expiration_policy: true))
 
       subject
     end

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -130,6 +130,38 @@ describe Projects::ContainerRepository::CleanupTagsService do
         is_expected.to include(status: :success, deleted: %w(Bb Ba C))
       end
     end
+
+    context 'when running a container_expiration_policy' do
+      let(:user) { nil }
+
+      context 'with valid container_expiration_policy param' do
+        let(:params) do
+          { 'name_regex' => '.*',
+            'keep_n' => 1,
+            'older_than' => '1 day',
+            'container_expiration_policy' => true }
+        end
+
+        it 'succeeds without a user' do
+          expect_delete('sha256:configB').twice
+          expect_delete('sha256:configC')
+
+          is_expected.to include(status: :success, deleted: %w(Bb Ba C))
+        end
+      end
+
+      context 'without container_expiration_policy param' do
+        let(:params) do
+          { 'name_regex' => '.*',
+            'keep_n' => 1,
+            'older_than' => '1 day' }
+        end
+
+        it 'fails' do
+          is_expected.to include(status: :error, message: 'access denied')
+        end
+      end
+    end
   end
 
   private

spec/workers/cleanup_container_repository_worker_spec.rb

@@ -6,34 +6,49 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
   let(:repository) { create(:container_repository) }
   let(:project) { repository.project }
   let(:user) { project.owner }
-  let(:params) { { key: 'value' } }
 
   subject { described_class.new }
 
   describe '#perform' do
     let(:service) { instance_double(Projects::ContainerRepository::CleanupTagsService) }
 
-    before do
-      allow(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
-        .with(project, user, params).and_return(service)
+    context 'bulk delete api' do
+      let(:params) { { key: 'value', 'container_expiration_policy' => false } }
+
+      it 'executes the destroy service' do
+        expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
+          .with(project, user, params.merge('container_expiration_policy' => false))
+          .and_return(service)
+        expect(service).to receive(:execute)
+
+        subject.perform(user.id, repository.id, params)
+      end
+
+      it 'does not raise error when user could not be found' do
+        expect do
+          subject.perform(-1, repository.id, params)
+        end.not_to raise_error
+      end
+
+      it 'does not raise error when repository could not be found' do
+        expect do
+          subject.perform(user.id, -1, params)
+        end.not_to raise_error
+      end
     end
 
-    it 'executes the destroy service' do
-      expect(service).to receive(:execute)
+    context 'container expiration policy' do
+      let(:params) { { key: 'value', 'container_expiration_policy' => true } }
 
-      subject.perform(user.id, repository.id, params)
-    end
+      it 'executes the destroy service' do
+        expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new)
+          .with(project, nil, params.merge('container_expiration_policy' => true))
+          .and_return(service)
 
-    it 'does not raise error when user could not be found' do
-      expect do
-        subject.perform(-1, repository.id, params)
-      end.not_to raise_error
-    end
+        expect(service).to receive(:execute)
 
-    it 'does not raise error when repository could not be found' do
-      expect do
-        subject.perform(user.id, -1, params)
-      end.not_to raise_error
+        subject.perform(nil, repository.id, params)
+      end
     end
   end
 end