Fix audit event streaming BBMs uniqueness creation and nil verification_token

Andrew Jungrequested to merge
fix-verification-token-nil-bbmigrations into master

What does this MR do and why?

In the instance-level audit_events_instance_external_audit_event_destinations table, verification tokens are encrypted. During this migration, a Customer had a nil verification token (either from inserting into the database through the console or database -- as it's not possible elsewhere), causing the migration to fail when migrating to the 'secret_token' column as part of another table (audit_events_instance_external_streaming_destinations)

This MR no-ops the older migration, and clones it with a nil verification token handler, which will generate a value if not found. It remains the same otherwise.

Another issue is with an ongoing migration where it failed during an upgrade -- causing duplicated rows to be created without a stream_destination_id and then conflict with the unique name index when running the db/migrate/20241121192044_add_indexes_for_external_streaming_destination_unique_name_scope.rb migration.

Backports:

References

Screenshots or screen recordings

Diff between the two migrations:

Instance patch:

diff --git a/ee/lib/ee/gitlab/background_migration/fix_incomplete_instance_external_audit_destinations.rb b/ee/lib/ee/gitlab/background_migration/fix_incomplete_instance_external_audit_destinations.rb
index 48025a0b52d1..8447a69cc667 100644
--- a/ee/lib/ee/gitlab/background_migration/fix_incomplete_instance_external_audit_destinations.rb
+++ b/ee/lib/ee/gitlab/background_migration/fix_incomplete_instance_external_audit_destinations.rb
@@ -96,8 +96,21 @@ def sync_migrated_record(legacy_destination)
         end

         def migrate_new_record(legacy_destination)
-          token = decrypt_verification_token(legacy_destination)
-          return unless token
+          token = decrypt_verification_token(legacy_destination) || SecureRandom.base58(18)
+
+          destination = find_or_create_instance_streaming_destination(legacy_destination, token)
+
+          legacy_destination.update_column(:stream_destination_id, destination.id)
+          sync_event_type_filters(legacy_destination, destination)
+          sync_namespace_filters(legacy_destination, destination)
+        end
+
+        def find_or_create_instance_streaming_destination(legacy_destination, token)
+          destination = InstanceStreamingDestination.find_by(
+            category: :http,
+            name: legacy_destination.name)
+
+          return destination if destination

           destination = InstanceStreamingDestination.new(
             name: legacy_destination.name,
@@ -109,24 +122,21 @@ def migrate_new_record(legacy_destination)
           )

           destination.secret_token = token
-
-          return unless destination.save
-
-          legacy_destination.update_column(:stream_destination_id, destination.id)
-
-          sync_event_type_filters(legacy_destination, destination)
-          sync_namespace_filters(legacy_destination, destination)
+          destination.save
+          destination
         end

         def decrypt_verification_token(legacy_destination)
           return unless legacy_destination.encrypted_verification_token.present? &&
             legacy_destination.encrypted_verification_token_iv.present?

+          return if legacy_destination.encrypted_verification_token.bytesize < 16
+
           ::Gitlab::CryptoHelper.aes256_gcm_decrypt(
             legacy_destination.encrypted_verification_token,
             nonce: legacy_destination.encrypted_verification_token_iv
           )
-        rescue OpenSSL::Cipher::CipherError
+        rescue OpenSSL::Cipher::CipherError, TypeError, ArgumentError, StandardError
           nil
         end

Group:

diff --git a/ee/lib/ee/gitlab/background_migration/fix_incomplete_external_audit_destinations.rb b/ee/lib/ee/gitlab/background_migration/fix_incomplete_external_audit_destinations.rb
index 86e99d3f9e47..8c11e7f9b2ab 100644
--- a/ee/lib/ee/gitlab/background_migration/fix_incomplete_external_audit_destinations.rb
+++ b/ee/lib/ee/gitlab/background_migration/fix_incomplete_external_audit_destinations.rb
@@ -89,6 +89,24 @@ def sync_migrated_record(legacy_destination)
         def migrate_new_record(legacy_destination)
           return unless legacy_destination.verification_token.present?

+          destination = find_or_create_group_streaming_destination(legacy_destination)
+
+          legacy_destination.update_column(:stream_destination_id, destination.id)
+
+          migrate_event_type_filters(legacy_destination, destination)
+          migrate_namespace_filter(legacy_destination, destination)
+
+          destination
+        end
+
+        def find_or_create_group_streaming_destination(legacy_destination)
+          destination = GroupStreamingDestination.find_by(
+            group_id: legacy_destination.namespace_id,
+            category: :http,
+            name: legacy_destination.name)
+
+          return destination if destination
+
           destination = GroupStreamingDestination.new(
             name: legacy_destination.name,
             category: :http,
@@ -100,13 +118,7 @@ def migrate_new_record(legacy_destination)
           )

           destination.secret_token = legacy_destination.verification_token
-          return unless destination.save
-
-          legacy_destination.update_column(:stream_destination_id, destination.id)
-
-          migrate_event_type_filters(legacy_destination, destination)
-          migrate_namespace_filter(legacy_destination, destination)
-
+          destination.save
           destination
         end

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Andrew Jung

Merge request reports