diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index cb201d05306..0291e87f7d1 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -6ca31a17e3225737d5cf9deba18c9b18951120a8 +20fe02e33885f459cddfb32717e02141d01ab12f diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 54a4c4be6a8..570a6dd175c 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -65,9 +65,7 @@ module Emails @target_url = profile_personal_access_tokens_url @token_name = token_name - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("A new personal access token has been created"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("A new personal access token has been created"))) end def access_token_about_to_expire_email(user, token_names) @@ -78,9 +76,7 @@ module Emails @target_url = profile_personal_access_tokens_url @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire })) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire })) end def access_token_expired_email(user, token_names = []) @@ -90,9 +86,7 @@ module Emails @token_names = token_names @target_url = profile_personal_access_tokens_url - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens have expired"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens have expired"))) end def access_token_revoked_email(user, token_name, source = nil) @@ -103,9 +97,7 @@ module Emails @target_url = profile_personal_access_tokens_url @source = source - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("A personal access token has been revoked"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("A personal access token has been revoked"))) end def ssh_key_expired_email(user, fingerprints) @@ -115,9 +107,7 @@ module Emails @fingerprints = fingerprints @target_url = profile_keys_url - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your SSH key has expired"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your SSH key has expired"))) end def ssh_key_expiring_soon_email(user, fingerprints) @@ -127,9 +117,7 @@ module Emails @fingerprints = fingerprints @target_url = profile_keys_url - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your SSH key is expiring soon."))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Your SSH key is expiring soon."))) end def unknown_sign_in_email(user, ip, time) @@ -138,11 +126,9 @@ module Emails @time = time @target_url = edit_profile_password_url - Gitlab::I18n.with_locale(@user.preferred_language) do - email_with_layout( - to: @user.notification_email_or_default, - subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host })) - end + email_with_layout( + to: @user.notification_email_or_default, + subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host })) end def two_factor_otp_attempt_failed_email(user, ip, time = Time.current) @@ -150,11 +136,9 @@ module Emails @ip = ip @time = time - Gitlab::I18n.with_locale(@user.preferred_language) do - email_with_layout( - to: @user.notification_email_or_default, - subject: subject(_("Attempted sign in to %{host} using an incorrect verification code") % { host: Gitlab.config.gitlab.host })) - end + email_with_layout( + to: @user.notification_email_or_default, + subject: subject(_("Attempted sign in to %{host} using an incorrect verification code") % { host: Gitlab.config.gitlab.host })) end def disabled_two_factor_email(user) @@ -162,9 +146,7 @@ module Emails @user = user - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Two-factor authentication disabled"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("Two-factor authentication disabled"))) end def new_email_address_added_email(user, email) @@ -173,9 +155,7 @@ module Emails @user = user @email = email - Gitlab::I18n.with_locale(@user.preferred_language) do - mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("New email address added"))) - end + mail_with_locale(to: @user.notification_email_or_default, subject: subject(_("New email address added"))) end def new_achievement_email(user, achievement) @@ -184,11 +164,9 @@ module Emails @user = user @achievement = achievement - Gitlab::I18n.with_locale(@user.preferred_language) do - email_with_layout( - to: @user.notification_email_or_default, - subject: subject(s_("Achievements|%{namespace_full_path} awarded you the %{achievement_name} achievement") % { namespace_full_path: @achievement.namespace.full_path, achievement_name: @achievement.name })) - end + email_with_layout( + to: @user.notification_email_or_default, + subject: subject(s_("Achievements|%{namespace_full_path} awarded you the %{achievement_name} achievement") % { namespace_full_path: @achievement.namespace.full_path, achievement_name: @achievement.name })) end end end diff --git a/config/application.rb b/config/application.rb index 05f3a726e27..1e2a6a69dc8 100644 --- a/config/application.rb +++ b/config/application.rb @@ -30,13 +30,6 @@ module Gitlab # Rails 7.0 config.action_controller.raise_on_open_redirects = false - config.action_dispatch.default_headers = { "X-Frame-Options" => "SAMEORIGIN", - "X-XSS-Protection" => "1; mode=block", - "X-Content-Type-Options" => "nosniff", - "X-Download-Options" => "noopen", - "X-Permitted-Cross-Domain-Policies" => "none", - "Referrer-Policy" => "strict-origin-when-cross-origin" } - config.action_dispatch.return_only_request_media_type_on_content_type = true config.action_mailer.smtp_timeout = nil # New default is 5 config.action_view.button_to_generates_button_tag = nil # New default is true diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 8c21767f583..a3a600084d6 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -4890,7 +4890,7 @@ Input type: `MergeRequestUpdateApprovalRuleInput` | `iid` | [`String!`](#string) | IID of the merge request to mutate. | | `name` | [`String!`](#string) | Name of the approval rule. | | `projectPath` | [`ID!`](#id) | Project the merge request to mutate is in. | -| `removeHiddenGroups` | [`[Boolean!]`](#boolean) | Whether hidden groups should be removed. | +| `removeHiddenGroups` | [`Boolean`](#boolean) | Whether hidden groups should be removed. | | `userIds` | [`[String!]`](#string) | IDs of users as approvers. | #### Fields diff --git a/lib/gitlab/database/load_balancing/load_balancer.rb b/lib/gitlab/database/load_balancing/load_balancer.rb index b7123784010..f6144b7b772 100644 --- a/lib/gitlab/database/load_balancing/load_balancer.rb +++ b/lib/gitlab/database/load_balancing/load_balancer.rb @@ -12,6 +12,8 @@ module Gitlab REPLICA_SUFFIX = '_replica' + attr_accessor :service_discovery + attr_reader :host_list, :configuration # configuration - An instance of `LoadBalancing::Configuration` that @@ -45,6 +47,8 @@ module Gitlab # If no secondaries were available this method will use the primary # instead. def read(&block) + service_discovery&.log_refresh_thread_interruption + conflict_retried = 0 while host @@ -103,6 +107,8 @@ module Gitlab # Yields a connection that can be used for both reads and writes. def read_write + service_discovery&.log_refresh_thread_interruption + connection = nil transaction_open = nil diff --git a/lib/gitlab/database/load_balancing/service_discovery.rb b/lib/gitlab/database/load_balancing/service_discovery.rb index 57a588db8a8..b260945bf9e 100644 --- a/lib/gitlab/database/load_balancing/service_discovery.rb +++ b/lib/gitlab/database/load_balancing/service_discovery.rb @@ -15,12 +15,14 @@ module Gitlab class ServiceDiscovery EmptyDnsResponse = Class.new(StandardError) + attr_accessor :refresh_thread, :refresh_thread_last_run, :refresh_thread_interruption_logged + attr_reader :interval, :record, :record_type, :disconnect_timeout, :load_balancer MAX_SLEEP_ADJUSTMENT = 10 - MAX_DISCOVERY_RETRIES = 3 + DISCOVERY_THREAD_REFRESH_DELTA = 3 RETRY_DELAY_RANGE = (0.1..0.2).freeze @@ -74,8 +76,10 @@ module Gitlab # rubocop:enable Metrics/ParameterLists def start - Thread.new do + self.refresh_thread = Thread.new do loop do + self.refresh_thread_last_run = Time.current + next_sleep_duration = perform_service_discovery # We slightly randomize the sleep() interval. This should reduce @@ -103,15 +107,6 @@ module Gitlab # Slightly randomize the retry delay so that, in the case of a total # dns outage, all starting services do not pressure the dns server at the same time. sleep(rand(RETRY_DELAY_RANGE)) - rescue Exception => error # rubocop:disable Lint/RescueException - # All exceptions are logged to find any pattern and solve https://gitlab.com/gitlab-org/gitlab/-/issues/364370 - # This will be removed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/120173 - Gitlab::Database::LoadBalancing::Logger.error( - event: :service_discovery_unexpected_exception, - message: "Service discovery encountered an uncaught error: #{error.message}" - ) - - raise end interval @@ -214,6 +209,20 @@ module Gitlab ) end + def log_refresh_thread_interruption + return if refresh_thread_last_run.blank? || refresh_thread_interruption_logged || + (refresh_thread_last_run + DISCOVERY_THREAD_REFRESH_DELTA.minutes).future? + + Gitlab::Database::LoadBalancing::Logger.error( + event: :service_discovery_refresh_thread_interrupt, + refresh_thread_last_run: refresh_thread_last_run, + thread_status: refresh_thread&.status&.to_s, + thread_backtrace: refresh_thread&.backtrace&.join('\n') + ) + + self.refresh_thread_interruption_logged = true + end + private def record_type_for(type) diff --git a/lib/gitlab/database/load_balancing/setup.rb b/lib/gitlab/database/load_balancing/setup.rb index eceea1d8d9c..2e65e1c8e56 100644 --- a/lib/gitlab/database/load_balancing/setup.rb +++ b/lib/gitlab/database/load_balancing/setup.rb @@ -55,6 +55,8 @@ module Gitlab sv = ServiceDiscovery.new(load_balancer, **configuration.service_discovery) + load_balancer.service_discovery = sv + sv.perform_service_discovery sv.start if @start_service_discovery diff --git a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb index 997c7a31cba..26c8969efd8 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do +RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store, feature_category: :database do let(:conflict_error) { Class.new(RuntimeError) } let(:model) { ActiveRecord::Base } let(:db_host) { model.connection_pool.db_config.host } @@ -71,7 +71,30 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end end + shared_examples 'logs service discovery thread interruption' do |lb_method| + context 'with service discovery' do + let(:service_discovery) do + instance_double( + Gitlab::Database::LoadBalancing::ServiceDiscovery, + log_refresh_thread_interruption: true + ) + end + + before do + allow(lb).to receive(:service_discovery).and_return(service_discovery) + end + + it 'calls logs service discovery thread interruption' do + expect(service_discovery).to receive(:log_refresh_thread_interruption) + + lb.public_send(lb_method) {} + end + end + end + describe '#read' do + it_behaves_like 'logs service discovery thread interruption', :read + it 'yields a connection for a read' do connection = double(:connection) host = double(:host) @@ -203,6 +226,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end describe '#read_write' do + it_behaves_like 'logs service discovery thread interruption', :read_write + it 'yields a connection for a write' do connection = ActiveRecord::Base.connection_pool.connection diff --git a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb index 9a559c7ccb4..789919d2a51 100644 --- a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb @@ -58,14 +58,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego end end - describe '#start' do + describe '#start', :freeze_time do before do allow(service) .to receive(:loop) .and_yield end - it 'starts service discovery in a new thread' do + it 'starts service discovery in a new thread with proper assignments' do expect(Thread).to receive(:new).ordered.and_call_original # Thread starts expect(service).to receive(:perform_service_discovery).ordered.and_return(5) @@ -73,6 +73,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego expect(service).to receive(:sleep).ordered.with(7) # Sleep runs after thread starts service.start.join + + expect(service.refresh_thread_last_run).to eq(Time.current) + expect(service.refresh_thread).to be_present end end @@ -142,21 +145,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego service.perform_service_discovery end end - - context 'with Exception' do - it 'logs error and re-raises the exception' do - error = Exception.new('uncaught-test-error') - - expect(service).to receive(:refresh_if_necessary).and_raise(error) - - expect(Gitlab::Database::LoadBalancing::Logger).to receive(:error).with( - event: :service_discovery_unexpected_exception, - message: "Service discovery encountered an uncaught error: uncaught-test-error" - ) - - expect { service.perform_service_discovery }.to raise_error(Exception, error.message) - end - end end describe '#refresh_if_necessary' do @@ -427,4 +415,64 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego end end end + + describe '#log_refresh_thread_interruption' do + before do + service.refresh_thread = refresh_thread + service.refresh_thread_last_run = last_run_timestamp + end + + let(:refresh_thread) { nil } + let(:last_run_timestamp) { nil } + + subject { service.log_refresh_thread_interruption } + + context 'without refresh thread timestamp' do + it 'does not log any interruption' do + expect(service.refresh_thread_last_run).to be_nil + + expect(Gitlab::Database::LoadBalancing::Logger).not_to receive(:error) + + subject + end + end + + context 'with refresh thread timestamp' do + let(:last_run_timestamp) { Time.current } + + it 'does not log if last run time plus delta is in future' do + expect(Gitlab::Database::LoadBalancing::Logger).not_to receive(:error) + + subject + end + + context 'with way past last run timestamp' do + let(:refresh_thread) { instance_double(Thread, status: :run, backtrace: %w[backtrace foo]) } + let(:last_run_timestamp) { 20.minutes.before + described_class::DISCOVERY_THREAD_REFRESH_DELTA.minutes } + + it 'does not log if the interruption is already logged' do + service.refresh_thread_interruption_logged = true + + expect(Gitlab::Database::LoadBalancing::Logger).not_to receive(:error) + + subject + end + + it 'logs the error if the interruption was not logged before' do + expect(service.refresh_thread_interruption_logged).not_to be_present + + expect(Gitlab::Database::LoadBalancing::Logger).to receive(:error).with( + event: :service_discovery_refresh_thread_interrupt, + refresh_thread_last_run: last_run_timestamp, + thread_status: refresh_thread.status.to_s, + thread_backtrace: 'backtrace\nfoo' + ) + + subject + + expect(service.refresh_thread_interruption_logged).to be_truthy + end + end + end + end end