(gh-26) Update rather than overwrite puppet.conf

In particular, openvox-server lays down some settings in puppet.conf at
package installation time, so it would be best if this task didn't throw
out an existing puppet.conf. Using puppet-config set is safe (it's what
the openvox-server package uses); it does not trigger any of puppet's
auto certificate generation code the way an agent run does.

Author: jpartlow

spec/tasks/configure_spec.rb

@@ -5,17 +5,45 @@
 
 # rubocop:disable RSpec/MessageSpies
 # rubocop:disable RSpec/StubbedMock
+# rubocop:disable RSpec/MultipleMemoizedHelpers
 describe 'openvox_bootstrap::configure' do
   let(:tmpdir) { Dir.mktmpdir('openvox_bootstrap-configure-spec') }
   let(:task) { OpenvoxBootstrap::Configure.new }
+  let(:puppet_config_set_calls) { [] }
 
   around do |example|
     example.run
   ensure
     FileUtils.remove_entry_secure(tmpdir)
   end
 
-  describe '#write_puppet_conf' do
+  before do
+    allow(task).to receive(:puppet_config_set) do |section, key, value|
+      puppet_config_set_calls << [section, key, value]
+      if key == 'oops'
+        ['error output', instance_double(Process::Status, success?: false)]
+      else
+        ['', instance_double(Process::Status, success?: true)]
+      end
+    end
+  end
+
+  describe '#puppet_config_set' do
+    it 'calls puppet config set with the correct arguments' do
+      expect(Open3).to receive(:capture2e).with(
+        '/opt/puppetlabs/bin/puppet',
+        'config',
+        'set',
+        '--section', 'main',
+        'server',
+        'puppet.spec'
+      )
+      t = OpenvoxBootstrap::Configure.new
+      t.puppet_config_set('main', 'server', 'puppet.spec')
+    end
+  end
+
+  describe '#update_puppet_conf' do
     let(:puppet_conf) do
       {
         'main' => {
@@ -28,34 +56,49 @@
       }
     end
     let(:puppet_conf_path) { File.join(tmpdir, 'puppet.conf') }
-    let(:puppet_conf_contents) do
-      <<~CONF
-        [main]
-        server = puppet.spec
-        certname = agent.spec
-
-        [agent]
-        environment = test
-      CONF
-    end
 
-    it 'writes a puppet.conf ini' do
-      expect(task.write_puppet_conf(puppet_conf, tmpdir)).to(
+    it 'call puppet config set' do
+      expect(task.update_puppet_conf(puppet_conf, tmpdir)).to(
         eq(
           {
             puppet_conf: {
               path: puppet_conf_path,
-              contents: puppet_conf_contents
+              contents: '',
+              successful: true,
             }
           }
         )
       )
-      expect(File.read(puppet_conf_path)).to eq(puppet_conf_contents)
+      expect(puppet_config_set_calls).to eq(
+        [
+          ['main', 'server', 'puppet.spec'],
+          ['main', 'certname', 'agent.spec'],
+          ['agent', 'environment', 'test'],
+        ]
+      )
     end
 
     it 'does nothing if given an empty config' do
-      expect(task.write_puppet_conf(nil)).to eq({})
-      expect(task.write_puppet_conf({})).to eq({})
+      expect(task.update_puppet_conf(nil)).to eq({})
+      expect(task.update_puppet_conf({})).to eq({})
+    end
+
+    it 'records error output if puppet config set fails' do
+      puppet_conf['main']['oops'] = 'fail'
+      expect(task.update_puppet_conf(puppet_conf, tmpdir)).to(
+        eq(
+          {
+            puppet_conf: {
+              path: puppet_conf_path,
+              contents: '',
+              successful: false,
+              errors: {
+                '--section=main oops=fail' => 'error output',
+              },
+            }
+          }
+        )
+      )
     end
   end
 
@@ -115,6 +158,7 @@ def check_returned_id(uid_or_gid)
             csr_attributes: {
               path: csr_attributes_path,
               contents: csr_attributes_contents,
+              successful: true,
             }
           }
         )
