Add new properties to firewalld_ipset type and improve logging of changes

lib/puppet/provider/firewalld.rb

@@ -34,7 +34,7 @@ def check_running_state
   def self.check_running_state
     begin
       self.debug("Executing --state command - current value #{@state}")
-      ret = execute_firewall_cmd(['--state'], nil, false, false)
+      ret = execute_firewall_cmd(['--state'], nil, false, false, check_online=false)
       Puppet::Provider::Firewalld.runstate = ret.exitstatus == 0 ? true : false
 
     rescue Puppet::MissingCommand => e
@@ -52,7 +52,13 @@ def self.check_running_state
   end
 
   # v3.0.0
-  def self.execute_firewall_cmd(args,  zone=nil, perm=true, failonfail=true, shell_cmd='firewall-cmd')
+  def self.execute_firewall_cmd(args,  zone=nil, perm=true, failonfail=true, check_online=true)
+    if check_online and not online?
+      shell_cmd = 'firewall-offline-cmd'
+      perm = false
+    else
+      shell_cmd = 'firewall-cmd'
+    end
     cmd_args = []
     cmd_args << '--permanent' if perm
     cmd_args << [ '--zone', zone ] unless zone.nil?
@@ -79,11 +85,7 @@ def self.execute_firewall_cmd(args,  zone=nil, perm=true, failonfail=true, shell
 
 
   def execute_firewall_cmd(args, zone=@resource[:zone], perm=true, failonfail=true)
-    if online?
-      self.class.execute_firewall_cmd(args, zone, perm, failonfail)
-    else
-      self.class.execute_firewall_cmd(args, zone, false, failonfail, 'firewall-offline-cmd')
-    end
+    self.class.execute_firewall_cmd(args, zone, perm, failonfail)
   end
 
   # Arguments should be parsed as separate array entities, but quoted arg
@@ -111,6 +113,10 @@ def offline?
   end
 
   def online?
+    self.class.online?
+  end
+
+  def self.online?
     check_running_state unless state == true
     state == true
   end

lib/puppet/provider/firewalld_ipset/firewall_cmd.rb

@@ -7,21 +7,78 @@
 ) do
   desc "Interact with firewall-cmd"
 
+  mk_resource_methods
+
+  def self.instances
+    ipset_ids = execute_firewall_cmd(['--get-ipsets'], nil).split(" ")
+    ipset_ids.collect do |ipset_id|
+      ipset_raw = execute_firewall_cmd(["--info-ipset=#{ipset_id}"], nil)
+      raw_options = ipset_raw.match(/options: (.*)/)
+      options = {}
+      if raw_options
+        raw_options[1].split(' ').each { |v|
+          k, v = v.split('=')
+          options[k.to_sym] = v
+        }
+      end
+      new(
+        ensure: :present,
+        name: ipset_id,
+        type: ipset_raw.match(/type: (.*)/)[1],
+        **options
+      )
+    end
+  end
+
+  def self.prefetch(resources)
+    instances.each do |prov|
+      if (resource = resources[prov.name])
+        resource.provider = prov
+      end
+    end
+  end
+
   def exists?
-    execute_firewall_cmd(['--get-ipsets'], nil).split(" ").include?(@resource[:name])
+    @property_hash[:ensure] == :present
   end
 
   def create
     args = []
     args << ["--new-ipset=#{@resource[:name]}"]
     args << ["--type=#{@resource[:type]}"]
