diff --git a/Gemfile b/Gemfile index c3150bea86273091bebd9b9aea1a3b6ca722692b..2fa325a57147881b8ecac88e95942c0255bfe392 100644 --- a/Gemfile +++ b/Gemfile @@ -417,7 +417,7 @@ end gem 'octokit', '~> 4.9' -gem 'mail_room', '~> 0.9.1' +gem 'mail_room', '~> 0.10.0' gem 'email_reply_trimmer', '~> 0.1' gem 'html2text' diff --git a/Gemfile.lock b/Gemfile.lock index 9913d2d1df9b63e911e45cfbda5f1e0b36ddb65c..30dd5d4027878eabdaab685a09c73296b09bf4e6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -593,7 +593,7 @@ GEM lumberjack (1.0.13) mail (2.7.1) mini_mime (>= 0.1.1) - mail_room (0.9.1) + mail_room (0.10.0) marcel (0.3.3) mimemagic (~> 0.3.2) marginalia (1.8.0) @@ -1247,7 +1247,7 @@ DEPENDENCIES licensee (~> 8.9) lograge (~> 0.5) loofah (~> 2.2) - mail_room (~> 0.9.1) + mail_room (~> 0.10.0) marginalia (~> 1.8.0) memory_profiler (~> 0.9) method_source (~> 0.8) diff --git a/changelogs/unreleased/32115-structured-logging-mail_room.yml b/changelogs/unreleased/32115-structured-logging-mail_room.yml new file mode 100644 index 0000000000000000000000000000000000000000..17b678a82d4e72497d87864adb28e06ef7995995 --- /dev/null +++ b/changelogs/unreleased/32115-structured-logging-mail_room.yml @@ -0,0 +1,5 @@ +--- +title: Upgrade `mail_room` gem to 0.10.0 and enable structured logging +merge_request: 19186 +author: +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index a7b0a0f0b62aecd23d14171b8a3c41f384274de0..5ac9b7ee6e5143eb28aec3fb485e40b200fd43df 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -181,6 +181,11 @@ production: &base mailbox: "inbox" # The IDLE command timeout. idle_timeout: 60 + # The log file path for the structured log file. + # Since `mail_room` is run independently of Rails, an absolute path is preferred. + # The default is 'log/mail_room_json.log' relative to the root of the Rails app. + # + # log_path: log/mail_room_json.log ## Build Artifacts artifacts: diff --git a/config/mail_room.yml b/config/mail_room.yml index c3a5be8d38c5ab1a2f19b2500c42c9a1cc0535f5..75024c2b2e18f1172c9290900aa550bbcca555b9 100644 --- a/config/mail_room.yml +++ b/config/mail_room.yml @@ -13,6 +13,8 @@ :email: <%= config[:user].to_json %> :password: <%= config[:password].to_json %> :idle_timeout: <%= config[:idle_timeout].to_json %> + :logger: + :log_path: <%= config[:log_path].to_json %> :name: <%= config[:mailbox].to_json %> diff --git a/doc/administration/logs.md b/doc/administration/logs.md index ed0d402c0308fa8ff4ecc761e89d51f876557b94..f4a1c7542525960a9128cb61353d0a9499d0a21e 100644 --- a/doc/administration/logs.md +++ b/doc/administration/logs.md @@ -360,6 +360,17 @@ Introduced in GitLab 12.3. This file lives in `/var/log/gitlab/gitlab-rails/migr Omnibus GitLab packages or in `/home/git/gitlab/log/migrations.log` for installations from source. +## `mail_room_json.log` (default) + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/merge_requests/19186) in GitLab 12.6. + +This file lives in `/var/log/gitlab/mail_room/mail_room_json.log` for +Omnibus GitLab packages or in `/home/git/gitlab/log/mail_room_json.log` for +installations from source. + +This structured log file records internal activity in the `mail_room` gem. +Its name and path are configurable, so the name and path may not match the above. + ## Reconfigure Logs Reconfigure log files live in `/var/log/gitlab/reconfigure` for Omnibus GitLab diff --git a/lib/gitlab/mail_room.rb b/lib/gitlab/mail_room.rb index 78f2d83c1af55fc230c7e83f1574a54125ac10d5..f7699ef1718ce65d2f84ad19b4719314c27ae00e 100644 --- a/lib/gitlab/mail_room.rb +++ b/lib/gitlab/mail_room.rb @@ -4,15 +4,21 @@ require 'json' require_relative 'redis/queues' unless defined?(Gitlab::Redis::Queues) +# This service is run independently of the main Rails process, +# therefore the `Rails` class and its methods are unavailable. + module Gitlab module MailRoom + RAILS_ROOT_DIR = Pathname.new('../..').expand_path(__dir__).freeze + DEFAULT_CONFIG = { enabled: false, port: 143, ssl: false, start_tls: false, mailbox: 'inbox', - idle_timeout: 60 + idle_timeout: 60, + log_path: RAILS_ROOT_DIR.join('log', 'mail_room_json.log') }.freeze class << self @@ -33,7 +39,7 @@ def reset_config! def fetch_config return {} unless File.exist?(config_file) - config = YAML.load_file(config_file)[rails_env].deep_symbolize_keys[:incoming_email] || {} + config = load_from_yaml || {} config = DEFAULT_CONFIG.merge(config) do |_key, oldval, newval| newval.nil? ? oldval : newval end @@ -47,6 +53,7 @@ def fetch_config end end + config[:log_path] = File.expand_path(config[:log_path], RAILS_ROOT_DIR) config end @@ -57,6 +64,10 @@ def rails_env def config_file ENV['MAIL_ROOM_GITLAB_CONFIG_FILE'] || File.expand_path('../../config/gitlab.yml', __dir__) end + + def load_from_yaml + YAML.load_file(config_file)[rails_env].deep_symbolize_keys[:incoming_email] + end end end end diff --git a/spec/lib/gitlab/mail_room/mail_room_spec.rb b/spec/lib/gitlab/mail_room/mail_room_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cb3e214d38b42e9c068b2621be4a39975a957934 --- /dev/null +++ b/spec/lib/gitlab/mail_room/mail_room_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::MailRoom do + let(:default_port) { 143 } + let(:default_config) do + { + enabled: false, + port: default_port, + ssl: false, + start_tls: false, + mailbox: 'inbox', + idle_timeout: 60, + log_path: Rails.root.join('log', 'mail_room_json.log').to_s + } + end + + before do + described_class.reset_config! + allow(File).to receive(:exist?).and_return true + end + + describe '#config' do + context 'if the yml file cannot be found' do + before do + allow(File).to receive(:exist?).and_return false + end + + it 'returns an empty hash' do + expect(described_class.config).to be_empty + end + end + + before do + allow(described_class).to receive(:load_from_yaml).and_return(default_config) + end + + it 'sets up config properly' do + expected_result = default_config + + expect(described_class.config).to match expected_result + end + + context 'when a config value is missing from the yml file' do + it 'overwrites missing values with the default' do + stub_config(port: nil) + + expect(described_class.config[:port]).to eq default_port + end + end + + describe 'setting up redis settings' do + let(:fake_redis_queues) { double(url: "localhost", sentinels: "yes, them", sentinels?: true) } + + before do + allow(Gitlab::Redis::Queues).to receive(:new).and_return(fake_redis_queues) + end + + target_proc = proc { described_class.config[:redis_url] } + + it_behaves_like 'only truthy if both enabled and address are truthy', target_proc + end + + describe 'setting up the log path' do + context 'if the log path is a relative path' do + it 'expands the log path to an absolute value' do + stub_config(log_path: 'tiny_log.log') + + new_path = Pathname.new(described_class.config[:log_path]) + expect(new_path.absolute?).to be_truthy + end + end + + context 'if the log path is absolute path' do + it 'leaves the path as-is' do + new_path = '/dev/null' + stub_config(log_path: new_path) + + expect(described_class.config[:log_path]).to eq new_path + end + end + end + end + + describe '#enabled?' do + target_proc = proc { described_class.enabled? } + + it_behaves_like 'only truthy if both enabled and address are truthy', target_proc + end + + describe '#reset_config?' do + it 'resets config' do + described_class.instance_variable_set(:@config, { some_stuff: 'hooray' }) + + described_class.reset_config! + + expect(described_class.instance_variable_get(:@config)).to be_nil + end + end + + def stub_config(override_values) + modified_config = default_config.merge(override_values) + allow(described_class).to receive(:load_from_yaml).and_return(modified_config) + end +end diff --git a/spec/support/shared_examples/mail_room_shared_examples.rb b/spec/support/shared_examples/mail_room_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..4cca29250e27f11eb89c43ef8efe1231183eab5c --- /dev/null +++ b/spec/support/shared_examples/mail_room_shared_examples.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +shared_examples_for 'only truthy if both enabled and address are truthy' do |target_proc| + context 'with both enabled and address as truthy values' do + it 'is truthy' do + stub_config(enabled: true, address: 'localhost') + + expect(target_proc.call).to be_truthy + end + end + + context 'with address only as truthy' do + it 'is falsey' do + stub_config(enabled: false, address: 'localhost') + + expect(target_proc.call).to be_falsey + end + end + + context 'with enabled only as truthy' do + it 'is falsey' do + stub_config(enabled: true, address: nil) + + expect(target_proc.call).to be_falsey + end + end + + context 'with neither address nor enabled as truthy' do + it 'is falsey' do + stub_config(enabled: false, address: nil) + + expect(target_proc.call).to be_falsey + end + end +end