diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 20:59:58 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 20:59:58 +0000 |
commit | 9b1ce4081a52d259c18325ea27501934eb2dfc59 (patch) | |
tree | 55129d62cb8313b57f1fb67dd862bf047e3dd249 | |
parent | 81d8c016732a3d316b24763d678d44d687797c28 (diff) | |
download | gitlab-ce-9b1ce4081a52d259c18325ea27501934eb2dfc59.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-5-stable-ee
-rw-r--r-- | app/workers/all_queues.yml | 9 | ||||
-rw-r--r-- | app/workers/concerns/dependency_proxy/expireable.rb | 17 | ||||
-rw-r--r-- | app/workers/dependency_proxy/cleanup_dependency_proxy_worker.rb | 28 | ||||
-rw-r--r-- | app/workers/dependency_proxy/image_ttl_group_policy_worker.rb | 26 | ||||
-rw-r--r-- | app/workers/purge_dependency_proxy_cache_worker.rb | 5 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 3 | ||||
-rw-r--r-- | doc/api/dependency_proxy.md | 3 | ||||
-rw-r--r-- | lib/api/dependency_proxy.rb | 12 | ||||
-rw-r--r-- | spec/requests/api/dependency_proxy_spec.rb | 22 | ||||
-rw-r--r-- | spec/workers/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb | 34 | ||||
-rw-r--r-- | spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb | 16 | ||||
-rw-r--r-- | spec/workers/purge_dependency_proxy_cache_worker_spec.rb | 31 |
12 files changed, 113 insertions, 93 deletions
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 699744b355c..1441bcfa402 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -291,6 +291,15 @@ :weight: 1 :idempotent: true :tags: [] +- :name: cronjob:dependency_proxy_cleanup_dependency_proxy + :worker_name: DependencyProxy::CleanupDependencyProxyWorker + :feature_category: :dependency_proxy + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:dependency_proxy_image_ttl_group_policy :worker_name: DependencyProxy::ImageTtlGroupPolicyWorker :feature_category: :dependency_proxy diff --git a/app/workers/concerns/dependency_proxy/expireable.rb b/app/workers/concerns/dependency_proxy/expireable.rb new file mode 100644 index 00000000000..9650ac85c6c --- /dev/null +++ b/app/workers/concerns/dependency_proxy/expireable.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module DependencyProxy + module Expireable + extend ActiveSupport::Concern + + UPDATE_BATCH_SIZE = 100 + + private + + def expire_artifacts(collection) + collection.each_batch(of: UPDATE_BATCH_SIZE) do |batch| + batch.update_all(status: :expired) + end + end + end +end diff --git a/app/workers/dependency_proxy/cleanup_dependency_proxy_worker.rb b/app/workers/dependency_proxy/cleanup_dependency_proxy_worker.rb new file mode 100644 index 00000000000..d77c782267a --- /dev/null +++ b/app/workers/dependency_proxy/cleanup_dependency_proxy_worker.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module DependencyProxy + class CleanupDependencyProxyWorker + include ApplicationWorker + include CronjobQueue # rubocop:disable Scalability/CronWorkerContext + + data_consistency :always + idempotent! + + feature_category :dependency_proxy + + def perform + enqueue_blob_cleanup_job if DependencyProxy::Blob.expired.any? + enqueue_manifest_cleanup_job if DependencyProxy::Manifest.expired.any? + end + + private + + def enqueue_blob_cleanup_job + DependencyProxy::CleanupBlobWorker.perform_with_capacity + end + + def enqueue_manifest_cleanup_job + DependencyProxy::CleanupManifestWorker.perform_with_capacity + end + end +end diff --git a/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb b/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb index 6a1de00ce80..3de2364fc71 100644 --- a/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb +++ b/app/workers/dependency_proxy/image_ttl_group_policy_worker.rb @@ -4,20 +4,19 @@ module DependencyProxy class ImageTtlGroupPolicyWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker include CronjobQueue # rubocop:disable Scalability/CronWorkerContext + include DependencyProxy::Expireable data_consistency :always feature_category :dependency_proxy - UPDATE_BATCH_SIZE = 100 - def perform DependencyProxy::ImageTtlGroupPolicy.enabled.each do |policy| qualified_blobs = policy.group.dependency_proxy_blobs.active.read_before(policy.ttl) qualified_manifests = policy.group.dependency_proxy_manifests.active.read_before(policy.ttl) - enqueue_blob_cleanup_job if expire_artifacts(qualified_blobs, DependencyProxy::Blob) - enqueue_manifest_cleanup_job if expire_artifacts(qualified_manifests, DependencyProxy::Manifest) + expire_artifacts(qualified_blobs) + expire_artifacts(qualified_manifests) end log_counts @@ -25,25 +24,6 @@ module DependencyProxy private - def expire_artifacts(artifacts, model) - rows_updated = false - - artifacts.each_batch(of: UPDATE_BATCH_SIZE) do |batch| - rows = batch.update_all(status: :expired) - rows_updated ||= rows > 0 - end - - rows_updated - end - - def enqueue_blob_cleanup_job - DependencyProxy::CleanupBlobWorker.perform_with_capacity - end - - def enqueue_manifest_cleanup_job - DependencyProxy::CleanupManifestWorker.perform_with_capacity - end - def log_counts use_replica_if_available do expired_blob_count = DependencyProxy::Blob.expired.count diff --git a/app/workers/purge_dependency_proxy_cache_worker.rb b/app/workers/purge_dependency_proxy_cache_worker.rb index db43e4adf20..c0ddf190210 100644 --- a/app/workers/purge_dependency_proxy_cache_worker.rb +++ b/app/workers/purge_dependency_proxy_cache_worker.rb @@ -2,6 +2,7 @@ class PurgeDependencyProxyCacheWorker include ApplicationWorker + include DependencyProxy::Expireable data_consistency :always @@ -18,8 +19,8 @@ class PurgeDependencyProxyCacheWorker return unless valid? - @group.dependency_proxy_blobs.destroy_all # rubocop:disable Cop/DestroyAll - @group.dependency_proxy_manifests.destroy_all # rubocop:disable Cop/DestroyAll + expire_artifacts(@group.dependency_proxy_blobs) + expire_artifacts(@group.dependency_proxy_manifests) end private diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 8fb2161b14e..58ff11737d6 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -534,6 +534,9 @@ Settings.cron_jobs['container_expiration_policy_worker']['job_class'] = 'Contain Settings.cron_jobs['image_ttl_group_policy_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['image_ttl_group_policy_worker']['cron'] ||= '40 0 * * *' Settings.cron_jobs['image_ttl_group_policy_worker']['job_class'] = 'DependencyProxy::ImageTtlGroupPolicyWorker' +Settings.cron_jobs['cleanup_dependency_proxy_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['cleanup_dependency_proxy_worker']['cron'] ||= '20 3,15 * * *' +Settings.cron_jobs['cleanup_dependency_proxy_worker']['job_class'] = 'DependencyProxy::CleanupDependencyProxyWorker' Settings.cron_jobs['x509_issuer_crl_check_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['x509_issuer_crl_check_worker']['cron'] ||= '30 1 * * *' Settings.cron_jobs['x509_issuer_crl_check_worker']['job_class'] = 'X509IssuerCrlCheckWorker' diff --git a/doc/api/dependency_proxy.md b/doc/api/dependency_proxy.md index 535c6607cad..5401c007c0d 100644 --- a/doc/api/dependency_proxy.md +++ b/doc/api/dependency_proxy.md @@ -11,7 +11,8 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/11631) in GitLab 12.10. > - [Moved](https://gitlab.com/gitlab-org/gitlab/-/issues/273655) from GitLab Premium to GitLab Free in 13.6. -Deletes the cached manifests and blobs for a group. This endpoint requires the [Owner role](../user/permissions.md) +Schedules for deletion the cached manifests and blobs for a group. This endpoint requires the +[Owner role](../user/permissions.md) for the group. ```plaintext diff --git a/lib/api/dependency_proxy.rb b/lib/api/dependency_proxy.rb index 185b8d5a15d..9d0b1bf4423 100644 --- a/lib/api/dependency_proxy.rb +++ b/lib/api/dependency_proxy.rb @@ -6,15 +6,6 @@ module API feature_category :dependency_proxy - helpers do - def obtain_new_purge_cache_lease - Gitlab::ExclusiveLease - .new("dependency_proxy:delete_group_blobs:#{user_group.id}", - timeout: 1.hour) - .try_obtain - end - end - after_validation do authorize! :admin_group, user_group end @@ -29,9 +20,6 @@ module API delete ':id/dependency_proxy/cache' do not_found! unless user_group.dependency_proxy_feature_available? - message = 'This request has already been made. It may take some time to purge the cache. You can run this at most once an hour for a given group' - render_api_error!(message, 409) unless obtain_new_purge_cache_lease - # rubocop:disable CodeReuse/Worker PurgeDependencyProxyCacheWorker.perform_async(current_user.id, user_group.id) # rubocop:enable CodeReuse/Worker diff --git a/spec/requests/api/dependency_proxy_spec.rb b/spec/requests/api/dependency_proxy_spec.rb index 2837d1c02c4..067852ef1e9 100644 --- a/spec/requests/api/dependency_proxy_spec.rb +++ b/spec/requests/api/dependency_proxy_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe API::DependencyProxy, api: true do - include ExclusiveLeaseHelpers - let_it_be(:user) { create(:user) } let_it_be(:blob) { create(:dependency_proxy_blob )} let_it_be(:group, reload: true) { blob.group } @@ -20,11 +18,8 @@ RSpec.describe API::DependencyProxy, api: true do shared_examples 'responding to purge requests' do context 'with feature available and enabled' do - let_it_be(:lease_key) { "dependency_proxy:delete_group_blobs:#{group.id}" } - context 'an admin user' do it 'deletes the blobs and returns no content' do - stub_exclusive_lease(lease_key, timeout: 1.hour) expect(PurgeDependencyProxyCacheWorker).to receive(:perform_async) subject @@ -32,23 +27,6 @@ RSpec.describe API::DependencyProxy, api: true do expect(response).to have_gitlab_http_status(:accepted) expect(response.body).to eq('202') end - - context 'called multiple times in one hour', :clean_gitlab_redis_shared_state do - it 'returns 409 with an error message' do - stub_exclusive_lease_taken(lease_key, timeout: 1.hour) - - subject - - expect(response).to have_gitlab_http_status(:conflict) - expect(response.body).to include('This request has already been made.') - end - - it 'executes service only for the first time' do - expect(PurgeDependencyProxyCacheWorker).to receive(:perform_async).once - - 2.times { subject } - end - end end context 'a non-admin' do diff --git a/spec/workers/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb b/spec/workers/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb new file mode 100644 index 00000000000..ed0bdefbdb8 --- /dev/null +++ b/spec/workers/dependency_proxy/cleanup_dependency_proxy_worker_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe DependencyProxy::CleanupDependencyProxyWorker do + describe '#perform' do + subject { described_class.new.perform } + + context 'when there are records to be deleted' do + it_behaves_like 'an idempotent worker' do + it 'queues the cleanup jobs', :aggregate_failures do + create(:dependency_proxy_blob, :expired) + create(:dependency_proxy_manifest, :expired) + + expect(DependencyProxy::CleanupBlobWorker).to receive(:perform_with_capacity).twice + expect(DependencyProxy::CleanupManifestWorker).to receive(:perform_with_capacity).twice + + subject + end + end + end + + context 'when there are not records to be deleted' do + it_behaves_like 'an idempotent worker' do + it 'does not queue the cleanup jobs', :aggregate_failures do + expect(DependencyProxy::CleanupBlobWorker).not_to receive(:perform_with_capacity) + expect(DependencyProxy::CleanupManifestWorker).not_to receive(:perform_with_capacity) + + subject + end + end + end + end +end diff --git a/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb b/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb index ae0cb097ebf..b035a2ec0b7 100644 --- a/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb +++ b/spec/workers/dependency_proxy/image_ttl_group_policy_worker_spec.rb @@ -17,13 +17,6 @@ RSpec.describe DependencyProxy::ImageTtlGroupPolicyWorker do let_it_be_with_reload(:new_blob) { create(:dependency_proxy_blob, group: group) } let_it_be_with_reload(:new_manifest) { create(:dependency_proxy_manifest, group: group) } - it 'calls the limited capacity workers', :aggregate_failures do - expect(DependencyProxy::CleanupBlobWorker).to receive(:perform_with_capacity) - expect(DependencyProxy::CleanupManifestWorker).to receive(:perform_with_capacity) - - subject - end - it 'updates the old images to expired' do expect { subject } .to change { old_blob.reload.status }.from('default').to('expired') @@ -33,15 +26,6 @@ RSpec.describe DependencyProxy::ImageTtlGroupPolicyWorker do end end - context 'when there are no images to expire' do - it 'does not do anything', :aggregate_failures do - expect(DependencyProxy::CleanupBlobWorker).not_to receive(:perform_with_capacity) - expect(DependencyProxy::CleanupManifestWorker).not_to receive(:perform_with_capacity) - - subject - end - end - context 'counts logging' do let_it_be(:expired_blob) { create(:dependency_proxy_blob, :expired, group: group) } let_it_be(:expired_blob2) { create(:dependency_proxy_blob, :expired, group: group) } diff --git a/spec/workers/purge_dependency_proxy_cache_worker_spec.rb b/spec/workers/purge_dependency_proxy_cache_worker_spec.rb index 393745958be..b928104fb58 100644 --- a/spec/workers/purge_dependency_proxy_cache_worker_spec.rb +++ b/spec/workers/purge_dependency_proxy_cache_worker_spec.rb @@ -4,18 +4,18 @@ require 'spec_helper' RSpec.describe PurgeDependencyProxyCacheWorker do let_it_be(:user) { create(:admin) } - let_it_be(:blob) { create(:dependency_proxy_blob )} - let_it_be(:group, reload: true) { blob.group } - let_it_be(:manifest) { create(:dependency_proxy_manifest, group: group )} + let_it_be_with_refind(:blob) { create(:dependency_proxy_blob )} + let_it_be_with_reload(:group) { blob.group } + let_it_be_with_refind(:manifest) { create(:dependency_proxy_manifest, group: group )} let_it_be(:group_id) { group.id } subject { described_class.new.perform(user.id, group_id) } describe '#perform' do - shared_examples 'not removing blobs and manifests' do - it 'does not remove blobs and manifests', :aggregate_failures do - expect { subject }.not_to change { group.dependency_proxy_blobs.size } - expect { subject }.not_to change { group.dependency_proxy_manifests.size } + shared_examples 'not expiring blobs and manifests' do + it 'does not expire blobs and manifests', :aggregate_failures do + expect { subject }.not_to change { blob.status } + expect { subject }.not_to change { manifest.status } expect(subject).to be_nil end end @@ -25,39 +25,36 @@ RSpec.describe PurgeDependencyProxyCacheWorker do include_examples 'an idempotent worker' do let(:job_args) { [user.id, group_id] } - it 'deletes the blobs and returns ok', :aggregate_failures do - expect(group.dependency_proxy_blobs.size).to eq(1) - expect(group.dependency_proxy_manifests.size).to eq(1) - + it 'expires the blobs and returns ok', :aggregate_failures do subject - expect(group.dependency_proxy_blobs.size).to eq(0) - expect(group.dependency_proxy_manifests.size).to eq(0) + expect(blob).to be_expired + expect(manifest).to be_expired end end end context 'when admin mode is disabled' do - it_behaves_like 'not removing blobs and manifests' + it_behaves_like 'not expiring blobs and manifests' end end context 'a non-admin user' do let(:user) { create(:user) } - it_behaves_like 'not removing blobs and manifests' + it_behaves_like 'not expiring blobs and manifests' end context 'an invalid user id' do let(:user) { double('User', id: 99999 ) } - it_behaves_like 'not removing blobs and manifests' + it_behaves_like 'not expiring blobs and manifests' end context 'an invalid group' do let(:group_id) { 99999 } - it_behaves_like 'not removing blobs and manifests' + it_behaves_like 'not expiring blobs and manifests' end end end |