-    args << ["--option=#{@resource[:options].map { |a,b| "#{a}=#{b}" }.join(',')}"] if @resource[:options]
+    options = {
+      :family => @resource[:family],
+      :hashsize => @resource[:hashsize],
+      :maxelem => @resource[:maxelem],
+      :timeout => @resource[:timeout]
+    }
+    options = options.merge(@resource[:options]) if @resource[:options]
+    options.each do |option_name, value|
+      if value
+        args << ["--option=#{option_name}=#{value}"]
+      end
+    end
     execute_firewall_cmd(args.flatten, nil)
-    @resource[:entries].each { |e| add_entry(e) }
+    if @resource[:manage_entries]
+      @resource[:entries].each { |e| add_entry(e) }
+    end
+  end
+
+  %i[type maxelem family hashsize timeout].each do |method|
+    define_method("#{method}=") do |should|
+      info("Destroying and creating ipset #{@resource[:name]}")
+      destroy
+      create
+      @property_hash[method] = should
+    end
   end
 
   def entries
-    execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--get-entries"], nil).split("\n").sort
+    if @resource[:manage_entries]
+      execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--get-entries"], nil).split("\n").sort
+    else
+      @resource[:entries]
+    end
   end
 
   def add_entry(entry)
@@ -33,6 +90,10 @@ def remove_entry(entry)
   end
 
   def entries=(should_entries)
+    unless @resource[:manage_entries]
+      debug("Not managing entries for ipset #{@resource[:name]}")
+      return
+    end
     cur_entries = entries
     delete_entries = cur_entries-should_entries
     add_entries = should_entries-cur_entries

lib/puppet/type/firewalld_ipset.rb

@@ -1,16 +1,16 @@
+require_relative '../../puppet_x/firewalld/property/positive_integer'
 
 Puppet::Type.newtype(:firewalld_ipset) do
 
   @doc =%q{
     Configure IPsets in Firewalld
 
     Example:
-
-        firewalld_port {'Open port 8080 in the public Zone':
+        firewalld_ipset {'internal net':
             ensure   => 'present',
-            zone     => 'public',
-            port     => 8080,
-            protocol => 'tcp',
+            type     => 'hash:net',
+            family   => 'inet',
+            entries  => ['192.168.0.0/24']
         }
   }
 
@@ -26,6 +26,7 @@
   newparam(:type) do
     desc "Type of the ipset (default: hash:ip)"
     defaultto "hash:ip"
+    newvalues(:'bitmap:ip', :'bitmap:ip,mac', :'bitmap:port', :'hash:ip', :'hash:ip,mark', :'hash:ip,port', :'hash:ip,port,ip', :'hash:ip,port,net', :'hash:mac', :'hash:net', :'hash:net,iface', :'hash:net,net', :'hash:net,port', :'hash:net,port,net', :'list:set')
   end
 
   newparam(:options) do
@@ -40,6 +41,47 @@
     def insync?(is)
       should.sort == is.sort
     end
+
+    def change_to_s(current, desire)
+      if @resource.value(:keep_in_sync)
+        "removing entries from ipset #{(current - desire).sort.inspect},
+         adding in ipset entries #{(desire - current).sort.inspect}"
+      else
+        "adding in ipset entries #{(desire - current).sort.inspect}"
+      end
+    end
+
+    munge do |value|
+      value.gsub('/32', '')
+    end
+  end
+
+  newproperty(:family) do
+    desc "Protocol family of the IPSet"
+    newvalues(:inet6, :inet)
+  end
+
+  newproperty(:hashsize, :parent => PuppetX::Firewalld::Property::PositiveInteger) do
+    desc "Initial hash size of the IPSet"
+  end
+
+  newproperty(:maxelem, :parent => PuppetX::Firewalld::Property::PositiveInteger) do
+    desc "Maximal number of elements that can be stored in the set"
+  end
+
+  newproperty(:timeout, :parent => PuppetX::Firewalld::Property::PositiveInteger) do
+    desc "Timeout in seconds before entries expiry"
+  end
+
+  newparam(:manage_entries, :parent => Puppet::Parameter::Boolean) do
+    desc "Should we manage entries in this ipset or leave another process manage those entries"
+    defaultto true
+  end
+
+  validate do
+    if not self[:manage_entries] and self[:entries]
+      raise(Puppet::Error, "Ipset should not declare entries if it doesn't manage entries")
+    end
   end
 
 end

lib/puppet_x/firewalld/property/positive_integer.rb

@@ -0,0 +1,15 @@
+module PuppetX
+  module Firewalld
+    module Property
+      class PositiveInteger < Puppet::Property
+        def insync?(is)
+          is.to_i == should.to_i
+        end
+        validate do |value|
+          raise "#{name} should be an Integer" unless value.to_i.to_s == value.to_s
+          raise "#{name} should be greater than 0" unless value.to_i > 0
+        end
+      end
+    end
+  end
+end

manifests/init.pp

@@ -57,6 +57,7 @@
   Boolean $purge_direct_rules        = false,
   Boolean $purge_direct_chains       = false,
   Boolean $purge_direct_passthroughs = false,
+  Boolean $purge_unknown_ipsets      = false,
   Optional[String] $default_zone     = undef,
   Optional[Enum['off','all','unicast','broadcast','multicast']] $log_denied = undef,
   Optional[Enum['yes', 'no']] $cleanup_on_exit = undef,
@@ -254,4 +255,10 @@
     Service['firewalld'] -> Firewalld_direct_rule <||> ~> Exec['firewalld::reload']
     Service['firewalld'] -> Firewalld_direct_passthrough <||> ~> Exec['firewalld::reload']
 
+    if $purge_unknown_ipsets {
+      Firewalld_ipset <||>
+      ~> resources { 'firewalld_ipset':
+        purge => true,
+      }
+    }
 }

spec/classes/init_spec.rb

@@ -33,6 +33,7 @@
         :purge_direct_rules => true,
         :purge_direct_chains => true,
         :purge_direct_passthroughs => true,
+        :purge_unknown_ipsets => true
       }
     end
 
