diff --git a/config/mail_room.yml b/config/mail_room.yml index 75024c2b2e18f1172c9290900aa550bbcca555b9..da37ef60587a0a782d81a7f88b4fd19374970d43 100644 --- a/config/mail_room.yml +++ b/config/mail_room.yml @@ -1,9 +1,7 @@ :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| %> diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 8974b646cd9d9dbf9499589652374a2010a9cb01..1e1ed52650be20d632ac031acf3fc25cca1925b1 100644 --- a/config/sidekiq_queues.yml +++ b/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 diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 2decfadc50451a2a3ecad281912344933dd17b59..45115d1e2dc1eee07fa551f8a440cb4613dea07e 100644 --- a/ee/app/workers/all_queues.yml +++ b/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 diff --git a/ee/app/workers/service_desk_email_receiver_worker.rb b/ee/app/workers/service_desk_email_receiver_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..1277507c78558c13ac3aca42d237a5d0e1886c0d --- /dev/null +++ b/ee/app/workers/service_desk_email_receiver_worker.rb @@ -0,0 +1,21 @@ +# 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 diff --git a/ee/spec/workers/service_desk_email_receiver_worker_spec.rb b/ee/spec/workers/service_desk_email_receiver_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..05fca2d5497b3d91e448930a1985d6225dd9e68d --- /dev/null +++ b/ee/spec/workers/service_desk_email_receiver_worker_spec.rb @@ -0,0 +1,48 @@ +# 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 diff --git a/lib/gitlab/mail_room.rb b/lib/gitlab/mail_room.rb index f7699ef1718ce65d2f84ad19b4719314c27ae00e..bd69843adf1b2813b6cd78493f3c4bc8954e96c0 100644 --- a/lib/gitlab/mail_room.rb +++ b/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 MailRoom 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 @@ 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] + def load_yaml + @yaml ||= YAML.load_file(config_file)[rails_env].deep_symbolize_keys end end end diff --git a/spec/config/mail_room_spec.rb b/spec/config/mail_room_spec.rb index 94b29b89f240aa49f3f08ba6c1aa897d5e34fef5..fcef4e7a9b04a10478210a8245ed13a05ae69bcf 100644 --- a/spec/config/mail_room_spec.rb +++ b/spec/config/mail_room_spec.rb @@ -39,39 +39,31 @@ 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 diff --git a/spec/fixtures/config/mail_room_disabled.yml b/spec/fixtures/config/mail_room_disabled.yml index 97f8cff051fac08a96f745f3572a8ba09aefad25..538f2a35f81dade5a618d7a331183ba39af3faf4 100644 --- a/spec/fixtures/config/mail_room_disabled.yml +++ b/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" diff --git a/spec/fixtures/config/mail_room_enabled.yml b/spec/fixtures/config/mail_room_enabled.yml index 9c94649244d7ee8072489c406863d3a990692305..e1f4c2f44de44d2182058fcabd1f81475fabcda3 100644 --- a/spec/fixtures/config/mail_room_enabled.yml +++ b/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" diff --git a/spec/lib/gitlab/mail_room/mail_room_spec.rb b/spec/lib/gitlab/mail_room/mail_room_spec.rb index 43218fc6e0da41cc9ea03d7ba2a1e10ed39ff96e..5d41ee0626365b1d16cb758da83c782a1b41dd7d 100644 --- a/spec/lib/gitlab/mail_room/mail_room_spec.rb +++ b/spec/lib/gitlab/mail_room/mail_room_spec.rb @@ -4,9 +4,10 @@ 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 @@ } 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 @@ 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