From 91feec99f2295d1cdca9e5d22323f9ab332e1238 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 19 Apr 2024 13:31:21 +0530 Subject: [PATCH 01/11] Removed namespace filters plan limits --- ...ents_streaming_group_namespace_filters.yml | 13 ++++++++ ...vents_streaming_group_namespace_filters.rb | 21 ++++++++++++ ...to_audit_events_group_namespace_filters.rb | 20 ++++++++++++ ...to_audit_events_group_namespace_filters.rb | 21 ++++++++++++ db/schema_migrations/20240419071412 | 1 + db/schema_migrations/20240419074624 | 1 + db/schema_migrations/20240419074648 | 1 + db/structure.sql | 32 +++++++++++++++++++ 8 files changed, 110 insertions(+) create mode 100644 db/docs/audit_events_streaming_group_namespace_filters.yml create mode 100644 db/migrate/20240419071412_create_audit_events_streaming_group_namespace_filters.rb create mode 100644 db/migrate/20240419074624_add_destination_fk_to_audit_events_group_namespace_filters.rb create mode 100644 db/migrate/20240419074648_add_namespace_fk_to_audit_events_group_namespace_filters.rb create mode 100644 db/schema_migrations/20240419071412 create mode 100644 db/schema_migrations/20240419074624 create mode 100644 db/schema_migrations/20240419074648 diff --git a/db/docs/audit_events_streaming_group_namespace_filters.yml b/db/docs/audit_events_streaming_group_namespace_filters.yml new file mode 100644 index 00000000000000..3e6043cbd68841 --- /dev/null +++ b/db/docs/audit_events_streaming_group_namespace_filters.yml @@ -0,0 +1,13 @@ +--- +table_name: audit_events_streaming_group_namespace_filters +classes: +- AuditEvents::Group::NamespaceFilter +feature_categories: +- audit_events +description: Stores audit event namespace filters for top-level group external audit + event destinations. +introduced_by_url: +milestone: '17.0' +gitlab_schema: gitlab_main_cell +sharding_key: + namespace_id: namespaces diff --git a/db/migrate/20240419071412_create_audit_events_streaming_group_namespace_filters.rb b/db/migrate/20240419071412_create_audit_events_streaming_group_namespace_filters.rb new file mode 100644 index 00000000000000..3a1e7d36d8fac9 --- /dev/null +++ b/db/migrate/20240419071412_create_audit_events_streaming_group_namespace_filters.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class CreateAuditEventsStreamingGroupNamespaceFilters < Gitlab::Database::Migration[2.2] + milestone '17.0' + enable_lock_retries! + + UNIQ_INDEX_NAME = 'uniq_idx_streaming_group_destination_id_and_namespace_id' + NAMESPACE_INDEX_NAME = 'idx_streaming_group_namespace_filters_on_namespace_id' + + def change + create_table :audit_events_streaming_group_namespace_filters do |t| + t.timestamps_with_timezone null: false + t.bigint :external_streaming_destination_id, + null: false + t.bigint :namespace_id, + null: false, + index: { name: NAMESPACE_INDEX_NAME } + t.index [:external_streaming_destination_id, :namespace_id], unique: true, name: UNIQ_INDEX_NAME + end + end +end diff --git a/db/migrate/20240419074624_add_destination_fk_to_audit_events_group_namespace_filters.rb b/db/migrate/20240419074624_add_destination_fk_to_audit_events_group_namespace_filters.rb new file mode 100644 index 00000000000000..3fd8eb710032bd --- /dev/null +++ b/db/migrate/20240419074624_add_destination_fk_to_audit_events_group_namespace_filters.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddDestinationFkToAuditEventsGroupNamespaceFilters < Gitlab::Database::Migration[2.2] + milestone '17.0' + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :audit_events_streaming_group_namespace_filters, + :audit_events_group_external_streaming_destinations, + column: :external_streaming_destination_id, + on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key_if_exists :audit_events_streaming_group_namespace_filters, + column: :external_streaming_destination_id + end + end +end diff --git a/db/migrate/20240419074648_add_namespace_fk_to_audit_events_group_namespace_filters.rb b/db/migrate/20240419074648_add_namespace_fk_to_audit_events_group_namespace_filters.rb new file mode 100644 index 00000000000000..4014485e468912 --- /dev/null +++ b/db/migrate/20240419074648_add_namespace_fk_to_audit_events_group_namespace_filters.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class AddNamespaceFkToAuditEventsGroupNamespaceFilters < Gitlab::Database::Migration[2.2] + milestone '17.0' + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :audit_events_streaming_group_namespace_filters, + :namespaces, + column: :namespace_id, + on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key_if_exists :audit_events_streaming_group_namespace_filters, + column: :namespace_id + end + end +end diff --git a/db/schema_migrations/20240419071412 b/db/schema_migrations/20240419071412 new file mode 100644 index 00000000000000..2e84653e3dfb62 --- /dev/null +++ b/db/schema_migrations/20240419071412 @@ -0,0 +1 @@ +ead26ae033acc27c74c2e88e05b14489ceffd485086e5b6887d628fe612aa49d \ No newline at end of file diff --git a/db/schema_migrations/20240419074624 b/db/schema_migrations/20240419074624 new file mode 100644 index 00000000000000..2fb8dc03dec31e --- /dev/null +++ b/db/schema_migrations/20240419074624 @@ -0,0 +1 @@ +aa50efa31a1bd8aa4d9065ce7f008606ef747675993977a222f9b3bb73be4e7a \ No newline at end of file diff --git a/db/schema_migrations/20240419074648 b/db/schema_migrations/20240419074648 new file mode 100644 index 00000000000000..943b4cb664ff50 --- /dev/null +++ b/db/schema_migrations/20240419074648 @@ -0,0 +1 @@ +b9b047cc69136028e0037ce85dcd445a18dee7fc01978a27a4d0ee9e805643ee \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 0345cf539c9365..b3addb4d094e7e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -5016,6 +5016,23 @@ CREATE SEQUENCE audit_events_streaming_event_type_filters_id_seq ALTER SEQUENCE audit_events_streaming_event_type_filters_id_seq OWNED BY audit_events_streaming_event_type_filters.id; +CREATE TABLE audit_events_streaming_group_namespace_filters ( + id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + external_streaming_destination_id bigint NOT NULL, + namespace_id bigint NOT NULL +); + +CREATE SEQUENCE audit_events_streaming_group_namespace_filters_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE audit_events_streaming_group_namespace_filters_id_seq OWNED BY audit_events_streaming_group_namespace_filters.id; + CREATE TABLE audit_events_streaming_headers ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -18851,6 +18868,8 @@ ALTER TABLE ONLY audit_events_instance_streaming_event_type_filters ALTER COLUMN ALTER TABLE ONLY audit_events_streaming_event_type_filters ALTER COLUMN id SET DEFAULT nextval('audit_events_streaming_event_type_filters_id_seq'::regclass); +ALTER TABLE ONLY audit_events_streaming_group_namespace_filters ALTER COLUMN id SET DEFAULT nextval('audit_events_streaming_group_namespace_filters_id_seq'::regclass); + ALTER TABLE ONLY audit_events_streaming_headers ALTER COLUMN id SET DEFAULT nextval('audit_events_streaming_headers_id_seq'::regclass); ALTER TABLE ONLY audit_events_streaming_http_group_namespace_filters ALTER COLUMN id SET DEFAULT nextval('audit_events_streaming_http_group_namespace_filters_id_seq'::regclass); @@ -20648,6 +20667,9 @@ ALTER TABLE ONLY audit_events ALTER TABLE ONLY audit_events_streaming_event_type_filters ADD CONSTRAINT audit_events_streaming_event_type_filters_pkey PRIMARY KEY (id); +ALTER TABLE ONLY audit_events_streaming_group_namespace_filters + ADD CONSTRAINT audit_events_streaming_group_namespace_filters_pkey PRIMARY KEY (id); + ALTER TABLE ONLY audit_events_streaming_headers ADD CONSTRAINT audit_events_streaming_headers_pkey PRIMARY KEY (id); @@ -24117,6 +24139,8 @@ CREATE UNIQUE INDEX idx_software_license_policies_unique_on_project_and_scan_pol CREATE INDEX idx_status_check_responses_on_id_and_status ON status_check_responses USING btree (id, status); +CREATE INDEX idx_streaming_group_namespace_filters_on_namespace_id ON audit_events_streaming_group_namespace_filters USING btree (namespace_id); + CREATE INDEX idx_streaming_headers_on_external_audit_event_destination_id ON audit_events_streaming_headers USING btree (external_audit_event_destination_id); CREATE INDEX idx_test_reports_on_issue_id_created_at_and_id ON requirements_management_test_reports USING btree (issue_id, created_at, id); @@ -27997,6 +28021,8 @@ CREATE UNIQUE INDEX uniq_google_cloud_logging_configuration_namespace_id_and_nam CREATE UNIQUE INDEX uniq_idx_packages_packages_on_project_id_name_version_ml_model ON packages_packages USING btree (project_id, name, version) WHERE (package_type = 14); +CREATE UNIQUE INDEX uniq_idx_streaming_group_destination_id_and_namespace_id ON audit_events_streaming_group_namespace_filters USING btree (external_streaming_destination_id, namespace_id); + CREATE UNIQUE INDEX uniq_idx_user_add_on_assignments_on_add_on_purchase_and_user ON subscription_user_add_on_assignments USING btree (add_on_purchase_id, user_id); CREATE UNIQUE INDEX uniq_pkgs_deb_grp_architectures_on_distribution_id_and_name ON packages_debian_group_architectures USING btree (distribution_id, name); @@ -30376,6 +30402,9 @@ ALTER TABLE ONLY protected_tags ALTER TABLE ONLY merge_request_review_llm_summaries ADD CONSTRAINT fk_8ec009c6ab FOREIGN KEY (merge_request_diff_id) REFERENCES merge_request_diffs(id) ON DELETE CASCADE; +ALTER TABLE ONLY audit_events_streaming_group_namespace_filters + ADD CONSTRAINT fk_8ed182d7da FOREIGN KEY (external_streaming_destination_id) REFERENCES audit_events_group_external_streaming_destinations(id) ON DELETE CASCADE; + ALTER TABLE ONLY todos ADD CONSTRAINT fk_91d1f47b13 FOREIGN KEY (note_id) REFERENCES notes(id) ON DELETE CASCADE; @@ -30451,6 +30480,9 @@ ALTER TABLE ONLY deployment_merge_requests ALTER TABLE ONLY issues ADD CONSTRAINT fk_a194299be1 FOREIGN KEY (moved_to_id) REFERENCES issues(id) ON DELETE SET NULL; +ALTER TABLE ONLY audit_events_streaming_group_namespace_filters + ADD CONSTRAINT fk_a1a4486a96 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE; + ALTER TABLE ONLY ml_candidates ADD CONSTRAINT fk_a1d5f1bc45 FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE SET NULL; -- GitLab From 413d7d7d8d3eefee5a0b6dff6d4d1e96e48b7ef2 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 19 Apr 2024 13:52:10 +0530 Subject: [PATCH 02/11] Added models and rspecs --- .../group/external_streaming_destination.rb | 13 +++++ .../audit_events/group/namespace_filter.rb | 17 ++++++ ee/app/models/ee/namespace.rb | 1 + ...vents_streaming_group_namespace_filters.rb | 8 +++ .../external_streaming_destination_spec.rb | 17 ++++++ .../group/namespace_filter_spec.rb | 58 +++++++++++++++++++ ee/spec/models/ee/namespace_spec.rb | 1 + 7 files changed, 115 insertions(+) create mode 100644 ee/app/models/audit_events/group/namespace_filter.rb create mode 100644 ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb create mode 100644 ee/spec/models/audit_events/group/namespace_filter_spec.rb diff --git a/ee/app/models/audit_events/group/external_streaming_destination.rb b/ee/app/models/audit_events/group/external_streaming_destination.rb index bc53aa6bc0cca5..26f282b0271123 100644 --- a/ee/app/models/audit_events/group/external_streaming_destination.rb +++ b/ee/app/models/audit_events/group/external_streaming_destination.rb @@ -6,6 +6,8 @@ class ExternalStreamingDestination < ApplicationRecord include Limitable include ExternallyStreamable + MAXIMUM_NAMESPACE_FILTER_COUNT = 5 + self.limit_name = 'external_audit_event_destinations' self.limit_scope = :group self.table_name = 'audit_events_group_external_streaming_destinations' @@ -15,6 +17,17 @@ class ExternalStreamingDestination < ApplicationRecord validates :name, uniqueness: { scope: :group_id } has_many :event_type_filters, class_name: 'AuditEvents::Group::EventTypeFilter' + has_many :namespace_filters, class_name: 'AuditEvents::Group::NamespaceFilter' + + validate :no_more_than_5_namespace_filters? + + private + + def no_more_than_5_namespace_filters? + return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT + + errors.add(:namespace_filters, "are limited to #{MAXIMUM_NAMESPACE_FILTER_COUNT} per destination") + end def top_level_group? errors.add(:group, 'must not be a subgroup. Use a top-level group.') if group.subgroup? diff --git a/ee/app/models/audit_events/group/namespace_filter.rb b/ee/app/models/audit_events/group/namespace_filter.rb new file mode 100644 index 00000000000000..597dd7805312d9 --- /dev/null +++ b/ee/app/models/audit_events/group/namespace_filter.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module AuditEvents + module Group + class NamespaceFilter < ApplicationRecord + include AuditEvents::Streaming::HTTP::NamespaceFilterable + + self.table_name = 'audit_events_streaming_group_namespace_filters' + + belongs_to :namespace, inverse_of: :audit_events_streaming_group_namespace_filters + belongs_to :external_streaming_destination, class_name: 'ExternalStreamingDestination' + + validates :namespace, presence: true, uniqueness: { scope: :external_streaming_destination_id } + validates :external_streaming_destination, presence: true + end + end +end diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index 74c5b2f46949ff..faac7e84512f1a 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -59,6 +59,7 @@ module Namespace has_one :audit_event_http_namespace_filter, class_name: 'AuditEvents::Streaming::HTTP::NamespaceFilter' has_one :audit_event_http_instance_namespace_filter, class_name: 'AuditEvents::Streaming::HTTP::Instance::NamespaceFilter' has_many :work_items_colors, inverse_of: :namespace, class_name: 'WorkItems::Color' + has_many :audit_events_streaming_group_namespace_filters, class_name: 'AuditEvents::Group::NamespaceFilter' has_one :zoekt_enabled_namespace, class_name: '::Search::Zoekt::EnabledNamespace', foreign_key: :root_namespace_id, inverse_of: :namespace diff --git a/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb b/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb new file mode 100644 index 00000000000000..718c1f9956086c --- /dev/null +++ b/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :audit_events_streaming_group_namespace_filters, class: 'AuditEvents::Group::NamespaceFilter' do + namespace factory: :group + external_streaming_destination factory: :audit_events_group_external_streaming_destination + end +end diff --git a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb index 7fbca728ea4228..32574431d28324 100644 --- a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb +++ b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb @@ -11,6 +11,7 @@ end it { is_expected.to have_many(:event_type_filters) } + it { is_expected.to have_many(:namespace_filters) } end describe 'Validations' do @@ -24,6 +25,22 @@ expect(destination.errors.full_messages).to include('Name has already been taken') end + context 'for namespace filters' do + it 'can have 5 namespace filters' do + create_list(:audit_events_streaming_group_namespace_filters, 5, external_streaming_destination: destination) + + expect(destination).to be_valid + end + + it 'cannot have more than 5 namespace filters' do + create_list(:audit_events_streaming_group_namespace_filters, 6, external_streaming_destination: destination) + + expect(destination).not_to be_valid + expect(destination.errors.full_messages) + .to contain_exactly('Namespace filters are limited to 5 per destination') + end + end + context 'when group' do it 'is a subgroup' do destination.group = build(:group, :nested) diff --git a/ee/spec/models/audit_events/group/namespace_filter_spec.rb b/ee/spec/models/audit_events/group/namespace_filter_spec.rb new file mode 100644 index 00000000000000..a3a39632948802 --- /dev/null +++ b/ee/spec/models/audit_events/group/namespace_filter_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::Group::NamespaceFilter, feature_category: :audit_events do + subject(:namespace_filter) { build(:audit_events_streaming_group_namespace_filters) } + + let_it_be(:destination) { create(:audit_events_group_external_streaming_destination) } + + describe 'Associations' do + it 'belongs to an external audit event destination' do + expect(namespace_filter.external_streaming_destination).not_to be_nil + end + + it 'belongs to a namespace' do + expect(namespace_filter.namespace).not_to be_nil + end + end + + describe 'Validations' do + it { is_expected.to validate_presence_of(:namespace) } + it { is_expected.to validate_presence_of(:external_streaming_destination) } + it { is_expected.to validate_uniqueness_of(:namespace).scoped_to(:external_streaming_destination_id) } + + describe 'validates external destination with namespace' do + shared_examples 'validate namespace with external destination' do |namespace_type| + let_it_be(:namespace) { build(namespace_type.to_sym) } + + subject do + build(:audit_events_streaming_group_namespace_filters, namespace: namespace, + external_streaming_destination: destination) + end + + it { is_expected.to be_valid } + end + + context 'when namespace is group' do + it_behaves_like 'validate namespace with external destination', 'group' + end + + context 'when namespace is project' do + it_behaves_like 'validate namespace with external destination', 'project_namespace' + end + + context 'when namespace is neither project nor group' do + it 'returns error' do + namespace_filter = build(:audit_events_streaming_group_namespace_filters, + namespace: create(:user_namespace), + external_streaming_destination: destination) + + expect(namespace_filter).to be_invalid + expect(namespace_filter.errors.full_messages) + .to include("Namespace is not supported. Only project and group are supported.") + end + end + end + end +end diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 8743b04313cf52..5dc6bb9e9887ab 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -24,6 +24,7 @@ it { is_expected.to have_one(:audit_event_http_instance_namespace_filter) } it { is_expected.to have_one(:zoekt_enabled_namespace) } it { is_expected.to have_many(:work_items_colors) } + it { is_expected.to have_many(:audit_events_streaming_group_namespace_filters) } it { is_expected.to delegate_method(:trial?).to(:gitlab_subscription) } it { is_expected.to delegate_method(:trial_ends_on).to(:gitlab_subscription) } -- GitLab From 24abd7a8f4e7786007c0f503c3cc9ddf32e1abca Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 19 Apr 2024 17:04:34 +0530 Subject: [PATCH 03/11] Added validation for destination and group --- .../audit_events/group/namespace_filter.rb | 11 +++++++ .../group/namespace_filter_spec.rb | 31 ++++++++++++++++--- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/ee/app/models/audit_events/group/namespace_filter.rb b/ee/app/models/audit_events/group/namespace_filter.rb index 597dd7805312d9..9318b4937f1c94 100644 --- a/ee/app/models/audit_events/group/namespace_filter.rb +++ b/ee/app/models/audit_events/group/namespace_filter.rb @@ -12,6 +12,17 @@ class NamespaceFilter < ApplicationRecord validates :namespace, presence: true, uniqueness: { scope: :external_streaming_destination_id } validates :external_streaming_destination, presence: true + + validate :valid_destination_for_namespace, + if: -> { namespace.present? && external_streaming_destination.present? } + + private + + def valid_destination_for_namespace + return if namespace.root_ancestor == external_streaming_destination.group + + errors.add(:external_streaming_destination, 'does not belong to the top-level group of the namespace.') + end end end end diff --git a/ee/spec/models/audit_events/group/namespace_filter_spec.rb b/ee/spec/models/audit_events/group/namespace_filter_spec.rb index a3a39632948802..71a74f61ac8e6b 100644 --- a/ee/spec/models/audit_events/group/namespace_filter_spec.rb +++ b/ee/spec/models/audit_events/group/namespace_filter_spec.rb @@ -23,15 +23,36 @@ it { is_expected.to validate_uniqueness_of(:namespace).scoped_to(:external_streaming_destination_id) } describe 'validates external destination with namespace' do + let_it_be(:grandparent_group) { create(:group) } + let_it_be(:parent_group) { create(:group, parent: grandparent_group) } + shared_examples 'validate namespace with external destination' do |namespace_type| - let_it_be(:namespace) { build(namespace_type.to_sym) } + let_it_be(:namespace) { create(namespace_type.to_sym, parent: parent_group) } - subject do - build(:audit_events_streaming_group_namespace_filters, namespace: namespace, - external_streaming_destination: destination) + context 'when external destination belongs to root ancestor of namespace' do + let(:destination) { create(:audit_events_group_external_streaming_destination, group: grandparent_group) } + + subject do + build(:audit_events_streaming_group_namespace_filters, namespace: namespace, + external_streaming_destination: destination) + end + + it { is_expected.to be_valid } end - it { is_expected.to be_valid } + context 'when external destination does not belong to root ancestor of namespace' do + it 'returns error' do + destination = create(:audit_events_group_external_streaming_destination, group: create(:group)) + namespace_filter = build(:audit_events_streaming_group_namespace_filters, namespace: namespace, + external_streaming_destination: destination) + + expect(namespace_filter).to be_invalid + expect(namespace_filter.errors.full_messages) + .to contain_exactly( + 'External streaming destination does not belong to the top-level group of the namespace.' + ) + end + end end context 'when namespace is group' do -- GitLab From b8be96acae9609c489ab8a0e6a292745bd7b16fc Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 19 Apr 2024 19:55:06 +0530 Subject: [PATCH 04/11] Fixed failing rspecs --- .../group/external_streaming_destination_spec.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb index 32574431d28324..2b981fe547a69b 100644 --- a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb +++ b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb @@ -27,13 +27,19 @@ context 'for namespace filters' do it 'can have 5 namespace filters' do - create_list(:audit_events_streaming_group_namespace_filters, 5, external_streaming_destination: destination) + 5.times do + create(:audit_events_streaming_group_namespace_filters, external_streaming_destination: destination, + namespace: create(:group, parent: destination.group)) + end expect(destination).to be_valid end it 'cannot have more than 5 namespace filters' do - create_list(:audit_events_streaming_group_namespace_filters, 6, external_streaming_destination: destination) + 6.times do + create(:audit_events_streaming_group_namespace_filters, external_streaming_destination: destination, + namespace: create(:group, parent: destination.group)) + end expect(destination).not_to be_valid expect(destination.errors.full_messages) -- GitLab From 64ec311e70a16a53d4dd8ca3be26aac5208a1fc5 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Fri, 19 Apr 2024 23:31:52 +0530 Subject: [PATCH 05/11] Fixed failing rspecs --- .../audit_events_streaming_group_namespace_filters.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb b/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb index 718c1f9956086c..03ed83bad6bafc 100644 --- a/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb +++ b/ee/spec/factories/audit_events/audit_events_streaming_group_namespace_filters.rb @@ -2,7 +2,7 @@ FactoryBot.define do factory :audit_events_streaming_group_namespace_filters, class: 'AuditEvents::Group::NamespaceFilter' do - namespace factory: :group external_streaming_destination factory: :audit_events_group_external_streaming_destination + namespace { external_streaming_destination.group } end end -- GitLab From 9ab745072b5d8b47327c3766bc00bdcab795e7b9 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Mon, 22 Apr 2024 14:59:28 +0000 Subject: [PATCH 06/11] Adding merge request url --- db/docs/audit_events_streaming_group_namespace_filters.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/docs/audit_events_streaming_group_namespace_filters.yml b/db/docs/audit_events_streaming_group_namespace_filters.yml index 3e6043cbd68841..d1d726f2082c4e 100644 --- a/db/docs/audit_events_streaming_group_namespace_filters.yml +++ b/db/docs/audit_events_streaming_group_namespace_filters.yml @@ -6,7 +6,7 @@ feature_categories: - audit_events description: Stores audit event namespace filters for top-level group external audit event destinations. -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/150092 milestone: '17.0' gitlab_schema: gitlab_main_cell sharding_key: -- GitLab From d2e09a847d0401a30f8ef6ded4ade810065ecca5 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Tue, 23 Apr 2024 11:19:05 +0000 Subject: [PATCH 07/11] Adding suggestions from reviewer --- .../audit_events/group/external_streaming_destination.rb | 2 +- .../group/external_streaming_destination_spec.rb | 6 +++--- .../models/audit_events/group/namespace_filter_spec.rb | 9 ++------- ee/spec/models/ee/namespace_spec.rb | 2 +- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/ee/app/models/audit_events/group/external_streaming_destination.rb b/ee/app/models/audit_events/group/external_streaming_destination.rb index 26f282b0271123..f5c125dffde42c 100644 --- a/ee/app/models/audit_events/group/external_streaming_destination.rb +++ b/ee/app/models/audit_events/group/external_streaming_destination.rb @@ -26,7 +26,7 @@ class ExternalStreamingDestination < ApplicationRecord def no_more_than_5_namespace_filters? return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT - errors.add(:namespace_filters, "are limited to #{MAXIMUM_NAMESPACE_FILTER_COUNT} per destination") + errors.add(:namespace_filters, _("are limited to #{MAXIMUM_NAMESPACE_FILTER_COUNT} per destination")) end def top_level_group? diff --git a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb index 2b981fe547a69b..8499c446eed1a5 100644 --- a/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb +++ b/ee/spec/models/audit_events/group/external_streaming_destination_spec.rb @@ -11,7 +11,7 @@ end it { is_expected.to have_many(:event_type_filters) } - it { is_expected.to have_many(:namespace_filters) } + it { is_expected.to have_many(:namespace_filters).class_name('AuditEvents::Group::NamespaceFilter') } end describe 'Validations' do @@ -25,7 +25,7 @@ expect(destination.errors.full_messages).to include('Name has already been taken') end - context 'for namespace filters' do + describe '#no_more_than_5_namespace_filters?' do it 'can have 5 namespace filters' do 5.times do create(:audit_events_streaming_group_namespace_filters, external_streaming_destination: destination, @@ -43,7 +43,7 @@ expect(destination).not_to be_valid expect(destination.errors.full_messages) - .to contain_exactly('Namespace filters are limited to 5 per destination') + .to contain_exactly(_('Namespace filters are limited to 5 per destination')) end end diff --git a/ee/spec/models/audit_events/group/namespace_filter_spec.rb b/ee/spec/models/audit_events/group/namespace_filter_spec.rb index 71a74f61ac8e6b..a62a9f3de508db 100644 --- a/ee/spec/models/audit_events/group/namespace_filter_spec.rb +++ b/ee/spec/models/audit_events/group/namespace_filter_spec.rb @@ -8,13 +8,8 @@ let_it_be(:destination) { create(:audit_events_group_external_streaming_destination) } describe 'Associations' do - it 'belongs to an external audit event destination' do - expect(namespace_filter.external_streaming_destination).not_to be_nil - end - - it 'belongs to a namespace' do - expect(namespace_filter.namespace).not_to be_nil - end + it { is_expected.to belong_to(:external_streaming_destination).class_name('ExternalStreamingDestination') } + it { is_expected.to belong_to(:namespace).inverse_of(:audit_events_streaming_group_namespace_filters) } end describe 'Validations' do diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 5dc6bb9e9887ab..dc73bfb1f5ced0 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -24,7 +24,7 @@ it { is_expected.to have_one(:audit_event_http_instance_namespace_filter) } it { is_expected.to have_one(:zoekt_enabled_namespace) } it { is_expected.to have_many(:work_items_colors) } - it { is_expected.to have_many(:audit_events_streaming_group_namespace_filters) } + it { is_expected.to have_many(:audit_events_streaming_group_namespace_filters).class_name('AuditEvents::Group::NamespaceFilter') } it { is_expected.to delegate_method(:trial?).to(:gitlab_subscription) } it { is_expected.to delegate_method(:trial_ends_on).to(:gitlab_subscription) } -- GitLab From b70db2f076658653355d31a9864a9f3e87ae8661 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Tue, 23 Apr 2024 16:55:42 +0530 Subject: [PATCH 08/11] Added translation and inverse --- ee/app/models/audit_events/group/namespace_filter.rb | 5 +++-- ee/spec/models/audit_events/group/namespace_filter_spec.rb | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/app/models/audit_events/group/namespace_filter.rb b/ee/app/models/audit_events/group/namespace_filter.rb index 9318b4937f1c94..9ba542b59d6cca 100644 --- a/ee/app/models/audit_events/group/namespace_filter.rb +++ b/ee/app/models/audit_events/group/namespace_filter.rb @@ -8,7 +8,8 @@ class NamespaceFilter < ApplicationRecord self.table_name = 'audit_events_streaming_group_namespace_filters' belongs_to :namespace, inverse_of: :audit_events_streaming_group_namespace_filters - belongs_to :external_streaming_destination, class_name: 'ExternalStreamingDestination' + belongs_to :external_streaming_destination, class_name: 'ExternalStreamingDestination', + inverse_of: :namespace_filters validates :namespace, presence: true, uniqueness: { scope: :external_streaming_destination_id } validates :external_streaming_destination, presence: true @@ -21,7 +22,7 @@ class NamespaceFilter < ApplicationRecord def valid_destination_for_namespace return if namespace.root_ancestor == external_streaming_destination.group - errors.add(:external_streaming_destination, 'does not belong to the top-level group of the namespace.') + errors.add(:external_streaming_destination, _('does not belong to the top-level group of the namespace.')) end end end diff --git a/ee/spec/models/audit_events/group/namespace_filter_spec.rb b/ee/spec/models/audit_events/group/namespace_filter_spec.rb index a62a9f3de508db..7c80c865fec82a 100644 --- a/ee/spec/models/audit_events/group/namespace_filter_spec.rb +++ b/ee/spec/models/audit_events/group/namespace_filter_spec.rb @@ -44,7 +44,7 @@ expect(namespace_filter).to be_invalid expect(namespace_filter.errors.full_messages) .to contain_exactly( - 'External streaming destination does not belong to the top-level group of the namespace.' + _('External streaming destination does not belong to the top-level group of the namespace.') ) end end -- GitLab From 05b0a9d87e68c284354fa5c0dfb4ab9cdda2fae0 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Tue, 23 Apr 2024 17:04:42 +0530 Subject: [PATCH 09/11] Generated gitlab pot --- .../audit_events/group/external_streaming_destination.rb | 3 ++- locale/gitlab.pot | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ee/app/models/audit_events/group/external_streaming_destination.rb b/ee/app/models/audit_events/group/external_streaming_destination.rb index f5c125dffde42c..1190c6a60052c4 100644 --- a/ee/app/models/audit_events/group/external_streaming_destination.rb +++ b/ee/app/models/audit_events/group/external_streaming_destination.rb @@ -26,7 +26,8 @@ class ExternalStreamingDestination < ApplicationRecord def no_more_than_5_namespace_filters? return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT - errors.add(:namespace_filters, _("are limited to #{MAXIMUM_NAMESPACE_FILTER_COUNT} per destination")) + errors.add(:namespace_filters, + format(_("are limited to %{max_count} per destination"), max_count: MAXIMUM_NAMESPACE_FILTER_COUNT)) end def top_level_group? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a02b0ea497cc1c..6317ddbf3b05f5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -60124,6 +60124,9 @@ msgstr "" msgid "archived:" msgstr "" +msgid "are limited to %{max_count} per destination" +msgstr "" + msgid "artifacts" msgstr "" @@ -60714,6 +60717,9 @@ msgstr "" msgid "disabled" msgstr "" +msgid "does not belong to the top-level group of the namespace." +msgstr "" + msgid "does not exist" msgstr "" -- GitLab From ebbe16b63500c4dd9f9a8a26704d4561acce5a65 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Tue, 23 Apr 2024 17:13:29 +0530 Subject: [PATCH 10/11] Moved validation to externally streamable --- .../group/external_streaming_destination.rb | 11 ----------- .../concerns/audit_events/externally_streamable.rb | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/ee/app/models/audit_events/group/external_streaming_destination.rb b/ee/app/models/audit_events/group/external_streaming_destination.rb index 1190c6a60052c4..c4c1fecd599238 100644 --- a/ee/app/models/audit_events/group/external_streaming_destination.rb +++ b/ee/app/models/audit_events/group/external_streaming_destination.rb @@ -6,8 +6,6 @@ class ExternalStreamingDestination < ApplicationRecord include Limitable include ExternallyStreamable - MAXIMUM_NAMESPACE_FILTER_COUNT = 5 - self.limit_name = 'external_audit_event_destinations' self.limit_scope = :group self.table_name = 'audit_events_group_external_streaming_destinations' @@ -19,17 +17,8 @@ class ExternalStreamingDestination < ApplicationRecord has_many :event_type_filters, class_name: 'AuditEvents::Group::EventTypeFilter' has_many :namespace_filters, class_name: 'AuditEvents::Group::NamespaceFilter' - validate :no_more_than_5_namespace_filters? - private - def no_more_than_5_namespace_filters? - return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT - - errors.add(:namespace_filters, - format(_("are limited to %{max_count} per destination"), max_count: MAXIMUM_NAMESPACE_FILTER_COUNT)) - end - def top_level_group? errors.add(:group, 'must not be a subgroup. Use a top-level group.') if group.subgroup? end diff --git a/ee/app/models/concerns/audit_events/externally_streamable.rb b/ee/app/models/concerns/audit_events/externally_streamable.rb index 981a9b79f6df9d..c17b4b564109e8 100644 --- a/ee/app/models/concerns/audit_events/externally_streamable.rb +++ b/ee/app/models/concerns/audit_events/externally_streamable.rb @@ -4,6 +4,8 @@ module AuditEvents module ExternallyStreamable extend ActiveSupport::Concern + MAXIMUM_NAMESPACE_FILTER_COUNT = 5 + included do before_validation :assign_default_name @@ -19,6 +21,8 @@ module ExternallyStreamable validates :config, presence: true, json_schema: { filename: 'external_streaming_destination_config' } validates :secret_token, presence: true + validate :no_more_than_5_namespace_filters? + attr_encrypted :secret_token, mode: :per_attribute_iv, key: Settings.attr_encrypted_db_key_base_32, @@ -31,6 +35,13 @@ module ExternallyStreamable def assign_default_name self.name ||= "Destination_#{SecureRandom.uuid}" end + + def no_more_than_5_namespace_filters? + return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT + + errors.add(:namespace_filters, + format(_("are limited to %{max_count} per destination"), max_count: MAXIMUM_NAMESPACE_FILTER_COUNT)) + end end end end -- GitLab From e3e677717a514514d3f19cefce04bda4578715bb Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi Date: Wed, 24 Apr 2024 10:29:37 +0530 Subject: [PATCH 11/11] Revert "Moved validation to externally streamable" This reverts commit ebbe16b63500c4dd9f9a8a26704d4561acce5a65. --- .../group/external_streaming_destination.rb | 11 +++++++++++ .../concerns/audit_events/externally_streamable.rb | 11 ----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/ee/app/models/audit_events/group/external_streaming_destination.rb b/ee/app/models/audit_events/group/external_streaming_destination.rb index c4c1fecd599238..1190c6a60052c4 100644 --- a/ee/app/models/audit_events/group/external_streaming_destination.rb +++ b/ee/app/models/audit_events/group/external_streaming_destination.rb @@ -6,6 +6,8 @@ class ExternalStreamingDestination < ApplicationRecord include Limitable include ExternallyStreamable + MAXIMUM_NAMESPACE_FILTER_COUNT = 5 + self.limit_name = 'external_audit_event_destinations' self.limit_scope = :group self.table_name = 'audit_events_group_external_streaming_destinations' @@ -17,8 +19,17 @@ class ExternalStreamingDestination < ApplicationRecord has_many :event_type_filters, class_name: 'AuditEvents::Group::EventTypeFilter' has_many :namespace_filters, class_name: 'AuditEvents::Group::NamespaceFilter' + validate :no_more_than_5_namespace_filters? + private + def no_more_than_5_namespace_filters? + return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT + + errors.add(:namespace_filters, + format(_("are limited to %{max_count} per destination"), max_count: MAXIMUM_NAMESPACE_FILTER_COUNT)) + end + def top_level_group? errors.add(:group, 'must not be a subgroup. Use a top-level group.') if group.subgroup? end diff --git a/ee/app/models/concerns/audit_events/externally_streamable.rb b/ee/app/models/concerns/audit_events/externally_streamable.rb index c17b4b564109e8..981a9b79f6df9d 100644 --- a/ee/app/models/concerns/audit_events/externally_streamable.rb +++ b/ee/app/models/concerns/audit_events/externally_streamable.rb @@ -4,8 +4,6 @@ module AuditEvents module ExternallyStreamable extend ActiveSupport::Concern - MAXIMUM_NAMESPACE_FILTER_COUNT = 5 - included do before_validation :assign_default_name @@ -21,8 +19,6 @@ module ExternallyStreamable validates :config, presence: true, json_schema: { filename: 'external_streaming_destination_config' } validates :secret_token, presence: true - validate :no_more_than_5_namespace_filters? - attr_encrypted :secret_token, mode: :per_attribute_iv, key: Settings.attr_encrypted_db_key_base_32, @@ -35,13 +31,6 @@ module ExternallyStreamable def assign_default_name self.name ||= "Destination_#{SecureRandom.uuid}" end - - def no_more_than_5_namespace_filters? - return unless namespace_filters.count > MAXIMUM_NAMESPACE_FILTER_COUNT - - errors.add(:namespace_filters, - format(_("are limited to %{max_count} per destination"), max_count: MAXIMUM_NAMESPACE_FILTER_COUNT)) - end end end end -- GitLab