@@ -47,6 +48,12 @@
     it do
       should contain_firewalld_direct_purge('chain')
     end
+
+    it do
+      should contain_resources('firewalld_ipset')
+        .with_purge(true)
+    end
+
   end
 
   context 'with parameter ports' do

spec/unit/puppet/provider/firewalld_ipset_spec.rb

@@ -0,0 +1,121 @@
+require 'spec_helper'
+
+provider_class = Puppet::Type.type(:firewalld_ipset).provider(:firewall_cmd)
+
+describe provider_class do
+  let(:resource) do
+    @resource = Puppet::Type.type(:firewalld_ipset).new(
+      ensure: :present,
+      name: 'white',
+      type: 'hash:net',
+      entries: ['8.8.8.8'],
+      provider: described_class.name
+    )
+  end
+  let(:provider) { resource.provider }
+  before :each do
+    provider.class.stubs(:execute_firewall_cmd).with(['--get-ipsets'], nil).returns('white black')
+    provider.class.stubs(:execute_firewall_cmd).with(['--state'], nil, false, false, false).returns(Object.any_instance.stubs(exitstatus: 0))
+    provider.class.stubs(:execute_firewall_cmd).with(['--info-ipset=white'], nil).returns('white
+  type: hash:ip
+  options: maxelem=200 family=inet6
+  entries:')
+    provider.class.stubs(:execute_firewall_cmd).with(['--info-ipset=black'], nil).returns('black
+  type: hash:ip
+  options: maxelem=400 family=inet hashsize=2048
+  entries:')
+    provider.class.stubs(:execute_firewall_cmd).with(['--ipset=white', '--get-entries'], nil).returns('')
+  end
+
+  describe 'self.instances' do
+    it 'returns an array of ip sets' do
+      ipsets_names = provider.class.instances.collect(&:name)
+      expect(ipsets_names).to include('black', 'white')
+      ipsets_families = provider.class.instances.collect(&:family)
+      expect(ipsets_families).to include('inet', 'inet6')
+      ipsets_hashsize = provider.class.instances.collect(&:hashsize)
+      expect(ipsets_hashsize).to include('2048')
+      ipsets_maxelem = provider.class.instances.collect(&:maxelem)
+      expect(ipsets_maxelem).to include('200', '400')
+    end
+  end
+
+  describe 'when creating' do
+    context 'basic ipset' do
+      it 'should create a new ipset with entries' do
+        resource.expects(:[]).with(:name).returns('white').at_least_once
+        resource.expects(:[]).with(:type).returns('hash:net').at_least_once
+        resource.expects(:[]).with(:family).returns('inet').at_least_once
+        resource.expects(:[]).with(:hashsize).returns(1024).at_least_once
+        resource.expects(:[]).with(:maxelem).returns(65_536).at_least_once
+        resource.expects(:[]).with(:timeout).returns(nil).at_least_once
+        resource.expects(:[]).with(:options).returns({}).at_least_once
+        resource.expects(:[]).with(:manage_entries).returns(true)
+        resource.expects(:[]).with(:entries).returns(['192.168.0/24', '10.0.0/8'])
+        provider.expects(:execute_firewall_cmd).with(['--new-ipset=white', '--type=hash:net', '--option=family=inet', '--option=hashsize=1024', '--option=maxelem=65536'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=192.168.0/24'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=10.0.0/8'], nil)
+        provider.create
+      end
+    end
+  end
+
+  describe 'when modifying' do
+    context 'hashsize' do
+      it 'should remove and create a new ipset' do
+        resource.expects(:[]).with(:name).returns('white').at_least_once
+        resource.expects(:[]).with(:type).returns('hash:net').at_least_once
+        resource.expects(:[]).with(:family).returns('inet').at_least_once
+        resource.expects(:[]).with(:hashsize).returns(nil)
+        resource.expects(:[]).with(:hashsize).returns(2048)
+        resource.expects(:[]).with(:maxelem).returns(nil).at_least_once
+        resource.expects(:[]).with(:timeout).returns(nil).at_least_once
+        resource.expects(:[]).with(:options).returns({}).at_least_once
+        resource.expects(:[]).with(:manage_entries).returns(true).at_least_once
+        resource.expects(:[]).with(:entries).returns(['192.168.0/24', '10.0.0/8']).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--new-ipset=white', '--type=hash:net', '--option=family=inet'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--new-ipset=white', '--type=hash:net', '--option=family=inet', '--option=hashsize=2048'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=192.168.0/24'], nil).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=10.0.0/8'], nil).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--delete-ipset=white'], nil)
+        provider.create
+        provider.hashsize = 2048
+      end
+    end
+    context 'entries' do
+      it 'should remove and add entries' do
+        resource.expects(:[]).with(:name).returns('white').at_least_once
+        resource.expects(:[]).with(:type).returns('hash:net').at_least_once
+        resource.expects(:[]).with(:family).returns('inet').at_least_once
+        resource.expects(:[]).with(:hashsize).returns(nil)
+        resource.expects(:[]).with(:maxelem).returns(nil).at_least_once
+        resource.expects(:[]).with(:timeout).returns(nil).at_least_once
+        resource.expects(:[]).with(:options).returns({}).at_least_once
+        resource.expects(:[]).with(:manage_entries).returns(true).at_least_once
+        resource.expects(:[]).with(:entries).returns(['192.168.0.0/24', '10.0.0.0/8']).at_least_once
+        provider.expects(:entries).returns(['192.168.0.0/24', '10.0.0.0/8'])
+        provider.expects(:execute_firewall_cmd).with(['--new-ipset=white', '--type=hash:net', '--option=family=inet'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=192.168.0.0/24'], nil).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=10.0.0.0/8'], nil).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entry=192.168.14.0/24'], nil)
+        provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--remove-entry=192.168.0.0/24'], nil)
+        provider.create
+        provider.entries = ['192.168.14.0/24', '10.0.0.0/8']
+      end
+      it 'should ignore entries when manage_entries is false ' do
+        resource.expects(:[]).with(:name).returns('white').at_least_once
+        resource.expects(:[]).with(:type).returns('hash:net').at_least_once
+        resource.expects(:[]).with(:family).returns('inet').at_least_once
+        resource.expects(:[]).with(:hashsize).returns(nil)
+        resource.expects(:[]).with(:maxelem).returns(nil).at_least_once
+        resource.expects(:[]).with(:timeout).returns(nil).at_least_once
+        resource.expects(:[]).with(:options).returns({}).at_least_once
+        resource.expects(:[]).with(:manage_entries).returns(false).at_least_once
+        provider.expects(:execute_firewall_cmd).with(['--new-ipset=white', '--type=hash:net', '--option=family=inet'], nil)
+        provider.create
+        provider.entries = ['192.168.14.0/24', '10.0.0.0/8']
+      end
+
+    end
+  end
+end