Enforce namespace filter limit at filter creation

Raounak Sharmarequested to merge
616453-enforce-namespace-filter-limit into master

What does this MR do and why?

The limit of five namespace filters per audit event streaming destination was enforced by a validation on the destination, so it only ran when the destination itself was saved. The create mutations save a NamespaceFilter record directly and never save the destination, so the validation never ran at the moment a filter was added. It counted zero on destination create, and counted a stale total on destination update, because the frontend always saves the destination before adding filters.

The result: auditEventsGroupDestinationNamespaceFilterCreate could be called any number of times and every call returned errors: [] and persisted a row. The guard in ee/app/assets/javascripts/audit_events/constants.js is a UX truncation only, so calling GraphQL directly bypassed it.

This moves the check onto the filter models as a create-time validation, so the rule lives on the object whose save is the INSERT being guarded.

Removing it from the destination also clears the knock-on effect described in the issue. A destination pushed over the limit currently fails every subsequent save, so renaming a stream errors with a message about filters until someone deletes them. Those destinations stay saveable now.

The save is wrapped in a row lock on the destination so that concurrent requests cannot both pass the count check and exceed the cap. This matters in practice: the frontend fires the add mutations in parallel with Promise.all. It follows the same pattern as MergeRequests::SavedViews::CreateService, which enforces an identical five-item cap.

The user-facing error message is unchanged: Namespace filters are limited to 5 per destination.

How to reproduce the issue

Requires an Ultimate license and Owner access on a top-level group. Check out master, since this branch fixes it.

  1. Create a top-level group parent with six subgroups, parent/sub-1 through parent/sub-6.

  2. Create a group-level streaming destination for parent, either through Secure > Audit events > Streams or the auditEventsGroupDestinationCreate mutation.

  3. Open /-/graphql-explorer and run the mutation below six times, changing namespacePath each time. You must bypass the UI: the namespace dropdown truncates your selection to five client-side, so the sixth request is never sent from the browser.

    mutation {
  auditEventsGroupDestinationNamespaceFilterCreate(input: {
    destinationId: "gid://gitlab/AuditEvents::Group::ExternalStreamingDestination/<ID>",
    namespacePath: "parent/sub-N"
  }) {
    errors
    namespaceFilter { id }
  }
}

    All six calls return an empty errors array and a persisted filter.

  4. Confirm in the Rails console:

    AuditEvents::Group::NamespaceFilter.where(external_streaming_destination_id: <ID>).count
# => 6

  5. Observe the knock-on effect. Rename that destination in the UI. The save fails with Namespace filters are limited to 5 per destination, on a screen that says nothing about filters. The destination is stuck until filters are deleted.

How to test the fix

On this branch, repeat the steps above.

  1. Calls 1 to 5 succeed. Call 6 returns errors: ["Namespace filters are limited to 5 per destination"] with namespaceFilter: null, and the count stays at 5.

  2. Confirm destinations already over the limit are not bricked. In the Rails console, force a sixth row past validation to simulate data created by this bug, then rename:

    dest = AuditEvents::Group::ExternalStreamingDestination.find(<ID>)
AuditEvents::Group::NamespaceFilter.new(
  external_streaming_destination: dest, namespace: Group.find_by_full_path('parent/sub-6')
).save(validate: false)

dest.namespace_filters.count   # => 6
dest.update(name: 'Renamed')   # => true

  3. Confirm editing an existing filter is unaffected, since the validation is on: :create:

    dest.namespace_filters.first.update(namespace: Group.find_by_full_path('parent/sub-1'))
# => true

  4. Confirm the concurrency guard. With a destination holding four filters, fire six creates in parallel threads on separate connections. Without the lock all six commit; with it, exactly one does.

  5. Run the specs:

    bundle exec rspec \
  ee/spec/models/audit_events/group/namespace_filter_spec.rb \
  ee/spec/models/audit_events/instance/namespace_filter_spec.rb \
  ee/spec/requests/api/graphql/audit_events/group/namespace_filters/create_spec.rb \
  ee/spec/requests/api/graphql/audit_events/instance/namespace_filters/create_spec.rb

    Wider sweep across ee/spec/models/audit_events, ee/spec/requests/api/graphql/audit_events and the namespace filter sync helper spec: 1317 examples, 0 failures.

Before and after

Running the reproduction steps above:

calls 1-5 call 6 final count destination renameable
before persisted persisted 6 yes, until the next save
after persisted rejected 5 yes

Six threads released simultaneously against a destination already holding four filters:

resulting rows (cap is 5)
without the row lock 10
with the row lock 5

Test coverage added

  • Create-time limit specs on both filter models: the fifth filter is allowed, the sixth is rejected, and updates are exempt.
  • Limit rejection specs on both create mutations.
  • Lock assertions on both create mutations using the lock_recorder matcher, so the concurrency guard is covered without thread-based flakiness.
  • Removed the destination-side limit specs, which covered the validation this MR moves.

Database

No migration. Two queries are added, both on the namespace filter create path, which runs only when an administrator adds a filter to a streaming destination.

Group level:

-- create-time validation, once per filter create
SELECT COUNT(*)
FROM audit_events_streaming_group_namespace_filters
WHERE external_streaming_destination_id = $1;

-- with_lock, once per filter create
SELECT *
FROM audit_events_group_external_streaming_destinations
WHERE id = $1
LIMIT 1
FOR UPDATE;

Instance level is identical, against audit_events_streaming_instance_namespace_filters and audit_events_instance_external_streaming_destinations.

Index coverage:

Query Index Notes
COUNT(*), group uniq_idx_streaming_group_destination_id_and_namespace_id on (external_streaming_destination_id, namespace_id) leading column match
COUNT(*), instance uniq_idx_streaming_destination_id_and_namespace_id on (external_streaming_destination_id, namespace_id) leading column match
SELECT ... FOR UPDATE primary key single row

Query plans, from the gitlab-production-main Database Lab clone:

Query Plan
COUNT(*), group index only scan, Heap Fetches: 0, 0.044 ms
SELECT ... FOR UPDATE, group LockRows over a primary key scan, 0.019 ms
COUNT(*), instance sequential scan, empty table, 0.009 ms
SELECT ... FOR UPDATE, instance LockRows over a sequential scan, empty table, 0.018 ms

The two instance-level plans sequential scan because those tables are empty in production, not because an index is missing. Both the unique index and the primary key exist, so those queries take the same paths as the group-level ones once the tables hold rows.

The rows counted are bounded at five by the limit being enforced, and all three tables are table_size: small in db/docs/. The lock is held only for the count and the insert; the audit event and the legacy destination sync both run outside the transaction.

Destinations already over the limit

A query against audit_events_streaming_group_namespace_filters and audit_events_streaming_instance_namespace_filters on the gitlab-production-main Database Lab clone shows no destination currently over the limit of five namespace filters. The group table holds 37 filters across 37 distinct destinations; the instance table is empty. If any destination were already over the cap, this change would leave it there rather than trim it, because removing the destination-side validation is what stops an over-limit destination from failing every subsequent save, such as a rename. The create-time validation on the namespace filter models still rejects any new filter added to such a destination. Since the count over the limit is zero, no cleanup follow-up is needed.

References

Edited by Raounak Sharma

Merge request reports