Merge pull request #2122 from smortex/use_port_for_filenames

(IAC-1186) Add $use_port_for_filenames parameter

Author: sanfrancrisko

manifests/init.pp

@@ -837,6 +837,7 @@
       logroot_mode                 => $logroot_mode,
       manage_docroot               => $default_vhost,
       use_servername_for_filenames => true,
+      use_port_for_filenames       => true,
     }
     $ssl_access_log_file = $::osfamily ? {
       'freebsd' => $access_log_file,
@@ -855,6 +856,7 @@
       logroot_mode                 => $logroot_mode,
       manage_docroot               => $default_ssl_vhost,
       use_servername_for_filenames => true,
+      use_port_for_filenames       => true,
     }
   }
 

manifests/vhost.pp

@@ -1729,6 +1729,13 @@
 #   When set to false (default), the existing behaviour of using the $name parameter
 #   will remain.
 #
+# @param $use_port_for_filenames
+#   When set to true and use_servername_for_filenames is also set to true, default log /
+#   config file names will be derived from the sanitized value of both the $servername and
+#   $port parameters.
+#   When set to false (default), the port is not included in the file names and may lead to
+#   duplicate declarations if two virtual hosts use the same domain.
+#
 # @param $mdomain
 #   All the names in the list are managed as one Managed Domain (MD). mod_md will request
 #   one single certificate that is valid for all these names.
@@ -1800,6 +1807,7 @@
   $access_log_env_var                                                               = false,
   Optional[Array] $access_logs                                                      = undef,
   Optional[Boolean] $use_servername_for_filenames                                   = false,
+  Optional[Boolean] $use_port_for_filenames                                         = false,
   $aliases                                                                          = undef,
   Optional[Variant[Hash, Array[Variant[Array,Hash]]]] $directories                  = undef,
   Boolean $error_log                                                                = true,
@@ -2064,16 +2072,22 @@
   # 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.
   #
+  # Because a single hostname may be use by multiple virtual hosts listening on different ports, the $port paramter can
+  # optionaly be used to avoid duplicate resources.
+  #
   # 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.
+  # used, using the new $use_servername_for_filenames and $use_port_for_filenames parameters.
   #
   # 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.
+  # In v7.0.0, we will deprecate the $use_servername_for_filenames and $use_port_for_filenames parameters altogether
+  # and use the sanitized value of $servername for default log / config filenames.
   $filename = $use_servername_for_filenames ? {
-    true => regsubst($servername, ' ', '_', 'G'),
+    true => $use_port_for_filenames ? {
+      true  => regsubst("${servername}-${port}", ' ', '_', 'G'),
+      false => regsubst($servername, ' ', '_', 'G'),
+    },
     false => $name,
   }
 
@@ -2088,6 +2102,17 @@
     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)
+  } elsif ! $use_port_for_filenames {
+    $use_port_for_filenames_warn_msg = '
+    It is possible for multiple virtual hosts to be configured using the same $servername but a different port. When
+    using $use_servername_for_filenames, this can lead to duplicate resource declarations.
+    When $use_port_for_filenames = true, the $servername and $port parameters, sanitized, are 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_port_for_filenames will be removed and log/config file names will be dervied from the
+    sanitized $servername parameter when not explicitly defined.'
+    warning($use_port_for_filenames_warn_msg)
   }
 
   # This ensures that the docroot exists

manifests/vhost/fragment.pp

@@ -55,6 +55,7 @@
 #
 define apache::vhost::fragment (
   String[1] $vhost,
+  Optional[Integer[0]] $port = undef,
   $priority = undef,
   Optional[String] $content = undef,
   Integer[0] $order = 900,
@@ -68,7 +69,10 @@
     $priority_real = '25-'
   }
 
