diff --git a/app/models/concerns/from_set_operator.rb b/app/models/concerns/from_set_operator.rb index ce3a83e9fa1..56b788eb1ab 100644 --- a/app/models/concerns/from_set_operator.rb +++ b/app/models/concerns/from_set_operator.rb @@ -10,7 +10,9 @@ module FromSetOperator raise "Trying to redefine method '#{method(method_name)}'" if methods.include?(method_name) - define_method(method_name) do |members, remove_duplicates: true, remove_order: true, alias_as: table_name| + define_method(method_name) do |*members, remove_duplicates: true, remove_order: true, alias_as: table_name| + members = flatten_ar_array(members) + operator_sql = if members.any? operator.new(members, remove_duplicates: remove_duplicates, remove_order: remove_order).to_sql @@ -20,5 +22,26 @@ module FromSetOperator from(Arel.sql("(#{operator_sql}) #{alias_as}")) end + + # Array#flatten with ActiveRecord::Relation items will load the ActiveRecord::Relation. + # Therefore we need to roll our own flatten method. + unless method_defined?(:flatten_ar_array) # rubocop:disable Style/GuardClause + define_method :flatten_ar_array do |ary| + arrays = ary.dup + result = [] + + until arrays.empty? + item = arrays.shift + if item.is_a?(Array) + arrays.concat(item.dup) + else + result.push(item) + end + end + + result + end + private :flatten_ar_array + end end end diff --git a/app/services/projects/alerting/notify_service.rb b/app/services/projects/alerting/notify_service.rb index c21a61bcb52..408d9dd24f2 100644 --- a/app/services/projects/alerting/notify_service.rb +++ b/app/services/projects/alerting/notify_service.rb @@ -2,14 +2,13 @@ module Projects module Alerting - class NotifyService + class NotifyService < ::BaseProjectService extend ::Gitlab::Utils::Override include ::AlertManagement::AlertProcessing include ::AlertManagement::Responses - def initialize(project, payload) - @project = project - @payload = payload + def initialize(project, params) + super(project: project, params: params.to_h) end def execute(token, integration = nil) @@ -29,10 +28,11 @@ module Projects private - attr_reader :project, :payload, :integration + attr_reader :integration + alias_method :payload, :params def valid_payload_size? - Gitlab::Utils::DeepSize.new(payload.to_h).valid? + Gitlab::Utils::DeepSize.new(params).valid? end override :alert_source diff --git a/app/services/projects/prometheus/alerts/notify_service.rb b/app/services/projects/prometheus/alerts/notify_service.rb index 6265a74fad2..9f260345937 100644 --- a/app/services/projects/prometheus/alerts/notify_service.rb +++ b/app/services/projects/prometheus/alerts/notify_service.rb @@ -3,9 +3,8 @@ module Projects module Prometheus module Alerts - class NotifyService + class NotifyService < ::BaseProjectService include Gitlab::Utils::StrongMemoize - include ::IncidentManagement::Settings include ::AlertManagement::Responses # This set of keys identifies a payload as a valid Prometheus @@ -26,14 +25,13 @@ module Projects # https://gitlab.com/gitlab-com/gl-infra/production/-/issues/6086 PROCESS_MAX_ALERTS = 100 - def initialize(project, payload) - @project = project - @payload = payload + def initialize(project, params) + super(project: project, params: params.to_h) end def execute(token, integration = nil) return bad_request unless valid_payload_size? - return unprocessable_entity unless self.class.processable?(payload) + return unprocessable_entity unless self.class.processable?(params) return unauthorized unless valid_alert_manager_token?(token, integration) truncate_alerts! if max_alerts_exceeded? @@ -53,10 +51,8 @@ module Projects private - attr_reader :project, :payload - def valid_payload_size? - Gitlab::Utils::DeepSize.new(payload.to_h).valid? + Gitlab::Utils::DeepSize.new(params).valid? end def max_alerts_exceeded? @@ -75,11 +71,11 @@ module Projects } ) - payload['alerts'] = alerts.first(PROCESS_MAX_ALERTS) + params['alerts'] = alerts.first(PROCESS_MAX_ALERTS) end def alerts - payload['alerts'] + params['alerts'] end def valid_alert_manager_token?(token, integration) @@ -152,7 +148,7 @@ module Projects def process_prometheus_alerts alerts.map do |alert| AlertManagement::ProcessPrometheusAlertService - .new(project, alert.to_h) + .new(project, alert) .execute end end diff --git a/db/post_migrate/20220820221036_update_tmp_non_migrated_index_on_container_repositories.rb b/db/post_migrate/20220820221036_update_tmp_non_migrated_index_on_container_repositories.rb new file mode 100644 index 00000000000..eea58ad7951 --- /dev/null +++ b/db/post_migrate/20220820221036_update_tmp_non_migrated_index_on_container_repositories.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class UpdateTmpNonMigratedIndexOnContainerRepositories < Gitlab::Database::Migration[2.0] + disable_ddl_transaction! + + NEW_INDEX_NAME = 'tmp_index_container_repos_on_non_migrated' + OLD_INDEX_NAME = 'tmp_idx_container_repos_on_non_migrated' + MIGRATION_PHASE_1_ENDED_AT = '2022-01-23' + + def up + add_concurrent_index :container_repositories, + [:project_id, :id], + name: NEW_INDEX_NAME, + where: "migration_state != 'import_done'" + remove_concurrent_index_by_name :container_repositories, OLD_INDEX_NAME + end + + def down + add_concurrent_index :container_repositories, + [:project_id, :id], + name: OLD_INDEX_NAME, + where: "migration_state != 'import_done' AND created_at < '#{MIGRATION_PHASE_1_ENDED_AT}'" + remove_concurrent_index_by_name :container_repositories, NEW_INDEX_NAME + end +end diff --git a/db/post_migrate/20220822071909_remove_other_role_from_user_details.rb b/db/post_migrate/20220822071909_remove_other_role_from_user_details.rb new file mode 100644 index 00000000000..a0177bf2605 --- /dev/null +++ b/db/post_migrate/20220822071909_remove_other_role_from_user_details.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class RemoveOtherRoleFromUserDetails < Gitlab::Database::Migration[2.0] + enable_lock_retries! + + def change + remove_column :user_details, :other_role, :text + end +end diff --git a/db/schema_migrations/20220820221036 b/db/schema_migrations/20220820221036 new file mode 100644 index 00000000000..6f7c4059487 --- /dev/null +++ b/db/schema_migrations/20220820221036 @@ -0,0 +1 @@ +16825936e8e6a4f0a1f001a83ecf81f180ee2eb15589eebe821fee2706456cef \ No newline at end of file diff --git a/db/schema_migrations/20220822071909 b/db/schema_migrations/20220822071909 new file mode 100644 index 00000000000..fd8af68d1ee --- /dev/null +++ b/db/schema_migrations/20220822071909 @@ -0,0 +1 @@ +60a72830780190214d6c86fc2d07dc0fc138f6cc258689c1d106bb456b130047 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index b291dfe7591..98622c25058 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -21887,7 +21887,6 @@ CREATE TABLE user_details ( job_title character varying(200) DEFAULT ''::character varying NOT NULL, bio character varying(255) DEFAULT ''::character varying NOT NULL, webauthn_xid text, - other_role text, provisioned_by_group_id bigint, pronouns text, pronunciation text, @@ -21896,7 +21895,6 @@ CREATE TABLE user_details ( requires_credit_card_verification boolean DEFAULT false NOT NULL, CONSTRAINT check_245664af82 CHECK ((char_length(webauthn_xid) <= 100)), CONSTRAINT check_a73b398c60 CHECK ((char_length(phone) <= 50)), - CONSTRAINT check_b132136b01 CHECK ((char_length(other_role) <= 100)), CONSTRAINT check_eeeaf8d4f0 CHECK ((char_length(pronouns) <= 50)), CONSTRAINT check_f932ed37db CHECK ((char_length(pronunciation) <= 255)) ); @@ -30633,14 +30631,14 @@ CREATE UNIQUE INDEX taggings_idx ON taggings USING btree (tag_id, taggable_id, t CREATE UNIQUE INDEX term_agreements_unique_index ON term_agreements USING btree (user_id, term_id); -CREATE INDEX tmp_idx_container_repos_on_non_migrated ON container_repositories USING btree (project_id, id) WHERE ((migration_state <> 'import_done'::text) AND (created_at < '2022-01-23 00:00:00'::timestamp without time zone)); - CREATE INDEX tmp_index_ci_job_artifacts_on_expire_at_where_locked_unknown ON ci_job_artifacts USING btree (expire_at, job_id) WHERE ((locked = 2) AND (expire_at IS NOT NULL)); CREATE INDEX tmp_index_ci_job_artifacts_on_id_where_trace_and_expire_at ON ci_job_artifacts USING btree (id) WHERE ((file_type = 3) AND (expire_at = ANY (ARRAY['2021-04-22 00:00:00+00'::timestamp with time zone, '2021-05-22 00:00:00+00'::timestamp with time zone, '2021-06-22 00:00:00+00'::timestamp with time zone, '2022-01-22 00:00:00+00'::timestamp with time zone, '2022-02-22 00:00:00+00'::timestamp with time zone, '2022-03-22 00:00:00+00'::timestamp with time zone, '2022-04-22 00:00:00+00'::timestamp with time zone]))); CREATE INDEX tmp_index_cis_vulnerability_reads_on_id ON vulnerability_reads USING btree (id) WHERE (report_type = 7); +CREATE INDEX tmp_index_container_repos_on_non_migrated ON container_repositories USING btree (project_id, id) WHERE (migration_state <> 'import_done'::text); + CREATE INDEX tmp_index_container_repositories_on_id_migration_state ON container_repositories USING btree (id, migration_state); CREATE INDEX tmp_index_for_namespace_id_migration_on_group_members ON members USING btree (id) WHERE ((member_namespace_id IS NULL) AND ((type)::text = 'GroupMember'::text)); diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 6f475fa8d74..c4464666020 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -133,11 +133,6 @@ module API 'Could not find a user for the given key' unless actor.user end - # TODO: backwards compatibility; remove after https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/454 is merged - def two_factor_otp_check - { success: false, message: 'Feature is not available' } - end - def two_factor_manual_otp_check { success: false, message: 'Feature is not available' } end @@ -339,13 +334,6 @@ module API end end - # TODO: backwards compatibility; remove after https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/454 is merged - post '/two_factor_otp_check', feature_category: :authentication_and_authorization do - status 200 - - two_factor_manual_otp_check - end - post '/two_factor_push_otp_check', feature_category: :authentication_and_authorization do status 200 diff --git a/lib/gitlab/database/lock_writes_manager.rb b/lib/gitlab/database/lock_writes_manager.rb index cd483d616bb..fe75cd763b4 100644 --- a/lib/gitlab/database/lock_writes_manager.rb +++ b/lib/gitlab/database/lock_writes_manager.rb @@ -5,42 +5,63 @@ module Gitlab class LockWritesManager TRIGGER_FUNCTION_NAME = 'gitlab_schema_prevent_write' - def initialize(table_name:, connection:, database_name:, logger: nil) + def initialize(table_name:, connection:, database_name:, logger: nil, dry_run: false) @table_name = table_name @connection = connection @database_name = database_name @logger = logger + @dry_run = dry_run + end + + def table_locked_for_writes?(table_name) + query = <<~SQL + SELECT COUNT(*) from information_schema.triggers + WHERE event_object_table = '#{table_name}' + AND trigger_name = '#{write_trigger_name(table_name)}' + SQL + + connection.select_value(query) == 3 end def lock_writes + if table_locked_for_writes?(table_name) + logger&.info "Skipping lock_writes, because #{table_name} is already locked for writes" + return + end + logger&.info "Database: '#{database_name}', Table: '#{table_name}': Lock Writes".color(:yellow) - sql = <<-SQL - DROP TRIGGER IF EXISTS #{write_trigger_name(table_name)} ON #{table_name}; + sql_statement = <<~SQL CREATE TRIGGER #{write_trigger_name(table_name)} BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE ON #{table_name} FOR EACH STATEMENT EXECUTE FUNCTION #{TRIGGER_FUNCTION_NAME}(); SQL - with_retries(connection) do - connection.execute(sql) - end + execute_sql_statement(sql_statement) end def unlock_writes logger&.info "Database: '#{database_name}', Table: '#{table_name}': Allow Writes".color(:green) - sql = <<-SQL - DROP TRIGGER IF EXISTS #{write_trigger_name(table_name)} ON #{table_name} + sql_statement = <<~SQL + DROP TRIGGER IF EXISTS #{write_trigger_name(table_name)} ON #{table_name}; SQL - with_retries(connection) do - connection.execute(sql) - end + execute_sql_statement(sql_statement) end private - attr_reader :table_name, :connection, :database_name, :logger + attr_reader :table_name, :connection, :database_name, :logger, :dry_run + + def execute_sql_statement(sql) + if dry_run + logger&.info sql + else + with_retries(connection) do + connection.execute(sql) + end + end + end def with_retries(connection, &block) with_statement_timeout_retries do diff --git a/spec/lib/gitlab/database/lock_writes_manager_spec.rb b/spec/lib/gitlab/database/lock_writes_manager_spec.rb index eb527d492cf..b1cc8add55a 100644 --- a/spec/lib/gitlab/database/lock_writes_manager_spec.rb +++ b/spec/lib/gitlab/database/lock_writes_manager_spec.rb @@ -6,13 +6,15 @@ RSpec.describe Gitlab::Database::LockWritesManager do let(:connection) { ApplicationRecord.connection } let(:test_table) { '_test_table' } let(:logger) { instance_double(Logger) } + let(:dry_run) { false } subject(:lock_writes_manager) do described_class.new( table_name: test_table, connection: connection, database_name: 'main', - logger: logger + logger: logger, + dry_run: dry_run ) end @@ -27,6 +29,16 @@ RSpec.describe Gitlab::Database::LockWritesManager do SQL end + describe "#table_locked_for_writes?" do + it 'returns false for a table that is not locked for writes' do + expect(subject.table_locked_for_writes?(test_table)).to eq(false) + end + + it 'returns true for a table that is locked for writes' do + expect { subject.lock_writes }.to change { subject.table_locked_for_writes?(test_table) }.from(false).to(true) + end + end + describe '#lock_writes' do it 'prevents any writes on the table' do subject.lock_writes @@ -84,11 +96,57 @@ RSpec.describe Gitlab::Database::LockWritesManager do subject.lock_writes end.to raise_error(ActiveRecord::QueryCanceled) end + + it 'skips the operation if the table is already locked for writes' do + subject.lock_writes + + expect(logger).to receive(:info).with("Skipping lock_writes, because #{test_table} is already locked for writes") + expect(connection).not_to receive(:execute).with(/CREATE TRIGGER/) + + expect do + subject.lock_writes + end.not_to change { + number_of_triggers_on(connection, test_table) + } + end + + context 'when running in dry_run mode' do + let(:dry_run) { true } + + it 'prints the sql statement to the logger' do + expect(logger).to receive(:info).with("Database: 'main', Table: '#{test_table}': Lock Writes") + expected_sql_statement = <<~SQL + CREATE TRIGGER gitlab_schema_write_trigger_for_#{test_table} + BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE + ON #{test_table} + FOR EACH STATEMENT EXECUTE FUNCTION gitlab_schema_prevent_write(); + SQL + expect(logger).to receive(:info).with(expected_sql_statement) + + subject.lock_writes + end + + it 'does not lock the tables for writes' do + subject.lock_writes + + expect do + connection.execute("delete from #{test_table}") + connection.execute("truncate #{test_table}") + end.not_to raise_error + end + end end describe '#unlock_writes' do before do - subject.lock_writes + # Locking the table without the considering the value of dry_run + described_class.new( + table_name: test_table, + connection: connection, + database_name: 'main', + logger: logger, + dry_run: false + ).lock_writes end it 'allows writing on the table again' do @@ -114,6 +172,28 @@ RSpec.describe Gitlab::Database::LockWritesManager do subject.unlock_writes end + + context 'when running in dry_run mode' do + let(:dry_run) { true } + + it 'prints the sql statement to the logger' do + expect(logger).to receive(:info).with("Database: 'main', Table: '#{test_table}': Allow Writes") + expected_sql_statement = <<~SQL + DROP TRIGGER IF EXISTS gitlab_schema_write_trigger_for_#{test_table} ON #{test_table}; + SQL + expect(logger).to receive(:info).with(expected_sql_statement) + + subject.unlock_writes + end + + it 'does not unlock the tables for writes' do + subject.unlock_writes + + expect do + connection.execute("delete from #{test_table}") + end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{test_table}" is write protected/) + end + end end def number_of_triggers_on(connection, table_name) diff --git a/spec/models/concerns/from_set_operator_spec.rb b/spec/models/concerns/from_set_operator_spec.rb index 8ebbb5550c9..1ef0d1adb08 100644 --- a/spec/models/concerns/from_set_operator_spec.rb +++ b/spec/models/concerns/from_set_operator_spec.rb @@ -3,7 +3,22 @@ require 'spec_helper' RSpec.describe FromSetOperator do - describe 'when set operator method already exists' do + let_it_be(:from_set_operator) do + Class.new do + extend FromSetOperator + define_set_operator Gitlab::SQL::Union + + def table_name + 'groups' + end + + def from(*args) + '' + end + end + end + + context 'when set operator method already exists' do let(:redefine_method) do Class.new do def self.from_union @@ -17,4 +32,38 @@ RSpec.describe FromSetOperator do it { expect { redefine_method }.to raise_exception(RuntimeError) } end + + context 'with members' do + let_it_be(:group1) { create :group } + let_it_be(:group2) { create :group } + let_it_be(:groups) do + [ + Group.where(id: group1), + Group.where(id: group2) + ] + end + + shared_examples 'set operator called with correct members' do + it do + expect(Gitlab::SQL::Union).to receive(:new).with(groups, anything).and_call_original + subject + end + end + + context 'as array' do + subject { from_set_operator.new.from_union(groups) } + + it_behaves_like 'set operator called with correct members' + + it { expect { subject }.not_to make_queries } + end + + context 'as multiple parameters' do + subject { from_set_operator.new.from_union(*groups) } + + it_behaves_like 'set operator called with correct members' + + it { expect { subject }.not_to make_queries } + end + end end diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 9faeadc8433..26849a8eea0 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -1502,24 +1502,6 @@ RSpec.describe API::Internal::Base do end end - describe 'POST /internal/two_factor_otp_check' do - let(:key_id) { key.id } - let(:otp) { '123456' } - - subject do - post api('/internal/two_factor_otp_check'), - params: { key_id: key_id, otp_attempt: otp }, - headers: gitlab_shell_internal_api_request_header - end - - it 'is not available' do - subject - - expect(json_response['success']).to be_falsey - expect(json_response['message']).to eq 'Feature is not available' - end - end - describe 'POST /internal/two_factor_manual_otp_check' do let(:key_id) { key.id } let(:otp) { '123456' }