Merge branch 'sdesk-multi-address' into 'master'

Fetch incoming emails from multiple addresses

See merge request gitlab-org/gitlab!23524

config/mail_room.yml

 :mailboxes:
   <%
     require_relative "../lib/gitlab/mail_room" unless defined?(Gitlab::MailRoom)
-    config = Gitlab::MailRoom.config
-
-    if Gitlab::MailRoom.enabled?
+    Gitlab::MailRoom.enabled_configs.each do |config|
   %>
     -
       :host: <%= config[:host].to_json %>
@@ -24,8 +22,8 @@
       :delivery_options:
         :redis_url: <%= config[:redis_url].to_json %>
         :namespace: <%= Gitlab::Redis::Queues::SIDEKIQ_NAMESPACE %>
-        :queue: email_receiver
-        :worker: EmailReceiverWorker
+        :queue: <%= config[:queue] %>
+        :worker: <%= config[:worker] %>
         <% if config[:sentinels] %>
         :sentinels:
           <% config[:sentinels].each do |sentinel| %>

config/sidekiq_queues.yml

@@ -224,6 +224,8 @@
   - 2
 - - self_monitoring_project_delete
   - 2
+- - service_desk_email_receiver
+  - 1
 - - system_hook_push
   - 1
 - - todos_destroyer

ee/app/workers/all_queues.yml

@@ -465,3 +465,9 @@
   :latency_sensitive: 
   :resource_boundary: :unknown
   :weight: 1
+- :name: service_desk_email_receiver
+  :feature_category: :issue_tracking
+  :has_external_dependencies: 
+  :latency_sensitive: 
+  :resource_boundary: :unknown
+  :weight: 1

ee/app/workers/service_desk_email_receiver_worker.rb

+# frozen_string_literal: true
+
+class ServiceDeskEmailReceiverWorker < EmailReceiverWorker
+  include ApplicationWorker
+
+  def perform(raw)
+    return unless service_desk_enabled?
+
+    raise NotImplementedError
+  end
+
+  private
+
+  def service_desk_enabled?
+    !!config&.enabled && Feature.enabled?(:service_desk_email)
+  end
+
+  def config
+    @config ||= Gitlab.config.service_desk_email
+  end
+end

ee/spec/workers/service_desk_email_receiver_worker_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe ServiceDeskEmailReceiverWorker, :mailer do
+  describe '#perform' do
+    let(:worker) { described_class.new }
+    let(:email) { 'foo' }
+
+    context 'when service_desk_email config is enabled' do
+      before do
+        allow(worker).to receive(:config)
+          .and_return(double(enabled: true, address: 'foo'))
+      end
+
+      context 'when service_desk_email feature is enabled' do
+        before do
+          stub_feature_flags(service_desk_email: true)
+        end
+
+        it 'does not ignore the email' do
+          expect { worker.perform(email) }.to raise_error(NotImplementedError)
+        end
+      end
+
+      context 'when service_desk_email feature is disabled' do
+        before do
+          stub_feature_flags(service_desk_email: false)
+        end
+
+        it 'ignores the email' do
+          expect { worker.perform(email) }.not_to raise_error(NotImplementedError)
+        end
+      end
+    end
+
+    context 'when service_desk_email config is disabled' do
+      before do
+        allow(worker).to receive(:config)
+          .and_return(double(enabled: false, address: 'foo'))
+      end
+
+      it 'ignores the email' do
+        expect { worker.perform(email) }.not_to raise_error(NotImplementedError)
+      end
+    end
+  end
+end

lib/gitlab/mail_room.rb

@@ -2,6 +2,7 @@
 
 require 'yaml'
 require 'json'
+require 'pathname'
 require_relative 'redis/queues' unless defined?(Gitlab::Redis::Queues)
 
 # This service is run independently of the main Rails process,
@@ -21,39 +22,60 @@ module Gitlab
       log_path: RAILS_ROOT_DIR.join('log', 'mail_room_json.log')
     }.freeze
 
+    # Email specific configuration which is merged with configuration
+    # fetched from YML config file.
+    ADDRESS_SPECIFIC_CONFIG = {
+      incoming_email: {
+        queue: 'email_receiver',
+        worker: 'EmailReceiverWorker'
+      },
+      service_desk_email: {
+        queue: 'service_desk_email_receiver',
+        worker: 'ServiceDeskEmailReceiverWorker'
+      }
+    }.freeze
+
     class << self
-      def enabled?
-        config[:enabled] && config[:address]
+      def enabled_configs
+        @enabled_configs ||= configs.select { |config| enabled?(config) }
       end
 
-      def config
-        @config ||= fetch_config
-      end
+      private
 
-      def reset_config!
-        @config = nil
+      def enabled?(config)
+        config[:enabled] && !config[:address].to_s.empty?
       end
 
-      private
+      def configs
+        ADDRESS_SPECIFIC_CONFIG.keys.map { |key| fetch_config(key) }
+      end
 
