Merge branch '32115-structured-logging-mail_room' into 'master'

Enable `mail_room`'s structured logging in GitLab

Closes #35108

See merge request gitlab-org/gitlab!19186

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'

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)

changelogs/unreleased/32115-structured-logging-mail_room.yml

+---
+title: Upgrade `mail_room` gem to 0.10.0 and enable structured logging
+merge_request: 19186
+author:
+type: added

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:

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 %>
 

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

lib/gitlab/mail_room.rb

@@ -4,15 +4,21 @@ require 'yaml'
 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 @@ module Gitlab
       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 @@ module Gitlab
           end
         end
 
+        config[:log_path] = File.expand_path(config[:log_path], RAILS_ROOT_DIR)
         config
       end
 
@@ -57,6 +64,10 @@ module Gitlab
       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

spec/lib/gitlab/mail_room/mail_room_spec.rb

+# 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

spec/support/shared_examples/mail_room_shared_examples.rb

+# 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