Merge pull request #306 from airbnb/rushy--haproxy-validation-config

Ensure that haproxy check parameters are set together

Author: panchr

lib/synapse/config_generator/haproxy.rb

@@ -816,19 +816,24 @@ def initialize(opts)
       @opts['do_socket'] = true unless @opts.key?('do_socket')
       @opts['do_checks'] = false unless @opts.key?('do_checks')
       @opts['do_reloads'] = true unless @opts.key?('do_reloads')
-      req_pairs = {
-        'do_writes' => 'config_file_path',
-        'do_socket' => 'socket_file_path',
-        'do_checks' => 'check_command',
-        'do_reloads' => 'reload_command'
+      req_groups = {
+        'do_writes' => ['config_file_path'],
+        'do_socket' => ['socket_file_path'],
+        'do_checks' => ['check_command', 'candidate_config_file_path'],
+        'do_reloads' => ['reload_command'],
       }
 
-      req_pairs.each do |cond, req|
-        if @opts[cond]
-          raise ArgumentError, "the `#{req}` option is required when `#{cond}` is true" unless @opts[req]
+      req_groups.each do |cond, reqs|
+        reqs.each do |req_config|
+          if @opts[cond]
+            raise ArgumentError, "the `#{req_config}` option is required when `#{cond}` is true" unless @opts[req_config]
+          end
         end
       end
 
+      # Default candidate_config_file_path so that write_config can do atomic writes.
+      @opts['candidate_config_file_path'] = "#{@opts['config_file_path']}.tmp" unless @opts.key?('candidate_config_file_path')
+
       # socket_file_path can be a string or a list
       # lets make a new option which is always a list (plural)
       @opts['socket_file_paths'] = [@opts['socket_file_path']].flatten

spec/lib/synapse/haproxy_spec.rb

@@ -272,7 +272,47 @@ class MockWatcher; end;
         expect{Synapse::ConfigGenerator::Haproxy.new(conf)}.
           to raise_error(ArgumentError, "the `#{value}` option is required when `#{key}` is true")
       end
+    end
 
+    context 'when do_checks is true' do
+      let(:invalid_conf) {
+        {
+          'global' => [],
+          'defaults' => [],
+          'do_reloads' => false,
+          'do_socket' => false,
+          'do_writes' => false,
+          'do_checks' => true,
+        }
+      }
+      let(:conf_additions) {
+        {
+          'check_command' => 'haproxy -c -f /etc/haproxy/haproxy-candidate.cfg',
+          'candidate_config_file_path' => '/etc/haproxy/haproxy-candidate.cfg',
+        }
+      }
+
+      context 'when check parameters are not set together' do
+        it 'raises an error' do
+          conf_additions.each do |key, value|
+            conf = invalid_conf.clone
+            conf[key] = value
+            expect{Synapse::ConfigGenerator::Haproxy.new(conf)}.
+              to raise_error(ArgumentError)
+          end
+        end
+      end
+
+      context 'when check parameters are set together' do
+        it 'does not raise an error' do
+          conf = invalid_conf.clone
+          conf_additions.each do |key, value|
+            conf[key] = value
+          end
+
+          expect{Synapse::ConfigGenerator::Haproxy.new(conf)}.not_to raise_error
+        end
+      end
     end
 
     it 'properly defaults do_writes, do_socket, do_checks, do_reloads, use_nerve_weights' do
@@ -793,6 +833,20 @@ class MockWatcher; end;
       allow(File).to receive(:read).with('candidate_config_file').and_return('candidate-haproxy-config')
     }
 
+    context 'when candidate_config_file_path is not set' do
+      before {
+        config['haproxy'].delete('candidate_config_file_path')
+        config['haproxy']['do_checks'] = false
+      }
+
+      it 'still succeeds' do
+        allow(FileUtils).to receive(:mv)
+
+        expect(File).to receive(:write).with('config_file.tmp', 'new-config')
+        expect{subject.write_config('new-config')}.not_to raise_error
+      end
+    end
+
     context 'when config changes' do
       it 'writes candidate config' do
         allow(FileUtils).to receive(:mv)
@@ -883,6 +937,7 @@ class MockWatcher; end;
 
       config['haproxy']['do_checks'] = true
       config['haproxy']['check_command'] = 'haproxy_check_mock'
+      config['haproxy']['candidate_config_file_path'] = 'candidate-haproxy-config-file'
     }
 
     it 'calls the supplied command' do