From fa5ebf36bd172be2771a5b0191f7da0c706aaead Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Wed, 22 Oct 2025 19:06:13 +0200 Subject: [PATCH] Log audit events for virtual registry cleanup policy --- doc/user/compliance/audit_event_types.md | 6 ++ .../container/cache/entry.rb | 1 + .../packages/maven/cache/entry.rb | 1 + .../cleanup/create_audit_events_service.rb | 60 ++++++++++++++ .../cleanup/execute_policy_service.rb | 24 ++++-- .../virtual_registry_cache_entry_deleted.yml | 10 +++ .../container/cache/entry_spec.rb | 2 + .../packages/maven/cache/entry_spec.rb | 1 + .../create_audit_events_service_spec.rb | 81 +++++++++++++++++++ .../cleanup/execute_policy_service_spec.rb | 32 +++++++- 10 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 ee/app/services/virtual_registries/cleanup/create_audit_events_service.rb create mode 100644 ee/config/audit_events/types/virtual_registry_cache_entry_deleted.yml create mode 100644 ee/spec/services/virtual_registries/cleanup/create_audit_events_service_spec.rb diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 6257492f7ea317..f2f6cd864881ff 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -742,6 +742,12 @@ Audit event types belong to the following product categories. | [`secure_ci_job_token_project_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115350) | Project added to inbound CI_JOB_TOKEN scope | {{< icon name="check-circle" >}} Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338255) | Project | | [`secure_ci_job_token_project_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115350) | Project removed from inbound CI_JOB_TOKEN scope | {{< icon name="check-circle" >}} Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338255) | Project | +### Virtual registry + +| Type name | Event triggered when | Saved to database | Introduced in | Scope | +|:----------|:---------------------|:------------------|:--------------|:------| +| [`virtual_registry_cache_entry_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209828) | A virtual registry cache entry got deleted | {{< icon name="check-circle" >}} Yes | GitLab [18.6](https://gitlab.com/gitlab-org/gitlab/-/issues/548566) | Group | + ### Vulnerability management | Type name | Event triggered when | Saved to database | Introduced in | Scope | diff --git a/ee/app/models/virtual_registries/container/cache/entry.rb b/ee/app/models/virtual_registries/container/cache/entry.rb index d8d68f1b00d860..b77b9471131eeb 100644 --- a/ee/app/models/virtual_registries/container/cache/entry.rb +++ b/ee/app/models/virtual_registries/container/cache/entry.rb @@ -8,6 +8,7 @@ class Entry < ApplicationRecord include Gitlab::SQL::Pattern include ShaAttribute include CounterAttribute + include ::Auditable self.primary_key = %i[upstream_id relative_path status] diff --git a/ee/app/models/virtual_registries/packages/maven/cache/entry.rb b/ee/app/models/virtual_registries/packages/maven/cache/entry.rb index f6b8659965eab2..df7f9cb73f9a3e 100644 --- a/ee/app/models/virtual_registries/packages/maven/cache/entry.rb +++ b/ee/app/models/virtual_registries/packages/maven/cache/entry.rb @@ -10,6 +10,7 @@ class Entry < ApplicationRecord include ::UpdateNamespaceStatistics include ShaAttribute include CounterAttribute + include ::Auditable self.primary_key = %i[upstream_id relative_path status] diff --git a/ee/app/services/virtual_registries/cleanup/create_audit_events_service.rb b/ee/app/services/virtual_registries/cleanup/create_audit_events_service.rb new file mode 100644 index 00000000000000..9f83eddf627b8a --- /dev/null +++ b/ee/app/services/virtual_registries/cleanup/create_audit_events_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module VirtualRegistries + module Cleanup + class CreateAuditEventsService + include Gitlab::Utils::StrongMemoize + + EVENT_NAME = 'virtual_registry_cache_entry_deleted' + EVENT_MESSAGE = 'Marked cache entry for deletion' + DELETION_REGEX = %r{/deleted/.*} + + def initialize(group:, upstream:, model:, destroyed_paths:) + @group = group + @upstream = upstream + @model = model + @destroyed_paths = destroyed_paths + end + + def execute + return ServiceResponse.error(message: 'No entries to audit') if destroyed_paths.empty? + + ::Gitlab::Audit::Auditor.audit(initial_audit_context) do + destroyed_paths.each { |path| send_event(path) } + end + + ServiceResponse.success + rescue StandardError => e + ServiceResponse.error(message: e.message) + end + + private + + attr_reader :group, :upstream, :model, :destroyed_paths + + def initial_audit_context + { + name: EVENT_NAME, + author: group.first_owner || ::Gitlab::Audit::UnauthenticatedAuthor.new, + scope: group, + target: ::Gitlab::Audit::NullTarget.new + } + end + + def send_event(destroyed_path) + event = { + target: cache_entry.tap { |ce| ce.relative_path = destroyed_path.sub(DELETION_REGEX, '') }, + target_details: "#{cache_entry.relative_path} marked for deletion by cleanup policy", + message: EVENT_MESSAGE + } + + cache_entry.push_audit_event(event, after_commit: false) + end + + def cache_entry + model.new(group:, upstream:) + end + strong_memoize_attr :cache_entry + end + end +end diff --git a/ee/app/services/virtual_registries/cleanup/execute_policy_service.rb b/ee/app/services/virtual_registries/cleanup/execute_policy_service.rb index a9f210294b99cf..7870d5eac65f85 100644 --- a/ee/app/services/virtual_registries/cleanup/execute_policy_service.rb +++ b/ee/app/services/virtual_registries/cleanup/execute_policy_service.rb @@ -42,26 +42,25 @@ def process_upstream(upstream, key) .requiring_cleanup(policy.keep_n_days_after_download) .each_batch(of: BATCH_SIZE, column: :relative_path) do |batch| result = mark_batch_for_destruction(batch) + log_audit_events(upstream, batch.model, result.map(&:last)) counts[key][:deleted_entries_count] += result.size - counts[key][:deleted_size] += result.sum + counts[key][:deleted_size] += result.sum(&:first) end end def mark_batch_for_destruction(batch) sql = build_update_sql(batch) - batch.connection.query_values(sql) + batch.connection.select_rows(sql) end def build_update_sql(batch) - table = batch.arel_table - update_manager = build_update_manager(batch, table) - returning = Arel::Nodes::Grouping.new(table[:size]) - - "#{batch.connection.to_sql(update_manager)} RETURNING #{returning.to_sql}" + update_manager = build_update_manager(batch) + "#{batch.connection.to_sql(update_manager)} RETURNING size, relative_path" end - def build_update_manager(batch, table) + def build_update_manager(batch) + table = batch.arel_table Arel::UpdateManager.new(table).tap do |manager| manager.set([ [table[:status], batch.model.statuses[:pending_destruction]], @@ -71,6 +70,15 @@ def build_update_manager(batch, table) manager.wheres = batch.arel.constraints end end + + def log_audit_events(upstream, model, destroyed_paths) + return if destroyed_paths.empty? + + group = policy.group + ::VirtualRegistries::Cleanup::CreateAuditEventsService + .new(group:, upstream:, model:, destroyed_paths:) + .execute + end end end end diff --git a/ee/config/audit_events/types/virtual_registry_cache_entry_deleted.yml b/ee/config/audit_events/types/virtual_registry_cache_entry_deleted.yml new file mode 100644 index 00000000000000..16fdd907fb78eb --- /dev/null +++ b/ee/config/audit_events/types/virtual_registry_cache_entry_deleted.yml @@ -0,0 +1,10 @@ +--- +name: virtual_registry_cache_entry_deleted +description: A virtual registry cache entry got deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/548566 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209828 +feature_category: virtual_registry +milestone: '18.6' +saved_to_database: true +streamed: true +scope: [Group] diff --git a/ee/spec/models/virtual_registries/container/cache/entry_spec.rb b/ee/spec/models/virtual_registries/container/cache/entry_spec.rb index 420aa6c888a7a0..e0932dacd17729 100644 --- a/ee/spec/models/virtual_registries/container/cache/entry_spec.rb +++ b/ee/spec/models/virtual_registries/container/cache/entry_spec.rb @@ -5,6 +5,8 @@ RSpec.describe VirtualRegistries::Container::Cache::Entry, feature_category: :virtual_registry do subject(:cache_entry) { build(:virtual_registries_container_cache_entry) } + it { is_expected.to include_module(::Auditable) } + describe 'validations' do %i[group file file_sha1 relative_path size].each do |attr| it { is_expected.to validate_presence_of(attr) } diff --git a/ee/spec/models/virtual_registries/packages/maven/cache/entry_spec.rb b/ee/spec/models/virtual_registries/packages/maven/cache/entry_spec.rb index c05fa0d72d5867..3ad74d545b066e 100644 --- a/ee/spec/models/virtual_registries/packages/maven/cache/entry_spec.rb +++ b/ee/spec/models/virtual_registries/packages/maven/cache/entry_spec.rb @@ -7,6 +7,7 @@ it { is_expected.to include_module(FileStoreMounter) } it { is_expected.to include_module(::UpdateNamespaceStatistics) } + it { is_expected.to include_module(::Auditable) } it_behaves_like 'updates namespace statistics' do let(:statistic_source) { cache_entry } diff --git a/ee/spec/services/virtual_registries/cleanup/create_audit_events_service_spec.rb b/ee/spec/services/virtual_registries/cleanup/create_audit_events_service_spec.rb new file mode 100644 index 00000000000000..4996c0610219d4 --- /dev/null +++ b/ee/spec/services/virtual_registries/cleanup/create_audit_events_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe VirtualRegistries::Cleanup::CreateAuditEventsService, feature_category: :virtual_registry do + let(:user) { build_stubbed(:user) } + let(:group) { build_stubbed(:group) } + let(:upstream) { build_stubbed(:virtual_registries_container_upstream) } + let(:model) { VirtualRegistries::Container::Cache::Entry } + let(:destroyed_paths) { [] } + let(:service) { described_class.new(group:, upstream:, model:, destroyed_paths:) } + + describe '#execute' do + subject(:execute) { service.execute } + + context 'when no paths are destroyed' do + it { is_expected.to be_error.and have_attributes(message: 'No entries to audit') } + + it 'does not create audit events' do + expect { execute }.not_to change { AuditEvent.count } + end + end + + context 'when paths are destroyed', :request_store do + let(:destroyed_paths) do + [ + '/path/to/image/deleted/uuid-1', + '/another/path/deleted/uuid-2', + '/third/path/deleted/uuid-3' + ] + end + + let(:operation) { execute } + let(:event_type) { 'virtual_registry_cache_entry_deleted' } + let(:event_count) { destroyed_paths.size } + let(:fail_condition!) { allow(destroyed_paths).to receive(:empty?).and_return(true) } + + let(:attributes) do + destroyed_paths.map do |path| + relative_path = path.sub(described_class::DELETION_REGEX, '') + { + author_id: user.id, + entity_id: group.id, + entity_type: group.class.name, + details: { + author_name: user.name, + author_class: user.class.name, + event_name: event_type, + target_id: service.send(:cache_entry).id, + target_type: model.name, + target_details: "#{relative_path} marked for deletion by cleanup policy", + custom_message: 'Marked cache entry for deletion' + } + } + end + end + + before do + allow(group).to receive(:first_owner).and_return(user) + end + + it { is_expected.to be_success } + + include_examples 'audit event logging' + + context 'when an error occurs during auditing' do + before do + allow_next_instance_of(Gitlab::Audit::Auditor) do |instance| + allow(instance).to receive(:build_event).and_raise(StandardError, 'audit failure') + end + end + + it { is_expected.to be_error.and have_attributes(message: 'audit failure') } + + it 'does not create audit events' do + expect { execute }.not_to change { AuditEvent.count } + end + end + end + end +end diff --git a/ee/spec/services/virtual_registries/cleanup/execute_policy_service_spec.rb b/ee/spec/services/virtual_registries/cleanup/execute_policy_service_spec.rb index ba7be30146fd76..f214387aea7139 100644 --- a/ee/spec/services/virtual_registries/cleanup/execute_policy_service_spec.rb +++ b/ee/spec/services/virtual_registries/cleanup/execute_policy_service_spec.rb @@ -77,10 +77,18 @@ ) end - it 'marks old entries for destruction and returns correct counts', :aggregate_failures do + before do + allow(::VirtualRegistries::Cleanup::CreateAuditEventsService).to receive(:new).and_call_original + end + + it 'marks old entries for destruction and returns correct counts, and creates audit events', + :aggregate_failures do expect { execute }.to change { maven_upstream.cache_entries.pending_destruction.count }.by(2) .and change { container_upstream.cache_entries.pending_destruction.count }.by(1) + expect_audit_events_for(maven_upstream, [old_maven_entry1, old_maven_entry2]) + expect_audit_events_for(container_upstream, [old_container_entry]) + is_expected.to be_success.and have_attributes( payload: { maven: { deleted_entries_count: 2, deleted_size: 3072 }, @@ -130,12 +138,21 @@ ) end - it 'processes entries from all upstreams', :aggregate_failures do + before do + allow(::VirtualRegistries::Cleanup::CreateAuditEventsService).to receive(:new).and_call_original + end + + it 'processes entries from all upstreams and creates audit events', :aggregate_failures do expect { execute }.to change { maven_upstream.cache_entries.pending_destruction.count }.by(1) .and change { maven_upstream2.cache_entries.pending_destruction.count }.by(1) .and change { container_upstream.cache_entries.pending_destruction.count }.by(1) .and change { container_upstream2.cache_entries.pending_destruction.count }.by(1) + expect_audit_events_for(maven_upstream, [maven_entry_upstream1]) + expect_audit_events_for(maven_upstream2, [maven_entry_upstream2]) + expect_audit_events_for(container_upstream, [container_entry_upstream1]) + expect_audit_events_for(container_upstream2, [container_entry_upstream2]) + is_expected.to be_success.and have_attributes( payload: { maven: { deleted_entries_count: 2, deleted_size: 3072 }, @@ -179,5 +196,16 @@ it { is_expected.to be_error.and have_attributes(message: /Database error/) } end end + + def expect_audit_events_for(upstream, entries) + expect(::VirtualRegistries::Cleanup::CreateAuditEventsService).to have_received(:new).once.with( + group: group, + upstream: upstream, + model: entries.first.class, + destroyed_paths: array_including( + *entries.map { |e| a_string_starting_with(e.relative_path) } + ) + ) + end end end -- GitLab