-      def fetch_config
+      def fetch_config(config_key)
         return {} unless File.exist?(config_file)
 
-        config = load_from_yaml || {}
-        config = DEFAULT_CONFIG.merge(config) do |_key, oldval, newval|
+        config = merged_configs(config_key)
+        config.merge!(redis_config) if enabled?(config)
+        config[:log_path] = File.expand_path(config[:log_path], RAILS_ROOT_DIR)
+
+        config
+      end
+
+      def merged_configs(config_key)
+        yml_config = load_yaml.fetch(config_key, {})
+        specific_config = ADDRESS_SPECIFIC_CONFIG.fetch(config_key, {})
+        DEFAULT_CONFIG.merge(specific_config, yml_config) do |_key, oldval, newval|
           newval.nil? ? oldval : newval
         end
+      end
 
-        if config[:enabled] && config[:address]
-          gitlab_redis_queues = Gitlab::Redis::Queues.new(rails_env)
-          config[:redis_url] = gitlab_redis_queues.url
+      def redis_config
+        gitlab_redis_queues = Gitlab::Redis::Queues.new(rails_env)
+        config = { redis_url: gitlab_redis_queues.url }
 
-          if gitlab_redis_queues.sentinels?
-            config[:sentinels] = gitlab_redis_queues.sentinels
-          end
+        if gitlab_redis_queues.sentinels?
+          config[:sentinels] = gitlab_redis_queues.sentinels
         end
 
-        config[:log_path] = File.expand_path(config[:log_path], RAILS_ROOT_DIR)
         config
       end
 
@@ -65,8 +87,8 @@ module Gitlab
         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]
+      def load_yaml
+        @yaml ||= YAML.load_file(config_file)[rails_env].deep_symbolize_keys
       end
     end
   end

spec/config/mail_room_spec.rb

@@ -39,39 +39,31 @@ describe 'mail_room.yml' do
     end
   end
 
-  context 'when incoming email is enabled' do
+  context 'when both incoming email and service desk email are enabled' do
     let(:gitlab_config_path) { 'spec/fixtures/config/mail_room_enabled.yml' }
     let(:queues_config_path) { 'spec/fixtures/config/redis_queues_new_format_host.yml' }
-
     let(:gitlab_redis_queues) { Gitlab::Redis::Queues.new(Rails.env) }
 
     it 'contains the intended configuration' do
-      expect(configuration[:mailboxes].length).to eq(1)
-      mailbox = configuration[:mailboxes].first
-
-      expect(mailbox[:host]).to eq('imap.gmail.com')
-      expect(mailbox[:port]).to eq(993)
-      expect(mailbox[:ssl]).to eq(true)
-      expect(mailbox[:start_tls]).to eq(false)
-      expect(mailbox[:email]).to eq('gitlab-incoming@gmail.com')
-      expect(mailbox[:password]).to eq('[REDACTED]')
-      expect(mailbox[:name]).to eq('inbox')
-      expect(mailbox[:idle_timeout]).to eq(60)
-
-      redis_url = gitlab_redis_queues.url
-      sentinels = gitlab_redis_queues.sentinels
-
-      expect(mailbox[:delivery_options][:redis_url]).to be_present
-      expect(mailbox[:delivery_options][:redis_url]).to eq(redis_url)
-
-      expect(mailbox[:delivery_options][:sentinels]).to be_present
-      expect(mailbox[:delivery_options][:sentinels]).to eq(sentinels)
-
-      expect(mailbox[:arbitration_options][:redis_url]).to be_present
-      expect(mailbox[:arbitration_options][:redis_url]).to eq(redis_url)
-
-      expect(mailbox[:arbitration_options][:sentinels]).to be_present
-      expect(mailbox[:arbitration_options][:sentinels]).to eq(sentinels)
+      expected_mailbox = {
+        host: 'imap.gmail.com',
+        port: 993,
+        ssl: true,
+        start_tls: false,
+        email: 'gitlab-incoming@gmail.com',
+        password: '[REDACTED]',
+        name: 'inbox',
+        idle_timeout: 60
+      }
+      expected_options = {
+        redis_url: gitlab_redis_queues.url,
+        sentinels: gitlab_redis_queues.sentinels
+      }
+
+      expect(configuration[:mailboxes].length).to eq(2)
+      expect(configuration[:mailboxes]).to all(include(expected_mailbox))
+      expect(configuration[:mailboxes].map { |m| m[:delivery_options] }).to all(include(expected_options))
+      expect(configuration[:mailboxes].map { |m| m[:arbitration_options] }).to all(include(expected_options))
     end
   end
 

spec/fixtures/config/mail_room_disabled.yml

@@ -9,3 +9,14 @@ test:
     ssl: true
     start_tls: false
     mailbox: "inbox"
+
+  service_desk_email:
+    enabled: false
+    address: "gitlab-incoming+%{key}@gmail.com"
+    user: "gitlab-incoming@gmail.com"
+    password: "[REDACTED]"
+    host: "imap.gmail.com"
+    port: 993
+    ssl: true
+    start_tls: false
+    mailbox: "inbox"

spec/fixtures/config/mail_room_enabled.yml

@@ -9,3 +9,14 @@ test:
     ssl: true
     start_tls: false
     mailbox: "inbox"
+
+  service_desk_email:
+    enabled: true
+    address: "gitlab-incoming+%{key}@gmail.com"
+    user: "gitlab-incoming@gmail.com"
+    password: "[REDACTED]"
+    host: "imap.gmail.com"
+    port: 993
+    ssl: true
+    start_tls: false
+    mailbox: "inbox"

spec/lib/gitlab/mail_room/mail_room_spec.rb

@@ -4,9 +4,10 @@ require 'spec_helper'
 
 describe Gitlab::MailRoom do
   let(:default_port) { 143 }
-  let(:default_config) do
+  let(:yml_config) do
     {
-      enabled: false,
+      enabled: true,
+      address: 'address@example.com',
       port: default_port,
       ssl: false,
       start_tls: false,
@@ -16,71 +17,73 @@ describe Gitlab::MailRoom do
     }
   end
 
-  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')
+  let(:custom_config) { {} }
+  let(:incoming_email_config) { yml_config.merge(custom_config) }
+  let(:service_desk_email_config) { yml_config.merge(custom_config) }
 
-        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
+  let(:configs) do
+    {
+      incoming_email: incoming_email_config,
+      service_desk_email: service_desk_email_config
+    }
+  end
 
-    context 'with enabled only as truthy' do
-      it 'is falsey' do
-        stub_config(enabled: true, address: nil)
+  before do
+    described_class.instance_variable_set(:@enabled_configs, nil)
+  end
 
-        expect(target_proc.call).to be_falsey
-      end
+  describe '#enabled_configs' do
+    before do
+      allow(described_class).to receive(:load_yaml).and_return(configs)
     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
+    context 'when both email and address is set' do
+      it 'returns email configs' do
+        expect(described_class.enabled_configs.size).to eq(2)
       end
     end
-  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
+    context 'when the yml file cannot be found' do
       before do
-        allow(File).to receive(:exist?).and_return false
+        allow(described_class).to receive(:config_file).and_return('not_existing_file')
       end
 
-      it 'returns an empty hash' do
-        expect(described_class.config).to be_empty
+      it 'returns an empty list' do
+        expect(described_class.enabled_configs).to be_empty
       end
     end
 
-    before do
-      allow(described_class).to receive(:load_from_yaml).and_return(default_config)
+    context 'when email is disabled' do
+      let(:custom_config) { { enabled: false } }
+
+      it 'returns an empty list' do
+        expect(described_class.enabled_configs).to be_empty
+      end
     end
 
-    it 'sets up config properly' do
-      expected_result = default_config
+    context 'when email is enabled but address is not set' do
+      let(:custom_config) { { enabled: true, address: '' } }
 
-      expect(described_class.config).to match expected_result
+      it 'returns an empty list' do
+        expect(described_class.enabled_configs).to be_empty
+      end
     end
 
     context 'when a config value is missing from the yml file' do
+      let(:yml_config) { {} }
+      let(:custom_config) { { enabled: true, address: 'address@example.com' } }
+
       it 'overwrites missing values with the default' do
-        stub_config(port: nil)
+        expect(described_class.enabled_configs.first[:port]).to eq(Gitlab::MailRoom::DEFAULT_CONFIG[:port])
+      end
+    end
+
+    context 'when only incoming_email config is present' do
+      let(:configs) { { incoming_email: incoming_email_config } }
 
-        expect(described_class.config[:port]).to eq default_port
+      it 'returns only encoming_email' do
+        expect(described_class.enabled_configs.size).to eq(1)
+        expect(described_class.enabled_configs.first[:worker]).to eq('EmailReceiverWorker')
       end
     end
 
@@ -91,50 +94,31 @@ describe Gitlab::MailRoom do
         allow(Gitlab::Redis::Queues).to receive(:new).and_return(fake_redis_queues)
       end
 
-      target_proc = proc { described_class.config[:redis_url] }
+      it 'sets redis config' do
+        config = described_class.enabled_configs.first
 
-      it_behaves_like 'only truthy if both enabled and address are truthy', target_proc
+        expect(config[:redis_url]).to eq('localhost')
+        expect(config[:sentinels]).to eq('yes, them')
+      end
     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')
+        let(:custom_config) { { log_path: 'tiny_log.log' } }
 
-          new_path = Pathname.new(described_class.config[:log_path])
+        it 'expands the log path to an absolute value' do
+          new_path = Pathname.new(described_class.enabled_configs.first[: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)
+        let(:custom_config) { { log_path: '/dev/null' } }
 
-          expect(described_class.config[:log_path]).to eq new_path
+        it 'leaves the path as-is' do
+          expect(described_class.enabled_configs.first[:log_path]).to eq '/dev/null'
         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