(IAC-1186) Add new $use_servername_for_filenames parameter

Prior to this commit, the `$filename` variable, which is used to
construct the filename of Apache's various config and log files,
obtained it's default value from the `$name` parameter.

As #2064 highlights, it is possible for `$name` to contain spaces
in it and this can cause cumbersome log file names, albeit POSIX
compliant.

Also related is #2068, which changes the `$filename` variable source
from `$name` to `$servername`. This arguably seems more appropriate,
especially given that `$servername` defaults to `$name` if undefined.

This commit attempts to create a satisfactory solution to both #2064
and #2068 by introducing the `$use_servername_for_filenames` param.
When set to `true`, a sanitized `$servername` parameter value will be
used to construct `$filename`.

When undefined or set to `$false`, it will retain the existing
behaviour and use the `$name` parameter value.

This will default to `false` until the next major release (v6.0.0),
after which it will default to `true`. Then, in the subsequent major
release (v7.0.0) it will be deprecated altogether and the default
behaviour will be to use the sanitized value of `$servername` for the
`$filename` var.

Warning messages are output to the console to alert users of this
change in behaviour.

Author: Ciaran McCrisken

manifests/vhost.pp

@@ -1717,6 +1717,12 @@
 #   client request exceeds that limit, the server will return an error response
 #   instead of servicing the request.
 #
+# @param $use_servername_for_filenames
+#   When set to true, default log / config file names will be derived from the sanitized
+#   value of the $servername parameter.
+#   When set to false (default), the existing behaviour of using the $name parameter
+#   will remain.
+
 define apache::vhost (
   Variant[Boolean,String] $docroot,
   $manage_docroot                                                                   = true,
@@ -1782,6 +1788,7 @@
   $access_log_format                                                                = false,
   $access_log_env_var                                                               = false,
   Optional[Array] $access_logs                                                      = undef,
+  Optional[Boolean] $use_servername_for_filenames                                   = false,
   $aliases                                                                          = undef,
   Optional[Variant[Hash, Array[Variant[Array,Hash]]]] $directories                  = undef,
   Boolean $error_log                                                                = true,
@@ -2036,8 +2043,39 @@
     $priority_real = '25-'
   }
 
-  ## Apache include does not always work with spaces in the filename
-  $filename = regsubst($name, ' ', '_', 'G')
+  # IAC-1186: A number of configuration and log file names are generated using the $name parameter. It is possile for
+  # the $name parameter to contain spaces, which could then be transferred to the log / config filenames. Although
+  # POSIX compliant, this can be cumbersome.
+  #
+  # It seems more appropriate to use the $servername parameter to derive default log / config filenames from. We should
+  # also perform some sanitiation on the $servername parameter to strip spaces from it, as it defaults to the value of
+  # $name, should $servername NOT be defined.
+  #
+  # We will retain the default behaviour for filenames but allow the use of a sanitized version of $servername to be
+  # used, using the new $use_servername_for_filenames parameter.
+  #
+  # This will default to false until the next major release (v6.0.0), at which point, we will default this to true and
+  # warn about it's imminent deprecation in the subsequent major release (v7.0.0)
+  #
+  # In v7.0.0, we will deprecate the $use_servername_for_filenames parameter altogether and use the sanitized value of
+  # $servername for default log / config filenames.
+  $filename = $use_servername_for_filenames ? {
+    true => regsubst($servername, ' ', '_', 'G'),
+    false => $name,
+  }
+
+  if ! $use_servername_for_filenames {
+    $use_servername_for_filenames_warn_msg = '
+    It is possible for the $name parameter to be defined with spaces in it. Although supported on POSIX systems, this
+    can lead to cumbersome file names. The $servername attribute has stricter conditions from Apache (i.e. no spaces)
+    When $use_servername_for_filenames = true, the $servername parameter, sanitized, is used to construct log and config
+    file names.
+
+    From version v6.0.0 of the puppetlabs-apache module, this parameter will default to true. From version v7.0.0 of the
+    module, the $use_servername_for_filenames will be removed and log/config file names will be dervied from the
+    sanitized $servername parameter when not explicitly defined.'
+    warning($use_servername_for_filenames_warn_msg)
+  }
 
   # This ensures that the docroot exists
   # But enables it to be specified across multiple vhost resources
