From f50f681090fc18bcd9fc1456746f8f6c87a8fa9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Romain=20Tarti=C3=A8re?= <romain@blogreen.org>
Date: Mon, 1 Apr 2024 09:39:50 -1000
Subject: [PATCH] Add new ini_section type

In order to explicitly manage sections of an ini file regardless of
their content, we introduce a new ini_section type.  This make it
possible to remove a whole section of an ini file, regardless of its
content:

```puppet
ini_section { 'remove puppet.conf agent section':
  ensure  => abent,
  path    => '/etc/puppetlabs/puppet/puppet.conf',
  section => 'agent',
}
```

Just like the ini_setting type, ini_section can be subclassed:

```puppet
puppet_config { 'remove agent section':
  ensure  => abent,
  section => 'agent',
}
```

Fixes #524
---
 lib/puppet/provider/ini_section/ruby.rb       |  89 ++++++
 lib/puppet/type/ini_section.rb                |  52 ++++
 lib/puppet/util/ini_file.rb                   |  17 +-
 lib/puppet/util/ini_file/section.rb           |  10 +-
 .../inherit_ini_section/ini_section.rb        |  12 +
 .../lib/puppet/type/inherit_ini_section.rb    |   5 +
 .../provider/ini_section/inheritance_spec.rb  |  63 ++++
 .../puppet/provider/ini_section/ruby_spec.rb  | 290 ++++++++++++++++++
 spec/unit/puppet/type/ini_section_spec.rb     | 104 +++++++
 9 files changed, 637 insertions(+), 5 deletions(-)
 create mode 100644 lib/puppet/provider/ini_section/ruby.rb
 create mode 100644 lib/puppet/type/ini_section.rb
 create mode 100644 spec/fixtures/inherit_ini_section/lib/puppet/provider/inherit_ini_section/ini_section.rb
 create mode 100644 spec/fixtures/inherit_ini_section/lib/puppet/type/inherit_ini_section.rb
 create mode 100644 spec/unit/puppet/provider/ini_section/inheritance_spec.rb
 create mode 100644 spec/unit/puppet/provider/ini_section/ruby_spec.rb
 create mode 100644 spec/unit/puppet/type/ini_section_spec.rb

