(CONT-962) Simplify type of all address fields from Pattern to String[1]

This is done as the regex to accurately assess whether the passed string is a valid IPv4 or IPv6 address is overly complex and too difficult to ensure accuracy.
For reference an attempt at creating one is shown below:
/^(?:!\s)?(?>(?>([a-f0-9]{1,4})(?>:(?1)){7}|(?!(?:.*[a-f0-9](?>:|$)){8,})((?1)(?>:(?1)){0,6})?::(?2)?)|(?>(?>(?1)(?>:(?1)){5}:|(?!(?:.*[a-f0-9]:){6,})(?3)?::(?>((?1)(?>:(?1)){0,4}):)?)?(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])(?>\.(?4)){3}))$

Author: david22swan

lib/puppet/provider/firewall/firewall.rb

@@ -554,7 +554,7 @@ def self.process_get(_context, rule_hash, rule, counter)
     if !rule_hash[:name]
       num = 9000 + counter
       rule_hash[:name] = "#{num} #{Digest::SHA256.hexdigest(rule[0])}"
-    elsif !rule_hash[:name].match(%r{(^\d+(?:\s\w+)+$)})
+    elsif !rule_hash[:name].match(%r{(^\d+(?:[ \t-]\S+)+$)})
       num = 9000 + counter
       rule_hash[:name] = "#{num} #{rule_hash[:name]}"
     end

lib/puppet/type/firewall.rb

@@ -133,7 +133,7 @@
       DESC
     },
     name: {
-      type: 'Pattern[/(^\d+(?:[ \t]\S+)+$)/]',
+      type: 'Pattern[/(^\d+(?:[ \t-]\S+)+$)/]',
       behaviour: :namevar,
       desc: <<-DESC
       The canonical name of the rule. This name is also used for ordering
@@ -147,7 +147,7 @@
       DESC
     },
     line: {
-      type: 'Optional[String]',
+      type: 'Optional[String[1]]',
       behaviour: :read_only,
       desc: <<-DESC
       A read only attribute containing the full rule, used when deleting only.
@@ -200,7 +200,7 @@
       DESC
     },
     source: {
-      type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       The source address. For example:
 
@@ -214,7 +214,7 @@
       DESC
     },
     destination: {
-      type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       The destination address to match. For example:
 
@@ -624,7 +624,7 @@
       DESC
     },
     ctorigsrc: {
-      type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       The original source address using the conntrack module. For example:
 
@@ -640,7 +640,7 @@
     ctorigdst: {
       type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
       desc: <<-DESC
-      The original destination address using the conntrack module. For example:
+      type: 'Optional[String[1]]',
 
           ctorigdst => '192.168.2.0/24'
 
@@ -652,7 +652,7 @@
       DESC
     },
     ctreplsrc: {
-      type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       The reply source address using the conntrack module. For example:
 
@@ -666,7 +666,7 @@
       DESC
     },
     ctrepldst: {
-      type: 'Optional[Pattern[/^(?:!\s)?\d+\.\d+\.\d+\.\d+(?:\/\d+)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       The reply destination address using the conntrack module. For example:
 
@@ -1163,7 +1163,7 @@
       DESC
     },
     todest: {
-      type: 'Optional[Pattern[/^\d+\.\d+\.\d+\.\d+(?:-\d+\.\d+\.\d+\.\d+)?(?:\:\d+(?:-\d+(?:\/\d+)?)?)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       When using jump => "DNAT" you can specify the new destination address using this paramter.
       Can specify a single new destination IP address or an inclusive range of IP addresses.
@@ -1172,7 +1172,7 @@
       DESC
     },
     tosource: {
-      type: 'Optional[Pattern[/^\d+\.\d+\.\d+\.\d+(?:-\d+\.\d+\.\d+\.\d+)?(?:\:\d+(?:-\d+)?)?$/]]',
+      type: 'Optional[String[1]]',
       desc: <<-DESC
       When using jump => "SNAT" you can specify the new source address using this paramter.
       Can specify a single new destination IP address or an inclusive range of IP addresses.

lib/puppet_x/puppetlabs/firewall/utility.rb

@@ -242,7 +242,7 @@ def self.to_hex32(value)
     # Converts a given number to its protocol keyword
     # https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
     def self.proto_number_to_name(value)
-      return value if %r{^(?:!\s)?([a-z]+)$}.match?(value)
+      return value if %r{^(?:!\s)?([a-z])}.match?(value)
       match = value.to_s.match(%r{^(!\s)?(.*)})
       keyword = case match[2]
                 when '1' then 'icmp'

spec/acceptance/firewall_duplicate_comment_spec.rb

@@ -1,40 +1,40 @@
-# # frozen_string_literal: true
+# frozen_string_literal: true
 
-# require 'spec_helper_acceptance'
+require 'spec_helper_acceptance'
 
-# describe 'firewall - duplicate comments' do
-#   before(:all) do
-#     if os[:family] == 'ubuntu' || os[:family] == 'debian'
-#       update_profile_file
-#     end
-#   end
+describe 'firewall - duplicate comments' do
+  before(:all) do
+    if os[:family] == 'ubuntu' || os[:family] == 'debian'
+      update_profile_file
+    end
+  end
 
-#   after(:each) do
-#     iptables_flush_all_tables
-#   end
+  after(:each) do
+    iptables_flush_all_tables
+  end
 
-#   context 'when a duplicate comment is found' do
-#     pp = <<-PUPPETCODE
-#     class { 'firewall': }
-#     resources { 'firewall':
-#       purge => true,
-#     }
+  context 'when a duplicate comment is found' do
+    pp = <<-PUPPETCODE
+    class { 'firewall': }
+    resources { 'firewall':
+      purge => true,
+    }
 
-#     firewall { '550 destination':
-#       proto  => tcp,
-#       dport   => '550',
-#       action => accept,
-#       destination => '192.168.2.0/24',
-#     }
-#     PUPPETCODE
+    firewall { '550 destination':
+      proto => tcp,
+      dport => '550',
+      jump => accept,
+      destination => '192.168.2.0/24',
+    }
+    PUPPETCODE
 
-#     it 'raises an error' do
-#       run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
-#       run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 552 -j ACCEPT -m comment --comment "550 destination"')
+    it 'raises an error' do
+      run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+      run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 552 -j ACCEPT -m comment --comment "550 destination"')
 
-#       apply_manifest(pp) do |r|
-#         expect(r.stderr).to include('Duplicate rule found for 550 destination. Skipping update.')
-#       end
-#     end
-#   end
-# end
+      apply_manifest(pp) do |r|
+        expect(r.stderr).to include('Duplicate names have been found within your Firewalls. This will prevent the module from working correctly and must be manually resolved.')
+      end
+    end
+  end
+end

spec/acceptance/resource_cmd_spec.rb

@@ -202,10 +202,9 @@
     end
   end
 
-  # version of iptables that ships with el5 doesn't work with the
   # ip6tables provider
   # TODO: Test below fails if this file is run seperately. i.e. bundle exec rspec spec/acceptance/resource_cmd_spec.rb
-  context 'when dport/sport with ip6tables', unless: os[:family] == 'redhat' && os[:release].start_with?('5') do
+  context 'when dport/sport with ip6tables' do
     before :all do
       if os['family'] == 'debian'
         run_shell('echo "iptables-persistent iptables-persistent/autosave_v4 boolean false" | debconf-set-selections')