From 27dad65c7da2e570559998fe39da4fc505d9c2ce Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Wed, 18 Mar 2015 10:02:07 +0100 Subject: [PATCH 1/7] Completely rewritten vlan logic With this refactor I implement the idea that the provider should just check for the right syntax (i.e. all the needed options are passed to it) instead of trying to reimplement what Debian *vlan-interfaces* already does. --- .../provider/network_config/interfaces.rb | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/lib/puppet/provider/network_config/interfaces.rb b/lib/puppet/provider/network_config/interfaces.rb index b34de4fd..8a9a352b 100644 --- a/lib/puppet/provider/network_config/interfaces.rb +++ b/lib/puppet/provider/network_config/interfaces.rb @@ -190,8 +190,18 @@ def self.parse_file(filename, contents) #Instance[name].name = name Instance[name].family = family Instance[name].method = method - Instance[name].mode = :raw - + # for the vlan naming conventions (a mess), see + # man 5 vlan-interfaces + case name + # 'vlan22' + when /^vlan/ + Instance[name].mode = :vlan + # 'eth2.0003' or 'br1.2' + when /^[a-z]{1,}[0-9]{1,}\.[0-9]{1,4}/ + Instance[name].mode = :vlan + else + Instance[name].mode = :raw + end else # If we match on a string with a leading iface, but it isn't in the # expected format, malformed blar blar @@ -225,7 +235,6 @@ def self.parse_file(filename, contents) when 'address'; Instance[name].ipaddress = val when 'netmask'; Instance[name].netmask = val when 'mtu'; Instance[name].mtu = val - when 'vlan-raw-device'; Instance[name].mode = :vlan else Instance[name].options[key] << val end else @@ -273,15 +282,14 @@ def self.format_file(filename, providers) stanza << %{iface #{provider.name} #{provider.family} #{provider.method}} if provider.mode == :vlan - # if this is a :vlan mode interface than the name of the - # `vlan-raw-device` is implied by the `iface` name in the format - # fooX. - - # The valid vlan ID range is 0-4095; 4096 is out of range - vlan_range_regex = %r[\d{1,3}|40[0-9][0-5]] - raw_device = provider.name.match(%r[\A([a-z]+\d+)(?::\d+|\.#{vlan_range_regex})?\Z])[1] - - stanza << %{vlan-raw-device #{raw_device}} + if provider.options["vlan-raw-device"] + stanza << " vlan-raw-device #{provider.options["vlan-raw-device"]}" + else + vlan_range_regex = %r[\d{1,3}|40[0-9][0-5]] + if ! provider.name.match(%r[\A([a-z]+\d+)(?::\d+|\.#{vlan_range_regex})\Z]) + raise Puppet::Error, "Interface #{provider.name}: missing vlan-raw-device or wrong VLAN ID in the iface name " + end + end end [ @@ -295,7 +303,10 @@ def self.format_file(filename, providers) if provider.options and provider.options != :absent provider.options.each_pair do |key, val| if val.is_a? String - stanza << " #{key} #{val}" + # dont print property because we've already added it to the stanza + if key != "vlan-raw-device" + stanza << " #{key} #{val}" + end elsif val.is_a? Array val.each { |entry| stanza << " #{key} #{entry}" } else From db7302741d2dae0113dba460c347e0fbd3737d2e Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Fri, 27 Mar 2015 12:15:29 +0100 Subject: [PATCH 2/7] add bundler Gemfile.lock to run tests in more environments --- Gemfile.lock | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Gemfile.lock diff --git a/Gemfile.lock b/Gemfile.lock new file mode 100644 index 00000000..40e10b2e --- /dev/null +++ b/Gemfile.lock @@ -0,0 +1,44 @@ +GIT + remote: https://github.com/rodjek/rspec-puppet + revision: a9a837669cf6955279f02d1d9b524dc140b9d3e8 + specs: + rspec-puppet (2.0.1) + rspec + +GEM + remote: https://rubygems.org/ + specs: + CFPropertyList (2.2.8) + diff-lcs (1.1.3) + facter (2.4.1) + CFPropertyList (~> 2.2.6) + hiera (1.3.4) + json_pure + json_pure (1.8.2) + metaclass (0.0.4) + mocha (0.10.5) + metaclass (~> 0.0.1) + puppet (3.7.5) + facter (> 1.6, < 3) + hiera (~> 1.0) + json_pure + rake (10.4.2) + rspec (2.10.0) + rspec-core (~> 2.10.0) + rspec-expectations (~> 2.10.0) + rspec-mocks (~> 2.10.0) + rspec-core (2.10.1) + rspec-expectations (2.10.0) + diff-lcs (~> 1.1.3) + rspec-mocks (2.10.1) + +PLATFORMS + ruby + +DEPENDENCIES + facter (>= 1.6.2) + mocha (~> 0.10.5) + puppet + rake + rspec (~> 2.10.0) + rspec-puppet! From c3e6179fa2749b3f3736b98cdc3c39502ecfb1f6 Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Fri, 27 Mar 2015 12:19:10 +0100 Subject: [PATCH 3/7] reorder stanza output when using vlanNN format and fix spec tests. I removed the old eth0.1 test because it does not really test anything new, because the vlan-raw-device option should be present only if specified and not always. Maybe I should add another eth0.1 test to see if the vlan regexp raise an error when a mispelled vlan is found --- .../provider/network_config/interfaces.rb | 20 ++++++++-------- .../network_config/interfaces_spec.rb | 23 +++++++++++-------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/lib/puppet/provider/network_config/interfaces.rb b/lib/puppet/provider/network_config/interfaces.rb index 8a9a352b..a72c5b0b 100644 --- a/lib/puppet/provider/network_config/interfaces.rb +++ b/lib/puppet/provider/network_config/interfaces.rb @@ -281,24 +281,24 @@ def self.format_file(filename, providers) stanza = [] stanza << %{iface #{provider.name} #{provider.family} #{provider.method}} + [ + [:ipaddress, 'address'], + [:netmask, 'netmask'], + [:mtu, 'mtu'], + ].each do |(property, section)| + stanza << "#{section} #{provider.send property}" if provider.send(property) and provider.send(property) != :absent + end + if provider.mode == :vlan if provider.options["vlan-raw-device"] - stanza << " vlan-raw-device #{provider.options["vlan-raw-device"]}" + stanza << "vlan-raw-device #{provider.options["vlan-raw-device"]}" else vlan_range_regex = %r[\d{1,3}|40[0-9][0-5]] if ! provider.name.match(%r[\A([a-z]+\d+)(?::\d+|\.#{vlan_range_regex})\Z]) raise Puppet::Error, "Interface #{provider.name}: missing vlan-raw-device or wrong VLAN ID in the iface name " end end - end - - [ - [:ipaddress, 'address'], - [:netmask, 'netmask'], - [:mtu, 'mtu'], - ].each do |(property, section)| - stanza << "#{section} #{provider.send property}" if provider.send(property) and provider.send(property) != :absent - end + end if provider.options and provider.options != :absent provider.options.each_pair do |key, val| diff --git a/spec/unit/provider/network_config/interfaces_spec.rb b/spec/unit/provider/network_config/interfaces_spec.rb index 4af3647a..0d3969e4 100644 --- a/spec/unit/provider/network_config/interfaces_spec.rb +++ b/spec/unit/provider/network_config/interfaces_spec.rb @@ -135,8 +135,9 @@ def fixture_data(file) :mtu => "1500", :mode => :vlan, :options => { - "broadcast" => "172.16.0.255", - "gateway" => "172.16.0.1", + "broadcast" => "172.16.0.255", + "gateway" => "172.16.0.1", + "vlan-raw-device" => "eth0", } } end @@ -174,9 +175,9 @@ def fixture_data(file) ) end - let(:eth0_1_provider) do - stub('eth0_1_provider', - :name => "eth0.1", + let(:vlan20_provider) do + stub('vlan20_provider', + :name => "vlan20", :ensure => :present, :onboot => true, :hotplug => true, @@ -186,7 +187,9 @@ def fixture_data(file) :netmask => "255.255.0.0", :mtu => "1500", :mode => :vlan, - :options => nil + :options => { + 'vlan-raw-device' => 'eth1' + } ) end @@ -300,17 +303,17 @@ def fixture_data(file) end describe "writing vlan iface blocks" do - let(:content) { described_class.format_file('', [eth0_1_provider]) } + let(:content) { described_class.format_file('', [vlan20_provider]) } it "should add all options following the iface block" do block = [ - "iface eth0.1 inet static", - "vlan-raw-device eth0", + "iface vlan20 inet static", "address 169.254.0.1", "netmask 255.255.0.0", "mtu 1500", + "vlan-raw-device eth1", ].join("\n") - content.split('\n').find {|line| line.match(/iface eth0/)}.should match(block) + content.split('\n').find {|line| line.match(/iface vlan20/)}.should match(block) end end From a0ed4c4a301dd5c5e79133ff0bf4773ec2fa5bcf Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Fri, 27 Mar 2015 13:47:33 +0100 Subject: [PATCH 4/7] added test to check that ethN.MM syntax is correct (i.e. vlan ID is lower than 4096) --- .../network_config/interfaces_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/unit/provider/network_config/interfaces_spec.rb b/spec/unit/provider/network_config/interfaces_spec.rb index 0d3969e4..85f44758 100644 --- a/spec/unit/provider/network_config/interfaces_spec.rb +++ b/spec/unit/provider/network_config/interfaces_spec.rb @@ -193,6 +193,23 @@ def fixture_data(file) ) end + let(:eth1_4500_provider) do + stub('eth1_4500_provider', + :name => "eth1.4500", + :ensure => :present, + :onboot => true, + :hotplug => true, + :family => "inet", + :method => "dhcp", + :ipaddress => nil, + :netmask => nil, + :mtu => nil, + :mode => :vlan, + :options => {}, + ) + end + + let(:eth1_provider) do stub('eth1_provider', :name => "eth1", @@ -317,6 +334,13 @@ def fixture_data(file) end end + describe "writing wrong vlan iface blocks" do + let(:content) { described_class.format_file('', [eth1_4500_provider]) } + it "should fail with wrong VLAN ID" do + expect { content }.to raise_error(Puppet::Error, /Interface eth1.4500: missing vlan-raw-device or wrong VLAN ID in the iface name/) + end + end + describe "writing the options section" do let(:content) { described_class.format_file('', [eth1_provider]) } From a3ece2f38680cc71a2004d9b2072895f2ca2993f Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Fri, 27 Mar 2015 18:09:44 +0100 Subject: [PATCH 5/7] added test for erroneous vlan configuration --- .../provider/network_config/interfaces.rb | 4 ++-- .../network_config/interfaces_spec.rb | 23 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/puppet/provider/network_config/interfaces.rb b/lib/puppet/provider/network_config/interfaces.rb index a72c5b0b..9fa96eff 100644 --- a/lib/puppet/provider/network_config/interfaces.rb +++ b/lib/puppet/provider/network_config/interfaces.rb @@ -294,8 +294,8 @@ def self.format_file(filename, providers) stanza << "vlan-raw-device #{provider.options["vlan-raw-device"]}" else vlan_range_regex = %r[\d{1,3}|40[0-9][0-5]] - if ! provider.name.match(%r[\A([a-z]+\d+)(?::\d+|\.#{vlan_range_regex})\Z]) - raise Puppet::Error, "Interface #{provider.name}: missing vlan-raw-device or wrong VLAN ID in the iface name " + if ! provider.name.match(%r[\A([a-z]+\d+)(?::\d+|\.#{vlan_range_regex})\Z]) + raise Puppet::Error, "Interface #{provider.name}: missing vlan-raw-device or wrong VLAN ID in the iface name" end end end diff --git a/spec/unit/provider/network_config/interfaces_spec.rb b/spec/unit/provider/network_config/interfaces_spec.rb index 85f44758..2639c9fa 100644 --- a/spec/unit/provider/network_config/interfaces_spec.rb +++ b/spec/unit/provider/network_config/interfaces_spec.rb @@ -193,6 +193,22 @@ def fixture_data(file) ) end + let(:vlan10_provider) do + stub('vlan10_provider', + :name => "vlan10", + :ensure => :present, + :onboot => true, + :hotplug => true, + :family => "inet", + :method => "dhcp", + :ipaddress => nil, + :netmask => nil, + :mtu => nil, + :mode => :vlan, + :options => {} + ) + end + let(:eth1_4500_provider) do stub('eth1_4500_provider', :name => "eth1.4500", @@ -341,6 +357,13 @@ def fixture_data(file) end end + describe "writing wrong vlanNN iface blocks" do + let(:content) { described_class.format_file('', [vlan10_provider]) } + it "should fail with missing vlan-raw-device" do + expect { content }.to raise_error(Puppet::Error, /Interface vlan10: missing vlan-raw-device or wrong VLAN ID in the iface name/) + end + end + describe "writing the options section" do let(:content) { described_class.format_file('', [eth1_provider]) } From 9b6bb229cf6c232d2df7097c4e5dcfbd5152dabf Mon Sep 17 00:00:00 2001 From: Davide Ferrari Date: Fri, 27 Mar 2015 18:57:59 +0100 Subject: [PATCH 6/7] Remove & ignore Gemfile.lock --- .gitignore | 1 + Gemfile.lock | 44 -------------------------------------------- 2 files changed, 1 insertion(+), 44 deletions(-) delete mode 100644 Gemfile.lock diff --git a/.gitignore b/.gitignore index 63f779fc..b776c831 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ doc Gemfile.local +Gemfile.lock diff --git a/Gemfile.lock b/Gemfile.lock deleted file mode 100644 index 40e10b2e..00000000 --- a/Gemfile.lock +++ /dev/null @@ -1,44 +0,0 @@ -GIT - remote: https://github.com/rodjek/rspec-puppet - revision: a9a837669cf6955279f02d1d9b524dc140b9d3e8 - specs: - rspec-puppet (2.0.1) - rspec - -GEM - remote: https://rubygems.org/ - specs: - CFPropertyList (2.2.8) - diff-lcs (1.1.3) - facter (2.4.1) - CFPropertyList (~> 2.2.6) - hiera (1.3.4) - json_pure - json_pure (1.8.2) - metaclass (0.0.4) - mocha (0.10.5) - metaclass (~> 0.0.1) - puppet (3.7.5) - facter (> 1.6, < 3) - hiera (~> 1.0) - json_pure - rake (10.4.2) - rspec (2.10.0) - rspec-core (~> 2.10.0) - rspec-expectations (~> 2.10.0) - rspec-mocks (~> 2.10.0) - rspec-core (2.10.1) - rspec-expectations (2.10.0) - diff-lcs (~> 1.1.3) - rspec-mocks (2.10.1) - -PLATFORMS - ruby - -DEPENDENCIES - facter (>= 1.6.2) - mocha (~> 0.10.5) - puppet - rake - rspec (~> 2.10.0) - rspec-puppet! From 3f5708a78ad9dae734e475c29bbad6a03d188952 Mon Sep 17 00:00:00 2001 From: Adrien Thebo Date: Wed, 18 Mar 2015 08:25:05 -0700 Subject: [PATCH 7/7] Use gem version of rspec-puppet --- Gemfile | 2 +- spec/unit/provider/network_config/interfaces_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index af908045..05fb9dab 100644 --- a/Gemfile +++ b/Gemfile @@ -16,7 +16,7 @@ gem 'facter', '>= 1.6.2' group :test, :development do gem 'rspec', '~> 2.10.0' gem 'mocha', '~> 0.10.5' - gem 'rspec-puppet', :git => 'https://github.com/rodjek/rspec-puppet' + gem 'rspec-puppet' gem 'rake' end diff --git a/spec/unit/provider/network_config/interfaces_spec.rb b/spec/unit/provider/network_config/interfaces_spec.rb index 2639c9fa..ba4f75f9 100644 --- a/spec/unit/provider/network_config/interfaces_spec.rb +++ b/spec/unit/provider/network_config/interfaces_spec.rb @@ -221,7 +221,7 @@ def fixture_data(file) :netmask => nil, :mtu => nil, :mode => :vlan, - :options => {}, + :options => {} ) end