-  $filename = regsubst($vhost, ' ', '_', 'G')
+  $filename = $port ? {
+    Integer => regsubst("${vhost}-${port}", ' ', '_', 'G'),
+    Undef   => regsubst($vhost, ' ', '_', 'G'),
+  }
 
   if $content =~ String[1] {
     concat::fragment { "${vhost}-${title}":

spec/acceptance/apache_ssl_spec.rb

@@ -30,7 +30,7 @@ class { 'apache':
       end
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default-ssl.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-ssl-443.conf") do
       it { is_expected.to be_file }
       it { is_expected.to contain 'SSLCertificateFile      "/tmp/ssl_cert"' }
       it { is_expected.to contain 'SSLCertificateKeyFile   "/tmp/ssl_key"' }

spec/acceptance/vhost_spec.rb

@@ -25,11 +25,11 @@ class { 'apache':
       apply_manifest(pp, catch_failures: true)
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-80.conf") do
       it { is_expected.not_to be_file }
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default-ssl.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-ssl-443.conf") do
       it { is_expected.not_to be_file }
     end
   end
@@ -42,11 +42,11 @@ class { 'apache': }
       apply_manifest(pp, catch_failures: true)
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-80.conf") do
       it { is_expected.to contain '<VirtualHost \*:80>' }
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default-ssl.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-ssl-443.conf") do
       it { is_expected.not_to be_file }
     end
   end
@@ -67,11 +67,11 @@ class { 'apache':
       apply_manifest(pp, catch_failures: true)
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-80.conf") do
       it { is_expected.to contain '<VirtualHost \*:80>' }
     end
 
-    describe file("#{apache_hash['vhost_dir']}/15-default-ssl.conf") do
+    describe file("#{apache_hash['vhost_dir']}/15-default-ssl-443.conf") do
       it { is_expected.to contain '<VirtualHost \*:443>' }
       it { is_expected.to contain 'SSLEngine on' }
     end

spec/defines/vhost_fragment_spec.rb

@@ -14,6 +14,7 @@
         let(:params) do
           {
             vhost: 'default',
+            port: 80,
             priority: '15',
           }
         end
@@ -23,9 +24,9 @@
 
           it 'creates a vhost concat fragment' do
             is_expected.to compile.with_all_deps
-            is_expected.to contain_concat('15-default.conf')
+            is_expected.to contain_concat('15-default-80.conf')
             is_expected.to create_concat__fragment('default-myfragment')
-              .with_target('15-default.conf')
+              .with_target('15-default-80.conf')
               .with_order(900)
               .with_content('# Foo')
           end
@@ -36,7 +37,7 @@
 
           it 'does not create a vhost concat fragment' do
             is_expected.to compile.with_all_deps
-            is_expected.to contain_concat('15-default.conf')
+            is_expected.to contain_concat('15-default-80.conf')
             is_expected.not_to contain_concat__fragment('default-myfragment')
           end
         end

spec/spec_helper_acceptance_local.rb

@@ -89,7 +89,7 @@ def apache_settings_hash
     apache['conf_file']        = '/etc/httpd/conf/httpd.conf'
     apache['ports_file']       = '/etc/httpd/conf/ports.conf'
     apache['vhost_dir']        = '/etc/httpd/conf.d'
-    apache['vhost']            = '/etc/httpd/conf.d/15-default.conf'
+    apache['vhost']            = '/etc/httpd/conf.d/15-default-80.conf'
     apache['run_dir']          = '/var/run/httpd'
     apache['doc_root']         = '/var/www'
     apache['service_name']     = 'httpd'
@@ -119,7 +119,7 @@ def apache_settings_hash
     apache['mod_dir']          = '/etc/apache2/mods-available'
     apache['conf_file']        = '/etc/apache2/apache2.conf'
     apache['ports_file']       = '/etc/apache2/ports.conf'
-    apache['vhost']            = '/etc/apache2/sites-available/15-default.conf'
+    apache['vhost']            = '/etc/apache2/sites-available/15-default-80.conf'
     apache['vhost_dir']        = '/etc/apache2/sites-enabled'
     apache['run_dir']          = '/var/run/apache2'
     apache['doc_root']         = '/var/www'
@@ -141,7 +141,7 @@ def apache_settings_hash
     apache['mod_dir']          = '/usr/local/etc/apache24/Modules'
     apache['conf_file']        = '/usr/local/etc/apache24/httpd.conf'
     apache['ports_file']       = '/usr/local/etc/apache24/Includes/ports.conf'
-    apache['vhost']            = '/usr/local/etc/apache24/Vhosts/15-default.conf'
+    apache['vhost']            = '/usr/local/etc/apache24/Vhosts/15-default-80.conf'
     apache['vhost_dir']        = '/usr/local/etc/apache24/Vhosts'
     apache['run_dir']          = '/var/run/apache24'
     apache['doc_root']         = '/var/www'
@@ -155,7 +155,7 @@ def apache_settings_hash
     apache['mod_dir']          = '/etc/apache2/modules.d'
     apache['conf_file']        = '/etc/apache2/httpd.conf'
     apache['ports_file']       = '/etc/apache2/ports.conf'
-    apache['vhost']            = '/etc/apache2/vhosts.d/15-default.conf'
+    apache['vhost']            = '/etc/apache2/vhosts.d/15-default-80.conf'
     apache['vhost_dir']        = '/etc/apache2/vhosts.d'
     apache['run_dir']          = '/var/run/apache2'
     apache['doc_root']         = '/var/www'
@@ -169,7 +169,7 @@ def apache_settings_hash
     apache['mod_dir']          = '/etc/apache2/mods-available'
     apache['conf_file']        = '/etc/apache2/httpd.conf'
     apache['ports_file']       = '/etc/apache2/ports.conf'
-    apache['vhost']            = '/etc/apache2/sites-available/15-default.conf'
+    apache['vhost']            = '/etc/apache2/sites-available/15-default-80.conf'
     apache['vhost_dir']        = '/etc/apache2/sites-available'
     apache['run_dir']          = '/var/run/apache2'
     apache['doc_root']         = '/srv/www'