diff --git a/lib/puppet/provider/ini_section/ruby.rb b/lib/puppet/provider/ini_section/ruby.rb
new file mode 100644
index 00000000..14083f8c
--- /dev/null
+++ b/lib/puppet/provider/ini_section/ruby.rb
@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+require File.expand_path('../../util/ini_file', __dir__)
+
+Puppet::Type.type(:ini_section).provide(:ruby) do
+  def self.instances
+    desc '
+    Creates new ini_section file, a specific config file with a provider that uses
+    this as its parent and implements the method
+    self.file_path, and that will provide the value for the path to the
+    ini file.'
+    raise(Puppet::Error, 'Ini_section only support collecting instances when a file path is hard coded') unless respond_to?(:file_path)
+
+    # figure out what to do about the seperator
+    ini_file  = Puppet::Util::IniFile.new(file_path, '=')
+    resources = []
+    ini_file.section_names.each do |section_name|
+      next if section_name.empty?
+      resources.push(
+        new(
+          name: namevar(section_name),
+          ensure: :present,
+        ),
+      )
+    end
+    resources
+  end
+
+  def self.namevar(section_name)
+    section_name
+  end
+
+  def exists?
+    ini_file.section_names.include?(section)
+  end
+
+  def create
+    ini_file.set_value(section)
+    ini_file.save
+    @ini_file = nil
+  end
+
+  def destroy
+    ini_file.remove_section(section)
+    ini_file.save
+    @ini_file = nil
+  end
+
+  def file_path
+    # this method is here to support purging and sub-classing.
+    # if a user creates a type and subclasses our provider and provides a
+    # 'file_path' method, then they don't have to specify the
+    # path as a parameter for every ini_section declaration.
+    # This implementation allows us to support that while still
+    # falling back to the parameter value when necessary.
+    if self.class.respond_to?(:file_path)
+      self.class.file_path
+    else
+      resource[:path]
+    end
+  end
+
+  def section
+    # this method is here so that it can be overridden by a child provider
+    resource[:section]
+  end
+
+  def section_prefix
+    if resource.class.validattr?(:section_prefix)
+      resource[:section_prefix] || '['
+    else
+      '['
+    end
+  end
+
+  def section_suffix
+    if resource.class.validattr?(:section_suffix)
+      resource[:section_suffix] || ']'
+    else
+      ']'
+    end
+  end
+
+  private
+
+  def ini_file
+    @ini_file ||= Puppet::Util::IniFile.new(file_path, '=', section_prefix, section_suffix, ' ', 2)
+  end
+end
diff --git a/lib/puppet/type/ini_section.rb b/lib/puppet/type/ini_section.rb
new file mode 100644
index 00000000..9f562aae
--- /dev/null
+++ b/lib/puppet/type/ini_section.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+Puppet::Type.newtype(:ini_section) do
+  desc 'ini_section is used to manage a single section in an INI file'
+  ensurable do
+    desc 'Ensurable method handles modeling creation. It creates an ensure property'
+    newvalue(:present) do
+      provider.create
+    end
+    newvalue(:absent) do
+      provider.destroy
+    end
+    def insync?(current)
+      if @resource[:refreshonly]
+        true
+      else
+        current == should
+      end
+    end
+    defaultto :present
+  end
+
+  newparam(:name, namevar: true) do
+    desc 'An arbitrary name used as the identity of the resource.'
+  end
+
+  newparam(:section) do
+    desc 'The name of the section in the ini file which should be managed.'
+    defaultto('')
+  end
+
+  newparam(:path) do
+    desc 'The ini file Puppet will ensure contains the specified section.'
+    validate do |value|
+      raise(Puppet::Error, _("File paths must be fully qualified, not '%{value}'") % { value: value }) unless Puppet::Util.absolute_path?(value)
+    end
+  end
+
+  newparam(:section_prefix) do
+    desc 'The prefix to the section name\'s header.'
+    defaultto('[')
+  end
+
+  newparam(:section_suffix) do
+    desc 'The suffix to the section name\'s header.'
+    defaultto(']')
+  end
+
+  autorequire(:file) do
+    Pathname.new(self[:path]).parent.to_s
+  end
+end
diff --git a/lib/puppet/util/ini_file.rb b/lib/puppet/util/ini_file.rb
index 478cc208..c337e65c 100644
--- a/lib/puppet/util/ini_file.rb
+++ b/lib/puppet/util/ini_file.rb
@@ -124,6 +124,19 @@ def remove_setting(section_name, setting)
       decrement_section_line_numbers(section_index + 1)
     end
 
+    def remove_section(section_name)
+      section = @sections_hash[section_name]
+      return unless section
+
+      lines.replace(lines[0..(section.start_line - 1)] + lines[(section.end_line + 1)..-1])
+
+      section_index = @section_names.index(section.name)
+      decrement_section_line_numbers(section_index + 1, amount: section.length)
+
+      @section_names.delete_at(section_index)
+      @sections_hash.delete(section.name)
+    end
+
     def save
       global_empty = @sections_hash[''].empty? && @sections_hash[''].additional_settings.empty?
       File.open(@path, 'w') do |fh|
@@ -300,10 +313,10 @@ def insert_inline_setting_line(result, section, complete_setting)
     # Utility method; given a section index (index into the @section_names
     # array), decrement the start/end line numbers for that section and all
     # all of the other sections that appear *after* the specified section.
-    def decrement_section_line_numbers(section_index)
+    def decrement_section_line_numbers(section_index, amount: 1)
       @section_names[section_index..(@section_names.length - 1)].each do |name|
         section = @sections_hash[name]
-        section.decrement_line_nums
+        section.decrement_line_nums(amount)
       end
     end
 
diff --git a/lib/puppet/util/ini_file/section.rb b/lib/puppet/util/ini_file/section.rb
index 46e01477..8dc73fdf 100644
--- a/lib/puppet/util/ini_file/section.rb
+++ b/lib/puppet/util/ini_file/section.rb
@@ -53,6 +53,10 @@ def empty?
       global? ? new_section? : start_line == end_line
     end
 
+    def length
+      end_line - start_line + 1
+    end
+
     def update_existing_setting(setting_name, value)
       @existing_settings[setting_name] = value
     end
@@ -79,9 +83,9 @@ def set_additional_setting(setting_name, value)
     # Decrement the start and end line numbers for the section (if they are
     # defined); this is intended to be called when a setting is removed
     # from a section that comes before this section in the ini file.
-    def decrement_line_nums
-      @start_line -= 1 if @start_line
-      @end_line -= 1 if @end_line
+    def decrement_line_nums(amount)
+      @start_line -= amount if @start_line
+      @end_line -= amount if @end_line
     end
 
     # Increment the start and end line numbers for the section (if they are
diff --git a/spec/fixtures/inherit_ini_section/lib/puppet/provider/inherit_ini_section/ini_section.rb b/spec/fixtures/inherit_ini_section/lib/puppet/provider/inherit_ini_section/ini_section.rb
new file mode 100644
index 00000000..a4b9b4c9
--- /dev/null
+++ b/spec/fixtures/inherit_ini_section/lib/puppet/provider/inherit_ini_section/ini_section.rb
@@ -0,0 +1,12 @@
+Puppet::Type.type(:inherit_ini_section).provide(
+  :ini_section,
+  parent: Puppet::Type.type(:ini_section).provider(:ruby),
+) do
+  def self.namevar(section)
+    section
+  end
+
+  def self.file_path
+    File.expand_path(File.dirname(__FILE__) + '/../../../../../../tmp/inherit_inifile.cfg')
+  end
+end
diff --git a/spec/fixtures/inherit_ini_section/lib/puppet/type/inherit_ini_section.rb b/spec/fixtures/inherit_ini_section/lib/puppet/type/inherit_ini_section.rb
new file mode 100644
index 00000000..b15a76cb
--- /dev/null
+++ b/spec/fixtures/inherit_ini_section/lib/puppet/type/inherit_ini_section.rb
@@ -0,0 +1,5 @@
+Puppet::Type.newtype(:inherit_ini_section) do
+  ensurable
+  newparam(:section, namevar: true)
+  newproperty(:value)
+end
diff --git a/spec/unit/puppet/provider/ini_section/inheritance_spec.rb b/spec/unit/puppet/provider/ini_section/inheritance_spec.rb
new file mode 100644
index 00000000..b013c788
--- /dev/null
+++ b/spec/unit/puppet/provider/ini_section/inheritance_spec.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# This is a reduced version of ruby_spec.rb just to ensure we can subclass as
+# documented
+$LOAD_PATH << './spec/fixtures/inherit_ini_section/lib'
+
+describe Puppet::Type.type(:inherit_ini_section).provider(:ini_section) do
+  include PuppetlabsSpec::Files
+
+  let(:tmpfile) { tmpfilename('inherit_ini_section_test') }
+
+  def validate_file(expected_content, tmpfile)
+    expect(File.read(tmpfile)).to eq(expected_content)
+  end
+
+  before :each do
+    File.write(tmpfile, orig_content)
+  end
+
+  context 'when calling instances' do
+    let(:orig_content) { '' }
+
+    it 'parses nothing when the file is empty' do
+      allow(described_class).to receive(:file_path).and_return(tmpfile)
+      expect(described_class.instances).to eq([])
+    end
+
+    context 'when the file has contents' do
+      let(:orig_content) do
+        <<-INIFILE
+          [red]
+          # A comment
+          red = blue
+          [green]
+          green = purple
+        INIFILE
+      end
+
+      it 'parses the results' do
+        allow(described_class).to receive(:file_path).and_return(tmpfile)
+        instances = described_class.instances
+        expect(instances.size).to eq(2)
+        # inherited version of namevar flattens the names
+        names = instances.map do |instance| instance.instance_variable_get(:@property_hash)[:name] end # rubocop:disable Style/BlockDelimiters
+        expect(names.sort).to eq(['green', 'red'])
+      end
+    end
+  end
+
+  context 'when ensuring that a setting is present' do
+    let(:orig_content) { '' }
+
+    it 'adds a value to the file' do
+      allow(described_class).to receive(:file_path).and_return(tmpfile)
+      resource = Puppet::Type::Inherit_ini_section.new(section: 'set_this')
+      provider = described_class.new(resource)
+      provider.create
+      expect(validate_file("[set_this]\n", tmpfile)).to be_truthy
+    end
+  end
+end
diff --git a/spec/unit/puppet/provider/ini_section/ruby_spec.rb b/spec/unit/puppet/provider/ini_section/ruby_spec.rb
new file mode 100644
index 00000000..872ada08
--- /dev/null
+++ b/spec/unit/puppet/provider/ini_section/ruby_spec.rb
@@ -0,0 +1,290 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'puppet'
+
+provider_class = Puppet::Type.type(:ini_section).provider(:ruby)
+describe provider_class do
+  include PuppetlabsSpec::Files
+
+  let(:tmpfile) { tmpfilename('ini_section_test') }
+  let(:emptyfile) { tmpfilename('ini_section_test_empty') }
+
+  let(:common_params) do
+    {
+      title: 'ini_section_ensure_present_test',
+      path: tmpfile,
+    }
+  end
+
+  def validate_file(expected_content, tmpfile)
+    expect(File.read(tmpfile)).to eq(expected_content)
+  end
+
+  before :each do
+    File.write(tmpfile, orig_content)
+    File.write(emptyfile, '')
+  end
+
+  context 'when calling instances' do
+    let :orig_content do
+      ''
+    end
+
+    it 'fails when file path is not set' do
+      expect {
+        provider_class.instances
+      }.to raise_error(Puppet::Error, 'Ini_section only support collecting instances when a file path is hard coded')
+    end
+
+    context 'when file path is set by a child class' do
+      child_one = Class.new(provider_class) do
+        def self.file_path
+          emptyfile
+        end
+      end
+      it 'returns [] when file is empty' do
+        allow(child_one).to receive(:file_path).and_return(emptyfile)
+        expect(child_one.instances).to eq([])
+      end
+
+      child_two = Class.new(provider_class) do
+        def self.file_path
+          '/some/file/path'
+        end
+      end
+      it 'overrides the provider instances file_path' do
+        resource = Puppet::Type::Ini_section.new(common_params)
+        provider = child_two.new(resource)
+        expect(provider.file_path).to eq('/some/file/path')
+      end
+    end
+
+    context 'when file has contents' do
+      let(:orig_content) do
+        <<-INIFILE
+          # This is a comment
+          [section1]
+          ; This is also a comment
+          foo=foovalue
+
+          bar = barvalue
+          main = true
+          [section2]
+
+          foo= foovalue2
+          baz=bazvalue
+          url = http://192.168.1.1:8080
+          [section:sub]
+          subby=bar
+              #another comment
+           ; yet another comment
+        INIFILE
+      end
+
+      it 'is able to parse the results' do
+        child_three = Class.new(provider_class) do
+          def self.file_path
+            '/some/file/path'
+          end
+        end
+        expect(child_three).to receive(:file_path).twice.and_return(tmpfile)
+        expect(child_three.instances.size).to eq(3)
+        expected_array = [
+          { name: 'section1' },
+          { name: 'section2' },
+          { name: 'section:sub' },
+        ]
+        real_array = []
+        ensure_array = []
+        child_three.instances.each do |x|
+          prop_hash    = x.instance_variable_get(:@property_hash)
+          ensure_value = prop_hash.delete(:ensure)
+          ensure_array.push(ensure_value)
+          real_array.push(prop_hash)
+        end
+        expect(ensure_array.uniq).to eq([:present])
+        expect((real_array - expected_array) && (expected_array - real_array)).to eq([])
+      end
+    end
+  end
+
+  context 'when ensuring that a section is present' do
+    let(:orig_content) do
+      <<~INIFILE
+        # This is a comment
+        [section1]
+        ; This is also a comment
+        foo=foovalue
+
+        bar = barvalue
+        main = true
+        [section2]
+
+        foo= foovalue2
+        baz=bazvalue
+        url = http://192.168.1.1:8080
+        [section:sub]
+        subby=bar
+            #another comment
+         ; yet another comment
+
+        -nonstandard-
+          shoes = purple
+      INIFILE
+    end
+
+    expected_content_one = <<~INIFILE
+      # This is a comment
+      [section1]
+      ; This is also a comment
+      foo=foovalue
+
+      bar = barvalue
+      main = true
+      [section2]
+
+      foo= foovalue2
+      baz=bazvalue
+      url = http://192.168.1.1:8080
+      [section:sub]
+      subby=bar
+          #another comment
+       ; yet another comment
+
+      -nonstandard-
+        shoes = purple
+
+      [yippee]
+    INIFILE
+    it 'adds a missing section' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'yippee'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be false
+      provider.create
+      validate_file(expected_content_one, tmpfile)
+    end
+
+    expected_content_two = <<~INIFILE
+      # This is a comment
+      [section1]
+      ; This is also a comment
+      foo=foovalue
+
+      bar = barvalue
+      main = true
+      [section2]
+
+      foo= foovalue2
+      baz=bazvalue
+      url = http://192.168.1.1:8080
+      [section:sub]
+      subby=bar
+          #another comment
+       ; yet another comment
+
+      -nonstandard-
+        shoes = purple
+    INIFILE
+    it 'does nothing when the section is already there' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'section2'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be true
+      provider.create
+      validate_file(expected_content_two, tmpfile)
+    end
+
+    it 'does nothing when the section with custom prefix/suffix is already there' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'nonstandard', section_prefix: '-', section_suffix: '-'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be true
+      provider.create
+      validate_file(expected_content_two, tmpfile)
+    end
+  end
+
+  context 'when ensuring that a section is absent' do
+    let(:orig_content) do
+      <<~INIFILE
+        # This is a comment
+        [section1]
+        ; This is also a comment
+        foo=foovalue
+
+        bar = barvalue
+        main = true
+        [section2]
+
+        foo= foovalue2
+        baz=bazvalue
+        url = http://192.168.1.1:8080
+        [section:sub]
+        subby=bar
+            #another comment
+         ; yet another comment
+
+        -nonstandard-
+          shoes = purple
+      INIFILE
+    end
+
+    expected_content_one = <<~INIFILE
+      # This is a comment
+      [section1]
+      ; This is also a comment
+      foo=foovalue
+
+      bar = barvalue
+      main = true
+      [section:sub]
+      subby=bar
+          #another comment
+       ; yet another comment
+
+      -nonstandard-
+        shoes = purple
+    INIFILE
+    it 'removes an existing section' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'section2'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be true
+      provider.destroy
+      validate_file(expected_content_one, tmpfile)
+    end
+
+    expected_content_two = <<~INIFILE
+      # This is a comment
+      [section1]
+      ; This is also a comment
+      foo=foovalue
+
+      bar = barvalue
+      main = true
+      [section2]
+
+      foo= foovalue2
+      baz=bazvalue
+      url = http://192.168.1.1:8080
+      [section:sub]
+      subby=bar
+          #another comment
+       ; yet another comment
+
+    INIFILE
+    it 'removes an existing section with custom prefix/suffix' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'nonstandard', section_prefix: '-', section_suffix: '-'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be true
+      provider.destroy
+      validate_file(expected_content_two, tmpfile)
+    end
+
+    it 'does nothing when the section does not exist' do
+      resource = Puppet::Type::Ini_section.new(common_params.merge(section: 'yippee'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be false
+      provider.destroy
+      validate_file(orig_content, tmpfile)
+    end
+  end
+end
diff --git a/spec/unit/puppet/type/ini_section_spec.rb b/spec/unit/puppet/type/ini_section_spec.rb
new file mode 100644
index 00000000..7a2f1b5c
--- /dev/null
+++ b/spec/unit/puppet/type/ini_section_spec.rb
@@ -0,0 +1,104 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+ini_section = Puppet::Type.type(:ini_section)
+
+describe ini_section do
+  describe 'path validation' do
+    subject(:ini_section_path) { described_class.new(name: 'foo', path: path) }
+
+    context 'when on posix platforms' do
+      before(:each) do
+        Puppet.features.stub(:posix?) { true }
+        Puppet.features.stub(:microsoft_windows?) { false }
+        Puppet::Util::Platform.stub(:windows?) { false }
+      end
+
+      context 'with an absolute path' do
+        let(:path) { '/absolute/path' }
+
+        it { expect { ini_section_path }.not_to raise_exception }
+      end
+
+      context 'with a relative path' do
+        let(:path) { 'relative/path' }
+
+        it { expect { ini_section_path }.to raise_exception(Puppet::ResourceError) }
+      end
+    end
+
+    context 'when on windows platforms' do
+      before(:each) do
+        Puppet.features.stub(:posix?) { false }
+        Puppet.features.stub(:microsoft_windows?) { true }
+        Puppet::Util::Platform.stub(:windows?) { true }
+      end
+
+      context 'with an absolute path with front slashes' do
+        let(:path) { 'c:/absolute/path' }
+
+        it { expect { ini_section_path }.not_to raise_exception }
+      end
+
+      context 'with an absolute path with backslashes' do
+        let(:path) { 'c:\absolute\path' }
+
+        it { expect { ini_section_path }.not_to raise_exception }
+      end
+
+      context 'with an absolute path with mixed slashes' do
+        let(:path) { 'c:/absolute\path' }
+
+        it { expect { ini_section_path }.not_to raise_exception }
+      end
+
+      context 'with a relative path with front slashes' do
+        let(:path) { 'relative/path' }
+
+        it { expect { ini_section_path }.to raise_exception(Puppet::ResourceError) }
+      end
+
+      context 'with a relative path with back slashes' do
+        let(:path) { 'relative\path' }
+
+        it { expect { ini_section_path }.to raise_exception(Puppet::ResourceError) }
+      end
+    end
+  end
+
+  describe 'when parent of :path is in the catalog' do
+    ['posix', 'windows'].each do |platform|
+      context "when on #{platform} platforms" do
+        before(:each) do
+          Puppet.features.stub(:posix?) { platform == 'posix' }
+          Puppet.features.stub(:microsoft_windows?) { platform == 'windows' }
+          Puppet::Util::Platform.stub(:windows?) { platform == 'windows' }
+        end
+
+        let(:file_path) { (platform == 'posix') ? '/tmp' : 'c:/tmp' }
+        let(:file_resource) { Puppet::Type.type(:file).new(name: file_path) }
+        let(:ini_section_resource) { described_class.new(name: 'foo', path: "#{file_path}/foo.ini") }
+        let(:auto_req) do
+          catalog = Puppet::Resource::Catalog.new
+          catalog.add_resource(file_resource)
+          catalog.add_resource(ini_section_resource)
+
+          ini_section_resource.autorequire
+        end
+
+        it 'creates relationship' do
+          expect(auto_req.size).to be 1
+        end
+
+        it 'links to ini_section resource' do
+          expect(auto_req[0].target).to eq(ini_section_resource)
+        end
+
+        it 'autorequires parent directory' do
+          expect(auto_req[0].source).to eq(file_resource)
+        end
+      end
+    end
+  end
+end