From 52327e2caa1fa1c3c616a83f6b8a69fcf3caf578 Mon Sep 17 00:00:00 2001
From: Trevor Vaughan <tvaughan@onyxpoint.com>
Date: Wed, 4 Mar 2020 17:38:53 -0500
Subject: [PATCH] Service filename bugfix

This started out as a fix for the custom service filename but spiraled a
bit further during debugging.

* Auto-correct the filename for firewalld::custom_service
* Add a function `firewalld::safe_filename` for munging filenames to
  safely work with firewalld. The allowed characters were determined by
  experimentation and are not documented.
* Ensure that firewalld_zone resources automatically reload the firewall
* Ensure that firewalld_zone names are no longer than 17 characters per
  the manual
* Create firewalld::reload and firewalld::reload::complete classes to
  allow easier resource chaining
* Fix spacing in init.pp
* Ensure that all of the Execs that call firewall-cmd happen after the
  service is running
* Ensure that all permanent configuration changes happen before the
  firewalld service is started/restarted. This is critical if switching
  from nft to iptables due to the segfault bug in nft
* Ensure that all custom service declarations happen before all
  firewalld_zone resources are triggered. This is automatic in all
  native types
* Updated the service.xml.erb to an EPP file

Closes #265
---
 functions/safe_filename.pp                    |  72 ++++
 .../provider/firewalld_zone/firewall_cmd.rb   |   4 +
 lib/puppet/type/firewalld_zone.rb             |   9 +
 manifests/custom_service.pp                   |  32 +-
 manifests/init.pp                             | 325 +++++++++---------
 manifests/reload.pp                           |  12 +
 manifests/reload/complete.pp                  |  14 +
 spec/classes/init_spec.rb                     |  28 +-
 spec/classes/reload/complete_spec.rb          |  13 +
 spec/classes/reload_spec.rb                   |  13 +
 spec/functions/safe_filename_spec.rb          |  56 +++
 templates/service.xml.epp                     |  40 +++
 templates/service.xml.erb                     |  28 --
 13 files changed, 433 insertions(+), 213 deletions(-)
 create mode 100644 functions/safe_filename.pp
 create mode 100644 manifests/reload.pp
 create mode 100644 manifests/reload/complete.pp
 create mode 100644 spec/classes/reload/complete_spec.rb
 create mode 100644 spec/classes/reload_spec.rb
 create mode 100644 spec/functions/safe_filename_spec.rb
 create mode 100644 templates/service.xml.epp
 delete mode 100644 templates/service.xml.erb

diff --git a/functions/safe_filename.pp b/functions/safe_filename.pp
new file mode 100644
index 00000000..3613ae60
--- /dev/null
+++ b/functions/safe_filename.pp
@@ -0,0 +1,72 @@
+# @summary Returns a string that is safe for firewalld filenames
+#
+# @example Regular Filename
+#   $filename = 'B@d Characters!'
+#   firewalld::safe_filename($orig_string)
+#
+#   Result => 'B_d_Characters_'
+#
+# @example Filename with Options
+#   $filename = 'B@d Characters!.txt'
+#   firewalld::safe_filename(
+#     $filename,
+#     {
+#       'replacement_string' => '@@',
+#       'file_extension'     => '.txt'
+#     }
+#   )
+#
+#   Result => 'B@@d@@Characters@@.txt'
+#
+# @param filename
+#   The String to process
+#
+# @param options
+#   Various processing options
+#
+# @param options [String[1]] replacement_string
+#   The String to use when replacing invalid characters
+#
+# @option options [String[1]] file_extension
+#   This will be stripped from the end of the string prior to processing and
+#   re-added afterwards
+#
+# @return [String]
+#   Processed string
+#
+function firewalld::safe_filename(
+  String[1] $filename,
+  Struct[
+    {
+      'replacement_string' => Pattern[/[\w-]/],
+      'file_extension'     => Optional[String[1]]
+    }
+  ]         $options  = { 'replacement_string' => '_'}
+) {
+
+  $_badchar_regex = '[^\w-]'
+
+  # If we have an extension defined
+  if $options['file_extension'] {
+
+    # See if the string ends with the extension
+    $_extension_length = length($options['file_extension'])
+    if $filename[-($_extension_length), -1] == $options['file_extension'] {
+
+      # And extract the base filename
+      $_basename = $filename[0, -($_extension_length) - 1]
+    }
+  }
+
+  # If we extraced a base filename substitute on that and re-add the file extension
+  if defined('$_basename') {
+    sprintf('%s%s',
+      regsubst($_basename, $_badchar_regex, $options['replacement_string'], 'G'),
+      $options['file_extension']
+    )
+  }
+  # Otherwise, just substitute on the original filename
+  else {
+    regsubst($filename, $_badchar_regex, $options['replacement_string'], 'G')
+  }
+}
diff --git a/lib/puppet/provider/firewalld_zone/firewall_cmd.rb b/lib/puppet/provider/firewalld_zone/firewall_cmd.rb
index 2bfe1d75..ab412a10 100644
--- a/lib/puppet/provider/firewalld_zone/firewall_cmd.rb
+++ b/lib/puppet/provider/firewalld_zone/firewall_cmd.rb
@@ -197,4 +197,8 @@ def short
   def short=(new_short)
     execute_firewall_cmd(['--set-short', new_short], @resource[:name], true, false)
   end
