Fixing #762. The main problem was that I accepted the patch

in #744 which broke the templates.
In the process, I also added test code for the redhat interface
provider and rewrote how parsing worked to make it more testable.

4 files changed, 261 insertions, 78 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 040f9a176..d49aa8fc0 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,7 @@
+    Fixing some of the problems with interface management on Red Hat.
+    Puppet now uses the :netmask property and does not try to set
+    the bootproto (#762).
+
     You now must specify an environment and you are required to specify
     the valid environments for your site. (#911) 
 
diff --git a/lib/puppet/provider/interface/redhat.rb b/lib/puppet/provider/interface/redhat.rb
index 2f9c36903..467256e61 100644
--- a/lib/puppet/provider/interface/redhat.rb
+++ b/lib/puppet/provider/interface/redhat.rb
@@ -2,6 +2,9 @@ require 'puppet/provider/parsedfile'
 require 'erb'
 
 Puppet::Type.type(:interface).provide(:redhat) do
+    desc "Manage network interfaces on Red Hat operating systems.  This provider
+        parses and generates configuration files in ``/etc/sysconfig/network-scripts``."
+
 	@interface_dir = "/etc/sysconfig/network-scripts"
     confine :exists => @interface_dir
     defaultfor :operatingsystem => [:fedora, :centos, :redhat]
@@ -9,22 +12,34 @@ Puppet::Type.type(:interface).provide(:redhat) do
     # Create the setter/gettor methods to match the model.
     mk_resource_methods
 
-    @alias_template = ERB.new <<-ALIAS
+    @templates = {}
+
+    # Register a template.
+    def self.register_template(name, string)
+        @templates[name] = ERB.new(string)
+    end
+
+    # Retrieve a template by name.
+    def self.template(name)
+        @templates[name]
+    end
+
+    register_template :alias, <<-ALIAS
 DEVICE=<%= self.device %>
 ONBOOT=<%= self.on_boot %>
-BOOTPROTO=<%= self.bootproto %>
+BOOTPROTO=none
 IPADDR=<%= self.name %>
 NETMASK=<%= self.netmask %>
 BROADCAST=
 ALIAS
 
 
-    @loopback_template = ERB.new <<-LOOPBACKDUMMY
+    register_template :loopback, <<-LOOPBACKDUMMY
 DEVICE=<%= self.device %>
 ONBOOT=<%= self.on_boot %>
 BOOTPROTO=static
 IPADDR=<%= self.name %>
-NETMASK=255.255.255.255
+NETMASK=<%= self.netmask %>
 BROADCAST=
 LOOPBACKDUMMY
 
@@ -34,7 +49,6 @@ LOOPBACKDUMMY
 	# maximum number of aliases per interface
 	@max_aliases_per_iface = 10
 
-
 	@@dummies = []
 	@@aliases = Hash.new { |hash, key| hash[key] = [] }
 
@@ -43,17 +57,13 @@ LOOPBACKDUMMY
 	def self.instances
 		# parse all of the config files at once
 		Dir.glob("%s/ifcfg-*" % @interface_dir).collect do |file|
-
 			record = parse(file)
 
 			# store the existing dummy interfaces
-			if record[:interface_type] == :dummy
-				@@dummies << record[:ifnum] unless @@dummies.include?(record[:ifnum])
-			end
+            @@dummies << record[:ifnum] if (record[:interface_type] == :dummy and ! @@dummies.include?(record[:ifnum]))
+
+            @@aliases[record[:interface]] << record[:ifnum] if record[:interface_type] == :alias
 
-			if record[:interface_type] == :alias
-				@@aliases[record[:interface]] << record[:ifnum]
-			end
             new(record)
 		end
 	end
@@ -85,68 +95,16 @@ LOOPBACKDUMMY
 
     # Parse the existing file.
     def self.parse(file)
+        instance = new()
+        return instance unless FileTest.exist?(file)
 
-        opts = {}
-        return opts unless FileTest.exists?(file)
-
-        File.open(file) do |f|
-            f.readlines.each do |line|
-                if line =~ /^(\w+)=(.+)$/
-                    opts[$1.downcase.intern] = $2
-                end
-            end
-        end
-
-        # figure out the "real" device information
-        case opts[:device]
-        when /:/:
-            if opts[:device].include?(":")
-                opts[:interface], opts[:ifnum] = opts[:device].split(":")
-            end
-
-            opts[:interface_type] = :alias
-        when /^dummy/:
-            opts[:interface_type] = :loopback
-			opts[:interface] = "dummy"
-
-			# take the number of the dummy interface, as this is used
-			# when working out whether to call next_dummy when dynamically
-			# creating these
-			opts[:ifnum] = opts[:device].sub("dummy",'')
-
-			@@dummies << opts[:ifnum].to_s unless @@dummies.include?(opts[:ifnum].to_s)
-        else
-			opts[:interface_type] = :normal
-			opts[:interface] = opts[:device]
-        end
-
-		# translate whether we come up on boot to true/false
-		case opts[:onboot].downcase
-		when "yes":
-			opts[:onboot] = :true
-		when "no":
-			opts[:onboot] = :false
-		else
-			# this case should never happen, but just in case
-			opts[:onboot] = false
-		end
-
-
-        # Remove any attributes we don't want.  These would be
-        # pretty easy to support.
-        [:bootproto, :broadcast, :netmask, :device].each do |opt|
-            if opts.include?(opt)
-                opts.delete(opt)
+        File.readlines(file).each do |line|
+            if line =~ /^(\w+)=(.+)$/
+                instance.send($1.downcase + "=", $2)
             end
         end
 
-        if opts.include?(:ipaddr)
-            opts[:name] = opts[:ipaddr]
-            opts.delete(:ipaddr)
-        end
-
-        return opts
-
+        return instance
     end
 
     # Prefetch our interface list, yo.
@@ -182,14 +140,7 @@ LOOPBACKDUMMY
     # address (also dummy) on linux. For linux it's quite involved, and we
     # will use an ERB template
 	def generate
-		# choose which template to use for the interface file, based on
-		# the interface type
-        case @resource.should(:interface_type)
-        when :loopback
-			return @loopback_template.result(binding)
-        when :alias
-			return @alias_template.result(binding)
-		end
+        self.class.template(@property_hash[:interface_type]).result(binding)
 	end
 
     # Where should the file be written out?
@@ -198,7 +149,28 @@ LOOPBACKDUMMY
     def file_path
 		@resource[:interface_desc] ||= @resource[:name]
        	return File.join(@interface_dir, "ifcfg-" + @resource[:interface_desc])
+    end
 
+    # Use the device value to figure out all kinds of nifty things.
+    def device=(value)
+        case value
+        when /:/:
+            @property_hash[:interface], @property_hash[:ifnum] = value.split(":")
+            @property_hash[:interface_type] = :alias
+        when /^dummy/:
+            @property_hash[:interface_type] = :loopback
+            @property_hash[:interface] = "dummy"
+
+            # take the number of the dummy interface, as this is used
+            # when working out whether to call next_dummy when dynamically
+            # creating these
+            @property_hash[:ifnum] = value.sub("dummy",'')
+
+            @@dummies << @property_hash[:ifnum].to_s unless @@dummies.include?(@property_hash[:ifnum].to_s)
+        else
+            @property_hash[:interface_type] = :normal
+            @property_hash[:interface] = value
+        end
     end
 
 	# create the device name, so this based on the IP, and interface + type
@@ -213,6 +185,11 @@ LOOPBACKDUMMY
 		end
     end
 
+    # Set the name to our ip address.
+    def ipaddr=(value)
+        @property_hash[:name] = value
+    end
+
 	# whether the device is to be brought up on boot or not. converts
 	# the true / false of the type, into yes / no values respectively
 	# writing out the ifcfg-* files
@@ -227,6 +204,17 @@ LOOPBACKDUMMY
 		end
 	end
 
+    # Mark whether the interface should be started on boot.
+    def on_boot=(value)
+        # translate whether we come up on boot to true/false
+        case value.downcase
+        when "yes":
+            @property_hash[:onboot] = :true
+        else
+            @property_hash[:onboot] = :false
+        end
+    end
+
     # Write the new file out.
     def flush
         # Don't flush to disk if we're removing the config.
diff --git a/lib/puppet/type/interface.rb b/lib/puppet/type/interface.rb
index 1ddf68641..357adeea8 100644
--- a/lib/puppet/type/interface.rb
+++ b/lib/puppet/type/interface.rb
@@ -43,6 +43,10 @@ Puppet::Type.newtype(:interface) do
               and alias. This is use to force a given number to be used"
 	end
 
+	newproperty(:netmask) do
+		desc "The netmask for the interface."
+	end
+
 	newproperty(:ifopts) do
 		desc "Interface options."
 	end
diff --git a/spec/unit/ral/provider/interface/redhat.rb b/spec/unit/ral/provider/interface/redhat.rb
new file mode 100755
index 000000000..db56eeea8
--- /dev/null
+++ b/spec/unit/ral/provider/interface/redhat.rb
@@ -0,0 +1,187 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-11-20.
+#  Copyright (c) 2006. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../../../spec_helper'
+
+require 'puppet/provider/interface/redhat'
+
+
+provider_class = Puppet::Type.type(:interface).provider(:redhat)
+
+describe provider_class do
+    it "should not be functional on systems without a network-scripts directory" do
+        FileTest.expects(:exists?).with("/etc/sysconfig/network-scripts").returns(false)
+        provider_class.should_not be_suitable
+    end
+
+    it "should be functional on systems with a network-scripts directory" do
+        FileTest.expects(:exists?).with("/etc/sysconfig/network-scripts").returns(true)
+        provider_class.should be_suitable
+    end
+end
+
+describe provider_class, " when returning instances" do
+    it "should consider each file in the network-scripts directory an interface instance" do
+        Dir.expects(:glob).with("/etc/sysconfig/network-scripts/ifcfg-*").returns(%w{one two})
+        one = {:name => "one"}
+        two = {:name => "two"}
+        Puppet::Type::Interface::ProviderRedhat.expects(:parse).with("one").returns(one)
+        Puppet::Type::Interface::ProviderRedhat.expects(:parse).with("two").returns(two)
+        Puppet::Type::Interface::ProviderRedhat.expects(:new).with(one).returns(:one)
+        Puppet::Type::Interface::ProviderRedhat.expects(:new).with(two).returns(:two)
+        Puppet::Type::Interface::ProviderRedhat.instances.should == [:one, :two]
+    end
+end
+
+describe provider_class, " when parsing" do
+    it "should return an unmodified provider if the file does not exist" do
+        FileTest.expects(:exist?).with("/my/file").returns(false)
+        provider = mock 'provider'
+        Puppet::Type::Interface::ProviderRedhat.expects(:new).returns(provider)
+        Puppet::Type::Interface::ProviderRedhat.parse("/my/file").should equal(provider)
+    end
+
+    it "should set each attribute in the file on the provider" do
+        FileTest.expects(:exist?).with("/my/file").returns(true)
+        File.expects(:readlines).with("/my/file").returns(%w{one=two three=four})
+        provider = mock 'provider'
+        Puppet::Type::Interface::ProviderRedhat.expects(:new).returns(provider)
+        provider.expects(:one=).with('two')
+        provider.expects(:three=).with('four')
+        Puppet::Type::Interface::ProviderRedhat.parse("/my/file").should equal(provider)
+    end
+end
+
+describe provider_class do
+    before { @provider = Puppet::Type::Interface::ProviderRedhat.new }
+    it "should have a bootproto attribute" do
+        @provider.bootproto = "whatever"
+        @provider.bootproto.should == "whatever"
+    end
+
+    it "should have a netmask attribute" do
+        @provider.netmask = "whatever"
+        @provider.netmask.should == "whatever"
+    end
+
+    it "should have a broadcast attribute" do
+        @provider.broadcast = "whatever"
+        @provider.broadcast.should == "whatever"
+    end
+end
+
+describe provider_class, " when setting the device to a value containing ':'" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+        @provider.device = "one:two"
+    end
+    it "should set the interface type to :alias" do
+        @provider.interface_type.should == :alias
+    end
+    it "should set the interface to the string to the left of the ':'" do
+        @provider.interface.should == "one"
+    end
+    it "should set the ifnum to the string to the right of the ':'" do
+        @provider.ifnum.should == "two"
+    end
+end
+
+describe provider_class, " when setting the device to a value starting with 'dummy-'" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+        @provider.device = "dummy5"
+    end
+    it "should set the interface type to :loopback" do
+        @provider.interface_type.should == :loopback
+    end
+    it "should set the interface to 'dummy'" do
+        @provider.interface.should == "dummy"
+    end
+    it "should set the ifnum to remainder of value after removing 'dummy'" do
+        @provider.ifnum.should == "5"
+    end
+end
+
+describe provider_class, " when setting the device to a value containing neither 'dummy-' nor ':'" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+        @provider.device = "whatever"
+    end
+    it "should set the interface type to :normal" do
+        @provider.interface_type.should == :normal
+    end
+    it "should set the interface to the device value" do
+        @provider.interface.should == "whatever"
+    end
+end
+
+describe provider_class, " when setting the on_boot value" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+    end
+    it "should set it to :true if the value is 'yes'" do
+        @provider.on_boot = "yes"
+        @provider.onboot.should == :true
+    end
+    it "should set it to :false if the value is not 'yes'" do
+        @provider.on_boot = "no"
+        @provider.onboot.should == :false
+    end
+end
+
+describe provider_class, " when setting the ipaddr value" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+    end
+
+    it "should set the name to the provided value" do
+        @provider.ipaddr = "yay"
+        @provider.name.should == "yay"
+    end
+end
+
+describe provider_class, " when generating" do
+    before do
+        @provider = Puppet::Type::Interface::ProviderRedhat.new
+        @provider.interface_type = :alias
+        @provider.stubs(:device).returns("mydevice")
+        @provider.stubs(:on_boot).returns("myboot")
+        @provider.stubs(:name).returns("myname")
+        @provider.stubs(:interface_type).returns("myname")
+        @provider.stubs(:netmask).returns("mynetmask")
+
+        @text = @provider.generate
+    end
+
+    it "should set the bootproto to none if the interface is an alias" do
+        @text.should =~ /^BOOTPROTO=none$/
+    end
+
+    it "should set the bootproto to static if the interface is a loopback" do
+        @provider.interface_type = :loopback
+        @text = @provider.generate
+        @text.should =~ /^BOOTPROTO=static$/
+    end
+
+    it "should set the broadcast address to nothing" do
+        @text.should =~ /^BROADCAST=$/
+    end
+
+    it "should set the netmask to mynetmask" do
+        @text.should =~ /^NETMASK=mynetmask$/
+    end
+
+    it "should set the device to the provider's device" do
+        @text.should =~ /^DEVICE=mydevice$/
+    end
+
+    it "should set the onboot to the provider's on_boot value" do
+        @text.should =~ /^ONBOOT=myboot$/
+    end
+
+    it "should set the ipaddr to the provider's name" do
+        @text.should =~ /^IPADDR=myname$/
+    end
+end