From 13bf59cbb19927a5150076988528b9b88fcd9684 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Thu, 5 Sep 2024 10:08:37 +0200 Subject: [PATCH] Adds audit events for MlModelVersion Model version additions and removals need to be tracked for regulatory reasons. Changelog: added --- .../mutations/ml/model_versions/create.rb | 3 +- .../ml/create_model_version_service.rb | 14 ++ .../ml/destroy_model_version_service.rb | 13 + .../types/ml_model_version_created.yml | 9 + .../types/ml_model_version_destroyed.yml | 9 + doc/user/compliance/audit_event_types.md | 2 + lib/api/ml/mlflow/model_versions.rb | 3 +- .../ml/create_model_version_service_spec.rb | 231 ++++++++++-------- .../ml/destroy_model_version_service_spec.rb | 25 +- 9 files changed, 200 insertions(+), 109 deletions(-) create mode 100644 config/audit_events/types/ml_model_version_created.yml create mode 100644 config/audit_events/types/ml_model_version_destroyed.yml diff --git a/app/graphql/mutations/ml/model_versions/create.rb b/app/graphql/mutations/ml/model_versions/create.rb index adfeeaac4cc7d3..86066ad38deae4 100644 --- a/app/graphql/mutations/ml/model_versions/create.rb +++ b/app/graphql/mutations/ml/model_versions/create.rb @@ -39,7 +39,8 @@ def resolve(**args) result = ::Ml::CreateModelVersionService.new(model, { version: args[:version], - description: args[:description] + description: args[:description], + user: current_user } ).execute diff --git a/app/services/ml/create_model_version_service.rb b/app/services/ml/create_model_version_service.rb index 74490f000241ec..882cd0b1748519 100644 --- a/app/services/ml/create_model_version_service.rb +++ b/app/services/ml/create_model_version_service.rb @@ -43,6 +43,8 @@ def execute user: @user ) + audit_creation_event + ServiceResponse.success(message: [], payload: { model_version: @model_version }) end rescue ActiveRecord::RecordInvalid => e @@ -75,5 +77,17 @@ def initialize(errors) @errors = errors end end + + def audit_creation_event + audit_context = { + name: 'ml_model_version_created', + author: @user, + scope: @model.project, + target: @model_version, + message: "MlModelVersion #{@model_version.name}/#{@model_version.version} created" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end diff --git a/app/services/ml/destroy_model_version_service.rb b/app/services/ml/destroy_model_version_service.rb index 157cb6f2db9731..339c64474ee745 100644 --- a/app/services/ml/destroy_model_version_service.rb +++ b/app/services/ml/destroy_model_version_service.rb @@ -17,6 +17,7 @@ def execute end if model_version.destroy + audit_destroy_event ServiceResponse.success(payload: payload) else ServiceResponse.error(message: model_version.errors.full_messages, payload: payload) @@ -29,6 +30,18 @@ def payload { model_version: model_version } end + def audit_destroy_event + audit_context = { + name: 'ml_model_version_destroyed', + author: @user, + scope: model_version.project, + target: model_version, + message: "MlModelVersion #{model_version.name}/#{model_version.version} destroyed" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + attr_reader :model_version, :user end end diff --git a/config/audit_events/types/ml_model_version_created.yml b/config/audit_events/types/ml_model_version_created.yml new file mode 100644 index 00000000000000..6852561703bcda --- /dev/null +++ b/config/audit_events/types/ml_model_version_created.yml @@ -0,0 +1,9 @@ +name: ml_model_version_created +description: ML model version is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165145 +feature_category: mlops +milestone: '17.4' +saved_to_database: true +scope: [Project] +streamed: true diff --git a/config/audit_events/types/ml_model_version_destroyed.yml b/config/audit_events/types/ml_model_version_destroyed.yml new file mode 100644 index 00000000000000..471021c635771c --- /dev/null +++ b/config/audit_events/types/ml_model_version_destroyed.yml @@ -0,0 +1,9 @@ +name: ml_model_version_destroyed +description: ML model version destroyed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165145 +feature_category: mlops +milestone: '17.4' +saved_to_database: true +scope: [Project] +streamed: true diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 42ca0fcd07b5ef..f38d27ef7a4091 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -381,6 +381,8 @@ Audit event types belong to the following product categories. | [`project_feature_model_experiments_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121027) | Model experiments access level was updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.1](https://gitlab.com/gitlab-org/gitlab/-/issues/412384) | Project | | [`ml_model_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165011) | ML model is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/463215) | Project | | [`ml_model_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165011) | ML model destroyed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/463215) | Project | +| [`ml_model_version_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165145) | ML model version is created | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/463215) | Project | +| [`ml_model_version_destroyed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165145) | ML model version destroyed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/463215) | Project | | [`project_feature_model_registry_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138399) | Model registry access level was updated | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/issues/412734) | Project | ### Not categorized diff --git a/lib/api/ml/mlflow/model_versions.rb b/lib/api/ml/mlflow/model_versions.rb index 04c4f15f223b55..43ff89ab3d3990 100644 --- a/lib/api/ml/mlflow/model_versions.rb +++ b/lib/api/ml/mlflow/model_versions.rb @@ -36,7 +36,8 @@ class ModelVersions < ::API::Base model_name: params[:name], description: params[:description], metadata: params[:tags], - version: custom_version + version: custom_version, + user: current_user } ).execute diff --git a/spec/services/ml/create_model_version_service_spec.rb b/spec/services/ml/create_model_version_service_spec.rb index 1d840893bd8a46..dbfbdbc79cf2b9 100644 --- a/spec/services/ml/create_model_version_service_spec.rb +++ b/spec/services/ml/create_model_version_service_spec.rb @@ -3,147 +3,172 @@ require 'spec_helper' RSpec.describe ::Ml::CreateModelVersionService, feature_category: :mlops do - let(:model) { create(:ml_models) } + let_it_be(:model) { create(:ml_models) } + let_it_be(:project) { model.project } + let_it_be(:user) { project.owner } + let(:service) { described_class.new(model, params).execute } - let(:params) { {} } + let(:params) { { user: user } } + + let(:audit_event) do + { + name: 'ml_model_version_created', + author: user, + scope: project + } + end before do allow(Gitlab::InternalEvents).to receive(:track_event) + allow(Gitlab::Audit::Auditor).to receive(:audit).and_call_original end subject(:model_version) { service.payload[:model_version] } - context 'when no versions exist and no value is passed for version' do - it 'creates a model version', :aggregate_failures do - expect { service }.to change { Ml::ModelVersion.count }.by(1) - .and change { Ml::Candidate.count }.by(1) - .and change { Packages::MlModel::Package.count }.by(1) + describe '#execute', :aggregate_failures do + let(:audit_context) do + audit_event.merge(target: model_version, + message: "MlModelVersion #{model_version.name}/#{model_version.version} created") + end - expect(model.reload.latest_version.version).to eq('1.0.0') - expect(service).to be_success + context 'when no versions exist and no value is passed for version' do + it 'creates a model version' do + expect { service }.to change { Ml::ModelVersion.count }.by(1) + .and change { Ml::Candidate.count }.by(1) + .and change { Packages::MlModel::Package.count }.by(1) - expect(model.latest_version.package.name).to eq(model.name) - expect(model.latest_version.package.version).to eq('1.0.0') + expect(model.reload.latest_version.version).to eq('1.0.0') + expect(service).to be_success - expect(Gitlab::InternalEvents).to have_received(:track_event).with( - 'model_registry_ml_model_version_created', - { project: model.project, user: nil } - ) - end - end + expect(model.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq('1.0.0') - context 'when a version exist and no value is passed for version' do - before do - create(:ml_model_versions, model: model, version: '3.0.0') + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_version_created', + { project: model.project, user: user } + ) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) + end end - it 'creates another model version and increments the version number', :aggregate_failures do - expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) - expect(model.reload.latest_version.version).to eq('4.0.0') - expect(service).to be_success + context 'when a version exist and no value is passed for version' do + before do + create(:ml_model_versions, model: model, version: '3.0.0') + model.reload + end - expect(Gitlab::InternalEvents).to have_received(:track_event).with( - 'model_registry_ml_model_version_created', - { project: model.project, user: nil } - ) - end - end + it 'creates another model version and increments the version number' do + expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) + expect(model.reload.latest_version.version).to eq('4.0.0') + expect(service).to be_success - context 'when a version is created and the package already exists' do - it 'does not creates a package' do - next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute - create(:ml_model_package, name: model.name, version: next_version, project: model.project) - - expect { service }.to change { Ml::ModelVersion.count }.by(1).and not_change { - Packages::MlModel::Package.count - } - expect(model.reload.latest_version.package.name).to eq(model.name) - expect(model.latest_version.package.version).to eq(model.latest_version.version) - expect(service).to be_success + expect(Gitlab::InternalEvents).to have_received(:track_event).with( + 'model_registry_ml_model_version_created', + { project: model.project, user: user } + ) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) + end end - end - context 'when creation of a model_version fails' do - it 'returns error' do - allow_next_instance_of(::Ml::ModelVersion) do |model_version| - allow(model_version).to receive(:save).and_return(false) - errors = ActiveModel::Errors.new(model_version).tap { |e| e.add(:id, 'some error') } - allow(model_version).to receive(:errors).and_return(errors) + context 'when a version is created and the package already exists' do + it 'does not creates a package' do + next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute + create(:ml_model_package, name: model.name, version: next_version, project: model.project) + + expect { service }.to change { Ml::ModelVersion.count }.by(1).and not_change { + Packages::MlModel::Package.count + } + expect(model.reload.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq(model.latest_version.version) + expect(service).to be_success + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) end - - expect { service }.to not_change { Ml::ModelVersion.count }.and not_change { Packages::MlModel::Package.count } - expect(service).to be_error - expect(service.message).to include('Id some error') end - end - context 'when a version is created and an existing package supplied' do - it 'does not creates a package' do - next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute - package = create(:ml_model_package, name: model.name, version: next_version, project: model.project) - service = described_class.new(model, { package: package }) - - expect { service.execute }.to change { Ml::ModelVersion.count }.by(1).and not_change { - Packages::MlModel::Package.count - } - expect(model.reload.latest_version.package.name).to eq(model.name) - expect(model.latest_version.package.version).to eq(model.latest_version.version) + context 'when creation of a model_version fails' do + it 'returns error' do + allow_next_instance_of(::Ml::ModelVersion) do |model_version| + allow(model_version).to receive(:save).and_return(false) + errors = ActiveModel::Errors.new(model_version).tap { |e| e.add(:id, 'some error') } + allow(model_version).to receive(:errors).and_return(errors) + end + + expect { service }.to not_change { Ml::ModelVersion.count }.and not_change { Packages::MlModel::Package.count } + expect(service).to be_error + expect(service.message).to include('Id some error') + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) + end end - context 'when metadata are supplied, add them as metadata' do - let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key2', value: 'value2' }] } - let(:params) { { metadata: metadata } } + context 'when a version is created and an existing package supplied' do + it 'does not creates a package' do + next_version = Ml::IncrementVersionService.new(model.latest_version.try(:version)).execute + package = create(:ml_model_package, name: model.name, version: next_version, project: model.project) + service = described_class.new(model, { package: package }) + + expect { service.execute }.to change { Ml::ModelVersion.count }.by(1).and not_change { + Packages::MlModel::Package.count + } + expect(model.reload.latest_version.package.name).to eq(model.name) + expect(model.latest_version.package.version).to eq(model.latest_version.version) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) + end - it 'creates metadata records', :aggregate_failures do - expect { service }.to change { Ml::ModelVersion.count }.by(1) + context 'when metadata are supplied, add them as metadata' do + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key2', value: 'value2' }] } + let(:params) { { metadata: metadata } } - expect(model_version.metadata.count).to eq 2 + it 'creates metadata records' do + expect { service }.to change { Ml::ModelVersion.count }.by(1) + + expect(model_version.metadata.count).to eq 2 + end end - end - # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 - context 'for metadata with duplicate keys, it does not create duplicate records' do - let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key1', value: 'value2' }] } - let(:params) { { metadata: metadata } } + # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 + context 'for metadata with duplicate keys, it does not create duplicate records' do + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key1', value: 'value2' }] } + let(:params) { { metadata: metadata } } - it 'raises an error', :aggregate_failures do - expect(service).to be_error - expect(service.message).to include("Validation failed: Name 'key1' already taken") + it 'raises an error' do + expect(service).to be_error + expect(service.message).to include("Validation failed: Name 'key1' already taken") + end end - end - # # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 - context 'for metadata with invalid keys, it does not create invalid records' do - let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: '', value: 'value2' }] } - let(:params) { { metadata: metadata } } + # # TODO: Ensure consisted error responses https://gitlab.com/gitlab-org/gitlab/-/issues/429731 + context 'for metadata with invalid keys, it does not create invalid records' do + let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: '', value: 'value2' }] } + let(:params) { { metadata: metadata } } - it 'raises an error', :aggregate_failures do - expect(service).to be_error - expect(service.message).to include("Validation failed: Name can't be blank") + it 'raises an error' do + expect(service).to be_error + expect(service.message).to include("Validation failed: Name can't be blank") + end end end - end - context 'when a version string is supplied during creation' do - let(:params) { { version: '1.2.3' } } + context 'when a version string is supplied during creation' do + let(:params) { { version: '1.2.3' } } - it 'creates a package' do - expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { - Packages::MlModel::Package.count - }.by(1) - expect(model.reload.latest_version.version).to eq('1.2.3') - expect(model.latest_version.package.version).to eq('1.2.3') + it 'creates a package' do + expect { service }.to change { Ml::ModelVersion.count }.by(1).and change { + Packages::MlModel::Package.count + }.by(1) + expect(model.reload.latest_version.version).to eq('1.2.3') + expect(model.latest_version.package.version).to eq('1.2.3') + end end - end - context 'when version string supplied is invalid' do - let(:params) { { version: 'invalid-version' } } + context 'when version string supplied is invalid' do + let(:params) { { version: 'invalid-version' } } - it 'returns error' do - expect { service }.to not_change { Ml::ModelVersion.count }.and not_change { Packages::MlModel::Package.count } - expect(service).to be_error - expect(model_version).to be_nil - expect(service.message).to include('Version must be semantic version') + it 'returns error' do + expect { service }.to not_change { Ml::ModelVersion.count }.and not_change { Packages::MlModel::Package.count } + expect(service).to be_error + expect(model_version).to be_nil + expect(service.message).to include('Version must be semantic version') + end end end end diff --git a/spec/services/ml/destroy_model_version_service_spec.rb b/spec/services/ml/destroy_model_version_service_spec.rb index 46ac3be6c2bed9..f384eb81761ca3 100644 --- a/spec/services/ml/destroy_model_version_service_spec.rb +++ b/spec/services/ml/destroy_model_version_service_spec.rb @@ -8,25 +8,41 @@ let(:user) { project.owner } + let(:audit_event) do + { + name: 'ml_model_version_destroyed', + author: user, + scope: project, + message: "MlModelVersion #{model_version.name}/#{model_version.version} destroyed", + target: model_version + } + end + subject(:execute_service) { described_class.new(model_version, user).execute } - describe '#execute' do + before do + allow(Gitlab::Audit::Auditor).to receive(:audit).and_call_original + end + + describe '#execute', :aggregate_failures do context 'when model version exists' do let(:model_version) { create(:ml_model_versions, :with_package, model: model) } - it 'deletes the model version', :aggregate_failures do + it 'deletes the model version' do expect(execute_service).to be_success expect(execute_service.payload[:model_version]).to eq(model_version) expect(Ml::ModelVersion.find_by(id: model_version.id)).to be_nil + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_event) end end context 'when model version has no package' do let(:model_version) { create(:ml_model_versions, model: model) } - it 'does not trigger destroy package service', :aggregate_failures do + it 'does not trigger destroy package service' do expect(Packages::MarkPackageForDestructionService).not_to receive(:new) expect(execute_service).to be_success + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_event) end end @@ -34,9 +50,10 @@ let(:model_version) { create(:ml_model_versions, :with_package, model: model) } let(:user) { nil } - it 'does not delete the model version', :aggregate_failures do + it 'does not delete the model version' do is_expected.to be_error.and have_attributes(message: "You don't have access to this package") expect(Ml::ModelVersion.find_by(id: model_version.id)).to eq(model_version) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end end -- GitLab