Skip to content
Snippets Groups Projects

Audit events for package deletion

Merged Moaz Khalifa requested to merge 329588-audit-events-for-package-deletion into master

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 to pending_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.
  • To send audit events for the Packages::MarkPackagesForDestructionService bulk destruction, I needed to apply some changes on how we could utilize Auditable#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 flag package_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

Screenshot_2025-02-12_at_12.44.50

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Make sure the feature flag package_registry_audit_events is enabled.
  2. 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
  3. 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>))
  4. Navigate to the group or project package registry UI page, and delete packages in bulk.
  5. Navigate to the group audit events page in the UI. Each destroyed package should have an audit event on the page.

Related to #329588

Edited by Moaz Khalifa

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 5 Warnings
    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning: 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
    :book: 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.

    :book: 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:

    The review does not need to block merging this merge request. See the:

    Reviewer roulette

    Category Reviewer Maintainer
    backend @aluthra2 profile link current availability (UTC+5.5, 4.5 hours ahead of author) @radbatnag profile link current availability (UTC+8, 7 hours ahead of author)
    groupcompliance Reviewer review is optional for groupcompliance @xanf profile link current availability (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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    Edited by ****
  • Moaz Khalifa added 877 commits

    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

    Compare with previous version

  • Moaz Khalifa mentioned in issue #329588

    mentioned in issue #329588

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-cng: :white_check_mark: test report for 8de8e981

    expand 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: :white_check_mark: test report for 8de8e981

    expand 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 ****
  • Moaz Khalifa added 217 commits

    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

    Compare with previous version

  • Moaz Khalifa added 1 commit

    added 1 commit

    • ba054b65 - Audit events for package deletion

    Compare with previous version

  • Moaz Khalifa added 416 commits

    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

    Compare with previous version

  • Moaz Khalifa added 222 commits

    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

    Compare with previous version

  • Moaz Khalifa added 2679 commits

    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

    Compare with previous version

  • Moaz Khalifa changed the description

    changed the description

  • Moaz Khalifa requested review from @xanf and @ddieulivol

    requested review from @xanf and @ddieulivol

  • Moaz Khalifa
  • Moaz Khalifa
  • Moaz Khalifa
  • Moaz Khalifa
  • David Dieulivol approved this merge request

    approved this merge request

  • David Dieulivol requested review from @10io

    requested review from @10io

  • 🤖 GitLab Bot 🤖 changed milestone to %17.10

    changed milestone to %17.10

  • Moaz Khalifa added 1016 commits

    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

    Compare with previous version

  • Moaz Khalifa reset approvals from @ddieulivol by pushing to the branch

    reset approvals from @ddieulivol by pushing to the branch

  • added workflowin review label and removed workflowin dev label

  • David Fernandez requested changes

    requested changes

  • Moaz Khalifa added 1 commit

    added 1 commit

    Compare with previous version

  • Moaz Khalifa added 1 commit

    added 1 commit

    Compare with previous version

  • David Dieulivol removed review request for @ddieulivol

    removed review request for @ddieulivol

  • Moaz Khalifa requested review from @10io

    requested review from @10io

  • Illya Klymov requested review from @huzaifaiftikhar1 and removed review request for @xanf

    requested review from @huzaifaiftikhar1 and removed review request for @xanf

  • David Fernandez requested changes

    requested changes

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading