Initial creation of class firewall for issue #10984

[mrwacky42]

• Add Exec[firewall-persist] to save rules. This allows the host to
  have iptables rules on reboot, before puppet runs.
• Add brains to the iptables/ip6tables providers to work around issues with old iptales not autoloading the kernel
  modules.
• Debian Lenny and older hates you. Add iptables init scripts for loading iptables at
  boot on Debian.

[kbarber]

I think this file probably can be removed IMHO. It's not that useful from a unit test perspective. Better to use rspec-puppet for testing.

[mrwacky42]

Agreed. I added it because puppet-module would create it. However, it seems I did it wrong.

[kbarber]

Yeah - I'm not a fan of the scaffold we use for puppet-module. Its not bad for most things, but there are parts I don't agree with. I probably should help fix that instead of whinging about it right? :-).

[kbarber]

So generally:

• Debian Squeeze and higher have 'iptables-persistent' which deals with all the persistent stuff. So the way you deal with squeeze and lenny (or pre lenny) may actually be quite different
• I would like to see the use of 'rspec-puppet' to test this puppet class, I can see this is going to be something people poke with forever (since people are more likely to raise patches on puppet code versus ruby code) - and without unit tests we may find ourselves regressing.

[mrwacky42]

I'm working on future-proofing against Squeeze and Wheezy.

Meanwhile, are there any working examples of rspec-puppet that I can reference? I'm very much a ruby neophyte.

[kbarber]

Sure. Well rpec-puppet isn't the easiest of things but I believe rodjek's project has some sweet docs these days.

https://github.com/rodjek/rspec-puppet

You'll need the rspec and mocha gems for any testing - plus the rspec-puppet gem of course (I just use 'gem' & 'rvm' on my mac).

So the idea is ... if you want to test that on debian for example a file gets created, you mock those facts:

let(:facts) { {:operatingsystem => 'Debian', :kernel => 'Linux', } }

Then using this kind of describe you declare that you are analyzing a class context:

describe 'myclass', :type => :class do
  ...
end

And inside you want an example that makes sure in the context the correct file is installed.

Putting it all together in say spec/classes/firewall_spec.rb (the path is magic and implies the firewall class in this case):

describe "debian tests" do
  let(:facts) { {:operatingsystem => 'Debian', :kernel => 'Linux', } }

  it { should contain_file('/etc/iptables/rules.v4').with_ensure("present") }
end

You can then see if it works with:

rspec spec/classes/firewall_spec.rb

Other examples:

debian "ipv6 tests" do
  let(:facts) { {:operatingsystem => 'Debian', :kernel => 'Linux', ip6tables_version => '1.4.1.1' } }
  it { should contain_file('/etc/iptables/rules.v6').with_ensure("present") }
end

You starting to get the idea?

Even if you do the most basic of tests, its going to:

• Do the most basic of validation (which is more or less the original aim of the test/firewall_class.pp file you created)
• At least test some of your assertions - and ensure no one messes up the logic in later patches
• Once you get the basic tests in there, people can add to them - and we can suggest additions to help stop regressions.
• The tests get into our CI which is tested on lots of platforms and rubies: https://jenkins.puppetlabs.com/view/Puppet%20Modules/job/Puppet%20Module%20-%20firewall/

So yeah - I'm happy to help if you need it ... I don't want make this feel overwhelming so if you want to pair up on this patch or you want me to help just let me know.

[mrwacky42]

This doesn't work yet.

Failures:

1. firewall Debian Lenny tests
   Failure/Error: it { should contain_service('iptables-persistent') }
   Puppet::Error:
   Could not find class firewall for testhost at line 2 on node testhost

   ./spec/classes/firewall_spec.rb:14:in `block (3 levels) in <top (required)>'

Definitely seeking insight here.

[kbarber]

So ... you need to do this:

--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -8,6 +8,7 @@ require 'puppet'
 require 'mocha'
 gem 'rspec', '>=2.0.0'
 require 'rspec/expectations'
+require 'rspec-puppet'

 # So everyone else doesn't have to include this base constant.
 module PuppetSpec
@@ -31,6 +32,8 @@ end
 RSpec.configure do |config|
   include PuppetSpec::Fixtures

+  config.module_path = File.join(File.dirname(__FILE__), '../../')
+
   config.mock_with :mocha

   config.before :each do
@@ -47,7 +50,7 @@ RSpec.configure do |config|

     # Set the confdir and vardir to gibberish so that tests
     # have to be correctly mocked.
-    Puppet[:confdir] = "/dev/null"
     Puppet[:vardir] = "/dev/null"

     # Avoid opening ports to the outside world
@@ -79,4 +82,5 @@ RSpec.configure do |config|

     GC.enable
   end

And the directory needs to be 'firewall' for the autoloader to do the right thing. I'd love a workaround for that though ... I guess there might be a way to do an explicit 'import' somehow before the tests run.

[mrwacky42]

Ok, tests implemented!

This is ready for final review.

[kbarber]

Where does ip6tables_config come from? I can't see it defined elsewhere in the code.

[mrwacky42]

Is there a way to reopen this pull request so it gets merged ?

[geoffdavis]

I'd love to see this merged as well. As it stands, I backported this into a site-specific class on my local configuration.

Can pull requests be re-opened, or do you have to file another one?