Commit f209cd65 authored by Steve Abrams's avatar Steve Abrams Committed by Natalia Tepluhina

ContainerExpirationPolicies run without user

When a policy runs, it uses a new worker to allow
it to run without a user.

The policy is only accessible to users with destroy image ability
parent 2bbb6c5b
...@@ -82,7 +82,7 @@ export default { ...@@ -82,7 +82,7 @@ export default {
regexHelpText() { regexHelpText() {
return sprintf( return sprintf(
s__( 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>', codeStart: '<code>',
......
...@@ -717,6 +717,6 @@ module ProjectsHelper ...@@ -717,6 +717,6 @@ module ProjectsHelper
def settings_container_registry_expiration_policy_available?(project) def settings_container_registry_expiration_policy_available?(project)
Feature.enabled?(:registry_retention_policies_settings, project) && Feature.enabled?(:registry_retention_policies_settings, project) &&
Gitlab.config.registry.enabled && Gitlab.config.registry.enabled &&
can?(current_user, :read_container_image, project) can?(current_user, :destroy_container_image, project)
end end
end end
...@@ -6,9 +6,11 @@ class ContainerExpirationPolicyService < BaseService ...@@ -6,9 +6,11 @@ class ContainerExpirationPolicyService < BaseService
container_expiration_policy.container_repositories.find_each do |container_repository| container_expiration_policy.container_repositories.find_each do |container_repository|
CleanupContainerRepositoryWorker.perform_async( CleanupContainerRepositoryWorker.perform_async(
current_user.id, nil,
container_repository.id, 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
end end
......
...@@ -5,7 +5,7 @@ module Projects ...@@ -5,7 +5,7 @@ module Projects
class CleanupTagsService < BaseService class CleanupTagsService < BaseService
def execute(container_repository) def execute(container_repository)
return error('feature disabled') unless can_use? 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 = container_repository.tags
tags_by_digest = group_by_digest(tags) tags_by_digest = group_by_digest(tags)
...@@ -82,8 +82,10 @@ module Projects ...@@ -82,8 +82,10 @@ module Projects
end end
end end
def can_admin? def can_destroy?
can?(current_user, :admin_container_image, project) return true if params['container_expiration_policy']
can?(current_user, :destroy_container_image, project)
end end
def can_use? def can_use?
......
...@@ -11,6 +11,7 @@ class CleanupContainerRepositoryWorker ...@@ -11,6 +11,7 @@ class CleanupContainerRepositoryWorker
def perform(current_user_id, container_repository_id, params) def perform(current_user_id, container_repository_id, params)
@current_user = User.find_by_id(current_user_id) @current_user = User.find_by_id(current_user_id)
@container_repository = ContainerRepository.find_by_id(container_repository_id) @container_repository = ContainerRepository.find_by_id(container_repository_id)
@params = params
return unless valid? return unless valid?
...@@ -22,9 +23,15 @@ class CleanupContainerRepositoryWorker ...@@ -22,9 +23,15 @@ class CleanupContainerRepositoryWorker
private private
def valid? def valid?
return true if run_by_container_expiration_policy?
current_user && container_repository && project current_user && container_repository && project
end end
def run_by_container_expiration_policy?
@params['container_expiration_policy'] && container_repository && project
end
def project def project
container_repository&.project container_repository&.project
end end
......
...@@ -80,7 +80,7 @@ module API ...@@ -80,7 +80,7 @@ module API
render_api_error!(message, 400) unless obtain_new_cleanup_container_lease render_api_error!(message, 400) unless obtain_new_cleanup_container_lease
CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id, 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') track_event('delete_tag_bulk')
......
...@@ -5184,7 +5184,7 @@ msgstr "" ...@@ -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}" 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 "" 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 "" msgstr ""
msgid "ContainerRegistry|With the Container Registry, every project can have its own space to store its Docker images. %{docLinkStart}More Information%{docLinkEnd}" msgid "ContainerRegistry|With the Container Registry, every project can have its own space to store its Docker images. %{docLinkStart}More Information%{docLinkEnd}"
......
...@@ -142,7 +142,8 @@ describe API::ProjectContainerRepositories do ...@@ -142,7 +142,8 @@ describe API::ProjectContainerRepositories do
let(:worker_params) do let(:worker_params) do
{ name_regex: 'v10.*', { name_regex: 'v10.*',
keep_n: 100, keep_n: 100,
older_than: '1 day' } older_than: '1 day',
container_expiration_policy: false }
end end
let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" } let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" }
......
...@@ -17,7 +17,7 @@ describe ContainerExpirationPolicyService do ...@@ -17,7 +17,7 @@ describe ContainerExpirationPolicyService do
it 'kicks off a cleanup worker for the container repository' do it 'kicks off a cleanup worker for the container repository' do
expect(CleanupContainerRepositoryWorker).to receive(:perform_async) 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 subject
end end
......
...@@ -130,6 +130,38 @@ describe Projects::ContainerRepository::CleanupTagsService do ...@@ -130,6 +130,38 @@ describe Projects::ContainerRepository::CleanupTagsService do
is_expected.to include(status: :success, deleted: %w(Bb Ba C)) is_expected.to include(status: :success, deleted: %w(Bb Ba C))
end end
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 end
private private
......
...@@ -6,19 +6,19 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do ...@@ -6,19 +6,19 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
let(:repository) { create(:container_repository) } let(:repository) { create(:container_repository) }
let(:project) { repository.project } let(:project) { repository.project }
let(:user) { project.owner } let(:user) { project.owner }
let(:params) { { key: 'value' } }
subject { described_class.new } subject { described_class.new }
describe '#perform' do describe '#perform' do
let(:service) { instance_double(Projects::ContainerRepository::CleanupTagsService) } let(:service) { instance_double(Projects::ContainerRepository::CleanupTagsService) }
before do context 'bulk delete api' do
allow(Projects::ContainerRepository::CleanupTagsService).to receive(:new) let(:params) { { key: 'value', 'container_expiration_policy' => false } }
.with(project, user, params).and_return(service)
end
it 'executes the destroy service' do 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) expect(service).to receive(:execute)
subject.perform(user.id, repository.id, params) subject.perform(user.id, repository.id, params)
...@@ -36,4 +36,19 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do ...@@ -36,4 +36,19 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
end.not_to raise_error end.not_to raise_error
end end
end end
context 'container expiration policy' do
let(:params) { { key: 'value', 'container_expiration_policy' => true } }
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)
expect(service).to receive(:execute)
subject.perform(nil, repository.id, params)
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment