Skip to content

Require alert integration identifiers to be unique by project

Sarah Yasonik requested to merge sy-reindex-alert-integrations into master

What does this MR do and why?

Before this change, inactive alert integrations could have non-unique identifiers. Users don't have control over the identifiers, however, so it would be impossible to enable an existing inactive integration that matched an active one.

This commit extends uniqueness requirements to include inactive integrations, so all integrations will be unique. In the rare circumstance of a hash collision, inactive integrations with matching identifiers will be updated with a new identifier deleted.

Motivation:

  • Better reflection of actual usage / improve query performance
    • ~1600 existing AlertManagement::HttpIntegrations (~1500 active, ~100 inactive)
    • Any query by endpoint identifier which doesn't scope to active integrations can't use the index. With !120046 (merged), this has become a more common scenario.
  • Make alert integrations easier to reason about 🤷

How likely are non-unique records?

  • ~0 integrations on gitlab.com impacted by new unique index
    • When would non-unique records exist? If there was a random collision from SecureRandom.hex(8) on multiple inactive integrations in the same project. As there are only even ~30 projects with more than one integration, this is highly unlikely to crop up.
    • Service ping data suggests that all of self-managed combined is about the same scale as gitlab.com. It'd be surprising if we saw collisions on self-managed, but it is still technically a possibility.
  • It's only currently possible for inactive integrations to be non-unique, so we know we won't break any active integrations outright. And because of the existing unique index, it's impossible to enable any inactive duplicates of active integrations.
  • There's also no mechanism for users to change the identifier of an integration, so updating or deleting could both be appropriate. In this MR, I opted to swap the identifier of any non-unique, inactive integrations. This allows the integrations to be enabled in future and we aren't deleting any user data.

Query plans:

  1. Check for integrations with duplicate identifiers
    • Query: plan | table size
    • This query plan utilizes a sequential scan on the table, but as the table on gitlab.com contains only ~1600 records and this is a one-time operation, I don't think it's worth adding an index.
HttpIntegration
  .group(:project_id, :endpoint_identifier)
  .having('count(id) > 1')
  .pluck(:project_id, :endpoint_identifier)
SELECT "alert_management_http_integrations"."project_id", "alert_management_http_integrations"."endpoint_identifier"  FROM "alert_management_http_integrations"  GROUP BY "alert_management_http_integrations"."project_id", "alert_management_http_integrations"."endpoint_identifier"  HAVING (count(id) > 1)
  1. Fetch individual records to update (not expected to run on gitlab.com)
HttpIntegration.where(
  project_id: project_id,
  endpoint_identifier: endpoint_identifier,
  active: false
)
SELECT "alert_management_http_integrations".id FROM "alert_management_http_integrations" WHERE "alert_management_http_integrations"."endpoint_identifier" = 'legacy' AND "alert_management_http_integrations"."active" = FALSE AND "alert_management_http_integrations".project_id = 5614029
  1. Delete records with repeat collision problems (not expected to run on gitlab.com)
HttpIntegration.where(
  project_id: project_id,
  endpoint_identifier: endpoint_identifier,
  active: false
).delete_all
DELETE FROM "alert_management_http_integrations" WHERE "alert_management_http_integrations"."project_id" = 5614029 AND "alert_management_http_integrations"."endpoint_identifier" = 'legacy' AND "alert_management_http_integrations"."active" = FALSE

How to set up and validate locally

This isn't really testable through the UI. The updated validation can be tested via the rails console:

integration = AlertManagement::HttpIntegration.last

AlertManagement::HttpIntegration.create!(project_id: integration.project_id, active: false, name: 'Name', endpoint_identifier: integration.endpoint_identifier)

# See ActiveRecord::RecordInvalid error

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sarah Yasonik

Merge request reports