+
+  def flush
+    reload_firewall
+  end
 end
diff --git a/lib/puppet/type/firewalld_zone.rb b/lib/puppet/type/firewalld_zone.rb
index 93e9b3c7..49f63f4c 100644
--- a/lib/puppet/type/firewalld_zone.rb
+++ b/lib/puppet/type/firewalld_zone.rb
@@ -52,6 +52,7 @@ def generate
 
   newparam(:name) do
     desc 'Name of the rule resource in Puppet'
+    isnamevar
   end
 
   newparam(:zone) do
@@ -165,6 +166,14 @@ def retrieve
     end
   end
 
+  validate do
+    [:zone, :name].each do |attr|
+      if self[attr] && (self[attr]).to_s.length > 17
+        raise(Puppet::Error, "Zone identifier '#{attr}' must be less than 18 characters long")
+      end
+    end
+  end
+
   autorequire(:service) do
     ['firewalld']
   end
diff --git a/manifests/custom_service.pp b/manifests/custom_service.pp
index 4093395a..1adca429 100644
--- a/manifests/custom_service.pp
+++ b/manifests/custom_service.pp
@@ -45,18 +45,28 @@
   Enum['present','absent'] $ensure      = 'present',
 ) {
 
-  file{"${config_dir}/${filename}.xml":
+  include firewalld::reload
+
+  # Service files may only contain alphanumeric characters and underscores.
+  # This is not documented, but has been experimentally confirmed.
+  $_safe_filename = firewalld::safe_filename($filename)
+
+  $_content = epp(
+    "${module_name}/service.xml.epp",
+    'short'       => $short,
+    'description' => $description,
+    'port'        => $port,
+    'module'      => $module,
+    'destination' => $destination,
+    'filename'    => $filename,
+    'config_dir'  => $config_dir,
+    'ensure'      => $ensure
+  )
+
+  file{ "${config_dir}/${_safe_filename}.xml":
     ensure  => $ensure,
-    content => template('firewalld/service.xml.erb'),
+    content => $_content,
     mode    => '0644',
-    notify  => Exec["firewalld::custom_service::reload-${name}"],
+    notify  => Class['firewalld::reload'],
   }
-
-  exec{ "firewalld::custom_service::reload-${name}":
-    path        =>'/usr/bin:/bin',
-    command     => 'firewall-cmd --reload',
-    onlyif      => 'firewall-cmd --state',
-    refreshonly => true,
-  }
-
 }
diff --git a/manifests/init.pp b/manifests/init.pp
index da36c6ad..fe1d0552 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -1,6 +1,4 @@
-# == Class: firewalld
-#
-# Manage the firewalld service
+# @summary Manage the firewalld service
 #
 # See the README.md for usage instructions for the firewalld_zone and
 # firewalld_rich_rule types
@@ -19,8 +17,6 @@
 #      install_gui => true,
 #    }
 #
-#
-#
 # === Authors
 #
 # Craig Dunn <craig@craigdunn.org>
@@ -29,7 +25,6 @@
 #
 # Copyright 2015 Craig Dunn
 #
-#
 class firewalld (
   Enum['present','absent','latest','installed']                 $package_ensure            = 'installed',
   String                                                        $package                   = 'firewalld',
@@ -62,210 +57,208 @@
   Optional[String]                                              $default_port_protocol     = undef,
 ) {
 
-    package { $package:
-      ensure => $package_ensure,
-      notify => Service['firewalld'],
-    }
+  include firewalld::reload
+  include firewalld::reload::complete
 
-    if $install_gui {
-      package { $config_package:
-        ensure => installed,
-      }
-    }
+  package { $package:
+    ensure => $package_ensure,
+    notify => Service['firewalld'],
+  }
 
-    Exec {
-      path => '/usr/bin:/bin',
+  if $install_gui {
+    package { $config_package:
+      ensure => installed,
     }
+  }
 
-    service { 'firewalld':
-      ensure => $service_ensure,
-      enable => $service_enable,
-    }
+  Exec {
+    path => '/usr/bin:/bin',
+  }
 
-    # firewall-cmd commands won't work if the service is stopped
-    exec { 'firewalld::reload':
-      command     => 'firewall-cmd --reload',
-      refreshonly => true,
-      onlyif      => 'firewall-cmd --state',
-    }
+  service { 'firewalld':
+    ensure => $service_ensure,
+    enable => $service_enable,
+  }
 
-    exec { 'firewalld::complete-reload':
-      command     => 'firewall-cmd --complete-reload',
-      refreshonly => true,
-      require     => Exec['firewalld::reload'],
-    }
+  # create ports
+  Firewalld_port {
+    zone      => $default_port_zone,
+    protocol  => $default_port_protocol,
+  }
 
-    # create ports
-    Firewalld_port {
-      zone      => $default_port_zone,
-      protocol  => $default_port_protocol,
+  $ports.each |String $key, Hash $attrs| {
+    firewalld_port { $key:
+      *       => $attrs,
     }
+  }
 
-    $ports.each |String $key, Hash $attrs| {
-      firewalld_port { $key:
-        *       => $attrs,
-      }
+  #...zones
+  $zones.each | String $key, Hash $attrs| {
+    firewalld_zone { $key:
+      *       => $attrs,
     }
+  }
 
-    #...zones
-    $zones.each | String $key, Hash $attrs| {
-      firewalld_zone { $key:
-        *       => $attrs,
-      }
-    }
+  #...services
+  Firewalld_service {
+    zone      => $default_service_zone,
+  }
 
-    #...services
-    Firewalld_service {
-      zone      => $default_service_zone,
+  $services.each | String $key, Hash $attrs| {
+    firewalld_service { $key:
+      *       => $attrs,
     }
+  }
 
-    $services.each | String $key, Hash $attrs| {
-      firewalld_service { $key:
-        *       => $attrs,
-      }
+  #...rich rules
+  $rich_rules.each | String $key, Hash $attrs| {
+    firewalld_rich_rule { $key:
+      *       => $attrs,
     }
+  }
 
-    #...rich rules
-    $rich_rules.each | String $key, Hash $attrs| {
-      firewalld_rich_rule { $key:
-        *       => $attrs,
-      }
+  #...custom services
+  $custom_services.each | String $key, Hash $attrs| {
+    firewalld::custom_service { $key:
+      *       => $attrs,
     }
+  }
 
-    #...custom services
-    $custom_services.each | String $key, Hash $attrs| {
-      firewalld::custom_service { $key:
-        *       => $attrs,
-      }
+  #...ipsets
+  $ipsets.each | String $key, Hash $attrs| {
+    firewalld_ipset { $key:
+      *       => $attrs,
     }
+  }
 
-    #...ipsets
-    $ipsets.each | String $key, Hash $attrs| {
-      firewalld_ipset { $key:
-        *       => $attrs,
-      }
+  # Direct rules, chains and passthroughs
+  $direct_chains.each | String $key, Hash $attrs| {
+    firewalld_direct_chain { $key:
+      *       => $attrs,
     }
+  }
 
-    # Direct rules, chains and passthroughs
-    $direct_chains.each | String $key, Hash $attrs| {
-      firewalld_direct_chain { $key:
-        *       => $attrs,
-      }
+  $direct_rules.each | String $key, Hash $attrs| {
+    firewalld_direct_rule { $key:
+      *       => $attrs,
     }
+  }
 
-    $direct_rules.each | String $key, Hash $attrs| {
-      firewalld_direct_rule { $key:
-        *       => $attrs,
-      }
+  $direct_passthroughs.each | String $key, Hash $attrs| {
+    firewalld_direct_passthrough { $key:
+      *       => $attrs,
     }
+  }
 
-    $direct_passthroughs.each | String $key, Hash $attrs| {
-      firewalld_direct_passthrough { $key:
-        *       => $attrs,
-      }
-    }
+  Firewalld_direct_purge {
+    notify => Class['firewalld::reload'],
+  }
 
-    Firewalld_direct_purge {
-      notify => Exec['firewalld::reload'],
-    }
+  if $purge_direct_chains {
+    firewalld_direct_purge { 'chain': }
+  }
+  if $purge_direct_rules {
+    firewalld_direct_purge { 'rule': }
+  }
+  if $purge_direct_passthroughs {
+    firewalld_direct_purge { 'passthrough': }
+  }
 
-    if $purge_direct_chains {
-      firewalld_direct_purge { 'chain': }
-    }
-    if $purge_direct_rules {
-      firewalld_direct_purge { 'rule': }
-    }
-    if $purge_direct_passthroughs {
-      firewalld_direct_purge { 'passthrough': }
+  if $default_zone {
+    exec { 'firewalld::set_default_zone':
+      command => "firewall-cmd --set-default-zone ${default_zone}",
+      unless  => "[ $(firewall-cmd --get-default-zone) = ${default_zone} ]",
+      require => Service['firewalld'],
     }
 
-    if $default_zone {
-      exec { 'firewalld::set_default_zone':
-        command => "firewall-cmd --set-default-zone ${default_zone}",
-        unless  => "[ $(firewall-cmd --get-default-zone) = ${default_zone} ]",
-        require => Exec['firewalld::reload'],
-      }
-    }
+    Firewalld_zone <||> -> Exec['firewalld::set_default_zone']
+  }
 
-    if $log_denied {
-      exec { 'firewalld::set_log_denied':
-        command => "firewall-cmd --set-log-denied ${log_denied} && firewall-cmd --reload",
-        unless  => "[ $(firewall-cmd --get-log-denied) = ${log_denied} ]",
-      }
+  if $log_denied {
+    exec { 'firewalld::set_log_denied':
+      command => "firewall-cmd --set-log-denied ${log_denied}",
+      unless  => "[ $(firewall-cmd --get-log-denied) = ${log_denied} ]",
+      require => Service['firewalld'],
     }
+  }
 
-    Augeas {
-      lens    => 'Shellvars.lns',
-      incl    => '/etc/firewalld/firewalld.conf',
-      notify => Exec['firewalld::reload'],
-    }
+  Augeas {
+    lens    => 'Shellvars.lns',
+    incl    => '/etc/firewalld/firewalld.conf',
+    notify => [
+      Class['firewalld::reload'],
+      Service['firewalld'],
+    ],
+  }
 
-    if $cleanup_on_exit {
-      augeas {
-        'firewalld::cleanup_on_exit':
-          changes => [
-            "set CleanupOnExit \"${cleanup_on_exit}\"",
-          ];
-      }
+  if $cleanup_on_exit {
+    augeas {
+      'firewalld::cleanup_on_exit':
+        changes => [
+          "set CleanupOnExit \"${cleanup_on_exit}\"",
+        ];
     }
+  }
 
-    if $minimal_mark {
-      augeas {
-        'firewalld::minimal_mark':
-          changes => [
-            "set MinimalMark \"${minimal_mark}\"",
-          ];
-      }
+  if $minimal_mark {
+    augeas {
+      'firewalld::minimal_mark':
+        changes => [
+          "set MinimalMark \"${minimal_mark}\"",
+        ];
     }
+  }
 
-    if $lockdown {
-      augeas {
-        'firewalld::lockdown':
-          changes => [
-            "set Lockdown \"${lockdown}\"",
-          ];
-      }
+  if $lockdown {
+    augeas {
+      'firewalld::lockdown':
+        changes => [
+          "set Lockdown \"${lockdown}\"",
+        ];
     }
+  }
 
-    if $ipv6_rpfilter {
-      augeas {
-        'firewalld::ipv6_rpfilter':
-          changes => [
-            "set IPv6_rpfilter \"${ipv6_rpfilter}\"",
-          ];
-      }
+  if $ipv6_rpfilter {
+    augeas {
+      'firewalld::ipv6_rpfilter':
+        changes => [
+          "set IPv6_rpfilter \"${ipv6_rpfilter}\"",
+        ];
     }
+  }
 
-    if $facts['firewalld_version'] and
-      (versioncmp($facts['firewalld_version'], '0.6.0') >= 0) and
-      $firewall_backend
-    {
-      augeas {
-        'firewalld::firewall_backend':
-          changes => [
-            "set FirewallBackend \"${firewall_backend}\"",
-          ];
-      }
+  if $facts['firewalld_version'] and
+    (versioncmp($facts['firewalld_version'], '0.6.0') >= 0) and
+    $firewall_backend
+  {
+    augeas {
+      'firewalld::firewall_backend':
+        changes => [
+          "set FirewallBackend \"${firewall_backend}\"",
+        ];
     }
+  }
+
+  # Custom service files have to be in place before the zone is triggered
+  # otherwise you may end up with the zone being unable to find declared
+  # services.
+  Firewalld::Custom_service <||> -> Firewalld_zone <||>
 
-    # Set dependencies using resource chaining so that resource declarations made
-    # outside of this class (eg: from the profile) also get their dependencies set
-    # automatically, this addresses various issues found in
-    # https://github.com/crayfishx/puppet-firewalld/issues/38
-    #
-    Service['firewalld'] -> Firewalld_zone <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_rich_rule <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_service <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_port <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_ipset <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_direct_chain <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_direct_rule <||> ~> Exec['firewalld::reload']
-    Service['firewalld'] -> Firewalld_direct_passthrough <||> ~> Exec['firewalld::reload']
+  # TODO: Replace these with a native firewalld_reload type and automatically
+  # hook the types together properly.
+  Firewalld_direct_chain <||> ~> Class['firewalld::reload']
+  Firewalld_direct_passthrough <||> ~> Class['firewalld::reload']
+  Firewalld_direct_purge <||> ~> Class['firewalld::reload']
+  Firewalld_direct_rule <||> ~> Class['firewalld::reload']
+  Firewalld_ipset <||> ~> Class['firewalld::reload']
+  Firewalld_port <||> ~> Class['firewalld::reload']
+  Firewalld_rich_rule <||> ~> Class['firewalld::reload']
+  Firewalld_service <||> ~> Class['firewalld::reload']
 
-    if $purge_unknown_ipsets {
-      Firewalld_ipset <||>
-      ~> resources { 'firewalld_ipset':
-        purge => true,
-      }
+  if $purge_unknown_ipsets {
+    Firewalld_ipset <||>
+    ~> resources { 'firewalld_ipset':
+      purge => true,
     }
+  }
 }
diff --git a/manifests/reload.pp b/manifests/reload.pp
new file mode 100644
index 00000000..b4b2b754
--- /dev/null
+++ b/manifests/reload.pp
@@ -0,0 +1,12 @@
+# @summary A common point for triggering an intermediary firewalld reload using firewall-cmd
+#
+class firewalld::reload {
+  assert_private()
+
+  exec{ 'firewalld::reload':
+    path        =>'/usr/bin:/bin',
+    command     => 'firewall-cmd --reload',
+    onlyif      => 'firewall-cmd --state',
+    refreshonly => true,
+  }
+}
diff --git a/manifests/reload/complete.pp b/manifests/reload/complete.pp
new file mode 100644
index 00000000..d1183d0f
--- /dev/null
+++ b/manifests/reload/complete.pp
@@ -0,0 +1,14 @@
+# @summary A common point for triggering an intermediary firewalld full reload using firewall-cmd
+#
+class firewalld::reload::complete {
+  assert_private()
+
+  include firewalld::reload
+
+  exec{ 'firewalld::complete-reload':
+    path        => '/usr/bin:/bin',
+    command     => 'firewall-cmd --complete-reload',
+    refreshonly => true,
+    require     => Class['firewalld::reload'],
+  }
+}
diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb
index e3647d1f..4fb4ba7a 100644
--- a/spec/classes/init_spec.rb
+++ b/spec/classes/init_spec.rb
@@ -14,6 +14,19 @@
 
   context 'with defaults for all parameters' do
     it { is_expected.to contain_class('firewalld') }
+
+    it {
+      is_expected.to contain_package('firewalld').
+        with_ensure('installed').
+        that_notifies('Service[firewalld]')
+    }
+
+    it {
+      is_expected.to contain_service('firewalld').
+        with_ensure('running').
+        with_enable(true)
+    }
+
     it { is_expected.not_to contain_augeas('firewalld::firewallbackend') }
   end
 
@@ -28,7 +41,7 @@
       is_expected.to contain_exec('firewalld::set_default_zone').with(
         command: 'firewall-cmd --set-default-zone restricted',
         unless: '[ $(firewall-cmd --get-default-zone) = restricted ]'
-      ).that_requires('Exec[firewalld::reload]')
+      ).that_requires('Service[firewalld]')
     end
   end
 
@@ -83,7 +96,7 @@
         with_port('9999').
         with_protocol('tcp').
         with_zone('public').
-        that_notifies('Exec[firewalld::reload]').
+        that_notifies('Class[firewalld::reload]').
         that_requires('Service[firewalld]')
     end
   end
@@ -106,7 +119,6 @@
       is_expected.to contain_firewalld_zone('restricted').
         with_ensure('present').
         with_target('%%REJECT%%').
-        that_notifies('Exec[firewalld::reload]').
         that_requires('Service[firewalld]')
     end
   end
@@ -129,7 +141,7 @@
       is_expected.to contain_firewalld_service('mysql').
         with_ensure('present').
         with_zone('public').
-        that_notifies('Exec[firewalld::reload]').
+        that_notifies('Class[firewalld::reload]').
         that_requires('Service[firewalld]')
     end
   end
@@ -155,7 +167,7 @@
       is_expected.to contain_firewalld_rich_rule('Accept SSH from Gondor').
         with_ensure('present').
         with_zone('restricted').
-        that_notifies('Exec[firewalld::reload]').
+        that_notifies('Class[firewalld::reload]').
         that_requires('Service[firewalld]')
     end
   end
@@ -198,7 +210,7 @@
       is_expected.to contain_exec('firewalld::set_default_zone').with(
         command: 'firewall-cmd --set-default-zone public',
         unless: '[ $(firewall-cmd --get-default-zone) = public ]'
-      ).that_requires('Exec[firewalld::reload]')
+      ).that_requires('Service[firewalld]')
     end
   end
 
@@ -212,9 +224,9 @@
 
       it do
         is_expected.to contain_exec('firewalld::set_log_denied').with(
-          command: "firewall-cmd --set-log-denied #{cond} && firewall-cmd --reload",
+          command: "firewall-cmd --set-log-denied #{cond}",
           unless: "[ \$\(firewall-cmd --get-log-denied) = #{cond} ]"
-        )
+        ).that_requires('Service[firewalld]')
       end
     end
   end
diff --git a/spec/classes/reload/complete_spec.rb b/spec/classes/reload/complete_spec.rb
new file mode 100644
index 00000000..d66f718c
--- /dev/null
+++ b/spec/classes/reload/complete_spec.rb
@@ -0,0 +1,13 @@
+require 'spec_helper'
+require 'puppet/provider/firewalld'
+
+describe 'firewalld::reload::complete' do
+  let(:pre_condition) { 'function assert_private{}' }
+
+  it {
+    is_expected.to contain_exec('firewalld::complete-reload').with(path: '/usr/bin:/bin',
+                                                                   command: 'firewall-cmd --complete-reload',
+                                                                   refreshonly: true,
+                                                                   require: 'Class[Firewalld::Reload]')
+  }
+end
diff --git a/spec/classes/reload_spec.rb b/spec/classes/reload_spec.rb
new file mode 100644
index 00000000..bfba7392
--- /dev/null
+++ b/spec/classes/reload_spec.rb
@@ -0,0 +1,13 @@
+require 'spec_helper'
+require 'puppet/provider/firewalld'
+
+describe 'firewalld::reload' do
+  let(:pre_condition) { 'function assert_private{}' }
+
+  it {
+    is_expected.to contain_exec('firewalld::reload').with(path: '/usr/bin:/bin',
+                                                          command: 'firewall-cmd --reload',
+                                                          onlyif: 'firewall-cmd --state',
+                                                          refreshonly: true)
+  }
+end
diff --git a/spec/functions/safe_filename_spec.rb b/spec/functions/safe_filename_spec.rb
new file mode 100644
index 00000000..c90e696d
--- /dev/null
+++ b/spec/functions/safe_filename_spec.rb
@@ -0,0 +1,56 @@
+require 'spec_helper'
+
+describe 'firewalld::safe_filename' do
+  let(:valid_filenames) do
+    {
+      'ThisShouldWork' => 'ThisShouldWork',
+      'this_should_work' => 'this_should_work',
+      'th1s_Sh0uld_w0rk' => 'th1s_Sh0uld_w0rk'
+    }
+  end
+
+  let(:invalid_filenames) do
+    {
+      'This Should Work' => 'This_Should_Work',
+      'this should work'   => 'this_should_work',
+      'th1s$Sh0uld w0rk!!' => 'th1s_Sh0uld_w0rk__'
+    }
+  end
+
+  let(:filenames_with_options) do
+    {
+      'ThisShouldWork.test' => 'ThisShouldWork.test',
+      'this_should_!work.xml'   => 'this_should_--work--xml',
+      'th1s$Sh0uld w0rk!!.test' => 'th1s--Sh0uld--w0rk----.test'
+    }
+  end
+
+  it 'runs with valid filenames' do
+    valid_filenames.each do |k, v|
+      is_expected.to run.with_params(k).and_return(v)
+    end
+  end
+
+  it 'translates invalid filenames' do
+    invalid_filenames.each do |k, v|
+      is_expected.to run.with_params(k).and_return(v)
+    end
+  end
+
+  it 'translates with options' do
+    filenames_with_options.each do |k, v|
+      opts = {
+        'replacement_string' => '--',
+        'file_extension'     => '.test'
+      }
+
+      is_expected.to run.with_params(k, opts).and_return(v)
+    end
+  end
+
+  it 'raises an error if provided with an invalid replacement_string' do
+    expect{
+      is_expected.to run.with_params('test', { 'replacement_string' => '@' })
+    }.to raise_error(/expects a match for.+got '@'/)
+  end
+end
diff --git a/templates/service.xml.epp b/templates/service.xml.epp
new file mode 100644
index 00000000..15ba249c
--- /dev/null
+++ b/templates/service.xml.epp
@@ -0,0 +1,40 @@
+<%- |
+  String                  $short,
+  Optional[String]        $description,
+  Optional[Array[Hash]]   $port,
+  Optional[Array[String]] $module,
+  Optional[Hash]          $destination,
+  String                  $filename     = $short,
+  Stdlib::Unixpath        $config_dir,
+  String                  $ensure
+| -%>
+<?xml version="1.0" encoding="utf-8"?>
+<service>
+    <short><%= $short %></short>
+    <%- if $description { -%>
+    <description><%= $description %></description>
+    <%- } -%>
+
+<%
+  if $port {
+    $port.each |$i| {
+      if $i['port'] and $i['port'] != '' {
+-%>
+    <port<% if $i['protocol'] { %> protocol="<%= $i['protocol'] %>"<% } %> port="<%= regsubst("${i['port']}", ':', '-', 'G') %>" />
+<%    } else { -%>
+    <port protocol="<%= $i['protocol'] %>" port="" />
+<%    } -%>
+<%  } -%>
+
+<% } -%>
+<% if $module {
+     sort($module).each |$j| {
+-%>
+    <module name="<%= $j %>" />
+<%   } -%>
+
+<% } -%>
+<% if $destination { -%>
+    <destination<% if $destination['ipv4'] { %> ipv4="<%= $destination['ipv4'] %>"<% } %><% if $destination['ipv6'] { %> ipv6="<%= $destination['ipv6'] %>"<% } %> />
+<% } -%>
+</service>
diff --git a/templates/service.xml.erb b/templates/service.xml.erb
deleted file mode 100644
index 431b29da..00000000
--- a/templates/service.xml.erb
+++ /dev/null
@@ -1,28 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>
-<service>
-    <short><%= @short %></short>
-    <%- if @description -%>
-    <description><%= @description %></description>
-    <%- end -%>
-
-    <%- if @port -%>
-    <%- @port.each do |i| -%>
-      <%- if i['port'] and i['port'] != '' -%>
-        <%- p = sprintf('%s',i['port']).sub(/:/, '-') -%>
-    <port<% if i['protocol'] -%> protocol="<%= i['protocol'] %>"<% end -%> port="<%= p %>" />
-      <%- else -%>
-    <port protocol="<%= i['protocol'] %>" port="" />
-      <%- end -%>
-    <%- end -%>
-
-    <%- end -%>
-    <%- if @module -%>
-    <%- @module.sort.each do |j| -%>
-    <module name="<%= j %>" />
-    <%- end -%>
-
-    <%- end -%>
-    <%- if @destination -%>
-    <destination<% if @destination['ipv4'] -%> ipv4="<%= @destination['ipv4'] %>"<% end -%><% if @destination['ipv6'] -%> ipv6="<%= @destination['ipv6'] %>"<% end -%> />
-    <%- end -%>
-</service>