From caf44e76f3bd06be81b1fc217b880d12fb875a6c Mon Sep 17 00:00:00 2001 From: agius Date: Thu, 5 Sep 2024 15:09:44 -0700 Subject: [PATCH 1/3] Add database columns for 7+30+60 day PAT expiry notification Part 1 of 3, this adds db columns to use for 7, 30, and 60 day notifications for expiring access tokens. The end goal is to replace the existing boolean `expire_notification_delivered` column with the new datetime `*_delivered_at` columns. The next two MRs will backfill data for the new columns, then make use of them in new workers for handling expiring token notifications. Broken out from https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163818 Changelog: added --- ...fication_cols_to_personal_access_tokens.rb | 43 +++++++++++++++ ...0619_add_indices_for_pat_expiry_columns.rb | 53 +++++++++++++++++++ db/schema_migrations/20240822195511 | 1 + db/schema_migrations/20240822200619 | 1 + db/structure.sql | 9 ++++ 5 files changed, 107 insertions(+) create mode 100644 db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb create mode 100644 db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb create mode 100644 db/schema_migrations/20240822195511 create mode 100644 db/schema_migrations/20240822200619 diff --git a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb new file mode 100644 index 00000000000000..6946f182bd4107 --- /dev/null +++ b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddNotificationColsToPersonalAccessTokens < Gitlab::Database::Migration[2.2] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + # restrict_gitlab_migration gitlab_schema: :gitlab_main + + # Add dependent 'batched_background_migrations.queued_migration_version' values. + # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] + milestone '17.4' + + def up + with_lock_retries do + add_column :personal_access_tokens, :seven_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :thirty_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :sixty_days_notification_sent_at, :datetime_with_timezone + end + end + + def down + with_lock_retries do + remove_column :personal_access_tokens, :seven_days_notification_sent_at + remove_column :personal_access_tokens, :thirty_days_notification_sent_at + remove_column :personal_access_tokens, :sixty_days_notification_sent_at + end + end +end diff --git a/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb new file mode 100644 index 00000000000000..443137dc29ba10 --- /dev/null +++ b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndicesForPatExpiryColumns < Gitlab::Database::Migration[2.2] + # When using the methods "add_concurrent_index" or "remove_concurrent_index" + # you must disable the use of transactions + # as these methods can not run in an existing transaction. + # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure + # that either of them is the _only_ method called in the migration, + # any other changes should go in a separate migration. + # This ensures that upon failure _only_ the index creation or removing fails + # and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + # + # Configure the `gitlab_schema` to perform data manipulation (DML). + # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html + # restrict_gitlab_migration gitlab_schema: :gitlab_main + + # Add dependent 'batched_background_migrations.queued_migration_version' values. + # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] + milestone '17.4' + + def up + add_concurrent_index :personal_access_tokens, + [:expires_at, :id], + where: 'impersonation = false AND revoked = false AND seven_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_seven_days_notification_sent_at' + + add_concurrent_index :personal_access_tokens, + [:expires_at, :id], + where: 'impersonation = false AND revoked = false AND thirty_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_thirty_days_notification_sent_at' + + add_concurrent_index :personal_access_tokens, + [:expires_at, :id], + where: 'impersonation = false AND revoked = false AND sixty_days_notification_sent_at IS NULL', + name: 'index_pats_on_expiring_at_sixty_days_notification_sent_at' + end + + def down + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_seven_days_notification_sent_at' + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_thirty_days_notification_sent_at' + remove_concurrent_index_by_name :personal_access_tokens, + name: 'index_pats_on_expiring_at_sixty_days_notification_sent_at' + end +end diff --git a/db/schema_migrations/20240822195511 b/db/schema_migrations/20240822195511 new file mode 100644 index 00000000000000..45c3c17bdf0ac2 --- /dev/null +++ b/db/schema_migrations/20240822195511 @@ -0,0 +1 @@ +234b63a3ca62c94f3d2a7b56d466528151d82c36dbdb11614fc035c7ac23d434 \ No newline at end of file diff --git a/db/schema_migrations/20240822200619 b/db/schema_migrations/20240822200619 new file mode 100644 index 00000000000000..9495d9228b5ded --- /dev/null +++ b/db/schema_migrations/20240822200619 @@ -0,0 +1 @@ +d7ef657ff096c75aa7113e0e2562d9432fddfa4e1bb9a3a148fb29aa2926f006 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 9ce4f88579bd33..ea7adbad95fdba 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -15396,6 +15396,9 @@ CREATE TABLE personal_access_tokens ( previous_personal_access_token_id bigint, advanced_scopes text, organization_id bigint DEFAULT 1 NOT NULL, + seven_days_notification_sent_at timestamp with time zone, + thirty_days_notification_sent_at timestamp with time zone, + sixty_days_notification_sent_at timestamp with time zone, CONSTRAINT check_aa95773861 CHECK ((char_length(advanced_scopes) <= 4096)) ); @@ -29435,6 +29438,12 @@ CREATE INDEX index_path_locks_on_project_id ON path_locks USING btree (project_i CREATE INDEX index_path_locks_on_user_id ON path_locks USING btree (user_id); +CREATE INDEX index_pats_on_expiring_at_seven_days_notification_sent_at ON personal_access_tokens USING btree (expires_at, id) WHERE ((impersonation = false) AND (revoked = false) AND (seven_days_notification_sent_at IS NULL)); + +CREATE INDEX index_pats_on_expiring_at_sixty_days_notification_sent_at ON personal_access_tokens USING btree (expires_at, id) WHERE ((impersonation = false) AND (revoked = false) AND (sixty_days_notification_sent_at IS NULL)); + +CREATE INDEX index_pats_on_expiring_at_thirty_days_notification_sent_at ON personal_access_tokens USING btree (expires_at, id) WHERE ((impersonation = false) AND (revoked = false) AND (thirty_days_notification_sent_at IS NULL)); + CREATE INDEX index_pe_approval_rules_on_required_approvals_and_created_at ON protected_environment_approval_rules USING btree (required_approvals, created_at); CREATE INDEX index_personal_access_tokens_on_id_and_created_at ON personal_access_tokens USING btree (id, created_at); -- GitLab From 529c69607e91beeb2437b205ec5d7d391a630e45 Mon Sep 17 00:00:00 2001 From: agius Date: Fri, 6 Sep 2024 10:34:53 -0700 Subject: [PATCH 2/3] Remove comments and with_lock_retries per review feedback --- ...fication_cols_to_personal_access_tokens.rb | 35 ++++--------------- ...0619_add_indices_for_pat_expiry_columns.rb | 17 --------- 2 files changed, 6 insertions(+), 46 deletions(-) diff --git a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb index 6946f182bd4107..7333dba2f56ffd 100644 --- a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb +++ b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb @@ -4,40 +4,17 @@ # for more information on how to write migrations for GitLab. class AddNotificationColsToPersonalAccessTokens < Gitlab::Database::Migration[2.2] - # When using the methods "add_concurrent_index" or "remove_concurrent_index" - # you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - disable_ddl_transaction! - # - # Configure the `gitlab_schema` to perform data manipulation (DML). - # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html - # restrict_gitlab_migration gitlab_schema: :gitlab_main - - # Add dependent 'batched_background_migrations.queued_migration_version' values. - # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] milestone '17.4' def up - with_lock_retries do - add_column :personal_access_tokens, :seven_days_notification_sent_at, :datetime_with_timezone - add_column :personal_access_tokens, :thirty_days_notification_sent_at, :datetime_with_timezone - add_column :personal_access_tokens, :sixty_days_notification_sent_at, :datetime_with_timezone - end + add_column :personal_access_tokens, :seven_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :thirty_days_notification_sent_at, :datetime_with_timezone + add_column :personal_access_tokens, :sixty_days_notification_sent_at, :datetime_with_timezone end def down - with_lock_retries do - remove_column :personal_access_tokens, :seven_days_notification_sent_at - remove_column :personal_access_tokens, :thirty_days_notification_sent_at - remove_column :personal_access_tokens, :sixty_days_notification_sent_at - end + remove_column :personal_access_tokens, :seven_days_notification_sent_at + remove_column :personal_access_tokens, :thirty_days_notification_sent_at + remove_column :personal_access_tokens, :sixty_days_notification_sent_at end end diff --git a/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb index 443137dc29ba10..fb3a019fa3d930 100644 --- a/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb +++ b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb @@ -4,25 +4,8 @@ # for more information on how to write migrations for GitLab. class AddIndicesForPatExpiryColumns < Gitlab::Database::Migration[2.2] - # When using the methods "add_concurrent_index" or "remove_concurrent_index" - # you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: disable_ddl_transaction! - # - # Configure the `gitlab_schema` to perform data manipulation (DML). - # Visit: https://docs.gitlab.com/ee/development/database/migrations_for_multiple_databases.html - # restrict_gitlab_migration gitlab_schema: :gitlab_main - # Add dependent 'batched_background_migrations.queued_migration_version' values. - # DEPENDENT_BATCHED_BACKGROUND_MIGRATIONS = [] milestone '17.4' def up -- GitLab From bfa7f832051b8e0d190552310ac83d864ec0cb54 Mon Sep 17 00:00:00 2001 From: agius Date: Fri, 6 Sep 2024 10:42:13 -0700 Subject: [PATCH 3/3] Remove helper comments per review feedback --- ...22195511_add_notification_cols_to_personal_access_tokens.rb | 3 --- .../20240822200619_add_indices_for_pat_expiry_columns.rb | 3 --- 2 files changed, 6 deletions(-) diff --git a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb index 7333dba2f56ffd..e934da182fd1a1 100644 --- a/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb +++ b/db/migrate/20240822195511_add_notification_cols_to_personal_access_tokens.rb @@ -1,8 +1,5 @@ # frozen_string_literal: true -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - class AddNotificationColsToPersonalAccessTokens < Gitlab::Database::Migration[2.2] milestone '17.4' diff --git a/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb index fb3a019fa3d930..73f1ab9225a667 100644 --- a/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb +++ b/db/post_migrate/20240822200619_add_indices_for_pat_expiry_columns.rb @@ -1,8 +1,5 @@ # frozen_string_literal: true -# See https://docs.gitlab.com/ee/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - class AddIndicesForPatExpiryColumns < Gitlab::Database::Migration[2.2] disable_ddl_transaction! -- GitLab