@@ -181,6 +225,20 @@ def status(code = 0)
       )
     end
 
+    it 'returns a result has if all steps are successful' do
+      expect(task).to receive(:update_puppet_conf).and_return({ puppet_conf: { successful: true } })
+      expect(task).to receive(:write_csr_attributes).and_return({ csr_attributes: { successful: true } })
+      expect(task).to receive(:manage_puppet_service).and_return({ puppet_service: { successful: true } })
+
+      expect(task.task).to eq(
+        {
+          csr_attributes: { successful: true },
+          puppet_conf: { successful: true },
+          puppet_service: { successful: true },
+        }
+      )
+    end
+
     it 'prints results and exits 1 if puppet service fails' do
       expect(task).to(
         receive(:manage_puppet_service).
@@ -204,14 +262,52 @@ def status(code = 0)
               }
             }
 
-            Failed managing the puppet service:
+            Failed managing puppet_service:
 
             apply failed
           EOM
         ).and(output('').to_stderr)
       )
     end
+
+    it 'prints results and exits 1 if any step fails' do
+      expect(task).to receive(:manage_puppet_service).and_return({ puppet_service: { successful: true } })
+      expect(task).to(
+        receive(:update_puppet_conf).
+        and_return(
+          {
+            puppet_conf: {
+              successful: false,
+              errors: { '--section=main server=puppet.spec' => 'error output' }
+            }
+          }
+        )
+      )
+
+      expect { task.task }.to(
+        raise_error(SystemExit).and(
+          output(<<~EOM).to_stdout
+            {
+              "puppet_conf": {
+                "successful": false,
+                "errors": {
+                  "--section=main server=puppet.spec": "error output"
+                }
+              },
+              "puppet_service": {
+                "successful": true
+              }
+            }
+
+            Failed managing puppet_conf:
+
+            {"--section=main server=puppet.spec"=>"error output"}
+          EOM
+        ).and(output('').to_stderr)
+      )
+    end
   end
 end
 # rubocop:enable RSpec/MessageSpies
 # rubocop:enable RSpec/StubbedMock
+# rubocop:enable RSpec/MultipleMemoizedHelpers

tasks/configure.json