@@ -2096,9 +2134,9 @@
     $error_log_destination = $error_log_syslog
   } else {
     if $ssl {
-      $error_log_destination = "${logroot}/${name}_error_ssl.log"
+      $error_log_destination = "${logroot}/${filename}_error_ssl.log"
     } else {
-      $error_log_destination = "${logroot}/${name}_error.log"
+      $error_log_destination = "${logroot}/${filename}_error.log"
     }
   }
 
@@ -2117,9 +2155,9 @@
     $modsec_audit_log_destination = $modsec_audit_log_pipe
   } elsif $modsec_audit_log {
     if $ssl {
-      $modsec_audit_log_destination = "${logroot}/${name}_security_ssl.log"
+      $modsec_audit_log_destination = "${logroot}/${filename}_security_ssl.log"
     } else {
-      $modsec_audit_log_destination = "${logroot}/${name}_security.log"
+      $modsec_audit_log_destination = "${logroot}/${filename}_security.log"
     }
   } else {
     $modsec_audit_log_destination = undef

spec/acceptance/vhost_spec.rb

@@ -948,6 +948,58 @@ class { 'apache': }
     end
   end
 
+  context 'when a manifest defines $servername' do
+    describe 'when the $use_servername_for_filenames parameter is set to true' do
+      pp = <<-MANIFEST
+          class { 'apache': }
+          host { 'test.server': ip => '127.0.0.1' }
+          apache::vhost { 'test.server':
+            use_servername_for_filenames  => true,
+            servername                    => 'test.servername',
+            docroot                       => '/tmp',
+            logroot                       => '/tmp',
+          }
+      MANIFEST
+      it 'applies cleanly and NOT print warning about $use_servername_for_filenames usage for test.server vhost' do
+        result = apply_manifest(pp, catch_failures: true)
+        expect(result.stderr).not_to contain %r{
+          .*Warning\:\sScope\(Apache::Vhost\[test\.server\]\)\:.*
+          It\sis\spossible\sfor\sthe\s\$name\sparameter.*
+          sanitized\s\$servername\sparameter\swhen\snot\sexplicitly\sdefined\.
+        }xm
+      end
+      describe file("#{apache_hash['vhost_dir']}/25-test.servername.conf") do
+        it { is_expected.to be_file }
+        it { is_expected.to contain '  ErrorLog "/tmp/test.servername_error.log' }
+        it { is_expected.to contain '  CustomLog "/tmp/test.servername_access.log' }
+      end
+    end
+    describe 'when the $use_servername_for_filenames parameter is NOT defined' do
+      pp = <<-MANIFEST
+          class { 'apache': }
+          host { 'test.server': ip => '127.0.0.1' }
+          apache::vhost { 'test.server':
+            servername                    => 'test.servername',
+            docroot                       => '/tmp',
+            logroot                       => '/tmp',
+          }
+      MANIFEST
+      it 'applies cleanly and print warning about $use_servername_for_filenames usage for test.server vhost' do
+        result = apply_manifest(pp, catch_failures: true)
+        expect(result.stderr).to contain %r{
+          .*Warning\:\sScope\(Apache::Vhost\[test\.server\]\)\:.*
+          It\sis\spossible\sfor\sthe\s\$name\sparameter.*
+          sanitized\s\$servername\sparameter\swhen\snot\sexplicitly\sdefined\.
+        }xm
+      end
+      describe file("#{apache_hash['vhost_dir']}/25-test.server.conf") do
+        it { is_expected.to be_file }
+        it { is_expected.to contain '  ErrorLog "/tmp/test.server_error.log' }
+        it { is_expected.to contain '  CustomLog "/tmp/test.server_access.log' }
+      end
+    end
+  end
+
   ['access', 'error'].each do |logtype|
     case logtype
     when 'access'

templates/vhost/_access_log.erb

@@ -14,8 +14,8 @@
 <%   elsif log['pipe'] -%>
 <%     destination = log['pipe'] -%>
 <%   else -%>
-<%     destination ||= "#{@logroot}/#{@name}_access_ssl.log" if @ssl -%>
-<%     destination ||= "#{@logroot}/#{@name}_access.log" -%>
+<%     destination ||= "#{@logroot}/#{@filename}_access_ssl.log" if @ssl -%>
+<%     destination ||= "#{@logroot}/#{@filename}_access.log" -%>
 <%   end -%>
   CustomLog "<%= destination %>" <%= format %> <%= env %>
 <% end -%>