From 2f9106aed514cbbb2724ca07c6f44c754d3d21e1 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Wed, 4 Sep 2024 12:01:01 +0200 Subject: [PATCH 1/3] Adds audit events for ML Models Customers need to audit when models are created or destroyed for regulatory reasons Changelog: added --- app/services/ml/create_model_service.rb | 13 ++++++++++++ app/services/ml/destroy_model_service.rb | 14 +++++++++++++ .../audit_events/types/ml_model_created.yml | 9 ++++++++ .../audit_events/types/ml_model_destroyed.yml | 9 ++++++++ doc/user/compliance/audit_event_types.md | 2 ++ spec/services/ml/create_model_service_spec.rb | 21 +++++++++++++++++++ .../services/ml/destroy_model_service_spec.rb | 16 ++++++++++++++ 7 files changed, 84 insertions(+) create mode 100644 config/audit_events/types/ml_model_created.yml create mode 100644 config/audit_events/types/ml_model_destroyed.yml diff --git a/app/services/ml/create_model_service.rb b/app/services/ml/create_model_service.rb index 46035194a12f5b..331441e5105589 100644 --- a/app/services/ml/create_model_service.rb +++ b/app/services/ml/create_model_service.rb @@ -35,6 +35,7 @@ def execute ) add_metadata(model, @metadata) + audit_creation_event(model) success(model) end @@ -69,5 +70,17 @@ def add_metadata(model, metadata_key_value) def experiment_name Ml::Model.prefixed_experiment(@name) end + + def audit_creation_event(model) + audit_context = { + name: 'ml_model_created', + author: @user, + scope: @project, + target: model, + message: "MlModel #{model.name} created" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end diff --git a/app/services/ml/destroy_model_service.rb b/app/services/ml/destroy_model_service.rb index f2a33d58cd8828..1ff9e08e6b13fa 100644 --- a/app/services/ml/destroy_model_service.rb +++ b/app/services/ml/destroy_model_service.rb @@ -17,6 +17,8 @@ def execute return error unless @model.destroy + audit_destroy_event(@model) + success end @@ -37,5 +39,17 @@ def packages_not_deleted(error_message) def payload { model: @model } end + + def audit_destroy_event(model) + audit_context = { + name: 'ml_model_destroyed', + author: @user, + scope: model.project, + target: model, + message: "MlModel #{model.name} destroyed" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end diff --git a/config/audit_events/types/ml_model_created.yml b/config/audit_events/types/ml_model_created.yml new file mode 100644 index 00000000000000..06a1ec28fa4913 --- /dev/null +++ b/config/audit_events/types/ml_model_created.yml @@ -0,0 +1,9 @@ +name: ml_model_created +description: Triggered when an ml model is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164877 +feature_category: mlops +milestone: '17.4' +saved_to_database: true +scope: [Project] +streamed: true diff --git a/config/audit_events/types/ml_model_destroyed.yml b/config/audit_events/types/ml_model_destroyed.yml new file mode 100644 index 00000000000000..159d2dc03bcb4a --- /dev/null +++ b/config/audit_events/types/ml_model_destroyed.yml @@ -0,0 +1,9 @@ +name: ml_model_destroyed +description: Triggered when an ml model is destroyed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164877 +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 fe2e20c830f392..f3068e11b9940b 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -379,6 +379,8 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Streamed | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`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/164877) | Triggered when an 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/164877) | Triggered when an ml model is 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/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb index dd4d41b49955e7..aa2fb71b7880b5 100644 --- a/spec/services/ml/create_model_service_spec.rb +++ b/spec/services/ml/create_model_service_spec.rb @@ -9,8 +9,17 @@ let_it_be(:description) { 'description' } let_it_be(:metadata) { [] } + let(:audit_event) do + { + name: 'ml_model_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(:create_model) { described_class.new(project, name, user, description, metadata).execute } @@ -18,6 +27,10 @@ describe '#execute' do subject(:model_payload) { create_model.payload } + let(:audit_context) do + audit_event.merge(target: model_payload, message: "MlModel #{name} created") + end + context 'when model name is not supplied' do let(:name) { nil } let(:project) { existing_model.project } @@ -27,6 +40,7 @@ expect(create_model).to be_error expect(Gitlab::InternalEvents).not_to have_received(:track_event) expect(create_model.message).to include("Name can't be blank") + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end @@ -41,6 +55,8 @@ { project: project, user: user } ) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) + expect(model_payload.name).to eq('new_model') expect(model_payload.default_experiment.name).to eq('[model]new_model') end @@ -57,6 +73,7 @@ { project: project, user: user } ) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) expect(model_payload.name).to eq(name) end end @@ -70,6 +87,7 @@ expect(create_model).to be_error expect(Gitlab::InternalEvents).not_to have_received(:track_event) expect(create_model.message).to eq(["Name should be unique in the project"]) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end @@ -82,6 +100,7 @@ expect { create_model }.to change { Ml::Model.count }.by(1) expect(model_payload.name).to eq(name) + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_context) expect(model_payload.metadata.count).to be 2 end end @@ -94,6 +113,7 @@ it 'raises an error', :aggregate_failures do expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end @@ -105,6 +125,7 @@ it 'raises an error', :aggregate_failures do expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end end diff --git a/spec/services/ml/destroy_model_service_spec.rb b/spec/services/ml/destroy_model_service_spec.rb index 0e21d9dc39c9b5..16d5d4ceeb821d 100644 --- a/spec/services/ml/destroy_model_service_spec.rb +++ b/spec/services/ml/destroy_model_service_spec.rb @@ -9,6 +9,19 @@ let(:model) { model0 } let(:service) { described_class.new(model, user) } + let(:audit_event) do + { + name: 'ml_model_destroyed', + author: user, + scope: model.project, + message: "MlModel #{model.name} destroyed", + target: model + } + end + + before do + allow(Gitlab::Audit::Auditor).to receive(:audit).and_call_original + end describe '#execute' do subject(:service_result) { service.execute } @@ -18,6 +31,7 @@ allow(model).to receive(:destroy).and_return(false) expect(service_result).to be_error + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end @@ -29,6 +43,7 @@ expect { service_result }.to change { Ml::Model.count }.by(-1).and change { Ml::ModelVersion.count }.by(-1) expect(service_result).to be_success + expect(Gitlab::Audit::Auditor).to have_received(:audit).with(audit_event) end context 'when a package cannot be marked for destruction' do @@ -44,6 +59,7 @@ expect { service_result }.not_to change { Ml::Model.count } expect(service_result).to be_error expect(service_result.message).to eq("An error") + expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end end end -- GitLab From 175f41bcb98e7a1ca9435ada13b3948ed848a9a7 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Wed, 4 Sep 2024 13:56:47 +0200 Subject: [PATCH 2/3] Moves aggregate failures to a higher scope --- spec/services/ml/create_model_service_spec.rb | 16 ++++++++-------- spec/services/ml/destroy_model_service_spec.rb | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb index aa2fb71b7880b5..c6341cd4c2560e 100644 --- a/spec/services/ml/create_model_service_spec.rb +++ b/spec/services/ml/create_model_service_spec.rb @@ -24,7 +24,7 @@ subject(:create_model) { described_class.new(project, name, user, description, metadata).execute } - describe '#execute' do + describe '#execute', :aggregate_failures do subject(:model_payload) { create_model.payload } let(:audit_context) do @@ -35,7 +35,7 @@ let(:name) { nil } let(:project) { existing_model.project } - it 'returns a model with errors', :aggregate_failures do + it 'returns a model with errors' do expect { create_model }.not_to change { Ml::Model.count } expect(create_model).to be_error expect(Gitlab::InternalEvents).not_to have_received(:track_event) @@ -48,7 +48,7 @@ let(:name) { 'new_model' } let(:project) { existing_model.project } - it 'creates a model', :aggregate_failures do + it 'creates a model' do expect { create_model }.to change { Ml::Model.count }.by(1) expect(Gitlab::InternalEvents).to have_received(:track_event).with( 'model_registry_ml_model_created', @@ -66,7 +66,7 @@ let(:name) { existing_model.name } let(:project) { another_project } - it 'creates a model', :aggregate_failures do + it 'creates a model' do expect { create_model }.to change { Ml::Model.count }.by(1) expect(Gitlab::InternalEvents).to have_received(:track_event).with( 'model_registry_ml_model_created', @@ -82,7 +82,7 @@ let(:name) { existing_model.name } let(:project) { existing_model.project } - it 'returns a model with errors', :aggregate_failures do + it 'returns a model with errors' do expect { create_model }.not_to change { Ml::Model.count } expect(create_model).to be_error expect(Gitlab::InternalEvents).not_to have_received(:track_event) @@ -96,7 +96,7 @@ let(:project) { existing_model.project } let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key2', value: 'value2' }] } - it 'creates metadata records', :aggregate_failures do + it 'creates metadata records' do expect { create_model }.to change { Ml::Model.count }.by(1) expect(model_payload.name).to eq(name) @@ -111,7 +111,7 @@ let(:project) { existing_model.project } let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: 'key1', value: 'value2' }] } - it 'raises an error', :aggregate_failures do + it 'raises an error' do expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end @@ -123,7 +123,7 @@ let(:project) { existing_model.project } let(:metadata) { [{ key: 'key1', value: 'value1' }, { key: '', value: 'value2' }] } - it 'raises an error', :aggregate_failures do + it 'raises an error' do expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) expect(Gitlab::Audit::Auditor).not_to have_received(:audit) end diff --git a/spec/services/ml/destroy_model_service_spec.rb b/spec/services/ml/destroy_model_service_spec.rb index 16d5d4ceeb821d..d1c70bab5621d3 100644 --- a/spec/services/ml/destroy_model_service_spec.rb +++ b/spec/services/ml/destroy_model_service_spec.rb @@ -23,7 +23,7 @@ allow(Gitlab::Audit::Auditor).to receive(:audit).and_call_original end - describe '#execute' do + describe '#execute', :aggregate_failures do subject(:service_result) { service.execute } context 'when model fails to delete' do @@ -36,7 +36,7 @@ end context 'when a model exists' do - it 'destroys the model', :aggregate_failures do + it 'destroys the model' do allow_next_instance_of(Packages::MarkPackagesForDestructionService) do |instance| allow(instance).to receive(:execute).and_return ServiceResponse.success(message: "") end @@ -55,7 +55,7 @@ end end - it 'returns success with warning', :aggregate_failures do + it 'returns success with warning' do expect { service_result }.not_to change { Ml::Model.count } expect(service_result).to be_error expect(service_result.message).to eq("An error") -- GitLab From 7492af45e7177eba75be5c2a2a97199922241206 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Wed, 4 Sep 2024 17:02:40 +0200 Subject: [PATCH 3/3] Small fixes to the yaml files --- config/audit_events/types/ml_model_created.yml | 4 ++-- config/audit_events/types/ml_model_destroyed.yml | 4 ++-- doc/user/compliance/audit_event_types.md | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config/audit_events/types/ml_model_created.yml b/config/audit_events/types/ml_model_created.yml index 06a1ec28fa4913..c455c96bc03bb6 100644 --- a/config/audit_events/types/ml_model_created.yml +++ b/config/audit_events/types/ml_model_created.yml @@ -1,7 +1,7 @@ name: ml_model_created -description: Triggered when an ml model is created +description: ML model is created introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164877 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165011 feature_category: mlops milestone: '17.4' saved_to_database: true diff --git a/config/audit_events/types/ml_model_destroyed.yml b/config/audit_events/types/ml_model_destroyed.yml index 159d2dc03bcb4a..fb750103cf8f76 100644 --- a/config/audit_events/types/ml_model_destroyed.yml +++ b/config/audit_events/types/ml_model_destroyed.yml @@ -1,7 +1,7 @@ name: ml_model_destroyed -description: Triggered when an ml model is destroyed +description: ML model destroyed introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/463215 -introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164877 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165011 feature_category: mlops milestone: '17.4' saved_to_database: true diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index f3068e11b9940b..42ca0fcd07b5ef 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -379,8 +379,8 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Streamed | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| | [`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/164877) | Triggered when an 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/164877) | Triggered when an ml model is destroyed | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.4](https://gitlab.com/gitlab-org/gitlab/-/issues/463215) | 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 | | [`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 -- GitLab