Audit events for package deletion
Context
GitLab provides audit events, which allow users to track a variety of different actions within GitLab. In Add the Package Registry to the list of audited... (#329588), it was proposed to implement audit events for the package registry. The scope of the implementation is detailed in this comment. This MR is the 2nd part of the implementation: Add audit logs for Package destruction.
What does this MR do and why?
-
When a package of any format is deleted from the package registry, an audit event is created. The audit events is a GitLab Premium feature, so its code lives in the
ee
folder. -
In the package registry, packages are destroyed in two fashions:
- Single package destruction through the
Packages::MarkPackageForDestructionService
. - Bulk package destruction through the
Packages::MarkPackagesForDestructionService
.
In those two services, the packages'
status
is updated topending_destruction
, and then a background job should pick them up and call.destroy!
on each package.So, in order to be able to send the audit event, we have to hook into each service's
execute
method instead of using a model's callback for two main reasons:- The user who is performing the destruction is a very important piece of info in the audit event, and we can have access to the
current_user
only in the services. - In the
Packages::MarkPackagesForDestructionService
bulk action, the packages'status
update is done using.update_all
, which doesn't trigger any model callbacks.
- Single package destruction through the
-
To send audit events for the
Packages::MarkPackagesForDestructionService
bulk destruction, I needed to apply some changes on how we could utilizeAuditable#push_audit_event
to store multiple events in the::Gitlab::Audit::EventQueue
and create them in bulk. This is faster and more efficient than iterating over destroyed packages and create events one by one. -
The audit events are saved on the direct parent group. So when a package is destroyed in a project, the event will be available/visible in the parent group of the project. In case the project doesn't have a parent group (belongs to a user namespace), the events will be available in the project.
-
The feature is behind a
WIP
feature flagpackage_registry_audit_events
, so that we can add the rest of the implementation behind the same feature flag. -
The implementation is guided by this documentation page: https://docs.gitlab.com/ee/development/audit_event_guide/#how-to-instrument-new-audit-events.
References
Please include cross-links to any resources that are relevant to this MR. This will give reviewers and future readers helpful context to give an efficient review of the changes introduced.
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Screenshots or screen recordings
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
- Make sure the feature flag
package_registry_audit_events
is enabled. - Make sure your GDK has an enterprise licence: https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/index.md#use-gitlab-enterprise-features
- Create a couple of packages from rails console:
# stub file upload def fixture_file_upload(*args, **kwargs) Rack::Test::UploadedFile.new(*args, **kwargs) end FactoryBot.create(:npm_package, project: Project.find(<project_id>)) FactoryBot.create(:generic_package, project: Project.find(<project_id>)) FactoryBot.create(:nuget_package, project: Project.find(<project_id>))
- Navigate to the group or project package registry UI page, and delete packages in bulk.
- Navigate to the group audit events page in the UI. Each destroyed package should have an audit event on the page.
Related to #329588
Merge request reports
Activity
changed milestone to %17.9
assigned to @mkhalifa3
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels
added backend documentation labels
- Resolved by David Fernandez
5 Warnings 8de8e981: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 44f51ee0: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. d76c8321: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 2206b949: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. ffd3829d: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 2 Messages CHANGELOG missing: If this merge request needs a changelog entry, add the
Changelog
trailer to the commit message you want to add to the changelog.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
This merge request adds or changes documentation files and requires Technical Writing review. The review should happen before merge, but can be post-merge if the merge request is time sensitive. Documentation review
The following files require a review from a technical writer:
-
doc/user/compliance/audit_event_types.md
(Link to current live version)
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Category Reviewer Maintainer backend @aluthra2
(UTC+5.5, 4.5 hours ahead of author)
@radbatnag
(UTC+8, 7 hours ahead of author)
groupcompliance Reviewer review is optional for groupcompliance @xanf
(UTC+2, 1 hour ahead of author)
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by ****-
- Resolved by David Fernandez
added 877 commits
-
bac9e34a...2131648a - 876 commits from branch
329588-add-the-package-registry-to-the-list-of-audited-events
- a67b49b4 - Audit events for package deletion
-
bac9e34a...2131648a - 876 commits from branch
mentioned in issue #329588
- Resolved by David Fernandez
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-cng:
test report for 8de8e981expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Create | 140 | 0 | 22 | 0 | 162 | ✅ | | Package | 29 | 0 | 15 | 0 | 44 | ✅ | | Verify | 52 | 0 | 20 | 0 | 72 | ✅ | | Govern | 84 | 0 | 10 | 0 | 94 | ✅ | | Ai-powered | 0 | 0 | 2 | 0 | 2 | ➖ | | Configure | 0 | 0 | 3 | 0 | 3 | ➖ | | Data Stores | 33 | 0 | 10 | 0 | 43 | ✅ | | Plan | 86 | 0 | 8 | 0 | 94 | ✅ | | Manage | 1 | 0 | 9 | 0 | 10 | ✅ | | Fulfillment | 2 | 0 | 7 | 0 | 9 | ✅ | | Monitor | 8 | 0 | 12 | 0 | 20 | ✅ | | ModelOps | 0 | 0 | 1 | 0 | 1 | ➖ | | Analytics | 2 | 0 | 0 | 0 | 2 | ✅ | | Secure | 3 | 0 | 5 | 0 | 8 | ✅ | | Release | 5 | 0 | 1 | 0 | 6 | ✅ | | Growth | 0 | 0 | 2 | 0 | 2 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 445 | 0 | 127 | 0 | 572 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-test-on-gdk:
test report for 8de8e981expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Verify | 46 | 0 | 2 | 0 | 48 | ✅ | | Create | 107 | 0 | 3 | 0 | 110 | ✅ | | Plan | 16 | 0 | 0 | 0 | 16 | ✅ | | Govern | 56 | 0 | 3 | 0 | 59 | ✅ | | Release | 3 | 0 | 0 | 0 | 3 | ✅ | | Package | 24 | 0 | 0 | 0 | 24 | ✅ | | Data Stores | 19 | 0 | 0 | 0 | 19 | ✅ | | Manage | 1 | 0 | 0 | 0 | 1 | ✅ | | Secure | 5 | 0 | 0 | 0 | 5 | ✅ | | Fulfillment | 1 | 0 | 0 | 0 | 1 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 278 | 0 | 8 | 0 | 286 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
Edited by ****added 217 commits
-
a67b49b4...71e46a08 - 216 commits from branch
329588-add-the-package-registry-to-the-list-of-audited-events
- 060d402c - Audit events for package deletion
-
a67b49b4...71e46a08 - 216 commits from branch
added 416 commits
-
ba054b65...19f7e392 - 415 commits from branch
329588-add-the-package-registry-to-the-list-of-audited-events
- a2c2e03b - Audit events for package deletion
-
ba054b65...19f7e392 - 415 commits from branch
added 222 commits
-
a2c2e03b...f285a08b - 220 commits from branch
329588-add-the-package-registry-to-the-list-of-audited-events
- effcef02 - Address maintainer feedback
- 95d0d09f - Audit events for package deletion
-
a2c2e03b...f285a08b - 220 commits from branch
added 2679 commits
-
95d0d09f...f898d8f8 - 2671 commits from branch
master
- 3af9ec39 - Send audit event for packages creation
- 9be937c1 - Apply backend reviewer feedback
- db117df9 - Move event yml to ee
- b4c4641a - Avoid creating event in transaction
- 719cabf4 - Use model callback instead of services to send audit events
- f75ba248 - Address maintainer feedback
- 64f7437b - Address maintainer feedback
- abb8d618 - Audit events for package deletion
Toggle commit list-
95d0d09f...f898d8f8 - 2671 commits from branch
requested review from @xanf and @ddieulivol
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez
Hi @ddieulivol
Are you available for the initial backend review here? Please pass it to
@10io
for the maintainer review once you're happy with the MR.--
Hi @xanf
Could you please do the groupcompliance review here?
Edited by Moaz Khalifa
requested review from @10io
added pipeline:mr-approved label
changed milestone to %17.10
added missed:17.9 label
added 1016 commits
-
abb8d618...5ab5bf8f - 1008 commits from branch
master
- dab487d8 - Send audit event for packages creation
- 15f3e234 - Apply backend reviewer feedback
- 54e97879 - Move event yml to ee
- a1d78af4 - Avoid creating event in transaction
- 6322a084 - Use model callback instead of services to send audit events
- 2c596c37 - Address maintainer feedback
- 99f2ebf2 - Address maintainer feedback
- 4ce7f593 - Audit events for package deletion
Toggle commit list-
abb8d618...5ab5bf8f - 1008 commits from branch
reset approvals from @ddieulivol by pushing to the branch
- Resolved by David Fernandez
added workflowin review label and removed workflowin dev label
removed review request for @ddieulivol
- Resolved by David Fernandez
- Resolved by David Fernandez
requested review from @10io
requested review from @huzaifaiftikhar1 and removed review request for @xanf
- Resolved by David Fernandez
- Resolved by David Fernandez
- Resolved by David Fernandez