From f32e696ec3a9a17fc84c204ff34a9098e5bcfcf7 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Fri, 25 Oct 2019 23:33:42 +1300 Subject: [PATCH 1/4] Update `mail_room` gem - add config option for `log_path` - add default config option to example yml --- Gemfile | 2 +- Gemfile.lock | 4 ++-- config/gitlab.yml.example | 2 ++ config/mail_room.yml | 2 ++ 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index c3150bea8627..2fa325a57147 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 9913d2d1df9b..30dd5d402787 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/config/gitlab.yml.example b/config/gitlab.yml.example index a7b0a0f0b62a..d4087c3f8512 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -181,6 +181,8 @@ production: &base mailbox: "inbox" # The IDLE command timeout. idle_timeout: 60 + # The default log file. To switch off structured logging, remove + log_path: "mail_room_structured.log" ## Build Artifacts artifacts: diff --git a/config/mail_room.yml b/config/mail_room.yml index c3a5be8d38c5..75024c2b2e18 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 %> -- GitLab From 504fdb9090de4fbeb7a6bd8b8754bb00a52c89ef Mon Sep 17 00:00:00 2001 From: charlie ablett Date: Thu, 31 Oct 2019 05:51:27 +0000 Subject: [PATCH 2/4] Change logfile to mail_room_json - Modify mailroom logfile key - add value to DEFAULT_CONFIG - Add logfile entry in docs - Add absolute path for default config - Integrate log file preferences with Omnibus - Warn against relative paths in config (use absolute paths) - Add Omnibus branch config as per https://docs.gitlab.com/ee/development/build_test_package.html --- .../unreleased/32115-structured-logging-mail_room.yml | 5 +++++ config/gitlab.yml.example | 6 ++++-- doc/administration/logs.md | 9 +++++++++ lib/gitlab/mail_room.rb | 5 ++++- 4 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/32115-structured-logging-mail_room.yml 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 000000000000..17b678a82d4e --- /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 d4087c3f8512..a1ff84e53b10 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -181,8 +181,10 @@ production: &base mailbox: "inbox" # The IDLE command timeout. idle_timeout: 60 - # The default log file. To switch off structured logging, remove - log_path: "mail_room_structured.log" + # The default log file path. + # An absolute path is preferred, otherwise your logs might wind up in an unexpected place. + # depending on where `bin/mail_room` is run. + log_path: "log/mail_room_json.log" ## Build Artifacts artifacts: diff --git a/doc/administration/logs.md b/doc/administration/logs.md index ed0d402c0308..d82751b337a0 100644 --- a/doc/administration/logs.md +++ b/doc/administration/logs.md @@ -360,6 +360,15 @@ 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 in GitLab 12.5. 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 78f2d83c1af5..8070692171a9 100644 --- a/lib/gitlab/mail_room.rb +++ b/lib/gitlab/mail_room.rb @@ -6,13 +6,16 @@ module Gitlab module MailRoom + rails_root_dir = Pathname.new('..').expand_path(File.dirname(__FILE__)) + 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 -- GitLab From 20e963177930883e275bca7748b206793472bea2 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Wed, 11 Dec 2019 15:58:51 +1300 Subject: [PATCH 3/4] Change example default text - Use constantised file path for default config --- config/gitlab.yml.example | 9 +++++---- doc/administration/logs.md | 2 +- lib/gitlab/mail_room.rb | 5 +++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index a1ff84e53b10..5ac9b7ee6e51 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -181,10 +181,11 @@ production: &base mailbox: "inbox" # The IDLE command timeout. idle_timeout: 60 - # The default log file path. - # An absolute path is preferred, otherwise your logs might wind up in an unexpected place. - # depending on where `bin/mail_room` is run. - log_path: "log/mail_room_json.log" + # 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/doc/administration/logs.md b/doc/administration/logs.md index d82751b337a0..be06db6d7648 100644 --- a/doc/administration/logs.md +++ b/doc/administration/logs.md @@ -362,7 +362,7 @@ installations from source. ## `mail_room_json.log` (default) -Introduced in GitLab 12.5. This file lives in `/var/log/gitlab/mail_room/mail_room_json.log` for +Introduced 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. diff --git a/lib/gitlab/mail_room.rb b/lib/gitlab/mail_room.rb index 8070692171a9..fde8c842265d 100644 --- a/lib/gitlab/mail_room.rb +++ b/lib/gitlab/mail_room.rb @@ -6,7 +6,7 @@ module Gitlab module MailRoom - rails_root_dir = Pathname.new('..').expand_path(File.dirname(__FILE__)) + RAILS_ROOT_DIR = Pathname.new('..').expand_path(File.dirname(__FILE__)).freeze DEFAULT_CONFIG = { enabled: false, @@ -15,7 +15,7 @@ module MailRoom start_tls: false, mailbox: 'inbox', idle_timeout: 60, - log_path: rails_root_dir.join('log', 'mail_room_json.log') + log_path: RAILS_ROOT_DIR.join('log', 'mail_room_json.log') }.freeze class << self @@ -50,6 +50,7 @@ def fetch_config end end + config[:log_path] = File.expand_path(config[:log_path], RAILS_ROOT_DIR) config end -- GitLab From d90f93b0d697e95df32d3bd5bb8938029492de71 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 16 Dec 2019 16:17:47 +1300 Subject: [PATCH 4/4] Add rspec test for `mail_room.rb` - Test methods - Test config defaults --- doc/administration/logs.md | 4 +- lib/gitlab/mail_room.rb | 11 +- spec/lib/gitlab/mail_room/mail_room_spec.rb | 106 ++++++++++++++++++ .../mail_room_shared_examples.rb | 35 ++++++ 4 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 spec/lib/gitlab/mail_room/mail_room_spec.rb create mode 100644 spec/support/shared_examples/mail_room_shared_examples.rb diff --git a/doc/administration/logs.md b/doc/administration/logs.md index be06db6d7648..f4a1c7542525 100644 --- a/doc/administration/logs.md +++ b/doc/administration/logs.md @@ -362,7 +362,9 @@ installations from source. ## `mail_room_json.log` (default) -Introduced in GitLab 12.6. This file lives in `/var/log/gitlab/mail_room/mail_room_json.log` for +> [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. diff --git a/lib/gitlab/mail_room.rb b/lib/gitlab/mail_room.rb index fde8c842265d..f7699ef1718c 100644 --- a/lib/gitlab/mail_room.rb +++ b/lib/gitlab/mail_room.rb @@ -4,9 +4,12 @@ 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(File.dirname(__FILE__)).freeze + RAILS_ROOT_DIR = Pathname.new('../..').expand_path(__dir__).freeze DEFAULT_CONFIG = { enabled: false, @@ -36,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 @@ -61,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 000000000000..cb3e214d38b4 --- /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 000000000000..4cca29250e27 --- /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 -- GitLab