@@ -6,11 +6,11 @@
   ],
   "parameters": {
     "puppet_conf": {
-      "description": "Hash of puppet configuration settings to write to the puppet.conf ini file. NOTE: This will completely overwrite any existing settings in the file; there is no merging behavior in this task.",
+      "description": "Hash of puppet configuration settings to add to the puppet.conf ini file. These will be merged into the existing puppet.conf, if any.",
       "type": "Optional[Openvox_bootstrap::Ini_file]"
     },
     "csr_attributes": {
-      "description": "Hash of CSR attributes (custom_attributes and extension_requests) to write to the csr_attributes.yaml file. NOTE: This will completely overwrite any existing settings in the file; there is no merging behavior in this task.",
+      "description": "Hash of CSR attributes (custom_attributes and extension_requests) to write to the csr_attributes.yaml file. NOTE: This will completely overwrite any pre-existing csr_attributes.yaml.",
       "type": "Optional[Openvox_bootstrap::Csr_attributes]"
     },
     "puppet_service_running": {

tasks/configure.rb

@@ -6,6 +6,7 @@
 require 'open3'
 require 'yaml'
 
+# rubocop:disable Style/NegatedIf
 module OpenvoxBootstrap
   class Configure < Task
     def puppet_uid
@@ -20,33 +21,59 @@ def puppet_gid
       nil
     end
 
-    # Overwrite puppet.conf with the values in the puppet_conf hash.
+    def puppet_config_set(section, key, value)
+      command = [
+        '/opt/puppetlabs/bin/puppet',
+        'config',
+        'set',
+        '--section', section,
+        key,
+        value,
+      ]
+      Open3.capture2e(*command)
+    end
+
+    # Add the given settings to the puppet.conf file using
+    # puppet-config.
     #
-    # Does nothing if given an empty or nil puppet_conf.
+    # Does nothing if given an empty or nil settings hash.
     #
-    # @param puppet_conf [Hash<String,Hash<String,String>>] A hash of
-    #   sections and settings to write to the puppet.conf file.
+    # @param settings [Hash<String,Hash<String,String>>]
+    #   A hash of sections and settings to add to the
+    #   puppet.conf file.
     # @return [Hash]
-    def write_puppet_conf(puppet_conf, etc_puppet_path = '/etc/puppetlabs/puppet')
-      return {} if puppet_conf.nil? || puppet_conf.empty?
+    def update_puppet_conf(settings, etc_puppet_path = '/etc/puppetlabs/puppet')
+      return {} if settings.nil? || settings.empty?
 
       conf_path = File.join(etc_puppet_path, 'puppet.conf')
-      sections = puppet_conf.map do |section, settings|
-        "[#{section}]\n" +
-          settings.map { |key, value| "#{key} = #{value}" }.join("\n")
+      success = true
+      errors = {}
+      settings.each do |section, section_settings|
+        section_settings.each do |key, value|
+          output, status = puppet_config_set(section, key, value)
+          success &&= status.success?
+          if !status.success?
+            err_key = "--section=#{section} #{key}=#{value}"
+            errors[err_key] = output
+          end
+        end
       end
-      puppet_conf_contents = "#{sections.join("\n\n")}\n"
 
-      File.open(conf_path, 'w', perm: 0o644) do |f|
-        f.write(puppet_conf_contents)
-      end
+      puppet_conf_contents = if File.exist?(conf_path)
+                               File.read(conf_path)
+                             else
+                               ''
+                             end
 
-      {
+      result = {
         puppet_conf: {
           path: conf_path,
           contents: puppet_conf_contents,
+          successful: success,
         }
       }
+      result[:puppet_conf][:errors] = errors if !success
+      result
     end
 
     # Overwrite the csr_attributes.yaml file with the given
@@ -78,6 +105,7 @@ def write_csr_attributes(csr_attributes, etc_puppet_path = '/etc/puppetlabs/pupp
         csr_attributes: {
           path: csr_attributes_path,
           contents: csr_attributes_contents,
+          successful: true,
         }
       }
     end
@@ -118,25 +146,31 @@ def task(
       puppet_service_enabled: true,
       **_kwargs
     )
-      puppet_conf_result = write_puppet_conf(puppet_conf)
-      csr_result = write_csr_attributes(csr_attributes)
-      puppet_service_result = manage_puppet_service(puppet_service_running, puppet_service_enabled)
+      results = {}
+      results.merge!(update_puppet_conf(puppet_conf))
+      results.merge!(write_csr_attributes(csr_attributes))
+      results.merge!(
+        manage_puppet_service(puppet_service_running, puppet_service_enabled)
+      )
 
-      results = puppet_conf_result.merge(
-        csr_result
-      ).merge(puppet_service_result)
+      success = results.all? { |_, details| details[:successful] }
 
-      success = results[:puppet_service][:successful]
       if success
         results
       else
         puts JSON.pretty_generate(results)
-        puts "\nFailed managing the puppet service:\n\n"
-        puts results[:puppet_service][:output]
+        results.each do |config, details|
+          next if details[:successful]
+
+          puts "\nFailed managing #{config}:\n\n"
+          puts details[:output] if details.key?(:output)
+          pp details[:errors] if details.key?(:errors)
+        end
         exit 1
       end
     end
   end
 end
+# rubocop:enable Style/NegatedIf
 
 OpenvoxBootstrap::Configure.run if __FILE__ == $PROGRAM_NAME