From dd95fb72c241b484aea7632c314b8aaac1e4d6f8 Mon Sep 17 00:00:00 2001 From: Jean-Francois Roche Date: Thu, 26 Apr 2018 12:58:37 +0200 Subject: [PATCH] Speed up ipset entries changes We now use `--add-entries-from-file` and `--remove-entries-from-file` to change firewalld ipset. Adding or removing entries one by one was really slow. This pull request is based on https://github.com/42wim/puppet-firewalld/blob/04683b46cbe6e6a925c585283941cc363752aceb/lib/puppet/provider/firewalld_ipset/firewall_cmd.rb --- .../provider/firewalld_ipset/firewall_cmd.rb | 20 ++++++++++++------- .../puppet/provider/firewalld_ipset_spec.rb | 19 ++++++++++-------- spec/unit/puppet/type/firewalld_ipset_spec.rb | 19 +++++++++++------- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/lib/puppet/provider/firewalld_ipset/firewall_cmd.rb b/lib/puppet/provider/firewalld_ipset/firewall_cmd.rb index c9f671e7..190d7471 100644 --- a/lib/puppet/provider/firewalld_ipset/firewall_cmd.rb +++ b/lib/puppet/provider/firewalld_ipset/firewall_cmd.rb @@ -60,7 +60,7 @@ def create args << ["--option=#{option_name}=#{value}"] if value end execute_firewall_cmd(args.flatten, nil) - @resource[:entries].each { |e| add_entry(e) } if @resource[:manage_entries] + add_entries_from_file(@resource[:entries]) if @resource[:manage_entries] end %i[type maxelem family hashsize timeout].each do |method| @@ -80,12 +80,18 @@ def entries end end - def add_entry(entry) - execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--add-entry=#{entry}"], nil) + def add_entries_from_file(entries) + f = Tempfile.new('ipset') + entries.each { |e| f.write("#{e}\n") } + f.close + execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--add-entries-from-file=#{f.path}"], nil) end - def remove_entry(entry) - execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--remove-entry=#{entry}"], nil) + def remove_entries_from_file(entries) + f = Tempfile.new('ipset') + entries.each { |e| f.write("#{e}\n") } + f.close + execute_firewall_cmd(["--ipset=#{@resource[:name]}", "--remove-entries-from-file=#{f.path}"], nil) end def entries=(should_entries) @@ -96,8 +102,8 @@ def entries=(should_entries) cur_entries = entries delete_entries = cur_entries - should_entries add_entries = should_entries - cur_entries - delete_entries.each { |e| remove_entry(e) } - add_entries.each { |e| add_entry(e) } + remove_entries_from_file(delete_entries) if delete_entries + add_entries_from_file(add_entries) if add_entries end def destroy diff --git a/spec/unit/puppet/provider/firewalld_ipset_spec.rb b/spec/unit/puppet/provider/firewalld_ipset_spec.rb index 345d55d0..c22203db 100644 --- a/spec/unit/puppet/provider/firewalld_ipset_spec.rb +++ b/spec/unit/puppet/provider/firewalld_ipset_spec.rb @@ -17,6 +17,13 @@ let(:provider) { resource.provider } before do + tempfile = stub('tempfile', class: Tempfile, + write: true, + flush: true, + close!: true, + close: true, + path: '/tmp/ipset-rspec') + Tempfile.stubs(:new).returns(tempfile) 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)) # rubocop:disable RSpec/AnyInstance provider.class.stubs(:execute_firewall_cmd).with(['--info-ipset=white'], nil).returns('white @@ -67,8 +74,7 @@ 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.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entries-from-file=/tmp/ipset-rspec'], nil) provider.create end end @@ -89,8 +95,7 @@ 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(['--ipset=white', '--add-entries-from-file=/tmp/ipset-rspec'], nil).at_least_once provider.expects(:execute_firewall_cmd).with(['--delete-ipset=white'], nil) provider.create provider.hashsize = 2048 @@ -110,10 +115,8 @@ 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.expects(:execute_firewall_cmd).with(['--ipset=white', '--remove-entries-from-file=/tmp/ipset-rspec'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=white', '--add-entries-from-file=/tmp/ipset-rspec'], nil).at_least_once provider.create provider.entries = ['192.168.14.0/24', '10.0.0.0/8'] end diff --git a/spec/unit/puppet/type/firewalld_ipset_spec.rb b/spec/unit/puppet/type/firewalld_ipset_spec.rb index a90fc036..e6a7182f 100644 --- a/spec/unit/puppet/type/firewalld_ipset_spec.rb +++ b/spec/unit/puppet/type/firewalld_ipset_spec.rb @@ -5,6 +5,13 @@ describe Puppet::Type.type(:firewalld_ipset) do before do Puppet::Provider::Firewalld.any_instance.stubs(:state).returns(:true) # rubocop:disable RSpec/AnyInstance + tempfile = stub('tempfile', class: Tempfile, + write: true, + flush: true, + close!: true, + close: true, + path: '/tmp/ipset-rspec') + Tempfile.stubs(:new).returns(tempfile) end describe 'type' do @@ -116,8 +123,7 @@ it 'creates' do provider.expects(:execute_firewall_cmd).with(['--new-ipset=whitelist', '--type=hash:ip'], nil) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entry=192.168.2.2'], nil) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entry=10.72.1.100'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entries-from-file=/tmp/ipset-rspec'], nil) provider.create end @@ -128,16 +134,15 @@ it 'sets entries' do provider.expects(:entries).returns([]) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entry=192.168.2.2'], nil) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entry=10.72.1.100'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entries-from-file=/tmp/ipset-rspec'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--remove-entries-from-file=/tmp/ipset-rspec'], nil) provider.entries = ['192.168.2.2', '10.72.1.100'] end it 'removes unconfigured entries' do provider.expects(:entries).returns(['10.9.9.9', '10.8.8.8', '10.72.1.100']) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entry=192.168.2.2'], nil) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--remove-entry=10.9.9.9'], nil) - provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--remove-entry=10.8.8.8'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--add-entries-from-file=/tmp/ipset-rspec'], nil) + provider.expects(:execute_firewall_cmd).with(['--ipset=whitelist', '--remove-entries-from-file=/tmp/ipset-rspec'], nil) provider.entries = ['192.168.2.2', '10.72.1.100'] end end