Service filename bugfix

REFERENCE.md

@@ -5,7 +5,9 @@
 
 **Classes**
 
-* [`firewalld`](#firewalld): == Class: firewalld  Manage the firewalld service  See the README.md for usage instructions for the firewalld_zone and firewalld_rich_rule ty
+* [`firewalld`](#firewalld): Manage the firewalld service
+* [`firewalld::reload`](#firewalldreload): A common point for triggering an intermediary firewalld reload using firewall-cmd
+* [`firewalld::reload::complete`](#firewalldreloadcomplete): A common point for triggering an intermediary firewalld full reload using firewall-cmd
 
 **Defined types**
 
@@ -23,13 +25,13 @@
 * [`firewalld_service`](#firewalld_service): Assigns a service to a specific firewalld zone.
 * [`firewalld_zone`](#firewalld_zone): Creates and manages firewalld zones.
 
-## Classes
+**Functions**
 
-### firewalld
+* [`firewalld::safe_filename`](#firewalldsafe_filename): Returns a string that is safe for firewalld filenames
 
-== Class: firewalld
+## Classes
 
-Manage the firewalld service
+### firewalld
 
 See the README.md for usage instructions for the firewalld_zone and
 firewalld_rich_rule types
@@ -48,8 +50,6 @@ firewalld_rich_rule types
      install_gui => true,
    }
 
-
-
 === Authors
 
 Craig Dunn <craig@craigdunn.org>
@@ -294,6 +294,14 @@ Data type: `Optional[String]`
 
 Default value: `undef`
 
+### firewalld::reload
+
+A common point for triggering an intermediary firewalld reload using firewall-cmd
+
+### firewalld::reload::complete
+
+A common point for triggering an intermediary firewalld full reload using firewall-cmd
+
 ## Defined types
 
 ### firewalld::custom_service
@@ -982,3 +990,108 @@ Description of the zone to add
 
 Short description of the zone to add
 
+## Functions
+
+### firewalld::safe_filename
+
+Type: Puppet Language
+
+Returns a string that is safe for firewalld filenames
+
+#### Examples
+
+##### Regular Filename
+
+```puppet
+$filename = 'B@d Characters!'
+firewalld::safe_filename($orig_string)
+
+Result => 'B_d_Characters_'
+```
+
+##### Filename with Options
+
+```puppet
+$filename = 'B@d Characters!.txt'
+firewalld::safe_filename(
+  $filename,
+  {
+    'replacement_string' => '--',
+    'file_extension'     => '.txt'
+  }
+)
+
+Result => 'B--d--Characters--.txt'
+```
+
+#### `firewalld::safe_filename(String[1] $filename, Struct[
+    {
+      'replacement_string' => Pattern[/[\w-]/],
+      'file_extension'     => Optional[String[1]]
+    }
+  ] $options = { 'replacement_string' => '_'})`
+
+The firewalld::safe_filename function.
+
+Returns: `String` Processed string
+
+##### Examples
+
+###### Regular Filename
+
+```puppet
+$filename = 'B@d Characters!'
+firewalld::safe_filename($orig_string)
+
+Result => 'B_d_Characters_'
+```
+
+###### Filename with Options
+
+```puppet
+$filename = 'B@d Characters!.txt'
+firewalld::safe_filename(
+  $filename,
+  {
+    'replacement_string' => '--',
+    'file_extension'     => '.txt'
+  }
+)
+
+Result => 'B--d--Characters--.txt'
+```
+
+##### `filename`
+
+Data type: `String[1]`
+
+The String to process
+
+##### `options`
+
+Data type: `Struct[
+    {
+      'replacement_string' => Pattern[/[\w-]/],
+      'file_extension'     => Optional[String[1]]
+    }
+  ]`
+
+Various processing options
+
+Options:
+
+* **file_extension** `String[1]`: This will be stripped from the end of the string prior to processing and
+re-added afterwards
+
+##### `options`
+
+Data type: `String[1]`
+
+replacement_string
+The String to use when replacing invalid characters
+
+Options:
+
+* **file_extension** `String[1]`: This will be stripped from the end of the string prior to processing and
+re-added afterwards
+

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')
+  }
+}

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

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

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,
-  }
-
 }